Re: [PATCH 5/8] Add Advantech EIO Backlight driver
flat view
From: Daniel Thompson <hidden>
Date: 2025-12-12 17:59:30
Also in:
dri-devel, linux-gpio, linux-hwmon, linux-i2c, linux-pm, linux-watchdog, lkml
On Fri, Dec 12, 2025 at 05:40:56PM +0100, Ramiro Oliveira wrote:
This driver controls the Video Backlight block of the Advantech EIO chip. Signed-off-by: Ramiro Oliveira <redacted>
Thanks for the patch. Review below...
quoted hunk ↗ jump to hunk
--- MAINTAINERS | 1 + drivers/video/backlight/Kconfig | 6 + drivers/video/backlight/Makefile | 1 + drivers/video/backlight/eio_bl.c | 268 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 276 insertions(+)diff --git a/MAINTAINERS b/MAINTAINERS index be9d3c4e1ce1..df4b4cc31257 100644 --- a/MAINTAINERS +++ b/MAINTAINERS@@ -623,6 +623,7 @@ F: drivers/gpio/gpio-eio.c F: drivers/hwmon/eio-hwmon.c F: drivers/i2c/busses/i2c-eio.c F: drivers/mfd/eio_core.c +F: drivers/video/backlight/eio_bl.c F: include/linux/mfd/eio.h ADXL313 THREE-AXIS DIGITAL ACCELEROMETER DRIVERdiff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig index a1422ddd1c22..ddd3d6922553 100644 --- a/drivers/video/backlight/Kconfig +++ b/drivers/video/backlight/Kconfig@@ -496,6 +496,12 @@ config BACKLIGHT_RAVE_SP help Support for backlight control on RAVE SP device. +config BACKLIGHT_EIO + tristate "Advantech EIO Backlight" + depends on MFD_EIO && BACKLIGHT_CLASS_DEVICE + help + Backlight driver for Advantech EIO. + config BACKLIGHT_LED tristate "Generic LED based Backlight Driver" depends on LEDS_CLASS && OFdiff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile index a5d62b018102..4601b644b6d4 100644 --- a/drivers/video/backlight/Makefile +++ b/drivers/video/backlight/Makefile@@ -30,6 +30,7 @@ obj-$(CONFIG_BACKLIGHT_BD6107) += bd6107.o obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o obj-$(CONFIG_BACKLIGHT_DA903X) += da903x_bl.o obj-$(CONFIG_BACKLIGHT_DA9052) += da9052_bl.o +obj-$(CONFIG_BACKLIGHT_EIO) += eio_bl.o obj-$(CONFIG_BACKLIGHT_EP93XX) += ep93xx_bl.o obj-$(CONFIG_BACKLIGHT_GPIO) += gpio_backlight.o obj-$(CONFIG_BACKLIGHT_HP680) += hp680_bl.odiff --git a/drivers/video/backlight/eio_bl.c b/drivers/video/backlight/eio_bl.c new file mode 100644 index 000000000000..2b9fd4d48d30 --- /dev/null +++ b/drivers/video/backlight/eio_bl.c@@ -0,0 +1,268 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Backlight driver for Advantech EIO Embedded controller. + * + * Copyright (C) 2025 Advantech Corporation. All rights reserved. + */ + +#include <linux/backlight.h> +#include <linux/errno.h> +#include <linux/mfd/core.h> +#include <linux/mfd/eio.h> +#include <linux/module.h> +#include <linux/uaccess.h> + +#define PMC_BL_WRITE 0x20 +#define PMC_BL_READ 0x21 + +#define BL_CTRL_STATUS 0x00 +#define BL_CTRL_ENABLE 0x12 +#define BL_CTRL_ENABLE_INVERT 0x13 +#define BL_CTRL_DUTY 0x14 +#define BL_CTRL_INVERT 0x15 +#define BL_CTRL_FREQ 0x16 + +#define BL_MAX 2 + +#define BL_STATUS_AVAIL 0x01 +#define BL_ENABLE_OFF 0x00 +#define BL_ENABLE_ON 0x01 +#define BL_ENABLE_AUTO BIT(1) + +#define USE_DEFAULT -1 +#define THERMAL_MAX 100 + +#define BL_AVAIL BIT(0) +#define BL_PWM_DC BIT(1) +#define BL_PWM_SRC BIT(2) +#define BL_BRI_INVERT BIT(3) +#define BL_ENABLE_PIN_SUPP BIT(4) +#define BL_POWER_INVERT BIT(5) +#define BL_ENABLE_PIN_EN BIT(6) +#define BL_FIRMWARE_ERROR BIT(7)
These appear to be unused.
quoted hunk ↗ jump to hunk
+ +static uint bri_freq = USE_DEFAULT; +module_param(bri_freq, uint, 0444); +MODULE_PARM_DESC(bri_freq, "Setup backlight PWM frequency.\n"); + +static int bri_invert = USE_DEFAULT; +module_param(bri_invert, int, 0444); +MODULE_PARM_DESC(bri_invert, "Setup backlight PWM polarity.\n"); + +static int bl_power_invert = USE_DEFAULT; +module_param(bl_power_invert, int, 0444); +MODULE_PARM_DESC(bl_power_invert, "Setup backlight enable pin polarity.\n"); + +static int timeout; +module_param(timeout, int, 0444); +MODULE_PARM_DESC(timeout, "Set PMC command timeout value.\n");
Module parameters are not really expected these days and are pretty user hostile. Are they really needed? AFAICT this is a firmware based device. Why doesn't the firmware provide this information if the drivers need it (either directly or via PNP ID and a lookup table)?
quoted hunk ↗ jump to hunk
+ +struct eio_bl_dev { + struct device *mfd; + u8 id; + u8 max;
The value in max is never read.
quoted hunk ↗ jump to hunk
+}; + +static int pmc_write(struct device *mfd, u8 ctrl, u8 dev_id, void *data) +{ + struct pmc_op op = { + .cmd = PMC_BL_WRITE, + .control = ctrl, + .device_id = dev_id, + .payload = (u8 *)data, + .size = (ctrl == BL_CTRL_FREQ) ? 4 : 1, + .timeout = timeout, + }; + + return eio_core_pmc_operation(mfd, &op); +} + +static int pmc_read(struct device *mfd, u8 ctrl, u8 dev_id, void *data) +{ + struct pmc_op op = { + .cmd = PMC_BL_READ, + .control = ctrl, + .device_id = dev_id, + .payload = (u8 *)data, + .size = (ctrl == BL_CTRL_FREQ) ? 4 : 1, + .timeout = timeout, + }; + + return eio_core_pmc_operation(mfd, &op); +} + +static int bl_update_status(struct backlight_device *bl) +{ + struct eio_bl_dev *eio_bl = bl_get_data(bl); + u32 max = bl->props.max_brightness; + u8 duty = clamp_val(bl->props.brightness, 0, max); + u8 sw = bl->props.power == BACKLIGHT_POWER_OFF;
You shouldn't need to read anything from bl->props directly. Please use the helper functions to get the duty value and power state.
quoted hunk ↗ jump to hunk
+ int ret; + + /* Setup PWM duty */ + ret = pmc_write(eio_bl->mfd, BL_CTRL_DUTY, eio_bl->id, &duty); + if (ret) + return ret; + + /* Setup backlight enable pin */ + return pmc_write(eio_bl->mfd, BL_CTRL_ENABLE, eio_bl->id, &sw); +} + +static int bl_get_brightness(struct backlight_device *bl) +{ + struct eio_bl_dev *eio_bl = bl_get_data(bl); + u8 duty = 0; + int ret; + + ret = pmc_read(eio_bl->mfd, BL_CTRL_DUTY, eio_bl->id, &duty); + + if (ret) + return ret; + + return duty; +} + +static const struct backlight_ops bl_ops = { + .get_brightness = bl_get_brightness, + .update_status = bl_update_status, + .options = BL_CORE_SUSPENDRESUME, +}; + +static int bl_init(struct device *dev, int id, + struct backlight_properties *props) +{ + int ret; + u8 enabled = 0; + u8 status = 0; + + /* Check EC-supported backlight */ + ret = pmc_read(dev, BL_CTRL_STATUS, id, &status); + if (ret) + return ret; + + if (!(status & BL_STATUS_AVAIL)) { + dev_dbg(dev, "eio_bl%d hardware report disabled.\n", id); + return -ENXIO;
Is -ENODEV more appropriate here?
quoted hunk ↗ jump to hunk
+ } + + ret = pmc_read(dev, BL_CTRL_DUTY, id, &props->brightness); + if (ret) + return ret; + + /* Invert PWM */ + dev_dbg(dev, "bri_invert=%d\n", bri_invert);
Let's drop the dev_dbg() messages, printing module parameter values isn't very useful.
quoted hunk ↗ jump to hunk
+ if (bri_invert > USE_DEFAULT) { + ret = pmc_write(dev, BL_CTRL_INVERT, id, &bri_invert); + if (ret) + return ret; + } + + bri_invert = 0; + ret = pmc_read(dev, BL_CTRL_INVERT, id, &bri_invert); + if (ret) + return ret;
Writing back to module parameters during probe is rather unusual. Is it really needed?
quoted hunk ↗ jump to hunk
+ + dev_dbg(dev, "bri_freq=%u\n", bri_freq); + if (bri_freq != USE_DEFAULT) { + ret = pmc_write(dev, BL_CTRL_FREQ, id, &bri_freq); + if (ret) + return ret; + } + + ret = pmc_read(dev, BL_CTRL_FREQ, id, &bri_freq); + if (ret) + return ret; + + dev_dbg(dev, "bl_power_invert=%d\n", bl_power_invert); + if (bl_power_invert >= USE_DEFAULT) { + ret = pmc_write(dev, BL_CTRL_ENABLE_INVERT, id, &bl_power_invert); + if (ret) + return ret; + } + + bl_power_invert = 0; + ret = pmc_read(dev, BL_CTRL_ENABLE_INVERT, id, &bl_power_invert); + if (ret) + return ret; + + /* Read power state */ + ret = pmc_read(dev, BL_CTRL_ENABLE, id, &enabled); + if (ret) + return ret; + + props->power = enabled ? BACKLIGHT_POWER_OFF : BACKLIGHT_POWER_ON; + + return 0; +} + +static int bl_probe(struct platform_device *pdev) +{ + u8 id; + struct device *dev = &pdev->dev; + struct eio_dev *eio_dev = dev_get_drvdata(dev->parent); + + if (!eio_dev) { + dev_err(dev, "eio_core not present\n"); + return -ENODEV; + } + + for (id = 0; id < BL_MAX; id++) { + char name[32]; + struct backlight_properties props; + struct eio_bl_dev *eio_bl; + struct backlight_device *bl; + int ret; + + memset(&props, 0, sizeof(props)); + props.type = BACKLIGHT_RAW; + props.max_brightness = THERMAL_MAX; + props.power = BACKLIGHT_POWER_OFF; + props.brightness = props.max_brightness;
New drivers should not initialize props.scale as UNKNOWN. Please set to match the hardware behaviour (if brightness 50% looks roughly half as bright as 100% then the scale is non-linear).
quoted hunk ↗ jump to hunk
+ + eio_bl = devm_kzalloc(dev, sizeof(*eio_bl), GFP_KERNEL); + if (!eio_bl) + return -ENOMEM; + + eio_bl->mfd = dev->parent; + eio_bl->id = id; + eio_bl->max = props.max_brightness; + + ret = bl_init(eio_bl->mfd, id, &props); + if (ret) { + dev_info(dev, "%d No Backlight %u enabled!\n", ret, id); + continue; + }
If neither backlight is enabled if would be good to propagate the return value (to prevent the probe from spuriously succeeding).
quoted hunk ↗ jump to hunk
+ + snprintf(name, sizeof(name), "%s%u", pdev->name, id); + + bl = devm_backlight_device_register(dev, name, dev, eio_bl, + &bl_ops, &props); + + if (IS_ERR(bl)) { + ret = PTR_ERR(bl); + if (ret == -EPROBE_DEFER) + return ret; + + dev_err(dev, "register %s failed: %d\n", name, ret); + continue; + } + + dev_info(dev, "%s registered (max=%u)\n", name, props.max_brightness);
Silence (on success) is golden. Please remove this.
quoted hunk ↗ jump to hunk
+ } + + return 0; +} + +static struct platform_driver bl_driver = { + .probe = bl_probe, + .driver = { + .name = "eio_bl", + }, +}; + +module_platform_driver(bl_driver); + +MODULE_AUTHOR("Wenkai Chung <wenkai.chung@advantech.com.tw>"); +MODULE_AUTHOR("Ramiro Oliveira <ramiro.oliveira@advantech.com>"); +MODULE_DESCRIPTION("Backlight driver for Advantech EIO embedded controller"); +MODULE_LICENSE("GPL");
Thanks Daniel.