Re: [PATCH 1/2] iio: light: add VEML6075 UVA and UVB light sensor driver

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 19 Nov 2023 18:40:16 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> 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
> 
>