Re: [PATCH 7/7] iio: light: veml6030: add support for veml6035

[Jonathan Cameron <jic23@kernel.org>]

...

> >>  
> >> +/*
> >> + * 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()?

Move it in here so that you register the powerdown alongside the power
up. Doesn't matter that it is inside the hw_init() function so a little
less obvious.

Jonathan
> 
> 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