Re: [PATCH v6 1/2] staging: iio: adc: ad7280a: power down the device on error in probe

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 11 Nov 2018 16:59:10 +0100
Slawomir Stepien <sst@poczta.fm> wrote:

> Power down the device if anything goes wrong after the SPI has been
> setup correctly in the probe function.
> 
> Existing code that toggles the AD7280A_CTRL_LB_SWRST bit inside
> ad7280_chain_setup function is responsible for powering up the device.
> 
> Signed-off-by: Slawomir Stepien <sst@poczta.fm>
Hi Slawomir,

Nearly perfect... But see inline...
> ---
>  drivers/staging/iio/adc/ad7280a.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> index 58420dcb406d..3ab06fd87675 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -870,7 +870,7 @@ static int ad7280_probe(struct spi_device *spi)
>  
>  	ret = ad7280_chain_setup(st);
>  	if (ret < 0)
> -		return ret;
> +		goto error_power_down;
The ideal is always for a function to unwind on error to the point where
it leaves no visible side effects.  As such we should power down 'inside'
chain_setup on error (unfortunately there are a lot exit paths in that
function so the resulting patch will be larger than this :(

This may seem a pain to do this way, but it makes the code flow
more obvious generally making things cleaner.

Rather than go around again for such a trivial thing, I've
made the change and pushed out the updated patch to the togreg
branch of iio.git as testing for the autobuilders to see if
I messed it up!

Note this will move the resulting devm call as well. I'll do that for
patch 2.

Jonathan


>  
>  	st->slave_num = ret;
>  	st->scan_cnt = (st->slave_num + 1) * AD7280A_NUM_CH;
> @@ -901,7 +901,7 @@ static int ad7280_probe(struct spi_device *spi)
>  
>  	ret = ad7280_channel_init(st);
>  	if (ret < 0)
> -		return ret;
> +		goto error_power_down;
>  
>  	indio_dev->num_channels = ret;
>  	indio_dev->channels = st->channels;
> @@ -950,6 +950,9 @@ static int ad7280_probe(struct spi_device *spi)
>  error_free_channels:
>  	kfree(st->channels);
>  
> +error_power_down:
> +	ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
> +		     AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);
>  	return ret;
>  }
>