Re: [PATCH v1 6/7] auxdisplay: hd44780: Call charlcd_alloc() from hd44780_common_alloc()

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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