* Re: [PATCH v4 05/14] iio: light: lm3533-als: Remove redundant pdata helpers
From: Andy Shevchenko @ 2026-06-09 19:10 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
In-Reply-To: <20260606045738.21050-6-clamor95@gmail.com>
On Sat, Jun 06, 2026 at 07:57:29AM +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.
Use func() when referring to a function in the commit message.
...
> 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_assign_bits(als->regmap, LM3533_REG_ALS_CONF,
> + LM3533_ALS_INPUT_MODE_MASK, pdata->pwm_mode);
> 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)
> + return dev_err_probe(dev, -EINVAL,
> + "invalid resistor value\n");
> +
> + ret = regmap_write(als->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;
Wondering if it would be better to
/* Bail out when in PWM-mode */
if (pdata->pwm_mode)
return 0;
/* ALS input is always high impedance in PWM-mode. */
...
as the above changes almost every line in that conditional.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 07/14] mfd: lm3533: Switch sysfs_create_group() to device_add_group()
From: Andy Shevchenko @ 2026-06-09 19:13 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: <20260606045738.21050-8-clamor95@gmail.com>
On Sat, Jun 06, 2026 at 07:57:31AM +0300, Svyatoslav Ryhel wrote:
> Switch from sysfs_create_group() to device_add_group() including device
> managed where appropriate.
This should use .dev_groups member of struct device_driver.
...
> + ret = devm_device_add_group(&bd->dev, &lm3533_bl_attribute_group);
This will make Greg KH very grumpy. (For the record, original code as well
but it already is in upstream. So, thanks for trying to address this, just
needs a bit more of work.)
> + if (ret < 0)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to create sysfs attributes\n");
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 10/14] mfd: lm3533: Set DMA mask
From: Andy Shevchenko @ 2026-06-09 19:17 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: <20260606045738.21050-11-clamor95@gmail.com>
On Sat, Jun 06, 2026 at 07:57:34AM +0300, Svyatoslav Ryhel wrote:
> Missing coherent_dma_mask assigning triggers the following warning in
> dmesg:
>
> [ 3.287872] platform lm3533-backlight.0: DMA mask not set
>
> Since this warning might be elevated to an error in the future, set
> coherent_dma_mask to zero because both the core and cells do not utilize
> DMA.
Hmm... I am not sure about this. The entire kernel has only two drivers that
do that, and thanks to their commit messages one of them pointed out to the
commit from 2018. So, if no other devices suffer from this, I think it has to
be a better way of achieving the same.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 14/14] video: leds: backlight: lm3533: Support getting LED sources from DT
From: Andy Shevchenko @ 2026-06-09 19: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: <20260606045738.21050-15-clamor95@gmail.com>
On Sat, Jun 06, 2026 at 07:57:38AM +0300, Svyatoslav Ryhel wrote:
> Add Control Bank to HVLED/LVLED muxing support based on the led-sources
> defined in the device tree.
...
> static int lm3533_led_setup(struct lm3533_led *led)
> {
> - int ret;
> + u32 output_cfg_shift = 0;
No need to assign the default to this.
> + u32 output_cfg_val = 0;
> + u32 output_cfg_mask = 0;
> + int ret, i;
No need to add 'i'.
> + if (led->num_leds) {
> + for (i = 0; i < led->num_leds; i++) {
for (unsigned int i = 0; i < led->num_leds; i++) {
> + if (led->leds[i] >= LM3533_LVCTRLBANK_MAX)
> + continue;
> +
> + output_cfg_shift = led->leds[i] * 2;
> + output_cfg_val |= led->id << output_cfg_shift;
> + output_cfg_mask |= OUTPUT_LVLED_MASK << output_cfg_shift;
> + }
> +
> + /* LVLED1, LVLED2 and LVLED3 */
> + ret = regmap_update_bits(led->regmap, LM3533_REG_OUTPUT_CONF1,
> + output_cfg_mask << OUTPUT_CONF1_SHIFT,
> + output_cfg_val << OUTPUT_CONF1_SHIFT);
> + if (ret)
> + return ret;
> +
> + /* LVLED4 and LVLED5 */
> + ret = regmap_update_bits(led->regmap, LM3533_REG_OUTPUT_CONF2,
> + output_cfg_mask >> OUTPUT_CONF2_SHIFT,
> + output_cfg_val >> OUTPUT_CONF2_SHIFT);
> + if (ret)
> + return ret;
> + }
...
> + if (led->num_leds > 0) {
> + ret = device_property_read_u32_array(&pdev->dev, "led-sources",
> + led->leds, led->num_leds);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to get led-sources\n");
> + goto err_deregister;
> + }
> + }
This and other pieces may benefit from local variable
struct device *dev = &pdev->dev;
defined at the top of the function.
...
> static int lm3533_bl_setup(struct lm3533_bl *bl)
As per above.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
page: | 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