* [PATCH v3 2/7] video/logo: add a type parameter to the logo makefile function
From: Vincent Mailhol @ 2026-01-08 19:04 UTC (permalink / raw)
To: Helge Deller, Greg Kroah-Hartman, Yoshinori Sato, Rich Felker,
John Paul Adrian Glaubitz
Cc: Geert Uytterhoeven, linux-fbdev, dri-devel, linux-kernel,
linux-sh, linux-m68k, Vincent Mailhol
In-Reply-To: <20260108-custom-logo-v3-0-5a7aada7a6d4@kernel.org>
When translating a portable pixmap file into a .c file, the pnmtologo
tool expects to receive the image type (either mono, vga16 or clut224)
as an argument under the -t option.
Currently, this information is stored in the file name. Because we
will allow for custom logo in an upcoming change, it is preferable to
decouple the image name from its type.
Add a new $2 parameter to the Makefile logo function which contains
the image type.
Update all the individual targets to provide this new argument.
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
drivers/video/logo/Makefile | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/video/logo/Makefile b/drivers/video/logo/Makefile
index 8b67c4941a4c..3f249e9dcf37 100644
--- a/drivers/video/logo/Makefile
+++ b/drivers/video/logo/Makefile
@@ -22,13 +22,16 @@ hostprogs := pnmtologo
# Create commands like "pnmtologo -t mono -n logo_mac_mono -o ..."
quiet_cmd_logo = LOGO $@
- cmd_logo = $(obj)/pnmtologo -t $(lastword $(subst _, ,$*)) -n $* -o $@ $<
+ cmd_logo = $(obj)/pnmtologo -t $2 -n $* -o $@ $<
$(obj)/%.c: $(src)/%.pbm $(obj)/pnmtologo FORCE
- $(call if_changed,logo)
+ $(call if_changed,logo,mono)
-$(obj)/%.c: $(src)/%.ppm $(obj)/pnmtologo FORCE
- $(call if_changed,logo)
+$(obj)/%_vga16.c: $(src)/%_vga16.ppm $(obj)/pnmtologo FORCE
+ $(call if_changed,logo,vga16)
+
+$(obj)/%_clut224.c: $(src)/%_clut224.ppm $(obj)/pnmtologo FORCE
+ $(call if_changed,logo,clut224)
# generated C files
targets += *_mono.c *_vga16.c *_clut224.c
--
2.52.0
^ permalink raw reply related
* [PATCH v3 1/7] video/logo: remove orphan .pgm Makefile rule
From: Vincent Mailhol @ 2026-01-08 19:04 UTC (permalink / raw)
To: Helge Deller, Greg Kroah-Hartman, Yoshinori Sato, Rich Felker,
John Paul Adrian Glaubitz
Cc: Geert Uytterhoeven, linux-fbdev, dri-devel, linux-kernel,
linux-sh, linux-m68k, Vincent Mailhol
In-Reply-To: <20260108-custom-logo-v3-0-5a7aada7a6d4@kernel.org>
The kernel has no actual grey-scale logos. And looking at the git
history, it seems that there never was one (or maybe there was in the
pre-git history? I did not check that far…)
Remove the Makefile rule for the .pgm grey scale images.
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
drivers/video/logo/Makefile | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/video/logo/Makefile b/drivers/video/logo/Makefile
index 895c60b8402e..8b67c4941a4c 100644
--- a/drivers/video/logo/Makefile
+++ b/drivers/video/logo/Makefile
@@ -30,8 +30,5 @@ $(obj)/%.c: $(src)/%.pbm $(obj)/pnmtologo FORCE
$(obj)/%.c: $(src)/%.ppm $(obj)/pnmtologo FORCE
$(call if_changed,logo)
-$(obj)/%.c: $(src)/%.pgm $(obj)/pnmtologo FORCE
- $(call if_changed,logo)
-
# generated C files
-targets += *_mono.c *_vga16.c *_clut224.c *_gray256.c
+targets += *_mono.c *_vga16.c *_clut224.c
--
2.52.0
^ permalink raw reply related
* [PATCH v3 0/7] video/logo: allow custom boot logo and simplify logic
From: Vincent Mailhol @ 2026-01-08 19:04 UTC (permalink / raw)
To: Helge Deller, Greg Kroah-Hartman, Yoshinori Sato, Rich Felker,
John Paul Adrian Glaubitz
Cc: Geert Uytterhoeven, linux-fbdev, dri-devel, linux-kernel,
linux-sh, linux-m68k, Vincent Mailhol
This series allows the user to replace the default kernel boot logo by
a custom one directly in the kernel configuration. This makes it
easier to customise the boot logo without the need to modify the
sources and allows such customisation to remain persistent after
applying the configuration to another version of the kernel.
Patch #1 and #2 are clean-up and preparation while patch #3 is the
main feature of this series: making the boot logo customisable.
While working on this, I realised that managing the logo file directly
in Kbuild allows us to simplify how we handle the different existing
variants of the Tux logo. This series thus ends with a clean-up which
moves all the logo selection logic to Kbuild except from one special
case (details in the patch description), simplifying the Makefile and
the C code.
Patch #4 and #5 do a tree-wide clean-up on the Kconfig symbols that
are to be removed in patch #7. Patch #6 removes the Macintosh 68k logo
which is conflicting with our simplification plans. Patch #7 finally
simplifies the logic as explained above.
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Changes in v3:
- v2 broke the logo selection for m68k resulting in the MAC logo
replacing the default logo on some machine where it shouldn't.
v3 resolves the conflict by removing that logo.
- Typo fix in patch #4 description.
Link to v2: https://lore.kernel.org/r/20260101-custom-logo-v2-0-8eec06dfbf85@kernel.org
Changes in v2:
- By removing the logo_spe_clut224.o target from the Makefile, v1
also removed the logo_spe_clut224 object which is still being
referenced in
arch/powerpc/platforms/cell/spu_base.c
Restore the logo_spe_clut224.o target.
Link to v1: https://lore.kernel.org/r/20251230-custom-logo-v1-0-4736374569ee@kernel.org
---
Vincent Mailhol (7):
video/logo: remove orphan .pgm Makefile rule
video/logo: add a type parameter to the logo makefile function
video/logo: allow custom logo
newport_con: depend on LOGO_LINUX_CLUT224 instead of LOGO_SGI_CLUT224
sh: defconfig: remove CONFIG_LOGO_SUPERH_*
video/logo: remove logo_mac_clut224
video/logo: move logo selection logic to Kconfig
arch/sh/configs/dreamcast_defconfig | 2 -
arch/sh/configs/ecovec24_defconfig | 2 -
arch/sh/configs/kfr2r09_defconfig | 2 -
arch/sh/configs/migor_defconfig | 2 -
arch/sh/configs/rts7751r2d1_defconfig | 2 -
arch/sh/configs/rts7751r2dplus_defconfig | 2 -
arch/sh/configs/se7724_defconfig | 2 -
arch/sh/configs/se7780_defconfig | 2 -
arch/sh/configs/sh7785lcr_defconfig | 3 -
arch/sh/configs/urquell_defconfig | 3 -
drivers/video/console/newport_con.c | 4 +-
drivers/video/logo/Kconfig | 82 +-
drivers/video/logo/Makefile | 29 +-
drivers/video/logo/logo.c | 46 +-
drivers/video/logo/logo_mac_clut224.ppm | 1604 ------------------------------
include/linux/linux_logo.h | 8 -
16 files changed, 63 insertions(+), 1732 deletions(-)
---
base-commit: 9ace4753a5202b02191d54e9fdf7f9e3d02b85eb
change-id: 20251227-custom-logo-932df316a02c
Best regards,
--
Vincent Mailhol <mailhol@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 6/6] video/logo: move logo selection logic to Kconfig
From: Vincent Mailhol @ 2026-01-08 19:03 UTC (permalink / raw)
To: Geert Uytterhoeven, Helge Deller
Cc: Greg Kroah-Hartman, Rich Felker, John Paul Adrian Glaubitz,
linux-fbdev, dri-devel, linux-kernel, linux-sh, linux-m68k
In-Reply-To: <CAMuHMdUqHUrg6XwqXCSwv-3TwiYUOfbBMpMiuS89d1uwChBYaQ@mail.gmail.com>
On 07/01/2026 at 14:53, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Wed, 7 Jan 2026 at 13:21, Helge Deller <deller@gmx.de> wrote:
>> On 1/7/26 11:36, Geert Uytterhoeven wrote:
>>> On Tue, 6 Jan 2026 at 21:10, Vincent Mailhol <mailhol@kernel.org> wrote:
>>>> On 06/01/2026 at 12:48, Geert Uytterhoeven wrote:
>>>>> Thanks for your patch, which is now commit bd710b3da7308cb1
>>>>> ("video/logo: move logo selection logic to Kconfig") in fbdev/for-next.
>>>>>
>>>>> On Thu, 1 Jan 2026 at 16:26, Vincent Mailhol <mailhol@kernel.org> wrote:
>>>>>> Now that the path to the logo file can be directly entered in Kbuild,
>>>>>> there is no more need to handle all the logo file selection in the
>>>>>> Makefile and the C files.
>>>>>
>>>>> This may do the wrong thing when booting a multi-platform kernel.
>>>>>
>>>>>>
>>>>>> The only exception is the logo_spe_clut224 which is only used by the
>>>>>> Cell processor (found for example in the Playstation 3) [1]. This
>>>>>> extra logo uses its own different image which shows up on a separate
>>>>>> line just below the normal logo. Because the extra logo uses a
>>>>>> different image, it can not be factorized under the custom logo logic.
>>>>>>
>>>>>> Move all the logo file selection logic to Kbuild (except from the
>>>>>> logo_spe_clut224.ppm), this done, clean-up the C code to only leave
>>>>>> one entry for each logo type (monochrome, 16-colors and 224-colors).
>>>>>>
>>>>>> [1] Cell SPE logos
>>>>>> Link: https://lore.kernel.org/all/20070710122702.765654000@pademelon.sonytel.be/
>>>>>>
>>>>>> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
>>>>>
>>>>>> --- a/drivers/video/logo/Kconfig
>>>>>> +++ b/drivers/video/logo/Kconfig
>>>>>
>>>>>> @@ -61,6 +63,12 @@ config LOGO_LINUX_CLUT224
>>>>>> config LOGO_LINUX_CLUT224_FILE
>>>>>> string "224-color logo .ppm file"
>>>>>> depends on LOGO_LINUX_CLUT224
>>>>>> + default "drivers/video/logo/logo_dec_clut224.ppm" if MACH_DECSTATION || ALPHA
>>>>>> + default "drivers/video/logo/logo_mac_clut224.ppm" if MAC
>>>>>
>>>>> E.g. an m68k multi-platform kernel including Mac support will scare
>>>>> non-Mac users into thinking their machine was assimilated by the
>>>>> Apple Empire...
>
>>>>>> --- a/drivers/video/logo/logo.c
>>>>>> +++ b/drivers/video/logo/logo.c
>
>>>>>> -#ifdef CONFIG_LOGO_MAC_CLUT224
>>>>>> - /* Macintosh Linux logo on m68k */
>>>>>> - if (MACH_IS_MAC)
>>>>>
>>>>> MACH_IS_MAC can be a runtime check.
>>>>
>>>> OK. I missed this.
>>>>
>>>> I think there are two options to fix this:
>>>>
>>>> 1. Keep CONFIG_LOGO_MAC_CLUT224 untouched
>>>> 2. Remove logo_mac_clut224.ppm
>>>>
>>>> The first option is less controversial but I would like to ask you what
>>>> you think about removing the logo_mac_clut224 file.
>>>>
>>>> Here, we are speaking of the Macintosh 68k which ended sales in 1995,
>>>> right? So the user base should be rather small, I guess.
>>>
>>> Yes, the user base is small.
>>>
>>> BTW, the only reason you don't have this issue with MACH_DECSTATION and
>>> the various SGI_IP* options is that MIPS does not support multi-platform
>>> kernels.
>>>
>>>> And people who still want the custom MAC logo would still be able to add
>>>>
>>>> CONFIG_LOGO_MAC_CLUT224="path/to/logo_mac_clut224.ppm"
>>>
>>> LOGO_LINUX_CLUT224_FILE ;-)
D'oh!
>>>> to their config to restore the old behaviour anyway.
>>>>
>>>> My choice would go more toward the removal option but what do you think?
>>>
>>> I am not too attached to keeping the dynamic behavior for the Mac logo,
>>> I just wanted to point out the impact.
>>> I expect most people who care about logos (in products) just have their
>>> own custom out-of-tree code. As fb_find_logo() and the underlying
>>> infrastructure still exists, I don't expect them to have too much
>>> trouble forward porting that to newer kernels.
>>>
>>> What do other people think?
>>
>> This is about a small visible icon. It's not some relevant feature.
>> So, I think it's unfortunate that the patch then drops the specific mac logo.
>> But adding additional coding and complexity to simply make this logo
>> visible for such a small user base IMHO does not justify the effort.
>
> This patch does not drop the specific Mac logo.
> Instead, it prioritizes the Mac logo over the generic logo when Mac
> support is enabled in a multi-platform kernel.
It seems that there is a consensus between the three of us that we can
drop the MAC logo.
I will send right away a v3 with will contain one extra patch to drop
the Macintosh logo with a note in the patch of how to restore it using
LOGO_LINUX_CLUT224_FILE.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply
* Re: [PATCH 1/8] Add Advantech EIO MFD driver
From: Lee Jones @ 2026-01-08 14:45 UTC (permalink / raw)
To: Ramiro Oliveira
Cc: Linus Walleij, Bartosz Golaszewski, Guenter Roeck, Andi Shyti,
Daniel Thompson, Jingoo Han, Helge Deller, Wim Van Sebroeck,
Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
linux-kernel, linux-gpio, linux-hwmon, linux-i2c, dri-devel,
linux-fbdev, linux-watchdog, linux-pm, Wenkai Chung,
Francisco Aragon-Trivino, Hongzhi Wang, Mikhail Tsukerman,
Thomas Kastner
In-Reply-To: <20251212-upstream-v1-v1-1-d50d40ec8d8a@advantech.com>
On Fri, 12 Dec 2025, Ramiro Oliveira wrote:
> Creating the MFD core driver for Advantech EIO, all other drivers (GPIO,
> I2C, etc) depend on this core driver.
You're going to have to come up with a MUCH better commit message than
that for 800 line driver!
> Signed-off-by: Ramiro Oliveira <ramiro.oliveira@advantech.com>
> ---
> MAINTAINERS | 6 +
> drivers/mfd/Kconfig | 10 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/eio_core.c | 621 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/eio.h | 127 ++++++++++
> 5 files changed, 765 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 663e86eb9ff1..bd9279796c2f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -616,6 +616,12 @@ L: platform-driver-x86@vger.kernel.org
> S: Maintained
> F: drivers/platform/x86/adv_swbutton.c
>
> +ADVANTECH EIO DRIVER
> +M: Ramiro Oliveira <ramiro.oliveira@advantech.com>
> +S: Maintained
> +F: drivers/mfd/eio_core.c
> +F: include/linux/mfd/eio.h
> +
> ADXL313 THREE-AXIS DIGITAL ACCELEROMETER DRIVER
> M: Lucas Stankus <lucas.p.stankus@gmail.com>
> S: Supported
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index aace5766b38a..02a0b324eb6a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -506,6 +506,16 @@ config MFD_DLN2
> etc. must be enabled in order to use the functionality of
> the device.
>
> +config MFD_EIO
> + tristate "Advantech EIO MFD core"
Drop the term MFD, it doesn't mean anything. We made it up.
What is this device?
> + select MFD_CORE
> + help
> + This enables support for the Advantech EIO multi-function device.
Remove all mentions of MFD.
> + This core driver provides register access and coordination for the
> + EIO's subdevices (GPIO, watchdog, hwmon, thermal, backlight, I2C).
> + This driver supports EIO-IS200, EIO-201, EIO-210 and EIO-211.
Which are?
> +
> +
> config MFD_ENE_KB3930
> tristate "ENE KB3930 Embedded Controller support"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e75e8045c28a..f8c53b55b679 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_MFD_CROS_EC_DEV) += cros_ec_dev.o
> obj-$(CONFIG_MFD_CS42L43) += cs42l43.o
> obj-$(CONFIG_MFD_CS42L43_I2C) += cs42l43-i2c.o
> obj-$(CONFIG_MFD_CS42L43_SDW) += cs42l43-sdw.o
> +obj-$(CONFIG_MFD_EIO) += eio_core.o
> obj-$(CONFIG_MFD_ENE_KB3930) += ene-kb3930.o
> obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o
> obj-$(CONFIG_MFD_GATEWORKS_GSC) += gateworks-gsc.o
> diff --git a/drivers/mfd/eio_core.c b/drivers/mfd/eio_core.c
> new file mode 100644
> index 000000000000..7a58c62595a5
> --- /dev/null
> +++ b/drivers/mfd/eio_core.c
> @@ -0,0 +1,621 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Advantech Embedded Controller base Driver
> + *
> + * This driver provides an interface to access the EIO Series EC
> + * firmware via its own Power Management Channel (PMC) for subdrivers:
':' without follow-up looks odd.
> + * A system may have one or two independent EIO devices.
> + *
> + * Copyright (C) 2025 Advantech Co., Ltd.
This needs updating on the next iteration.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/isa.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/sysfs.h>
> +#include <linux/time.h>
> +#include <linux/uaccess.h>
> +#include <linux/version.h>
> +#include <linux/mfd/eio.h>
Alphabetical.
Can you make sure that _all_ of these are in use.
> +#define TIMEOUT_MAX (10 * USEC_PER_SEC)
> +#define TIMEOUT_MIN 200
> +#define SLEEP_MAX 200
> +#define DEFAULT_TIMEOUT 5000
Tab these out.
Are these values arbitrary or do they come from some spec?
> +
> +/**
Why are you using kernel-doc comments here?
Did you compile with W=1?
> + * Timeout: Default timeout in microseconds when a PMC command's
> + * timeout is unspecified. PMC command responses typically range
> + * from 200us to 2ms. 5ms is quite a safe value for timeout. In
Superfluous "In".
> + * In some cases, responses are longer. In such situations, please
In what cases?
> + * adding the timeout parameter loading related sub-drivers or
> + * this core driver (not recommended).
I can't read this.
> + */
> +static uint timeout = DEFAULT_TIMEOUT;
> +module_param(timeout, uint, 0444);
> +MODULE_PARM_DESC(timeout, "Default PMC command timeout in usec.\n");
You want the user to override the timeout? Are you sure?
> +struct eio_dev_port {
> + u16 idx_port;
> + u16 data_port;
> +};
> +
> +static struct eio_dev_port pnp_port[] = {
> + { .idx_port = EIO_PNP_INDEX, .data_port = EIO_PNP_DATA },
> + { .idx_port = EIO_SUB_PNP_INDEX,
> + .data_port = EIO_SUB_PNP_DATA },
Either place this on the line above or use proper multi-line format.
> +};
> +
> +static struct mfd_cell mfd_devs[] = {
eio_devs
> + { .name = "eio_wdt" },
> + { .name = "gpio_eio" },
> + { .name = "eio_hwmon" },
> + { .name = "i2c_eio" },
> + { .name = "eio_thermal" },
> + { .name = "eio_fan" },
> + { .name = "eio_bl" },
MFD_CELL_NAME()
> +};
> +
> +static const struct regmap_range eio_range[] = {
> + regmap_reg_range(EIO_PNP_INDEX, EIO_PNP_DATA),
> + regmap_reg_range(EIO_SUB_PNP_INDEX, EIO_SUB_PNP_DATA),
> + regmap_reg_range(0x200, 0x3FF),
> +};
> +
> +static const struct regmap_access_table volatile_regs = {
> + .yes_ranges = eio_range,
> + .n_yes_ranges = ARRAY_SIZE(eio_range),
> +};
> +
> +static const struct regmap_config pnp_regmap_config = {
> + .name = "eio_core",
> + .reg_bits = 16,
> + .val_bits = 8,
> + .volatile_table = &volatile_regs,
> + .io_port = true,
> + .cache_type = REGCACHE_NONE,
> +};
> +
> +static struct {
> + char name[32];
> + int cmd;
> + int ctrl;
> + int dev;
> + int size;
> + enum {
> + HEX,
> + NUMBER,
> + PNP_ID,
> + } type;
> +
Remove this line.
> +} attrs[] = {
> + { "board_name", 0x53, 0x10, 0, 16 },
> + { "board_serial", 0x53, 0x1F, 0, 16 },
> + { "board_manufacturer", 0x53, 0x11, 0, 16 },
> + { "board_id", 0x53, 0x1E, 0, 4 },
> + { "firmware_version", 0x53, 0x21, 0, 4 },
> + { "firmware_name", 0x53, 0x22, 0, 16 },
> + { "firmware_build", 0x53, 0x23, 0, 26 },
> + { "firmware_date", 0x53, 0x24, 0, 16 },
> + { "chip_id", 0x53, 0x12, 0, 12 },
> + { "chip_detect", 0x53, 0x15, 0, 12 },
> + { "platform_type", 0x53, 0x13, 0, 16 },
> + { "platform_revision", 0x53, 0x04, 0x44, 4 },
> + { "eapi_version", 0x53, 0x04, 0x64, 4 },
> + { "eapi_id", 0x53, 0x31, 0, 4 },
> + { "boot_count", 0x55, 0x10, 0, 4, NUMBER },
> + { "powerup_hour", 0x55, 0x11, 0, 4, NUMBER },
> + { "pnp_id", 0x53, 0x04, 0x68, 4, PNP_ID },
> +};
As "fun" as all of these sysfs entries are, how useful are they to you?
Can you say in good conscience that they are all in active use?
> +static ssize_t info_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
Use 100-chars to avoid these line-wraps.
> +{
> + uint i;
> +
> + for (i = 0; i < ARRAY_SIZE(attrs); i++) {
> + int ret;
> + char str[32] = "";
> + int val;
> +
> + struct pmc_op op = {
> + .cmd = attrs[i].cmd,
> + .control = attrs[i].ctrl,
> + .device_id = attrs[i].dev,
> + .payload = (u8 *)str,
> + .size = attrs[i].size,
> + };
> +
> + if (strcmp(attr->attr.name, attrs[i].name))
> + continue;
> +
> + ret = eio_core_pmc_operation(dev, &op);
> + if (ret)
> + return ret;
> +
> + if (attrs[i].size != 4)
Is this dictated by the user?
> + return sprintf(buf, "%s\n", str);
sprintf() is unsafe. Use sysfs_emit() instead. Throughout.
> + val = *(u32 *)str;
> +
> + if (attrs[i].type == HEX)
> + return sprintf(buf, "0x%08X\n", val);
> +
> + if (attrs[i].type == NUMBER)
> + return sprintf(buf, "%d\n", val);
> +
> + /* Should be pnp_id */
"Should"?
Not good enough. Why not check for PNP_ID instead?
> + return sprintf(buf, "%c%c%c, %X\n", (val >> 14 & 0x3F) + 0x40,
> + ((val >> 9 & 0x18) | (val >> 25 & 0x07)) + 0x40,
> + (val >> 20 & 0x1F) + 0x40, val & 0xFFF);
> + }
> +
> + return -EINVAL;
> +}
> +
> +#define PMC_DEVICE_ATTR_RO(_name) \
> + static ssize_t _name##_show(struct device *dev, \
> + struct device_attribute *attr, char *buf) \
> + { \
> + return info_show(dev, attr, buf); \
> + } \
> + static DEVICE_ATTR_RO(_name)
Place this out the way, in a header file.
> +PMC_DEVICE_ATTR_RO(board_name);
> +PMC_DEVICE_ATTR_RO(board_serial);
> +PMC_DEVICE_ATTR_RO(board_manufacturer);
> +PMC_DEVICE_ATTR_RO(firmware_name);
> +PMC_DEVICE_ATTR_RO(firmware_version);
> +PMC_DEVICE_ATTR_RO(firmware_build);
> +PMC_DEVICE_ATTR_RO(firmware_date);
> +PMC_DEVICE_ATTR_RO(chip_id);
> +PMC_DEVICE_ATTR_RO(chip_detect);
> +PMC_DEVICE_ATTR_RO(platform_type);
> +PMC_DEVICE_ATTR_RO(platform_revision);
> +PMC_DEVICE_ATTR_RO(board_id);
> +PMC_DEVICE_ATTR_RO(eapi_version);
> +PMC_DEVICE_ATTR_RO(eapi_id);
> +PMC_DEVICE_ATTR_RO(boot_count);
> +PMC_DEVICE_ATTR_RO(powerup_hour);
> +PMC_DEVICE_ATTR_RO(pnp_id);
> +
> +static struct attribute *pmc_attrs[] = { &dev_attr_board_name.attr,
The attribute goes on a new line, then you can reign in all of the
crazy tabbing that follows.
> + &dev_attr_board_serial.attr,
> + &dev_attr_board_manufacturer.attr,
> + &dev_attr_firmware_name.attr,
> + &dev_attr_firmware_version.attr,
> + &dev_attr_firmware_build.attr,
> + &dev_attr_firmware_date.attr,
> + &dev_attr_chip_id.attr,
> + &dev_attr_chip_detect.attr,
> + &dev_attr_platform_type.attr,
> + &dev_attr_platform_revision.attr,
> + &dev_attr_board_id.attr,
> + &dev_attr_eapi_version.attr,
> + &dev_attr_eapi_id.attr,
> + &dev_attr_boot_count.attr,
> + &dev_attr_powerup_hour.attr,
> + &dev_attr_pnp_id.attr,
> + NULL };
> +
> +ATTRIBUTE_GROUPS(pmc);
> +
> +static unsigned int eio_pnp_read(struct device *dev,
> + struct eio_dev_port *port, u8 idx)
100-chars throughout.
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + unsigned int val;
> +
> + if (regmap_write(eio->map, port->idx_port, idx))
> + dev_err(dev, "Error port write 0x%X\n", port->idx_port);
> +
> + if (regmap_read(eio->map, port->data_port, &val))
> + dev_err(dev, "Error port read 0x%X\n", port->data_port);
> +
> + return val;
> +}
> +
> +static void eio_pnp_write(struct device *dev, struct eio_dev_port *port,
Why are these functions not propagating errors?
> + u8 idx, u8 data)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> +
> + if (regmap_write(eio->map, port->idx_port, idx) ||
> + regmap_write(eio->map, port->data_port, data))
> + dev_err(dev, "Error port write 0x%X %X\n", port->idx_port,
> + port->data_port);
You cannot print and error, then return like everything is okay.
> +}
> +
> +static void eio_pnp_enter(struct device *dev, struct eio_dev_port *port)
_unlock_port()
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
'\n'
> + /* Write 0x87 to index port twice to unlock IO port */
> + if (regmap_write(eio->map, port->idx_port,
> + EIO_EXT_MODE_ENTER) ||
This does on the line above.
> + regmap_write(eio->map, port->idx_port, EIO_EXT_MODE_ENTER))
> + dev_err(dev, "Error port write 0x%X\n", port->idx_port);
> +}
> +
> +static void eio_pnp_leave(struct device *dev, struct eio_dev_port *port)
_lock_port()
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
'\n'
> + /* Write 0xAA to index port once to lock IO port */
> + if (regmap_write(eio->map, port->idx_port, EIO_EXT_MODE_EXIT))
> + dev_err(dev, "Error port write 0x%X\n", port->idx_port);
> +}
What's the difference between these eio_ calls and the pmc_ ones below?
Please find a way to make that clear - a header comment?
> +static int pmc_write_data(struct device *dev, int id, u8 value, u16 timeout)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + int ret;
> +
> + if (WAIT_IBF(dev, id, timeout))
This is not a macro.
Just use eio_core_pmc_wait() and have done.
> + return -ETIME;
I think you mean -ETIMEDOUT, throughout.
Also, eio_core_pmc_wait() returns its own error value which you are now
overwriting. Why not simply propagate the original error?
> +
> + ret = regmap_write(eio->map, eio->pmc[id].data, value);
> + if (ret)
> + dev_err(dev, "Error PMC write %X:%X\n",
> + eio->pmc[id].data, value);
> +
> + return ret;
> +}
> +
> +static int pmc_write_cmd(struct device *dev, int id, u8 value, u16 timeout)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + int ret;
> +
> + if (WAIT_IBF(dev, id, timeout))
> + return -ETIME;
> +
> + ret = regmap_write(eio->map, eio->pmc[id].cmd, value);
> + if (ret)
> + dev_err(dev, "Error PMC write %X:%X\n",
> + eio->pmc[id].cmd, value);
> +
> + return ret;
> +}
> +
> +static int pmc_read_data(struct device *dev, int id, u8 *value, u16 timeout)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + unsigned int val;
> + int ret;
> +
> + if (WAIT_OBF(dev, id, timeout))
> + return -ETIME;
> +
> + ret = regmap_read(eio->map, eio->pmc[id].data, &val);
> + if (ret)
> + dev_err(dev, "Error PMC read %X\n", eio->pmc[id].data);
> + else
> + *value = (u8)(val & 0xFF);
> +
> + return ret;
> +}
> +
> +static int pmc_read_status(struct device *dev, int id)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + unsigned int val;
> +
> + if (regmap_read(eio->map, eio->pmc[id].status, &val)) {
> + dev_err(dev, "Error PMC read %X\n",
> + eio->pmc[id].status);
> + return 0;
> + }
> +
> + return val;
> +}
> +
> +static void pmc_clear(struct device *dev, int id)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + unsigned int val;
> +
> + /* Check if input buffer blocked */
> + if ((pmc_read_status(dev, id) & EIO_PMC_STATUS_IBF) == 0)
> + return;
> +
> + /* Read out previous garbage */
> + if (regmap_read(eio->map, eio->pmc[id].data, &val))
> + dev_err(dev, "Error pmc clear\n");
What do you expect the user to do about this?
Why is it an issue that there is nothing to read?
> +
> + usleep_range(10, 100);
> +}
> +
> +int eio_core_pmc_wait(struct device *dev, int id,
> + enum eio_pmc_wait wait, uint max_duration)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + uint val;
> + int new_timeout = max_duration ? max_duration : timeout;
max_duration?: timeout;
> +
> + if (new_timeout < TIMEOUT_MIN || new_timeout > TIMEOUT_MAX) {
> + dev_err(dev,
> + "Error timeout value: %dus. Timeout value should between %d and %ld\n",
Suggest that the user should not specify a timeout, then all of this can go.
> + new_timeout, TIMEOUT_MIN, TIMEOUT_MAX);
> + return -ETIME;
> + }
> +
> + if (wait == PMC_WAIT_INPUT)
> + return regmap_read_poll_timeout(eio->map, eio->pmc[id].status,
> + val, (val & EIO_PMC_STATUS_IBF) == 0,
> + SLEEP_MAX, new_timeout);
> + return regmap_read_poll_timeout(eio->map,
> + eio->pmc[id].status, val,
> + (val & EIO_PMC_STATUS_OBF) != 0,
> + SLEEP_MAX, new_timeout);
These stacked timeouts are going to need some explanation.
> +}
> +EXPORT_SYMBOL_GPL(eio_core_pmc_wait);
> +
> +int eio_core_pmc_operation(struct device *dev, struct pmc_op *op)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + u8 i;
> + int ret;
> + bool read_cmd = op->cmd & EIO_FLAG_PMC_READ;
Suggest a rename.
read_cmd sounds more like a function call or command value rather than a
bool to match on.
What about "reading".
> + ktime_t t = ktime_get();
Nit: Reverse Christmas tree is kinder on the reader.
> + mutex_lock(&eio->mutex);
> +
> + pmc_clear(dev, op->chip);
> +
> + ret = pmc_write_cmd(dev, op->chip, op->cmd, op->timeout);
Why not just provide "op" and let the callee extract what it needs?
> + if (ret)
> + goto err;
> +
> + ret = pmc_write_data(dev, op->chip, op->control, op->timeout);
> + if (ret)
> + goto err;
> +
> + ret = pmc_write_data(dev, op->chip, op->device_id, op->timeout);
> + if (ret)
> + goto err;
> +
> + ret = pmc_write_data(dev, op->chip, op->size, op->timeout);
> + if (ret)
> + goto err;
> +
> + for (i = 0; i < op->size; i++) {
> + if (read_cmd)
> + ret = pmc_read_data(dev, op->chip, &op->payload[i],
> + op->timeout);
> + else
> + ret = pmc_write_data(dev, op->chip, op->payload[i],
> + op->timeout);
> +
> + if (ret)
> + goto err;
Why not break, unlock, then return 0 if (!ret).
> + }
> +
> + mutex_unlock(&eio->mutex);
> +
> + return 0;
> +
> +err:
> + mutex_unlock(&eio->mutex);
> +
> + dev_err(dev, "PMC error duration:%lldus",
> + ktime_to_us(ktime_sub(ktime_get(), t)));
Who is this helpful to?
> + dev_err(dev,
> + ".cmd=0x%02X, .ctrl=0x%02X .id=0x%02X, .size=0x%02X .data=0x%02X%02X",
> + op->cmd, op->control, op->device_id, op->size, op->payload[0],
> + op->payload[1]);
This looks like debug crud that can be removed when the driver is published.
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eio_core_pmc_operation);
> +
> +static int get_pmc_port(struct device *dev, int id,
This does not tell me what the function does.
Please improve the nomenclature.
> + struct eio_dev_port *port)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + struct _pmc_port *pmc = &eio->pmc[id];
> +
> + eio_pnp_enter(dev, port);
> +
> + /* Switch to PMC device page */
> + eio_pnp_write(dev, port, EIO_LDN, EIO_LDN_PMC1);
> +
> + /* Active this device */
> + eio_pnp_write(dev, port, EIO_LDAR, EIO_LDAR_LDACT);
> +
> + /* Get PMC cmd and data port */
> + pmc->data = eio_pnp_read(dev, port, EIO_IOBA0H) << 8;
> + pmc->data |= eio_pnp_read(dev, port, EIO_IOBA0L);
> + pmc->cmd = eio_pnp_read(dev, port, EIO_IOBA1H) << 8;
> + pmc->cmd |= eio_pnp_read(dev, port, EIO_IOBA1L);
> +
> + /* Disable IRQ */
> + eio_pnp_write(dev, port, EIO_IRQCTRL, 0);
> +
> + eio_pnp_leave(dev, port);
> +
> + /* Make sure IO ports are not occupied */
> + if (!devm_request_region(dev, pmc->data, 2, KBUILD_MODNAME)) {
Break the call out of the if please.
> + dev_err(dev, "Request region %X error\n", pmc->data);
> + return -EBUSY;
> + }
> +
> + return 0;
> +}
> +
> +static int eio_init(struct device *dev)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + u16 chip_id = 0;
> + u8 tmp = 0;
control
> + int chip = 0;
Why are all of these being pre-initialised?
> + int ret = -ENOMEM;
> +
> + for (chip = 0; chip < ARRAY_SIZE(pnp_port); chip++) {
for (int chip_id ...)
Hold on, you have chip_id below.
What's the difference between chip and chip_id?
> + struct eio_dev_port *port = pnp_port + chip;
> +
> + if (!devm_request_region(dev, pnp_port[chip].idx_port,
Break this out.
> + pnp_port[chip].data_port -
> + pnp_port[chip].idx_port,
> + KBUILD_MODNAME))
> + continue;
> +
> + eio_pnp_enter(dev, port);
> +
> + chip_id = eio_pnp_read(dev, port, EIO_CHIPID1) << 8;
> + chip_id |= eio_pnp_read(dev, port, EIO_CHIPID2);
> +
> + if (chip_id != EIO200_CHIPID && chip_id != EIO201_211_CHIPID)
> + continue;
> +
> + /* Turn on the enable flag */
> + tmp = eio_pnp_read(dev, port, EIO_SIOCTRL);
> + tmp |= EIO_SIOCTRL_SIOEN;
> +
> + eio_pnp_write(dev, port, EIO_SIOCTRL, tmp);
> +
> + eio_pnp_leave(dev, port);
> +
> + ret = get_pmc_port(dev, chip, port);
> + if (ret)
> + return ret;
> +
> + if (chip == 0)
> + eio->flag |= EIO_F_CHIP_EXIST;
> + else
> + eio->flag |= EIO_F_SUB_CHIP_EXIST;
> + }
> +
> + return ret;
> +}
> +
> +static uint8_t acpiram_access(struct device *dev, uint8_t offset)
What's actually happening here?
Should that be "acpi_ram"
> +{
> + u8 val;
> + int ret;
> + int timeout = 0;
> + struct eio_dev *eio = dev_get_drvdata(dev);
> +
> + /* We only store information on primary EC */
> + int chip = 0;
> +
> + mutex_lock(&eio->mutex);
> +
> + pmc_clear(dev, chip);
> +
> + ret = pmc_write_cmd(dev, chip, EIO_PMC_CMD_ACPIRAM_READ, timeout);
Isn't timeout always 0?
> + if (ret)
> + goto err;
> +
> + ret = pmc_write_data(dev, chip, offset, timeout);
> + if (ret)
> + goto err;
> +
> + ret = pmc_write_data(dev, chip, sizeof(val), timeout);
> + if (ret)
> + goto err;
> +
> + ret = pmc_read_data(dev, chip, &val, timeout);
> + if (ret)
> + goto err;
> +
> +err:
> + mutex_unlock(&eio->mutex);
> + return ret ? 0 : val;
What? No. Return the error.
> +}
> +
> +static int firmware_code_base(struct device *dev)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + u8 ic_vendor, ic_code, code_base;
> +
> + ic_vendor = acpiram_access(dev, EIO_ACPIRAM_ICVENDOR);
> + ic_code = acpiram_access(dev, EIO_ACPIRAM_ICCODE);
> + code_base = acpiram_access(dev, EIO_ACPIRAM_CODEBASE);
> +
> + if (ic_vendor != 'R')
> + return -ENODEV;
> +
> + if (ic_code != EIO200_ICCODE && ic_code != EIO201_ICCODE &&
> + ic_code != EIO211_ICCODE)
> + goto err;
> +
> + if (code_base == EIO_ACPIRAM_CODEBASE_NEW) {
> + eio->flag |= EIO_F_NEW_CODE_BASE;
> + return 0;
> + }
> +
> + if (code_base == 0 &&
> + (ic_code != EIO201_ICCODE && ic_code != EIO211_ICCODE)) {
> + dev_info(dev, "Old code base not supported, yet.");
Drop the yet. If it becomes supported later, so be it.
> + return -ENODEV;
> + }
> +
> +err:
> + /* Codebase error. This should only happen on firmware error. */
> + dev_err(dev,
> + "Codebase check fail: vendor: 0x%X, code: 0x%X, base: 0x%X\n",
> + ic_vendor, ic_code, code_base);
> + return -ENODEV;
> +}
> +
> +static int eio_probe(struct device *dev, unsigned int id)
> +{
> + int ret = 0;
> + struct eio_dev *eio;
> +
> + eio = devm_kzalloc(dev, sizeof(*eio), GFP_KERNEL);
> + if (!eio)
> + return -ENOMEM;
> +
> + eio->dev = dev;
> + mutex_init(&eio->mutex);
> +
> + eio->iomem = devm_ioport_map(dev, 0, EIO_SUB_PNP_DATA + 1);
> + if (IS_ERR(eio->iomem))
> + return PTR_ERR(eio->iomem);
> +
> + eio->map = devm_regmap_init_mmio(dev, eio->iomem, &pnp_regmap_config);
> + if (IS_ERR(eio->map))
> + return PTR_ERR(eio->map);
> +
> + /* publish instance for subdrivers (dev_get_drvdata(dev->parent)) */
"Publish"
Actually drop this - it's not required.
> + dev_set_drvdata(dev, eio);
> +
> + if (eio_init(dev)) {
> + dev_dbg(dev, "No device found\n");
Drop all debug cruft.
> + return -ENODEV;
> + }
> +
> + ret = firmware_code_base(dev);
> + if (ret) {
> + dev_err(dev, "Chip code base check fail\n");
> + return ret; /* keep helper's return (e.g., -EIO) */
Always do this.
> + }
> +
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> + mfd_devs, ARRAY_SIZE(mfd_devs),
> + NULL, 0, NULL);
> + if (ret)
> + dev_err(dev, "Cannot register child devices (error = %d)\n", ret);
> +
> + dev_dbg(dev, "Module insert completed\n");
Drop.
> + return 0;
> +}
> +
> +static struct isa_driver eio_driver = {
> + .probe = eio_probe,
> +
Remove this line.
Nothing to remove?
> + .driver = {
> + .name = "eio_core",
> + .dev_groups = pmc_groups,
> + },
> +};
> +module_isa_driver(eio_driver, 1);
What's 1? Please define.
> +MODULE_AUTHOR("Wenkai Chung <wenkai.chung@advantech.com.tw>");
> +MODULE_AUTHOR("Ramiro Oliveira <ramiro.oliveira@advantech.com>");
> +MODULE_DESCRIPTION("Advantech EIO series EC core driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/eio.h b/include/linux/mfd/eio.h
> new file mode 100644
> index 000000000000..b87614274201
> --- /dev/null
> +++ b/include/linux/mfd/eio.h
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Header for the Advantech EIO core driver and its sub-drivers
No need for the header part. We can see that this is a header file.
> + *
> + * Copyright (C) 2025 Advantech Co., Ltd.
> + */
> +
> +#ifndef _MFD_EIO_H_
> +#define _MFD_EIO_H_
'\n'
> +#include <linux/io.h>
> +#include <linux/regmap.h>
> +
> +/* Definition */
???
> +#define EIO_CHIPID1 0x20
> +#define EIO_CHIPID2 0x21
> +#define EIO_CHIPVER 0x22
> +#define EIO_SIOCTRL 0x23
> +#define EIO_SIOCTRL_SIOEN BIT(0)
> +#define EIO_SIOCTRL_SWRST BIT(1)
> +#define EIO_IRQCTRL 0x70
> +#define EIO200_CHIPID 0x9610
> +#define EIO201_211_CHIPID 0x9620
> +#define EIO200_ICCODE 0x10
> +#define EIO201_ICCODE 0x20
> +#define EIO211_ICCODE 0x21
> +
> +/* LPC PNP */
> +#define EIO_PNP_INDEX 0x299
> +#define EIO_PNP_DATA 0x29A
> +#define EIO_SUB_PNP_INDEX 0x499
> +#define EIO_SUB_PNP_DATA 0x49A
> +#define EIO_EXT_MODE_ENTER 0x87
> +#define EIO_EXT_MODE_EXIT 0xAA
> +
> +/* LPC LDN */
> +#define EIO_LDN 0x07
> +#define EIO_LDN_PMC0 0x0C
> +#define EIO_LDN_PMC1 0x0D
> +
> +/* PMC registers */
> +#define EIO_PMC_STATUS_IBF BIT(1)
> +#define EIO_PMC_STATUS_OBF BIT(0)
> +#define EIO_LDAR 0x30
> +#define EIO_LDAR_LDACT BIT(0)
> +#define EIO_IOBA0H 0x60
> +#define EIO_IOBA0L 0x61
> +#define EIO_IOBA1H 0x62
> +#define EIO_IOBA1L 0x63
> +#define EIO_FLAG_PMC_READ BIT(0)
> +
> +/* PMC command list */
> +#define EIO_PMC_CMD_ACPIRAM_READ 0x31
> +#define EIO_PMC_CMD_CFG_SAVE 0x56
> +
> +/* OLD PMC */
> +#define EIO_PMC_NO_INDEX 0xFF
> +
> +/* ACPI RAM Address Table */
> +#define EIO_ACPIRAM_VERSIONSECTION (0xFA)
> +#define EIO_ACPIRAM_ICVENDOR (EIO_ACPIRAM_VERSIONSECTION + 0x00)
> +#define EIO_ACPIRAM_ICCODE (EIO_ACPIRAM_VERSIONSECTION + 0x01)
> +#define EIO_ACPIRAM_CODEBASE (EIO_ACPIRAM_VERSIONSECTION + 0x02)
> +
> +#define EIO_ACPIRAM_CODEBASE_NEW BIT(7)
> +
> +/* Firmware */
> +#define EIO_F_SUB_NEW_CODE_BASE BIT(6)
> +#define EIO_F_SUB_CHANGED BIT(7)
> +#define EIO_F_NEW_CODE_BASE BIT(8)
> +#define EIO_F_CHANGED BIT(9)
> +#define EIO_F_SUB_CHIP_EXIST BIT(30)
> +#define EIO_F_CHIP_EXIST BIT(31)
> +
> +/* Others */
> +#define EIO_EC_NUM 2
> +
> +struct _pmc_port {
> + union {
> + u16 cmd;
> + u16 status;
> + };
> + u16 data;
> +};
> +
> +struct pmc_op {
> + u8 cmd;
> + u8 control;
> + u8 device_id;
> + u8 size;
> + u8 *payload;
> + u8 chip;
> + u16 timeout;
> +};
> +
> +enum eio_rw_operation {
> + OPERATION_READ,
> + OPERATION_WRITE,
> +};
> +
> +struct eio_dev {
> + struct device *dev;
> + struct regmap *map;
> + void __iomem *iomem;
> + struct mutex mutex; /* Protects PMC command access */
> + struct _pmc_port pmc[EIO_EC_NUM];
> + u32 flag;
> +};
> +
> +int eio_core_pmc_operation(struct device *dev, struct pmc_op *operation);
> +
> +enum eio_pmc_wait {
> + PMC_WAIT_INPUT,
> + PMC_WAIT_OUTPUT,
> +};
> +
> +int eio_core_pmc_wait(struct device *dev, int id, enum eio_pmc_wait wait,
> + uint timeout);
> +
> +#define WAIT_IBF(dev, id, timeout) eio_core_pmc_wait(dev, id, PMC_WAIT_INPUT, timeout)
> +#define WAIT_OBF(dev, id, timeout) eio_core_pmc_wait(dev, id, PMC_WAIT_OUTPUT, timeout)
> +
> +#ifdef pr_fmt
> +#undef pr_fmt
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +#endif
Does this really do anything valuable?
> +
> +#endif
>
> --
> 2.43.0
>
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH v2 2/7] backlight: qcom-wled: Support ovp values for PMI8994
From: Daniel Thompson @ 2026-01-08 11:28 UTC (permalink / raw)
To: Barnabás Czémán
Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Kiran Gunda,
Helge Deller, Luca Weiss, Konrad Dybcio, Eugene Lepshy,
Gianluca Boiano, Alejandro Tafalla, dri-devel, linux-leds,
devicetree, linux-kernel, Daniel Thompson, linux-arm-msm,
linux-fbdev, Konrad Dybcio
In-Reply-To: <20260108-pmi8950-wled-v2-2-8687f23147d7@mainlining.org>
On Thu, Jan 08, 2026 at 04:43:20AM +0100, Barnabás Czémán wrote:
> WLED4 found in PMI8994 supports different ovp values.
>
> Fixes: 6fc632d3e3e0 ("video: backlight: qcom-wled: Add PMI8994 compatible")
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> ---
> drivers/video/backlight/qcom-wled.c | 41 +++++++++++++++++++++++++++++++++++--
> 1 file changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index a63bb42c8f8b..5decbd39b789 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1244,6 +1244,15 @@ static const struct wled_var_cfg wled4_ovp_cfg = {
> .size = ARRAY_SIZE(wled4_ovp_values),
> };
>
> +static const u32 pmi8994_wled_ovp_values[] = {
> + 31000, 29500, 19400, 17800,
> +};
> +
> +static const struct wled_var_cfg pmi8994_wled_ovp_cfg = {
> + .values = pmi8994_wled_ovp_values,
> + .size = ARRAY_SIZE(pmi8994_wled_ovp_values),
> +};
> +
Do these *have* to be named after one of the two PMICs that implement
this OVP range.
Would something like wled4_alternative_ovp_values[] (and the same
throughout the patch) be more descriptive?
Daniel.
^ permalink raw reply
* Re: (subset) [PATCH v6 1/2] backlight: Add Congatec Board Controller (CGBC) backlight support
From: Lee Jones @ 2026-01-08 11:15 UTC (permalink / raw)
To: Thomas Richard, Lee Jones, Daniel Thompson, Jingoo Han,
Helge Deller, Petri Karhula
Cc: linux-kernel, dri-devel, linux-fbdev
In-Reply-To: <20251205-cgbc-backlight-v6-1-e4175b0bf406@novatron.fi>
On Fri, 05 Dec 2025 12:19:47 +0000, Petri Karhula wrote:
> This driver provides backlight brightness control through the Linux
> backlight subsystem. It communicates with the board controller to
> adjust LCD backlight using PWM signals. Communication is done
> through Congatec Board Controller core driver.
>
>
Applied, thanks!
[1/2] backlight: Add Congatec Board Controller (CGBC) backlight support
commit: b516456e9f916070fde6aa50cad5680a689687e6
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH v2 3/7] dt-bindings: backlight: qcom-wled: Document ovp values for PMI8950
From: Krzysztof Kozlowski @ 2026-01-08 10:17 UTC (permalink / raw)
To: Barnabás Czémán
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Kiran Gunda,
Helge Deller, Luca Weiss, Konrad Dybcio, Eugene Lepshy,
Gianluca Boiano, Alejandro Tafalla, dri-devel, linux-leds,
devicetree, linux-kernel, Daniel Thompson, linux-arm-msm,
linux-fbdev, Konrad Dybcio
In-Reply-To: <20260108-pmi8950-wled-v2-3-8687f23147d7@mainlining.org>
On Thu, Jan 08, 2026 at 04:43:21AM +0100, Barnabás Czémán wrote:
> Document ovp values supported by wled found in PMI8950.
>
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> ---
> Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH] staging: fbtft: align tinylcd write_reg args
From: Chaitanya Mishra @ 2026-01-08 10:10 UTC (permalink / raw)
To: gregkh; +Cc: andy, dri-devel, linux-fbdev, linux-staging, linux-kernel
In-Reply-To: <20260108094709.92561-1-chaitanyamishra.ai@gmail.com>
Thanks Greg,
Agreed. This change does not improve readability, and checkpatch is only a guide.
I will drop this patch.
Regards,
Chaitanya
^ permalink raw reply
* Re: [PATCH v2 1/7] dt-bindings: backlight: qcom-wled: Document ovp values for PMI8994
From: Krzysztof Kozlowski @ 2026-01-08 10:09 UTC (permalink / raw)
To: Barnabás Czémán
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Kiran Gunda,
Helge Deller, Luca Weiss, Konrad Dybcio, Eugene Lepshy,
Gianluca Boiano, Alejandro Tafalla, dri-devel, linux-leds,
devicetree, linux-kernel, Daniel Thompson, linux-arm-msm,
linux-fbdev, Konrad Dybcio
In-Reply-To: <20260108-pmi8950-wled-v2-1-8687f23147d7@mainlining.org>
On Thu, Jan 08, 2026 at 04:43:19AM +0100, Barnabás Czémán wrote:
> Document ovp values supported by wled found in PMI8994.
>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> ---
> .../bindings/leds/backlight/qcom-wled.yaml | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH] staging: fbtft: align tinylcd write_reg args
From: Greg KH @ 2026-01-08 9:57 UTC (permalink / raw)
To: Chaitanya Mishra
Cc: andy, dri-devel, linux-fbdev, linux-staging, linux-kernel
In-Reply-To: <20260108094709.92561-1-chaitanyamishra.ai@gmail.com>
On Thu, Jan 08, 2026 at 03:17:09PM +0530, Chaitanya Mishra wrote:
> Checkpatch reports a misaligned continuation line in the
>
> fb_tinylcd init_display() write_reg() gamma table. Align the
>
> continuation line with the open parenthesis to match kernel style.
>
> No functional change.
>
> Signed-off-by: Chaitanya Mishra <chaitanyamishra.ai@gmail.com>
> ---
> drivers/staging/fbtft/fb_tinylcd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/fbtft/fb_tinylcd.c b/drivers/staging/fbtft/fb_tinylcd.c
> index 9469248f2c50..60cda57bcb33 100644
> --- a/drivers/staging/fbtft/fb_tinylcd.c
> +++ b/drivers/staging/fbtft/fb_tinylcd.c
> @@ -38,7 +38,7 @@ static int init_display(struct fbtft_par *par)
> write_reg(par, 0xE5, 0x00);
> write_reg(par, 0xF0, 0x36, 0xA5, 0x53);
> write_reg(par, 0xE0, 0x00, 0x35, 0x33, 0x00, 0x00, 0x00,
> - 0x00, 0x35, 0x33, 0x00, 0x00, 0x00);
> + 0x00, 0x35, 0x33, 0x00, 0x00, 0x00);
That kind of makes the code harder to read, right?
checkpatch is a guide, always use it as a hint, not as a rule.
thanks,
greg k-h
^ permalink raw reply
* [PATCH] staging: fbtft: align tinylcd write_reg args
From: Chaitanya Mishra @ 2026-01-08 9:47 UTC (permalink / raw)
To: andy, gregkh
Cc: dri-devel, linux-fbdev, linux-staging, linux-kernel,
chaitanyamishra.ai
Checkpatch reports a misaligned continuation line in the
fb_tinylcd init_display() write_reg() gamma table. Align the
continuation line with the open parenthesis to match kernel style.
No functional change.
Signed-off-by: Chaitanya Mishra <chaitanyamishra.ai@gmail.com>
---
drivers/staging/fbtft/fb_tinylcd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/fbtft/fb_tinylcd.c b/drivers/staging/fbtft/fb_tinylcd.c
index 9469248f2c50..60cda57bcb33 100644
--- a/drivers/staging/fbtft/fb_tinylcd.c
+++ b/drivers/staging/fbtft/fb_tinylcd.c
@@ -38,7 +38,7 @@ static int init_display(struct fbtft_par *par)
write_reg(par, 0xE5, 0x00);
write_reg(par, 0xF0, 0x36, 0xA5, 0x53);
write_reg(par, 0xE0, 0x00, 0x35, 0x33, 0x00, 0x00, 0x00,
- 0x00, 0x35, 0x33, 0x00, 0x00, 0x00);
+ 0x00, 0x35, 0x33, 0x00, 0x00, 0x00);
write_reg(par, MIPI_DCS_SET_PIXEL_FORMAT, 0x55);
write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE);
udelay(250);
--
2.50.1 (Apple Git-155)
^ permalink raw reply related
* Re: [PATCH v2 1/7] dt-bindings: backlight: qcom-wled: Document ovp values for PMI8994
From: barnabas.czeman @ 2026-01-08 9:30 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Kiran Gunda,
Helge Deller, Luca Weiss, Konrad Dybcio, Eugene Lepshy,
Gianluca Boiano, Alejandro Tafalla, dri-devel, linux-leds,
devicetree, linux-kernel, Daniel Thompson, linux-arm-msm,
linux-fbdev
In-Reply-To: <52778327-69bb-4f6c-8d64-094f33809480@oss.qualcomm.com>
On 2026-01-08 10:22, Konrad Dybcio wrote:
> On 1/8/26 10:17 AM, barnabas.czeman@mainlining.org wrote:
>> On 2026-01-08 09:54, Konrad Dybcio wrote:
>>> On 1/8/26 4:43 AM, Barnabás Czémán wrote:
>>>> Document ovp values supported by wled found in PMI8994.
>>>>
>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>
>>> I reviewed a version of this that said PMI8950, which is very much
>>> not the same..
>>>
>>> Let me try and get some more docs to confirm or deny what you're
>>> saying..
>> I have sent it for the previous revision msm-4.4 contains the
>> documentation.
>> https://lore.kernel.org/all/95f7c89fdf9b856f567a498cc56ecfa5@mainlining.org/T/#u
>
> Right, but I didn't send any reply acknowledging that.
>
> On v1, my review tag meant "I concur this is the case for PMI8950"
Yes, sorry. Since they are pretty much same i have considered it is a
minor change i have only changed everything to pmi8994 because it
was introduced before pmi8950 at upstream. I will be more careful next
time.
>
> Because you carried it in v2, it appears as if I said "I concur this
> is the case for PMI8994", which is not what I then said.
>
> Konrad
^ permalink raw reply
* Re: [PATCH v2 4/7] backlight: qcom-wled: Fix ovp values for PMI8950
From: Konrad Dybcio @ 2026-01-08 9:22 UTC (permalink / raw)
To: Barnabás Czémán, Lee Jones, Daniel Thompson,
Jingoo Han, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Kiran Gunda, Helge Deller,
Luca Weiss, Konrad Dybcio, Eugene Lepshy, Gianluca Boiano,
Alejandro Tafalla
Cc: dri-devel, linux-leds, devicetree, linux-kernel, Daniel Thompson,
linux-arm-msm, linux-fbdev
In-Reply-To: <20260108-pmi8950-wled-v2-4-8687f23147d7@mainlining.org>
On 1/8/26 4:43 AM, Barnabás Czémán wrote:
> PMI8950 WLED support same ovp values like PMI8994 WLED.
>
> Fixes: 10258bf4534b ("backlight: qcom-wled: Add PMI8950 compatible")
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v2 3/7] dt-bindings: backlight: qcom-wled: Document ovp values for PMI8950
From: Konrad Dybcio @ 2026-01-08 9:22 UTC (permalink / raw)
To: Barnabás Czémán, Lee Jones, Daniel Thompson,
Jingoo Han, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Kiran Gunda, Helge Deller,
Luca Weiss, Konrad Dybcio, Eugene Lepshy, Gianluca Boiano,
Alejandro Tafalla
Cc: dri-devel, linux-leds, devicetree, linux-kernel, Daniel Thompson,
linux-arm-msm, linux-fbdev
In-Reply-To: <20260108-pmi8950-wled-v2-3-8687f23147d7@mainlining.org>
On 1/8/26 4:43 AM, Barnabás Czémán wrote:
> Document ovp values supported by wled found in PMI8950.
>
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v2 2/7] backlight: qcom-wled: Support ovp values for PMI8994
From: Konrad Dybcio @ 2026-01-08 9:22 UTC (permalink / raw)
To: Barnabás Czémán, Lee Jones, Daniel Thompson,
Jingoo Han, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Kiran Gunda, Helge Deller,
Luca Weiss, Konrad Dybcio, Eugene Lepshy, Gianluca Boiano,
Alejandro Tafalla
Cc: dri-devel, linux-leds, devicetree, linux-kernel, Daniel Thompson,
linux-arm-msm, linux-fbdev
In-Reply-To: <736c8986-7adf-446e-9720-c4a71272825b@oss.qualcomm.com>
On 1/8/26 9:55 AM, Konrad Dybcio wrote:
> On 1/8/26 4:43 AM, Barnabás Czémán wrote:
>> WLED4 found in PMI8994 supports different ovp values.
>>
>> Fixes: 6fc632d3e3e0 ("video: backlight: qcom-wled: Add PMI8994 compatible")
>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
>> ---
>
> Likewise, I reviewed a version that said PMI8950 instead.
We can keep it because PMI8994 seems to fall under the same category
indeed
Konrad
^ permalink raw reply
* Re: [PATCH v2 1/7] dt-bindings: backlight: qcom-wled: Document ovp values for PMI8994
From: Konrad Dybcio @ 2026-01-08 9:22 UTC (permalink / raw)
To: barnabas.czeman
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Kiran Gunda,
Helge Deller, Luca Weiss, Konrad Dybcio, Eugene Lepshy,
Gianluca Boiano, Alejandro Tafalla, dri-devel, linux-leds,
devicetree, linux-kernel, Daniel Thompson, linux-arm-msm,
linux-fbdev
In-Reply-To: <c7bca43b1b912a6a100d83229d78abde@mainlining.org>
On 1/8/26 10:17 AM, barnabas.czeman@mainlining.org wrote:
> On 2026-01-08 09:54, Konrad Dybcio wrote:
>> On 1/8/26 4:43 AM, Barnabás Czémán wrote:
>>> Document ovp values supported by wled found in PMI8994.
>>>
>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> I reviewed a version of this that said PMI8950, which is very much
>> not the same..
>>
>> Let me try and get some more docs to confirm or deny what you're
>> saying..
> I have sent it for the previous revision msm-4.4 contains the documentation.
> https://lore.kernel.org/all/95f7c89fdf9b856f567a498cc56ecfa5@mainlining.org/T/#u
Right, but I didn't send any reply acknowledging that.
On v1, my review tag meant "I concur this is the case for PMI8950"
Because you carried it in v2, it appears as if I said "I concur this
is the case for PMI8994", which is not what I then said.
Konrad
^ permalink raw reply
* Re: [PATCH v2 1/7] dt-bindings: backlight: qcom-wled: Document ovp values for PMI8994
From: Konrad Dybcio @ 2026-01-08 9:21 UTC (permalink / raw)
To: Barnabás Czémán, Lee Jones, Daniel Thompson,
Jingoo Han, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Kiran Gunda, Helge Deller,
Luca Weiss, Konrad Dybcio, Eugene Lepshy, Gianluca Boiano,
Alejandro Tafalla
Cc: dri-devel, linux-leds, devicetree, linux-kernel, Daniel Thompson,
linux-arm-msm, linux-fbdev
In-Reply-To: <45a2b510-c825-4191-975a-1389f4f18903@oss.qualcomm.com>
On 1/8/26 9:54 AM, Konrad Dybcio wrote:
> On 1/8/26 4:43 AM, Barnabás Czémán wrote:
>> Document ovp values supported by wled found in PMI8994.
>>
>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> I reviewed a version of this that said PMI8950, which is very much
> not the same..
>
> Let me try and get some more docs to confirm or deny what you're
> saying..
PMI8994 indeed uses the same values as PMI8950, so let's keep my
review tag (but please be more careful about that when making
major changes between revisions in the future)
Konrad
^ permalink raw reply
* Re: [PATCH v2 1/7] dt-bindings: backlight: qcom-wled: Document ovp values for PMI8994
From: barnabas.czeman @ 2026-01-08 9:17 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Kiran Gunda,
Helge Deller, Luca Weiss, Konrad Dybcio, Eugene Lepshy,
Gianluca Boiano, Alejandro Tafalla, dri-devel, linux-leds,
devicetree, linux-kernel, Daniel Thompson, linux-arm-msm,
linux-fbdev
In-Reply-To: <45a2b510-c825-4191-975a-1389f4f18903@oss.qualcomm.com>
On 2026-01-08 09:54, Konrad Dybcio wrote:
> On 1/8/26 4:43 AM, Barnabás Czémán wrote:
>> Document ovp values supported by wled found in PMI8994.
>>
>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> I reviewed a version of this that said PMI8950, which is very much
> not the same..
>
> Let me try and get some more docs to confirm or deny what you're
> saying..
I have sent it for the previous revision msm-4.4 contains the
documentation.
https://lore.kernel.org/all/95f7c89fdf9b856f567a498cc56ecfa5@mainlining.org/T/#u
>
> Konrad
>
>> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
>> ---
>> .../bindings/leds/backlight/qcom-wled.yaml | 22
>> ++++++++++++++++++++--
>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
>> b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
>> index a8490781011d..19166186a1ff 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
>> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
>> @@ -98,8 +98,8 @@ properties:
>> description: |
>> Over-voltage protection limit. This property is for WLED4 only.
>> $ref: /schemas/types.yaml#/definitions/uint32
>> - enum: [ 18100, 19600, 29600, 31100 ]
>> - default: 29600
>> + minimum: 17800
>> + maximum: 31100
>>
>> qcom,num-strings:
>> description: |
>> @@ -239,6 +239,24 @@ allOf:
>> minimum: 0
>> maximum: 4095
>>
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + const: qcom,pmi8994-wled
>> +
>> + then:
>> + properties:
>> + qcom,ovp-millivolt:
>> + enum: [ 17800, 19400, 29500, 31000 ]
>> + default: 29500
>> +
>> + else:
>> + properties:
>> + qcom,ovp-millivolt:
>> + enum: [ 18100, 19600, 29600, 31100 ]
>> + default: 29600
>> +
>> required:
>> - compatible
>> - reg
>>
^ permalink raw reply
* Re: [PATCH v2 2/7] backlight: qcom-wled: Support ovp values for PMI8994
From: Konrad Dybcio @ 2026-01-08 8:55 UTC (permalink / raw)
To: Barnabás Czémán, Lee Jones, Daniel Thompson,
Jingoo Han, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Kiran Gunda, Helge Deller,
Luca Weiss, Konrad Dybcio, Eugene Lepshy, Gianluca Boiano,
Alejandro Tafalla
Cc: dri-devel, linux-leds, devicetree, linux-kernel, Daniel Thompson,
linux-arm-msm, linux-fbdev
In-Reply-To: <20260108-pmi8950-wled-v2-2-8687f23147d7@mainlining.org>
On 1/8/26 4:43 AM, Barnabás Czémán wrote:
> WLED4 found in PMI8994 supports different ovp values.
>
> Fixes: 6fc632d3e3e0 ("video: backlight: qcom-wled: Add PMI8994 compatible")
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> ---
Likewise, I reviewed a version that said PMI8950 instead.
Konrad
^ permalink raw reply
* Re: [PATCH v2 1/7] dt-bindings: backlight: qcom-wled: Document ovp values for PMI8994
From: Konrad Dybcio @ 2026-01-08 8:54 UTC (permalink / raw)
To: Barnabás Czémán, Lee Jones, Daniel Thompson,
Jingoo Han, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Kiran Gunda, Helge Deller,
Luca Weiss, Konrad Dybcio, Eugene Lepshy, Gianluca Boiano,
Alejandro Tafalla
Cc: dri-devel, linux-leds, devicetree, linux-kernel, Daniel Thompson,
linux-arm-msm, linux-fbdev
In-Reply-To: <20260108-pmi8950-wled-v2-1-8687f23147d7@mainlining.org>
On 1/8/26 4:43 AM, Barnabás Czémán wrote:
> Document ovp values supported by wled found in PMI8994.
>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
I reviewed a version of this that said PMI8950, which is very much
not the same..
Let me try and get some more docs to confirm or deny what you're
saying..
Konrad
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> ---
> .../bindings/leds/backlight/qcom-wled.yaml | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
> index a8490781011d..19166186a1ff 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
> @@ -98,8 +98,8 @@ properties:
> description: |
> Over-voltage protection limit. This property is for WLED4 only.
> $ref: /schemas/types.yaml#/definitions/uint32
> - enum: [ 18100, 19600, 29600, 31100 ]
> - default: 29600
> + minimum: 17800
> + maximum: 31100
>
> qcom,num-strings:
> description: |
> @@ -239,6 +239,24 @@ allOf:
> minimum: 0
> maximum: 4095
>
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: qcom,pmi8994-wled
> +
> + then:
> + properties:
> + qcom,ovp-millivolt:
> + enum: [ 17800, 19400, 29500, 31000 ]
> + default: 29500
> +
> + else:
> + properties:
> + qcom,ovp-millivolt:
> + enum: [ 18100, 19600, 29600, 31100 ]
> + default: 29600
> +
> required:
> - compatible
> - reg
>
^ permalink raw reply
* [PATCH v2 3/7] dt-bindings: backlight: qcom-wled: Document ovp values for PMI8950
From: Barnabás Czémán @ 2026-01-08 3:43 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Kiran Gunda,
Helge Deller, Luca Weiss, Konrad Dybcio, Eugene Lepshy,
Gianluca Boiano, Alejandro Tafalla
Cc: dri-devel, linux-leds, devicetree, linux-kernel, Daniel Thompson,
linux-arm-msm, linux-fbdev, Konrad Dybcio,
Barnabás Czémán
In-Reply-To: <20260108-pmi8950-wled-v2-0-8687f23147d7@mainlining.org>
Document ovp values supported by wled found in PMI8950.
Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
---
Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
index 19166186a1ff..a54448cfdb38 100644
--- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
@@ -243,7 +243,9 @@ allOf:
properties:
compatible:
contains:
- const: qcom,pmi8994-wled
+ enum:
+ - qcom,pmi8950-wled
+ - qcom,pmi8994-wled
then:
properties:
--
2.52.0
^ permalink raw reply related
* [PATCH v2 4/7] backlight: qcom-wled: Fix ovp values for PMI8950
From: Barnabás Czémán @ 2026-01-08 3:43 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Kiran Gunda,
Helge Deller, Luca Weiss, Konrad Dybcio, Eugene Lepshy,
Gianluca Boiano, Alejandro Tafalla
Cc: dri-devel, linux-leds, devicetree, linux-kernel, Daniel Thompson,
linux-arm-msm, linux-fbdev, Konrad Dybcio,
Barnabás Czémán
In-Reply-To: <20260108-pmi8950-wled-v2-0-8687f23147d7@mainlining.org>
PMI8950 WLED support same ovp values like PMI8994 WLED.
Fixes: 10258bf4534b ("backlight: qcom-wled: Add PMI8950 compatible")
Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
---
drivers/video/backlight/qcom-wled.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 5decbd39b789..8054e4787725 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -1455,7 +1455,8 @@ static int wled_configure(struct wled *wled)
break;
case 4:
- if (of_device_is_compatible(dev->of_node, "qcom,pmi8994-wled")) {
+ if (of_device_is_compatible(dev->of_node, "qcom,pmi8950-wled") ||
+ of_device_is_compatible(dev->of_node, "qcom,pmi8994-wled")) {
u32_opts = pmi8994_wled_opts;
size = ARRAY_SIZE(pmi8994_wled_opts);
} else {
--
2.52.0
^ permalink raw reply related
* [PATCH v2 5/7] arm64: dts: qcom: msm8953-xiaomi-vince: correct wled ovp value
From: Barnabás Czémán @ 2026-01-08 3:43 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Kiran Gunda,
Helge Deller, Luca Weiss, Konrad Dybcio, Eugene Lepshy,
Gianluca Boiano, Alejandro Tafalla
Cc: dri-devel, linux-leds, devicetree, linux-kernel, Daniel Thompson,
linux-arm-msm, linux-fbdev, Konrad Dybcio,
Barnabás Czémán
In-Reply-To: <20260108-pmi8950-wled-v2-0-8687f23147d7@mainlining.org>
PMI8950 doesn't actually support setting an OVP threshold value of
29.6 V. The closest allowed value is 29.5 V. Set that instead.
Fixes: aa17e707e04a ("arm64: dts: qcom: msm8953: Add device tree for Xiaomi Redmi 5 Plus")
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
---
arch/arm64/boot/dts/qcom/msm8953-xiaomi-vince.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/msm8953-xiaomi-vince.dts b/arch/arm64/boot/dts/qcom/msm8953-xiaomi-vince.dts
index d46325e79917..c2a290bf493c 100644
--- a/arch/arm64/boot/dts/qcom/msm8953-xiaomi-vince.dts
+++ b/arch/arm64/boot/dts/qcom/msm8953-xiaomi-vince.dts
@@ -169,7 +169,7 @@ &pm8953_resin {
&pmi8950_wled {
qcom,current-limit-microamp = <20000>;
- qcom,ovp-millivolt = <29600>;
+ qcom,ovp-millivolt = <29500>;
qcom,num-strings = <2>;
qcom,external-pfet;
qcom,cabc;
--
2.52.0
^ permalink raw reply related
* [PATCH v2 0/7] Fix PMI8950 WLED ovp values and more
From: Barnabás Czémán @ 2026-01-08 3:43 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Kiran Gunda,
Helge Deller, Luca Weiss, Konrad Dybcio, Eugene Lepshy,
Gianluca Boiano, Alejandro Tafalla
Cc: dri-devel, linux-leds, devicetree, linux-kernel, Daniel Thompson,
linux-arm-msm, linux-fbdev, Konrad Dybcio,
Barnabás Czémán
This patch series fixes supported ovp values related to pmi8950 wled
and corrects wled related properties in xiaomi-daisy, xiaomi-land and
in xiaomi-vince.
Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
---
Changes in v2:
- Rework ovp change to support pmi8994 also.
- Reword commits.
- dt-bindings: Set min max for qcom,ovp-millivolt.
- Link to v1: https://lore.kernel.org/r/20260107-pmi8950-wled-v1-0-5e52f5caa39c@mainlining.org
---
Barnabás Czémán (7):
dt-bindings: backlight: qcom-wled: Document ovp values for PMI8994
backlight: qcom-wled: Support ovp values for PMI8994
dt-bindings: backlight: qcom-wled: Document ovp values for PMI8950
backlight: qcom-wled: Fix ovp values for PMI8950
arm64: dts: qcom: msm8953-xiaomi-vince: correct wled ovp value
arm64: dts: qcom: msm8937-xiaomi-land: correct wled ovp value
arm64: dts: qcom: msm8953-xiaomi-daisy: fix backlight
.../bindings/leds/backlight/qcom-wled.yaml | 24 +++++++++++--
arch/arm64/boot/dts/qcom/msm8937-xiaomi-land.dts | 2 +-
arch/arm64/boot/dts/qcom/msm8953-xiaomi-daisy.dts | 2 +-
arch/arm64/boot/dts/qcom/msm8953-xiaomi-vince.dts | 2 +-
drivers/video/backlight/qcom-wled.c | 42 ++++++++++++++++++++--
5 files changed, 65 insertions(+), 7 deletions(-)
---
base-commit: f96074c6d01d8a5e9e2fccd0bba5f2ed654c1f2d
change-id: 20260107-pmi8950-wled-b014578f67a6
Best regards,
--
Barnabás Czémán <barnabas.czeman@mainlining.org>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox