| 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: <20240914170347.54959319@jic23-huawei>
Date: Sat, 14 Sep 2024 17:03:47 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Rishi Gupta <gupt21@...il.com>,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Jonathan Cameron
<Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH 7/7] iio: light: veml6030: add support for veml6035
On Fri, 13 Sep 2024 15:19:02 +0200
Javier Carrasco <javier.carrasco.cruz@...il.com> wrote:
> The veml6035 is an ALS that shares most of its functionality with the
> veml6030, which allows for some code recycling.
>
> Some chip-specific properties differ and dedicated functions to get and
> set the sensor gain as well as its initialization are required.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
Mostly a request to first switch to using read_avail() and the relevant
bit masks instead of custom attributes. That will require converting the
driver to that approach first, but looks straight forward.
> ---
> drivers/iio/light/veml6030.c | 300 +++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 273 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index 2945cc1db599..105f310c4954 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -1,13 +1,19 @@
> // SPDX-License-Identifier: GPL-2.0+
> /*
> - * VEML6030 Ambient Light Sensor
> + * VEML6030 and VMEL6035 Ambient Light Sensors
> *
> * Copyright (c) 2019, Rishi Gupta <gupt21@...il.com>
> *
> + * VEML6030:
> * Datasheet: https://www.vishay.com/docs/84366/veml6030.pdf
> * Appnote-84367: https://www.vishay.com/docs/84367/designingveml6030.pdf
> + *
> + * VEML6035:
> + * Datasheet: https://www.vishay.com/docs/84889/veml6035.pdf
> + * Appnote-84944: https://www.vishay.com/docs/84944/designingveml6035.pdf
> */
>
> +#include <linux/bitfield.h>
> #include <linux/module.h>
> #include <linux/i2c.h>
> #include <linux/err.h>
> @@ -38,16 +44,33 @@
> #define VEML6030_ALS_INT_EN BIT(1)
> #define VEML6030_ALS_SD BIT(0)
>
> +#define VEML6035_GAIN_M GENMASK(12, 10)
> +#define VEML6035_GAIN BIT(10)
> +#define VEML6035_DG BIT(11)
> +#define VEML6035_SENS BIT(12)
> +#define VEML6035_INT_CHAN BIT(3)
> +#define VEML6035_CHAN_EN BIT(2)
> +
> +struct veml603x_chip {
> + const char *name;
> + const struct iio_info *info;
> + const struct iio_info *info_no_irq;
> + const char * const in_illuminance_scale_avail;
For this, better with read_avail() provided and a pointer to an array of
values + a size element in here. That way we can get rid of the
custom attribute handling. Might end up as similar amount of code, but
will be simpler to read.
> + int (*hw_init)(struct iio_dev *indio_dev);
> + int (*set_als_gain)(struct iio_dev *indio_dev, int val, int val2);
> + int (*get_als_gain)(struct iio_dev *indio_dev, int *val, int *val2);
> +};
>
> /* Integration time available in seconds */
> @@ -63,14 +87,25 @@ static IIO_CONST_ATTR(in_illuminance_integration_time_available,
>
> /*
> * Scale is 1/gain. Value 0.125 is ALS gain x (1/8), 0.25 is
> - * ALS gain x (1/4), 1.0 = ALS gain x 1 and 2.0 is ALS gain x 2.
> + * ALS gain x (1/4), 0.5 is ALS gain x (1/2), 1.0 is ALS gain x 1,
> + * 2.0 is ALS gain x2, and 4.0 is ALS gain x 4.
> */
> -static IIO_CONST_ATTR(in_illuminance_scale_available,
> +static IIO_CONST_ATTR_NAMED(veml6030_in_illuminance_scale_available,
> + in_illuminance_scale_available,
> "0.125 0.25 1.0 2.0");
> +static IIO_CONST_ATTR_NAMED(veml6035_in_illuminance_scale_available,
> + in_illuminance_scale_available,
> + "0.125 0.25 0.5 1.0 2.0 4.0");
>
> static struct attribute *veml6030_attributes[] = {
> &iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr,
> - &iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
> + &iio_const_attr_veml6030_in_illuminance_scale_available.dev_attr.attr,
> + NULL
> +};
> +
> +static struct attribute *veml6035_attributes[] = {
> + &iio_const_attr_in_illuminance_integration_time_available.dev_attr.attr,
> + &iio_const_attr_veml6035_in_illuminance_scale_available.dev_attr.attr,
Using get_avail() etc would let you handle these as arrays of numbers rather than
strings + get rid of the need for any custom attributes. This should be
a very simple conversion so perhaps worth doing before adding the
new support. Then you will have pointers to the value arrays + sizes
in your chip specific structures that just get looked up directly
by read_avail()
> NULL
> };
>
> +/*
> + * Set ALS gain to 1/8, integration time to 100 ms, ALS and WHITE
> + * channel enabled, ALS channel interrupt, PSM enabled,
> + * PSM_WAIT = 0.8 s, persistence to 1 x integration time and the
> + * threshold interrupt disabled by default. First shutdown the sensor,
> + * update registers and then power on the sensor.
> + */
> +static int veml6035_hw_init(struct iio_dev *indio_dev)
> +{
> + int ret, val;
> + struct veml6030_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> +
> + ret = veml6030_als_shut_down(data);
> + if (ret) {
> + dev_err(&client->dev, "can't shutdown als %d\n", ret);
> + return ret;
If this is only ever called from probe() (I think that's true?)
can use return dev_err_probe() for all these error cases.
Main advantage here being shorter simpler code.
> + }
> +
> + ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF,
> + VEML6035_SENS | VEML6035_CHAN_EN | VEML6030_ALS_SD);
> + if (ret) {
> + dev_err(&client->dev, "can't setup als configs %d\n", ret);
> + return ret;
> + }
> +
> + ret = regmap_update_bits(data->regmap, VEML6030_REG_ALS_PSM,
> + VEML6030_PSM | VEML6030_PSM_EN, 0x03);
> + if (ret) {
> + dev_err(&client->dev, "can't setup default PSM %d\n", ret);
> + return ret;
> + }
> +
> + ret = regmap_write(data->regmap, VEML6030_REG_ALS_WH, 0xFFFF);
> + if (ret) {
> + dev_err(&client->dev, "can't setup high threshold %d\n", ret);
> + return ret;
> + }
> +
> + ret = regmap_write(data->regmap, VEML6030_REG_ALS_WL, 0x0000);
> + if (ret) {
> + dev_err(&client->dev, "can't setup low threshold %d\n", ret);
> + return ret;
> + }
> +
> + ret = veml6030_als_pwr_on(data);
> + if (ret) {
> + dev_err(&client->dev, "can't poweron als %d\n", ret);
> + return ret;
> + }
> +
> + /* Clear stale interrupt status bits if any during start */
> + ret = regmap_read(data->regmap, VEML6030_REG_ALS_INT, &val);
> + if (ret < 0) {
> + dev_err(&client->dev,
> + "can't clear als interrupt status %d\n", ret);
> + return ret;
It's true of existing code, but I noticed it here.
Should we be powering down in this error path?
> + }
> +
> + /* Cache currently active measurement parameters */
> + data->cur_gain = 5;
> + data->cur_resolution = 1024;
> + data->cur_integration_time = 3;
> +
> + return 0;
> +}
Powered by blists - more mailing lists