* [PATCH v6 2/3] backlight: apple_dwi_bl: Add Apple DWI backlight driver
From: Nick Chan @ 2025-02-14 4:02 UTC (permalink / raw)
To: Janne Grunau, Sven Peter, Alyssa Rosenzweig, Lee Jones,
Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, Nick Chan, asahi,
linux-arm-kernel, dri-devel, linux-leds, devicetree, linux-kernel,
linux-fbdev
In-Reply-To: <20250214040306.16312-1-towinchenmi@gmail.com>
Add driver for backlight controllers attached via Apple DWI 2-wire
interface, which is found on some Apple iPhones, iPads and iPod touches
with a LCD display.
Although there is an existing apple_bl driver, it is for backlight
controllers on Intel Macs attached via PCI, which is completely different
from the Samsung-derived DWI block.
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org>
Signed-off-by: Nick Chan <towinchenmi@gmail.com>
---
drivers/video/backlight/Kconfig | 11 +++
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/apple_dwi_bl.c | 123 +++++++++++++++++++++++++
3 files changed, 135 insertions(+)
create mode 100644 drivers/video/backlight/apple_dwi_bl.c
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 3614a5d29c71..cee113bba30f 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -290,6 +290,17 @@ config BACKLIGHT_APPLE
If you have an Intel-based Apple say Y to enable a driver for its
backlight.
+config BACKLIGHT_APPLE_DWI
+ tristate "Apple DWI 2-Wire Interface Backlight Driver"
+ depends on ARCH_APPLE || COMPILE_TEST
+ help
+ Say Y to enable the backlight driver for backlight controllers
+ attached via the Apple DWI 2-wire interface which is found in some
+ Apple iPhones, iPads and iPod touches.
+
+ To compile this driver as a module, choose M here: the module will
+ be called apple_dwi_bl.
+
config BACKLIGHT_QCOM_WLED
tristate "Qualcomm PMIC WLED Driver"
select REGMAP
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index 8fc98f760a8a..156ff9461fb3 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_BACKLIGHT_ADP5520) += adp5520_bl.o
obj-$(CONFIG_BACKLIGHT_ADP8860) += adp8860_bl.o
obj-$(CONFIG_BACKLIGHT_ADP8870) += adp8870_bl.o
obj-$(CONFIG_BACKLIGHT_APPLE) += apple_bl.o
+obj-$(CONFIG_BACKLIGHT_APPLE_DWI) += apple_dwi_bl.o
obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
obj-$(CONFIG_BACKLIGHT_BD6107) += bd6107.o
obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
diff --git a/drivers/video/backlight/apple_dwi_bl.c b/drivers/video/backlight/apple_dwi_bl.c
new file mode 100644
index 000000000000..93bd744972d6
--- /dev/null
+++ b/drivers/video/backlight/apple_dwi_bl.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Driver for backlight controllers attached via Apple DWI 2-wire interface
+ *
+ * Copyright (c) 2024 Nick Chan <towinchenmi@gmail.com>
+ */
+
+#include <linux/backlight.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define DWI_BL_CTL 0x0
+#define DWI_BL_CTL_SEND1 BIT(0)
+#define DWI_BL_CTL_SEND2 BIT(4)
+#define DWI_BL_CTL_SEND3 BIT(5)
+#define DWI_BL_CTL_LE_DATA BIT(6)
+/* Only used on Apple A9 and later */
+#define DWI_BL_CTL_SEND4 BIT(12)
+
+#define DWI_BL_CMD 0x4
+#define DWI_BL_CMD_TYPE GENMASK(31, 28)
+#define DWI_BL_CMD_TYPE_SET_BRIGHTNESS 0xa
+#define DWI_BL_CMD_DATA GENMASK(10, 0)
+
+#define DWI_BL_CTL_SEND (DWI_BL_CTL_SEND1 | \
+ DWI_BL_CTL_SEND2 | \
+ DWI_BL_CTL_SEND3 | \
+ DWI_BL_CTL_LE_DATA | \
+ DWI_BL_CTL_SEND4)
+
+#define DWI_BL_MAX_BRIGHTNESS 2047
+
+struct apple_dwi_bl {
+ void __iomem *base;
+};
+
+static int dwi_bl_update_status(struct backlight_device *bl)
+{
+ struct apple_dwi_bl *dwi_bl = bl_get_data(bl);
+
+ int brightness = backlight_get_brightness(bl);
+
+ u32 cmd = 0;
+
+ cmd |= FIELD_PREP(DWI_BL_CMD_DATA, brightness);
+ cmd |= FIELD_PREP(DWI_BL_CMD_TYPE, DWI_BL_CMD_TYPE_SET_BRIGHTNESS);
+
+ writel(cmd, dwi_bl->base + DWI_BL_CMD);
+ writel(DWI_BL_CTL_SEND, dwi_bl->base + DWI_BL_CTL);
+
+ return 0;
+}
+
+static int dwi_bl_get_brightness(struct backlight_device *bl)
+{
+ struct apple_dwi_bl *dwi_bl = bl_get_data(bl);
+
+ u32 cmd = readl(dwi_bl->base + DWI_BL_CMD);
+
+ return FIELD_GET(DWI_BL_CMD_DATA, cmd);
+}
+
+static const struct backlight_ops dwi_bl_ops = {
+ .options = BL_CORE_SUSPENDRESUME,
+ .get_brightness = dwi_bl_get_brightness,
+ .update_status = dwi_bl_update_status
+};
+
+static int dwi_bl_probe(struct platform_device *dev)
+{
+ struct apple_dwi_bl *dwi_bl;
+ struct backlight_device *bl;
+ struct backlight_properties props;
+ struct resource *res;
+
+ dwi_bl = devm_kzalloc(&dev->dev, sizeof(*dwi_bl), GFP_KERNEL);
+ if (!dwi_bl)
+ return -ENOMEM;
+
+ dwi_bl->base = devm_platform_get_and_ioremap_resource(dev, 0, &res);
+ if (IS_ERR(dwi_bl->base))
+ return PTR_ERR(dwi_bl->base);
+
+ memset(&props, 0, sizeof(struct backlight_properties));
+ props.type = BACKLIGHT_PLATFORM;
+ props.max_brightness = DWI_BL_MAX_BRIGHTNESS;
+ props.scale = BACKLIGHT_SCALE_LINEAR;
+
+ bl = devm_backlight_device_register(&dev->dev, dev->name, &dev->dev,
+ dwi_bl, &dwi_bl_ops, &props);
+ if (IS_ERR(bl))
+ return PTR_ERR(bl);
+
+ platform_set_drvdata(dev, dwi_bl);
+
+ bl->props.brightness = dwi_bl_get_brightness(bl);
+
+ return 0;
+}
+
+static const struct of_device_id dwi_bl_of_match[] = {
+ { .compatible = "apple,dwi-bl" },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, dwi_bl_of_match);
+
+static struct platform_driver dwi_bl_driver = {
+ .driver = {
+ .name = "apple-dwi-bl",
+ .of_match_table = dwi_bl_of_match
+ },
+ .probe = dwi_bl_probe,
+};
+
+module_platform_driver(dwi_bl_driver);
+
+MODULE_DESCRIPTION("Apple DWI Backlight Driver");
+MODULE_AUTHOR("Nick Chan <towinchenmi@gmail.com>");
+MODULE_LICENSE("Dual MIT/GPL");
--
2.48.1
^ permalink raw reply related
* [PATCH v6 1/3] dt-bindings: leds: backlight: apple,dwi-bl: Add Apple DWI backlight
From: Nick Chan @ 2025-02-14 4:02 UTC (permalink / raw)
To: Janne Grunau, Sven Peter, Alyssa Rosenzweig, Lee Jones,
Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, Nick Chan, asahi,
linux-arm-kernel, dri-devel, linux-leds, devicetree, linux-kernel,
linux-fbdev
Cc: Krzysztof Kozlowski
In-Reply-To: <20250214040306.16312-1-towinchenmi@gmail.com>
Add backlight controllers attached via Apple DWI 2-wire interface.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org>
Signed-off-by: Nick Chan <towinchenmi@gmail.com>
---
.../bindings/leds/backlight/apple,dwi-bl.yaml | 57 +++++++++++++++++++
1 file changed, 57 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/backlight/apple,dwi-bl.yaml
diff --git a/Documentation/devicetree/bindings/leds/backlight/apple,dwi-bl.yaml b/Documentation/devicetree/bindings/leds/backlight/apple,dwi-bl.yaml
new file mode 100644
index 000000000000..29caeb356e6f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/apple,dwi-bl.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/apple,dwi-bl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple DWI 2-Wire Interface Backlight Controller
+
+maintainers:
+ - Nick Chan <towinchenmi@gmail.com>
+
+description:
+ Apple SoCs contain a 2-wire interface called DWI. On some Apple iPhones,
+ iPads and iPod touches with a LCD display, 1-2 backlight controllers
+ are connected via DWI. Interfacing with DWI controls all backlight
+ controllers at the same time. As such, the backlight controllers are
+ treated as a single controller regardless of the underlying
+ configuration.
+
+allOf:
+ - $ref: common.yaml#
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - apple,s5l8960x-dwi-bl
+ - apple,t7000-dwi-bl
+ - apple,s8000-dwi-bl
+ - apple,t8010-dwi-bl
+ - apple,t8015-dwi-bl
+ - const: apple,dwi-bl
+
+ reg:
+ maxItems: 1
+
+ power-domains:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ dwi_bl: backlight@20e200010 {
+ compatible = "apple,s5l8960x-dwi-bl", "apple,dwi-bl";
+ reg = <0x2 0x0e200010 0x0 0x8>;
+ power-domains = <&ps_dwi>;
+ };
+ };
--
2.48.1
^ permalink raw reply related
* [PATCH v6 0/3] Apple DWI backlight driver
From: Nick Chan @ 2025-02-14 4:02 UTC (permalink / raw)
To: Janne Grunau, Sven Peter, Alyssa Rosenzweig, Lee Jones,
Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, Nick Chan, asahi,
linux-arm-kernel, dri-devel, linux-leds, devicetree, linux-kernel,
linux-fbdev
Apple SoCs come with a 2-wire interface named DWI. On some iPhones, iPads
and iPod touches the backlight controller is connected via this interface.
This series adds a backlight driver for backlight controllers connected
this way.
Changes since v5:
- Remove default y from drivers/video/backlight/Kconfig
v5: https://lore.kernel.org/asahi/20250203115156.28174-1-towinchenmi@gmail.com/T
Changes since v4:
- Change type to BACKLIGHT_PLATFORM since the driver does not directly
interface with the backlight controller. The actual backlight controller
can be directly controlled via i2c and is not the same on all hardware
that supports the dwi interface.
- Rename file to apple_dwi_bl.c to better match config option.
- Rename driver to apple-dwi-bl to better match config option
- Indicate that the backlight scales linearly
v4: https://lore.kernel.org/asahi/20241211113512.19009-1-towinchenmi@gmail.com/T
Changes since v3:
- $ref to common.yaml in bindings
- (and then additionalProperties is changed to unevaluatedProperties)
- Use hex everywhere in bindings example
- Use sizeof(*dwi_bl) instead of the type of the struct when doing
devm_kzalloc()
- Use devm_platform_get_and_ioremap_resource() in driver
- Fix sorting in drivers/video/backlight/Makefile
- In drivers/video/backlight/Kconfig, move config to right after
BACKLIGHT_APPLE
- Explain this driver being completely different from apple_bl
v3: https://lore.kernel.org/asahi/20241209075908.140014-1-towinchenmi@gmail.com/T
Changes since v2:
- Add missing includes in driver
- Fix file path in MAINTAINERS
v2: https://lore.kernel.org/asahi/20241207130433.30351-1-towinchenmi@gmail.com/T
Changes since v1:
- Fixed dt-bindings $id.
- Make power-domains an optional property in dt-bindings.
- Added missing error checking after devm_ioremap_resource() in
dwi_bl_probe().
v1: https://lore.kernel.org/asahi/20241206172735.4310-1-towinchenmi@gmail.com/T
Nick Chan
---
Nick Chan (3):
dt-bindings: leds: backlight: apple,dwi-bl: Add Apple DWI backlight
backlight: apple_dwi_bl: Add Apple DWI backlight driver
MAINTAINERS: Add entries for Apple DWI backlight controller
.../bindings/leds/backlight/apple,dwi-bl.yaml | 57 ++++++++
MAINTAINERS | 2 +
drivers/video/backlight/Kconfig | 11 ++
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/apple_dwi_bl.c | 123 ++++++++++++++++++
5 files changed, 194 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/backlight/apple,dwi-bl.yaml
create mode 100644 drivers/video/backlight/apple_dwi_bl.c
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
--
2.48.1
^ permalink raw reply
* Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear
From: Helge Deller @ 2025-02-13 22:47 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: linux-fbdev, dri-devel
In-Reply-To: <Z64EzooZqdfLg0pM@intel.com>
On 2/13/25 15:42, Ville Syrjälä wrote:
> On Thu, Sep 26, 2024 at 12:13:04PM +0200, Helge Deller wrote:
>> On 9/26/24 11:57, Ville Syrjälä wrote:
>>> On Thu, Sep 26, 2024 at 08:08:07AM +0200, Helge Deller wrote:
>>>> Hi Ville,
>>>>
>>>> On 9/23/24 17:57, Ville Syrjala wrote:
>>>>> Currently setting cursor_blink attribute to 0 before any fb
>>>>> devices are around does absolutely nothing. When fb devices appear
>>>>> and fbcon becomes active the cursor starts blinking. Fix the problem
>>>>> by recoding the desired state of the attribute even if no fb devices
>>>>> are present at the time.
>>>>>
>>>>> Also adjust the show() method such that it shows the current
>>>>> state of the attribute even when no fb devices are in use.
>>>>>
>>>>> Note that store_cursor_blink() is still a bit dodgy:
>>>>> - seems to be missing some of the other checks that we do
>>>>> elsewhere when deciding whether the cursor should be
>>>>> blinking or not
>>>>> - when set to 0 when the cursor is currently invisible due
>>>>> to blinking, the cursor will remains invisible. We should
>>>>> either explicitly make it visible here, or wait until the
>>>>> full blink cycle has finished.
>>>>
>>>> are you planning to send follow-up patches to address those shortcomings?
>>>
>>> Nope. I don't really care about those as I never plan to
>>> turn the cursor blinking back on after turning it off via
>>> udev.
>>
>> Sad, but OK. I will look into this when I find time.
>> I'd hoped to push those patches upstream during this merge window,
>> but then I think I have to delay them at least until kernel 6.13.
>
> What happened to these? Not seeing them anywhere...
The issues above are not fixed yet, so they are still parked in my for-next-next tree:
https://git.kernel.org/pub/scm/linux/kernel/git/deller/linux-fbdev.git/log/?h=for-next-next
Helge
^ permalink raw reply
* Re: [PATCH v1 1/2] dt-bindings: mfd: Document TI LM3533 MFD
From: Daniel Thompson @ 2025-02-13 21:32 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Lars-Peter Clausen, Pavel Machek,
Daniel Thompson, Jingoo Han, Helge Deller, Andy Shevchenko,
Uwe Kleine-König, devicetree, linux-kernel, linux-iio,
linux-leds, dri-devel, linux-fbdev
In-Reply-To: <20250212075845.11338-2-clamor95@gmail.com>
On Wed, Feb 12, 2025 at 09:58:41AM +0200, Svyatoslav Ryhel wrote:
> Add bindings for the LM3533 - a complete power source for
> backlight, keypad, and indicator LEDs in smartphone handsets.
> The high-voltage inductive boost converter provides the
> power for two series LED strings display backlight and keypad
> functions.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> .../devicetree/bindings/mfd/ti,lm3533.yaml | 221 ++++++++++++++++++
> include/dt-bindings/mfd/lm3533.h | 19 ++
> 2 files changed, 240 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> create mode 100644 include/dt-bindings/mfd/lm3533.h
>
> diff --git a/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> new file mode 100644
> index 000000000000..d0307e5894f8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> @@ -0,0 +1,221 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/ti,lm3533.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI LM3533 Complete Lighting Power Solution
> +
> +description: |
> + The LM3533 is a complete power source for backlight,
> + keypad, and indicator LEDs in smartphone handsets. The
> + high-voltage inductive boost converter provides the
> + power for two series LED strings display backlight and
> + keypad functions.
> + https://www.ti.com/product/LM3533
> +
> +maintainers:
> + - Johan Hovold <jhovold@gmail.com>
This looks like it has been copied from the lm3533 driver. Did Johan
agree to this?
Daniel.
^ permalink raw reply
* Re: [PATCH] drivers: video: backlight: Fix NULL Pointer Dereference in backlight_device_register()
From: Daniel Thompson @ 2025-02-13 21:07 UTC (permalink / raw)
To: Jani Nikula
Cc: Haoyu Li, Lee Jones, Jingoo Han, Helge Deller, Rob Herring,
dri-devel, linux-fbdev, linux-kernel, chenyuan0y, zichenxie0106,
stable
In-Reply-To: <87ldun6u5o.fsf@intel.com>
On Mon, Feb 03, 2025 at 03:21:23PM +0200, Jani Nikula wrote:
> On Thu, 30 Jan 2025, Haoyu Li <lihaoyu499@gmail.com> wrote:
> > In the function "wled_probe", the "wled->name" is dynamically allocated
> > (wled_probe -> wled_configure -> devm_kasprintf), which is possible
> > to be null.
> >
> > In the call trace: wled_probe -> devm_backlight_device_register
> > -> backlight_device_register, this "name" variable is directly
> > dereferenced without checking. We add a null-check statement.
> >
> > Fixes: f86b77583d88 ("backlight: pm8941: Convert to using %pOFn instead of device_node.name")
> > Signed-off-by: Haoyu Li <lihaoyu499@gmail.com>
> > Cc: stable@vger.kernel.org
>
> IMO whoever allocates should be responsible for checking NULL instead of
> passing NULL around and expecting everyone check their input for NULL.
Agreed. This should be fixed in at callsites.
Daniel.
^ permalink raw reply
* Re: [PATCH v5 RESEND 2/3] backlight: apple_dwi_bl: Add Apple DWI backlight driver
From: Daniel Thompson @ 2025-02-13 20:51 UTC (permalink / raw)
To: Nick Chan
Cc: Hector Martin, Sven Peter, Alyssa Rosenzweig, Lee Jones,
Jingoo Han, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Helge Deller, asahi, linux-arm-kernel, dri-devel,
linux-leds, devicetree, linux-kernel, linux-fbdev
In-Reply-To: <20250203115156.28174-3-towinchenmi@gmail.com>
On Mon, Feb 03, 2025 at 07:50:33PM +0800, Nick Chan wrote:
> Add driver for backlight controllers attached via Apple DWI 2-wire
> interface, which is found on some Apple iPhones, iPads and iPod touches
> with a LCD display.
>
> Although there is an existing apple_bl driver, it is for backlight
> controllers on Intel Macs attached via PCI, which is completely different
> from the Samsung-derived DWI block.
>
> Signed-off-by: Nick Chan <towinchenmi@gmail.com>
> ---
> drivers/video/backlight/Kconfig | 12 +++
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/apple_dwi_bl.c | 123 +++++++++++++++++++++++++
> 3 files changed, 136 insertions(+)
> create mode 100644 drivers/video/backlight/apple_dwi_bl.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 3614a5d29c71..c6168727900a 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -290,6 +290,18 @@ config BACKLIGHT_APPLE
> If you have an Intel-based Apple say Y to enable a driver for its
> backlight.
>
> +config BACKLIGHT_APPLE_DWI
> + tristate "Apple DWI 2-Wire Interface Backlight Driver"
> + depends on ARCH_APPLE || COMPILE_TEST
> + default y
Sorry to pick this up late and on a resend but... I can't come up with
any justification for "default y" in this driver.
Other than that this is a really tidy driver so with that changed please
add:
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org>
Daniel.
^ permalink raw reply
* [PATCH 6.6 088/273] m68k: vga: Fix I/O defines
From: Greg Kroah-Hartman @ 2025-02-13 14:27 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, Thomas Zimmermann, kernel test robot,
Geert Uytterhoeven, linux-fbdev, dri-devel, Helge Deller
In-Reply-To: <20250213142407.354217048@linuxfoundation.org>
6.6-stable review patch. If anyone has any objections, please let me know.
------------------
From: Thomas Zimmermann <tzimmermann@suse.de>
commit 53036937a101b5faeaf98e7438555fa854a1a844 upstream.
Including m68k's <asm/raw_io.h> in vga.h on nommu platforms results
in conflicting defines with io_no.h for various I/O macros from the
__raw_read and __raw_write families. An example error is
In file included from arch/m68k/include/asm/vga.h:12,
from include/video/vga.h:22,
from include/linux/vgaarb.h:34,
from drivers/video/aperture.c:12:
>> arch/m68k/include/asm/raw_io.h:39: warning: "__raw_readb" redefined
39 | #define __raw_readb in_8
|
In file included from arch/m68k/include/asm/io.h:6,
from include/linux/io.h:13,
from include/linux/irq.h:20,
from include/asm-generic/hardirq.h:17,
from ./arch/m68k/include/generated/asm/hardirq.h:1,
from include/linux/hardirq.h:11,
from include/linux/interrupt.h:11,
from include/linux/trace_recursion.h:5,
from include/linux/ftrace.h:10,
from include/linux/kprobes.h:28,
from include/linux/kgdb.h:19,
from include/linux/fb.h:6,
from drivers/video/aperture.c:5:
arch/m68k/include/asm/io_no.h:16: note: this is the location of the previous definition
16 | #define __raw_readb(addr) \
|
Include <asm/io.h>, which avoids raw_io.h on nommu platforms.
Also change the defined values of some of the read/write symbols in
vga.h to __raw_read/__raw_write as the raw_in/raw_out symbols are not
generally available.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202501071629.DNEswlm8-lkp@intel.com/
Fixes: 5c3f968712ce ("m68k/video: Create <asm/vga.h>")
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: linux-fbdev@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: Helge Deller <deller@gmx.de>
Cc: stable@vger.kernel.org # v3.5+
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Link: https://lore.kernel.org/20250107095912.130530-1-tzimmermann@suse.de
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/m68k/include/asm/vga.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
--- a/arch/m68k/include/asm/vga.h
+++ b/arch/m68k/include/asm/vga.h
@@ -9,7 +9,7 @@
*/
#ifndef CONFIG_PCI
-#include <asm/raw_io.h>
+#include <asm/io.h>
#include <asm/kmap.h>
/*
@@ -29,9 +29,9 @@
#define inw_p(port) 0
#define outb_p(port, val) do { } while (0)
#define outw(port, val) do { } while (0)
-#define readb raw_inb
-#define writeb raw_outb
-#define writew raw_outw
+#define readb __raw_readb
+#define writeb __raw_writeb
+#define writew __raw_writew
#endif /* CONFIG_PCI */
#endif /* _ASM_M68K_VGA_H */
^ permalink raw reply
* [PATCH 6.13 154/443] m68k: vga: Fix I/O defines
From: Greg Kroah-Hartman @ 2025-02-13 14:25 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, Thomas Zimmermann, kernel test robot,
Geert Uytterhoeven, linux-fbdev, dri-devel, Helge Deller
In-Reply-To: <20250213142440.609878115@linuxfoundation.org>
6.13-stable review patch. If anyone has any objections, please let me know.
------------------
From: Thomas Zimmermann <tzimmermann@suse.de>
commit 53036937a101b5faeaf98e7438555fa854a1a844 upstream.
Including m68k's <asm/raw_io.h> in vga.h on nommu platforms results
in conflicting defines with io_no.h for various I/O macros from the
__raw_read and __raw_write families. An example error is
In file included from arch/m68k/include/asm/vga.h:12,
from include/video/vga.h:22,
from include/linux/vgaarb.h:34,
from drivers/video/aperture.c:12:
>> arch/m68k/include/asm/raw_io.h:39: warning: "__raw_readb" redefined
39 | #define __raw_readb in_8
|
In file included from arch/m68k/include/asm/io.h:6,
from include/linux/io.h:13,
from include/linux/irq.h:20,
from include/asm-generic/hardirq.h:17,
from ./arch/m68k/include/generated/asm/hardirq.h:1,
from include/linux/hardirq.h:11,
from include/linux/interrupt.h:11,
from include/linux/trace_recursion.h:5,
from include/linux/ftrace.h:10,
from include/linux/kprobes.h:28,
from include/linux/kgdb.h:19,
from include/linux/fb.h:6,
from drivers/video/aperture.c:5:
arch/m68k/include/asm/io_no.h:16: note: this is the location of the previous definition
16 | #define __raw_readb(addr) \
|
Include <asm/io.h>, which avoids raw_io.h on nommu platforms.
Also change the defined values of some of the read/write symbols in
vga.h to __raw_read/__raw_write as the raw_in/raw_out symbols are not
generally available.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202501071629.DNEswlm8-lkp@intel.com/
Fixes: 5c3f968712ce ("m68k/video: Create <asm/vga.h>")
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: linux-fbdev@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: Helge Deller <deller@gmx.de>
Cc: stable@vger.kernel.org # v3.5+
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Link: https://lore.kernel.org/20250107095912.130530-1-tzimmermann@suse.de
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/m68k/include/asm/vga.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
--- a/arch/m68k/include/asm/vga.h
+++ b/arch/m68k/include/asm/vga.h
@@ -9,7 +9,7 @@
*/
#ifndef CONFIG_PCI
-#include <asm/raw_io.h>
+#include <asm/io.h>
#include <asm/kmap.h>
/*
@@ -29,9 +29,9 @@
#define inw_p(port) 0
#define outb_p(port, val) do { } while (0)
#define outw(port, val) do { } while (0)
-#define readb raw_inb
-#define writeb raw_outb
-#define writew raw_outw
+#define readb __raw_readb
+#define writeb __raw_writeb
+#define writew __raw_writew
#endif /* CONFIG_PCI */
#endif /* _ASM_M68K_VGA_H */
^ permalink raw reply
* Re: [PATCH v1 2/2] mfd: lm3533: convert to use OF
From: Svyatoslav Ryhel @ 2025-02-13 15:03 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Lars-Peter Clausen, Pavel Machek,
Daniel Thompson, Jingoo Han, Helge Deller, Uwe Kleine-König,
devicetree, linux-kernel, linux-iio, linux-leds, dri-devel,
linux-fbdev
In-Reply-To: <Z64IPpW5Uhad4HjU@smile.fi.intel.com>
чт, 13 лют. 2025 р. о 16:57 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> пише:
>
> On Wed, Feb 12, 2025 at 09:58:42AM +0200, Svyatoslav Ryhel wrote:
> > Add ability to fill pdata from device tree. Common stuff is
> > filled from core driver and then pdata is filled per-device
> > since all cells are optional.
>
> ...
>
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/mfd/core.h>
>
> > +#include <linux/of.h>
>
> Is it used? In any case, please no OF-centric APIs in a new (feature) code.
>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
>
> ...
>
> > +static int lm3533_pass_of_node(struct platform_device *pdev,
>
> pass_of_node sounds a bit awkward.
> Perhaps you thought about parse_fwnode ?
>
> > + struct lm3533_als_platform_data *pdata)
> > +{
> > + struct device *parent_dev = pdev->dev.parent;
> > + struct device *dev = &pdev->dev;
> > + struct fwnode_handle *node;
> > + const char *label;
> > + int val, ret;
> > +
> > + device_for_each_child_node(parent_dev, node) {
> > + fwnode_property_read_string(node, "compatible", &label);
> > +
> > + if (!strcmp(label, pdev->name)) {
>
> This is a bit strange. Why one need to compare platform device instance (!)
> name with compatible which is part of ABI. This looks really wrong approach.
> Needs a very good explanation on what's going on here.
>
> Besides that the label is usually filled by LEDS core, why do we need to handle
> it in a special way?
>
> > + ret = fwnode_property_read_u32(node, "reg", &val);
> > + if (ret) {
> > + dev_err(dev, "reg property is missing: ret %d\n", ret);
> > + return ret;
> > + }
> > +
> > + if (val == pdev->id) {
>
> > + dev->fwnode = node;
> > + dev->of_node = to_of_node(node);
>
> No direct access to fwnode in struct device, please use device_set_node().
>
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > pdata = dev_get_platdata(&pdev->dev);
> > if (!pdata) {
> > - dev_err(&pdev->dev, "no platform data\n");
> > - return -EINVAL;
> > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return -ENOMEM;
> > +
> > + ret = lm3533_pass_of_node(pdev, pdata);
> > + if (ret)
> > + return dev_err_probe(&pdev->dev, ret,
> > + "failed to get als device node\n");
>
> With
>
> struct device *dev = &pdev->dev;
>
> at the top of the function will help a lot in making the code neater and easier
> to read.
>
> > + lm3533_parse_als(pdev, pdata);
> > }
>
> ...
>
> > #include <linux/leds.h>
> > #include <linux/mfd/core.h>
> > #include <linux/mutex.h>
>
> > +#include <linux/of.h>
>
> Cargo cult? "Proxy" header? Please follow IWYU principle.
>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/slab.h>
>
> ...
>
> > +static void lm3533_parse_led(struct platform_device *pdev,
> > + struct lm3533_led_platform_data *pdata)
> > +{
> > + struct device *dev = &pdev->dev;
> > + int val, ret;
> > +
> > + ret = device_property_read_string(dev, "default-trigger",
> > + &pdata->default_trigger);
> > + if (ret)
> > + pdata->default_trigger = "none";
>
> Isn't this done already in LEDS core?
>
> > + /* 5000 - 29800 uA (800 uA step) */
> > + ret = device_property_read_u32(dev, "max-current-microamp", &val);
> > + if (ret)
> > + val = 5000;
> > + pdata->max_current = val;
> > +
> > + /* 0 - 0x3f */
> > + ret = device_property_read_u32(dev, "pwm", &val);
> > + if (ret)
> > + val = 0;
> > + pdata->pwm = val;
> > +}
>
> ...
>
> > +static int lm3533_pass_of_node(struct platform_device *pdev,
> > + struct lm3533_led_platform_data *pdata)
>
> I think I already saw exactly the same piece of code. Please make sure
> you have no duplications.
>
> > +}
>
> ...
>
> > pdata = dev_get_platdata(&pdev->dev);
> > if (!pdata) {
> > - dev_err(&pdev->dev, "no platform data\n");
> > - return -EINVAL;
> > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return -ENOMEM;
> > +
> > + ret = lm3533_pass_of_node(pdev, pdata);
> > + if (ret)
> > + return dev_err_probe(&pdev->dev, ret,
> > + "failed to get led device node\n");
> > +
> > + lm3533_parse_led(pdev, pdata);
>
> Ditto.
>
> > }
>
> ...
>
> > - led->cdev.name = pdata->name;
> > + led->cdev.name = dev_name(&pdev->dev);
>
> Are you sure it's a good idea?
>
> ...
>
> > - if (!pdata->als)
> > + if (!pdata->num_als)
> > return 0;
> >
> > - lm3533_als_devs[0].platform_data = pdata->als;
> > - lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
> > + if (pdata->num_als > ARRAY_SIZE(lm3533_als_devs))
> > + pdata->num_als = ARRAY_SIZE(lm3533_als_devs);
>
> Looks like you want
>
> pdata->num_als = clamp(pdata->num_als, 0, ARRAY_SIZE(lm3533_als_devs));
> if (!pdata->num_als)
> return 0;
>
> instead of the above. You would need minmax.h for that.
>
> ...
>
> > + if (pdata->leds) {
>
> This is strange. I would expect num_leds == 0 in this case
>
> > + for (i = 0; i < pdata->num_leds; ++i) {
> > + lm3533_led_devs[i].platform_data = &pdata->leds[i];
> > + lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
> > + }
> > }
>
> ...
>
> > +static void lm3533_parse_nodes(struct lm3533 *lm3533,
> > + struct lm3533_platform_data *pdata)
> > +{
> > + struct fwnode_handle *node;
> > + const char *label;
> > +
> > + device_for_each_child_node(lm3533->dev, node) {
> > + fwnode_property_read_string(node, "compatible", &label);
> > +
> > + if (!strcmp(label, lm3533_bl_devs[pdata->num_backlights].name))
> > + pdata->num_backlights++;
> > +
> > + if (!strcmp(label, lm3533_led_devs[pdata->num_leds].name))
> > + pdata->num_leds++;
> > +
> > + if (!strcmp(label, lm3533_als_devs[pdata->num_als].name))
> > + pdata->num_als++;
> > + }
> > +}
>
> Oh, I don't like this approach. If you have compatible, you have driver_data
> available, all this is not needed as it may be hard coded.
>
> ...
>
> > if (!pdata) {
>
> I would expect actually that legacy platform data support will be simply killed
> by this patch(es). Do we have in-kernel users? If so, they can be easily
> converted to use software nodes, otherwise we even don't need to care.
>
> > - dev_err(lm3533->dev, "no platform data\n");
> > - return -EINVAL;
> > + pdata = devm_kzalloc(lm3533->dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return -ENOMEM;
> > +
> > + ret = device_property_read_u32(lm3533->dev,
> > + "ti,boost-ovp",
> > + &pdata->boost_ovp);
> > + if (ret)
> > + pdata->boost_ovp = LM3533_BOOST_OVP_16V;
> > +
> > + ret = device_property_read_u32(lm3533->dev,
> > + "ti,boost-freq",
> > + &pdata->boost_freq);
> > + if (ret)
> > + pdata->boost_freq = LM3533_BOOST_FREQ_500KHZ;
> > +
> > + lm3533_parse_nodes(lm3533, pdata);
> > +
> > + lm3533->dev->platform_data = pdata;
> > }
>
> ...
>
> > +static const struct of_device_id lm3533_match_table[] = {
> > + { .compatible = "ti,lm3533" },
> > + { },
>
> No comma in the terminator entry.
>
> > +};
>
> ...
>
> > +static void lm3533_parse_backlight(struct platform_device *pdev,
>
> pdev is not actually used, just pass struct device *dev directly.
> Same comment to other functions in this change. It will make code more
> bus independent and reusable.
>
> > + struct lm3533_bl_platform_data *pdata)
> > +{
> > + struct device *dev = &pdev->dev;
> > + int val, ret;
> > +
> > + /* 5000 - 29800 uA (800 uA step) */
> > + ret = device_property_read_u32(dev, "max-current-microamp", &val);
> > + if (ret)
> > + val = 5000;
> > + pdata->max_current = val;
>
> > + /* 0 - 255 */
> > + ret = device_property_read_u32(dev, "default-brightness", &val);
> > + if (ret)
> > + val = LM3533_BL_MAX_BRIGHTNESS;
> > + pdata->default_brightness = val;
>
> Isn't handled by LEDS core?
>
> > + /* 0 - 0x3f */
> > + ret = device_property_read_u32(dev, "pwm", &val);
> > + if (ret)
> > + val = 0;
> > + pdata->pwm = val;
> > +}
>
> ...
>
> > +static int lm3533_pass_of_node(struct platform_device *pdev,
> > + struct lm3533_bl_platform_data *pdata)
> > +{
>
> 3rd dup?
>
> > +}
>
> ...
>
> > pdata = dev_get_platdata(&pdev->dev);
> > if (!pdata) {
> > - dev_err(&pdev->dev, "no platform data\n");
> > - return -EINVAL;
> > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return -ENOMEM;
> > +
> > + ret = lm3533_pass_of_node(pdev, pdata);
> > + if (ret)
> > + return dev_err_probe(&pdev->dev, ret,
> > + "failed to get backlight device node\n");
> > +
> > + lm3533_parse_backlight(pdev, pdata);
> > }
>
> Ditto.
>
> > - bd = devm_backlight_device_register(&pdev->dev, pdata->name,
> > - pdev->dev.parent, bl, &lm3533_bl_ops,
> > - &props);
> > + bd = devm_backlight_device_register(&pdev->dev, dev_name(&pdev->dev),
>
> I'm not sure the dev_name() is a good idea. We usually try to rely on the
> predictable outcome, something like what "%pfw" prints against a certain fwnode.
>
> > + pdev->dev.parent, bl,
> > + &lm3533_bl_ops, &props);
>
> ...
>
> Also I feel that this change may be split to a few separate logical changes.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Acknowledged, thank you.
^ permalink raw reply
* Re: [PATCH v1 2/2] mfd: lm3533: convert to use OF
From: Andy Shevchenko @ 2025-02-13 14:57 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Lars-Peter Clausen, Pavel Machek,
Daniel Thompson, Jingoo Han, Helge Deller, Uwe Kleine-König,
devicetree, linux-kernel, linux-iio, linux-leds, dri-devel,
linux-fbdev
In-Reply-To: <20250212075845.11338-3-clamor95@gmail.com>
On Wed, Feb 12, 2025 at 09:58:42AM +0200, Svyatoslav Ryhel wrote:
> Add ability to fill pdata from device tree. Common stuff is
> filled from core driver and then pdata is filled per-device
> since all cells are optional.
...
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/mfd/core.h>
> +#include <linux/of.h>
Is it used? In any case, please no OF-centric APIs in a new (feature) code.
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
...
> +static int lm3533_pass_of_node(struct platform_device *pdev,
pass_of_node sounds a bit awkward.
Perhaps you thought about parse_fwnode ?
> + struct lm3533_als_platform_data *pdata)
> +{
> + struct device *parent_dev = pdev->dev.parent;
> + struct device *dev = &pdev->dev;
> + struct fwnode_handle *node;
> + const char *label;
> + int val, ret;
> +
> + device_for_each_child_node(parent_dev, node) {
> + fwnode_property_read_string(node, "compatible", &label);
> +
> + if (!strcmp(label, pdev->name)) {
This is a bit strange. Why one need to compare platform device instance (!)
name with compatible which is part of ABI. This looks really wrong approach.
Needs a very good explanation on what's going on here.
Besides that the label is usually filled by LEDS core, why do we need to handle
it in a special way?
> + ret = fwnode_property_read_u32(node, "reg", &val);
> + if (ret) {
> + dev_err(dev, "reg property is missing: ret %d\n", ret);
> + return ret;
> + }
> +
> + if (val == pdev->id) {
> + dev->fwnode = node;
> + dev->of_node = to_of_node(node);
No direct access to fwnode in struct device, please use device_set_node().
> + }
> + }
> + }
> +
> + return 0;
> +}
...
> pdata = dev_get_platdata(&pdev->dev);
> if (!pdata) {
> - dev_err(&pdev->dev, "no platform data\n");
> - return -EINVAL;
> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + ret = lm3533_pass_of_node(pdev, pdata);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to get als device node\n");
With
struct device *dev = &pdev->dev;
at the top of the function will help a lot in making the code neater and easier
to read.
> + lm3533_parse_als(pdev, pdata);
> }
...
> #include <linux/leds.h>
> #include <linux/mfd/core.h>
> #include <linux/mutex.h>
> +#include <linux/of.h>
Cargo cult? "Proxy" header? Please follow IWYU principle.
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/slab.h>
...
> +static void lm3533_parse_led(struct platform_device *pdev,
> + struct lm3533_led_platform_data *pdata)
> +{
> + struct device *dev = &pdev->dev;
> + int val, ret;
> +
> + ret = device_property_read_string(dev, "default-trigger",
> + &pdata->default_trigger);
> + if (ret)
> + pdata->default_trigger = "none";
Isn't this done already in LEDS core?
> + /* 5000 - 29800 uA (800 uA step) */
> + ret = device_property_read_u32(dev, "max-current-microamp", &val);
> + if (ret)
> + val = 5000;
> + pdata->max_current = val;
> +
> + /* 0 - 0x3f */
> + ret = device_property_read_u32(dev, "pwm", &val);
> + if (ret)
> + val = 0;
> + pdata->pwm = val;
> +}
...
> +static int lm3533_pass_of_node(struct platform_device *pdev,
> + struct lm3533_led_platform_data *pdata)
I think I already saw exactly the same piece of code. Please make sure
you have no duplications.
> +}
...
> pdata = dev_get_platdata(&pdev->dev);
> if (!pdata) {
> - dev_err(&pdev->dev, "no platform data\n");
> - return -EINVAL;
> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + ret = lm3533_pass_of_node(pdev, pdata);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to get led device node\n");
> +
> + lm3533_parse_led(pdev, pdata);
Ditto.
> }
...
> - led->cdev.name = pdata->name;
> + led->cdev.name = dev_name(&pdev->dev);
Are you sure it's a good idea?
...
> - if (!pdata->als)
> + if (!pdata->num_als)
> return 0;
>
> - lm3533_als_devs[0].platform_data = pdata->als;
> - lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
> + if (pdata->num_als > ARRAY_SIZE(lm3533_als_devs))
> + pdata->num_als = ARRAY_SIZE(lm3533_als_devs);
Looks like you want
pdata->num_als = clamp(pdata->num_als, 0, ARRAY_SIZE(lm3533_als_devs));
if (!pdata->num_als)
return 0;
instead of the above. You would need minmax.h for that.
...
> + if (pdata->leds) {
This is strange. I would expect num_leds == 0 in this case
> + for (i = 0; i < pdata->num_leds; ++i) {
> + lm3533_led_devs[i].platform_data = &pdata->leds[i];
> + lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
> + }
> }
...
> +static void lm3533_parse_nodes(struct lm3533 *lm3533,
> + struct lm3533_platform_data *pdata)
> +{
> + struct fwnode_handle *node;
> + const char *label;
> +
> + device_for_each_child_node(lm3533->dev, node) {
> + fwnode_property_read_string(node, "compatible", &label);
> +
> + if (!strcmp(label, lm3533_bl_devs[pdata->num_backlights].name))
> + pdata->num_backlights++;
> +
> + if (!strcmp(label, lm3533_led_devs[pdata->num_leds].name))
> + pdata->num_leds++;
> +
> + if (!strcmp(label, lm3533_als_devs[pdata->num_als].name))
> + pdata->num_als++;
> + }
> +}
Oh, I don't like this approach. If you have compatible, you have driver_data
available, all this is not needed as it may be hard coded.
...
> if (!pdata) {
I would expect actually that legacy platform data support will be simply killed
by this patch(es). Do we have in-kernel users? If so, they can be easily
converted to use software nodes, otherwise we even don't need to care.
> - dev_err(lm3533->dev, "no platform data\n");
> - return -EINVAL;
> + pdata = devm_kzalloc(lm3533->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + ret = device_property_read_u32(lm3533->dev,
> + "ti,boost-ovp",
> + &pdata->boost_ovp);
> + if (ret)
> + pdata->boost_ovp = LM3533_BOOST_OVP_16V;
> +
> + ret = device_property_read_u32(lm3533->dev,
> + "ti,boost-freq",
> + &pdata->boost_freq);
> + if (ret)
> + pdata->boost_freq = LM3533_BOOST_FREQ_500KHZ;
> +
> + lm3533_parse_nodes(lm3533, pdata);
> +
> + lm3533->dev->platform_data = pdata;
> }
...
> +static const struct of_device_id lm3533_match_table[] = {
> + { .compatible = "ti,lm3533" },
> + { },
No comma in the terminator entry.
> +};
...
> +static void lm3533_parse_backlight(struct platform_device *pdev,
pdev is not actually used, just pass struct device *dev directly.
Same comment to other functions in this change. It will make code more
bus independent and reusable.
> + struct lm3533_bl_platform_data *pdata)
> +{
> + struct device *dev = &pdev->dev;
> + int val, ret;
> +
> + /* 5000 - 29800 uA (800 uA step) */
> + ret = device_property_read_u32(dev, "max-current-microamp", &val);
> + if (ret)
> + val = 5000;
> + pdata->max_current = val;
> + /* 0 - 255 */
> + ret = device_property_read_u32(dev, "default-brightness", &val);
> + if (ret)
> + val = LM3533_BL_MAX_BRIGHTNESS;
> + pdata->default_brightness = val;
Isn't handled by LEDS core?
> + /* 0 - 0x3f */
> + ret = device_property_read_u32(dev, "pwm", &val);
> + if (ret)
> + val = 0;
> + pdata->pwm = val;
> +}
...
> +static int lm3533_pass_of_node(struct platform_device *pdev,
> + struct lm3533_bl_platform_data *pdata)
> +{
3rd dup?
> +}
...
> pdata = dev_get_platdata(&pdev->dev);
> if (!pdata) {
> - dev_err(&pdev->dev, "no platform data\n");
> - return -EINVAL;
> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + ret = lm3533_pass_of_node(pdev, pdata);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to get backlight device node\n");
> +
> + lm3533_parse_backlight(pdev, pdata);
> }
Ditto.
> - bd = devm_backlight_device_register(&pdev->dev, pdata->name,
> - pdev->dev.parent, bl, &lm3533_bl_ops,
> - &props);
> + bd = devm_backlight_device_register(&pdev->dev, dev_name(&pdev->dev),
I'm not sure the dev_name() is a good idea. We usually try to rely on the
predictable outcome, something like what "%pfw" prints against a certain fwnode.
> + pdev->dev.parent, bl,
> + &lm3533_bl_ops, &props);
...
Also I feel that this change may be split to a few separate logical changes.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 1/6] fbcon: Make cursor_blink=0 work when configured before fb devices appear
From: Ville Syrjälä @ 2025-02-13 14:42 UTC (permalink / raw)
To: Helge Deller; +Cc: linux-fbdev, dri-devel
In-Reply-To: <34a7d276-ee26-4a8d-b996-87faa5b224c4@gmx.de>
On Thu, Sep 26, 2024 at 12:13:04PM +0200, Helge Deller wrote:
> On 9/26/24 11:57, Ville Syrjälä wrote:
> > On Thu, Sep 26, 2024 at 08:08:07AM +0200, Helge Deller wrote:
> >> Hi Ville,
> >>
> >> On 9/23/24 17:57, Ville Syrjala wrote:
> >>> Currently setting cursor_blink attribute to 0 before any fb
> >>> devices are around does absolutely nothing. When fb devices appear
> >>> and fbcon becomes active the cursor starts blinking. Fix the problem
> >>> by recoding the desired state of the attribute even if no fb devices
> >>> are present at the time.
> >>>
> >>> Also adjust the show() method such that it shows the current
> >>> state of the attribute even when no fb devices are in use.
> >>>
> >>> Note that store_cursor_blink() is still a bit dodgy:
> >>> - seems to be missing some of the other checks that we do
> >>> elsewhere when deciding whether the cursor should be
> >>> blinking or not
> >>> - when set to 0 when the cursor is currently invisible due
> >>> to blinking, the cursor will remains invisible. We should
> >>> either explicitly make it visible here, or wait until the
> >>> full blink cycle has finished.
> >>
> >> are you planning to send follow-up patches to address those shortcomings?
> >
> > Nope. I don't really care about those as I never plan to
> > turn the cursor blinking back on after turning it off via
> > udev.
>
> Sad, but OK. I will look into this when I find time.
> I'd hoped to push those patches upstream during this merge window,
> but then I think I have to delay them at least until kernel 6.13.
What happened to these? Not seeing them anywhere...
--
Ville Syrjälä
Intel
^ permalink raw reply
* [PATCH 6.12 140/422] m68k: vga: Fix I/O defines
From: Greg Kroah-Hartman @ 2025-02-13 14:24 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, Thomas Zimmermann, kernel test robot,
Geert Uytterhoeven, linux-fbdev, dri-devel, Helge Deller
In-Reply-To: <20250213142436.408121546@linuxfoundation.org>
6.12-stable review patch. If anyone has any objections, please let me know.
------------------
From: Thomas Zimmermann <tzimmermann@suse.de>
commit 53036937a101b5faeaf98e7438555fa854a1a844 upstream.
Including m68k's <asm/raw_io.h> in vga.h on nommu platforms results
in conflicting defines with io_no.h for various I/O macros from the
__raw_read and __raw_write families. An example error is
In file included from arch/m68k/include/asm/vga.h:12,
from include/video/vga.h:22,
from include/linux/vgaarb.h:34,
from drivers/video/aperture.c:12:
>> arch/m68k/include/asm/raw_io.h:39: warning: "__raw_readb" redefined
39 | #define __raw_readb in_8
|
In file included from arch/m68k/include/asm/io.h:6,
from include/linux/io.h:13,
from include/linux/irq.h:20,
from include/asm-generic/hardirq.h:17,
from ./arch/m68k/include/generated/asm/hardirq.h:1,
from include/linux/hardirq.h:11,
from include/linux/interrupt.h:11,
from include/linux/trace_recursion.h:5,
from include/linux/ftrace.h:10,
from include/linux/kprobes.h:28,
from include/linux/kgdb.h:19,
from include/linux/fb.h:6,
from drivers/video/aperture.c:5:
arch/m68k/include/asm/io_no.h:16: note: this is the location of the previous definition
16 | #define __raw_readb(addr) \
|
Include <asm/io.h>, which avoids raw_io.h on nommu platforms.
Also change the defined values of some of the read/write symbols in
vga.h to __raw_read/__raw_write as the raw_in/raw_out symbols are not
generally available.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202501071629.DNEswlm8-lkp@intel.com/
Fixes: 5c3f968712ce ("m68k/video: Create <asm/vga.h>")
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: linux-fbdev@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: Helge Deller <deller@gmx.de>
Cc: stable@vger.kernel.org # v3.5+
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Link: https://lore.kernel.org/20250107095912.130530-1-tzimmermann@suse.de
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/m68k/include/asm/vga.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
--- a/arch/m68k/include/asm/vga.h
+++ b/arch/m68k/include/asm/vga.h
@@ -9,7 +9,7 @@
*/
#ifndef CONFIG_PCI
-#include <asm/raw_io.h>
+#include <asm/io.h>
#include <asm/kmap.h>
/*
@@ -29,9 +29,9 @@
#define inw_p(port) 0
#define outb_p(port, val) do { } while (0)
#define outw(port, val) do { } while (0)
-#define readb raw_inb
-#define writeb raw_outb
-#define writew raw_outw
+#define readb __raw_readb
+#define writeb __raw_writeb
+#define writew __raw_writew
#endif /* CONFIG_PCI */
#endif /* _ASM_M68K_VGA_H */
^ permalink raw reply
* Re: [PATCH 12/13] leds: backlight trigger: Replace fb events with a dedicated function call
From: Thomas Zimmermann @ 2025-02-13 14:34 UTC (permalink / raw)
To: Lee Jones
Cc: pavel, danielt, jingoohan1, deller, simona, linux-leds, dri-devel,
linux-fbdev
In-Reply-To: <20250211135752.GT1868108@google.com>
Hi
Am 11.02.25 um 14:57 schrieb Lee Jones:
> On Thu, 06 Feb 2025, Thomas Zimmermann wrote:
>
>> Remove support for fb events from the led backlight trigger. Provide the
>> helper ledtrig_backlight_blank() instead. Call it from fbdev to inform
>> the trigger of changes to a display's blank state.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> drivers/leds/trigger/ledtrig-backlight.c | 31 +++++-------------------
>> drivers/video/fbdev/core/fbmem.c | 21 +++++++++-------
>> include/linux/leds.h | 6 +++++
>> 3 files changed, 24 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/leds/trigger/ledtrig-backlight.c b/drivers/leds/trigger/ledtrig-backlight.c
>> index f9317f93483b..e3ae4adc29e3 100644
>> --- a/drivers/leds/trigger/ledtrig-backlight.c
>> +++ b/drivers/leds/trigger/ledtrig-backlight.c
>> @@ -10,7 +10,6 @@
>> #include <linux/kernel.h>
>> #include <linux/slab.h>
>> #include <linux/init.h>
>> -#include <linux/fb.h>
>> #include <linux/leds.h>
>> #include "../leds.h"
>>
>> @@ -21,7 +20,6 @@ struct bl_trig_notifier {
>> struct led_classdev *led;
>> int brightness;
>> int old_status;
>> - struct notifier_block notifier;
>> unsigned invert;
>>
>> struct list_head entry;
>> @@ -30,7 +28,7 @@ struct bl_trig_notifier {
>> static struct list_head ledtrig_backlight_list;
>> static struct mutex ledtrig_backlight_list_mutex;
>>
>> -static void ledtrig_backlight_blank(struct bl_trig_notifier *n, bool on)
>> +static void __ledtrig_backlight_blank(struct bl_trig_notifier *n, bool on)
> I'm confused. Didn't you just introduce this?
It's being renamed here; probably something to avoid.
>
>> {
>> struct led_classdev *led = n->led;
>> int new_status = !on ? BLANK : UNBLANK;
>> @@ -48,23 +46,14 @@ static void ledtrig_backlight_blank(struct bl_trig_notifier *n, bool on)
>> n->old_status = new_status;
>> }
>>
>> -static int fb_notifier_callback(struct notifier_block *p,
>> - unsigned long event, void *data)
>> +void ledtrig_backlight_blank(bool on)
>> {
>> - struct bl_trig_notifier *n = container_of(p,
>> - struct bl_trig_notifier, notifier);
>> - struct fb_event *fb_event = data;
>> - int *blank;
>> -
>> - /* If we aren't interested in this event, skip it immediately ... */
>> - if (event != FB_EVENT_BLANK)
>> - return 0;
>> -
>> - blank = fb_event->data;
>> + struct bl_trig_notifier *n;
>>
>> - ledtrig_backlight_blank(n, !blank[0]);
>> + guard(mutex)(&ledtrig_backlight_list_mutex);
>>
>> - return 0;
>> + list_for_each_entry(n, &ledtrig_backlight_list, entry)
>> + __ledtrig_backlight_blank(n, on);
>> }
>>
>> static ssize_t bl_trig_invert_show(struct device *dev,
>> @@ -110,8 +99,6 @@ ATTRIBUTE_GROUPS(bl_trig);
>>
>> static int bl_trig_activate(struct led_classdev *led)
>> {
>> - int ret;
>> -
>> struct bl_trig_notifier *n;
>>
>> n = kzalloc(sizeof(struct bl_trig_notifier), GFP_KERNEL);
>> @@ -122,11 +109,6 @@ static int bl_trig_activate(struct led_classdev *led)
>> n->led = led;
>> n->brightness = led->brightness;
>> n->old_status = UNBLANK;
>> - n->notifier.notifier_call = fb_notifier_callback;
>> -
>> - ret = fb_register_client(&n->notifier);
>> - if (ret)
>> - dev_err(led->dev, "unable to register backlight trigger\n");
>>
>> mutex_lock(&ledtrig_backlight_list_mutex);
>> list_add(&n->entry, &ledtrig_backlight_list);
>> @@ -143,7 +125,6 @@ static void bl_trig_deactivate(struct led_classdev *led)
>> list_del(&n->entry);
>> mutex_unlock(&ledtrig_backlight_list_mutex);
>>
>> - fb_unregister_client(&n->notifier);
>> kfree(n);
>> }
>>
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index fb7ca143a996..92c3fe2a7aff 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -16,6 +16,7 @@
>> #include <linux/fb.h>
>> #include <linux/fbcon.h>
>> #include <linux/lcd.h>
>> +#include <linux/leds.h>
>>
>> #include <video/nomodeset.h>
>>
>> @@ -373,11 +374,19 @@ static void fb_lcd_notify_blank(struct fb_info *info)
>> #endif
>> }
>>
>> +static void fb_ledtrig_backlight_notify_blank(struct fb_info *info)
>> +{
>> +#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_BACKLIGHT)
> #iferry is generally discouraged in C files.
>
> Does is_defined() work for you?
I don't think so. This ifdef covers the case that fbdev is built in, but
led is a module (i.e., fbdev=y && led=m).
> /
>> + if (info->blank == FB_BLANK_UNBLANK)
>> + ledtrig_backlight_blank(true);
> If !CONFIG_LEDS_TRIGGER_BACKLIGHT(), then ledtrig_backlight_blank() is a
> noop anyway, right? So why encompass it in the #if at all?
Because of (fbdev=y && led=m) again. ledtrig_backlight_blank() would be
defined then, but not linkable from fbdev. Preferably, I'd rather leave
out the ifdef in the led header file.
Best regards
Thomas
>
>> + else
>> + ledtrig_backlight_blank(false);
>> +#endif
>> +}
>> +
>> int fb_blank(struct fb_info *info, int blank)
>> {
>> int old_blank = info->blank;
>> - struct fb_event event;
>> - int data[2];
>> int ret;
>>
>> if (!info->fbops->fb_blank)
>> @@ -386,11 +395,6 @@ int fb_blank(struct fb_info *info, int blank)
>> if (blank > FB_BLANK_POWERDOWN)
>> blank = FB_BLANK_POWERDOWN;
>>
>> - data[0] = blank;
>> - data[1] = old_blank;
>> - event.info = info;
>> - event.data = data;
>> -
>> info->blank = blank;
>>
>> ret = info->fbops->fb_blank(blank, info);
>> @@ -399,8 +403,7 @@ int fb_blank(struct fb_info *info, int blank)
>>
>> fb_bl_notify_blank(info, old_blank);
>> fb_lcd_notify_blank(info);
>> -
>> - fb_notifier_call_chain(FB_EVENT_BLANK, &event);
>> + fb_ledtrig_backlight_notify_blank(info);
>>
>> return 0;
>>
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index 98f9719c924c..8c7c41888f7d 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -640,6 +640,12 @@ static inline void ledtrig_flash_ctrl(bool on) {}
>> static inline void ledtrig_torch_ctrl(bool on) {}
>> #endif
>>
>> +#if IS_ENABLED(CONFIG_LEDS_TRIGGER_BACKLIGHT)
>> +void ledtrig_backlight_blank(bool on);
>> +#else
>> +static inline void ledtrig_backlight_blank(bool on) {}
>> +#endif
>> +
>> /*
>> * Generic LED platform data for describing LED names and default triggers.
>> */
>> --
>> 2.48.1
>>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply
* Re: [PATCH 10/13] leds: backlight trigger: Maintain global list of led backlight triggers
From: Thomas Zimmermann @ 2025-02-13 14:23 UTC (permalink / raw)
To: Lee Jones
Cc: pavel, danielt, jingoohan1, deller, simona, linux-leds, dri-devel,
linux-fbdev
In-Reply-To: <20250211140046.GU1868108@google.com>
Hi
Am 11.02.25 um 15:00 schrieb Lee Jones:
> On Thu, 06 Feb 2025, Thomas Zimmermann wrote:
>
>> Maintain a list of led backlight triggers. This will replace the
>> fbdev notifiers that all backlight triggers currently subscribe to.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> drivers/leds/trigger/ledtrig-backlight.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/leds/trigger/ledtrig-backlight.c b/drivers/leds/trigger/ledtrig-backlight.c
>> index 487577d22cfc..c1c1aa60cf07 100644
>> --- a/drivers/leds/trigger/ledtrig-backlight.c
>> +++ b/drivers/leds/trigger/ledtrig-backlight.c
>> @@ -23,8 +23,13 @@ struct bl_trig_notifier {
>> int old_status;
>> struct notifier_block notifier;
>> unsigned invert;
>> +
>> + struct list_head entry;
> You don't appear to be doing anything with the list here.
>
> It would be better if you introduced the list when it's first utilised.
That's patch 12. I'll merge them.
Best regards
Thomas
>
>> };
>>
>> +static struct list_head ledtrig_backlight_list;
>> +static struct mutex ledtrig_backlight_list_mutex;
>> +
>> static int fb_notifier_callback(struct notifier_block *p,
>> unsigned long event, void *data)
>> {
>> @@ -118,6 +123,10 @@ static int bl_trig_activate(struct led_classdev *led)
>> if (ret)
>> dev_err(led->dev, "unable to register backlight trigger\n");
>>
>> + mutex_lock(&ledtrig_backlight_list_mutex);
>> + list_add(&n->entry, &ledtrig_backlight_list);
>> + mutex_unlock(&ledtrig_backlight_list_mutex);
>> +
>> return 0;
>> }
>>
>> @@ -125,6 +134,10 @@ static void bl_trig_deactivate(struct led_classdev *led)
>> {
>> struct bl_trig_notifier *n = led_get_trigger_data(led);
>>
>> + mutex_lock(&ledtrig_backlight_list_mutex);
>> + list_del(&n->entry);
>> + mutex_unlock(&ledtrig_backlight_list_mutex);
>> +
>> fb_unregister_client(&n->notifier);
>> kfree(n);
>> }
>> --
>> 2.48.1
>>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply
* Re: [PATCH v1 1/2] dt-bindings: mfd: Document TI LM3533 MFD
From: Svyatoslav Ryhel @ 2025-02-13 6:27 UTC (permalink / raw)
To: Conor Dooley
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Lars-Peter Clausen, Pavel Machek,
Daniel Thompson, Jingoo Han, Helge Deller, Andy Shevchenko,
Uwe Kleine-König, devicetree, linux-kernel, linux-iio,
linux-leds, dri-devel, linux-fbdev
In-Reply-To: <20250212-reprint-underfed-fd723ebf3df3@spud>
ср, 12 лют. 2025 р. о 21:49 Conor Dooley <conor@kernel.org> пише:
>
> On Wed, Feb 12, 2025 at 09:58:41AM +0200, Svyatoslav Ryhel wrote:
> > Add bindings for the LM3533 - a complete power source for
> > backlight, keypad, and indicator LEDs in smartphone handsets.
> > The high-voltage inductive boost converter provides the
> > power for two series LED strings display backlight and keypad
> > functions.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> > .../devicetree/bindings/mfd/ti,lm3533.yaml | 221 ++++++++++++++++++
> > include/dt-bindings/mfd/lm3533.h | 19 ++
> > 2 files changed, 240 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> > create mode 100644 include/dt-bindings/mfd/lm3533.h
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> > new file mode 100644
> > index 000000000000..d0307e5894f8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> > @@ -0,0 +1,221 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/ti,lm3533.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: TI LM3533 Complete Lighting Power Solution
> > +
> > +description: |
> > + The LM3533 is a complete power source for backlight,
> > + keypad, and indicator LEDs in smartphone handsets. The
> > + high-voltage inductive boost converter provides the
> > + power for two series LED strings display backlight and
> > + keypad functions.
> > + https://www.ti.com/product/LM3533
> > +
> > +maintainers:
> > + - Johan Hovold <jhovold@gmail.com>
> > +
> > +properties:
> > + compatible:
> > + const: ti,lm3533
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + '#address-cells':
> > + const: 1
> > +
> > + '#size-cells':
> > + const: 0
> > +
> > + enable-gpios:
> > + description: GPIO to use to enable/disable the backlight (HWEN pin).
> > + maxItems: 1
> > +
> > + ti,boost-ovp:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description:
> > + Boost OVP select (16V, 24V, 32V, 40V)
> > + enum: [ 0, 1, 2, 3 ]
> > + default: 0
> > +
> > + ti,boost-freq:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description:
> > + Boost frequency select (500KHz or 1MHz)
> > + enum: [ 0, 1 ]
> > + default: 0
>
> Properties like these should be in units, so some standard voltage-based
> one for the boost and in hertz for the second. See property-units.yaml
> in dt-schema.
>
> > +required:
> > + - compatible
> > + - reg
> > + - '#address-cells'
> > + - '#size-cells'
> > + - enable-gpios
> > +
> > +patternProperties:
> > + "^backlight@[01]$":
> > + type: object
> > + description:
> > + Properties for a backlight device.
> > +
> > + properties:
> > + compatible:
> > + const: lm3533-backlight
>
> Missing vendor prefix
>
> > +
> > + reg:
> > + description: |
> > + The control bank that is used to program the two current sinks. The
> > + LM3533 has two control banks (A and B) and are represented as 0 or 1
> > + in this property. The two current sinks can be controlled
> > + independently with both banks, or bank A can be configured to control
> > + both sinks with the led-sources property.
> > + minimum: 0
> > + maximum: 1
> > +
> > + max-current-microamp:
> > + description:
> > + Maximum current in µA with a 800 µA step.
> > + enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
> > + 10600, 11400, 12200, 13000, 13800, 14600,
> > + 15400, 16200, 17000, 17800, 18600, 19400,
> > + 20200, 21000, 21800, 22600, 23400, 24200,
> > + 25000, 25800, 26600, 27400, 28200, 29000,
> > + 29800 ]
> > + default: 5000
> > +
> > + default-brightness:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description:
> > + Default brightness level on boot.
> > + minimum: 0
> > + maximum: 255
> > + default: 255
> > +
> > + pwm:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description:
> > + Default pwm level on boot.
> > + minimum: 0
> > + maximum: 63
> > + default: 0
>
> Why is default-brightness /and/ a default for pwm needed? If the pwm
> drives the backlight, wouldn't you only need one of these two?
>
> If it stays, I think it needs a rename to be more clear about what it is
> doing. "pwm" is very close to "pwms", the property used for phandles to
> pwm providers but the use is rather different.
>
> backlight/common.yaml has standard properties that you could be using,
> so I'd expect to see a ref here, rather than redefining your own
> version.
>
> > +
> > + required:
> > + - compatible
> > + - reg
> > +
> > + additionalProperties: false
> > +
> > + "^led@[0-3]$":
> > + type: object
> > + description:
> > + Properties for a led device.
> > +
> > + properties:
> > + compatible:
> > + const: lm3533-leds
>
> Ditto here re: compatible.
>
> > +
> > + reg:
> > + description:
> > + 4 led banks
> > + minimum: 0
> > + maximum: 3
> > +
> > + max-current-microamp:
> > + description:
> > + Maximum current in µA with a 800 µA step.
> > + enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
> > + 10600, 11400, 12200, 13000, 13800, 14600,
> > + 15400, 16200, 17000, 17800, 18600, 19400,
> > + 20200, 21000, 21800, 22600, 23400, 24200,
> > + 25000, 25800, 26600, 27400, 28200, 29000,
> > + 29800 ]
> > + default: 5000
> > +
> > + default-trigger:
> > + $ref: /schemas/types.yaml#/definitions/string
> > + description: |
> > + This parameter, if present, is a string defining
> > + the trigger assigned to the LED. Check linux,default-trigger.
> > +
> > + pwm:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description:
> > + Default pwm level on boot.
> > + minimum: 0
> > + maximum: 63
> > + default: 0
>
> Same applies here, leds/common.yaml has some of these already.
>
> > +
> > + required:
> > + - compatible
> > + - reg
> > +
> > + additionalProperties: false
> > +
> > + "^light-sensor@[0]$":
>
> Not a pattern if it can only have 1 outcome.
>
> > + type: object
> > + description:
> > + Properties for an illumination sensor.
> > +
> > + properties:
> > + compatible:
> > + const: lm3533-als
> > +
> > + reg:
> > + const: 0
> > +
> > + resistor-value:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: |
> > + PWM resistor value a linear step in currents
> > + of 10 µA per code based upon 2V/R_ALS.
> > + minimum: 1
> > + maximum: 127
> > + default: 1
> > +
>
> I'd expect a resistor to be measured in ohms of some sort,
> hard to tell what the increments actually are here, they don't appear to
> be a real unit. Are they register values?
>
> This and all other custom properties need to have a vendor prefix on
> them btw.
>
> > + pwm-mode:
> > + type: boolean
> > + description: Mode in which ALS is running
>
> Are there multiple pwm modes? Or is this trying to say that, if set, the
> sensor is in pwm mode? Hard to tell from the description here, sorry.
>
> Cheers,
> Conor.
>
Acknowledged, thank you.
>
> > +
> > + required:
> > + - compatible
> > + - reg
> > +
> > + additionalProperties: false
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > + #include <dt-bindings/mfd/lm3533.h>
> > +
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + led-controller@36 {
> > + compatible = "ti,lm3533";
> > + reg = <0x36>;
> > +
> > + enable-gpios = <&gpio 110 GPIO_ACTIVE_HIGH>;
> > +
> > + ti,boost-ovp = <LM3533_BOOST_OVP_24V>;
> > + ti,boost-freq = <LM3533_BOOST_FREQ_500KHZ>;
> > +
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + backlight@0 {
> > + compatible = "lm3533-backlight";
> > + reg = <0>;
> > +
> > + max-current-microamp = <23400>;
> > + default-brightness = <113>;
> > + pwm = <0x00>;
> > + };
> > + };
> > + };
>
^ permalink raw reply
* RE: [PATCH 1/1] fbdev: hyperv_fb: iounmap() the correct memory when removing a device
From: Michael Kelley @ 2025-02-13 3:43 UTC (permalink / raw)
To: Saurabh Singh Sengar
Cc: haiyangz@microsoft.com, wei.liu@kernel.org, decui@microsoft.com,
deller@gmx.de, weh@microsoft.com, dri-devel@lists.freedesktop.org,
linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org
In-Reply-To: <20250213030650.GA24166@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Wednesday, February 12, 2025 7:07 PM
>
> On Thu, Feb 13, 2025 at 01:35:22AM +0000, Michael Kelley wrote:
> > From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Monday, February 10, 2025 8:52 AM
> > >
> > [snip]
> > > > > >
> > > > > > While we are at it, I want to mention that I also observed below WARN
> > > > > > while removing the hyperv_fb, but that needs a separate fix.
> > > > > >
> > > > > >
> > > > > > [ 44.111220] WARNING: CPU: 35 PID: 1882 at drivers/video/fbdev/core/fb_info.c:70 framebuffer_release+0x2c/0x40
> > > > > > < snip >
> > > > > > [ 44.111289] Call Trace:
> > > > > > [ 44.111290] <TASK>
> > > > > > [ 44.111291] ? show_regs+0x6c/0x80
> > > > > > [ 44.111295] ? __warn+0x8d/0x150
> > > > > > [ 44.111298] ? framebuffer_release+0x2c/0x40
> > > > > > [ 44.111300] ? report_bug+0x182/0x1b0
> > > > > > [ 44.111303] ? handle_bug+0x6e/0xb0
> > > > > > [ 44.111306] ? exc_invalid_op+0x18/0x80
> > > > > > [ 44.111308] ? asm_exc_invalid_op+0x1b/0x20
> > > > > > [ 44.111311] ? framebuffer_release+0x2c/0x40
> > > > > > [ 44.111313] ? hvfb_remove+0x86/0xa0 [hyperv_fb]
> > > > > > [ 44.111315] vmbus_remove+0x24/0x40 [hv_vmbus]
> > > > > > [ 44.111323] device_remove+0x40/0x80
> > > > > > [ 44.111325] device_release_driver_internal+0x20b/0x270
> > > > > > [ 44.111327] ? bus_find_device+0xb3/0xf0
> > > > > >
> > > > >
> > > > > Thanks for pointing this out. Interestingly, I'm not seeing this WARN
> > > > > in my experiments. What base kernel are you testing with? Are you
> > > > > testing on a local VM or in Azure? What exactly are you doing
> > > > > to create the problem? I've been doing unbind of the driver,
> > > > > but maybe you are doing something different.
> > > > >
> > > > > FWIW, there is yet another issue where after doing two unbind/bind
> > > > > cycles of the hyperv_fb driver, there's an error about freeing a
> > > > > non-existent resource. I know what that problem is, and it's in
> > > > > vmbus_drv.c. I'll be submitting a patch for that as soon as I figure out
> > > > > a clean fix.
> > > > >
> > > > > Michael
> > > >
> > > > This is on local Hyper-V. Kernel: 6.14.0-rc1-next-20250205+
> > > > I run below command to reproduce the above error:
> > > > echo "5620e0c7-8062-4dce-aeb7-520c7ef76171" >
> > > /sys/bus/vmbus/devices/5620e0c7-8062-4dce-aeb7-520c7ef76171/driver/unbind
> > > >
> > > > When hvfb_remove is called I can see the refcount for framebuffer is 2 when ,
> > > > I expect it to be 1. After unregistering this framebuffer there is still 1 refcount
> > > > remains, which is the reason for this WARN at the time of framebuffer_release.
> > > >
> > > > I wonder who is registering/using this extra framebuffer. Its not hyperv_drm or
> > > > hyperv_fb IIUC.
> > > >
> > > > - Saurabh
> > >
> > > Here are more details about this WARN:
> > >
> > > Xorg opens `/dev/fb0`, which increases the framebuffer's reference
> > > count, as mentioned above. As a result, when unbinding the driver,
> > > this WARN is expected, indicating that the framebuffer is still in use.
> > >
> > > I am open to suggestion what could be the correct behavior in this case.
> > > There acan be two possible options:
> > >
> > > 1. Check the framebuffer reference count and prevent the driver from
> > > unbinding/removal.
> > > OR
> > >
> > > 2. Allow the driver to unbind while issuing this WARN. (Current scenario)
> > >
> >
> > >From looking at things and doing an experiment, I think there's a 3rd
> > option, which gets rid of the of the WARN while still allowing the unbind.
> >
> > The experiment is to boot Linux in a Gen2 Hyper-V guest with both the
> > Hyper-V FB and Hyper-V DRM modules removed. In this case, the
> > generic EFI framebuffer driver (efifb) should get used. With this driver,
> > a program can open /dev/fb0, and while it is open, unbind the efifb
> > driver (which is in /sys/bus/platform/drivers/efi-framebuffer).
> > Interestingly, there's no WARN generated. But when the hyperv_fb
> > driver is loaded and used, the WARN *is* generated, as you observed.
> >
> > So I looked at the code for efifb. It does the framebuffer_release()
> > call in a function that hyperv_fb doesn't have. Based on the comments
> > in efifb.c, we need a similar function to handle the call to
> > framebuffer_release(). And the efifb driver also does the iounmap()
> > in that same function, which makes we wonder if the hyperv_fb
> > driver should do similarly. It will need a little more analysis to
> > figure that out.
> >
> > You found the bug. Do you want to work on fixing the hyperv_fb
> > driver? And maybe the Hyper-V DRM driver needs the same fix.
> > I haven't looked. Alternatively, if you are busy, I can work on the fix.
> > Let me know your preference.
> >
> > Michael
>
> Thanks for your analysis, its a good to know about fbib driver is not having
> this issue. We can take it as a reference.
>
> At the first look I see efib driver is having a fb_ops.fb_destroy function
> which gets called after put_fb_info (responsible for decrementing the
> ref count).
Yes, that's exactly what I was thinking. If some user space program has
/dev/fb0 open, the driver can be unbound and the unbind will succeed.
The user space program will get an error the next time it tries to reference
the open device file descriptor. Presumably the user space program will
close /dev/fb0 at that point, or just terminate with an error, in which case
Linux will close /dev/fb0 as the user space process terminates. In either
case, fb_info sticks around until that happens and causes the refcount to
be decremented to 1, and then the destroy function is called to do
the final cleanup and free the memory for the fb_info structure.
At least that's what I think happens based on the comments in the
efifb driver. :-) But I have not spent time checking all the details.
> Also it uses devm_register_framebuffer which handles the registration
> and unregister of framebuffer more gracefully.
>
> I will work on this.
>
Sounds good. It's in your court.
Michael
^ permalink raw reply
* Re: [PATCH 1/1] fbdev: hyperv_fb: iounmap() the correct memory when removing a device
From: Saurabh Singh Sengar @ 2025-02-13 3:06 UTC (permalink / raw)
To: Michael Kelley
Cc: haiyangz@microsoft.com, wei.liu@kernel.org, decui@microsoft.com,
deller@gmx.de, weh@microsoft.com, dri-devel@lists.freedesktop.org,
linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org
In-Reply-To: <SN6PR02MB4157C1DF0A0101EEF4CA79E2D4FF2@SN6PR02MB4157.namprd02.prod.outlook.com>
On Thu, Feb 13, 2025 at 01:35:22AM +0000, Michael Kelley wrote:
> From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Monday, February 10, 2025 8:52 AM
> >
> [snip]
> > > > >
> > > > > While we are at it, I want to mention that I also observed below WARN
> > > > > while removing the hyperv_fb, but that needs a separate fix.
> > > > >
> > > > >
> > > > > [ 44.111220] WARNING: CPU: 35 PID: 1882 at drivers/video/fbdev/core/fb_info.c:70 framebuffer_release+0x2c/0x40
> > > > > < snip >
> > > > > [ 44.111289] Call Trace:
> > > > > [ 44.111290] <TASK>
> > > > > [ 44.111291] ? show_regs+0x6c/0x80
> > > > > [ 44.111295] ? __warn+0x8d/0x150
> > > > > [ 44.111298] ? framebuffer_release+0x2c/0x40
> > > > > [ 44.111300] ? report_bug+0x182/0x1b0
> > > > > [ 44.111303] ? handle_bug+0x6e/0xb0
> > > > > [ 44.111306] ? exc_invalid_op+0x18/0x80
> > > > > [ 44.111308] ? asm_exc_invalid_op+0x1b/0x20
> > > > > [ 44.111311] ? framebuffer_release+0x2c/0x40
> > > > > [ 44.111313] ? hvfb_remove+0x86/0xa0 [hyperv_fb]
> > > > > [ 44.111315] vmbus_remove+0x24/0x40 [hv_vmbus]
> > > > > [ 44.111323] device_remove+0x40/0x80
> > > > > [ 44.111325] device_release_driver_internal+0x20b/0x270
> > > > > [ 44.111327] ? bus_find_device+0xb3/0xf0
> > > > >
> > > >
> > > > Thanks for pointing this out. Interestingly, I'm not seeing this WARN
> > > > in my experiments. What base kernel are you testing with? Are you
> > > > testing on a local VM or in Azure? What exactly are you doing
> > > > to create the problem? I've been doing unbind of the driver,
> > > > but maybe you are doing something different.
> > > >
> > > > FWIW, there is yet another issue where after doing two unbind/bind
> > > > cycles of the hyperv_fb driver, there's an error about freeing a
> > > > non-existent resource. I know what that problem is, and it's in
> > > > vmbus_drv.c. I'll be submitting a patch for that as soon as I figure out
> > > > a clean fix.
> > > >
> > > > Michael
> > >
> > > This is on local Hyper-V. Kernel: 6.14.0-rc1-next-20250205+
> > > I run below command to reproduce the above error:
> > > echo "5620e0c7-8062-4dce-aeb7-520c7ef76171" >
> > /sys/bus/vmbus/devices/5620e0c7-8062-4dce-aeb7-520c7ef76171/driver/unbind
> > >
> > > When hvfb_remove is called I can see the refcount for framebuffer is 2 when ,
> > > I expect it to be 1. After unregistering this framebuffer there is still 1 refcount
> > > remains, which is the reason for this WARN at the time of framebuffer_release.
> > >
> > > I wonder who is registering/using this extra framebuffer. Its not hyperv_drm or
> > > hyperv_fb IIUC.
> > >
> > > - Saurabh
> >
> > Here are more details about this WARN:
> >
> > Xorg opens `/dev/fb0`, which increases the framebuffer's reference
> > count, as mentioned above. As a result, when unbinding the driver,
> > this WARN is expected, indicating that the framebuffer is still in use.
> >
> > I am open to suggestion what could be the correct behavior in this case.
> > There acan be two possible options:
> >
> > 1. Check the framebuffer reference count and prevent the driver from
> > unbinding/removal.
> > OR
> >
> > 2. Allow the driver to unbind while issuing this WARN. (Current scenario)
> >
>
> >From looking at things and doing an experiment, I think there's a 3rd
> option, which gets rid of the of the WARN while still allowing the unbind.
>
> The experiment is to boot Linux in a Gen2 Hyper-V guest with both the
> Hyper-V FB and Hyper-V DRM modules removed. In this case, the
> generic EFI framebuffer driver (efifb) should get used. With this driver,
> a program can open /dev/fb0, and while it is open, unbind the efifb
> driver (which is in /sys/bus/platform/drivers/efi-framebuffer).
> Interestingly, there's no WARN generated. But when the hyperv_fb
> driver is loaded and used, the WARN *is* generated, as you observed.
>
> So I looked at the code for efifb. It does the framebuffer_release()
> call in a function that hyperv_fb doesn't have. Based on the comments
> in efifb.c, we need a similar function to handle the call to
> framebuffer_release(). And the efifb driver also does the iounmap()
> in that same function, which makes we wonder if the hyperv_fb
> driver should do similarly. It will need a little more analysis to
> figure that out.
>
> You found the bug. Do you want to work on fixing the hyperv_fb
> driver? And maybe the Hyper-V DRM driver needs the same fix.
> I haven't looked. Alternatively, if you are busy, I can work on the fix.
> Let me know your preference.
>
> Michael
Thanks for your analysis, its a good to know about fbib driver is not having
this issue. We can take it as a reference.
At the first look I see efib driver is having a fb_ops.fb_destroy function
which gets called after put_fb_info (responsible for decrementing the
ref count). Also it uses devm_register_framebuffer which handles the registration
and unregister of framebuffer more gracefully.
I will work on this.
- Saurabh
^ permalink raw reply
* RE: [PATCH 1/1] fbdev: hyperv_fb: iounmap() the correct memory when removing a device
From: Michael Kelley @ 2025-02-13 1:35 UTC (permalink / raw)
To: Saurabh Singh Sengar
Cc: haiyangz@microsoft.com, wei.liu@kernel.org, decui@microsoft.com,
deller@gmx.de, weh@microsoft.com, dri-devel@lists.freedesktop.org,
linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org
In-Reply-To: <20250210165221.GA3465@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Monday, February 10, 2025 8:52 AM
>
[snip]
> > > >
> > > > While we are at it, I want to mention that I also observed below WARN
> > > > while removing the hyperv_fb, but that needs a separate fix.
> > > >
> > > >
> > > > [ 44.111220] WARNING: CPU: 35 PID: 1882 at drivers/video/fbdev/core/fb_info.c:70 framebuffer_release+0x2c/0x40
> > > > < snip >
> > > > [ 44.111289] Call Trace:
> > > > [ 44.111290] <TASK>
> > > > [ 44.111291] ? show_regs+0x6c/0x80
> > > > [ 44.111295] ? __warn+0x8d/0x150
> > > > [ 44.111298] ? framebuffer_release+0x2c/0x40
> > > > [ 44.111300] ? report_bug+0x182/0x1b0
> > > > [ 44.111303] ? handle_bug+0x6e/0xb0
> > > > [ 44.111306] ? exc_invalid_op+0x18/0x80
> > > > [ 44.111308] ? asm_exc_invalid_op+0x1b/0x20
> > > > [ 44.111311] ? framebuffer_release+0x2c/0x40
> > > > [ 44.111313] ? hvfb_remove+0x86/0xa0 [hyperv_fb]
> > > > [ 44.111315] vmbus_remove+0x24/0x40 [hv_vmbus]
> > > > [ 44.111323] device_remove+0x40/0x80
> > > > [ 44.111325] device_release_driver_internal+0x20b/0x270
> > > > [ 44.111327] ? bus_find_device+0xb3/0xf0
> > > >
> > >
> > > Thanks for pointing this out. Interestingly, I'm not seeing this WARN
> > > in my experiments. What base kernel are you testing with? Are you
> > > testing on a local VM or in Azure? What exactly are you doing
> > > to create the problem? I've been doing unbind of the driver,
> > > but maybe you are doing something different.
> > >
> > > FWIW, there is yet another issue where after doing two unbind/bind
> > > cycles of the hyperv_fb driver, there's an error about freeing a
> > > non-existent resource. I know what that problem is, and it's in
> > > vmbus_drv.c. I'll be submitting a patch for that as soon as I figure out
> > > a clean fix.
> > >
> > > Michael
> >
> > This is on local Hyper-V. Kernel: 6.14.0-rc1-next-20250205+
> > I run below command to reproduce the above error:
> > echo "5620e0c7-8062-4dce-aeb7-520c7ef76171" >
> /sys/bus/vmbus/devices/5620e0c7-8062-4dce-aeb7-520c7ef76171/driver/unbind
> >
> > When hvfb_remove is called I can see the refcount for framebuffer is 2 when ,
> > I expect it to be 1. After unregistering this framebuffer there is still 1 refcount
> > remains, which is the reason for this WARN at the time of framebuffer_release.
> >
> > I wonder who is registering/using this extra framebuffer. Its not hyperv_drm or
> > hyperv_fb IIUC.
> >
> > - Saurabh
>
> Here are more details about this WARN:
>
> Xorg opens `/dev/fb0`, which increases the framebuffer's reference
> count, as mentioned above. As a result, when unbinding the driver,
> this WARN is expected, indicating that the framebuffer is still in use.
>
> I am open to suggestion what could be the correct behavior in this case.
> There acan be two possible options:
>
> 1. Check the framebuffer reference count and prevent the driver from
> unbinding/removal.
> OR
>
> 2. Allow the driver to unbind while issuing this WARN. (Current scenario)
>
From looking at things and doing an experiment, I think there's a 3rd
option, which gets rid of the of the WARN while still allowing the unbind.
The experiment is to boot Linux in a Gen2 Hyper-V guest with both the
Hyper-V FB and Hyper-V DRM modules removed. In this case, the
generic EFI framebuffer driver (efifb) should get used. With this driver,
a program can open /dev/fb0, and while it is open, unbind the efifb
driver (which is in /sys/bus/platform/drivers/efi-framebuffer).
Interestingly, there's no WARN generated. But when the hyperv_fb
driver is loaded and used, the WARN *is* generated, as you observed.
So I looked at the code for efifb. It does the framebuffer_release()
call in a function that hyperv_fb doesn't have. Based on the comments
in efifb.c, we need a similar function to handle the call to
framebuffer_release(). And the efifb driver also does the iounmap()
in that same function, which makes we wonder if the hyperv_fb
driver should do similarly. It will need a little more analysis to
figure that out.
You found the bug. Do you want to work on fixing the hyperv_fb
driver? And maybe the Hyper-V DRM driver needs the same fix.
I haven't looked. Alternatively, if you are busy, I can work on the fix.
Let me know your preference.
Michael
^ permalink raw reply
* Re: [PATCH v1 1/2] dt-bindings: mfd: Document TI LM3533 MFD
From: Conor Dooley @ 2025-02-12 19:49 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Lars-Peter Clausen, Pavel Machek,
Daniel Thompson, Jingoo Han, Helge Deller, Andy Shevchenko,
Uwe Kleine-König, devicetree, linux-kernel, linux-iio,
linux-leds, dri-devel, linux-fbdev
In-Reply-To: <20250212075845.11338-2-clamor95@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8469 bytes --]
On Wed, Feb 12, 2025 at 09:58:41AM +0200, Svyatoslav Ryhel wrote:
> Add bindings for the LM3533 - a complete power source for
> backlight, keypad, and indicator LEDs in smartphone handsets.
> The high-voltage inductive boost converter provides the
> power for two series LED strings display backlight and keypad
> functions.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> .../devicetree/bindings/mfd/ti,lm3533.yaml | 221 ++++++++++++++++++
> include/dt-bindings/mfd/lm3533.h | 19 ++
> 2 files changed, 240 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> create mode 100644 include/dt-bindings/mfd/lm3533.h
>
> diff --git a/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> new file mode 100644
> index 000000000000..d0307e5894f8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> @@ -0,0 +1,221 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/ti,lm3533.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI LM3533 Complete Lighting Power Solution
> +
> +description: |
> + The LM3533 is a complete power source for backlight,
> + keypad, and indicator LEDs in smartphone handsets. The
> + high-voltage inductive boost converter provides the
> + power for two series LED strings display backlight and
> + keypad functions.
> + https://www.ti.com/product/LM3533
> +
> +maintainers:
> + - Johan Hovold <jhovold@gmail.com>
> +
> +properties:
> + compatible:
> + const: ti,lm3533
> +
> + reg:
> + maxItems: 1
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> + enable-gpios:
> + description: GPIO to use to enable/disable the backlight (HWEN pin).
> + maxItems: 1
> +
> + ti,boost-ovp:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Boost OVP select (16V, 24V, 32V, 40V)
> + enum: [ 0, 1, 2, 3 ]
> + default: 0
> +
> + ti,boost-freq:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Boost frequency select (500KHz or 1MHz)
> + enum: [ 0, 1 ]
> + default: 0
Properties like these should be in units, so some standard voltage-based
one for the boost and in hertz for the second. See property-units.yaml
in dt-schema.
> +required:
> + - compatible
> + - reg
> + - '#address-cells'
> + - '#size-cells'
> + - enable-gpios
> +
> +patternProperties:
> + "^backlight@[01]$":
> + type: object
> + description:
> + Properties for a backlight device.
> +
> + properties:
> + compatible:
> + const: lm3533-backlight
Missing vendor prefix
> +
> + reg:
> + description: |
> + The control bank that is used to program the two current sinks. The
> + LM3533 has two control banks (A and B) and are represented as 0 or 1
> + in this property. The two current sinks can be controlled
> + independently with both banks, or bank A can be configured to control
> + both sinks with the led-sources property.
> + minimum: 0
> + maximum: 1
> +
> + max-current-microamp:
> + description:
> + Maximum current in µA with a 800 µA step.
> + enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
> + 10600, 11400, 12200, 13000, 13800, 14600,
> + 15400, 16200, 17000, 17800, 18600, 19400,
> + 20200, 21000, 21800, 22600, 23400, 24200,
> + 25000, 25800, 26600, 27400, 28200, 29000,
> + 29800 ]
> + default: 5000
> +
> + default-brightness:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Default brightness level on boot.
> + minimum: 0
> + maximum: 255
> + default: 255
> +
> + pwm:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Default pwm level on boot.
> + minimum: 0
> + maximum: 63
> + default: 0
Why is default-brightness /and/ a default for pwm needed? If the pwm
drives the backlight, wouldn't you only need one of these two?
If it stays, I think it needs a rename to be more clear about what it is
doing. "pwm" is very close to "pwms", the property used for phandles to
pwm providers but the use is rather different.
backlight/common.yaml has standard properties that you could be using,
so I'd expect to see a ref here, rather than redefining your own
version.
> +
> + required:
> + - compatible
> + - reg
> +
> + additionalProperties: false
> +
> + "^led@[0-3]$":
> + type: object
> + description:
> + Properties for a led device.
> +
> + properties:
> + compatible:
> + const: lm3533-leds
Ditto here re: compatible.
> +
> + reg:
> + description:
> + 4 led banks
> + minimum: 0
> + maximum: 3
> +
> + max-current-microamp:
> + description:
> + Maximum current in µA with a 800 µA step.
> + enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
> + 10600, 11400, 12200, 13000, 13800, 14600,
> + 15400, 16200, 17000, 17800, 18600, 19400,
> + 20200, 21000, 21800, 22600, 23400, 24200,
> + 25000, 25800, 26600, 27400, 28200, 29000,
> + 29800 ]
> + default: 5000
> +
> + default-trigger:
> + $ref: /schemas/types.yaml#/definitions/string
> + description: |
> + This parameter, if present, is a string defining
> + the trigger assigned to the LED. Check linux,default-trigger.
> +
> + pwm:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Default pwm level on boot.
> + minimum: 0
> + maximum: 63
> + default: 0
Same applies here, leds/common.yaml has some of these already.
> +
> + required:
> + - compatible
> + - reg
> +
> + additionalProperties: false
> +
> + "^light-sensor@[0]$":
Not a pattern if it can only have 1 outcome.
> + type: object
> + description:
> + Properties for an illumination sensor.
> +
> + properties:
> + compatible:
> + const: lm3533-als
> +
> + reg:
> + const: 0
> +
> + resistor-value:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: |
> + PWM resistor value a linear step in currents
> + of 10 µA per code based upon 2V/R_ALS.
> + minimum: 1
> + maximum: 127
> + default: 1
> +
I'd expect a resistor to be measured in ohms of some sort,
hard to tell what the increments actually are here, they don't appear to
be a real unit. Are they register values?
This and all other custom properties need to have a vendor prefix on
them btw.
> + pwm-mode:
> + type: boolean
> + description: Mode in which ALS is running
Are there multiple pwm modes? Or is this trying to say that, if set, the
sensor is in pwm mode? Hard to tell from the description here, sorry.
Cheers,
Conor.
> +
> + required:
> + - compatible
> + - reg
> +
> + additionalProperties: false
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/mfd/lm3533.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led-controller@36 {
> + compatible = "ti,lm3533";
> + reg = <0x36>;
> +
> + enable-gpios = <&gpio 110 GPIO_ACTIVE_HIGH>;
> +
> + ti,boost-ovp = <LM3533_BOOST_OVP_24V>;
> + ti,boost-freq = <LM3533_BOOST_FREQ_500KHZ>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + backlight@0 {
> + compatible = "lm3533-backlight";
> + reg = <0>;
> +
> + max-current-microamp = <23400>;
> + default-brightness = <113>;
> + pwm = <0x00>;
> + };
> + };
> + };
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v1 0/2] mfd: lm3533: convert to use OF
From: Andy Shevchenko @ 2025-02-12 11:02 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Lars-Peter Clausen, Pavel Machek,
Daniel Thompson, Jingoo Han, Helge Deller, Uwe Kleine-König,
devicetree, linux-kernel, linux-iio, linux-leds, dri-devel,
linux-fbdev
In-Reply-To: <20250212075845.11338-1-clamor95@gmail.com>
On Wed, Feb 12, 2025 at 09:58:40AM +0200, Svyatoslav Ryhel wrote:
> Add schema and add support for lm3533 mfd to use device
> tree bindings.
Thank you! I'm going to review the series this week, I definitely have
the comments. Stay tuned.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v2] staging: sm750fb: fix checkpatch warning architecture specific defines should be avoided
From: Michael Anckaert @ 2025-02-12 10:12 UTC (permalink / raw)
To: Sudip Mukherjee, Teddy Wang, Greg Kroah-Hartman,
open list:STAGING - SILICON MOTION SM750 FRAME BUFFER DRIVER,
open list:STAGING SUBSYSTEM, linux-kernel
Replace architecture-specific defines with CONFIG_X86 checks to improve
portability and adhere to kernel coding standards.
Fixes checkpatch warning:
- CHECK: architecture specific defines should be avoided.
Changes made:
- Using CONFIG_X86 instead of i386 and x86.
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Michael Anckaert <michael.anckaert@gmail.com>
---
Changes in v2:
- Moved the '} else {' into the ifdef to avoid the possibly empty
else branch
drivers/staging/sm750fb/ddk750_chip.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/sm750fb/ddk750_chip.c b/drivers/staging/sm750fb/ddk750_chip.c
index 02860d3ec365..025dae3756aa 100644
--- a/drivers/staging/sm750fb/ddk750_chip.c
+++ b/drivers/staging/sm750fb/ddk750_chip.c
@@ -228,8 +228,8 @@ int ddk750_init_hw(struct initchip_param *p_init_param)
reg = peek32(VGA_CONFIGURATION);
reg |= (VGA_CONFIGURATION_PLL | VGA_CONFIGURATION_MODE);
poke32(VGA_CONFIGURATION, reg);
+#ifdef CONFIG_X86
} else {
-#if defined(__i386__) || defined(__x86_64__)
/* set graphic mode via IO method */
outb_p(0x88, 0x3d4);
outb_p(0x06, 0x3d5);
--
2.39.5
^ permalink raw reply related
* Re: [PATCH] staging: sm750fb: fix checkpatch warning architecture specific defines should be avoided
From: Thomas Zimmermann @ 2025-02-12 8:00 UTC (permalink / raw)
To: Michael Anckaert, Sudip Mukherjee, Teddy Wang, Greg Kroah-Hartman,
open list:STAGING - SILICON MOTION SM750 FRAME BUFFER DRIVER,
open list:STAGING SUBSYSTEM, open list
In-Reply-To: <Z6xREbLLtexX4_uh@michael-devbox>
Hi
Am 12.02.25 um 08:43 schrieb Michael Anckaert:
> Replace architecture-specific defines with CONFIG_X86 checks to improve
> portability and adhere to kernel coding standards.
>
> Fixes checkpatch warning:
> - CHECK: architecture specific defines should be avoided.
>
> Changes made:
> - Using CONFIG_X86 instead of i386 and x86.
>
> Signed-off-by: Michael Anckaert <michael.anckaert@gmail.com>
> ---
> drivers/staging/sm750fb/ddk750_chip.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/sm750fb/ddk750_chip.c b/drivers/staging/sm750fb/ddk750_chip.c
> index 02860d3ec365..67a2f60440ca 100644
> --- a/drivers/staging/sm750fb/ddk750_chip.c
> +++ b/drivers/staging/sm750fb/ddk750_chip.c
> @@ -229,7 +229,7 @@ int ddk750_init_hw(struct initchip_param *p_init_param)
> reg |= (VGA_CONFIGURATION_PLL | VGA_CONFIGURATION_MODE);
> poke32(VGA_CONFIGURATION, reg);
> } else {
> -#if defined(__i386__) || defined(__x86_64__)
> +#ifdef CONFIG_X86
> /* set graphic mode via IO method */
> outb_p(0x88, 0x3d4);
> outb_p(0x06, 0x3d5);
Maybe move the '} else {' into the ifdef to avoid the possibly empty
else branch. I also wonder why this is only being done on x86. There are
other systems with VGA.
In any case
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Best regards
Thomas
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply
* [PATCH v1 2/2] mfd: lm3533: convert to use OF
From: Svyatoslav Ryhel @ 2025-02-12 7:58 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Lars-Peter Clausen, Pavel Machek,
Daniel Thompson, Jingoo Han, Helge Deller, Svyatoslav Ryhel,
Andy Shevchenko, Uwe Kleine-König
Cc: devicetree, linux-kernel, linux-iio, linux-leds, dri-devel,
linux-fbdev
In-Reply-To: <20250212075845.11338-1-clamor95@gmail.com>
Add ability to fill pdata from device tree. Common stuff is
filled from core driver and then pdata is filled per-device
since all cells are optional.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/iio/light/lm3533-als.c | 58 ++++++++++++++++++++-
drivers/leds/leds-lm3533.c | 69 +++++++++++++++++++++++--
drivers/mfd/lm3533-core.c | 79 ++++++++++++++++++++++++-----
drivers/video/backlight/lm3533_bl.c | 72 ++++++++++++++++++++++++--
include/linux/mfd/lm3533.h | 1 +
5 files changed, 256 insertions(+), 23 deletions(-)
diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
index 99f0b903018c..21fc5f215c80 100644
--- a/drivers/iio/light/lm3533-als.c
+++ b/drivers/iio/light/lm3533-als.c
@@ -16,7 +16,9 @@
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/mfd/core.h>
+#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/slab.h>
#include <linux/uaccess.h>
@@ -826,6 +828,50 @@ static const struct iio_info lm3533_als_info = {
.read_raw = &lm3533_als_read_raw,
};
+static void lm3533_parse_als(struct platform_device *pdev,
+ struct lm3533_als_platform_data *pdata)
+{
+ struct device *dev = &pdev->dev;
+ int val, ret;
+
+ /* 1 - 127 (ignored in PWM-mode) */
+ ret = device_property_read_u32(dev, "resistor-value", &val);
+ if (ret)
+ val = 1;
+ pdata->r_select = val;
+
+ pdata->pwm_mode = device_property_read_bool(dev, "pwm-mode");
+}
+
+static int lm3533_pass_of_node(struct platform_device *pdev,
+ struct lm3533_als_platform_data *pdata)
+{
+ struct device *parent_dev = pdev->dev.parent;
+ struct device *dev = &pdev->dev;
+ struct fwnode_handle *node;
+ const char *label;
+ int val, ret;
+
+ device_for_each_child_node(parent_dev, node) {
+ fwnode_property_read_string(node, "compatible", &label);
+
+ if (!strcmp(label, pdev->name)) {
+ ret = fwnode_property_read_u32(node, "reg", &val);
+ if (ret) {
+ dev_err(dev, "reg property is missing: ret %d\n", ret);
+ return ret;
+ }
+
+ if (val == pdev->id) {
+ dev->fwnode = node;
+ dev->of_node = to_of_node(node);
+ }
+ }
+ }
+
+ return 0;
+}
+
static int lm3533_als_probe(struct platform_device *pdev)
{
const struct lm3533_als_platform_data *pdata;
@@ -840,8 +886,16 @@ static int lm3533_als_probe(struct platform_device *pdev)
pdata = dev_get_platdata(&pdev->dev);
if (!pdata) {
- dev_err(&pdev->dev, "no platform data\n");
- return -EINVAL;
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ ret = lm3533_pass_of_node(pdev, pdata);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "failed to get als device node\n");
+
+ lm3533_parse_als(pdev, pdata);
}
indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als));
diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
index 45795f2a1042..587bc27b17c3 100644
--- a/drivers/leds/leds-lm3533.c
+++ b/drivers/leds/leds-lm3533.c
@@ -11,7 +11,9 @@
#include <linux/leds.h>
#include <linux/mfd/core.h>
#include <linux/mutex.h>
+#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/slab.h>
#include <linux/mfd/lm3533.h>
@@ -644,6 +646,59 @@ static int lm3533_led_setup(struct lm3533_led *led,
return lm3533_ctrlbank_set_pwm(&led->cb, pdata->pwm);
}
+static void lm3533_parse_led(struct platform_device *pdev,
+ struct lm3533_led_platform_data *pdata)
+{
+ struct device *dev = &pdev->dev;
+ int val, ret;
+
+ ret = device_property_read_string(dev, "default-trigger",
+ &pdata->default_trigger);
+ if (ret)
+ pdata->default_trigger = "none";
+
+ /* 5000 - 29800 uA (800 uA step) */
+ ret = device_property_read_u32(dev, "max-current-microamp", &val);
+ if (ret)
+ val = 5000;
+ pdata->max_current = val;
+
+ /* 0 - 0x3f */
+ ret = device_property_read_u32(dev, "pwm", &val);
+ if (ret)
+ val = 0;
+ pdata->pwm = val;
+}
+
+static int lm3533_pass_of_node(struct platform_device *pdev,
+ struct lm3533_led_platform_data *pdata)
+{
+ struct device *parent_dev = pdev->dev.parent;
+ struct device *dev = &pdev->dev;
+ struct fwnode_handle *node;
+ const char *label;
+ int val, ret;
+
+ device_for_each_child_node(parent_dev, node) {
+ fwnode_property_read_string(node, "compatible", &label);
+
+ if (!strcmp(label, pdev->name)) {
+ ret = fwnode_property_read_u32(node, "reg", &val);
+ if (ret) {
+ dev_info(dev, "reg property is missing: ret %d\n", ret);
+ return ret;
+ }
+
+ if (val == pdev->id) {
+ dev->fwnode = node;
+ dev->of_node = to_of_node(node);
+ }
+ }
+ }
+
+ return 0;
+}
+
static int lm3533_led_probe(struct platform_device *pdev)
{
struct lm3533 *lm3533;
@@ -659,8 +714,16 @@ static int lm3533_led_probe(struct platform_device *pdev)
pdata = dev_get_platdata(&pdev->dev);
if (!pdata) {
- dev_err(&pdev->dev, "no platform data\n");
- return -EINVAL;
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ ret = lm3533_pass_of_node(pdev, pdata);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "failed to get led device node\n");
+
+ lm3533_parse_led(pdev, pdata);
}
if (pdev->id < 0 || pdev->id >= LM3533_LVCTRLBANK_COUNT) {
@@ -673,7 +736,7 @@ static int lm3533_led_probe(struct platform_device *pdev)
return -ENOMEM;
led->lm3533 = lm3533;
- led->cdev.name = pdata->name;
+ led->cdev.name = dev_name(&pdev->dev);
led->cdev.default_trigger = pdata->default_trigger;
led->cdev.brightness_set_blocking = lm3533_led_set;
led->cdev.brightness_get = lm3533_led_get;
diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
index 0a2409d00b2e..cab60c25e3c7 100644
--- a/drivers/mfd/lm3533-core.c
+++ b/drivers/mfd/lm3533-core.c
@@ -381,11 +381,16 @@ static int lm3533_device_als_init(struct lm3533 *lm3533)
struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
int ret;
- if (!pdata->als)
+ if (!pdata->num_als)
return 0;
- lm3533_als_devs[0].platform_data = pdata->als;
- lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
+ if (pdata->num_als > ARRAY_SIZE(lm3533_als_devs))
+ pdata->num_als = ARRAY_SIZE(lm3533_als_devs);
+
+ if (pdata->als) {
+ lm3533_als_devs[0].platform_data = pdata->als;
+ lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
+ }
ret = mfd_add_devices(lm3533->dev, 0, lm3533_als_devs, 1, NULL,
0, NULL);
@@ -405,15 +410,17 @@ static int lm3533_device_bl_init(struct lm3533 *lm3533)
int i;
int ret;
- if (!pdata->backlights || pdata->num_backlights == 0)
+ if (!pdata->num_backlights)
return 0;
if (pdata->num_backlights > ARRAY_SIZE(lm3533_bl_devs))
pdata->num_backlights = ARRAY_SIZE(lm3533_bl_devs);
- for (i = 0; i < pdata->num_backlights; ++i) {
- lm3533_bl_devs[i].platform_data = &pdata->backlights[i];
- lm3533_bl_devs[i].pdata_size = sizeof(pdata->backlights[i]);
+ if (pdata->backlights) {
+ for (i = 0; i < pdata->num_backlights; ++i) {
+ lm3533_bl_devs[i].platform_data = &pdata->backlights[i];
+ lm3533_bl_devs[i].pdata_size = sizeof(pdata->backlights[i]);
+ }
}
ret = mfd_add_devices(lm3533->dev, 0, lm3533_bl_devs,
@@ -434,15 +441,17 @@ static int lm3533_device_led_init(struct lm3533 *lm3533)
int i;
int ret;
- if (!pdata->leds || pdata->num_leds == 0)
+ if (!pdata->num_leds)
return 0;
if (pdata->num_leds > ARRAY_SIZE(lm3533_led_devs))
pdata->num_leds = ARRAY_SIZE(lm3533_led_devs);
- for (i = 0; i < pdata->num_leds; ++i) {
- lm3533_led_devs[i].platform_data = &pdata->leds[i];
- lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
+ if (pdata->leds) {
+ for (i = 0; i < pdata->num_leds; ++i) {
+ lm3533_led_devs[i].platform_data = &pdata->leds[i];
+ lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
+ }
}
ret = mfd_add_devices(lm3533->dev, 0, lm3533_led_devs,
@@ -469,6 +478,26 @@ static int lm3533_device_setup(struct lm3533 *lm3533,
return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
}
+static void lm3533_parse_nodes(struct lm3533 *lm3533,
+ struct lm3533_platform_data *pdata)
+{
+ struct fwnode_handle *node;
+ const char *label;
+
+ device_for_each_child_node(lm3533->dev, node) {
+ fwnode_property_read_string(node, "compatible", &label);
+
+ if (!strcmp(label, lm3533_bl_devs[pdata->num_backlights].name))
+ pdata->num_backlights++;
+
+ if (!strcmp(label, lm3533_led_devs[pdata->num_leds].name))
+ pdata->num_leds++;
+
+ if (!strcmp(label, lm3533_als_devs[pdata->num_als].name))
+ pdata->num_als++;
+ }
+}
+
static int lm3533_device_init(struct lm3533 *lm3533)
{
struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
@@ -477,8 +506,25 @@ static int lm3533_device_init(struct lm3533 *lm3533)
dev_dbg(lm3533->dev, "%s\n", __func__);
if (!pdata) {
- dev_err(lm3533->dev, "no platform data\n");
- return -EINVAL;
+ pdata = devm_kzalloc(lm3533->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ ret = device_property_read_u32(lm3533->dev,
+ "ti,boost-ovp",
+ &pdata->boost_ovp);
+ if (ret)
+ pdata->boost_ovp = LM3533_BOOST_OVP_16V;
+
+ ret = device_property_read_u32(lm3533->dev,
+ "ti,boost-freq",
+ &pdata->boost_freq);
+ if (ret)
+ pdata->boost_freq = LM3533_BOOST_FREQ_500KHZ;
+
+ lm3533_parse_nodes(lm3533, pdata);
+
+ lm3533->dev->platform_data = pdata;
}
lm3533->hwen = devm_gpiod_get(lm3533->dev, NULL, GPIOD_OUT_LOW);
@@ -603,6 +649,12 @@ static void lm3533_i2c_remove(struct i2c_client *i2c)
lm3533_device_exit(lm3533);
}
+static const struct of_device_id lm3533_match_table[] = {
+ { .compatible = "ti,lm3533" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, lm3533_match_table);
+
static const struct i2c_device_id lm3533_i2c_ids[] = {
{ "lm3533" },
{ }
@@ -612,6 +664,7 @@ MODULE_DEVICE_TABLE(i2c, lm3533_i2c_ids);
static struct i2c_driver lm3533_i2c_driver = {
.driver = {
.name = "lm3533",
+ .of_match_table = lm3533_match_table,
},
.id_table = lm3533_i2c_ids,
.probe = lm3533_i2c_probe,
diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
index babfd3ceec86..bc568916f1b1 100644
--- a/drivers/video/backlight/lm3533_bl.c
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -258,6 +258,60 @@ static int lm3533_bl_setup(struct lm3533_bl *bl,
return lm3533_ctrlbank_set_pwm(&bl->cb, pdata->pwm);
}
+static void lm3533_parse_backlight(struct platform_device *pdev,
+ struct lm3533_bl_platform_data *pdata)
+{
+ struct device *dev = &pdev->dev;
+ int val, ret;
+
+ /* 5000 - 29800 uA (800 uA step) */
+ ret = device_property_read_u32(dev, "max-current-microamp", &val);
+ if (ret)
+ val = 5000;
+ pdata->max_current = val;
+
+ /* 0 - 255 */
+ ret = device_property_read_u32(dev, "default-brightness", &val);
+ if (ret)
+ val = LM3533_BL_MAX_BRIGHTNESS;
+ pdata->default_brightness = val;
+
+ /* 0 - 0x3f */
+ ret = device_property_read_u32(dev, "pwm", &val);
+ if (ret)
+ val = 0;
+ pdata->pwm = val;
+}
+
+static int lm3533_pass_of_node(struct platform_device *pdev,
+ struct lm3533_bl_platform_data *pdata)
+{
+ struct device *parent_dev = pdev->dev.parent;
+ struct device *dev = &pdev->dev;
+ struct fwnode_handle *node;
+ const char *label;
+ int val, ret;
+
+ device_for_each_child_node(parent_dev, node) {
+ fwnode_property_read_string(node, "compatible", &label);
+
+ if (!strcmp(label, pdev->name)) {
+ ret = fwnode_property_read_u32(node, "reg", &val);
+ if (ret) {
+ dev_info(dev, "reg property is missing: ret %d\n", ret);
+ return ret;
+ }
+
+ if (val == pdev->id) {
+ dev->fwnode = node;
+ dev->of_node = to_of_node(node);
+ }
+ }
+ }
+
+ return 0;
+}
+
static int lm3533_bl_probe(struct platform_device *pdev)
{
struct lm3533 *lm3533;
@@ -275,8 +329,16 @@ static int lm3533_bl_probe(struct platform_device *pdev)
pdata = dev_get_platdata(&pdev->dev);
if (!pdata) {
- dev_err(&pdev->dev, "no platform data\n");
- return -EINVAL;
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ ret = lm3533_pass_of_node(pdev, pdata);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "failed to get backlight device node\n");
+
+ lm3533_parse_backlight(pdev, pdata);
}
if (pdev->id < 0 || pdev->id >= LM3533_HVCTRLBANK_COUNT) {
@@ -299,9 +361,9 @@ static int lm3533_bl_probe(struct platform_device *pdev)
props.type = BACKLIGHT_RAW;
props.max_brightness = LM3533_BL_MAX_BRIGHTNESS;
props.brightness = pdata->default_brightness;
- bd = devm_backlight_device_register(&pdev->dev, pdata->name,
- pdev->dev.parent, bl, &lm3533_bl_ops,
- &props);
+ bd = devm_backlight_device_register(&pdev->dev, dev_name(&pdev->dev),
+ pdev->dev.parent, bl,
+ &lm3533_bl_ops, &props);
if (IS_ERR(bd)) {
dev_err(&pdev->dev, "failed to register backlight device\n");
return PTR_ERR(bd);
diff --git a/include/linux/mfd/lm3533.h b/include/linux/mfd/lm3533.h
index 69059a7a2ce5..e22fd2093a95 100644
--- a/include/linux/mfd/lm3533.h
+++ b/include/linux/mfd/lm3533.h
@@ -74,6 +74,7 @@ struct lm3533_platform_data {
enum lm3533_boost_freq boost_freq;
struct lm3533_als_platform_data *als;
+ int num_als;
struct lm3533_bl_platform_data *backlights;
int num_backlights;
--
2.43.0
^ permalink raw reply related
* [PATCH v1 1/2] dt-bindings: mfd: Document TI LM3533 MFD
From: Svyatoslav Ryhel @ 2025-02-12 7:58 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Lars-Peter Clausen, Pavel Machek,
Daniel Thompson, Jingoo Han, Helge Deller, Svyatoslav Ryhel,
Andy Shevchenko, Uwe Kleine-König
Cc: devicetree, linux-kernel, linux-iio, linux-leds, dri-devel,
linux-fbdev
In-Reply-To: <20250212075845.11338-1-clamor95@gmail.com>
Add bindings for the LM3533 - a complete power source for
backlight, keypad, and indicator LEDs in smartphone handsets.
The high-voltage inductive boost converter provides the
power for two series LED strings display backlight and keypad
functions.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
.../devicetree/bindings/mfd/ti,lm3533.yaml | 221 ++++++++++++++++++
include/dt-bindings/mfd/lm3533.h | 19 ++
2 files changed, 240 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
create mode 100644 include/dt-bindings/mfd/lm3533.h
diff --git a/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
new file mode 100644
index 000000000000..d0307e5894f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
@@ -0,0 +1,221 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/ti,lm3533.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI LM3533 Complete Lighting Power Solution
+
+description: |
+ The LM3533 is a complete power source for backlight,
+ keypad, and indicator LEDs in smartphone handsets. The
+ high-voltage inductive boost converter provides the
+ power for two series LED strings display backlight and
+ keypad functions.
+ https://www.ti.com/product/LM3533
+
+maintainers:
+ - Johan Hovold <jhovold@gmail.com>
+
+properties:
+ compatible:
+ const: ti,lm3533
+
+ reg:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ enable-gpios:
+ description: GPIO to use to enable/disable the backlight (HWEN pin).
+ maxItems: 1
+
+ ti,boost-ovp:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Boost OVP select (16V, 24V, 32V, 40V)
+ enum: [ 0, 1, 2, 3 ]
+ default: 0
+
+ ti,boost-freq:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Boost frequency select (500KHz or 1MHz)
+ enum: [ 0, 1 ]
+ default: 0
+
+required:
+ - compatible
+ - reg
+ - '#address-cells'
+ - '#size-cells'
+ - enable-gpios
+
+patternProperties:
+ "^backlight@[01]$":
+ type: object
+ description:
+ Properties for a backlight device.
+
+ properties:
+ compatible:
+ const: lm3533-backlight
+
+ reg:
+ description: |
+ The control bank that is used to program the two current sinks. The
+ LM3533 has two control banks (A and B) and are represented as 0 or 1
+ in this property. The two current sinks can be controlled
+ independently with both banks, or bank A can be configured to control
+ both sinks with the led-sources property.
+ minimum: 0
+ maximum: 1
+
+ max-current-microamp:
+ description:
+ Maximum current in µA with a 800 µA step.
+ enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
+ 10600, 11400, 12200, 13000, 13800, 14600,
+ 15400, 16200, 17000, 17800, 18600, 19400,
+ 20200, 21000, 21800, 22600, 23400, 24200,
+ 25000, 25800, 26600, 27400, 28200, 29000,
+ 29800 ]
+ default: 5000
+
+ default-brightness:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Default brightness level on boot.
+ minimum: 0
+ maximum: 255
+ default: 255
+
+ pwm:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Default pwm level on boot.
+ minimum: 0
+ maximum: 63
+ default: 0
+
+ required:
+ - compatible
+ - reg
+
+ additionalProperties: false
+
+ "^led@[0-3]$":
+ type: object
+ description:
+ Properties for a led device.
+
+ properties:
+ compatible:
+ const: lm3533-leds
+
+ reg:
+ description:
+ 4 led banks
+ minimum: 0
+ maximum: 3
+
+ max-current-microamp:
+ description:
+ Maximum current in µA with a 800 µA step.
+ enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
+ 10600, 11400, 12200, 13000, 13800, 14600,
+ 15400, 16200, 17000, 17800, 18600, 19400,
+ 20200, 21000, 21800, 22600, 23400, 24200,
+ 25000, 25800, 26600, 27400, 28200, 29000,
+ 29800 ]
+ default: 5000
+
+ default-trigger:
+ $ref: /schemas/types.yaml#/definitions/string
+ description: |
+ This parameter, if present, is a string defining
+ the trigger assigned to the LED. Check linux,default-trigger.
+
+ pwm:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Default pwm level on boot.
+ minimum: 0
+ maximum: 63
+ default: 0
+
+ required:
+ - compatible
+ - reg
+
+ additionalProperties: false
+
+ "^light-sensor@[0]$":
+ type: object
+ description:
+ Properties for an illumination sensor.
+
+ properties:
+ compatible:
+ const: lm3533-als
+
+ reg:
+ const: 0
+
+ resistor-value:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ PWM resistor value a linear step in currents
+ of 10 µA per code based upon 2V/R_ALS.
+ minimum: 1
+ maximum: 127
+ default: 1
+
+ pwm-mode:
+ type: boolean
+ description: Mode in which ALS is running
+
+ required:
+ - compatible
+ - reg
+
+ additionalProperties: false
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/mfd/lm3533.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led-controller@36 {
+ compatible = "ti,lm3533";
+ reg = <0x36>;
+
+ enable-gpios = <&gpio 110 GPIO_ACTIVE_HIGH>;
+
+ ti,boost-ovp = <LM3533_BOOST_OVP_24V>;
+ ti,boost-freq = <LM3533_BOOST_FREQ_500KHZ>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ backlight@0 {
+ compatible = "lm3533-backlight";
+ reg = <0>;
+
+ max-current-microamp = <23400>;
+ default-brightness = <113>;
+ pwm = <0x00>;
+ };
+ };
+ };
+...
diff --git a/include/dt-bindings/mfd/lm3533.h b/include/dt-bindings/mfd/lm3533.h
new file mode 100644
index 000000000000..929988190c52
--- /dev/null
+++ b/include/dt-bindings/mfd/lm3533.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * This header provides macros for TI LM3533 device bindings.
+ */
+
+#ifndef _DT_BINDINGS_MFD_LM3533_H
+#define _DT_BINDINGS_MFD_LM3533_H
+
+/* LM3533 boost freq */
+#define LM3533_BOOST_FREQ_500KHZ 0
+#define LM3533_BOOST_FREQ_1000KHZ 1
+
+/* LM3533 boost ovp */
+#define LM3533_BOOST_OVP_16V 0
+#define LM3533_BOOST_OVP_24V 1
+#define LM3533_BOOST_OVP_32V 2
+#define LM3533_BOOST_OVP_40V 3
+
+#endif
--
2.43.0
^ permalink raw reply related
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