On Mon Dec 23, 2024 at 12:29 PM CET, Jonathan Cameron wrote:
> On Fri, 20 Dec 2024 20:28:29 +0100
> Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:
>
> > The current scale is not ABI-compliant as it is just the sensor gain
> > instead of the value that acts as a multiplier to be applied to the raw
> > value (there is no offset).
> >
> > Use the iio-gts helpers to obtain the proper scale values according to
> > the gain and integration time to match the resolution tables from the
> > datasheet.
> >
> > Fixes: c5a23f80c164 ("iio: light: add support for veml3235")
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>
>
> Hi Javier,
>
> A few non fix related changes that make no functional difference made
> it in here. Those should be done in a additional patch after this one
> (to make the backport more minimal).
>
> This change is large enough I probably won't directly apply it as a fix anyway.
> Most likely it's material for the next merge window that will then get
> backported after it is upstream. That will give a small window in which
> the broken code is in a release kernel, but it should hit stable at .2/.3 or
> so and no one sane builds product before that point!
>
> Thanks,
>
> Jonathan
>
I am fine with applying this patch later if you think it makes more
sense. A couple of diffs will go away after dropping the code style
issues and the 'val' check, but it will still be a bit over 200 lines.
I am planning to fix the old veml6030 with a similar approach
after the iio-gts helpers become available. That code has been broken
from the beginning (around 5 years ago), and it did not seem to bother
anyone, so in that case it will make even more sense to apply it in the
merge window.
> > - ret = regmap_field_write(data->rf.gain, new_gain);
> > - if (ret) {
> > - dev_err(data->dev, "failed to set gain: %d\n", ret);
> > + ret = iio_gts_find_gain_sel_in_times(&data->gts, val, val2, &gain_sel,
> > + &time_sel);
> > + if (ret)
> > return ret;
> > +
> > + if (it_idx != time_sel) {
>
> Not part of this series, but might be worth turning on regcache for this driver.
> Then you can do this sort of write unconditionally as it will hit in the cache
> and do nothing anyway. Mind you, this isn't a high performance path, so maybe
> just write it anyway.
>
That makes sense, I will add a follow-up patch to use regcache. Even
though it is not a high performance path, doing it right will not cost
much more effort.
> > @@ -309,9 +323,12 @@ static int veml3235_write_raw(struct iio_dev *indio_dev,
> > {
> > switch (mask) {
> > case IIO_CHAN_INFO_INT_TIME:
> > - return veml3235_set_it(indio_dev, val, val2);
> > + if (val)
> > + return -EINVAL;
>
> This yanking of the test out of the set function is fine, but maybe
> as a precursor patch so as to reduce noise in the main change.
>
> I'm not sure it's technically necessary either. More of a sensible
> cleanup?
>
Actually, it could just stay as it was before by checking val
internally, which drops this diff. I will do that for v2.
> > +
> > + return veml3235_set_it(indio_dev, val2);
> > case IIO_CHAN_INFO_SCALE:
> > - return veml3235_set_gain(indio_dev, val, val2);
> > + return veml3235_set_scale(indio_dev, val, val2);
> > static const struct iio_info veml3235_info = {
> > - .read_raw = veml3235_read_raw,
> > - .read_avail = veml3235_read_avail,
> > + .read_raw = veml3235_read_raw,
> > + .read_avail = veml3235_read_avail,
>
> Whilst it would be good to fix that indent, doesn't belong in this patch
> as it means the reader has to check very carefully that there are no subtle
> changes in this line. Feel free to send a follow up white space cleanup
> patch that clearly states it makes not functional changes though.
>
I will move the code style fixes to another patch. I was a bit too lazy
here :)
Thanks for your feedback and best regards,
Javier Carrasco
