Re: [PATCH 3/3] iio: light: opt4001: add support for opt4041

[Jesper X Nilsson <jesperni@axis.com>]

On 10/23/26 11:14, Andy Shevchenko wrote:
> On Fri, Oct 23, 2026 at 09:48:15AM +0200, Jesper X Nilsson wrote:
>> Extend the opt4001 driver to also support OPT4041. The devices are very
>> similar, with the difference being OPT4041 having two channels, one
>> visible and one wide band channel ranging from 380nm to 1000nm.
>>
>> The register map is almost the same. OPT4001 has registers to read out
>> the last four values sampled. OPT4041 maps to the same registers, but
>> has two channels, saving the last two samples for each channel.
> 
> ...
> 
>> Assisted-by: LLM
> 
> Drop this.
> 
I will drop the tag in v2.

> ...
> 
>>   	help
>>   	  If you say Y or M here, you get support for Texas Instruments
>> -	  OPT4001 Ambient Light Sensor.
>> +	  OPT4001 and OPT4041 Ambient Light Sensors.
> 
> We in IIO established kinda a style for this to be scalable, id est list them
> one-per-line:
> 
> 	  If you say Y or M here, you get support for Texas Instruments
> 	  Ambient Light Sensors:
> 	  - OPT4001 (...can add some short useful detail here...)
> 	  - OPT4041 (...can add some short useful detail here...)
> 
I will change to this style in v2.

>>   	  If built as a dynamically linked module, it will be called
>>   	  opt4001.
> 
> Perhaps also consider changing summary text at tristate to make it like
> "...OPT4001 and compatible".
> 
Good suggestion, will add it in v2.

> ...
> 
>> - * Device driver for the Texas Instruments OPT4001.
>> + * Device driver for the Texas Instruments OPT4001 and OPT4041.
> 
> Make this and MODULE_DESCRIPTION() to be the same as summary in the Kconfig,
> see above.
> 
I will update the MODULE_DESCRIPTION() in v2.

>>    */
> 
> ...
> 
>> +static const struct iio_chan_spec opt4041_channels[] = {
>> +	{
>> +		.type = IIO_LIGHT,
>> +		.address = OPT4001_LIGHT1_MSB,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME)
> 
> Trailing comma.
> 
>> +	},
>> +	{
>> +		.type = IIO_INTENSITY,
>> +		.modified = 1,
>> +		.channel2 = IIO_MOD_LIGHT_BOTH,
>> +		.address = OPT4041_LIGHT2_MSB,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME)
> 
> Ditto.
> 
>> +	},
>> +};
> 
> ...
> 
>> +static const struct opt4001_chip_info opt4041_sot_5x3_info = {
>> +	.mul = 5850,
>> +	.div = 10000000,
> 
> Just sent a patch to move this to use struct s32_fract, I believe you may
> incorporate it into your series as a precursor).
> 
I will add the patch to my series.

>> +	.name = "opt4041-sot-5x3",
>> +	.channels = opt4041_channels,
>> +	.num_channels = ARRAY_SIZE(opt4041_channels),
>> +	.device_id = OPT4041_DEVICE_ID_VAL,
>> +};
> 
> ...
> 
>> +MODULE_DESCRIPTION("Texas Instruments opt4001 and opt4041 ambient light sensor driver");
> 
> See above.
> 

Thanks for the fast review!

Best regards,
Jesper