On Sun, 19 Nov 2023 18:40:16 +0100
Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:
> On 19.11.23 16:02, Jonathan Cameron wrote:
> >> +
> >> +struct veml6075_data {
> >> + struct i2c_client *client;
> >> + struct regmap *regmap;
> >> + struct mutex lock; /* register access lock */
> >
> > regmap provides register locking as typically does the bus lock, so good to
> > say exactly what you mean here. Is there a Read Modify Write cycle you need
> > to protect for instance, or consistency across multiple register accesses?
> >
> What I want to avoid with this lock is an access to the measurement
> trigger or an integration time modification from different places while
> there is a measurement reading going on. "register access lock" is
> probably not the best name I could have chosen though.
>
> I was not aware of that guard(mutex) mechanism. I guess it is new
> because only one driver uses it in the iio subsystem (up to v6.7-rc1).
> I will have a look at it.
Yup. It is very new.
> >> +};
> >
> >> +
> >> +static const struct iio_chan_spec veml6075_channels[] = {
> >> + {
> >> + .type = IIO_INTENSITY,
> >> + .channel = CH_UVA,
> >> + .modified = 1,
> >> + .channel2 = IIO_MOD_LIGHT_UV,
> >> + .extend_name = "UVA",
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >> + BIT(IIO_CHAN_INFO_SCALE),
> >> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> >> + },
> >> + {
> >> + .type = IIO_INTENSITY,
> >> + .channel = CH_UVB,
> >> + .modified = 1,
> >> + .channel2 = IIO_MOD_LIGHT_UV,
> >> + .extend_name = "UVB",
> >
> > Extent name is very rarely used any more. It's a horrible userspace interface
> > and an old design mistake.
> > Instead we use the channel label infrastructure. Provide the read_label()
> > callback to use that instead.
> >
> > I'm not sure this is a great solution here though. For some similar cases
> > such as visible light colours we've just added additional modifiers, but that
> > doesn't really scale to lots of sensitive ranges.
> >
> > One thing we have talked about in the past, but I don't think we have done in
> > a driver yet, is to provide actual characteristics of the sensitivity graph.
> > Perhaps just a wavelength of maximum sensitivity?
> >
> > Visible light sensors often have hideous sensitivity curves, including sometimes
> > have multiple peaks, but in this case they look pretty good.
> > Do you think such an ABI would be more useful than A, B labelling?
> >
> My first idea was adding new modifiers because I saw that
> IIO_MOD_LIGHT_UV and IIO_MOD_LIGHT_DUV coexist, but then I thought _UVA
> and _UVB might not be used very often (wrong assumption?) and opted for
> a local solution with extended names. But any cleaner solution would be
> welcome because the label attributes are redundant.
>
> Maybe adding UV-A, UV-B and UV-C modifiers is not a big deal as these
> are fairly common bands. Actually DUV is pretty much UV-C and could be
> left as it is.
Ok. Add UV-A and UV-B as that's inline with other cases.
Always a guessing game for how often a modifier will turn up. We have
space and the list isn't growing that fast so should be fine.
>
> This sensor has a single peak per channel, but I do not know how I would
> provide that information to the core if that is better than adding UVA
> and UVB bands. Would that add attributes to sysfs for the wavelengths or
> extend the channel name? In that case two new modifiers might be a
> better and more obvious solution.
Would be attributes if we did add max sensitivity wavelengths.
Might be worth a revisit at somepoint, but not feeling like it's necessary
for this driver.
> > Jonathan
> >
> >
> I will work on the other issues you pointed out. Thanks a lot for your
> review.
>
> Best regards,
> Javier Carrasco
>
>
