| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <113e077c-b8ab-4d81-acbd-0f7aa7538c04@tweaklogic.com>
Date: Mon, 4 Mar 2024 23:18:34 +1030
From: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Matti Vaittinen <mazziesaccount@...il.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Marek Vasut <marex@...x.de>, Anshul Dalal <anshulusr@...il.com>,
Javier Carrasco <javier.carrasco.cruz@...il.com>,
Matt Ranostay <matt@...ostay.sg>,
Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@...s.com>,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 5/5] iio: light: Add support for APDS9306 Light Sensor
On 4/3/24 01:44, Jonathan Cameron wrote:
> On Wed, 28 Feb 2024 22:54:08 +1030
> Subhajit Ghosh <subhajit.ghosh@...aklogic.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@...aklogic.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,
>> + },
>> +};
>
>
Powered by blists - more mailing lists