Re: [PATCH 2/2] LoongArch: Add ACPI-based generic laptop driver

[Arnd Bergmann <arnd@arndb.de>]

On Tue, Aug 16, 2022 at 10:55 AM Huacai Chen <chenhuacai@kernel.org> wrote:
> On Tue, Aug 16, 2022 at 3:11 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Mon, Aug 15, 2022 at 2:48 PM Huacai Chen <chenhuacai@loongson.cn> wrote:
> > > +
> > > +static int __init register_generic_subdriver(struct generic_struct *sub_driver)
> > > +{
> > > +       int rc;
> > > +
> > > +       BUG_ON(!sub_driver->acpi);
> > > +
> > > +       sub_driver->acpi->driver = kzalloc(sizeof(struct acpi_driver), GFP_KERNEL);
> > > +       if (!sub_driver->acpi->driver) {
> > > +               pr_err("failed to allocate memory for ibm->acpi->driver\n");
> > > +               return -ENOMEM;
> > > +       }
> >
> > Drivers should be statically allocated. Usually you want one 'struct
> > acpi_driver' or
> > 'struct platform_driver' per file, so you can just use 'module_acpi_driver()'.
> I found that "subdriver" in other laptop drivers also uses dynamical
> allocation, because there may be various numbers of subdrivers. I want
> to keep it, at least in the next version for review.

Fair enough, I'm not that familiar with drivers/platform/ conventions,
so this is
probably fine.  Adding the drivers/platform/x86 maintainers for clarification,
since your driver probably fits best into that general class regardless of the
CPU instruction set.

> > > +static struct generic_acpi_drv_struct ec_event_acpidriver = {
> > > +       .hid = loongson_htk_device_ids,
> > > +       .notify = event_notify,
> > > +       .handle = &hkey_handle,
> > > +       .type = ACPI_DEVICE_NOTIFY,
> > > +};
> >
> > Same here, this can probably just be an input driver in drivers/input.
>
> It seems the existing "laptop drivers" are also complex drivers to
> bind several "subdrivers" together.

Let's see what Hans and Mark think here as well. My feeling is that this
might be a little different. In the other laptop drivers you'd load a
module for a Dell/HP/Lenovo/Asus/Acer/... model and that has all the
bits that this vendor uses across the system, so a single driver makes
sense.

In embedded systems, you'd have SoC specific drivers that we tend
to put into the respective subsystems (backlight, led, input, ...) and
then users pick the drivers they want.

Loongarch is probably somewhere inbetween, since this is a chip
vendor rather than a laptop vendor, but you have all the same bits
here. The question is more about where we put it.

       Arnd