Re: [PATCH] iio: light : veml6030 Remove code duplication

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon, 23 Jun 2025 17:15:39 -0300
Gabriel Lima de Moraes <gabriellimamoraes@gmail.com> wrote:

> From: Vitor Marques <vitor.marques@ime.usp.br>
> 
> veml6030_hw_init() and veml6035_hw_init() have very similar bodies.
> 
> Reduce code duplication by creating a common initialization function veml603x_hw_common_init().
> 
> Signed-off-by: Vitor Marques <vitor.marques@ime.usp.br>
> Co-developed-by: Gabriel Lima <gabriellimamoraes@ime.usp.br>
> Co-developed-by: Gabriel José <gabrieljpe@ime.usp.br>
> ---
>  drivers/iio/light/veml6030.c | 187 ++++++++++++++++++-----------------

When a patch removing duplication ends up adding code it might not be
such a good idea...

A few comments inline, but in general there isn't a convincing argument
that I can see for this code unification.  So drop this.


>  1 file changed, 95 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index 473a9c3e32a3..7959075b1ae8 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c
> @@ -33,6 +33,18 @@
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>  
> +int veml603x_hw_common_init(
> +    struct iio_dev *indio_dev, 
> +    struct device *dev,
> +    int iio_init_val1,
> +    int iio_init_val2,
> +    const struct iio_gain_sel_pair *gain_sel,
> +    size_t gain_sel_size,
> +    const struct iio_itime_sel_mul *it_sel,
> +    size_t it_sel_size,
> +    int als_conf_val2
> +);

You have defined the function before it is used so why do you need
a forwards declaration?

> +
>  /* Device registers */
>  #define VEML6030_REG_ALS_CONF   0x00
>  #define VEML6030_REG_ALS_WH     0x01
> @@ -248,6 +260,66 @@ static void veml6030_als_shut_down_action(void *data)
>  	veml6030_als_shut_down(data);
>  }
>  
> +int veml603x_hw_common_init(
> +    struct iio_dev *indio_dev, 
> +    struct device *dev,
> +    int iio_init_val1,
> +    int iio_init_val2,

These parameters have names that managed to obscure what is going on
generally making this patch a bad idea.

> +    const struct iio_gain_sel_pair *gain_sel,
> +    size_t gain_sel_size,
> +    const struct iio_itime_sel_mul *it_sel,
> +    size_t it_sel_size,
> +    int als_conf_val2
Why 2?

> +)
> +{
> +	int ret, val;
> +	struct veml6030_data *data = iio_priv(indio_dev);
> +
> +	ret = devm_iio_init_iio_gts(dev, iio_init_val1, iio_init_val2,
> +			gain_sel, gain_sel_size,
> +			it_sel, it_sel_size,
> +			&data->gts);
> +	if (ret)
> +	return dev_err_probe(dev, ret, "failed to init iio gts\n");

Looks like you have some big formatting issues in here.

> +
> +	ret = veml6030_als_shut_down(data);
> +	if (ret)
> +	return dev_err_probe(dev, ret, "can't shutdown als\n");
> +
> +	ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF, als_conf_val2);

Wrapping this up doesn't provide any real advantage, requiring as it does
the reviewer to look at this function AND where the value is set rather
than seeing them in one place.

