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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, Arnd,

On Tue, Aug 16, 2022 at 3:11 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Mon, Aug 15, 2022 at 2:48 PM Huacai Chen <chenhuacai@xxxxxxxxxxx> wrote:
> >
> > From: Jianmin Lv <lvjianmin@xxxxxxxxxxx>
> >
> > This add ACPI-based generic laptop driver for Loongson-3. Some of the
> > codes are derived from drivers/platform/x86/thinkpad_acpi.c.
> >
> > Signed-off-by: Jianmin Lv <lvjianmin@xxxxxxxxxxx>
> > Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>
> > ---
> >  drivers/platform/loongarch/Kconfig          |  18 +
> >  drivers/platform/loongarch/Makefile         |   1 +
> >  drivers/platform/loongarch/generic-laptop.c | 775 ++++++++++++++++++++
> >  3 files changed, 794 insertions(+)
> >  create mode 100644 drivers/platform/loongarch/generic-laptop.c
> >
> > diff --git a/drivers/platform/loongarch/Kconfig b/drivers/platform/loongarch/Kconfig
> > index a1542843b0ad..086212d57251 100644
> > --- a/drivers/platform/loongarch/Kconfig
> > +++ b/drivers/platform/loongarch/Kconfig
> > @@ -23,4 +23,22 @@ config CPU_HWMON
> >         help
> >           Loongson-3A/3B/3C CPU HWMon (temperature sensor) driver.
> >
> > +config GENERIC_LAPTOP
> > +       tristate "Generic Loongson-3A Laptop Driver"
> > +       depends on MACH_LOONGSON64
> > +       depends on ACPI
> > +       depends on INPUT
> > +       select BACKLIGHT_CLASS_DEVICE
> > +       select BACKLIGHT_LCD_SUPPORT
> > +       select HWMON
> > +       select INPUT_EVDEV
> > +       select INPUT_SPARSEKMAP
> > +       select LCD_CLASS_DEVICE
> > +       select LEDS_CLASS
> > +       select POWER_SUPPLY
> > +       select VIDEO_OUTPUT_CONTROL
> > +       default y
> > +       help
> > +         ACPI-based Loongson-3 family laptops generic driver.
>
> It's rather bad style to 'select' entire subsystems from a device
> driver. This may be
> unavoidable in some cases, but please try to make it possible to build the
> driver when some or all of the other subsystems are disabled. In a lot
> of subsystems,
> there is an API stub like
OK, the Kconfig should be cleaned up, I will remove those unneeded
lines, and convert some others to "depends on".

>
> > +/****************************************************************************
> > + ****************************************************************************
> > + *
> > + * ACPI Helpers and device model
> > + *
> > + ****************************************************************************
> > + ****************************************************************************/
>
> Try to follow the normal commenting style
OK, thanks.

>
> > +/* ACPI basic handles */
> > +
> > +static int acpi_evalf(acpi_handle handle,
> > +                     int *res, char *method, char *fmt, ...);
> > +static acpi_handle ec_handle;
>
> Instead of forward function declarations, try sorting the functions in
> call order,
> which has the benefit of making more sense to most readers.
OK, thanks.

>
> > +#ifdef CONFIG_PM
> > +static int loongson_generic_suspend(struct device *dev)
> > +{
> > +       return 0;
> > +}
> > +static int loongson_generic_resume(struct device *dev)
> > +{
> > +       int status = 0;
> > +       struct key_entry ke;
> > +       struct backlight_device *bd;
> > +
> > +       /*
> > +        * Only if the firmware supports SW_LID event model, we can handle the
> > +        * event. This is for the consideration of development board without
> > +        * EC.
> > +        */
> > +       if (test_bit(SW_LID, generic_inputdev->swbit)) {
> > +               if (hotkey_status_get(&status))
> > +                       return -EIO;
> > +               /*
> > +                * The input device sw element records the last lid status.
> > +                * When the system is awakened by other wake-up sources,
> > +                * the lid event will also be reported. The judgment of
> > +                * adding SW_LID bit which in sw element can avoid this
> > +                * case.
> > +                *
> > +                * input system will drop lid event when current lid event
> > +                * value and last lid status in the same data set,which
> > +                * data set inclue zero set and no zero set. so laptop
> > +                * driver doesn't report repeated events.
> > +                *
> > +                * Lid status is generally 0, but hardware exception is
> > +                * considered. So add lid status confirmation.
> > +                */
> > +               if (test_bit(SW_LID, generic_inputdev->sw) && !(status & (1 << SW_LID))) {
> > +                       ke.type = KE_SW;
> > +                       ke.sw.value = (u8)status;
> > +                       ke.sw.code = SW_LID;
> > +                       sparse_keymap_report_entry(generic_inputdev, &ke,
> > +                                       1, true);
> > +               }
> > +       }
> > +
> > +       bd = backlight_device_get_by_type(BACKLIGHT_PLATFORM);
> > +       if (bd) {
> > +               loongson_laptop_backlight_update(bd) ?
> > +               pr_warn("Loongson_backlight:resume brightness failed") :
> > +               pr_info("Loongson_backlight:resume brightness %d\n", bd->props.brightness);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(loongson_generic_pm,
> > +               loongson_generic_suspend, loongson_generic_resume);
> > +#endif
>
>
> Instead of the #ifdef, use the newer DEFINE_SIMPLE_DEV_PM_OPS() in place
> of SIMPLE_DEV_PM_OPS() so the code will always be parsed but left out
> when CONFIG_PM is disabled.
OK, thanks.

>
> > +
> > +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.

>
> > +int ec_get_brightness(void)
> > +{
> > +       int status = 0;
> > +
> > +       if (!hkey_handle)
> > +               return -ENXIO;
> > +
> > +       if (!acpi_evalf(hkey_handle, &status, "ECBG", "d"))
> > +               return -EIO;
> > +
> > +       if (status < 0)
> > +               return status;
> > +
> > +       return status;
> > +}
> > +EXPORT_SYMBOL(ec_get_brightness);
>
> The name is too generic to have it in the global namespace for a platform
> specific driver. Use a prefix to make it clear which driver this belongs to.
>
> Not sure this function warrants an export though, it looks like you could
> just have it in the caller module.
Yes, they should be static.

>
> > +
> > +int turn_off_backlight(void)
> > +{
> > +       int status;
> > +       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> > +       struct acpi_object_list args = { 1, &arg0 };
> > +
> > +       arg0.integer.value = 0;
> > +       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> > +       if (ACPI_FAILURE(status)) {
> > +               pr_info("Loongson lvds error: 0x%x\n", status);
> > +               return -ENODEV;
> > +       }
> > +
> > +       return 0;
> > +}
>
> Again, the name is too generic for a global function.
Yes, they should be renamed.

>
> I suspect that if you split out the backlight handling into a separate
> driver, you can avoid
> the 'select' statements completely and make that driver 'depends on
> BACKLIGHT_CLASS_DEVICE'
> or move it within the 'if BACKLIGHT_CLASS_DEVICE' section of
> drivers/video/backlight/Kconfig.
>
>
> > +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.

Huacai
>
>        Arnd
>




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux