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
>
