Re: [PATCH v8 5/5] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>]

On 4/3/24 01:44, Jonathan Cameron wrote:
> On Wed, 28 Feb 2024 22:54:08 +1030
> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
> 
>> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor.
>> It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor)
>> channel approximates the response of the human-eye providing direct
>> read out where the output count is proportional to ambient light levels.
>> It is internally temperature compensated and rejects 50Hz and 60Hz flicker
>> caused by artificial light sources. Hardware interrupt configuration is
>> optional. It is a low power device with 20 bit resolution and has
>> configurable adaptive interrupt mode and interrupt persistence mode.
>> The device also features inbuilt hardware gain, multiple integration time
>> selection options and sampling frequency selection options.
>>
>> This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for
>> Scales, Gains and Integration time implementation.
>>
>> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
>> ---
>> v7 -> v8:
>>   - Renamed APDS9306_INT_CH_CLEAR to APDS9306_INT_SRC_CLEAR macro for higher
>>     readability
>>   - Removed APDS9306_CHANNEL macro for higher readability
>>   - Updated iio_push_event() functions with correct type of events (Light or Intensity)
> 
> Partly right.  Need to push the modified part for the intensity channel.
> The event should match the channel description.
> 
> I also noted some missing elements in the event specs (sorry missed those
> before!).  Whilst what you have will work, that's just because the error checking
> is relaxed in the IIO core and we don't complain if they aren't fully specified.
> What you have creates the correct attributes, but that's a side effect of how
> we use the data, not what data should be provided.
> 
> Thanks,
> 
> Jonathan
> 
>>   - Updated variable name "event_ch_is_light" to "int_src" and change as per
>>     review to fix compiler warning
>>   - Used scope for guard() functions
>>   - Other fixes as per reviews
>>     https://lore.kernel.org/all/20240224151340.3f2f51e8@jic23-huawei/
>>     https://lore.kernel.org/all/ZdycR6nr3rtrnuth@smile.fi.intel.com/
>>
> 
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index 2e5fdb33e0e9..a30f906e91ba 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -10,6 +10,7 @@ obj-$(CONFIG_ADUX1020)		+= adux1020.o
> ...
> 
>> +	GAIN_SCALE_ITIME_US(3125, APDS9306_MEAS_MODE_3125US, BIT(0)),
>> +};
>> +
>> +static struct iio_event_spec apds9306_event_spec_als[] = {
>> +	{
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.dir = IIO_EV_DIR_RISING,
>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
>> +	}, {
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.dir = IIO_EV_DIR_FALLING,
>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
>> +	}, {
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.dir = IIO_EV_DIR_EITHER,
>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
>> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
>> +	}, {
>> +		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
>> +			BIT(IIO_EV_INFO_ENABLE),
>> +	},
>> +};
>> +
>> +static struct iio_event_spec apds9306_event_spec_clear[] = {
>> +	{
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.dir = IIO_EV_DIR_EITHER,
>> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> 
> Can't configure the threshold for this channel?
Same threshold regs for both als and clear channels.
> 
> Whilst the IIO core doesn't check these for missing entries in
> shared attributes, you driver should replicate the parts that
> are in mask_shared_by_all above.  The code that builds the attributes
> expects duplication of entries so they are here to provide an easy
> place for us to visually check what is supported.
I understand this approach now.
> 
> I think that means this event spec will be identical to that for the
> als channel. So reuse that.
Yes, correct.
I tried using struct iio_event_spec apds9306_event_spec_als[] for both light
and clear channels and the ABI is identical to the previous version.
> 
> Let us know if you copied this pattern from another driver as we
> should fix any that have gotten through review doing this.
Initially I referenced many drivers but after so many iterations it
does not resemble anything that I have looked at previously.

Thank you for reviewing.

Regards,
Subhajit Ghosh

> 
>> +	},
>> +};
>> +
>> +static struct iio_chan_spec apds9306_channels_with_events[] = {
>> +	{
>> +		.type = IIO_LIGHT,
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +					   BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +						     BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +				      BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
>> +		.event_spec = apds9306_event_spec_als,
>> +		.num_event_specs = ARRAY_SIZE(apds9306_event_spec_als),
>> +	}, {
>> +		.type = IIO_INTENSITY,
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +					   BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +						     BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.channel2 = IIO_MOD_LIGHT_CLEAR,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +		.modified = 1,
>> +		.event_spec = apds9306_event_spec_clear,
>> +		.num_event_specs = ARRAY_SIZE(apds9306_event_spec_clear),
>> +	},
>> +};
>> +
>> +static struct iio_chan_spec apds9306_channels_without_events[] = {
>> +	{
>> +		.type = IIO_LIGHT,
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +					   BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +						     BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +				      BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
>> +	}, {
>> +		.type = IIO_INTENSITY,
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +					   BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
>> +						     BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.channel2 = IIO_MOD_LIGHT_CLEAR,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +		.modified = 1,
>> +	},
>> +};
> 
>