* Re: [PATCH] iio: light: veml6030: Generalize hw_init for veml6030 and veml6035
[not found] ` <CAOmGE-3_WYpL0AFpV-NcF=yZ2SBvUrSELuc=VXb=UVKGBbxnvw@mail.gmail.com>
@ 2026-04-28 18:07 ` Jonathan Cameron
0 siblings, 0 replies; 2+ messages in thread
From: Jonathan Cameron @ 2026-04-28 18:07 UTC (permalink / raw)
To: Gabriel Braga Lagrotaria de Oliveira
Cc: David Lechner, javier.carrasco.cruz, nuno.sa, andy,
Ricardo H H Kojo, Ellian Carlos, linux-iio
On Mon, 27 Apr 2026 20:37:23 -0300
Gabriel Braga Lagrotaria de Oliveira <gabrielblo@ime.usp.br> wrote:
> Thank you for the feedback! We have one remaining question regarding the
> naming conventions: while adding the init_config pointer to the chip
> structure, we noticed the structure name itself (veml603x_chip) includes
> the 'x' wildcard. Should we rename this to a specific model or keep it
> as-is for the v2 submission?
Other than top posting, good question.
If it had a lot of instances I'd argue not worth the effort, but as there
are only 4, sure an extra patch to tidy that up would remove one more
case of wildcards people might copy in future.
Thanks
Jonathan
>
> Em seg., 27 de abr. de 2026 às 20:32, Gabriel Braga Lagrotaria de Oliveira <
> gabrielblo@ime.usp.br> escreveu:
>
> > Thank you for the feedback! We have one remaining question regarding the
> > naming conventions: while adding the init_config pointer to the chip
> > structure, we noticed the structure name itself (veml603x_chip) includes
> > the 'x' wildcard. Should we rename this to a specific model or keep it
> > as-is for the v2 submission?
> >
> > Em seg., 20 de abr. de 2026 às 18:00, David Lechner <dlechner@baylibre.com>
> > escreveu:
> >
> >> On 4/20/26 3:14 PM, Gabriel Braga Lagrotaria wrote:
> >> > Add a new veml603x_hw_init() function to deduplicate the setup logic
> >> > for veml6030_hw_init() and veml6035_hw_init().
> >> >
> >> > Additionally, introduce struct veml603x_hw_init_config to store and
> >> > pass the custom configuration values specific to each device variation.
> >> >
> >> > Signed-off-by: Gabriel Braga Lagrotaria <gabrielblo@ime.usp.br>
> >> > Co-developed-by: Ricardo H H Kojo <ricardo.kojo@ime.usp.br>
> >> > Signed-off-by: Ricardo H H Kojo <ricardo.kojo@ime.usp.br>
> >> > Co-developed-by: Ellian Carlos <elliancarlos@gmail.com>
> >> > Signed-off-by: Ellian Carlos <elliancarlos@gmail.com>
> >> > ---
> >> > drivers/iio/light/veml6030.c | 98 +++++++++++++++---------------------
> >> > 1 file changed, 41 insertions(+), 57 deletions(-)
> >> >
> >> > diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> >> > index 6bcacae38..84a551683 100644
> >> > --- a/drivers/iio/light/veml6030.c
> >> > +++ b/drivers/iio/light/veml6030.c
> >> > @@ -963,19 +963,38 @@ static int veml6030_regfield_init(struct iio_dev
> >> *indio_dev)
> >> > return 0;
> >> > }
> >> >
> >> > -/*
> >> > - * Set ALS gain to 1/8, integration time to 100 ms, PSM to mode 2,
> >> > - * 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 veml6030_hw_init(struct iio_dev *indio_dev, struct device
> >> *dev)
> >> > +struct veml603x_hw_init_config {
> >>
> >> Generally we try to avoid "x" wildcard in names as it is not particularly
> >> future-proof. We can just use veml6030 since that is the driver name.
> >>
> >> > + const struct iio_gain_sel_pair *gain_sel;
> >> > + int gain_sel_size;
> >> > + int gain_max_scale_int;
> >> > + int gain_max_scale_nano;
> >> > + unsigned int reg_als_conf_value;
> >> > +};
> >>
> >> Would it make sense to add this to the chip info instead of creating
> >> new tables?
> >>
> >> > +
> >> > +static const struct veml603x_hw_init_config veml6030_hw_init_config = {
> >> > + .gain_sel = veml6030_gain_sel,
> >> > + .gain_sel_size = ARRAY_SIZE(veml6030_gain_sel),
> >> > + .gain_max_scale_int = 2,
> >> > + .gain_max_scale_nano = 150400000,
> >> > + .reg_als_conf_value = 0x1001,
> >> > +};
> >> > +
> >> > +static const struct veml603x_hw_init_config veml6035_hw_init_config = {
> >> > + .gain_sel = veml6035_gain_sel,
> >> > + .gain_sel_size = ARRAY_SIZE(veml6035_gain_sel),
> >> > + .gain_max_scale_int = 0,
> >> > + .gain_max_scale_nano = 409600000,
> >> > + .reg_als_conf_value = VEML6035_SENS | VEML6035_CHAN_EN |
> >> VEML6030_ALS_SD,
> >> > +};
> >> > +
> >>
> >> Or if we want to keep these to avoid duplication...
> >>
> >> > +static int veml603x_hw_init(struct iio_dev *indio_dev, struct device
> >> *dev,
> >> > + const struct veml603x_hw_init_config *cfg)
> >> > {
> >> > int ret, val;
> >> > struct veml6030_data *data = iio_priv(indio_dev);
> >> >
> >> > - ret = devm_iio_init_iio_gts(dev, 2, 150400000,
> >> > - veml6030_gain_sel,
> >> ARRAY_SIZE(veml6030_gain_sel),
> >> > + ret = devm_iio_init_iio_gts(dev, cfg->gain_max_scale_int,
> >> cfg->gain_max_scale_nano,
> >> > + cfg->gain_sel, cfg->gain_sel_size,
> >> > veml6030_it_sel,
> >> ARRAY_SIZE(veml6030_it_sel),
> >> > &data->gts);
> >> > if (ret)
> >> > @@ -985,7 +1004,7 @@ static int veml6030_hw_init(struct iio_dev
> >> *indio_dev, struct device *dev)
> >> > if (ret)
> >> > return dev_err_probe(dev, ret, "can't shutdown als\n");
> >> >
> >> > - ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF, 0x1001);
> >> > + ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF,
> >> cfg->reg_als_conf_value);
> >> > if (ret)
> >> > return dev_err_probe(dev, ret, "can't setup als
> >> configs\n");
> >> >
> >> > @@ -1019,6 +1038,17 @@ static int veml6030_hw_init(struct iio_dev
> >> *indio_dev, struct device *dev)
> >> > return ret;
> >> > }
> >> >
> >> > +/*
> >> > + * Set ALS gain to 1/8, integration time to 100 ms, PSM to mode 2,
> >> > + * 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 veml6030_hw_init(struct iio_dev *indio_dev, struct device
> >> *dev)
> >> > +{
> >> > + return veml603x_hw_init(indio_dev, dev, &veml6030_hw_init_config);
> >> > +}
> >> > +
> >>
> >> ... then drop these wrapper functions and just put the pointer to the
> >> hw_init_config in the chip info.
> >>
> >> > /*
> >> > * Set ALS gain to 1/8, integration time to 100 ms, ALS and WHITE
> >> > * channel enabled, ALS channel interrupt, PSM enabled,
> >>
> >> I think this comment and the one a bit above would mame more sense now
> >> on the hw_init_config structs since that is where the actual parameters
> >> are defined.
> >>
> >> > @@ -1028,53 +1058,7 @@ static int veml6030_hw_init(struct iio_dev
> >> *indio_dev, struct device *dev)
> >> > */
> >> > static int veml6035_hw_init(struct iio_dev *indio_dev, struct device
> >> *dev)
> >> > {
> >>
> >> ...
> >>
> >> > + return veml603x_hw_init(indio_dev, dev, &veml6035_hw_init_config);
> >> > }
> >> >
> >> > static int veml6030_probe(struct i2c_client *client)
> >>
> >>
^ permalink raw reply [flat|nested] 2+ messages in thread