Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions

[Marcus Wolf]

Am 06.12.2017 um 11:37 schrieb Dan Carpenter:
> On Wed, Dec 06, 2017 at 11:05:22AM +0200, Marcus Wolf wrote:
> > Am 06.12.2017 um 00:08 schrieb Simon SandstrÃm:
> > > Splits rf69_set_crc_enabled(dev, enabled) into
> > > rf69_enable_crc(dev) and rf69_disable_crc(dev).
> > >
> > > Signed-off-by: Simon SandstrÃm <simon@xxxxxxxxxx>
> > > ---
> > >    drivers/staging/pi433/pi433_if.c | 22 ++++++++++++++++++++--
> > >    drivers/staging/pi433/rf69.c     | 18 ++++++------------
> > >    drivers/staging/pi433/rf69.h     |  4 ++--
> > >    3 files changed, 28 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> > > index 2ae19ac565d1..614eec7dd904 100644
> > > --- a/drivers/staging/pi433/pi433_if.c
> > > +++ b/drivers/staging/pi433/pi433_if.c
> > > @@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
> > >    			return ret;
> > >    	}
> > >    	SET_CHECKED(rf69_set_adressFiltering(dev->spi, rx_cfg->enable_address_filtering));
> > > -	SET_CHECKED(rf69_set_crc_enable	    (dev->spi, rx_cfg->enable_crc));
> > > +
> > > +	if (rx_cfg->enable_crc == OPTION_ON) {
> > > +		ret = rf69_enable_crc(dev->spi);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	} else {
> > > +		ret = rf69_disable_crc(dev->spi);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> >
> > Why don't you use SET_CHECKED(...)?
>
> Marcus, please don't introduce new uses of SET_CHECKED().  It has a
> hidden return in it which is against kernel style and introduces very
> predictable and avoidable bugs.  For example, in probe().

Ah ok.

Thanks for clarifiytion!

What a pitty - another bunch of extra lines of code...

Or is there an other construction, allowing for one line per register 
change? Something like

	ret = rf69_set_xyz(...); if (ret) return ret;
	ret = rf69_set_abc(...); if (ret) return ret;

is pretty ugly and voids the style guide...

Thx,

Marcus