* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
From: Jonathan Cameron @ 2026-06-02 14:20 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: <CAPVz0n1r97d8-uzhPGBx0LFSp75A3_2mMXDQQ30utT-6NtpHNA@mail.gmail.com>
On Tue, 2 Jun 2026 16:50:16 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> вт, 2 черв. 2026 р. о 16:46 Jonathan Cameron <jic23@kernel.org> пише:
> >
> > On Mon, 1 Jun 2026 18:18:25 +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.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >
> >
> > > index 52136ca1abc9..55b35467a722 100644
> > > --- a/drivers/iio/light/lm3533-als.c
> > > +++ b/drivers/iio/light/lm3533-als.c
> > > @@ -16,16 +16,19 @@
> > > #include <linux/module.h>
> > > #include <linux/mutex.h>
> > > #include <linux/mfd/core.h>
> > > +#include <linux/mod_devicetable.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/property.h>
> > > #include <linux/regmap.h>
> > > #include <linux/slab.h>
> > > #include <linux/uaccess.h>
> > > +#include <linux/units.h>
> > >
> > > #include <linux/mfd/lm3533.h>
> > >
> > >
> > > -#define LM3533_ALS_RESISTOR_MIN 1
> > > -#define LM3533_ALS_RESISTOR_MAX 127
> > > +#define LM3533_ALS_RESISTOR_MIN 1575
> > > +#define LM3533_ALS_RESISTOR_MAX 200000
> > > #define LM3533_ALS_CHANNEL_CURRENT_MAX 2
> > > #define LM3533_ALS_THRESH_MAX 3
> > > #define LM3533_ALS_ZONE_MAX 4
> > > @@ -57,6 +60,9 @@ struct lm3533_als {
> > >
> > > atomic_t zone;
> > > struct mutex thresh_mutex;
> > > +
> > > + bool pwm_mode;
> > > + u32 r_select;
> > > };
> > >
> > >
> > > @@ -411,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev,
> > > int enable;
> > > int ret;
> > >
> > > - if (als->irq) {
> > > + if (als->irq > 0) {
> > > ret = lm3533_als_get_int_mode(indio_dev, &enable);
> > > if (ret)
> > > return ret;
> > > @@ -716,30 +722,34 @@ static const struct attribute_group lm3533_als_attribute_group = {
> > > .attrs = lm3533_als_attributes
> > > };
> > >
> > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > - const struct lm3533_als_platform_data *pdata)
> > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > {
> > > struct device *dev = &als->pdev->dev;
> > > int ret;
> > >
> > > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > > +
> > > ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > > LM3533_ALS_INPUT_MODE_MASK,
> > > - pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > > + als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > > if (ret)
> > > return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > > - pdata->pwm_mode);
> > > -
> > > + als->pwm_mode);
> > >
> > > /* ALS input is always high impedance in PWM-mode. */
> > > - if (!pdata->pwm_mode) {
> > > - if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> > > - pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> > > - dev_err(&als->pdev->dev, "invalid resistor value\n");
> > > - return -EINVAL;
> > > - }
> > > + if (!als->pwm_mode) {
> > > + ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
> > > + &als->r_select);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret,
> > > + "failed to ger resistor value\n");
> > > +
> > > + als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> > > + LM3533_ALS_RESISTOR_MAX);
> >
> > If we are getting garbage from DT I think I'd rather error out that paper over
> > that problem. So similar to before, check valid value and if not fail probe
> > so that hopefully someone goes and fixes it!
> >
>
> sure
>
> > > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
> > Why do we need this when we didn't before? The range checks are the same
> > so it smells like it shouldn't need transforming. I'd also rather we didn't do
> > rewriting of the meaning of r_select like this. Just use a local variable for
> > the intermediate result.
> >
>
> before pdata passed resistor value as actual register value, not we
> are getting the actual resistance in ohms from the tree and must
> convert it into register value.
ah. I missed the change of values. Can you make them explicitly now _OHMS or something
along those lines rather than reusing the macro name for a different thing.
>
> > >
> > > ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> > > - pdata->r_select);
> > > + als->r_select);
> > > if (ret)
> > > return dev_err_probe(dev, ret, "failed to set resistor\n");
^ permalink raw reply
* Re: [PATCH v3 10/11] video: backlight: lm3533_bl: Implement backlight_scale property
From: Daniel Thompson @ 2026-06-02 14:06 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, 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: <20260601151831.76350-11-clamor95@gmail.com>
On Mon, Jun 01, 2026 at 06:18:30PM +0300, Svyatoslav Ryhel wrote:
> Since the device supports linear and non-linear modes, implement the
> backlight_scale property to describe this state.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Thanks!
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org>
Daniel.
^ permalink raw reply
* Re: [PATCH v3 09/11] video: backlight: lm3533_bl: Set initial mapping mode from DT
From: Daniel Thompson @ 2026-06-02 14:05 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, 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: <20260601151831.76350-10-clamor95@gmail.com>
On Mon, Jun 01, 2026 at 06:18:29PM +0300, Svyatoslav Ryhel wrote:
> Add support to obtain the initial mapping mode from DT instead of leaving
> it unconfigured.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/video/backlight/lm3533_bl.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> index 36e6f027613a..f0d88b7bc229 100644
> --- a/drivers/video/backlight/lm3533_bl.c
> +++ b/drivers/video/backlight/lm3533_bl.c
> @@ -34,6 +34,7 @@ struct lm3533_bl {
>
> u32 max_current;
> u32 pwm;
> + bool linear;
> };
>
>
> @@ -247,8 +248,15 @@ static struct attribute_group lm3533_bl_attribute_group = {
>
> static int lm3533_bl_setup(struct lm3533_bl *bl)
> {
> + int id = lm3533_bl_get_ctrlbank_id(bl);
> int ret;
>
> + ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> + CTRLBANK_AB_BCONF_MODE(id),
> + bl->linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
I guess this is another candidate for regmap_assign_bits() but with that
change:
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org>
Daniel.
^ permalink raw reply
* Re: [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic
From: Daniel Thompson @ 2026-06-02 13:55 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, 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: <20260601151831.76350-9-clamor95@gmail.com>
On Mon, Jun 01, 2026 at 06:18:28PM +0300, Svyatoslav Ryhel wrote:
> Simplify the sysfs logic of the linear property by switching to a macro
> and a ternary operator.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
With the change to regmap_assign_bits():
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org>
Daniel.
^ permalink raw reply
* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
From: Svyatoslav Ryhel @ 2026-06-02 13:50 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: <20260602144640.433b4d35@jic23-huawei>
вт, 2 черв. 2026 р. о 16:46 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Mon, 1 Jun 2026 18:18:25 +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.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>
>
> > index 52136ca1abc9..55b35467a722 100644
> > --- a/drivers/iio/light/lm3533-als.c
> > +++ b/drivers/iio/light/lm3533-als.c
> > @@ -16,16 +16,19 @@
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/regmap.h>
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
> > +#include <linux/units.h>
> >
> > #include <linux/mfd/lm3533.h>
> >
> >
> > -#define LM3533_ALS_RESISTOR_MIN 1
> > -#define LM3533_ALS_RESISTOR_MAX 127
> > +#define LM3533_ALS_RESISTOR_MIN 1575
> > +#define LM3533_ALS_RESISTOR_MAX 200000
> > #define LM3533_ALS_CHANNEL_CURRENT_MAX 2
> > #define LM3533_ALS_THRESH_MAX 3
> > #define LM3533_ALS_ZONE_MAX 4
> > @@ -57,6 +60,9 @@ struct lm3533_als {
> >
> > atomic_t zone;
> > struct mutex thresh_mutex;
> > +
> > + bool pwm_mode;
> > + u32 r_select;
> > };
> >
> >
> > @@ -411,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev,
> > int enable;
> > int ret;
> >
> > - if (als->irq) {
> > + if (als->irq > 0) {
> > ret = lm3533_als_get_int_mode(indio_dev, &enable);
> > if (ret)
> > return ret;
> > @@ -716,30 +722,34 @@ static const struct attribute_group lm3533_als_attribute_group = {
> > .attrs = lm3533_als_attributes
> > };
> >
> > -static int lm3533_als_setup(struct lm3533_als *als,
> > - const struct lm3533_als_platform_data *pdata)
> > +static int lm3533_als_setup(struct lm3533_als *als)
> > {
> > struct device *dev = &als->pdev->dev;
> > int ret;
> >
> > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > +
> > ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > LM3533_ALS_INPUT_MODE_MASK,
> > - pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > + als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > if (ret)
> > return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > - pdata->pwm_mode);
> > -
> > + als->pwm_mode);
> >
> > /* ALS input is always high impedance in PWM-mode. */
> > - if (!pdata->pwm_mode) {
> > - if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> > - pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> > - dev_err(&als->pdev->dev, "invalid resistor value\n");
> > - return -EINVAL;
> > - }
> > + if (!als->pwm_mode) {
> > + ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
> > + &als->r_select);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "failed to ger resistor value\n");
> > +
> > + als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> > + LM3533_ALS_RESISTOR_MAX);
>
> If we are getting garbage from DT I think I'd rather error out that paper over
> that problem. So similar to before, check valid value and if not fail probe
> so that hopefully someone goes and fixes it!
>
sure
> > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
> Why do we need this when we didn't before? The range checks are the same
> so it smells like it shouldn't need transforming. I'd also rather we didn't do
> rewriting of the meaning of r_select like this. Just use a local variable for
> the intermediate result.
>
before pdata passed resistor value as actual register value, not we
are getting the actual resistance in ohms from the tree and must
convert it into register value.
> >
> > ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> > - pdata->r_select);
> > + als->r_select);
> > if (ret)
> > return dev_err_probe(dev, ret, "failed to set resistor\n");
^ permalink raw reply
* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
From: Daniel Thompson @ 2026-06-02 13:49 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, 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: <20260601151831.76350-6-clamor95@gmail.com>
On Mon, Jun 01, 2026 at 06:18:25PM +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.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org> #for backlight
Daniel.
^ permalink raw reply
* Re: [PATCH v3 01/11] dt-bindings: leds: Document TI LM3533 LED controller
From: Daniel Thompson @ 2026-06-02 13:49 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, 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: <20260601151831.76350-2-clamor95@gmail.com>
On Mon, Jun 01, 2026 at 06:18:21PM +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>
> Reviewed-by: Jonathan Cameron <jic23@kernel.org> #for light sensor
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org> #for backlight
Daniel.
^ permalink raw reply
* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
From: Jonathan Cameron @ 2026-06-02 13:46 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: <20260601151831.76350-6-clamor95@gmail.com>
On Mon, 1 Jun 2026 18:18:25 +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.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> index 52136ca1abc9..55b35467a722 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
> @@ -16,16 +16,19 @@
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/regmap.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> +#include <linux/units.h>
>
> #include <linux/mfd/lm3533.h>
>
>
> -#define LM3533_ALS_RESISTOR_MIN 1
> -#define LM3533_ALS_RESISTOR_MAX 127
> +#define LM3533_ALS_RESISTOR_MIN 1575
> +#define LM3533_ALS_RESISTOR_MAX 200000
> #define LM3533_ALS_CHANNEL_CURRENT_MAX 2
> #define LM3533_ALS_THRESH_MAX 3
> #define LM3533_ALS_ZONE_MAX 4
> @@ -57,6 +60,9 @@ struct lm3533_als {
>
> atomic_t zone;
> struct mutex thresh_mutex;
> +
> + bool pwm_mode;
> + u32 r_select;
> };
>
>
> @@ -411,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev,
> int enable;
> int ret;
>
> - if (als->irq) {
> + if (als->irq > 0) {
> ret = lm3533_als_get_int_mode(indio_dev, &enable);
> if (ret)
> return ret;
> @@ -716,30 +722,34 @@ static const struct attribute_group lm3533_als_attribute_group = {
> .attrs = lm3533_als_attributes
> };
>
> -static int lm3533_als_setup(struct lm3533_als *als,
> - const struct lm3533_als_platform_data *pdata)
> +static int lm3533_als_setup(struct lm3533_als *als)
> {
> struct device *dev = &als->pdev->dev;
> int ret;
>
> + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> +
> ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> LM3533_ALS_INPUT_MODE_MASK,
> - pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> + als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> if (ret)
> return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> - pdata->pwm_mode);
> -
> + als->pwm_mode);
>
> /* ALS input is always high impedance in PWM-mode. */
> - if (!pdata->pwm_mode) {
> - if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> - pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> - dev_err(&als->pdev->dev, "invalid resistor value\n");
> - return -EINVAL;
> - }
> + if (!als->pwm_mode) {
> + ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
> + &als->r_select);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to ger resistor value\n");
> +
> + als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> + LM3533_ALS_RESISTOR_MAX);
If we are getting garbage from DT I think I'd rather error out that paper over
that problem. So similar to before, check valid value and if not fail probe
so that hopefully someone goes and fixes it!
> + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
Why do we need this when we didn't before? The range checks are the same
so it smells like it shouldn't need transforming. I'd also rather we didn't do
rewriting of the meaning of r_select like this. Just use a local variable for
the intermediate result.
>
> ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> - pdata->r_select);
> + als->r_select);
> if (ret)
> return dev_err_probe(dev, ret, "failed to set resistor\n");
^ permalink raw reply
* Re: [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers
From: Svyatoslav Ryhel @ 2026-06-02 13:45 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: <20260602144222.7a50a041@jic23-huawei>
вт, 2 черв. 2026 р. о 16:42 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Mon, 1 Jun 2026 18:18:23 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > The lm3533_als_set_input_mode and lm3533_als_set_resistor functions are
> > used only in lm3533_als_setup. Incorporate their code into
> > lm3533_als_setup directly to simplify driver readability.
> Minor stuff inline.
>
>
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> > drivers/iio/light/lm3533-als.c | 61 +++++++++-------------------------
> > 1 file changed, 16 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > index fb61904f110f..52136ca1abc9 100644
> > --- a/drivers/iio/light/lm3533-als.c
> > +++ b/drivers/iio/light/lm3533-als.c
>
> > static int lm3533_als_setup(struct lm3533_als *als,
> > const struct lm3533_als_platform_data *pdata)
> > {
> > + struct device *dev = &als->pdev->dev;
> > int ret;
> >
> > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
>
> Maybe a local struct regmap pointer given dereferenced in a couple of places.
>
sure, why not
> > + LM3533_ALS_INPUT_MODE_MASK,
> > + pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
>
> Andy raised this in previous patch but in the interests of being specific
> regmap_assign_bits() is going to be cleaner here.
>
I am currently adjusting accordingly.
> > if (ret)
> > - return ret;
> > + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > + pdata->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 (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> > + pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> > + dev_err(&als->pdev->dev, "invalid resistor value\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> > + pdata->r_select);
> > if (ret)
> > - return ret;
> > + return dev_err_probe(dev, ret, "failed to set resistor\n");
> > }
> >
> > return 0;
>
^ permalink raw reply
* Re: [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers
From: Jonathan Cameron @ 2026-06-02 13:42 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: <20260601151831.76350-4-clamor95@gmail.com>
On Mon, 1 Jun 2026 18:18:23 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> The lm3533_als_set_input_mode and lm3533_als_set_resistor functions are
> used only in lm3533_als_setup. Incorporate their code into
> lm3533_als_setup directly to simplify driver readability.
Minor stuff inline.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/iio/light/lm3533-als.c | 61 +++++++++-------------------------
> 1 file changed, 16 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> index fb61904f110f..52136ca1abc9 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
> static int lm3533_als_setup(struct lm3533_als *als,
> const struct lm3533_als_platform_data *pdata)
> {
> + struct device *dev = &als->pdev->dev;
> int ret;
>
> - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
Maybe a local struct regmap pointer given dereferenced in a couple of places.
> + LM3533_ALS_INPUT_MODE_MASK,
> + pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
Andy raised this in previous patch but in the interests of being specific
regmap_assign_bits() is going to be cleaner here.
> if (ret)
> - return ret;
> + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> + pdata->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 (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> + pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> + dev_err(&als->pdev->dev, "invalid resistor value\n");
> + return -EINVAL;
> + }
> +
> + ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> + pdata->r_select);
> if (ret)
> - return ret;
> + return dev_err_probe(dev, ret, "failed to set resistor\n");
> }
>
> return 0;
^ permalink raw reply
* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
From: Svyatoslav Ryhel @ 2026-06-02 12:00 UTC (permalink / raw)
To: Andy Shevchenko
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: <ah648F2plc4UHTM1@ashevche-desk.local>
вт, 2 черв. 2026 р. о 14:05 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Tue, Jun 02, 2026 at 01:31:44PM +0300, Svyatoslav Ryhel wrote:
> > вт, 2 черв. 2026 р. о 11:24 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > > On Mon, Jun 01, 2026 at 06:18:25PM +0300, Svyatoslav Ryhel wrote:
>
>
> ...
>
> > > > + device_for_each_child_node_scoped(lm3533->dev, child) {
> > >
> > > > + if (!fwnode_device_is_available(child))
> > > > + continue;
> > >
> > > Do we need this check?
> >
> > This is nice to have if the node is disabled. If we assume that there
> > are no disabled nodes, I can remove it.
>
> It's already implied. See
>
> static struct fwnode_handle *
> of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, struct fwnode_handle *child)
> {
> return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), to_of_node(child)));
> }
>
> And I believe it's written somewhere in the documentation (if not, feel free to
> patch that).
>
Very nice. Thank you.
> ...
>
> > > > + ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
> > >
> > > No way. You should use .dev_groups.
> >
> > I did not change how driver does this, just swapped lm3533->dev to
> > dev. I will set is back as it was.
>
> This is a serious race condition that needs to be addressed. Since you are
> touching this driver the fixes against known issues probably are the first
> things that have to be done.
>
Fine, I will have a look.
> > > > + if (ret) {
> > > > + dev_err(dev, "failed to create sysfs attributes\n");
> > > > goto err_unregister;
> > > > }
>
> ...
>
> > > Can you think on how to split this change to smaller steps? I believe it's
> > > possible.
> >
> > No, I am done with tinkering with this patchset. It is broken enough
> > and it has inflated enough.
>
> Probably you don't want this to be reviewed then? I believe other kernel
> developers and maintainers will ask you the same.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
* Re: [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic
From: Svyatoslav Ryhel @ 2026-06-02 11:59 UTC (permalink / raw)
To: Andy Shevchenko
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: <ah65hfgoM67V6-iR@ashevche-desk.local>
вт, 2 черв. 2026 р. о 14:07 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Tue, Jun 02, 2026 at 01:19:00PM +0300, Svyatoslav Ryhel wrote:
> > вт, 2 черв. 2026 р. о 11:09 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > > On Mon, Jun 01, 2026 at 06:18:28PM +0300, Svyatoslav Ryhel wrote:
>
> ...
>
> > > > if (kstrtoul(buf, 0, &linear))
> > > > return -EINVAL;
> > >
> > > Besides _assign_bits() in the below, side note here to unshadow error codes:
> > >
> > > ret = kstrtoul(buf, 0, &linear);
> > > if (ret)
> > > return ret;
> > >
> > > (obviously in a separate change).
> >
> > Won't happen in this patches.
>
> You mean both suggestions or you are talking about kstrotoul() only? If it's
> only about the latter, it's fine with me, but _assign_bits() makes sense to do
> in this patch as you already change the parameters enough to make it better.
>
only kstrotoul()
> ...
>
> > > > ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> > > > - mask, val);
> > > > + CTRLBANK_AB_BCONF_MODE(id),
> > > > + linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
> > > > if (ret)
> > > > return ret;
>
> ^^^ left for the context.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
* Re: [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic
From: Andy Shevchenko @ 2026-06-02 11:07 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: <CAPVz0n0P7Jk17cM2M1zuHZfySo2=Uibr5izwKU2tqiBpBcg0FQ@mail.gmail.com>
On Tue, Jun 02, 2026 at 01:19:00PM +0300, Svyatoslav Ryhel wrote:
> вт, 2 черв. 2026 р. о 11:09 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > On Mon, Jun 01, 2026 at 06:18:28PM +0300, Svyatoslav Ryhel wrote:
...
> > > if (kstrtoul(buf, 0, &linear))
> > > return -EINVAL;
> >
> > Besides _assign_bits() in the below, side note here to unshadow error codes:
> >
> > ret = kstrtoul(buf, 0, &linear);
> > if (ret)
> > return ret;
> >
> > (obviously in a separate change).
>
> Won't happen in this patches.
You mean both suggestions or you are talking about kstrotoul() only? If it's
only about the latter, it's fine with me, but _assign_bits() makes sense to do
in this patch as you already change the parameters enough to make it better.
...
> > > ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> > > - mask, val);
> > > + CTRLBANK_AB_BCONF_MODE(id),
> > > + linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
> > > if (ret)
> > > return ret;
^^^ left for the context.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
From: Andy Shevchenko @ 2026-06-02 11:05 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: <CAPVz0n21RGAaJc1sda4xyp1h0z+6R6FJ4=XWdOtB1mgtV8=RUA@mail.gmail.com>
On Tue, Jun 02, 2026 at 01:31:44PM +0300, Svyatoslav Ryhel wrote:
> вт, 2 черв. 2026 р. о 11:24 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > On Mon, Jun 01, 2026 at 06:18:25PM +0300, Svyatoslav Ryhel wrote:
...
> > > + device_for_each_child_node_scoped(lm3533->dev, child) {
> >
> > > + if (!fwnode_device_is_available(child))
> > > + continue;
> >
> > Do we need this check?
>
> This is nice to have if the node is disabled. If we assume that there
> are no disabled nodes, I can remove it.
It's already implied. See
static struct fwnode_handle *
of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, struct fwnode_handle *child)
{
return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), to_of_node(child)));
}
And I believe it's written somewhere in the documentation (if not, feel free to
patch that).
...
> > > + ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
> >
> > No way. You should use .dev_groups.
>
> I did not change how driver does this, just swapped lm3533->dev to
> dev. I will set is back as it was.
This is a serious race condition that needs to be addressed. Since you are
touching this driver the fixes against known issues probably are the first
things that have to be done.
> > > + if (ret) {
> > > + dev_err(dev, "failed to create sysfs attributes\n");
> > > goto err_unregister;
> > > }
...
> > Can you think on how to split this change to smaller steps? I believe it's
> > possible.
>
> No, I am done with tinkering with this patchset. It is broken enough
> and it has inflated enough.
Probably you don't want this to be reviewed then? I believe other kernel
developers and maintainers will ask you the same.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
From: Svyatoslav Ryhel @ 2026-06-02 10:31 UTC (permalink / raw)
To: Andy Shevchenko
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: <ah6TGjRNnDpQGO60@ashevche-desk.local>
вт, 2 черв. 2026 р. о 11:24 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Mon, Jun 01, 2026 at 06:18:25PM +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.
>
> ...
>
> > @@ -57,6 +60,9 @@ struct lm3533_als {
> >
> > atomic_t zone;
> > struct mutex thresh_mutex;
> > +
> > + bool pwm_mode;
> > + u32 r_select;
> > };
>
> Have you run `pahole`? Does it agree with the layout you made here?
>
Noted.
> ...
>
> > - als->irq = lm3533->irq;
> > + als->irq = platform_get_irq_optional(pdev, 0);
>
> > +
>
> Redundant blank line.
>
Simplifies code perception, whatever.
> > + if (als->irq == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
>
> What about other error codes when IRQ is found by can't be retrieved for some
> reasons? IIRC we check against ENXIO in similar cases
>
Then we treat it as no IRQ. Original implementation cares only if IRQ
is present or no.
> als->irq = platform_get_irq_optional(pdev, 0);
> if (als->irq == -ENXIO)
> als->irq = 0;
> if (als->irq < 0)
> return als->irq;
>
> ...
>
> > + led->pwm = 0;
>
> Isn't it 0 by zalloc ?
It is, thanks.
>
> > + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
>
> ...
>
> > #define LM3533_BOOST_FREQ_MASK 0x01
> > #define LM3533_BOOST_FREQ_SHIFT 0
> > +#define LM3533_BOOST_FREQ_MIN 500000
> > +#define LM3533_BOOST_FREQ_MAX 1000000
>
> HZ_PER_KHZ (since you included units.h)?
>
500 * HZ_PER_KHZ
1000 * HZ_PER_KHZ
You meant this? Sure.
> ...
>
> > + nchilds = device_get_child_node_count(dev);
> > + if (!nchilds || nchilds > LM3533_CELLS_MAX) {
> > + dev_err(dev, "num of child nodes is not supported\n");
> > + return -ENODEV;
>
> Why not dev_err_probe() here and elsewhere? It looks inconsistent with this
> patch.
>
I must have overlooked it, thanks. WDYM elsewhere, this is the only occurance.
> > }
>
> ...
>
> > + device_for_each_child_node_scoped(lm3533->dev, child) {
>
> > + if (!fwnode_device_is_available(child))
> > + continue;
>
> Do we need this check?
>
This is nice to have if the node is disabled. If we assume that there
are no disabled nodes, I can remove it.
> ...
>
> > + dev_err(dev, "invalid LED node %s\n",
> > + fwnode_get_name(child));
>
> %pfw
>
Noted.
> ...
>
> > + ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
>
> No way. You should use .dev_groups.
>
I did not change how driver does this, just swapped lm3533->dev to
dev. I will set is back as it was.
> > + if (ret) {
> > + dev_err(dev, "failed to create sysfs attributes\n");
> > goto err_unregister;
> > }
>
> ...
>
> Can you think on how to split this change to smaller steps? I believe it's
> possible.
>
No, I am done with tinkering with this patchset. It is broken enough
and it has inflated enough.
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
* Re: [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic
From: Svyatoslav Ryhel @ 2026-06-02 10:19 UTC (permalink / raw)
To: Andy Shevchenko
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: <ah6PxFtoJUWkd79P@ashevche-desk.local>
вт, 2 черв. 2026 р. о 11:09 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Mon, Jun 01, 2026 at 06:18:28PM +0300, Svyatoslav Ryhel wrote:
> > Simplify the sysfs logic of the linear property by switching to a macro
> > and a ternary operator.
>
> ...
>
> > if (kstrtoul(buf, 0, &linear))
> > return -EINVAL;
>
> Besides _assign_bits() in the below, side note here to unshadow error codes:
>
> ret = kstrtoul(buf, 0, &linear);
> if (ret)
> return ret;
>
> (obviously in a separate change).
Won't happen in this patches.
>
> ...
>
> > ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> > - mask, val);
> > + CTRLBANK_AB_BCONF_MODE(id),
> > + linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
> > if (ret)
> > return ret;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
* Re: [PATCH v3 02/11] mfd: lm3533: Remove driver specific regmap wrappers
From: Svyatoslav Ryhel @ 2026-06-02 10:18 UTC (permalink / raw)
To: Andy Shevchenko
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: <ah6O1h8SPwjf3rV1@ashevche-desk.local>
вт, 2 черв. 2026 р. о 11:05 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Mon, Jun 01, 2026 at 06:18:22PM +0300, Svyatoslav Ryhel wrote:
> > Remove driver-specific regmap wrappers in favor of using regmap helpers
> > directly. The wrappers are mostly equivalent to the standard helpers, with
> > two exceptions: regmap_read requires an unsigned int pointer, and
> > regmap_update_bits has the mask and value arguments swapped. These
> > differences were accounted for and adjusted accordingly.
>
> We refer to functions as func(), exempli gratia, regmap_read().
>
Noted.
> ...
>
> > static int lm3533_als_get_current(struct iio_dev *indio_dev, unsigned channel,
> > int *val)
> > {
> > u8 zone;
> > - u8 target;
> > + u32 target;
> > int ret;
>
> While at it, move towards reversed xmas tree order
>
> u32 target;
> u8 zone;
> int ret;
>
Noted.
>
> ...
>
> > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, val, mask);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
> > + val, mask);
>
> It's better to replace this to use _set_bits()/_clear_bits() or even move from
> the above conditional (not in this context) to _assign_bits().
>
I will take a look.
> ...
>
> > else
> > val = 0; /* analog input */
> >
> > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, val, mask);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > + mask, val);
>
> Ditto.
>
> > if (ret) {
> > dev_err(&als->pdev->dev, "failed to set input mode %d\n",
> > pwm_mode);
>
> ...
>
> > /* Make sure interrupts are disabled. */
> > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, 0, mask);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
> > + mask, 0);
>
> _clear_bits().
>
> > if (ret) {
> > dev_err(&als->pdev->dev, "failed to disable interrupts\n");
> > return ret;
>
> ...
>
> > u8 mask = LM3533_ALS_ENABLE_MASK;
> > int ret;
> >
> > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, mask, mask);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > + mask, mask);
>
> _set_bits()
>
> > if (ret)
> > dev_err(&als->pdev->dev, "failed to enable ALS\n");
> >
>
> ...
>
> > u8 mask = LM3533_ALS_ENABLE_MASK;
> > int ret;
> >
> > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, 0, mask);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > + mask, 0);
>
> _clear_bits()
>
> > if (ret)
> > dev_err(&als->pdev->dev, "failed to disable ALS\n");
>
> ...
>
> > else
> > val = 0;
> >
> > - ret = lm3533_update(led->lm3533, LM3533_REG_PATTERN_ENABLE, val, mask);
> > + ret = regmap_update_bits(led->lm3533->regmap,
> > + LM3533_REG_PATTERN_ENABLE, mask, val);
>
> _assign_bits() and so on...
>
> > if (ret) {
> > dev_err(led->cdev.dev, "failed to enable pattern %d (%d)\n",
> > pattern, enable);
>
> ...
>
> > extern int lm3533_ctrlbank_set_brightness(struct lm3533_ctrlbank *cb, u8 val);
> > -extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u8 *val);
> > +extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u32 *val);
>
> We don't need to keep 'extern' for ages.
>
I will no inflate this patchset further
> > extern int lm3533_ctrlbank_set_max_current(struct lm3533_ctrlbank *cb,
> > u16 imax);
> > extern int lm3533_ctrlbank_set_pwm(struct lm3533_ctrlbank *cb, u8 val);
> > -extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u8 *val);
> > -
> > -extern int lm3533_read(struct lm3533 *lm3533, u8 reg, u8 *val);
> > -extern int lm3533_write(struct lm3533 *lm3533, u8 reg, u8 val);
> > -extern int lm3533_update(struct lm3533 *lm3533, u8 reg, u8 val, u8 mask);
> > +extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u32 *val);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
* [PATCH v2] fbdev: omap2: fix use-after-free in omapfb_mmap
From: Hongling Zeng @ 2026-06-02 8:54 UTC (permalink / raw)
To: deller, kees
Cc: linux-omap, linux-fbdev, dri-devel, linux-kernel, zhongling0719,
Hongling Zeng, stable
omapfb_mmap() has a race condition with OMAPFB_SETUP_PLANE ioctl that
can lead to use-after-free:
The fb_mmap() entry point holds mm_lock but not lock (fb_info->lock),
while ioctl handlers like OMAPFB_SETUP_PLANE hold lock but not mm_lock.
This allows concurrent execution.
In omapfb_mmap():
1. rg = omapfb_get_mem_region(ofbi->region); // Get old region ref
2. start = omapfb_get_region_paddr(ofbi); // Read from NEW region
3. len = fix->smem_len; // Read from NEW region
4. vm_iomap_memory(vma, start, len); // Map NEW region memory
5. atomic_inc(&rg->map_count); // Increment OLD region!
Concurrently, OMAPFB_SETUP_PLANE can:
- Reassign ofbi->region = new_rg
- Update fix->smem_len
- OMAPFB_SETUP_MEM then checks NEW region's map_count (0!) and frees it
This leaves userspace with a mapping to freed physical memory.
The fix is to read all required values (start, len) from the same
region reference (rg) that will have its map_count incremented,
preventing the region from being freed while still mapped.
Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
Change in V2:
-Restore fix->smem_len to maintain VRFB sparse mapping.
-Increment map_count before mapping to prevent use-after-free
on driver unload
-Add proper error handling for map_count
---
drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
index d70deb6a9150..046892682fc6 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
@@ -1099,7 +1099,11 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
rg = omapfb_get_mem_region(ofbi->region);
- start = omapfb_get_region_paddr(ofbi);
+ if (ofbi->rotation_type == OMAP_DSS_ROT_VRFB)
+ start = rg->vrfb.paddr[0];
+ else
+ start = rg->paddr;
+
len = fix->smem_len;
DBG("user mmap region start %lx, len %d, off %lx\n", start, len,
@@ -1109,6 +1113,8 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
vma->vm_ops = &mmap_user_ops;
vma->vm_private_data = rg;
+ atomic_inc(&rg->map_count);
+
r = vm_iomap_memory(vma, start, len);
if (r)
goto error;
@@ -1121,6 +1127,7 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
return 0;
error:
+ atomic_dec(&rg->map_count);
omapfb_put_mem_region(rg);
return r;
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
From: Andy Shevchenko @ 2026-06-02 8:23 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: <20260601151831.76350-6-clamor95@gmail.com>
On Mon, Jun 01, 2026 at 06:18:25PM +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.
...
> @@ -57,6 +60,9 @@ struct lm3533_als {
>
> atomic_t zone;
> struct mutex thresh_mutex;
> +
> + bool pwm_mode;
> + u32 r_select;
> };
Have you run `pahole`? Does it agree with the layout you made here?
...
> - als->irq = lm3533->irq;
> + als->irq = platform_get_irq_optional(pdev, 0);
> +
Redundant blank line.
> + if (als->irq == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
What about other error codes when IRQ is found by can't be retrieved for some
reasons? IIRC we check against ENXIO in similar cases
als->irq = platform_get_irq_optional(pdev, 0);
if (als->irq == -ENXIO)
als->irq = 0;
if (als->irq < 0)
return als->irq;
...
> + led->pwm = 0;
Isn't it 0 by zalloc ?
> + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
...
> #define LM3533_BOOST_FREQ_MASK 0x01
> #define LM3533_BOOST_FREQ_SHIFT 0
> +#define LM3533_BOOST_FREQ_MIN 500000
> +#define LM3533_BOOST_FREQ_MAX 1000000
HZ_PER_KHZ (since you included units.h)?
...
> + nchilds = device_get_child_node_count(dev);
> + if (!nchilds || nchilds > LM3533_CELLS_MAX) {
> + dev_err(dev, "num of child nodes is not supported\n");
> + return -ENODEV;
Why not dev_err_probe() here and elsewhere? It looks inconsistent with this
patch.
> }
...
> + device_for_each_child_node_scoped(lm3533->dev, child) {
> + if (!fwnode_device_is_available(child))
> + continue;
Do we need this check?
...
> + dev_err(dev, "invalid LED node %s\n",
> + fwnode_get_name(child));
%pfw
...
> + ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
No way. You should use .dev_groups.
> + if (ret) {
> + dev_err(dev, "failed to create sysfs attributes\n");
> goto err_unregister;
> }
...
Can you think on how to split this change to smaller steps? I believe it's
possible.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic
From: Andy Shevchenko @ 2026-06-02 8:09 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: <20260601151831.76350-9-clamor95@gmail.com>
On Mon, Jun 01, 2026 at 06:18:28PM +0300, Svyatoslav Ryhel wrote:
> Simplify the sysfs logic of the linear property by switching to a macro
> and a ternary operator.
...
> if (kstrtoul(buf, 0, &linear))
> return -EINVAL;
Besides _assign_bits() in the below, side note here to unshadow error codes:
ret = kstrtoul(buf, 0, &linear);
if (ret)
return ret;
(obviously in a separate change).
...
> ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> - mask, val);
> + CTRLBANK_AB_BCONF_MODE(id),
> + linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
> if (ret)
> return ret;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 1/4] lib/fonts: Look up glyph data with font_data_glyph_buf()
From: Thomas Zimmermann @ 2026-06-02 8:08 UTC (permalink / raw)
To: Jocelyn Falempe, javierm, deller, maarten.lankhorst, mripard,
airlied, simona
Cc: dri-devel, linux-fbdev
In-Reply-To: <db9abca9-0174-46fc-8952-aaa23a4e4574@redhat.com>
Hi
Am 01.06.26 um 13:18 schrieb Jocelyn Falempe:
> On 29/05/2026 16:01, Thomas Zimmermann wrote:
>> Add font_data_glyph_buf() to retrieve a character's glyph data or NULL
>> otherwise. Console fonts can currently contain 256 or 512 glyphs. The
>> kernel-internal characters are of type char, unsigned short or unsigned
>> int. Catch all of them by accepting unsigned int. Callers possibly have
>> to cast from signed to unsigned types to reach all glyphs in a font.
>
> Thanks, yes I missed to check font_data_num_glyphs(), and using signed
> index is also problematic.
>
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
Thanks for reviewing. I'll merge the series in a few days if no other
comments come in.
Best regards
Thomas
>
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> include/linux/font.h | 3 +++
>> lib/fonts/fonts.c | 31 +++++++++++++++++++++++++++++++
>> 2 files changed, 34 insertions(+)
>>
>> diff --git a/include/linux/font.h b/include/linux/font.h
>> index 6845f02d739a..ea23b727388b 100644
>> --- a/include/linux/font.h
>> +++ b/include/linux/font.h
>> @@ -101,6 +101,9 @@ font_data_t *font_data_import(const struct
>> console_font *font, unsigned int vpit
>> void font_data_get(font_data_t *fd);
>> bool font_data_put(font_data_t *fd);
>> unsigned int font_data_size(font_data_t *fd);
>> +const unsigned char *font_data_glyph_buf(font_data_t *fd,
>> + unsigned int width, unsigned int vpitch,
>> + unsigned int c);
>> bool font_data_is_equal(font_data_t *lhs, font_data_t *rhs);
>> int font_data_export(font_data_t *fd, struct console_font *font,
>> unsigned int vpitch);
>> diff --git a/lib/fonts/fonts.c b/lib/fonts/fonts.c
>> index f5d5333450a0..4fc66722d00d 100644
>> --- a/lib/fonts/fonts.c
>> +++ b/lib/fonts/fonts.c
>> @@ -178,6 +178,37 @@ unsigned int font_data_size(font_data_t *fd)
>> }
>> EXPORT_SYMBOL_GPL(font_data_size);
>> +static unsigned int font_data_num_glyphs(font_data_t *fd, unsigned
>> int width, unsigned int height)
>> +{
>> + return font_data_size(fd) / font_glyph_size(width, height);
>> +}
>> +
>> +/**
>> + * font_data_glyph_buf() - Returns the glyph for a specific
>> character as raw bytes
>> + * @fd: The font data
>> + * @width: The glyph width in bits per scanline
>> + * @vpitch: The number of scanlines per glyph
>> + * @c: The character
>> + *
>> + * Glyphs start at fixed intervals within the font data.
>> font_data_glyph_buf()
>> + * returns the glyph shape of the specified character. If no such glyph
>> + * exists in the font, it returns NULL.
>> + *
>> + * Returns:
>> + * The character's raw glyph shape, or NULL if no glyph exists for
>> the character. The
>> + * provided buffer is read-only.
>> + */
>> +const unsigned char *font_data_glyph_buf(font_data_t *fd,
>> + unsigned int width, unsigned int vpitch,
>> + unsigned int c)
>> +{
>> + if (c >= font_data_num_glyphs(fd, width, vpitch))
>> + return NULL;
>> +
>> + return font_data_buf(fd) + font_glyph_size(width, vpitch) * c;
>> +}
>> +EXPORT_SYMBOL_GPL(font_data_glyph_buf);
>> +
>> /**
>> * font_data_is_equal - Compares font data for equality
>> * @lhs: Left-hand side font data
>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
^ permalink raw reply
* Re: [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers
From: Andy Shevchenko @ 2026-06-02 8:07 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: <20260601151831.76350-4-clamor95@gmail.com>
On Mon, Jun 01, 2026 at 06:18:23PM +0300, Svyatoslav Ryhel wrote:
> The lm3533_als_set_input_mode and lm3533_als_set_resistor functions are
> used only in lm3533_als_setup. Incorporate their code into
> lm3533_als_setup directly to simplify driver readability.
Same comments as per previous patch.
I stop my review here as it seems may be more of this.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 02/11] mfd: lm3533: Remove driver specific regmap wrappers
From: Andy Shevchenko @ 2026-06-02 8:05 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: <20260601151831.76350-3-clamor95@gmail.com>
On Mon, Jun 01, 2026 at 06:18:22PM +0300, Svyatoslav Ryhel wrote:
> Remove driver-specific regmap wrappers in favor of using regmap helpers
> directly. The wrappers are mostly equivalent to the standard helpers, with
> two exceptions: regmap_read requires an unsigned int pointer, and
> regmap_update_bits has the mask and value arguments swapped. These
> differences were accounted for and adjusted accordingly.
We refer to functions as func(), exempli gratia, regmap_read().
...
> static int lm3533_als_get_current(struct iio_dev *indio_dev, unsigned channel,
> int *val)
> {
> u8 zone;
> - u8 target;
> + u32 target;
> int ret;
While at it, move towards reversed xmas tree order
u32 target;
u8 zone;
int ret;
...
> - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, val, mask);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
> + val, mask);
It's better to replace this to use _set_bits()/_clear_bits() or even move from
the above conditional (not in this context) to _assign_bits().
...
> else
> val = 0; /* analog input */
>
> - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, val, mask);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> + mask, val);
Ditto.
> if (ret) {
> dev_err(&als->pdev->dev, "failed to set input mode %d\n",
> pwm_mode);
...
> /* Make sure interrupts are disabled. */
> - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, 0, mask);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
> + mask, 0);
_clear_bits().
> if (ret) {
> dev_err(&als->pdev->dev, "failed to disable interrupts\n");
> return ret;
...
> u8 mask = LM3533_ALS_ENABLE_MASK;
> int ret;
>
> - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, mask, mask);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> + mask, mask);
_set_bits()
> if (ret)
> dev_err(&als->pdev->dev, "failed to enable ALS\n");
>
...
> u8 mask = LM3533_ALS_ENABLE_MASK;
> int ret;
>
> - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, 0, mask);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> + mask, 0);
_clear_bits()
> if (ret)
> dev_err(&als->pdev->dev, "failed to disable ALS\n");
...
> else
> val = 0;
>
> - ret = lm3533_update(led->lm3533, LM3533_REG_PATTERN_ENABLE, val, mask);
> + ret = regmap_update_bits(led->lm3533->regmap,
> + LM3533_REG_PATTERN_ENABLE, mask, val);
_assign_bits() and so on...
> if (ret) {
> dev_err(led->cdev.dev, "failed to enable pattern %d (%d)\n",
> pattern, enable);
...
> extern int lm3533_ctrlbank_set_brightness(struct lm3533_ctrlbank *cb, u8 val);
> -extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u8 *val);
> +extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u32 *val);
We don't need to keep 'extern' for ages.
> extern int lm3533_ctrlbank_set_max_current(struct lm3533_ctrlbank *cb,
> u16 imax);
> extern int lm3533_ctrlbank_set_pwm(struct lm3533_ctrlbank *cb, u8 val);
> -extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u8 *val);
> -
> -extern int lm3533_read(struct lm3533 *lm3533, u8 reg, u8 *val);
> -extern int lm3533_write(struct lm3533 *lm3533, u8 reg, u8 val);
> -extern int lm3533_update(struct lm3533 *lm3533, u8 reg, u8 val, u8 mask);
> +extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u32 *val);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH] fbdev: omap2: fix use-after-free in omapfb_mmap
From: Hongling Zeng @ 2026-06-02 8:04 UTC (permalink / raw)
To: deller, kees
Cc: linux-omap, linux-fbdev, dri-devel, linux-kernel, zhongling0719,
Hongling Zeng, stable
omapfb_mmap() has a race condition with OMAPFB_SETUP_PLANE ioctl that
can lead to use-after-free:
The fb_mmap() entry point holds mm_lock but not lock (fb_info->lock),
while ioctl handlers like OMAPFB_SETUP_PLANE hold lock but not mm_lock.
This allows concurrent execution.
In omapfb_mmap():
1. rg = omapfb_get_mem_region(ofbi->region); // Get old region ref
2. start = omapfb_get_region_paddr(ofbi); // Read from NEW region
3. len = fix->smem_len; // Read from NEW region
4. vm_iomap_memory(vma, start, len); // Map NEW region memory
5. atomic_inc(&rg->map_count); // Increment OLD region!
Concurrently, OMAPFB_SETUP_PLANE can:
- Reassign ofbi->region = new_rg
- Update fix->smem_len
- OMAPFB_SETUP_MEM then checks NEW region's map_count (0!) and frees it
This leaves userspace with a mapping to freed physical memory.
The fix is to read all required values (start, len) from the same
region reference (rg) that will have its map_count incremented,
preventing the region from being freed while still mapped.
Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
index d70deb6a9150..853bd55621ec 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
@@ -1099,8 +1099,11 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
rg = omapfb_get_mem_region(ofbi->region);
- start = omapfb_get_region_paddr(ofbi);
- len = fix->smem_len;
+ if (ofbi->rotation_type == OMAP_DSS_ROT_VRFB)
+ start = rg->vrfb.paddr[0];
+ else
+ start = rg->paddr;
+ len = rg->size;
DBG("user mmap region start %lx, len %d, off %lx\n", start, len,
vma->vm_pgoff << PAGE_SHIFT);
--
2.25.1
^ permalink raw reply related
* [PATCH] fbdev: grvga: Fix CLUT register address offset in comment
From: Eduardo Silva @ 2026-06-01 19:46 UTC (permalink / raw)
To: deller; +Cc: linux-fbdev, Eduardo Silva
The comment does not match the actual address offset. According
to the GRLIB IP Library Reference Manual (p. 2119), the CLUT register
is at offset 0x28, not the value stated in the comment.
Signed-off-by: Eduardo Silva <eduardo4silva@gmail.com>
---
drivers/video/fbdev/grvga.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/grvga.c b/drivers/video/fbdev/grvga.c
index de8ab817d406..a6594bcd74e8 100644
--- a/drivers/video/fbdev/grvga.c
+++ b/drivers/video/fbdev/grvga.c
@@ -33,7 +33,7 @@ struct grvga_regs {
u32 line_length; /* 0x10 */
u32 fb_pos; /* 0x14 */
u32 clk_vector[4]; /* 0x18 */
- u32 clut; /* 0x20 */
+ u32 clut; /* 0x28 */
};
struct grvga_par {
--
2.54.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox