Received-SPF: pass (google.com: domain of linux-kernel+bounces-90590-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <113e077c-b8ab-4d81-acbd-0f7aa7538c04@tweaklogic.com>
Date: Mon, 4 Mar 2024 23:18:34 +1030
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v8 5/5] iio: light: Add support for APDS9306 Light Sensor
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>,
 Matti Vaittinen <mazziesaccount@gmail.com>,
 Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
 Marek Vasut <marex@denx.de>, Anshul Dalal <anshulusr@gmail.com>,
 Javier Carrasco <javier.carrasco.cruz@gmail.com>,
 Matt Ranostay <matt@ranostay.sg>,
 Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>,
 linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240228122408.18619-1-subhajit.ghosh@tweaklogic.com>
 <20240228122408.18619-6-subhajit.ghosh@tweaklogic.com>
 <20240303151422.5fc3c2f2@jic23-huawei>
Content-Language: en-US
From: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
In-Reply-To: <20240303151422.5fc3c2f2@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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