On 14/09/2024 18:03, Jonathan Cameron wrote: > 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() > > Hi Jonathan, thanks for your review. I will update the update the driver to work that way. >> 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. > I know, I procrastinated a little bit and I left the dev_err() calls as they are. But that's easy to update, so I will add a patch for it in v2. >> + } >> + >> + 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? > We could, because this is the only error path where the device is powered on before the power off action gets registered. On the other hand, could we not move the call to devm_add_action_or_reset() a few lines up, so the action gets registered before calling hw_init()? Powering off the device is just writing a bit, so it would not hurt in the error paths where the device is already powered off. Then we would not need an explicit call to power off the device in this error path. >> + } >> + >> + /* Cache currently active measurement parameters */ >> + data->cur_gain = 5; >> + data->cur_resolution = 1024; >> + data->cur_integration_time = 3; >> + >> + return 0; >> +} > Best regards, Javier Carrasco
