diff mbox series

[v1,10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp

Message ID 20240208184919.2224986-11-andriy.shevchenko@linux.intel.com
State New
Headers
Series auxdisplay: linedisp: Clean up and add new driver |

Commit Message

Andy Shevchenko Feb. 8, 2024, 6:48 p.m. UTC 
  There is a driver that uses small buffer for the string, when we
add a new one, we may avoid duplication and use one provided by
the line display library. Allow user to skip buffer pointer when
registering a device.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/line-display.c | 4 ++--
 drivers/auxdisplay/line-display.h | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Robin van der Gracht Feb. 12, 2024, 8:25 a.m. UTC | #1 
Hi Andy,

Thank you for your patches.

See inline comment below.

On Thu,  8 Feb 2024 20:48:08 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> There is a driver that uses small buffer for the string, when we
> add a new one, we may avoid duplication and use one provided by
> the line display library. Allow user to skip buffer pointer when
> registering a device.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/auxdisplay/line-display.c | 4 ++--
>  drivers/auxdisplay/line-display.h | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
> index da47fc59f6cb..a3c706e1739d 100644
> --- a/drivers/auxdisplay/line-display.c
> +++ b/drivers/auxdisplay/line-display.c
> @@ -330,8 +330,8 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
>  	linedisp->dev.parent = parent;
>  	linedisp->dev.type = &linedisp_type;
>  	linedisp->ops = ops;
> -	linedisp->buf = buf;
> -	linedisp->num_chars = num_chars;
> +	linedisp->buf = buf ? buf : linedisp->curr;
> +	linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);

It's not a big buffer, but now it's always there even if it's not used.
And even if it's used, it might be only partially used.
Why not used a malloc instead?

>  	linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
>  
>  	err = ida_alloc(&linedisp_id, GFP_KERNEL);
> diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
> index 65d782111f53..4c354b8f376e 100644
> --- a/drivers/auxdisplay/line-display.h
> +++ b/drivers/auxdisplay/line-display.h
> @@ -54,12 +54,15 @@ struct linedisp_ops {
>  	void (*update)(struct linedisp *linedisp);
>  };
>  
> +#define LINEDISP_DEFAULT_BUF_SZ		8u
> +
>  /**
>   * struct linedisp - character line display private data structure
>   * @dev: the line display device
>   * @id: instance id of this display
>   * @timer: timer used to implement scrolling
>   * @ops: character line display operations
> + * @curr: fallback buffer for the string
>   * @buf: pointer to the buffer for the string currently displayed
>   * @message: the full message to display or scroll on the display
>   * @num_chars: the number of characters that can be displayed
> @@ -73,6 +76,7 @@ struct linedisp {
>  	struct timer_list timer;
>  	const struct linedisp_ops *ops;
>  	struct linedisp_map *map;
> +	char curr[LINEDISP_DEFAULT_BUF_SZ];
>  	char *buf;
>  	char *message;
>  	unsigned int num_chars;
Andy Shevchenko Feb. 12, 2024, 11:50 a.m. UTC | #2 
On Mon, Feb 12, 2024 at 09:25:00AM +0100, Robin van der Gracht wrote:
> On Thu,  8 Feb 2024 20:48:08 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> > +	linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
> 
> It's not a big buffer, but now it's always there even if it's not used.
> And even if it's used, it might be only partially used.
> Why not used a malloc instead?

malloc() infra takes more than this IIRC (something like up to 32 bytes on
64-bit platforms) or comparable sizes. Yes, the malloc() along with the
linedisp structure might make sense, but will require more invasive change.

Do you want me to drop this one from the set?
(I have no hard feelings about it, as I see better way and just having no
 time for taking care about, as it's not the main point of the series.)
Andy Shevchenko Feb. 12, 2024, 4:49 p.m. UTC | #3 
On Mon, Feb 12, 2024 at 01:50:13PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 09:25:00AM +0100, Robin van der Gracht wrote:
> > On Thu,  8 Feb 2024 20:48:08 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > > +	linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
> > 
> > It's not a big buffer, but now it's always there even if it's not used.
> > And even if it's used, it might be only partially used.
> > Why not used a malloc instead?
> 
> malloc() infra takes more than this IIRC (something like up to 32 bytes on
> 64-bit platforms) or comparable sizes. Yes, the malloc() along with the
> linedisp structure might make sense, but will require more invasive change.
> 
> Do you want me to drop this one from the set?
> (I have no hard feelings about it, as I see better way and just having no
>  time for taking care about, as it's not the main point of the series.)

Looking again into it, the allocation separately with linedisp structure
is indeed too much invasive. I prefer (as we save lines of code and deduplicate
the buffer at least for two drivers, including a new one) to leave this patch
for now. We may rework it later on. Do you agree?
Robin van der Gracht Feb. 13, 2024, 10:53 a.m. UTC | #4 
Hi Andy,

On Mon, 12 Feb 2024 18:49:49 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Looking again into it, the allocation separately with linedisp structure
> is indeed too much invasive. I prefer (as we save lines of code and deduplicate
> the buffer at least for two drivers, including a new one) to leave this patch
> for now. We may rework it later on. Do you agree?

Agreed. The additional overhead is probably not worth it. If the buffer size
needs to be increased later on we'll take another look.

Robin
diff mbox series

Patch

diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index da47fc59f6cb..a3c706e1739d 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -330,8 +330,8 @@  int linedisp_register(struct linedisp *linedisp, struct device *parent,
 	linedisp->dev.parent = parent;
 	linedisp->dev.type = &linedisp_type;
 	linedisp->ops = ops;
-	linedisp->buf = buf;
-	linedisp->num_chars = num_chars;
+	linedisp->buf = buf ? buf : linedisp->curr;
+	linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
 	linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
 
 	err = ida_alloc(&linedisp_id, GFP_KERNEL);
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index 65d782111f53..4c354b8f376e 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -54,12 +54,15 @@  struct linedisp_ops {
 	void (*update)(struct linedisp *linedisp);
 };
 
+#define LINEDISP_DEFAULT_BUF_SZ		8u
+
 /**
  * struct linedisp - character line display private data structure
  * @dev: the line display device
  * @id: instance id of this display
  * @timer: timer used to implement scrolling
  * @ops: character line display operations
+ * @curr: fallback buffer for the string
  * @buf: pointer to the buffer for the string currently displayed
  * @message: the full message to display or scroll on the display
  * @num_chars: the number of characters that can be displayed
@@ -73,6 +76,7 @@  struct linedisp {
 	struct timer_list timer;
 	const struct linedisp_ops *ops;
 	struct linedisp_map *map;
+	char curr[LINEDISP_DEFAULT_BUF_SZ];
 	char *buf;
 	char *message;
 	unsigned int num_chars;