> +	if (ret)
> +	return dev_err_probe(dev, ret, "can't setup als configs\n");
> +
> +	ret = regmap_update_bits(data->regmap, VEML6030_REG_ALS_PSM,
> +		VEML6030_PSM | VEML6030_PSM_EN, 0x03);
> +	if (ret)
> +	return dev_err_probe(dev, ret, "can't setup default PSM\n");
> +
> +	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WH, 0xFFFF);
> +	if (ret)
> +	return dev_err_probe(dev, ret, "can't setup high threshold\n");
> +
> +	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WL, 0x0000);
> +	if (ret)
> +	return dev_err_probe(dev, ret, "can't setup low threshold\n");
> +
> +	ret = veml6030_als_pwr_on(data);
> +	if (ret)
> +	return dev_err_probe(dev, ret, "can't poweron als\n");
> +
> +	ret = devm_add_action_or_reset(dev, veml6030_als_shut_down_action, data);
> +	if (ret < 0)
> +	return ret;
> +
> +	/* Clear stale interrupt status bits if any during start */
> +	ret = regmap_read(data->regmap, VEML6030_REG_ALS_INT, &val);
> +	if (ret < 0)
> +	return dev_err_probe(dev, ret,
> +			"can't clear als interrupt status\n");
> +
> +	return ret;
> +}
> +
>  static const struct iio_event_spec veml6030_event_spec[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> @@ -973,52 +1045,17 @@ static int veml6030_regfield_init(struct iio_dev *indio_dev)
>   */
>  static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev)
>  {
> -	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),
> -				    veml6030_it_sel, ARRAY_SIZE(veml6030_it_sel),
> -				    &data->gts);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "failed to init iio gts\n");
> -
> -	ret = veml6030_als_shut_down(data);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't shutdown als\n");
> -
> -	ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF, 0x1001);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't setup als configs\n");
> -
> -	ret = regmap_update_bits(data->regmap, VEML6030_REG_ALS_PSM,
> -				 VEML6030_PSM | VEML6030_PSM_EN, 0x03);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't setup default PSM\n");
> -
> -	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WH, 0xFFFF);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't setup high threshold\n");
> -
> -	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WL, 0x0000);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't setup low threshold\n");
> -
> -	ret = veml6030_als_pwr_on(data);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't poweron als\n");
> -
> -	ret = devm_add_action_or_reset(dev, veml6030_als_shut_down_action, data);
> -	if (ret < 0)
> -		return ret;
> -
> -	/* Clear stale interrupt status bits if any during start */
> -	ret = regmap_read(data->regmap, VEML6030_REG_ALS_INT, &val);
> -	if (ret < 0)
> -		return dev_err_probe(dev, ret,
> -				     "can't clear als interrupt status\n");
> -
> -	return ret;
> +	return veml603x_hw_common_init(
> +		indio_dev,
> +		dev,
> +		2,
> +		150400000,
> +		veml6030_gain_sel,
> +		ARRAY_SIZE(veml6030_gain_sel),
> +		veml6030_it_sel,
> +		ARRAY_SIZE(veml6030_it_sel),
> +		0x1001
> +	);
>  }
>  
>  /*
> @@ -1029,52 +1066,18 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev)
>   * update registers and then power on the sensor.
>   */
>  static int veml6035_hw_init(struct iio_dev *indio_dev, struct device *dev)
> -{
> -	int ret, val;
> -	struct veml6030_data *data = iio_priv(indio_dev);
> -
> -	ret = devm_iio_init_iio_gts(dev, 0, 409600000,
> -				    veml6035_gain_sel, ARRAY_SIZE(veml6035_gain_sel),
> -				    veml6030_it_sel, ARRAY_SIZE(veml6030_it_sel),
> -				    &data->gts);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "failed to init iio gts\n");
> -
> -	ret = veml6030_als_shut_down(data);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't shutdown als\n");
> -
> -	ret = regmap_write(data->regmap, VEML6030_REG_ALS_CONF,
> -			   VEML6035_SENS | VEML6035_CHAN_EN | VEML6030_ALS_SD);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't setup als configs\n");
> -
> -	ret = regmap_update_bits(data->regmap, VEML6030_REG_ALS_PSM,
> -				 VEML6030_PSM | VEML6030_PSM_EN, 0x03);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't setup default PSM\n");
> -
> -	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WH, 0xFFFF);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't setup high threshold\n");
> -
> -	ret = regmap_write(data->regmap, VEML6030_REG_ALS_WL, 0x0000);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't setup low threshold\n");
> -
> -	ret = veml6030_als_pwr_on(data);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "can't poweron als\n");
> -
> -	ret = devm_add_action_or_reset(dev, veml6030_als_shut_down_action, data);
> -	if (ret < 0)
> -		return ret;
> -
> -	/* Clear stale interrupt status bits if any during start */
> -	ret = regmap_read(data->regmap, VEML6030_REG_ALS_INT, &val);
> -	if (ret < 0)
> -		return dev_err_probe(dev, ret,
> -				     "can't clear als interrupt status\n");
> +{	
> +	veml603x_hw_common_init(
> +        indio_dev,
> +        dev,
> +        0,
> +        409600000,
> +        veml6035_gain_sel,
> +        ARRAY_SIZE(veml6035_gain_sel),
> +        veml6030_it_sel,
> +        ARRAY_SIZE(veml6030_it_sel),
> +        VEML6035_SENS | VEML6035_CHAN_EN | VEML6030_ALS_SD
> +	);
>  
>  	return 0;
>  }