| 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: <20250125142040.273f9cd6@jic23-huawei>
Date: Sat, 25 Jan 2025 14:20:40 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Mikael Gonella-Bolduc via B4 Relay
<devnull+mgonellabolduc.dimonoff.com@...nel.org>
Cc: mgonellabolduc@...onoff.com, Lars-Peter Clausen <lars@...afoo.de>, Rob
Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
Dooley <conor+dt@...nel.org>, Nathan Chancellor <nathan@...nel.org>, Nick
Desaulniers <ndesaulniers@...gle.com>, Bill Wendling <morbo@...gle.com>,
Justin Stitt <justinstitt@...gle.com>, Mikael Gonella-Bolduc
<m.gonella.bolduc@...il.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
llvm@...ts.linux.dev, Hugo Villeneuve <hvilleneuve@...onoff.com>, Matti
Vaittinen <mazziesaccount@...il.com>
Subject: Re: [PATCH v5 2/2] iio: light: Add APDS9160 ALS & Proximity sensor
driver
On Wed, 22 Jan 2025 17:59:34 -0500
Mikael Gonella-Bolduc via B4 Relay <devnull+mgonellabolduc.dimonoff.com@...nel.org> wrote:
> From: Mikael Gonella-Bolduc <mgonellabolduc@...onoff.com>
>
> APDS9160 is a combination of ALS and proximity sensors.
>
> This patch add supports for:
> - Intensity clear data and illuminance data
> - Proximity data
> - Gain control, rate control
> - Event thresholds
>
> Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@...onoff.com>
Hi Mikael,
Some minor things inline. I tidied up whilst applying.
Applied to the togreg branch of iio.git; initially pushed out as testing
as I'll be rebasing on rc1 once available.
thanks,
Jonathan
> ---
> MAINTAINERS | 7 +
> drivers/iio/light/Kconfig | 11 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/apds9160.c | 1597 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1616 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e69d1632c382fe0e366f7bb20e72ee0c9e91e30b..23d9fcbf71a311940ff86d8dc4cabd5be77925aa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4340,6 +4340,13 @@ F: kernel/bpf/stackmap.c
> F: kernel/trace/bpf_trace.c
> F: lib/buildid.c
>
> +BROADCOM APDS9160 AMBIENT LIGHT SENSOR AND PROXIMITY DRIVER
> +M: Mikael Gonella-Bolduc <m.gonella.bolduc@...il.com>
> +L: linux-iio@...r.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
> +F: drivers/iio/light/apds9160.c
> +
> BROADCOM ASP 2.0 ETHERNET DRIVER
> M: Justin Chen <justin.chen@...adcom.com>
> M: Florian Fainelli <florian.fainelli@...adcom.com>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 29ffa84919273d64b8464ab8bbf59661b0102f97..419360661d53973eda71d7d986ff7fd481c7aa2c 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -63,6 +63,17 @@ config AL3320A
> To compile this driver as a module, choose M here: the
> module will be called al3320a.
>
> +config APDS9160
> + tristate "APDS9160 combined als and proximity sensor"
> + depends on I2C
> + select REGMAP_I2C
> + help
> + Say Y here if you want to build support for a Broadcom APDS9160
> + combined ambient light and proximity sensor.
Strange indent.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apds9160.
> +
> diff --git a/drivers/iio/light/apds9160.c b/drivers/iio/light/apds9160.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..e98006de473790da66b92734a0527359e9cd1f40
> --- /dev/null
> +++ b/drivers/iio/light/apds9160.c
> +/*
> + * This parameter works in conjunction with the cancellation pulse duration
> + * The value determines the current used for crosstalk cancellation
> + * Coarse value is in steps of 60 nA
> + * Fine value is in steps of 2.4 nA
> + */
> +static int apds9160_set_ps_cancellation_current(struct apds9160_chip *data,
> + int coarse_val,
> + int fine_val)
> +{
> + int ret, val;
> +
> + if (coarse_val < 0 || coarse_val > 4)
> + return -EINVAL;
> +
> + if (fine_val < 0 || fine_val > 15)
> + return -EINVAL;
> +
> + /* Coarse value at B4:B5 and fine value at B0:B3 */
> + val = (coarse_val << 4) | fine_val;
> +
> + ret = regmap_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT,
> + val);
> +
> + return ret;
return regmap_write()
> +}
> +
> +static int apds9160_ps_init_analog_cancellation(struct device *dev,
> + struct apds9160_chip *data)
> +{
> + int ret, duration, picoamp, idx, coarse, fine;
> +
> + ret = device_property_read_u32(dev,
> + "ps-cancellation-duration", &duration);
> +
Good to keep call + return together so no blank line here.
> + if (ret || duration == 0)
Given the comment is in the block, I think this wants {} for readability
reasons (not correctness).
> + /* Don't fail since this is not required */
> + return 0;
> +
> + ret = device_property_read_u32(dev,
> + "ps-cancellation-current-picoamp", &picoamp);
> +
Odd line break here.
> + if (ret)
> + return ret;
> +
> + if (picoamp < 60000 || picoamp > 276000 || picoamp % 2400 != 0)
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid cancellation current\n");
> +
> + /* Compute required coarse and fine value from requested current */
> + fine = 0;
> + coarse = 0;
> + for (idx = 60000; idx < picoamp; idx += 2400) {
> + if (fine == 15) {
> + fine = 0;
> + coarse++;
> + idx += 21600;
> + } else {
> + fine++;
> + }
> + }
> +
> + if (picoamp != idx)
> + dev_warn(dev,
> + "Invalid cancellation current %i, rounding to %i\n",
> + picoamp, idx);
> +
> + ret = apds9160_set_ps_analog_cancellation(data, duration);
> + if (ret)
> + return ret;
> +
> + return apds9160_set_ps_cancellation_current(data, coarse, fine);
> +}
> +
> +/*
> + * Setting the integration time ajusts resolution, rate, scale and gain
Odd indent of the comment.
> + */
> +static int apds9160_set_als_int_time(struct apds9160_chip *data, int val)
> +static int apds9160_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long mask)
> +{
> + struct apds9160_chip *data = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_INT_TIME:
> + switch (chan->type) {
> + case IIO_LIGHT:
> + *length = ARRAY_SIZE(apds9160_als_rate_avail);
> + *vals = (const int *)apds9160_als_rate_avail;
> + *type = IIO_VAL_INT;
> +
> + return IIO_AVAIL_LIST;
> + case IIO_PROXIMITY:
> + *length = ARRAY_SIZE(apds9160_ps_rate_avail);
> + *vals = (const int *)apds9160_ps_rate_avail;
> + *type = IIO_VAL_INT;
> +
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + *length = ARRAY_SIZE(apds9160_ps_gain_avail);
> + *vals = (const int *)apds9160_ps_gain_avail;
> + *type = IIO_VAL_INT;
> +
> + return IIO_AVAIL_LIST;
> + case IIO_LIGHT:
> + /* The available scales changes depending on itime */
> + switch (data->als_itime) {
> + case 25:
> + *length = ARRAY_SIZE(apds9160_25ms_avail) * 2;
> + *vals = (const int *) apds9160_25ms_avail;
> + *type = IIO_VAL_INT_PLUS_MICRO;
> +
> + return IIO_AVAIL_LIST;
> + case 50:
> + *length = ARRAY_SIZE(apds9160_50ms_avail) * 2;
> + *vals = (const int *) apds9160_50ms_avail;
> + *type = IIO_VAL_INT_PLUS_MICRO;
> +
> + return IIO_AVAIL_LIST;
> + case 100:
> + *length = ARRAY_SIZE(apds9160_100ms_avail) * 2;
> + *vals = (const int *) apds9160_100ms_avail;
> + *type = IIO_VAL_INT_PLUS_MICRO;
> +
> + return IIO_AVAIL_LIST;
> + case 200:
> + *length = ARRAY_SIZE(apds9160_200ms_avail) * 2;
> + *vals = (const int *) apds9160_200ms_avail;
Completely trivial but the spacing here after) isn't consistent with some cases above.
> + *type = IIO_VAL_INT_PLUS_MICRO;
Powered by blists - more mailing lists