* 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
* Re: [PATCH v2] staging: sm750fb: remove unused variable
From: Dan Carpenter @ 2026-05-29 10:39 UTC (permalink / raw)
To: Onish Sharma
Cc: sudipm.mukherjee, teddy.wang, gregkh, linux-fbdev, linux-staging,
linux-kernel
In-Reply-To: <20260529101242.10189-1-neharora23587@gmail.com>
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
* Re: [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
From: Daniel Thompson @ 2026-05-29 10:35 UTC (permalink / raw)
To: Neil Armstrong
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: <e3c99fe3-9279-4dfa-af69-d9366ab06837@linaro.org>
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).
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?
> 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.
Daniel.
^ 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 10:16 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: <ahllT_HVTAJ5MbkS@aspen.lan>
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.
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.
This should probably be a comment in the code.
Neil
>
>
> Daniel.
^ permalink raw reply
* [PATCH v2] staging: sm750fb: remove unused variable
From: Onish Sharma @ 2026-05-29 10:12 UTC (permalink / raw)
To: sudipm.mukherjee
Cc: teddy.wang, gregkh, linux-fbdev, linux-staging, linux-kernel,
Onish Sharma, Dan Carpenter
In-Reply-To: <ahlXYIqzu4O5-u9J@stanley.mountain>
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>
---
drivers/staging/sm750fb/ddk750_chip.c | 28 ---------------------------
drivers/staging/sm750fb/ddk750_chip.h | 7 -------
drivers/staging/sm750fb/sm750.c | 1 -
drivers/staging/sm750fb/sm750.h | 1 -
4 files changed, 37 deletions(-)
diff --git a/drivers/staging/sm750fb/ddk750_chip.c b/drivers/staging/sm750fb/ddk750_chip.c
index 0bb56bbec43f..553654a77170 100644
--- a/drivers/staging/sm750fb/ddk750_chip.c
+++ b/drivers/staging/sm750fb/ddk750_chip.c
@@ -262,34 +262,6 @@ int ddk750_init_hw(struct initchip_param *p_init_param)
reg |= MISC_CTRL_LOCALMEM_RESET;
poke32(MISC_CTRL, reg);
}
-
- if (p_init_param->set_all_eng_off == 1) {
- sm750_enable_2d_engine(0);
-
- /* Disable Overlay, if a former application left it on */
- reg = peek32(VIDEO_DISPLAY_CTRL);
- reg &= ~DISPLAY_CTRL_PLANE;
- poke32(VIDEO_DISPLAY_CTRL, reg);
-
- /* Disable video alpha, if a former application left it on */
- reg = peek32(VIDEO_ALPHA_DISPLAY_CTRL);
- reg &= ~DISPLAY_CTRL_PLANE;
- poke32(VIDEO_ALPHA_DISPLAY_CTRL, reg);
-
- /* Disable alpha plane, if a former application left it on */
- reg = peek32(ALPHA_DISPLAY_CTRL);
- reg &= ~DISPLAY_CTRL_PLANE;
- poke32(ALPHA_DISPLAY_CTRL, reg);
-
- /* Disable DMA Channel, if a former application left it on */
- reg = peek32(DMA_ABORT_INTERRUPT);
- reg |= DMA_ABORT_INTERRUPT_ABORT_1;
- poke32(DMA_ABORT_INTERRUPT, reg);
-
- /* Disable DMA Power, if a former application left it on */
- sm750_enable_dma(0);
- }
-
/* We can add more initialization as needed. */
return 0;
diff --git a/drivers/staging/sm750fb/ddk750_chip.h b/drivers/staging/sm750fb/ddk750_chip.h
index ee2e9d90f7dd..2a13debc179f 100644
--- a/drivers/staging/sm750fb/ddk750_chip.h
+++ b/drivers/staging/sm750fb/ddk750_chip.h
@@ -76,13 +76,6 @@ struct initchip_param {
*/
unsigned short master_clock;
- /*
- * 0 = leave all engine state untouched.
- * 1 = make sure they are off: 2D, Overlay,
- * video alpha, alpha, hardware cursors
- */
- unsigned short set_all_eng_off;
-
/*
* 0 = Do not reset the memory controller
* 1 = Reset the memory controller
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index c2d2864f135b..127db29883d2 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -848,7 +848,6 @@ static void sm750fb_setup(struct sm750_dev *sm750_dev, char *src)
sm750_dev->init_parm.mem_clk = 0;
sm750_dev->init_parm.master_clk = 0;
sm750_dev->init_parm.power_mode = 0;
- sm750_dev->init_parm.set_all_eng_off = 0;
sm750_dev->init_parm.reset_memory = 1;
/* defaultly turn g_hwcursor on for both view */
diff --git a/drivers/staging/sm750fb/sm750.h b/drivers/staging/sm750fb/sm750.h
index c42800313c6a..7ab13da5d7cc 100644
--- a/drivers/staging/sm750fb/sm750.h
+++ b/drivers/staging/sm750fb/sm750.h
@@ -44,7 +44,6 @@ struct init_status {
ushort chip_clk;
ushort mem_clk;
ushort master_clk;
- ushort set_all_eng_off;
ushort reset_memory;
};
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller
From: Svyatoslav Ryhel @ 2026-05-29 10:07 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: <ahlhinOh3NxB7FY_@aspen.lan>
пт, 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.
>
> 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 10:07 UTC (permalink / raw)
To: Neil Armstrong
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: <20260519-topic-sm8650-ayaneo-pocket-s2-sy7758-v3-1-ec8194bbc885@linaro.org>
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).
Daniel.
^ permalink raw reply
* Re: [PATCH] backlight: Use named initializers for arrays of i2c_device_data
From: Daniel Thompson @ 2026-05-29 10:01 UTC (permalink / raw)
To: Uwe Kleine-König (The Capable Hub)
Cc: Lee Jones, Jingoo Han, Michael Hennerich, Helge Deller,
Junjie Cao, Jianhua Lu, Flavio Suligoi, dri-devel, linux-fbdev,
linux-kernel
In-Reply-To: <20260518111203.639603-2-u.kleine-koenig@baylibre.com>
On Mon, May 18, 2026 at 01:12:03PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> While being less compact, using named initializers allows to more easily
> see which members of the structs are assigned which value without having
> to lookup the declaration of the struct. And it's also more robust
> against changes to the struct definition.
>
> The mentioned robustness is relevant for a planned change to struct
> i2c_device_id that replaces .driver_data by an anonymous union.
>
> While touching all these arrays, unify usage of whitespace in the list
> terminator.
>
> This patch doesn't modify the compiled arrays, only their representation
> in source form benefits. The former was confirmed with x86 and arm64
> builds.
>
> Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org>
Daniel.
^ permalink raw reply
* [PATCH v2] staging: sm750fb: rename pv_reg to io_base
From: Onish Sharma @ 2026-05-29 9:52 UTC (permalink / raw)
To: sudipm.mukherjee
Cc: teddy.wang, gregkh, linux-fbdev, linux-staging, linux-kernel,
Onish Sharma
In-Reply-To: <ahhN_kiSb-yRWfiI@stanley.mountain>
Rename pv_reg to io_base to follow kernel naming style and improve
readability.
No functional changes intended.
---
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 1/6] dt-bindings: leds: Document TI LM3533 LED controller
From: Daniel Thompson @ 2026-05-29 9:51 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-2-clamor95@gmail.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?
Daniel.
^ permalink raw reply
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
From: Svyatoslav Ryhel @ 2026-05-29 9:39 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: <20260529100819.1823ebb3@jic23-huawei>
пт, 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.
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.
>
> >
> > >
> > > >
> > > > - 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.
> >
> > >
> > > > + 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.
>
> >
> > >
> > > > 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: Jonathan Cameron @ 2026-05-29 9:08 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: <CAPVz0n0qCekQVGGyAyBuYv+RKC6bpydYBLJNGfPrgTYjtOJOuA@mail.gmail.com>
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.
>
> >
> > >
> > > - 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.
>
> >
> > > + 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.
>
> >
> > > 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.
>
> > > + 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 v1 0/8] zorro: Improve handling of pointers in zorro_device_id::driver_data
From: patchwork-bot+netdevbpf @ 2026-05-29 1:20 UTC (permalink / raw)
To: =?utf-8?q?Uwe_Kleine-K=C3=B6nig_=28The_Capable_Hub=29_=3Cu=2Ekleine-koenig?=,
=?utf-8?q?=40baylibre=2Ecom=3E?=
Cc: geert, dlemoal, cassel, James.Bottomley, martin.petersen,
andrew+netdev, davem, edumazet, kuba, pabeni, tglx, mingo, max,
andi.shyti, deller, linux-ide, linux-m68k, linux-kernel,
linux-scsi, netdev, linux-i2c, linux-fbdev, dri-devel,
christian.ehrhardt, lk
In-Reply-To: <cover.1779803053.git.u.kleine-koenig@baylibre.com>
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Tue, 26 May 2026 16:17:26 +0200 you wrote:
> Hello,
>
> this series is about improving the handling of pointers in struct
> zorro_device_id's driver_data.
>
> While it's ok on all current Linux platforms to store a pointer in an
> unsigned long variable, it involves casting that loses type information.
> This can be nicely seen in patch #7 where after profiting from patch #6
> the compiler notices a missing const.
>
> [...]
Here is the summary with links:
- [v1,net-next,3/8] net: Use named initializer for zorro_device_id arrays
https://git.kernel.org/netdev/net-next/c/4933a658369a
- [v1,6/8] zorro: Simplify storing pointers in device id struct
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* [PATCH] staging: sm750fb: rename Bpp parameter to bpp
From: Gabry @ 2026-05-28 18:36 UTC (permalink / raw)
To: sudipm.mukherjee@gmail.com, teddy.wang@siliconmotion.com,
gregkh@linuxfoundation.org
Cc: linux-fbdev@vger.kernel.org, linux-staging@lists.linux.dev,
linux-kernel@vger.kernel.org
The Linux kernel coding style prefers snake_case over CamelCase foridentifier names.
Rename the 'Bpp' parameter (bytes per pixel) of sm750_hw_fillrect()
and sm750_hw_copyarea() to 'bpp' to comply with this standard. Update
the function prototypes in sm750_accel.h and the corresponding
kernel-doc descriptions accordingly.
This is a pure rename with no functional change, and addresses a
checkpatch.pl warning:
CHECK: Avoid CamelCase: <Bpp>
Signed-off-by: Gabriele Rizzo <gabry.256@proton.me>
---
drivers/staging/sm750fb/sm750_accel.c | 22 +++++++++++-----------
drivers/staging/sm750fb/sm750_accel.h | 6 +++---
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750_accel.c b/drivers/staging/sm750fb/sm750_accel.c
index 0f94d859e91c..4beabe1053f9 100644
--- a/drivers/staging/sm750fb/sm750_accel.c
+++ b/drivers/staging/sm750fb/sm750_accel.c
@@ -85,7 +85,7 @@ void sm750_hw_set2dformat(struct lynx_accel *accel, int fmt)
}
int sm750_hw_fillrect(struct lynx_accel *accel,
- u32 base, u32 pitch, u32 Bpp,
+ u32 base, u32 pitch, u32 bpp,
u32 x, u32 y, u32 width, u32 height,
u32 color, u32 rop)
{
@@ -102,14 +102,14 @@ int sm750_hw_fillrect(struct lynx_accel *accel,
write_dpr(accel, DE_WINDOW_DESTINATION_BASE, base); /* dpr40 */
write_dpr(accel, DE_PITCH,
- ((pitch / Bpp << DE_PITCH_DESTINATION_SHIFT) &
+ ((pitch / bpp << DE_PITCH_DESTINATION_SHIFT) &
DE_PITCH_DESTINATION_MASK) |
- (pitch / Bpp & DE_PITCH_SOURCE_MASK)); /* dpr10 */
+ (pitch / bpp & DE_PITCH_SOURCE_MASK)); /* dpr10 */
write_dpr(accel, DE_WINDOW_WIDTH,
- ((pitch / Bpp << DE_WINDOW_WIDTH_DST_SHIFT) &
+ ((pitch / bpp << DE_WINDOW_WIDTH_DST_SHIFT) &
DE_WINDOW_WIDTH_DST_MASK) |
- (pitch / Bpp & DE_WINDOW_WIDTH_SRC_MASK)); /* dpr44 */
+ (pitch / bpp & DE_WINDOW_WIDTH_SRC_MASK)); /* dpr44 */
write_dpr(accel, DE_FOREGROUND, color); /* DPR14 */
@@ -138,7 +138,7 @@ int sm750_hw_fillrect(struct lynx_accel *accel,
* @sy: Starting y coordinate of source surface
* @dest_base: Address of destination: offset in frame buffer
* @dest_pitch: Pitch value of destination surface in BYTE
- * @Bpp: Color depth of destination surface
+ * @bpp: Color depth of destination surface
* @dx: Starting x coordinate of destination surface
* @dy: Starting y coordinate of destination surface
* @width: width of rectangle in pixel value
@@ -149,7 +149,7 @@ int sm750_hw_copyarea(struct lynx_accel *accel,
unsigned int source_base, unsigned int source_pitch,
unsigned int sx, unsigned int sy,
unsigned int dest_base, unsigned int dest_pitch,
- unsigned int Bpp, unsigned int dx, unsigned int dy,
+ unsigned int bpp, unsigned int dx, unsigned int dy,
unsigned int width, unsigned int height,
unsigned int rop2)
{
@@ -249,9 +249,9 @@ int sm750_hw_copyarea(struct lynx_accel *accel,
* pixel values. Need Byte to pixel conversion.
*/
write_dpr(accel, DE_PITCH,
- ((dest_pitch / Bpp << DE_PITCH_DESTINATION_SHIFT) &
+ ((dest_pitch / bpp << DE_PITCH_DESTINATION_SHIFT) &
DE_PITCH_DESTINATION_MASK) |
- (source_pitch / Bpp & DE_PITCH_SOURCE_MASK)); /* dpr10 */
+ (source_pitch / bpp & DE_PITCH_SOURCE_MASK)); /* dpr10 */
/*
* Screen Window width in Pixels.
@@ -259,9 +259,9 @@ int sm750_hw_copyarea(struct lynx_accel *accel,
* for a given point.
*/
write_dpr(accel, DE_WINDOW_WIDTH,
- ((dest_pitch / Bpp << DE_WINDOW_WIDTH_DST_SHIFT) &
+ ((dest_pitch / bpp << DE_WINDOW_WIDTH_DST_SHIFT) &
DE_WINDOW_WIDTH_DST_MASK) |
- (source_pitch / Bpp & DE_WINDOW_WIDTH_SRC_MASK)); /* dpr3c */
+ (source_pitch / bpp & DE_WINDOW_WIDTH_SRC_MASK)); /* dpr3c */
if (accel->de_wait() != 0)
return -1;
diff --git a/drivers/staging/sm750fb/sm750_accel.h b/drivers/staging/sm750fb/sm750_accel.h
index 2c79cb730a0a..d15a40cacb84 100644
--- a/drivers/staging/sm750fb/sm750_accel.h
+++ b/drivers/staging/sm750fb/sm750_accel.h
@@ -190,7 +190,7 @@ void sm750_hw_set2dformat(struct lynx_accel *accel, int fmt);
void sm750_hw_de_init(struct lynx_accel *accel);
int sm750_hw_fillrect(struct lynx_accel *accel,
- u32 base, u32 pitch, u32 Bpp,
+ u32 base, u32 pitch, u32 bpp,
u32 x, u32 y, u32 width, u32 height,
u32 color, u32 rop);
@@ -202,7 +202,7 @@ int sm750_hw_fillrect(struct lynx_accel *accel,
* @sy: Starting y coordinate of source surface
* @dBase: Address of destination: offset in frame buffer
* @dPitch: Pitch value of destination surface in BYTE
- * @Bpp: Color depth of destination surface
+ * @bpp: Color depth of destination surface
* @dx: Starting x coordinate of destination surface
* @dy: Starting y coordinate of destination surface
* @width: width of rectangle in pixel value
@@ -213,7 +213,7 @@ int sm750_hw_copyarea(struct lynx_accel *accel,
unsigned int sBase, unsigned int sPitch,
unsigned int sx, unsigned int sy,
unsigned int dBase, unsigned int dPitch,
- unsigned int Bpp, unsigned int dx, unsigned int dy,
+ unsigned int bpp, unsigned int dx, unsigned int dy,
unsigned int width, unsigned int height,
unsigned int rop2);
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
From: Svyatoslav Ryhel @ 2026-05-28 15:03 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: <20260528155001.2bcb7003@jic23-huawei>
чт, 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
> 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
>
> >
> > - 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
> 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.
>
> > + 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.
>
> > 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
> > + 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: Jonathan Cameron @ 2026-05-28 14:54 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: <20260528135123.103745-2-clamor95@gmail.com>
On Thu, 28 May 2026 16:51:18 +0300
Svyatoslav Ryhel <clamor95@gmail.com> 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>
> ---
The light sensor binding looks fine to me. I didn't check the rest.
Reviewed-by: Jonathan Cameron <jic23@kernel.org> #for light sensor
^ permalink raw reply
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
From: Jonathan Cameron @ 2026-05-28 14:50 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: <20260528135123.103745-3-clamor95@gmail.com>
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
as a precursor patch to make the rest much more readable.
>
> 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.
>
> - 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,
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
that is about switching the binding type as feels largely unrelated.
Should really have been a precursor, easier to review patch.
> + 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.
> 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.
> 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.
> + 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 1/2] staging: sm750fb: rename pvReg to pv_reg
From: Dan Carpenter @ 2026-05-28 14:15 UTC (permalink / raw)
To: Onish Sharma
Cc: sudipm.mukherjee, gregkh, linux-staging, linux-fbdev,
linux-kernel
In-Reply-To: <20260528133627.10850-1-neharora23587@gmail.com>
On Thu, May 28, 2026 at 07:06:26PM +0530, Onish Sharma wrote:
> Rename pvReg to pv_reg to comply with kernel coding style (checkpatch.pl)
> and improve readability.
>
> Signed-off-by: Onish Sharma <neharora23587@gmail.com>
pv stands for pointer void. It's Hungarian notation and it's not
allowed.
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