Re: [PATCH v7 2/2] iio: light: add AS7343 multi-spectral sensor driver

["Jose A. Perez de Azpillaga" <azpijr@gmail.com>]

On 10/29/26 5:38 PM, Chang Yu wrote:
> On Thu, Oct 29, 2026 at 12:20:24PM +0100, Jose A. Perez de Azpillaga wrote:
>> On 10/29/26 8:48 AM, Chang Yu wrote:
>>> Add a driver for the AMS AS7343 14-channel multi-spectral sensor.
>>>
>>> The AS7343 is a 14-channel spectral sensor featuring 11 visible
>>> channels, 1 near-infrared channel, 1 clear channel (VIS), and 1
>>> flicker detection channel.
>>>
>>> The driver exposes 12 spectral channels (11 visible light and 1
>>> near-infrared) via sysfs. Runtime PM is implemented to stop
>>> measurements and enter SLEEP state when the device is suspended
>>> or torn down.
>>>
>>> Future patches will add configurable gain and integration time,
>>> interrupt support, buffered reads, VIS channel, and flicker
>>> detection.
>>>
>>> Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
>>> Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>>> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
>>
>>
>> [snip]
>>
>>> +		/* Check for digital saturation. */
>>> +		if (status & AS7343_ASTATUS_ASAT_STATUS) {
>>> +			/*
>>> +			 * Per the datasheet, ASTATUS is self-clearing: writing
>>> +			 * the value back clears the entire register.
>>> +			 */
>>> +			ret = regmap_write(map, AS7343_ASTATUS_REG, status);
>>
>>
>> isn't that the STATUS (0x93) text? the datasheet says ASTATUS is read
>> only and ASAT_STATUS clears on access, so the write shouldn't be needed.
>> it also covers analog saturation, not only digital
>>
> Aw man, how did I not notice that?

easy to mix up, STATUS and ASTATUS are one letter apart :)

> 
>>> +			return ret ? ret : -EIO;
>>
>> -EIO reads like a bus error. as73211 returns -EOVERFLOW for this,
>> wouldn't that fit better?
> 
> Sounds good to me. Jonathan, if v7 goes in, can you replace this if
> block with:
> 
> 		/* Check for saturation. */
> 		if (status & AS7343_ASTATUS_ASAT_STATUS)
> 			return -EOVERFLOW;
> 
> I've tested the change on hardware and everything works fine. Or do you
> prefer me sending over a v8 instead?

with that fixup, LGTM so:

Reviewed-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>

-- 
cheers,
jose a. p-a