Re: [PATCH] iio: light: ltr390: Add configurable gain, resolution and ALS reading

[Jonathan Cameron]

Please crop replies to only leave the section being discussed.
It saves time for everyone reading the thread.

> > > -static const struct iio_info ltr390_info = {
> > > -     .read_raw = ltr390_read_raw,
> > > +/* integration time in us */
> > > +static const int ltr390_int_time_map_us[] = {400000, 200000, 100000, 50000, 25000, 12500};
> > > +static const int ltr390_gain_map[] = {1, 3, 6, 9, 18};
> > > +
> > > +static IIO_CONST_ATTR_INT_TIME_AVAIL("400000 200000 100000 50000 25000 12500");  
> > Please use read_avail() callback and the appropriate mask to provide this.
> > That enables it to be used from in kernel consumers and enforces the
> > ABI without a reviewer having to check what you have aligns.
> >  
> > > +static IIO_CONST_ATTR(gain_available, "1 3 6 9 18");  
> > Given we don't have a 'gain' control, what is the available applying to?
> >  
> The gain gets controlled by writing to the iio_info_scale attribute,
> we write one of the above available values.
> So that we can scale the raw ALS and UVI values. I could use
> read_avail() for this too for the IIO_INFO_SCALE channel. Should I do
> that?

Yes, it would be appropriate to provide read_avail for IIO_INFO_SCALE
as that is standard ABI that userspace will have way to interpret.

> Can you elaborate more on your comment?

Basic rule of thumb is think very hard about whether there is an alternative
if you are providing attributes directly to an IIO driver.
There are a few corners where that is necessary for standard ABI
around FIFOs or certain event related attributes + a few special
corners for complex hardwware.

None of those apply here, so read_avail callback and choosing standard
ABI elements to match what you are trying to control / describe is the
way to go.  That's the stuff that userspace tooling knows how to use.

Jonathan