| 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: <a3bd83fb-589e-4814-9595-8f6343ecda83@gmail.com>
Date: Thu, 21 Nov 2024 13:12:15 +0100
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: mgonellabolduc@...onoff.com, Jonathan Cameron <jic23@...nel.org>,
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>
Cc: 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>
Subject: Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor
driver
Hi Mikael,
a few comments inline to add to what Krzysztof pointed out.
On 19/11/2024 21:36, Mikael Gonella-Bolduc via B4 Relay 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>
> ---
> MAINTAINERS | 7 +
> drivers/iio/light/Kconfig | 13 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/apds9160.c | 1420 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1441 insertions(+)
>
...
> +config APDS9160
> + tristate "APDS9160 combined als and proximity sensors"
> + select REGMAP_I2C
> + select IIO_BUFFER
> + select IIO_KFIFO_BUF
> + depends on I2C
> + help
> + Say Y here if you want to build a driver for Broadcom APDS9160
> + combined ambient light and proximity sensor chip.
> +
You can drop that "If unsure, say N here." as it is not common for such
drivers in IIO. There are a couple of entries in the whole subsystem
(not in this Kconfig, though) with this sentence, and some of them could
be dropped too.
> + To compile this driver as a module, choose M here: the
> + module will be called apds9160. If unsure, say N here.
> +
> config APDS9300
> tristate "APDS9300 ambient light sensor"
> depends on I2C
...
> +
> +static int apds9160_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct apds9160_chip *data = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_INT_TIME:
> + if (val2 != 0)
> + return -EINVAL;
> + switch (chan->type) {
> + case IIO_INTENSITY:
> + return apds9160_set_als_int_time(data, val);
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (val2 != 0)
> + return -EINVAL;
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + return apds9160_set_ps_rate(data, val);
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + if (val2 != 0)
> + return -EINVAL;
> + switch (chan->type) {
> + case IIO_INTENSITY:
> + return apds9160_set_als_gain(data, val);
> + case IIO_PROXIMITY:
> + return apds9160_set_ps_gain(data, val);
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_CALIBSCALE:
> + if (val2 != 0)
> + return -EINVAL;
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + return apds9160_set_ps_cancellation_level(data, val);
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_CALIBBIAS:
> + if (val2 != 0)
> + return -EINVAL;
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + return apds9160_set_ps_analog_cancellation(data, val);
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_RAW:
> + if (val2 != 0)
> + return -EINVAL;
> + switch (chan->type) {
> + case IIO_CURRENT:
> + return apds9160_set_ps_current(data, val);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +
If you only have a switch with a return in every path, this return 0
can't be reached.
> + return 0;
> +}
> +
> +static inline int apds9160_get_thres_reg(const struct iio_chan_spec *chan,
> + enum iio_event_direction dir, u8 *reg)
> +{
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + *reg = APDS9160_REG_PS_THRES_HI_LSB;
> + break;
> + case IIO_INTENSITY:
> + *reg = APDS9160_REG_LS_THRES_UP_LSB;
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> + case IIO_EV_DIR_FALLING:
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + *reg = APDS9160_REG_PS_THRES_LO_LSB;
> + break;
> + case IIO_INTENSITY:
> + *reg = APDS9160_REG_LS_THRES_LO_LSB;
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int apds9160_read_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int *val, int *val2)
> +{
> + u8 reg;
> +
> + int ret = 0;
> + struct apds9160_chip *data = iio_priv(indio_dev);
> +
> + if (info != IIO_EV_INFO_VALUE)
> + return -EINVAL;
> +
> + ret = apds9160_get_thres_reg(chan, dir, ®);
> + if (ret < 0)
> + return ret;
> +
> + if (chan->type == IIO_PROXIMITY) {
> + __le16 buf;
> +
> + ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
> + if (ret < 0)
> + return ret;
> + *val = le16_to_cpu(buf);
> + } else if (chan->type == IIO_INTENSITY) {
> + __le32 buf = 0;
> +
> + ret = regmap_bulk_read(data->regmap, reg, &buf, 3);
> + if (ret < 0)
> + return ret;
> + *val = le32_to_cpu(buf);
Missing braces for that else (use them in all arms if you need them in one).
> + } else
> + return -EINVAL;
> +
> + *val2 = 0;
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int apds9160_write_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int val, int val2)
> +{
> + u8 reg;
> + int ret = 0;
> + struct apds9160_chip *data = iio_priv(indio_dev);
> +
> + if (info != IIO_EV_INFO_VALUE)
> + return -EINVAL;
> +
> + ret = apds9160_get_thres_reg(chan, dir, ®);
> + if (ret < 0)
> + return ret;
> +
> + if (chan->type == IIO_PROXIMITY) {
> + if (val < 0 || val > APDS9160_PS_THRES_MAX)
> + return -EINVAL;
> + __le16 buf;
> +
> + buf = cpu_to_le16(val);
> + ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
> + if (ret < 0)
> + return ret;
> + } else if (chan->type == IIO_INTENSITY) {
> + if (val < 0 || val > APDS9160_LS_THRES_MAX)
> + return -EINVAL;
> + __le32 buf = 0;
> +
> + buf = cpu_to_le32(val);
> + ret = regmap_bulk_write(data->regmap, reg, &buf, 3);
> + if (ret < 0)
> +
Same here.
return ret;
> + } else
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int apds9160_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct apds9160_chip *data = iio_priv(indio_dev);
> +
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + return data->ps_int;
> + case IIO_INTENSITY:
> + return data->als_int;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
The 'state' argument is now a bool. To avoid issues, please rebase to
newer branches like linux-next, iio/testing iio/togreg. Otherwise it
will not compile with that modification. data->ps_int should then become
a bool too.
> +static int apds9160_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir, int state)
> +{
> + struct apds9160_chip *data = iio_priv(indio_dev);
> + int ret;
> +
> + state = !!state;
> +
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + if (data->ps_int == state)
> + return -EINVAL;
> +
> + ret = regmap_field_write(data->reg_int_ps, state);
> + if (ret)
> + return ret;
> + data->ps_int = state;
> + break;
> + case IIO_INTENSITY:
> + if (data->als_int == state)
> + return -EINVAL;
> +
> + ret = regmap_field_write(data->reg_int_als, state);
> + if (ret)
> + return ret;
> + data->als_int = state;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Best regards,
Javier Carrasco
Powered by blists - more mailing lists