HD44780 APIs are operate on struct charlcd object. Moreover, the current users
always call charlcd_alloc() and hd44780_common_alloc(). Make the latter call
the former, so eliminate the additional allocation, make it consistent with
the rest of API and avoid duplication.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/hd44780.c | 18 ++++++------------
drivers/auxdisplay/hd44780_common.c | 14 ++++++++------
drivers/auxdisplay/hd44780_common.h | 4 ++--
drivers/auxdisplay/panel.c | 17 +++++------------
4 files changed, 21 insertions(+), 32 deletions(-)
diff --git a/drivers/auxdisplay/hd44780.c b/drivers/auxdisplay/hd44780.c
index ef38cb7bf13d..cef42656c4b0 100644
--- a/drivers/auxdisplay/hd44780.c
+++ b/drivers/auxdisplay/hd44780.c
@@ -222,20 +222,17 @@ static int hd44780_probe(struct platform_device *pdev)
return -EINVAL;
}
- hdc = hd44780_common_alloc();
- if (!hdc)
- return -ENOMEM;
-
- lcd = charlcd_alloc(0);
+ lcd = hd44780_common_alloc();
if (!lcd)
- goto fail1;
+ return -ENOMEM;
hd = kzalloc(sizeof(*hd), GFP_KERNEL);
if (!hd)
goto fail2;
+ hdc = lcd->drvdata;
hdc->hd44780 = hd;
- lcd->drvdata = hdc;
+
for (i = 0; i < ifwidth; i++) {
hd->pins[base + i] = devm_gpiod_get_index(dev, "data", i,
GPIOD_OUT_LOW);
@@ -313,9 +310,7 @@ static int hd44780_probe(struct platform_device *pdev)
fail3:
kfree(hd);
fail2:
- charlcd_free(lcd);
-fail1:
- hd44780_common_free(hdc);
+ hd44780_common_free(lcd);
return ret;
}
@@ -326,8 +321,7 @@ static void hd44780_remove(struct platform_device *pdev)
charlcd_unregister(lcd);
kfree(hdc->hd44780);
- hd44780_common_free(hdc);
- charlcd_free(lcd);
+ hd44780_common_free(lcd);
}
static const struct of_device_id hd44780_of_match[] = {
diff --git a/drivers/auxdisplay/hd44780_common.c b/drivers/auxdisplay/hd44780_common.c
index 3f8a496ccb8e..fb340d18fcad 100644
--- a/drivers/auxdisplay/hd44780_common.c
+++ b/drivers/auxdisplay/hd44780_common.c
@@ -351,24 +351,26 @@ int hd44780_common_redefine_char(struct charlcd *lcd, char *esc)
}
EXPORT_SYMBOL_GPL(hd44780_common_redefine_char);
-struct hd44780_common *hd44780_common_alloc(void)
+struct charlcd *hd44780_common_alloc(void)
{
struct hd44780_common *hd;
+ struct charlcd *lcd;
- hd = kzalloc(sizeof(*hd), GFP_KERNEL);
- if (!hd)
+ lcd = charlcd_alloc(sizeof(*hd));
+ if (!lcd)
return NULL;
+ hd = lcd->drvdata;
hd->ifwidth = 8;
hd->bwidth = DEFAULT_LCD_BWIDTH;
hd->hwidth = DEFAULT_LCD_HWIDTH;
- return hd;
+ return lcd;
}
EXPORT_SYMBOL_GPL(hd44780_common_alloc);
-void hd44780_common_free(struct hd44780_common *hd)
+void hd44780_common_free(struct charlcd *lcd)
{
- kfree(hd);
+ charlcd_free(lcd);
}
EXPORT_SYMBOL_GPL(hd44780_common_free);
diff --git a/drivers/auxdisplay/hd44780_common.h b/drivers/auxdisplay/hd44780_common.h
index fe1386e3cf79..4c87f55722b6 100644
--- a/drivers/auxdisplay/hd44780_common.h
+++ b/drivers/auxdisplay/hd44780_common.h
@@ -31,5 +31,5 @@ int hd44780_common_fontsize(struct charlcd *lcd, enum charlcd_fontsize size);
int hd44780_common_lines(struct charlcd *lcd, enum charlcd_lines lines);
int hd44780_common_redefine_char(struct charlcd *lcd, char *esc);
-struct hd44780_common *hd44780_common_alloc(void);
-void hd44780_common_free(struct hd44780_common *hd);
+struct charlcd *hd44780_common_alloc(void);
+void hd44780_common_free(struct charlcd *lcd);
diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
index aa1d03fef22e..91ccb9789d43 100644
--- a/drivers/auxdisplay/panel.c
+++ b/drivers/auxdisplay/panel.c
@@ -831,18 +831,12 @@ static void lcd_init(void)
struct charlcd *charlcd;
struct hd44780_common *hdc;
- hdc = hd44780_common_alloc();
- if (!hdc)
+ charlcd = hd44780_common_alloc();
+ if (!charlcd)
return;
- charlcd = charlcd_alloc(0);
- if (!charlcd) {
- hd44780_common_free(hdc);
- return;
- }
-
+ hdc = charlcd->drvdata;
hdc->hd44780 = &lcd;
- charlcd->drvdata = hdc;
/*
* Init lcd struct with load-time values to preserve exact
@@ -1664,7 +1658,7 @@ static void panel_attach(struct parport *port)
if (lcd.enabled)
charlcd_unregister(lcd.charlcd);
err_unreg_device:
- charlcd_free(lcd.charlcd);
+ hd44780_common_free(lcd.charlcd);
lcd.charlcd = NULL;
parport_unregister_device(pprt);
pprt = NULL;
@@ -1691,8 +1685,7 @@ static void panel_detach(struct parport *port)
if (lcd.enabled) {
charlcd_unregister(lcd.charlcd);
lcd.initialized = false;
- hd44780_common_free(lcd.charlcd->drvdata);
- charlcd_free(lcd.charlcd);
+ hd44780_common_free(lcd.charlcd);
lcd.charlcd = NULL;
}
--
2.45.1.3035.g276e886db78bHi Andy,
Thanks for your patch!
On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> HD44780 APIs are operate on struct charlcd object. Moreover, the current users
s/are/all/
s/object/objects/
> always call charlcd_alloc() and hd44780_common_alloc(). Make the latter call
> the former, so eliminate the additional allocation, make it consistent with
either s/make/making/, or s/make/to make/
> the rest of API and avoid duplication.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
As the code looks correct to me:
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> --- a/drivers/auxdisplay/hd44780_common.c
> +++ b/drivers/auxdisplay/hd44780_common.c
> @@ -351,24 +351,26 @@ int hd44780_common_redefine_char(struct charlcd *lcd, char *esc)
> }
> EXPORT_SYMBOL_GPL(hd44780_common_redefine_char);
>
> -struct hd44780_common *hd44780_common_alloc(void)
> +struct charlcd *hd44780_common_alloc(void)
> {
> struct hd44780_common *hd;
> + struct charlcd *lcd;
>
> - hd = kzalloc(sizeof(*hd), GFP_KERNEL);
> - if (!hd)
> + lcd = charlcd_alloc(sizeof(*hd));
> + if (!lcd)
> return NULL;
>
> + hd = lcd->drvdata;
> hd->ifwidth = 8;
> hd->bwidth = DEFAULT_LCD_BWIDTH;
> hd->hwidth = DEFAULT_LCD_HWIDTH;
> - return hd;
> + return lcd;
> }
> EXPORT_SYMBOL_GPL(hd44780_common_alloc);
While I like the general idea, there are two things in the API I do
not like:
1. The function is called "hd44780_common_alloc()", but returns
a pointer to a different struct type than the name suggests,
2. The real "struct hd44780_common" must be obtained by the caller
from charlcd.drvdata, which is of type "void *", i.e. unsafe.
What about changing it to e.g.?
struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd)
so you can return pointers to both structs?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Fri, Mar 07, 2025 at 10:14:48AM +0100, Geert Uytterhoeven wrote: > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > HD44780 APIs are operate on struct charlcd object. Moreover, the current users > > s/are/all/ > s/object/objects/ > > > always call charlcd_alloc() and hd44780_common_alloc(). Make the latter call > > the former, so eliminate the additional allocation, make it consistent with > > either s/make/making/, or s/make/to make/ > > > the rest of API and avoid duplication. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > As the code looks correct to me: > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> Thanks I have corrected all grammar issues in the commit messages except one in the first patch which I do not understand. ... > While I like the general idea, there are two things in the API I do > not like: > 1. The function is called "hd44780_common_alloc()", but returns > a pointer to a different struct type than the name suggests, > 2. The real "struct hd44780_common" must be obtained by the caller > from charlcd.drvdata, which is of type "void *", i.e. unsafe. > > What about changing it to e.g.? > > struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd) > > so you can return pointers to both structs? I don't like this prototype as it seems and feels confusing. Also note, the APIs are using struct charlcd while being in the hd44780 namespace. perhaps better to rename the function to hd44780_common_and_lcd_alloc()? -- With Best Regards, Andy Shevchenko
Hi Andy,
On Fri, 7 Mar 2025 at 18:08, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Mar 07, 2025 at 10:14:48AM +0100, Geert Uytterhoeven wrote:
> > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > HD44780 APIs are operate on struct charlcd object. Moreover, the current users
> >
> > s/are/all/
> > s/object/objects/
> >
> > > always call charlcd_alloc() and hd44780_common_alloc(). Make the latter call
> > > the former, so eliminate the additional allocation, make it consistent with
> >
> > either s/make/making/, or s/make/to make/
> >
> > > the rest of API and avoid duplication.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > As the code looks correct to me:
> > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Thanks I have corrected all grammar issues in the commit messages except one in
> the first patch which I do not understand.
>
> ...
>
> > While I like the general idea, there are two things in the API I do
> > not like:
> > 1. The function is called "hd44780_common_alloc()", but returns
> > a pointer to a different struct type than the name suggests,
> > 2. The real "struct hd44780_common" must be obtained by the caller
> > from charlcd.drvdata, which is of type "void *", i.e. unsafe.
> >
> > What about changing it to e.g.?
> >
> > struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd)
> >
> > so you can return pointers to both structs?
>
> I don't like this prototype as it seems and feels confusing. Also note,
> the APIs are using struct charlcd while being in the hd44780 namespace.
> perhaps better to rename the function to hd44780_common_and_lcd_alloc()?
That is one option.
Another option would be to add a "charlcd *lcd" member to
struct hd44780_common.
That would allow to fix the other odd part in the API:
-void hd44780_common_free(struct charlcd *lcd)
+void hd44780_common_free(struct hd4480_common *hd)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Fri, Mar 07, 2025 at 07:19:57PM +0100, Geert Uytterhoeven wrote: > On Fri, 7 Mar 2025 at 18:08, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Mar 07, 2025 at 10:14:48AM +0100, Geert Uytterhoeven wrote: > > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: ... > > > While I like the general idea, there are two things in the API I do > > > not like: > > > 1. The function is called "hd44780_common_alloc()", but returns > > > a pointer to a different struct type than the name suggests, > > > 2. The real "struct hd44780_common" must be obtained by the caller > > > from charlcd.drvdata, which is of type "void *", i.e. unsafe. > > > > > > What about changing it to e.g.? > > > > > > struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd) > > > > > > so you can return pointers to both structs? > > > > I don't like this prototype as it seems and feels confusing. Also note, > > the APIs are using struct charlcd while being in the hd44780 namespace. > > perhaps better to rename the function to hd44780_common_and_lcd_alloc()? > > That is one option. > > Another option would be to add a "charlcd *lcd" member to > struct hd44780_common. > > That would allow to fix the other odd part in the API: > > -void hd44780_common_free(struct charlcd *lcd) > +void hd44780_common_free(struct hd4480_common *hd) This I like better. In a separate patch I think? -- With Best Regards, Andy Shevchenko
Hi Andy,
On Fri, 7 Mar 2025 at 20:01, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Mar 07, 2025 at 07:19:57PM +0100, Geert Uytterhoeven wrote:
> > On Fri, 7 Mar 2025 at 18:08, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Mar 07, 2025 at 10:14:48AM +0100, Geert Uytterhoeven wrote:
> > > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
>
> ...
>
> > > > While I like the general idea, there are two things in the API I do
> > > > not like:
> > > > 1. The function is called "hd44780_common_alloc()", but returns
> > > > a pointer to a different struct type than the name suggests,
> > > > 2. The real "struct hd44780_common" must be obtained by the caller
> > > > from charlcd.drvdata, which is of type "void *", i.e. unsafe.
> > > >
> > > > What about changing it to e.g.?
> > > >
> > > > struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd)
> > > >
> > > > so you can return pointers to both structs?
> > >
> > > I don't like this prototype as it seems and feels confusing. Also note,
> > > the APIs are using struct charlcd while being in the hd44780 namespace.
> > > perhaps better to rename the function to hd44780_common_and_lcd_alloc()?
> >
> > That is one option.
> >
> > Another option would be to add a "charlcd *lcd" member to
> > struct hd44780_common.
> >
> > That would allow to fix the other odd part in the API:
> >
> > -void hd44780_common_free(struct charlcd *lcd)
> > +void hd44780_common_free(struct hd4480_common *hd)
>
> This I like better. In a separate patch I think?
Fine for me...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Fri, Mar 07, 2025 at 08:06:37PM +0100, Geert Uytterhoeven wrote: > On Fri, 7 Mar 2025 at 20:01, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Mar 07, 2025 at 07:19:57PM +0100, Geert Uytterhoeven wrote: > > > On Fri, 7 Mar 2025 at 18:08, Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Mar 07, 2025 at 10:14:48AM +0100, Geert Uytterhoeven wrote: > > > > > On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: ... > > > > > While I like the general idea, there are two things in the API I do > > > > > not like: > > > > > 1. The function is called "hd44780_common_alloc()", but returns > > > > > a pointer to a different struct type than the name suggests, > > > > > 2. The real "struct hd44780_common" must be obtained by the caller > > > > > from charlcd.drvdata, which is of type "void *", i.e. unsafe. > > > > > > > > > > What about changing it to e.g.? > > > > > > > > > > struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd) > > > > > > > > > > so you can return pointers to both structs? > > > > > > > > I don't like this prototype as it seems and feels confusing. Also note, > > > > the APIs are using struct charlcd while being in the hd44780 namespace. > > > > perhaps better to rename the function to hd44780_common_and_lcd_alloc()? > > > > > > That is one option. > > > > > > Another option would be to add a "charlcd *lcd" member to > > > struct hd44780_common. > > > > > > That would allow to fix the other odd part in the API: > > > > > > -void hd44780_common_free(struct charlcd *lcd) > > > +void hd44780_common_free(struct hd4480_common *hd) > > > > This I like better. In a separate patch I think? > > Fine for me... Thanks, I will check what can I do, but currently it's not a high priority to me. -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.