[PATCH v1 1/8] iio: accel: bma220: Fix returned codes from bma220_init(), bma220_deinit()

[PATCH v1 1/8] iio: accel: bma220: Fix returned codes from bma220_init(), bma220_deinit()

[Andy Shevchenko]

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(-)

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,
-- 
2.28.0

[PATCH v1 2/8] iio: accel: bma220: Convert to use ->read_avail()

[Andy Shevchenko]

Convert to use ->read_avail() instead of open-coded attribute handling.

While here, fix the typo in array definition and append comma in case of
the future extension.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/bma220_spi.c | 36 ++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index 3247b9c8abcb..cb634a3a995d 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -30,7 +30,6 @@
 #define BMA220_SUSPEND_WAKE			0x00
 
 #define BMA220_DEVICE_NAME			"bma220"
-#define BMA220_SCALE_AVAILABLE			"0.623 1.248 2.491 4.983"
 
 #define BMA220_ACCEL_CHANNEL(index, reg, axis) {			\
 	.type = IIO_ACCEL,						\
@@ -55,19 +54,8 @@ enum bma220_axis {
 	AXIS_Z,
 };
 
-static IIO_CONST_ATTR(in_accel_scale_available, BMA220_SCALE_AVAILABLE);
-
-static struct attribute *bma220_attributes[] = {
-	&iio_const_attr_in_accel_scale_available.dev_attr.attr,
-	NULL,
-};
-
-static const struct attribute_group bma220_attribute_group = {
-	.attrs = bma220_attributes,
-};
-
-static const int bma220_scale_table[][4] = {
-	{0, 623000}, {1, 248000}, {2, 491000}, {4, 983000}
+static const int bma220_scale_table[][2] = {
+	{0, 623000}, {1, 248000}, {2, 491000}, {4, 983000},
 };
 
 struct bma220_data {
@@ -182,10 +170,28 @@ static int bma220_write_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int bma220_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	struct bma220_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (int *)bma220_scale_table;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*length = ARRAY_SIZE(bma220_scale_table) * 2;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct iio_info bma220_info = {
 	.read_raw		= bma220_read_raw,
 	.write_raw		= bma220_write_raw,
-	.attrs			= &bma220_attribute_group,
+	.read_avail		= bma220_read_avail,
 };
 
 static int bma220_init(struct spi_device *spi)
-- 
2.28.0

[PATCH v1 3/8] iio: accel: bma220: Use dev_get_drvdata() directly

[Andy Shevchenko]

Instead of using to_spi_dev() + spi_get_drvdata(),
use dev_get_drvdata() to make code simpler.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/bma220_spi.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index cb634a3a995d..529c88fc2949 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -291,8 +291,7 @@ static int bma220_remove(struct spi_device *spi)
 #ifdef CONFIG_PM_SLEEP
 static int bma220_suspend(struct device *dev)
 {
-	struct bma220_data *data =
-			iio_priv(spi_get_drvdata(to_spi_device(dev)));
+	struct bma220_data *data = iio_priv(dev_get_drvdata(dev));
 
 	/* The chip can be suspended/woken up by a simple register read. */
 	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
@@ -300,8 +299,7 @@ static int bma220_suspend(struct device *dev)
 
 static int bma220_resume(struct device *dev)
 {
-	struct bma220_data *data =
-			iio_priv(spi_get_drvdata(to_spi_device(dev)));
+	struct bma220_data *data = iio_priv(dev_get_drvdata(dev));
 
 	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
 }
-- 
2.28.0

[PATCH v1 4/8] iio: accel: bma220: Mark PM functions as __maybe_unused

[Andy Shevchenko]

Instead of using ugly ifdeffery, mark PM functions as __maybe_unused.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/bma220_spi.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index 529c88fc2949..e55c5f5dd6b4 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -288,8 +288,7 @@ static int bma220_remove(struct spi_device *spi)
 	return bma220_deinit(spi);
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int bma220_suspend(struct device *dev)
+static __maybe_unused int bma220_suspend(struct device *dev)
 {
 	struct bma220_data *data = iio_priv(dev_get_drvdata(dev));
 
@@ -297,7 +296,7 @@ static int bma220_suspend(struct device *dev)
 	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
 }
 
-static int bma220_resume(struct device *dev)
+static __maybe_unused int bma220_resume(struct device *dev)
 {
 	struct bma220_data *data = iio_priv(dev_get_drvdata(dev));
 
@@ -306,11 +305,6 @@ static int bma220_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(bma220_pm_ops, bma220_suspend, bma220_resume);
 
-#define BMA220_PM_OPS (&bma220_pm_ops)
-#else
-#define BMA220_PM_OPS NULL
-#endif
-
 static const struct spi_device_id bma220_spi_id[] = {
 	{"bma220", 0},
 	{}
@@ -328,7 +322,7 @@ MODULE_DEVICE_TABLE(spi, bma220_spi_id);
 static struct spi_driver bma220_driver = {
 	.driver = {
 		.name = "bma220_spi",
-		.pm = BMA220_PM_OPS,
+		.pm = &bma220_pm_ops,
 		.acpi_match_table = ACPI_PTR(bma220_acpi_id),
 	},
 	.probe =            bma220_probe,
-- 
2.28.0

[PATCH v1 5/8] iio: accel: bma220: Drop ACPI_PTR() and accompanying ifdeffery

[Andy Shevchenko]

The driver is quite likely used only on ACPI based platforms and
rarely build with CONFIG_ACPI=n. Even though, the few dozens of bytes
is better than ugly ifdeffery and inclusion of heavy header.

As a result, replace acpi.h with mod_devicetable.h.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/bma220_spi.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index e55c5f5dd6b4..105c1dece890 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -5,8 +5,8 @@
  * Copyright (c) 2016, Intel Corporation.
  */
 
-#include <linux/acpi.h>
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
@@ -310,20 +310,18 @@ static const struct spi_device_id bma220_spi_id[] = {
 	{}
 };
 
-#ifdef CONFIG_ACPI
 static const struct acpi_device_id bma220_acpi_id[] = {
 	{"BMA0220", 0},
 	{}
 };
 
 MODULE_DEVICE_TABLE(spi, bma220_spi_id);
-#endif
 
 static struct spi_driver bma220_driver = {
 	.driver = {
 		.name = "bma220_spi",
 		.pm = &bma220_pm_ops,
-		.acpi_match_table = ACPI_PTR(bma220_acpi_id),
+		.acpi_match_table = bma220_acpi_id,
 	},
 	.probe =            bma220_probe,
 	.remove =           bma220_remove,
-- 
2.28.0

[PATCH v1 6/8] iio: accel: bma220: Group IIO headers together

[Andy Shevchenko]

Group IIO headers together and follow the common pattern of header inclusion,
i.e. more generic first.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/bma220_spi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index 105c1dece890..1a2cf486e546 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -8,10 +8,11 @@
 #include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/spi/spi.h>
+
 #include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include <linux/spi/spi.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 
-- 
2.28.0

[PATCH v1 7/8] iio: accel: bma220: Use BIT() and GENMASK() macros

[Andy Shevchenko]

Code is better to read when numbers of bit are explicitly used.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/bma220_spi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index 1a2cf486e546..a766a0e13efb 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2016, Intel Corporation.
  */
 
+#include <linux/bits.h>
 #include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -24,8 +25,8 @@
 #define BMA220_REG_SUSPEND			0x18
 
 #define BMA220_CHIP_ID				0xDD
-#define BMA220_READ_MASK			0x80
-#define BMA220_RANGE_MASK			0x03
+#define BMA220_READ_MASK			BIT(7)
+#define BMA220_RANGE_MASK			GENMASK(1, 0)
 #define BMA220_DATA_SHIFT			2
 #define BMA220_SUSPEND_SLEEP			0xFF
 #define BMA220_SUSPEND_WAKE			0x00
-- 
2.28.0

[PATCH v1 8/8] iio: accel: bma220: Remove unneeded blank lines

[Andy Shevchenko]

There are few blank lines that split structure definitions
with their users. Remove them to increase readability.

While here, update copyright year.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/accel/bma220_spi.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index a766a0e13efb..4a8862ba366c 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -2,7 +2,7 @@
 /**
  * BMA220 Digital triaxial acceleration sensor driver
  *
- * Copyright (c) 2016, Intel Corporation.
+ * Copyright (c) 2016,2020 Intel Corporation.
  */
 
 #include <linux/bits.h>
@@ -304,7 +304,6 @@ static __maybe_unused int bma220_resume(struct device *dev)
 
 	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
 }
-
 static SIMPLE_DEV_PM_OPS(bma220_pm_ops, bma220_suspend, bma220_resume);
 
 static const struct spi_device_id bma220_spi_id[] = {
@@ -316,7 +315,6 @@ static const struct acpi_device_id bma220_acpi_id[] = {
 	{"BMA0220", 0},
 	{}
 };
-
 MODULE_DEVICE_TABLE(spi, bma220_spi_id);
 
 static struct spi_driver bma220_driver = {
@@ -329,7 +327,6 @@ static struct spi_driver bma220_driver = {
 	.remove =           bma220_remove,
 	.id_table =         bma220_spi_id,
 };
-
 module_spi_driver(bma220_driver);
 
 MODULE_AUTHOR("Tiberiu Breana <tiberiu.a.breana@intel.com>");
-- 
2.28.0

Re: [PATCH v1 5/8] iio: accel: bma220: Drop ACPI_PTR() and accompanying ifdeffery

[Jonathan Cameron]

On Mon, 31 Aug 2020 12:08:10 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> The driver is quite likely used only on ACPI based platforms and
> rarely build with CONFIG_ACPI=n. Even though, the few dozens of bytes
> is better than ugly ifdeffery and inclusion of heavy header.

Given this part is readily available on maker type break out boards
I doubt it is mostly ACPI.  Rest of the comment is fine though.

Jonathan


> 
> As a result, replace acpi.h with mod_devicetable.h.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iio/accel/bma220_spi.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
> index e55c5f5dd6b4..105c1dece890 100644
> --- a/drivers/iio/accel/bma220_spi.c
> +++ b/drivers/iio/accel/bma220_spi.c
> @@ -5,8 +5,8 @@
>   * Copyright (c) 2016, Intel Corporation.
>   */
>  
> -#include <linux/acpi.h>
>  #include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> @@ -310,20 +310,18 @@ static const struct spi_device_id bma220_spi_id[] = {
>  	{}
>  };
>  
> -#ifdef CONFIG_ACPI
>  static const struct acpi_device_id bma220_acpi_id[] = {
>  	{"BMA0220", 0},
>  	{}
>  };
>  
>  MODULE_DEVICE_TABLE(spi, bma220_spi_id);
> -#endif
>  
>  static struct spi_driver bma220_driver = {
>  	.driver = {
>  		.name = "bma220_spi",
>  		.pm = &bma220_pm_ops,
> -		.acpi_match_table = ACPI_PTR(bma220_acpi_id),
> +		.acpi_match_table = bma220_acpi_id,
>  	},
>  	.probe =            bma220_probe,
>  	.remove =           bma220_remove,

Re: [PATCH v1 1/8] iio: accel: bma220: Fix returned codes from bma220_init(), bma220_deinit()

[Jonathan Cameron]

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,

Re: [PATCH v1 5/8] iio: accel: bma220: Drop ACPI_PTR() and accompanying ifdeffery

[Andy Shevchenko]

On Mon, Aug 31, 2020 at 12:19 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 31 Aug 2020 12:08:10 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > The driver is quite likely used only on ACPI based platforms and
> > rarely build with CONFIG_ACPI=n. Even though, the few dozens of bytes
> > is better than ugly ifdeffery and inclusion of heavy header.
>
> Given this part is readily available on maker type break out boards
> I doubt it is mostly ACPI.  Rest of the comment is fine though.

In-kernel use seems only ACPI (I grepped for pure SPI driver by name
and didn't find anything).
I hope we will have DT support for that as well (basically means
adding OF ID table).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/8] iio: accel: bma220: Drop ACPI_PTR() and accompanying ifdeffery

[Jonathan Cameron]

On Mon, 31 Aug 2020 12:39:10 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Aug 31, 2020 at 12:19 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 31 Aug 2020 12:08:10 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >  
> > > The driver is quite likely used only on ACPI based platforms and
> > > rarely build with CONFIG_ACPI=n. Even though, the few dozens of bytes
> > > is better than ugly ifdeffery and inclusion of heavy header.  
> >
> > Given this part is readily available on maker type break out boards
> > I doubt it is mostly ACPI.  Rest of the comment is fine though.  
> 
> In-kernel use seems only ACPI (I grepped for pure SPI driver by name
> and didn't find anything).
> I hope we will have DT support for that as well (basically means
> adding OF ID table).
> 

Good to add the explicit DT support, but I think the nasty fallback is
still in place in which the old style ID table is used if we get a match
without manufacturer ID.

So should be possible to instantiate it from DT even without the table.

Jonathan

Re: [PATCH v1 5/8] iio: accel: bma220: Drop ACPI_PTR() and accompanying ifdeffery

[Andy Shevchenko]

On Mon, Aug 31, 2020 at 10:55:04AM +0100, Jonathan Cameron wrote:
> On Mon, 31 Aug 2020 12:39:10 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Mon, Aug 31, 2020 at 12:19 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > On Mon, 31 Aug 2020 12:08:10 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > >  
> > > > The driver is quite likely used only on ACPI based platforms and
> > > > rarely build with CONFIG_ACPI=n. Even though, the few dozens of bytes
> > > > is better than ugly ifdeffery and inclusion of heavy header.  
> > >
> > > Given this part is readily available on maker type break out boards
> > > I doubt it is mostly ACPI.  Rest of the comment is fine though.  
> > 
> > In-kernel use seems only ACPI (I grepped for pure SPI driver by name
> > and didn't find anything).
> > I hope we will have DT support for that as well (basically means
> > adding OF ID table).
> > 
> 
> Good to add the explicit DT support, but I think the nasty fallback is
> still in place in which the old style ID table is used if we get a match
> without manufacturer ID.
> 
> So should be possible to instantiate it from DT even without the table.

True, but as I said, there is no evidence that there is a single user in kernel.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/8] iio: accel: bma220: Fix returned codes from bma220_init(), bma220_deinit()

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/8] iio: accel: bma220: Fix returned codes from bma220_init(), bma220_deinit()

[Jonathan Cameron]

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

Re: [PATCH v1 1/8] iio: accel: bma220: Fix returned codes from bma220_init(), bma220_deinit()

[Jonathan Cameron]

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

Re: [PATCH v1 1/8] iio: accel: bma220: Fix returned codes from bma220_init(), bma220_deinit()

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/8] iio: accel: bma220: Convert to use ->read_avail()

[Jonathan Cameron]

On Mon, 31 Aug 2020 12:08:07 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Convert to use ->read_avail() instead of open-coded attribute handling.
> 
> While here, fix the typo in array definition and append comma in case of
> the future extension.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I missed a build warning inline. Now fixed.

> ---
>  drivers/iio/accel/bma220_spi.c | 36 ++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
> index 3247b9c8abcb..cb634a3a995d 100644
> --- a/drivers/iio/accel/bma220_spi.c
> +++ b/drivers/iio/accel/bma220_spi.c
> @@ -30,7 +30,6 @@
>  #define BMA220_SUSPEND_WAKE			0x00
>  
>  #define BMA220_DEVICE_NAME			"bma220"
> -#define BMA220_SCALE_AVAILABLE			"0.623 1.248 2.491 4.983"
>  
>  #define BMA220_ACCEL_CHANNEL(index, reg, axis) {			\
>  	.type = IIO_ACCEL,						\
> @@ -55,19 +54,8 @@ enum bma220_axis {
>  	AXIS_Z,
>  };
>  
> -static IIO_CONST_ATTR(in_accel_scale_available, BMA220_SCALE_AVAILABLE);
> -
> -static struct attribute *bma220_attributes[] = {
> -	&iio_const_attr_in_accel_scale_available.dev_attr.attr,
> -	NULL,
> -};
> -
> -static const struct attribute_group bma220_attribute_group = {
> -	.attrs = bma220_attributes,
> -};
> -
> -static const int bma220_scale_table[][4] = {
> -	{0, 623000}, {1, 248000}, {2, 491000}, {4, 983000}
> +static const int bma220_scale_table[][2] = {
> +	{0, 623000}, {1, 248000}, {2, 491000}, {4, 983000},
>  };
>  
>  struct bma220_data {
> @@ -182,10 +170,28 @@ static int bma220_write_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> +static int bma220_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long mask)
> +{
> +	struct bma220_data *data = iio_priv(indio_dev);
Not used.

> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (int *)bma220_scale_table;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		*length = ARRAY_SIZE(bma220_scale_table) * 2;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static const struct iio_info bma220_info = {
>  	.read_raw		= bma220_read_raw,
>  	.write_raw		= bma220_write_raw,
> -	.attrs			= &bma220_attribute_group,
> +	.read_avail		= bma220_read_avail,
>  };
>  
>  static int bma220_init(struct spi_device *spi)

Re: [PATCH v1 2/8] iio: accel: bma220: Convert to use ->read_avail()

[Andy Shevchenko]

On Thu, Sep 3, 2020 at 9:36 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 31 Aug 2020 12:08:07 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > Convert to use ->read_avail() instead of open-coded attribute handling.
> >
> > While here, fix the typo in array definition and append comma in case of
> > the future extension.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> I missed a build warning inline. Now fixed.

Thanks!

-- 
With Best Regards,
Andy Shevchenko