| Message ID | 20200831090813.78841-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/8] iio: accel: bma220: Fix returned codes from bma220_init(), bma220_deinit() | expand |
On Mon, 31 Aug 2020 12:08:06 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Potentially bma220_init() and bma220_deinit() may return positive codes. > Fix the logic to return proper error codes instead. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Hi Andy A nice straight forward set and I suspect we aren't going to get any other reviews, hence... Series applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to poke at it. Thanks, Jonathan > --- > drivers/iio/accel/bma220_spi.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c > index da8b36cc8628..3247b9c8abcb 100644 > --- a/drivers/iio/accel/bma220_spi.c > +++ b/drivers/iio/accel/bma220_spi.c > @@ -198,10 +198,12 @@ static int bma220_init(struct spi_device *spi) > > /* Make sure the chip is powered on */ > ret = bma220_read_reg(spi, BMA220_REG_SUSPEND); > + if (ret == BMA220_SUSPEND_WAKE) > + ret = bma220_read_reg(spi, BMA220_REG_SUSPEND); > if (ret < 0) > return ret; > - else if (ret == BMA220_SUSPEND_WAKE) > - return bma220_read_reg(spi, BMA220_REG_SUSPEND); > + if (ret == BMA220_SUSPEND_WAKE) > + return -EBUSY; > > return 0; > } > @@ -212,10 +214,12 @@ static int bma220_deinit(struct spi_device *spi) > > /* Make sure the chip is powered off */ > ret = bma220_read_reg(spi, BMA220_REG_SUSPEND); > + if (ret == BMA220_SUSPEND_SLEEP) > + ret = bma220_read_reg(spi, BMA220_REG_SUSPEND); > if (ret < 0) > return ret; > - else if (ret == BMA220_SUSPEND_SLEEP) > - return bma220_read_reg(spi, BMA220_REG_SUSPEND); > + if (ret == BMA220_SUSPEND_SLEEP) > + return -EBUSY; > > return 0; > } > @@ -245,7 +249,7 @@ static int bma220_probe(struct spi_device *spi) > indio_dev->available_scan_masks = bma220_accel_scan_masks; > > ret = bma220_init(data->spi_device); > - if (ret < 0) > + if (ret) > return ret; > > ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
On Mon, Aug 31, 2020 at 10:21:45AM +0100, Jonathan Cameron wrote: > On Mon, 31 Aug 2020 12:08:06 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > Potentially bma220_init() and bma220_deinit() may return positive codes. > > Fix the logic to return proper error codes instead. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Hi Andy > > A nice straight forward set and I suspect we aren't going to get any other > reviews, hence... > > Series applied to the togreg branch of iio.git and pushed out as testing for > the autobuilders to poke at it. Thanks! P.S. Consider this series as an example what can be done to many IIO drivers in order to clean them up. My focus, of course, ACPI interaction: - use of ACPI_PTR / ifdeffery - inclusion of acpi.h - ...
On Mon, 31 Aug 2020 14:49:04 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, Aug 31, 2020 at 10:21:45AM +0100, Jonathan Cameron wrote: > > On Mon, 31 Aug 2020 12:08:06 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > Potentially bma220_init() and bma220_deinit() may return positive codes. > > > Fix the logic to return proper error codes instead. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Hi Andy > > > > A nice straight forward set and I suspect we aren't going to get any other > > reviews, hence... > > > > Series applied to the togreg branch of iio.git and pushed out as testing for > > the autobuilders to poke at it. > > Thanks! > > P.S. Consider this series as an example what can be done to many IIO drivers > in order to clean them up. My focus, of course, ACPI interaction: > - use of ACPI_PTR / ifdeffery > - inclusion of acpi.h > - ... > Thanks. This is probably a good one for anyone who wants to grow their experience in kernel patches etc. I'll add it to my more or less never ending list if not and get to it eventually. In the meantime we'll try to avoid introducing any new variants! Jonathan
On Mon, 31 Aug 2020 15:12:01 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 31 Aug 2020 14:49:04 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Aug 31, 2020 at 10:21:45AM +0100, Jonathan Cameron wrote: > > > On Mon, 31 Aug 2020 12:08:06 +0300 > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > Potentially bma220_init() and bma220_deinit() may return positive codes. > > > > Fix the logic to return proper error codes instead. > > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Hi Andy > > > > > > A nice straight forward set and I suspect we aren't going to get any other > > > reviews, hence... > > > > > > Series applied to the togreg branch of iio.git and pushed out as testing for > > > the autobuilders to poke at it. > > > > Thanks! > > > > P.S. Consider this series as an example what can be done to many IIO drivers > > in order to clean them up. My focus, of course, ACPI interaction: > > - use of ACPI_PTR / ifdeffery > > - inclusion of acpi.h > > - ... > > > > Thanks. This is probably a good one for anyone who wants to grow their > experience in kernel patches etc. I'll add it to my more or less > never ending list if not and get to it eventually. > > In the meantime we'll try to avoid introducing any new variants! > > Jonathan Andy, one thing that might want adjusting is the docs that suggest doing ACPI_PTR and ifdeffery. https://elixir.bootlin.com/linux/v5.9-rc3/source/Documentation/firmware-guide/acpi/enumeration.rst#L254 J
On Thu, Sep 3, 2020 at 11:23 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Mon, 31 Aug 2020 15:12:01 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 31 Aug 2020 14:49:04 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Aug 31, 2020 at 10:21:45AM +0100, Jonathan Cameron wrote: ... > > > P.S. Consider this series as an example what can be done to many IIO drivers > > > in order to clean them up. My focus, of course, ACPI interaction: > > > - use of ACPI_PTR / ifdeffery > > > - inclusion of acpi.h > > > - ... > > Thanks. This is probably a good one for anyone who wants to grow their > > experience in kernel patches etc. I'll add it to my more or less > > never ending list if not and get to it eventually. > > > > In the meantime we'll try to avoid introducing any new variants! > Andy, one thing that might want adjusting is the docs that suggest > doing ACPI_PTR and ifdeffery. > > https://elixir.bootlin.com/linux/v5.9-rc3/source/Documentation/firmware-guide/acpi/enumeration.rst#L254 I briefly looked at the text and it seems that there is not one problem (typo, etc) in it. The entire document needs to be revisited. Unfortunately I have no time right now to do that. Regarding ACPI_PTR() it looks like case by case, because in the example you pointed out it's just a style preference (and somebody may actually want to save few dozen of bytes in their driver to reduce memory footprint). That said, we rather need to describe both options and tell the difference.
diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c index da8b36cc8628..3247b9c8abcb 100644 --- a/drivers/iio/accel/bma220_spi.c +++ b/drivers/iio/accel/bma220_spi.c @@ -198,10 +198,12 @@ static int bma220_init(struct spi_device *spi) /* Make sure the chip is powered on */ ret = bma220_read_reg(spi, BMA220_REG_SUSPEND); + if (ret == BMA220_SUSPEND_WAKE) + ret = bma220_read_reg(spi, BMA220_REG_SUSPEND); if (ret < 0) return ret; - else if (ret == BMA220_SUSPEND_WAKE) - return bma220_read_reg(spi, BMA220_REG_SUSPEND); + if (ret == BMA220_SUSPEND_WAKE) + return -EBUSY; return 0; } @@ -212,10 +214,12 @@ static int bma220_deinit(struct spi_device *spi) /* Make sure the chip is powered off */ ret = bma220_read_reg(spi, BMA220_REG_SUSPEND); + if (ret == BMA220_SUSPEND_SLEEP) + ret = bma220_read_reg(spi, BMA220_REG_SUSPEND); if (ret < 0) return ret; - else if (ret == BMA220_SUSPEND_SLEEP) - return bma220_read_reg(spi, BMA220_REG_SUSPEND); + if (ret == BMA220_SUSPEND_SLEEP) + return -EBUSY; return 0; } @@ -245,7 +249,7 @@ static int bma220_probe(struct spi_device *spi) indio_dev->available_scan_masks = bma220_accel_scan_masks; ret = bma220_init(data->spi_device); - if (ret < 0) + if (ret) return ret; ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
Potentially bma220_init() and bma220_deinit() may return positive codes. Fix the logic to return proper error codes instead. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/iio/accel/bma220_spi.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)