* [PATCH v5 0/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support
From: Neil Armstrong @ 2026-05-29 19:23 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller
Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev,
KancyJoe, Neil Armstrong, Krzysztof Kozlowski
Implement support for the Silergy SY7758 6-channel High Efficiency LED Driver
used for backlight brightness control in the Ayaneo Pocket S2 dual-DSI panel.
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v5:
- Rename vddio to vdd and make it optional in bindings
- Kept the bindings review since the change is trivial
- Link to v4: https://patch.msgid.link/20260521-topic-sm8650-ayaneo-pocket-s2-sy7758-v4-0-73c732615e4a@linaro.org
Changes in v4:
- Fixed Kconfig typo
- Remove again unused macros
- Added delay.h include
- Link to v3: https://patch.msgid.link/20260519-topic-sm8650-ayaneo-pocket-s2-sy7758-v3-0-ec8194bbc885@linaro.org
Changes in v3:
- Dropped unused macros
- Added second autho entry to match header and commit message
- Move my signof at the end
- Switched to flseep()
- Link to v2: https://patch.msgid.link/20260430-topic-sm8650-ayaneo-pocket-s2-sy7758-v2-0-308140640de9@linaro.org
Changes in v2:
- Fixed bindings subject and removed "|"
- Added review tag
- Added higher delay before reading ID from HW (100us was too short)
- Removed probe defer if i2c read fails
- Link to v1: https://patch.msgid.link/20260428-topic-sm8650-ayaneo-pocket-s2-sy7758-v1-0-0caade5fdb32@linaro.org
---
KancyJoe (1):
backlight: Add SY7758 6-channel High Efficiency LED Driver support
Neil Armstrong (1):
dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
.../bindings/leds/backlight/silergy,sy7758.yaml | 52 +++++
drivers/video/backlight/Kconfig | 8 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/sy7758.c | 259 +++++++++++++++++++++
4 files changed, 320 insertions(+)
---
base-commit: 39704f00f747aba3144289870b5fd8ac230a9aaf
change-id: 20260428-topic-sm8650-ayaneo-pocket-s2-sy7758-3081ee7f1e25
Best regards,
--
Neil Armstrong <neil.armstrong@linaro.org>
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
From: Neil Armstrong @ 2026-05-29 19:17 UTC (permalink / raw)
To: Daniel Thompson
Cc: Daniel Thompson, Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
linux-leds, devicetree, linux-kernel, linux-fbdev, KancyJoe,
Krzysztof Kozlowski
In-Reply-To: <ahmkirIuOYhd1rkM@aspen.lan>
On 5/29/26 16:36, Daniel Thompson wrote:
> On Fri, May 29, 2026 at 03:15:03PM +0100, Daniel Thompson wrote:
>> On Fri, May 29, 2026 at 02:50:43PM +0200, Neil Armstrong wrote:
>>> On 5/29/26 12:35, Daniel Thompson wrote:
>>>> On Fri, May 29, 2026 at 12:16:07PM +0200, Neil Armstrong wrote:
>>> So it's not really 2 regulators, and having regulators means the enable
>>> signal can be shared and would have regulator characteristics which it hasn't.
>>
>> Agreed. If the EN pin is merely use as an enable and voltage reference
>> then it are not two regulators.
>>
>> However, it is also *not* vddio-supply and enable-gpios. We don't need
>> the board design to check this. The pinout diagram in the datasheet
>> should be sufficient!
>>
>> If you have to activate vddio-supply for the backlight to work on the
>> board are you sure you don't just have a misnamed vdd-supply that needs
>> to be taken care of? That would make much more sense given the datasheet.
>
> After posting this I figured there is another possibility.
>
> If the host GPIO pin is not capable of delivering the 1mA requires by
> the chip then the board designer would have to add a buffer and that
> buffer would need a power supply... and that power supply could, in
> pinciple, be switchable.
>
> However if that were the case then I don't think the power supply for
> the buffer would belong in the bindings for the sy7758 so I'm afraid
> whichever way I turn it I can't make vddio-supply make sense.
Yeah seems you're probably right, I'll move it to vdd instead and make it optional.
Thanks,
Neil
>
> Daniel.
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
From: Daniel Thompson @ 2026-05-29 14:36 UTC (permalink / raw)
To: Neil Armstrong
Cc: Daniel Thompson, Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
linux-leds, devicetree, linux-kernel, linux-fbdev, KancyJoe,
Krzysztof Kozlowski
In-Reply-To: <ahmfZ0tdxbVfD_y4@aspen.lan>
On Fri, May 29, 2026 at 03:15:03PM +0100, Daniel Thompson wrote:
> On Fri, May 29, 2026 at 02:50:43PM +0200, Neil Armstrong wrote:
> > On 5/29/26 12:35, Daniel Thompson wrote:
> > > On Fri, May 29, 2026 at 12:16:07PM +0200, Neil Armstrong wrote:
> > So it's not really 2 regulators, and having regulators means the enable
> > signal can be shared and would have regulator characteristics which it hasn't.
>
> Agreed. If the EN pin is merely use as an enable and voltage reference
> then it are not two regulators.
>
> However, it is also *not* vddio-supply and enable-gpios. We don't need
> the board design to check this. The pinout diagram in the datasheet
> should be sufficient!
>
> If you have to activate vddio-supply for the backlight to work on the
> board are you sure you don't just have a misnamed vdd-supply that needs
> to be taken care of? That would make much more sense given the datasheet.
After posting this I figured there is another possibility.
If the host GPIO pin is not capable of delivering the 1mA requires by
the chip then the board designer would have to add a buffer and that
buffer would need a power supply... and that power supply could, in
pinciple, be switchable.
However if that were the case then I don't think the power supply for
the buffer would belong in the bindings for the sy7758 so I'm afraid
whichever way I turn it I can't make vddio-supply make sense.
Daniel.
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
From: Daniel Thompson @ 2026-05-29 14:15 UTC (permalink / raw)
To: Neil Armstrong
Cc: Daniel Thompson, Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
linux-leds, devicetree, linux-kernel, linux-fbdev, KancyJoe,
Krzysztof Kozlowski
In-Reply-To: <4001cf6a-b7de-4933-96bc-c9b4ccb53e4d@linaro.org>
On Fri, May 29, 2026 at 02:50:43PM +0200, Neil Armstrong wrote:
> On 5/29/26 12:35, Daniel Thompson wrote:
> > On Fri, May 29, 2026 at 12:16:07PM +0200, Neil Armstrong wrote:
> > > On 5/29/26 12:07, Daniel Thompson wrote:
> > > > On Tue, May 19, 2026 at 10:43:38AM +0200, Neil Armstrong wrote:
> > > > > Document the Silergy SY7758 6-channel High Efficiency LED Driver
> > > > > used for backlight brightness control.
> > > > >
> > > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> > > > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > > > ---
> > > > > .../bindings/leds/backlight/silergy,sy7758.yaml | 53 ++++++++++++++++++++++
> > > > > 1 file changed, 53 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..80e978d691c2
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
> > > > > @@ -0,0 +1,53 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/leds/backlight/silergy,sy7758.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Silergy SY7758 6-channel High Efficiency LED Driver
> > > > > +
> > > > > +maintainers:
> > > > > + - Neil Armstrong <neil.armstrong@linaro.org>
> > > > > +
> > > > > +description:
> > > > > + Silergy SY7758 is a high efficiency 6-channels LED backlight
> > > > > + driver with I2C brightness control.
> > > > > +
> > > > > +allOf:
> > > > > + - $ref: common.yaml#
> > > > > +
> > > > > +properties:
> > > > > + compatible:
> > > > > + const: silergy,sy7758
> > > > > +
> > > > > + reg:
> > > > > + maxItems: 1
> > > > > +
> > > > > + vddio-supply: true
> > > > > +
> > > > > + enable-gpios:
> > > > > + maxItems: 1
> > > > > +
> > > > > +required:
> > > > > + - compatible
> > > > > + - reg
> > > > > + - vddio-supply
> > > >
> > > > Sorry for missing this in v2 but is vddio-supply really a required
> > > > property?
> > > >
> > > > It's unusual for supplies to be mandatory (and the it is not mandatory
> > > > in the driver implementation).
> > >
> > > This device is a little bit special, the VDDIO regulator is used to provide
> > > power for the I/O via the enable input, so basically the enable gpio power
> > > level is provided by VDDIO.
> >
> > I don't follow. The EN pin acts as both VDDIO and as an enable but it's
> > still effectively a power rail isn't it (albeit one with very low current
> > draw).
>
> Here's the datasheet description:
> ```
> Dual-purpose pin serving both as a chip enable and as a power supply
> reference for PWM, SDA, and SCL inputs.
> ```
Thanks for the quote. It's a big help (I've been working from a site
that makes me read a page at a time so I struggled to keep track of
things).
This says that the GPIO from the host is merely serving as a voltage
reference (for the an internal LDO regulator?). That means it is *not*
a power supply!
It sounds to me like the chip is designed to work with a host where
enable GPIO and I2C interface use the same I/O voltage. By having an
active-high enables the chip can *avoid* having a separate vddio pin.
However, in a design with no separate vddio pin then it would make no
sense to model a vddio-supply in the DT.
> The VDD input is directly provided by the panel, so Linux has no control
> of it so I haven't added it.
Power supplies are often added ad-hoc when the first board that includes
a regulator enable appears. On that basis omitting vdd-supply would be
relatively harmless if you don't need it but I wonder if you do, see below!
> > It looked to me like the correct way to model to two power rails
> > going into the chip is vdd-supply (main power supply) and vddio-supply
> > (EN/VDDIO) I don't understand why a single pin needs both a regulator
> > *and* a GPIO in the DT bindings?
>
> I don't have a the schematics of the board, but as I understood one gpio is
> actually enabling an regulator which provides power to the IC (vddio) and
> a second gpio will either drive the EN signal to GND or VDDIO to provide a
> clean rising edge on the EN pin.
This doesn't make sense since it is a single pin. It cannot be both a
power supply to the chip *and* a GPIO input. It's one or the other...
and from the above is sounds so me like GPIO).
> So it's not really 2 regulators, and having regulators means the enable
> signal can be shared and would have regulator characteristics which it hasn't.
Agreed. If the EN pin is merely use as an enable and voltage reference
then it are not two regulators.
However, it is also *not* vddio-supply and enable-gpios. We don't need
the board design to check this. The pinout diagram in the datasheet
should be sufficient!
If you have to activate vddio-supply for the backlight to work on the
board are you sure you don't just have a misnamed vdd-supply that needs
to be taken care of? That would make much more sense given the datasheet.
> > > This is the recommended way from the datasheet, and I assume it will be used
> > > like that on other platforms (if it exists...)
> > >
> > > This is why it's mandatory and enabled first before setting the enable pin.
> >
> > It's not mandatory for the C implementation. devm_regulator_get_enable()
> > will provide a dummy regulator if the property is omitted.
>
> So yeah if you prefer I'll re-spin with the vddio regulator as optional
> because between both, the VDDIO is the only which could be shared with
> other devices or always-on.
Based on the above, I'd be happy with an optional vdd-supply and an
enable-gpios.
Daniel.
^ permalink raw reply
* [PATCH 4/4] drm/draw: Remove unused helper drm_draw_get_char_bitmap()
From: Thomas Zimmermann @ 2026-05-29 14:01 UTC (permalink / raw)
To: jfalempe, javierm, deller, maarten.lankhorst, mripard, airlied,
simona
Cc: dri-devel, linux-fbdev, Thomas Zimmermann
In-Reply-To: <20260529140759.529929-1-tzimmermann@suse.de>
Glyph-shape lookup has been integrated into the font-data interface
and all callers have been updated. Remove the old helper.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/drm_draw_internal.h | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_draw_internal.h b/drivers/gpu/drm/drm_draw_internal.h
index 261967145635..44ddcee4744c 100644
--- a/drivers/gpu/drm/drm_draw_internal.h
+++ b/drivers/gpu/drm/drm_draw_internal.h
@@ -7,7 +7,6 @@
#ifndef __DRM_DRAW_INTERNAL_H__
#define __DRM_DRAW_INTERNAL_H__
-#include <linux/font.h>
#include <linux/types.h>
struct iosys_map;
@@ -18,12 +17,6 @@ static inline bool drm_draw_is_pixel_fg(const u8 *sbuf8, unsigned int spitch, in
return (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) != 0;
}
-static inline const u8 *drm_draw_get_char_bitmap(const struct font_desc *font,
- char c, size_t font_pitch)
-{
- return font->data + (c * font->height) * font_pitch;
-}
-
bool drm_draw_can_convert_from_xrgb8888(u32 format);
u32 drm_draw_color_from_xrgb8888(u32 color, u32 format);
--
2.54.0
^ permalink raw reply related
* [PATCH 3/4] drm/panic: Look up glyph shape with font helper
From: Thomas Zimmermann @ 2026-05-29 14:01 UTC (permalink / raw)
To: jfalempe, javierm, deller, maarten.lankhorst, mripard, airlied,
simona
Cc: dri-devel, linux-fbdev, Thomas Zimmermann
In-Reply-To: <20260529140759.529929-1-tzimmermann@suse.de>
Look up glyph shapes with font_data_glyph_buf(). Handle non-existing
glyphs gracefully. Enable extended ASCII by casting to unsigned char.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/drm_panic.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index d6d3b8d85dea..e576c4791861 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -443,9 +443,11 @@ static void draw_txt_rectangle(struct drm_scanout_buffer *sb,
rec.x1 += (drm_rect_width(clip) - (line_len * font->width)) / 2;
for (j = 0; j < line_len; j++) {
- src = drm_draw_get_char_bitmap(font, msg[i].txt[j], font_pitch);
+ src = font_data_glyph_buf(font->data, font->width, font->height,
+ (unsigned char)msg[i].txt[j]);
rec.x2 = rec.x1 + font->width;
- drm_panic_blit(sb, &rec, src, font_pitch, 1, color);
+ if (src)
+ drm_panic_blit(sb, &rec, src, font_pitch, 1, color);
rec.x1 += font->width;
}
}
--
2.54.0
^ permalink raw reply related
* [PATCH 2/4] drm/client: log: Look up glyph shape with font helper
From: Thomas Zimmermann @ 2026-05-29 14:01 UTC (permalink / raw)
To: jfalempe, javierm, deller, maarten.lankhorst, mripard, airlied,
simona
Cc: dri-devel, linux-fbdev, Thomas Zimmermann
In-Reply-To: <20260529140759.529929-1-tzimmermann@suse.de>
Look up glyph shapes with font_data_glyph_buf(). Handle non-existing
glyphs gracefully. Enable extended ASCII by casting to unsigned char.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/clients/drm_log.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/clients/drm_log.c b/drivers/gpu/drm/clients/drm_log.c
index 8d21b785bead..e3e02c84a4cf 100644
--- a/drivers/gpu/drm/clients/drm_log.c
+++ b/drivers/gpu/drm/clients/drm_log.c
@@ -122,10 +122,12 @@ static void drm_log_draw_line(struct drm_log_scanout *scanout, const char *s,
iosys_map_incr(&map, r.y1 * fb->pitches[0]);
for (i = 0; i < len && i < scanout->columns; i++) {
u32 color = (i < prefix_len) ? scanout->prefix_color : scanout->front_color;
- src = drm_draw_get_char_bitmap(font, s[i], font_pitch);
- drm_log_blit(&map, fb->pitches[0], src, font_pitch,
- scanout->scaled_font_h, scanout->scaled_font_w,
- px_width, color);
+ src = font_data_glyph_buf(font->data, font->width, font->height,
+ (unsigned char)s[i]);
+ if (src)
+ drm_log_blit(&map, fb->pitches[0], src, font_pitch,
+ scanout->scaled_font_h, scanout->scaled_font_w,
+ px_width, color);
iosys_map_incr(&map, scanout->scaled_font_w * px_width);
}
--
2.54.0
^ permalink raw reply related
* [PATCH 0/4] drm: Safe font-data access in log/panic drawing
From: Thomas Zimmermann @ 2026-05-29 14:01 UTC (permalink / raw)
To: jfalempe, javierm, deller, maarten.lankhorst, mripard, airlied,
simona
Cc: dri-devel, linux-fbdev, Thomas Zimmermann
Looking up glyph shapes with a signed char in drm_draw_get_char_bitmap()
is unsafe. It also does not support extended ASCII codes with values
larger than 127.
Add the new function font_data_glyph_buf() to the font-data helpers. It
looks up the correct glyph from font data or returns NULL if no such
glyph exists. Convert DRM's log and panic code to the new function. Also
cast the character code to support all 256 ASCII characters.
Tested with drm_log on bochs.
Thomas Zimmermann (4):
lib/fonts: Look up glyph data with font_data_glyph_buf()
drm/client: log: Look up glyph shape with font helper
drm/panic: Look up glyph shape with font helper
drm/draw: Remove unused helper drm_draw_get_char_bitmap()
drivers/gpu/drm/clients/drm_log.c | 10 ++++++----
drivers/gpu/drm/drm_draw_internal.h | 7 -------
drivers/gpu/drm/drm_panic.c | 6 ++++--
include/linux/font.h | 3 +++
lib/fonts/fonts.c | 31 +++++++++++++++++++++++++++++
5 files changed, 44 insertions(+), 13 deletions(-)
--
2.54.0
^ permalink raw reply
* [PATCH 1/4] lib/fonts: Look up glyph data with font_data_glyph_buf()
From: Thomas Zimmermann @ 2026-05-29 14:01 UTC (permalink / raw)
To: jfalempe, javierm, deller, maarten.lankhorst, mripard, airlied,
simona
Cc: dri-devel, linux-fbdev, Thomas Zimmermann
In-Reply-To: <20260529140759.529929-1-tzimmermann@suse.de>
Add font_data_glyph_buf() to retrieve a character's glyph data or NULL
otherwise. Console fonts can currently contain 256 or 512 glyphs. The
kernel-internal characters are of type char, unsigned short or unsigned
int. Catch all of them by accepting unsigned int. Callers possibly have
to cast from signed to unsigned types to reach all glyphs in a font.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
include/linux/font.h | 3 +++
lib/fonts/fonts.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/include/linux/font.h b/include/linux/font.h
index 6845f02d739a..ea23b727388b 100644
--- a/include/linux/font.h
+++ b/include/linux/font.h
@@ -101,6 +101,9 @@ font_data_t *font_data_import(const struct console_font *font, unsigned int vpit
void font_data_get(font_data_t *fd);
bool font_data_put(font_data_t *fd);
unsigned int font_data_size(font_data_t *fd);
+const unsigned char *font_data_glyph_buf(font_data_t *fd,
+ unsigned int width, unsigned int vpitch,
+ unsigned int c);
bool font_data_is_equal(font_data_t *lhs, font_data_t *rhs);
int font_data_export(font_data_t *fd, struct console_font *font, unsigned int vpitch);
diff --git a/lib/fonts/fonts.c b/lib/fonts/fonts.c
index f5d5333450a0..4fc66722d00d 100644
--- a/lib/fonts/fonts.c
+++ b/lib/fonts/fonts.c
@@ -178,6 +178,37 @@ unsigned int font_data_size(font_data_t *fd)
}
EXPORT_SYMBOL_GPL(font_data_size);
+static unsigned int font_data_num_glyphs(font_data_t *fd, unsigned int width, unsigned int height)
+{
+ return font_data_size(fd) / font_glyph_size(width, height);
+}
+
+/**
+ * font_data_glyph_buf() - Returns the glyph for a specific character as raw bytes
+ * @fd: The font data
+ * @width: The glyph width in bits per scanline
+ * @vpitch: The number of scanlines per glyph
+ * @c: The character
+ *
+ * Glyphs start at fixed intervals within the font data. font_data_glyph_buf()
+ * returns the glyph shape of the specified character. If no such glyph
+ * exists in the font, it returns NULL.
+ *
+ * Returns:
+ * The character's raw glyph shape, or NULL if no glyph exists for the character. The
+ * provided buffer is read-only.
+ */
+const unsigned char *font_data_glyph_buf(font_data_t *fd,
+ unsigned int width, unsigned int vpitch,
+ unsigned int c)
+{
+ if (c >= font_data_num_glyphs(fd, width, vpitch))
+ return NULL;
+
+ return font_data_buf(fd) + font_glyph_size(width, vpitch) * c;
+}
+EXPORT_SYMBOL_GPL(font_data_glyph_buf);
+
/**
* font_data_is_equal - Compares font data for equality
* @lhs: Left-hand side font data
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
From: Jonathan Cameron @ 2026-05-29 13:10 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, David Lechner, Nuno Sá,
Andy Shevchenko, Helge Deller, Johan Hovold, dri-devel,
linux-leds, devicetree, linux-kernel, linux-iio, linux-fbdev
In-Reply-To: <CAPVz0n1u0z35rP8vUKjAzW_mrPm9yeMjK_-nKbyctUvQik6ECw@mail.gmail.com>
> > > > > > if (device_property_present(dev, "ti,resistor-value-ohm"))
> > > > > > ret = device_property_read_u32();
> > > > > > if (ret) //corrupt property in some fashion
> > > > > > return ret;
> > > > > > } else {
> > > > > > //set default
> > > > > > }
> > > > > > If there is no default then check it unconditionally.
> > > > >
> > > > > default value is LM3533_ALS_RESISTOR_MIN and if no property is present
> > > > > clamp will ensure that als->r_select will be set to
> > > > > LM3533_ALS_RESISTOR_MIN
> > > >
> > > > I don't see that default in the binding doc and relying in the 0 being clamped
> > > > isn't particularly readable - I'd set it explicitly.
> > > >
> > >
> > > Oh, ye, my bad. Schema enforces one of props to be present and if pwn
> > > is present then resistor is ignored. What if I move resistor reading,
> > > clamping and conversion under !als->pwm_mode check? Then resistor must
> > > be present and hence must be checked unconditionally.
> >
> > Sounds good.
> >
> > >
> > > Additionally, I can comment original lm3533_als_setup with #if 0
> > > #endif then git formatting will be much cleaner and easier to review,
> > > and once we all come to result I will just remove entire commented
> > > block and Lee can pick clean commits.
> >
> > No don't do that. If you flatten the two helpers as a precursor patch
> > then the changes in here will be easier to review anyway.
> >
>
> By "flatten the two helpers" you mean incorporate
> lm3533_als_set_input_mode and lm3533_als_set_resistor into
> lm3533_als_setup first and then convert it to use DT? I am asking,
> just to be sure.
>
yes
> > > > > > > @@ -852,25 +825,28 @@ static int lm3533_als_probe(struct platform_device *pdev)
> > > > > > > indio_dev->channels = lm3533_als_channels;
> > > > > > > indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
> > > > > > > indio_dev->name = dev_name(&pdev->dev);
> > > > > > > - iio_device_set_parent(indio_dev, pdev->dev.parent);
> > > > > >
> > > > > > I'm not sure why this was there in the first place. Hence not sure if it
> > > > > > is safe to remove.
> > > > > >
> > > > >
> > > > > This is directly related to OF conversion. The iio_device_set_parent
> > > > > bound indio_dev to parent, and it causes problems with OF now since
> > > > > als output has its own node and binding it to parent if wrong. Same
> > > > > story for backlight and leds btw.
> > > >
> > > > Is there any risk anyone was using the canonical path to get to the iio dev?
> > > > /sys/bus/platform/devices/..../iio\:deviceX
> > > > This is technically an ABI change be it a subtle one.
> > > >
> > >
> > > Linux kernel has no users of this driver, and it is in "stale" state
> > > for more then 2 years (maybe even longer). I have cc'd Johan Hovold.
> > >
> > > https://lore.kernel.org/lkml/ZmBcvtLCzllQDWVX@hovoldconsulting.com/
> > >
> > > This this 2 y. o. discussion and there were no actions ore movements.
> > > I assume this driver in its current form has no more users. This does
> > > not mean that it cannot be revived though.
> >
> > So, just to check, are you a user of this code or is this more trying to
> > help out with old code?
> >
>
> I am not insane enough to get myself into all this conversion if I did
> not need it. This is one of 2 remaining gaps in support of LG
> P880/P895 I own and support. I would really like to finally mainline
> all the patches I have locally since maintaining them becomes quite
> troublesome with time and additional patches layering on top.
Excellent! There are some odd people out there who do start on this
sort of thing despite no personal use case :)
Jonathan
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
From: Neil Armstrong @ 2026-05-29 12:50 UTC (permalink / raw)
To: Daniel Thompson
Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
linux-leds, devicetree, linux-kernel, linux-fbdev, KancyJoe,
Krzysztof Kozlowski
In-Reply-To: <ahlr5PnX5O0tEd6G@aspen.lan>
On 5/29/26 12:35, Daniel Thompson wrote:
> On Fri, May 29, 2026 at 12:16:07PM +0200, Neil Armstrong wrote:
>> On 5/29/26 12:07, Daniel Thompson wrote:
>>> On Tue, May 19, 2026 at 10:43:38AM +0200, Neil Armstrong wrote:
>>>> Document the Silergy SY7758 6-channel High Efficiency LED Driver
>>>> used for backlight brightness control.
>>>>
>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> ---
>>>> .../bindings/leds/backlight/silergy,sy7758.yaml | 53 ++++++++++++++++++++++
>>>> 1 file changed, 53 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
>>>> new file mode 100644
>>>> index 000000000000..80e978d691c2
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
>>>> @@ -0,0 +1,53 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/leds/backlight/silergy,sy7758.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Silergy SY7758 6-channel High Efficiency LED Driver
>>>> +
>>>> +maintainers:
>>>> + - Neil Armstrong <neil.armstrong@linaro.org>
>>>> +
>>>> +description:
>>>> + Silergy SY7758 is a high efficiency 6-channels LED backlight
>>>> + driver with I2C brightness control.
>>>> +
>>>> +allOf:
>>>> + - $ref: common.yaml#
>>>> +
>>>> +properties:
>>>> + compatible:
>>>> + const: silergy,sy7758
>>>> +
>>>> + reg:
>>>> + maxItems: 1
>>>> +
>>>> + vddio-supply: true
>>>> +
>>>> + enable-gpios:
>>>> + maxItems: 1
>>>> +
>>>> +required:
>>>> + - compatible
>>>> + - reg
>>>> + - vddio-supply
>>>
>>> Sorry for missing this in v2 but is vddio-supply really a required
>>> property?
>>>
>>> It's unusual for supplies to be mandatory (and the it is not mandatory
>>> in the driver implementation).
>>
>> This device is a little bit special, the VDDIO regulator is used to provide
>> power for the I/O via the enable input, so basically the enable gpio power
>> level is provided by VDDIO.
>
> I don't follow. The EN pin acts as both VDDIO and as an enable but it's
> still effectively a power rail isn't it (albeit one with very low current
> draw).
Here's the datasheet description:
```
Dual-purpose pin serving both as a chip enable and as a power supply
reference for PWM, SDA, and SCL inputs.
```
The VDD input is directly provided by the panel, so Linux has no control
of it so I haven't added it.
>
> It looked to me like the correct way to model to two power rails
> going into the chip is vdd-supply (main power supply) and vddio-supply
> (EN/VDDIO) I don't understand why a single pin needs both a regulator
> *and* a GPIO in the DT bindings?
I don't have a the schematics of the board, but as I understood one gpio is
actually enabling an regulator which provides power to the IC (vddio) and
a second gpio will either drive the EN signal to GND or VDDIO to provide a
clean rising edge on the EN pin.
So it's not really 2 regulators, and having regulators means the enable
signal can be shared and would have regulator characteristics which it hasn't.
>
>> This is the recommended way from the datasheet, and I assume it will be used
>> like that on other platforms (if it exists...)
>>
>> This is why it's mandatory and enabled first before setting the enable pin.
>
> It's not mandatory for the C implementation. devm_regulator_get_enable()
> will provide a dummy regulator if the property is omitted.
So yeah if you prefer I'll re-spin with the vddio regulator as optional
because between both, the VDDIO is the only which could be shared with
other devices or always-on.
Neil
>
>
> Daniel.
^ permalink raw reply
* Re: [PATCH v2 5/6] video: backlight: lm3533_bl: Set initial mapping mode from DT
From: Daniel Thompson @ 2026-05-29 11:40 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <CAPVz0n0kpYBACOo=YyNk31KGwBEoyrf+dii8V6QY4iRCGd2PNQ@mail.gmail.com>
On Fri, May 29, 2026 at 02:17:00PM +0300, Svyatoslav Ryhel wrote:
> пт, 29 трав. 2026 р. о 14:10 Daniel Thompson <daniel@riscstar.com> пише:
> >
> > On Thu, May 28, 2026 at 04:51:22PM +0300, Svyatoslav Ryhel wrote:
> > > Add support to obtain the initial mapping mode from DT instead of leaving
> > > it unconfigured. Additionally, update the linear sysfs code, which uses a
> > > similar coding pattern.
> >
> > Words like "additionally" in a patch description can be a sign the patch
> > should actually be two patches. In this case the patch would be a lot
> > easier to read if you cleaned up the linear sysfs code (patch N) and then
> > added the new DT logic (patch N+1).
> >
>
> I looked into this in reverse. My goal was to add DT logic I don't
> case how sysfs works. My code matched with what sysfs does I just
> included sysfs change as well. I might better drop sysfs changes
> entirely since with such pace this patchset will inflate from 6 to 15
> and beyond.
Not sure about that. The clean up is good and introducing the new
BIT(2 * n + 1) macro does need to be used pervasively or there's no
point in adding it.
Daniel.
^ permalink raw reply
* Re: [PATCH v2] staging: sm750fb: rename pv_reg to io_base
From: neha arora @ 2026-05-29 11:28 UTC (permalink / raw)
To: Dan Carpenter
Cc: sudipm.mukherjee, teddy.wang, gregkh, linux-fbdev, linux-staging,
linux-kernel
In-Reply-To: <CAOWJOpvJnSeS=1sO2mujC9tNi9_cQGgJ6S0j6xvkAp8KRnudjg@mail.gmail.com>
I have submitted patch v3 with note and Signed-off-by line
Regards,
Onish
On Fri, May 29, 2026 at 4:50 PM neha arora <neharora23587@gmail.com> wrote:
>
> I have submitted patch v3 with note and Signed-off-by line.
>
> Regards,
> Onish
>
> On Fri, May 29, 2026 at 4:11 PM Dan Carpenter <error27@gmail.com> wrote:
>>
>> On Fri, May 29, 2026 at 03:22:33PM +0530, Onish Sharma wrote:
>> > Rename pv_reg to io_base to follow kernel naming style and improve
>> > readability.
>> >
>> > No functional changes intended.
>>
>> Run your patches through checkpatch. Also v2 patches need to be sent
>> in a specific format. This should be [PATCH v2 1/2]
>>
>> https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/
>>
>> > ---
>> ^^^
>> There needs to be a note here to say what changed.
>>
>> regards,
>> dan carpenter
>>
^ permalink raw reply
* Re: [PATCH v2] staging: sm750fb: remove unused variable
From: neha arora @ 2026-05-29 11:25 UTC (permalink / raw)
To: Dan Carpenter
Cc: sudipm.mukherjee, teddy.wang, gregkh, linux-fbdev, linux-staging,
linux-kernel
In-Reply-To: <ahlszyY6Nd9ANz-X@stanley.mountain>
Hi Dan,
After looking into the structural dependencies and the cross-casting
between init_status and initchip_param, I've decided that this
refactoring is outside the scope of what I want to work on at this
time.
Please feel free to drop my previous patch. I'm going to shift my
focus to other areas.
Regards,
Onish
On Fri, May 29, 2026 at 4:09 PM Dan Carpenter <error27@gmail.com> wrote:
>
> On Fri, May 29, 2026 at 03:42:42PM +0530, Onish Sharma wrote:
> > Remove the set_all_eng_off flag and its associated cleanup logic.
> > The variable is redundant as the hardware should be initialized to a
> > known state regardless of prior usage.
> >
> > Suggested-by: Dan Carpenter <error27@gmail.com>
> > Signed-off-by: Onish Sharma <neharora23587@gmail.com>
> > ---
>
> Sorry, miscommunication. This breaks the driver. This is also a bit
> more involved than I thought...
>
> There are two structs:
>
> struct init_status {
> ushort power_mode;
> /* below three clocks are in unit of MHZ*/
> ushort chip_clk;
> ushort mem_clk;
> ushort master_clk;
> ushort setAllEngOff;
> ushort reset_memory;
> };
>
> And struct initchip_param. The initchip_param is exactly the same but
> with all the struct members renamed and comments added. They have to
> match because we cast back and forth.
>
> Why do we have two different struct that have to be the same? You might
> think it is for API, but as near as I can see that is not the case.
> Maybe it was at some point? We should get rid of one struct. Which
> everyone is API is the one we should keep. If neither is API then get
> rid of init_status and keep initchip_param.
>
> After that we can talk about getting rid of setAllEngOff/set_all_eng_off.
>
> regards,
> dan carpenter
>
^ permalink raw reply
* [PATCH v3] staging: sm750fb: rename pv_reg to io_base
From: Onish Sharma @ 2026-05-29 11:19 UTC (permalink / raw)
To: sudipm.mukherjee, teddy.wang, gregkh
Cc: linux-fbdev, linux-staging, linux-kernel, Onish Sharma
Rename pv_reg to io_base to follow kernel naming style and improve
readability.
No functional changes intended.
Signed-off-by: Onish Sharma <neharora23587@gmail.com>
---
Changes in v3:
- Added mandatory Signed-off-by line.
- Rename pv_reg to io_base to remove hungarian notation
drivers/staging/sm750fb/sm750.c | 4 ++--
drivers/staging/sm750fb/sm750.h | 2 +-
drivers/staging/sm750fb/sm750_hw.c | 12 ++++++------
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 716a8935f58d..c2d2864f135b 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -743,7 +743,7 @@ static int lynxfb_set_fbinfo(struct fb_info *info, int index)
* must be set after crtc member initialized
*/
crtc->cursor.offset = crtc->o_screen + crtc->vidmem_size - 1024;
- crtc->cursor.mmio = sm750_dev->pv_reg +
+ crtc->cursor.mmio = sm750_dev->io_base +
0x800f0 + (int)crtc->channel * 0x140;
crtc->cursor.max_h = 64;
@@ -1047,7 +1047,7 @@ static void lynxfb_pci_remove(struct pci_dev *pdev)
sm750fb_framebuffer_release(sm750_dev);
arch_phys_wc_del(sm750_dev->mtrr.vram);
- iounmap(sm750_dev->pv_reg);
+ iounmap(sm750_dev->io_base);
iounmap(sm750_dev->vmem);
pci_release_region(pdev, 1);
kfree(g_settings);
diff --git a/drivers/staging/sm750fb/sm750.h b/drivers/staging/sm750fb/sm750.h
index e8885133da2e..c42800313c6a 100644
--- a/drivers/staging/sm750fb/sm750.h
+++ b/drivers/staging/sm750fb/sm750.h
@@ -97,7 +97,7 @@ struct sm750_dev {
unsigned long vidreg_start;
__u32 vidmem_size;
__u32 vidreg_size;
- void __iomem *pv_reg;
+ void __iomem *io_base;
unsigned char __iomem *vmem;
/* locks*/
spinlock_t slock;
diff --git a/drivers/staging/sm750fb/sm750_hw.c b/drivers/staging/sm750fb/sm750_hw.c
index 95f797e5776a..dc1118808b4f 100644
--- a/drivers/staging/sm750fb/sm750_hw.c
+++ b/drivers/staging/sm750fb/sm750_hw.c
@@ -23,18 +23,18 @@ int hw_sm750_map(struct sm750_dev *sm750_dev, struct pci_dev *pdev)
}
/* now map mmio and vidmem */
- sm750_dev->pv_reg =
+ sm750_dev->io_base =
ioremap(sm750_dev->vidreg_start, sm750_dev->vidreg_size);
- if (!sm750_dev->pv_reg) {
+ if (!sm750_dev->io_base) {
dev_err(&pdev->dev, "mmio failed\n");
ret = -EFAULT;
goto err_release_region;
}
- sm750_dev->accel.dpr_base = sm750_dev->pv_reg + DE_BASE_ADDR_TYPE1;
- sm750_dev->accel.dp_port_base = sm750_dev->pv_reg + DE_PORT_ADDR_TYPE1;
+ sm750_dev->accel.dpr_base = sm750_dev->io_base + DE_BASE_ADDR_TYPE1;
+ sm750_dev->accel.dp_port_base = sm750_dev->io_base + DE_PORT_ADDR_TYPE1;
- mmio750 = sm750_dev->pv_reg;
+ mmio750 = sm750_dev->io_base;
sm750_set_chip_type(sm750_dev->devid, sm750_dev->revid);
sm750_dev->vidmem_start = pci_resource_start(pdev, 0);
@@ -58,7 +58,7 @@ int hw_sm750_map(struct sm750_dev *sm750_dev, struct pci_dev *pdev)
return 0;
err_unmap_reg:
- iounmap(sm750_dev->pv_reg);
+ iounmap(sm750_dev->io_base);
err_release_region:
pci_release_region(pdev, 1);
return ret;
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2 5/6] video: backlight: lm3533_bl: Set initial mapping mode from DT
From: Svyatoslav Ryhel @ 2026-05-29 11:17 UTC (permalink / raw)
To: Daniel Thompson
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <ahl0La8OQHXAlV3m@aspen.lan>
пт, 29 трав. 2026 р. о 14:10 Daniel Thompson <daniel@riscstar.com> пише:
>
> On Thu, May 28, 2026 at 04:51:22PM +0300, Svyatoslav Ryhel wrote:
> > Add support to obtain the initial mapping mode from DT instead of leaving
> > it unconfigured. Additionally, update the linear sysfs code, which uses a
> > similar coding pattern.
>
> Words like "additionally" in a patch description can be a sign the patch
> should actually be two patches. In this case the patch would be a lot
> easier to read if you cleaned up the linear sysfs code (patch N) and then
> added the new DT logic (patch N+1).
>
I looked into this in reverse. My goal was to add DT logic I don't
case how sysfs works. My code matched with what sysfs does I just
included sysfs change as well. I might better drop sysfs changes
entirely since with such pace this patchset will inflate from 6 to 15
and beyond.
>
> Daniel.
^ permalink raw reply
* Re: [PATCH v2 5/6] video: backlight: lm3533_bl: Set initial mapping mode from DT
From: Daniel Thompson @ 2026-05-29 11:10 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <20260528135123.103745-6-clamor95@gmail.com>
On Thu, May 28, 2026 at 04:51:22PM +0300, Svyatoslav Ryhel wrote:
> Add support to obtain the initial mapping mode from DT instead of leaving
> it unconfigured. Additionally, update the linear sysfs code, which uses a
> similar coding pattern.
Words like "additionally" in a patch description can be a sign the patch
should actually be two patches. In this case the patch would be a lot
easier to read if you cleaned up the linear sysfs code (patch N) and then
added the new DT logic (patch N+1).
Daniel.
^ permalink raw reply
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
From: Svyatoslav Ryhel @ 2026-05-29 11:06 UTC (permalink / raw)
To: Daniel Thompson
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <ahlxvGRVFDFrTUN3@aspen.lan>
пт, 29 трав. 2026 р. о 14:00 Daniel Thompson <daniel@riscstar.com> пише:
>
> On Thu, May 28, 2026 at 04:51:19PM +0300, Svyatoslav Ryhel wrote:
> > Since there are no users of this driver via platform data, remove the
> > platform data support and switch to using Device Tree bindings.
> > Additionally, optimize functions used only by platform data.
>
> The last sentence is a little vague and makes us have to hunt for the
> changes in a relatively large patch. If it is referring to the change to
> common up the init and update code then it's would better to say that
> explicitly!
>
If I understood Jonathan properly, the last sentence will get its own patch.
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> > drivers/iio/light/lm3533-als.c | 95 ++++------
> > drivers/leds/leds-lm3533.c | 51 ++++--
> > drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> > drivers/video/backlight/lm3533_bl.c | 52 ++++--
> > include/linux/mfd/lm3533.h | 51 +-----
>
> Just one comment for backlight, below:
>
> > diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> > index babfd3ceec86..42da652df58d 100644
> > --- a/drivers/video/backlight/lm3533_bl.c
> > +++ b/drivers/video/backlight/lm3533_bl.c
> > @@ -295,13 +293,20 @@ static int lm3533_bl_probe(struct platform_device *pdev)
> > bl->cb.id = lm3533_bl_get_ctrlbank_id(bl);
> > bl->cb.dev = NULL; /* until registered */
> >
> > + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
> > + pdev->name, pdev->id);
> > + if (!name)
> > + return -ENOMEM;
> > +
> > memset(&props, 0, sizeof(props));
> > props.type = BACKLIGHT_RAW;
> > props.max_brightness = LM3533_BL_MAX_BRIGHTNESS;
> > - props.brightness = pdata->default_brightness;
>
> Given the big changes to the driver is there any chance of putting a
> good value in props.scale (BACKLIGHT_SCALE_LINEAR or
> BACKLIGHT_SCALE_NON_LINEAR)?
>
> If the difference between 50% and 100% *looks* like 50% then the scale is
> non-linear (since humn perception of brightness is not linear).
>
Yes! But not in this patch. This patchset has a dedicated patch
implementing linear and non-linear configuration from tree which may
include this configuration as well. No guarantees though, but I will
keep in mind this request. Thanks!
>
> Daniel.
^ permalink raw reply
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
From: Svyatoslav Ryhel @ 2026-05-29 11:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, David Lechner, Nuno Sá,
Andy Shevchenko, Helge Deller, Johan Hovold, dri-devel,
linux-leds, devicetree, linux-kernel, linux-iio, linux-fbdev
In-Reply-To: <20260529114828.5a87c732@jic23-huawei>
пт, 29 трав. 2026 р. о 13:48 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Fri, 29 May 2026 12:39:56 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > пт, 29 трав. 2026 р. о 12:08 Jonathan Cameron <jic23@kernel.org> пише:
> > >
> > > On Thu, 28 May 2026 18:03:31 +0300
> > > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > >
> > > > чт, 28 трав. 2026 р. о 17:50 Jonathan Cameron <jic23@kernel.org> пише:
> > > > >
> > > > > On Thu, 28 May 2026 16:51:19 +0300
> > > > > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > > > >
> > > > > > Since there are no users of this driver via platform data, remove the
> > > > > > platform data support and switch to using Device Tree bindings.
> > > > > > Additionally, optimize functions used only by platform data.
> > > > >
> > > > >
> > > > > At least the IIO ones would have made much the same amount of sense for
> > > > > dt, just that they weren't having in the first place. I'd prefer that
> > >
> > > Gah. I write gibberish after too much reviewing. having/helping!
> > >
> > > > > as a precursor patch to make the rest much more readable.
> > > > >
> > > >
> > > > I can add you preferences into this commit, I don't mind.
> > > >
> > > > > >
> > > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > >
> > > > > I only looked in detail at the iio bit. A few changes requested.
> > > > >
> > > > > > ---
> > > > > > drivers/iio/light/lm3533-als.c | 95 ++++------
> > > > > > drivers/leds/leds-lm3533.c | 51 ++++--
> > > > > > drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> > > > > > drivers/video/backlight/lm3533_bl.c | 52 ++++--
> > > > > > include/linux/mfd/lm3533.h | 51 +-----
> > > > > > 5 files changed, 212 insertions(+), 305 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > > > > > index 99f0b903018c..cbd337b73bd9 100644
> > > > > > --- a/drivers/iio/light/lm3533-als.c
> > > > > > +++ b/drivers/iio/light/lm3533-als.c
> > > > >
> > > > > > @@ -714,59 +720,33 @@ static const struct attribute_group lm3533_als_attribute_group = {
> > > > > > .attrs = lm3533_als_attributes
> > > > > > };
> > > > > >
> > > > > > -static int lm3533_als_set_input_mode(struct lm3533_als *als, bool pwm_mode)
> > > > > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > > > > {
> > > > > > - u8 mask = LM3533_ALS_INPUT_MODE_MASK;
> > > > > > - u8 val;
> > > > > > + struct device *dev = &als->pdev.dev;
> > > > > > int ret;
> > > > > >
> > > > > > - if (pwm_mode)
> > > > > > - val = mask; /* pwm input */
> > > > > > - else
> > > > > > - val = 0; /* analog input */
> > > > > > -
> > > > > > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, val, mask);
> > > > > > - if (ret) {
> > > > > > - dev_err(&als->pdev->dev, "failed to set input mode %d\n",
> > > > > > - pwm_mode);
> > > > > > - return ret;
> > > > > > - }
> > > > > > -
> > > > > > - return 0;
> > > > > > -}
> > > > > > -
> > > > > > -static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
> > > > > > -{
> > > > > > - int ret;
> > > > > > -
> > > > > > - if (val < LM3533_ALS_RESISTOR_MIN || val > LM3533_ALS_RESISTOR_MAX) {
> > > > > > - dev_err(&als->pdev->dev, "invalid resistor value\n");
> > > > > > - return -EINVAL;
> > > > > > - }
> > > > > > -
> > > > > > - ret = lm3533_write(als->lm3533, LM3533_REG_ALS_RESISTOR_SELECT, val);
> > > > > > - if (ret) {
> > > > > > - dev_err(&als->pdev->dev, "failed to set resistor\n");
> > > > > > - return ret;
> > > > > > - }
> > > > > > + device_property_read_u32(dev, "ti,resistor-value-ohm",
> > > > > > + &als->r_select);
> > > > > Does this have a default? If so the pattern we've recently be setting on for IIO
> > > > > is
> > > > > if (device_property_present(dev, "ti,resistor-value-ohm"))
> > > > > ret = device_property_read_u32();
> > > > > if (ret) //corrupt property in some fashion
> > > > > return ret;
> > > > > } else {
> > > > > //set default
> > > > > }
> > > > > If there is no default then check it unconditionally.
> > > >
> > > > default value is LM3533_ALS_RESISTOR_MIN and if no property is present
> > > > clamp will ensure that als->r_select will be set to
> > > > LM3533_ALS_RESISTOR_MIN
> > >
> > > I don't see that default in the binding doc and relying in the 0 being clamped
> > > isn't particularly readable - I'd set it explicitly.
> > >
> >
> > Oh, ye, my bad. Schema enforces one of props to be present and if pwn
> > is present then resistor is ignored. What if I move resistor reading,
> > clamping and conversion under !als->pwm_mode check? Then resistor must
> > be present and hence must be checked unconditionally.
>
> Sounds good.
>
> >
> > Additionally, I can comment original lm3533_als_setup with #if 0
> > #endif then git formatting will be much cleaner and easier to review,
> > and once we all come to result I will just remove entire commented
> > block and Lee can pick clean commits.
>
> No don't do that. If you flatten the two helpers as a precursor patch
> then the changes in here will be easier to review anyway.
>
By "flatten the two helpers" you mean incorporate
lm3533_als_set_input_mode and lm3533_als_set_resistor into
lm3533_als_setup first and then convert it to use DT? I am asking,
just to be sure.
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > - return 0;
> > > > > > -}
> > > > > > + als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> > > > > > + LM3533_ALS_RESISTOR_MAX);
> > > > > > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
> > > > > >
> > > > > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > > > > - const struct lm3533_als_platform_data *pdata)
> > > > > > -{
> > > > > > - int ret;
> > > > > > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > > > > >
> > > > > > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > > > > > + ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, als->pwm_mode ?
> > > > > > + LM3533_ALS_INPUT_MODE_MASK : 0,
> > > > >
> > > > > That's ugly. Better as
> > > > >
> > > > > ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF,
> > > > > als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0,
> > > > >
> > > >
> > > > Yes sure, just followed 80 char limit.
> > > >
> > > > > Though if there wasn't a layer hiding the regmap, it could just have been
> > > > >
> > > > > ret = regmap_assign_bits(lm3533->regmap, LM3533_REG_ALS_CONF,
> > > > > LM3533_ALS_INPUT_MODE_MASK, als->pwm_mode);;
> > > > >
> > > > > which would have been nicer.
> > > > >
> > > > > I'm not particularly keen on the swashing of the helpers being in a patch
> > >
> > > smashing. (this definitely wasn't my best effort at English!)
> > >
> > > > > that is about switching the binding type as feels largely unrelated.
> > > > > Should really have been a precursor, easier to review patch.
> > > > >
> > > >
> > > > Removing of lm3533_update layer is not the scope of this patchset.
> > >
> > > Understood. I'm fine with just the refactor you are doing brought out as a precursor
> > > patch.
> > >
> >
> > I have looked into removing wrappers too. That seems to be less a
> > hassle that I anticipated, so I will include regmap switch in the v2.
>
> Ah ok. Even better.
>
> >
> > > >
> > > > >
> > > > > > + LM3533_ALS_INPUT_MODE_MASK);
> > > > > > if (ret)
> > > > > > - return ret;
> > > > > > + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > > > > > + als->pwm_mode);
> > > > > >
> > > > > > /* ALS input is always high impedance in PWM-mode. */
> > > > > > - if (!pdata->pwm_mode) {
> > > > > > - ret = lm3533_als_set_resistor(als, pdata->r_select);
> > > > > > + if (!als->pwm_mode) {
> > > > > > + ret = lm3533_write(lm3533, LM3533_REG_ALS_RESISTOR_SELECT,
> > > > > > + (u8)als->r_select);
> > > > >
> > > > > Same applies here. Mostly an unrelated change as the only thing switching that
> > > > > is related to the patch is one parameter.
> > > > >
> > > >
> > > > Removing of lm3533_write layer is not the scope of this patchset.
> > > >
> > > > > > if (ret)
> > > > > > - return ret;
> > > > > > + return dev_err_probe(dev, ret, "failed to set resistor\n");
> > > > > > }
> > > > > >
> > > > > > return 0;
> > > > >
> > > > > > @@ -852,25 +825,28 @@ static int lm3533_als_probe(struct platform_device *pdev)
> > > > > > indio_dev->channels = lm3533_als_channels;
> > > > > > indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
> > > > > > indio_dev->name = dev_name(&pdev->dev);
> > > > > > - iio_device_set_parent(indio_dev, pdev->dev.parent);
> > > > >
> > > > > I'm not sure why this was there in the first place. Hence not sure if it
> > > > > is safe to remove.
> > > > >
> > > >
> > > > This is directly related to OF conversion. The iio_device_set_parent
> > > > bound indio_dev to parent, and it causes problems with OF now since
> > > > als output has its own node and binding it to parent if wrong. Same
> > > > story for backlight and leds btw.
> > >
> > > Is there any risk anyone was using the canonical path to get to the iio dev?
> > > /sys/bus/platform/devices/..../iio\:deviceX
> > > This is technically an ABI change be it a subtle one.
> > >
> >
> > Linux kernel has no users of this driver, and it is in "stale" state
> > for more then 2 years (maybe even longer). I have cc'd Johan Hovold.
> >
> > https://lore.kernel.org/lkml/ZmBcvtLCzllQDWVX@hovoldconsulting.com/
> >
> > This this 2 y. o. discussion and there were no actions ore movements.
> > I assume this driver in its current form has no more users. This does
> > not mean that it cannot be revived though.
>
> So, just to check, are you a user of this code or is this more trying to
> help out with old code?
>
I am not insane enough to get myself into all this conversion if I did
not need it. This is one of 2 remaining gaps in support of LG
P880/P895 I own and support. I would really like to finally mainline
all the patches I have locally since maintaining them becomes quite
troublesome with time and additional patches layering on top.
> Jonathan
>
> >
> > >
> > > >
> > > > >
> > > > > > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> > > > > > index 45795f2a1042..d707d43d5526 100644
> > > > > > --- a/drivers/leds/leds-lm3533.c
> > > > > > +++ b/drivers/leds/leds-lm3533.c
> > > > >
> > > > > >
> > > > > > led->cb.dev = led->cdev.dev;
> > > > > >
> > > > > > - ret = lm3533_led_setup(led, pdata);
> > > > > > + device_property_read_u32(&pdev->dev, "led-max-microamp",
> > > > > > + &led->max_current);
> > > > >
> > > > > I'd prefer explicit setting of the default to be visible before this, or
> > > > > the property_present pattern I mention in the IIO review above.
> > > > >
> > > >
> > > > clamp will ensure that led->max_current will be set to
> > > > LM3533_LED_MAX_CURRENT_MIN regardless if it it present
> > >
> > > As above, I'd prefer it set explicitly.
> > >
> >
> > I understand your position and I am not denying it for ALS part, but
> > LEDs don't belong to IIO subsystem and different subsystem maintainers
> > may have drastically different preferences and requirements (ugh, PTSD
> > in its full glory).
> >
> > > >
> > > > > > + led->max_current = clamp(led->max_current, LM3533_LED_MAX_CURRENT_MIN,
> > > > > > + LM3533_LED_MAX_CURRENT_MAX);
> > > > >
> > > > > I didn't look any further (busy day!)
> > > >
> > >
>
^ permalink raw reply
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
From: Daniel Thompson @ 2026-05-29 11:00 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <20260528135123.103745-3-clamor95@gmail.com>
On Thu, May 28, 2026 at 04:51:19PM +0300, Svyatoslav Ryhel wrote:
> Since there are no users of this driver via platform data, remove the
> platform data support and switch to using Device Tree bindings.
> Additionally, optimize functions used only by platform data.
The last sentence is a little vague and makes us have to hunt for the
changes in a relatively large patch. If it is referring to the change to
common up the init and update code then it's would better to say that
explicitly!
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/iio/light/lm3533-als.c | 95 ++++------
> drivers/leds/leds-lm3533.c | 51 ++++--
> drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> drivers/video/backlight/lm3533_bl.c | 52 ++++--
> include/linux/mfd/lm3533.h | 51 +-----
Just one comment for backlight, below:
> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> index babfd3ceec86..42da652df58d 100644
> --- a/drivers/video/backlight/lm3533_bl.c
> +++ b/drivers/video/backlight/lm3533_bl.c
> @@ -295,13 +293,20 @@ static int lm3533_bl_probe(struct platform_device *pdev)
> bl->cb.id = lm3533_bl_get_ctrlbank_id(bl);
> bl->cb.dev = NULL; /* until registered */
>
> + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
> + pdev->name, pdev->id);
> + if (!name)
> + return -ENOMEM;
> +
> memset(&props, 0, sizeof(props));
> props.type = BACKLIGHT_RAW;
> props.max_brightness = LM3533_BL_MAX_BRIGHTNESS;
> - props.brightness = pdata->default_brightness;
Given the big changes to the driver is there any chance of putting a
good value in props.scale (BACKLIGHT_SCALE_LINEAR or
BACKLIGHT_SCALE_NON_LINEAR)?
If the difference between 50% and 100% *looks* like 50% then the scale is
non-linear (since humn perception of brightness is not linear).
Daniel.
^ permalink raw reply
* Re: [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller
From: Svyatoslav Ryhel @ 2026-05-29 10:56 UTC (permalink / raw)
To: Daniel Thompson
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <ahludIZPMUlPDTG_@aspen.lan>
пт, 29 трав. 2026 р. о 13:46 Daniel Thompson <daniel@riscstar.com> пише:
>
> On Fri, May 29, 2026 at 01:07:50PM +0300, Svyatoslav Ryhel wrote:
> > пт, 29 трав. 2026 р. о 12:51 Daniel Thompson <daniel@riscstar.com> пише:
> > >
> > > On Thu, May 28, 2026 at 04:51:18PM +0300, Svyatoslav Ryhel wrote:
> > > > Document 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>
> > > > ---
> > > > .../leds/backlight/ti,lm3533-backlight.yaml | 68 +++++++
> > > > .../bindings/leds/ti,lm3533-leds.yaml | 66 +++++++
> > > > .../devicetree/bindings/leds/ti,lm3533.yaml | 170 ++++++++++++++++++
> > > > 3 files changed, 304 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
> > > > create mode 100644 Documentation/devicetree/bindings/leds/ti,lm3533-leds.yaml
> > > > create mode 100644 Documentation/devicetree/bindings/leds/ti,lm3533.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
> > > > new file mode 100644
> > > > index 000000000000..866b0fb8ed04
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
> > > > @@ -0,0 +1,68 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/leds/backlight/ti,lm3533-backlight.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: TI LM3533 high voltage series LED strings
> > > > +
> > > > +description:
> > > > + This is part of the TI LM3533 MFD device. It represents two high voltage series
> > > > + LED strings for display backlight controlled by the TI LM3533.
> > > > +
> > > > +maintainers:
> > > > + - Svyatoslav Ryhel <clamor95@gmail.com>
> > > > +
> > > > +allOf:
> > > > + - $ref: /schemas/leds/backlight/common.yaml#
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > + const: ti,lm3533-backlight
> > > > +
> > > > + reg:
> > > > + description: Control bank selection (0 = bank A, 1 = bank B).
> > > > + maximum: 1
> > > > <snip>
> > > > + ti,pwm-config-mask:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + description: |
> > > > + Control Bank PWM Configuration Register mask that allows to configure
> > > > + PWM input in Zones 0-4
> > > > + BIT(0) - PWM Input is enabled
> > > > + BIT(1) - PWM Input is enabled in Zone 0
> > > > + BIT(2) - PWM Input is enabled in Zone 1
> > > > + BIT(3) - PWM Input is enabled in Zone 2
> > > > + BIT(4) - PWM Input is enabled in Zone 3
> > > > + BIT(5) - PWM Input is enabled in Zone 4
> > >
> > > This is optional and the drive implements a default (zero) that is not
> > > documented here.
> > >
> > > Is zero a sane default from a DT binding point of view?
> > >
> >
> > Yes, if property is missing then PWM input is disabled which is
> > equivalent to setting all bits to 0.
>
> So the default should be documented in the bindings?
>
Ye, sure, I can do that.
>
> Daniel.
^ permalink raw reply
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
From: Jonathan Cameron @ 2026-05-29 10:48 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, David Lechner, Nuno Sá,
Andy Shevchenko, Helge Deller, Johan Hovold, dri-devel,
linux-leds, devicetree, linux-kernel, linux-iio, linux-fbdev
In-Reply-To: <CAPVz0n0VHdUo5oHdALgcerLsykdz-2n7c+jxYHrMOV7Ra5x_qQ@mail.gmail.com>
On Fri, 29 May 2026 12:39:56 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> пт, 29 трав. 2026 р. о 12:08 Jonathan Cameron <jic23@kernel.org> пише:
> >
> > On Thu, 28 May 2026 18:03:31 +0300
> > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >
> > > чт, 28 трав. 2026 р. о 17:50 Jonathan Cameron <jic23@kernel.org> пише:
> > > >
> > > > On Thu, 28 May 2026 16:51:19 +0300
> > > > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > > >
> > > > > Since there are no users of this driver via platform data, remove the
> > > > > platform data support and switch to using Device Tree bindings.
> > > > > Additionally, optimize functions used only by platform data.
> > > >
> > > >
> > > > At least the IIO ones would have made much the same amount of sense for
> > > > dt, just that they weren't having in the first place. I'd prefer that
> >
> > Gah. I write gibberish after too much reviewing. having/helping!
> >
> > > > as a precursor patch to make the rest much more readable.
> > > >
> > >
> > > I can add you preferences into this commit, I don't mind.
> > >
> > > > >
> > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > >
> > > > I only looked in detail at the iio bit. A few changes requested.
> > > >
> > > > > ---
> > > > > drivers/iio/light/lm3533-als.c | 95 ++++------
> > > > > drivers/leds/leds-lm3533.c | 51 ++++--
> > > > > drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> > > > > drivers/video/backlight/lm3533_bl.c | 52 ++++--
> > > > > include/linux/mfd/lm3533.h | 51 +-----
> > > > > 5 files changed, 212 insertions(+), 305 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > > > > index 99f0b903018c..cbd337b73bd9 100644
> > > > > --- a/drivers/iio/light/lm3533-als.c
> > > > > +++ b/drivers/iio/light/lm3533-als.c
> > > >
> > > > > @@ -714,59 +720,33 @@ static const struct attribute_group lm3533_als_attribute_group = {
> > > > > .attrs = lm3533_als_attributes
> > > > > };
> > > > >
> > > > > -static int lm3533_als_set_input_mode(struct lm3533_als *als, bool pwm_mode)
> > > > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > > > {
> > > > > - u8 mask = LM3533_ALS_INPUT_MODE_MASK;
> > > > > - u8 val;
> > > > > + struct device *dev = &als->pdev.dev;
> > > > > int ret;
> > > > >
> > > > > - if (pwm_mode)
> > > > > - val = mask; /* pwm input */
> > > > > - else
> > > > > - val = 0; /* analog input */
> > > > > -
> > > > > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, val, mask);
> > > > > - if (ret) {
> > > > > - dev_err(&als->pdev->dev, "failed to set input mode %d\n",
> > > > > - pwm_mode);
> > > > > - return ret;
> > > > > - }
> > > > > -
> > > > > - return 0;
> > > > > -}
> > > > > -
> > > > > -static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
> > > > > -{
> > > > > - int ret;
> > > > > -
> > > > > - if (val < LM3533_ALS_RESISTOR_MIN || val > LM3533_ALS_RESISTOR_MAX) {
> > > > > - dev_err(&als->pdev->dev, "invalid resistor value\n");
> > > > > - return -EINVAL;
> > > > > - }
> > > > > -
> > > > > - ret = lm3533_write(als->lm3533, LM3533_REG_ALS_RESISTOR_SELECT, val);
> > > > > - if (ret) {
> > > > > - dev_err(&als->pdev->dev, "failed to set resistor\n");
> > > > > - return ret;
> > > > > - }
> > > > > + device_property_read_u32(dev, "ti,resistor-value-ohm",
> > > > > + &als->r_select);
> > > > Does this have a default? If so the pattern we've recently be setting on for IIO
> > > > is
> > > > if (device_property_present(dev, "ti,resistor-value-ohm"))
> > > > ret = device_property_read_u32();
> > > > if (ret) //corrupt property in some fashion
> > > > return ret;
> > > > } else {
> > > > //set default
> > > > }
> > > > If there is no default then check it unconditionally.
> > >
> > > default value is LM3533_ALS_RESISTOR_MIN and if no property is present
> > > clamp will ensure that als->r_select will be set to
> > > LM3533_ALS_RESISTOR_MIN
> >
> > I don't see that default in the binding doc and relying in the 0 being clamped
> > isn't particularly readable - I'd set it explicitly.
> >
>
> Oh, ye, my bad. Schema enforces one of props to be present and if pwn
> is present then resistor is ignored. What if I move resistor reading,
> clamping and conversion under !als->pwm_mode check? Then resistor must
> be present and hence must be checked unconditionally.
Sounds good.
>
> Additionally, I can comment original lm3533_als_setup with #if 0
> #endif then git formatting will be much cleaner and easier to review,
> and once we all come to result I will just remove entire commented
> block and Lee can pick clean commits.
No don't do that. If you flatten the two helpers as a precursor patch
then the changes in here will be easier to review anyway.
>
> >
> > >
> > > >
> > > > >
> > > > > - return 0;
> > > > > -}
> > > > > + als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> > > > > + LM3533_ALS_RESISTOR_MAX);
> > > > > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
> > > > >
> > > > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > > > - const struct lm3533_als_platform_data *pdata)
> > > > > -{
> > > > > - int ret;
> > > > > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > > > >
> > > > > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > > > > + ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, als->pwm_mode ?
> > > > > + LM3533_ALS_INPUT_MODE_MASK : 0,
> > > >
> > > > That's ugly. Better as
> > > >
> > > > ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF,
> > > > als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0,
> > > >
> > >
> > > Yes sure, just followed 80 char limit.
> > >
> > > > Though if there wasn't a layer hiding the regmap, it could just have been
> > > >
> > > > ret = regmap_assign_bits(lm3533->regmap, LM3533_REG_ALS_CONF,
> > > > LM3533_ALS_INPUT_MODE_MASK, als->pwm_mode);;
> > > >
> > > > which would have been nicer.
> > > >
> > > > I'm not particularly keen on the swashing of the helpers being in a patch
> >
> > smashing. (this definitely wasn't my best effort at English!)
> >
> > > > that is about switching the binding type as feels largely unrelated.
> > > > Should really have been a precursor, easier to review patch.
> > > >
> > >
> > > Removing of lm3533_update layer is not the scope of this patchset.
> >
> > Understood. I'm fine with just the refactor you are doing brought out as a precursor
> > patch.
> >
>
> I have looked into removing wrappers too. That seems to be less a
> hassle that I anticipated, so I will include regmap switch in the v2.
Ah ok. Even better.
>
> > >
> > > >
> > > > > + LM3533_ALS_INPUT_MODE_MASK);
> > > > > if (ret)
> > > > > - return ret;
> > > > > + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > > > > + als->pwm_mode);
> > > > >
> > > > > /* ALS input is always high impedance in PWM-mode. */
> > > > > - if (!pdata->pwm_mode) {
> > > > > - ret = lm3533_als_set_resistor(als, pdata->r_select);
> > > > > + if (!als->pwm_mode) {
> > > > > + ret = lm3533_write(lm3533, LM3533_REG_ALS_RESISTOR_SELECT,
> > > > > + (u8)als->r_select);
> > > >
> > > > Same applies here. Mostly an unrelated change as the only thing switching that
> > > > is related to the patch is one parameter.
> > > >
> > >
> > > Removing of lm3533_write layer is not the scope of this patchset.
> > >
> > > > > if (ret)
> > > > > - return ret;
> > > > > + return dev_err_probe(dev, ret, "failed to set resistor\n");
> > > > > }
> > > > >
> > > > > return 0;
> > > >
> > > > > @@ -852,25 +825,28 @@ static int lm3533_als_probe(struct platform_device *pdev)
> > > > > indio_dev->channels = lm3533_als_channels;
> > > > > indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
> > > > > indio_dev->name = dev_name(&pdev->dev);
> > > > > - iio_device_set_parent(indio_dev, pdev->dev.parent);
> > > >
> > > > I'm not sure why this was there in the first place. Hence not sure if it
> > > > is safe to remove.
> > > >
> > >
> > > This is directly related to OF conversion. The iio_device_set_parent
> > > bound indio_dev to parent, and it causes problems with OF now since
> > > als output has its own node and binding it to parent if wrong. Same
> > > story for backlight and leds btw.
> >
> > Is there any risk anyone was using the canonical path to get to the iio dev?
> > /sys/bus/platform/devices/..../iio\:deviceX
> > This is technically an ABI change be it a subtle one.
> >
>
> Linux kernel has no users of this driver, and it is in "stale" state
> for more then 2 years (maybe even longer). I have cc'd Johan Hovold.
>
> https://lore.kernel.org/lkml/ZmBcvtLCzllQDWVX@hovoldconsulting.com/
>
> This this 2 y. o. discussion and there were no actions ore movements.
> I assume this driver in its current form has no more users. This does
> not mean that it cannot be revived though.
So, just to check, are you a user of this code or is this more trying to
help out with old code?
Jonathan
>
> >
> > >
> > > >
> > > > > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> > > > > index 45795f2a1042..d707d43d5526 100644
> > > > > --- a/drivers/leds/leds-lm3533.c
> > > > > +++ b/drivers/leds/leds-lm3533.c
> > > >
> > > > >
> > > > > led->cb.dev = led->cdev.dev;
> > > > >
> > > > > - ret = lm3533_led_setup(led, pdata);
> > > > > + device_property_read_u32(&pdev->dev, "led-max-microamp",
> > > > > + &led->max_current);
> > > >
> > > > I'd prefer explicit setting of the default to be visible before this, or
> > > > the property_present pattern I mention in the IIO review above.
> > > >
> > >
> > > clamp will ensure that led->max_current will be set to
> > > LM3533_LED_MAX_CURRENT_MIN regardless if it it present
> >
> > As above, I'd prefer it set explicitly.
> >
>
> I understand your position and I am not denying it for ALS part, but
> LEDs don't belong to IIO subsystem and different subsystem maintainers
> may have drastically different preferences and requirements (ugh, PTSD
> in its full glory).
>
> > >
> > > > > + led->max_current = clamp(led->max_current, LM3533_LED_MAX_CURRENT_MIN,
> > > > > + LM3533_LED_MAX_CURRENT_MAX);
> > > >
> > > > I didn't look any further (busy day!)
> > >
> >
^ permalink raw reply
* Re: [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller
From: Daniel Thompson @ 2026-05-29 10:46 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <CAPVz0n3C8D+amSRkF=Koj6Niu6u8uz4LbMoRYEX32_ECm5-tSQ@mail.gmail.com>
On Fri, May 29, 2026 at 01:07:50PM +0300, Svyatoslav Ryhel wrote:
> пт, 29 трав. 2026 р. о 12:51 Daniel Thompson <daniel@riscstar.com> пише:
> >
> > On Thu, May 28, 2026 at 04:51:18PM +0300, Svyatoslav Ryhel wrote:
> > > Document 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>
> > > ---
> > > .../leds/backlight/ti,lm3533-backlight.yaml | 68 +++++++
> > > .../bindings/leds/ti,lm3533-leds.yaml | 66 +++++++
> > > .../devicetree/bindings/leds/ti,lm3533.yaml | 170 ++++++++++++++++++
> > > 3 files changed, 304 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
> > > create mode 100644 Documentation/devicetree/bindings/leds/ti,lm3533-leds.yaml
> > > create mode 100644 Documentation/devicetree/bindings/leds/ti,lm3533.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
> > > new file mode 100644
> > > index 000000000000..866b0fb8ed04
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
> > > @@ -0,0 +1,68 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/leds/backlight/ti,lm3533-backlight.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: TI LM3533 high voltage series LED strings
> > > +
> > > +description:
> > > + This is part of the TI LM3533 MFD device. It represents two high voltage series
> > > + LED strings for display backlight controlled by the TI LM3533.
> > > +
> > > +maintainers:
> > > + - Svyatoslav Ryhel <clamor95@gmail.com>
> > > +
> > > +allOf:
> > > + - $ref: /schemas/leds/backlight/common.yaml#
> > > +
> > > +properties:
> > > + compatible:
> > > + const: ti,lm3533-backlight
> > > +
> > > + reg:
> > > + description: Control bank selection (0 = bank A, 1 = bank B).
> > > + maximum: 1
> > > <snip>
> > > + ti,pwm-config-mask:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + description: |
> > > + Control Bank PWM Configuration Register mask that allows to configure
> > > + PWM input in Zones 0-4
> > > + BIT(0) - PWM Input is enabled
> > > + BIT(1) - PWM Input is enabled in Zone 0
> > > + BIT(2) - PWM Input is enabled in Zone 1
> > > + BIT(3) - PWM Input is enabled in Zone 2
> > > + BIT(4) - PWM Input is enabled in Zone 3
> > > + BIT(5) - PWM Input is enabled in Zone 4
> >
> > This is optional and the drive implements a default (zero) that is not
> > documented here.
> >
> > Is zero a sane default from a DT binding point of view?
> >
>
> Yes, if property is missing then PWM input is disabled which is
> equivalent to setting all bits to 0.
So the default should be documented in the bindings?
Daniel.
^ permalink raw reply
* Re: [PATCH v2] staging: sm750fb: remove unused variable
From: Dan Carpenter @ 2026-05-29 10:44 UTC (permalink / raw)
To: Onish Sharma
Cc: sudipm.mukherjee, teddy.wang, gregkh, linux-fbdev, linux-staging,
linux-kernel
In-Reply-To: <ahlszyY6Nd9ANz-X@stanley.mountain>
On Fri, May 29, 2026 at 01:39:11PM +0300, Dan Carpenter wrote:
> Which everyone is API is the one we should keep.
s/everyone/ever one/
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH v2] staging: sm750fb: rename pv_reg to io_base
From: Dan Carpenter @ 2026-05-29 10:41 UTC (permalink / raw)
To: Onish Sharma
Cc: sudipm.mukherjee, teddy.wang, gregkh, linux-fbdev, linux-staging,
linux-kernel
In-Reply-To: <20260529095233.9015-1-neharora23587@gmail.com>
On Fri, May 29, 2026 at 03:22:33PM +0530, Onish Sharma wrote:
> Rename pv_reg to io_base to follow kernel naming style and improve
> readability.
>
> No functional changes intended.
Run your patches through checkpatch. Also v2 patches need to be sent
in a specific format. This should be [PATCH v2 1/2]
https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/
> ---
^^^
There needs to be a note here to say what changed.
regards,
dan carpenter
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox