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

[Andy Shevchenko]

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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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