Re: [PATCH v3 2/2] iio: light: veml3328: add support for new device

[Joshua Crofts]

On Sun, 31 May 2026 at 11:19, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Sat, 30 May 2026 19:06:47 +0200
> Joshua Crofts <joshua.crofts1@xxxxxxxxx> wrote:
>
> > Add support for the Vishay VEML3328 RGB/IR light sensor communicating
> > via I2C (SMBus compatible).
> >
> > Also add a new entry for said driver into Kconfig and Makefile.
> >
> > Assisted-by: Gemini:3.1-Pro
> > Signed-off-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> Hi Joshua,
>
> I didn't find anything extras so this is just some more explanation for
> the Sashiko comments.
>
> Give we are approaching the end of the cycle, feel free to do a new version
> quicker than normal if you are sure on what needs changing.

Considering these are relatively simple fixes, I'll probably send a v4 today
(and hope that it will be the final iteration).

> > +     case IIO_CHAN_INFO_SCALE:
>
> Under the hood, like all the cleanup.h magic, this instantiates a local
> variable - the cleanup scope is tided to that and scope for switch statements
> is a funny thing - it's not per case, but rather the whole switch.
> Hence need to add {} to define the scope for this case block.
>
> Otherwise you enter this switch via another path and the local variable
> is unassigned, but the __free() that cleans it up is set. Hence you get
> a use of undefined variable.

Okay, thanks for the clarification.

> > +     case IIO_CHAN_INFO_SCALE:
> > +             ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
>
> Hmm. The comment made by sashiko on this is also possibly correct.
> The write to INT_TIME can race with the write to INFO_SCALE.  Whilst that's
> not a normal use model (one thread would typically set up all the parameters)
> you could end up with picking a scale based on an int time that isn't the current
> one. I'd just throw a guard(mutex)(&data->lock) above this whole switch so we don't have
> to think about it. (and a mutex in data, plus initialization etc).

Heh, my first iteration used guard, so we've come full circle :)

> > +
> > +     pm_runtime_set_active(dev);
> > +     pm_runtime_set_autosuspend_delay(dev, 2000);
> > +     pm_runtime_use_autosuspend(dev);
>
> As per other thread check this results in power down.
> If it doesn't, try moving autosupend calls after devm_pm_runtime_enable()
> and see if that is enough.  Otherwise, you can either manipulate the counters
> or I think just call pm_runtime_idle() to force it off immediately.

Sure, that seems reasonable. Testing pm_runtime stuff on a Pi 4 is a bit more
painful though, as the LTS kernel doesn't support the PM_RUNTIME_ACQUIRE
macros - they were introduced in kernel version 6.19 (which I found out the hard
way!)

Thanks

-- 
Kind regards

CJD