On Fri, 13 Sep 2024 15:19:02 +0200 Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> > * > + * 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; > +}
