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
> +/****************************************************************************
> + ****************************************************************************
> + *
> + * ACPI Helpers and device model
> + *
> + ****************************************************************************
> + ****************************************************************************/
Try to follow the normal commenting style
> +/* 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.
> +#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.
> +
> +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()'.
> +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.
> +
> +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.
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.
Arnd
