Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <588dd3f4-bea5-4453-9ef6-f92fb42c7514@gmail.com>
Date:   Sun, 19 Nov 2023 18:40:16 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] iio: light: add VEML6075 UVA and UVB light sensor
 driver
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org
References: <20231110-veml6075-v1-0-354b3245e14a@gmail.com>
 <20231110-veml6075-v1-1-354b3245e14a@gmail.com>
 <20231119150233.10fdc66e@jic23-huawei>
Content-Language: en-US
From:   Javier Carrasco <javier.carrasco.cruz@gmail.com>
In-Reply-To: <20231119150233.10fdc66e@jic23-huawei>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.
>> +};
> 
>> +
>> +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.

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.
> Jonathan
> 
> 
I will work on the other issues you pointed out. Thanks a lot for your
review.

Best regards,
Javier Carrasco