[PATCH V2 0/6] iio: accel: bmi088: support BMI085 BMI090L

[PATCH V2 0/6] iio: accel: bmi088: support BMI085 BMI090L

[LI Qingwu]

Modified the units after application of scale from 100*m/s^2 to m/s^2,
since the units in the ABI documents are m/s^2.
Add supports for the BMI085 accelerometer.
Add supports for the BMI090L accelerometer.
Make it possible to config scales.

Change in V2:

Use the correct bits only for `reg` to make sure it is in the correct range.
Remove the unused 'name' parameter.
Modified the compatible string for bmi088 and bmi090l in and binding document.
Reorder the commits.


LI Qingwu (6):
  iio: accel: bmi088: Modified the scale calculate
  iio: accel: bmi088: Make it possible to config scales
  iio: accel: bmi088: modified the device name
  iio: accel: bmi088: Add support for bmi085 accel
  iio: accel: bmi088: Add support for bmi090l accel
  dt-bindings: iio: accel: Add bmi085 and bmi090l bindings

 .../bindings/iio/accel/bosch,bmi088.yaml      |  2 +
 drivers/iio/accel/bmi088-accel-core.c         | 64 ++++++++++++++++---
 drivers/iio/accel/bmi088-accel-spi.c          |  6 +-
 drivers/iio/accel/bmi088-accel.h              |  2 +-
 4 files changed, 61 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH V2 1/6] iio: accel: bmi088: Modified the scale calculate

[LI Qingwu]

The units after application of scale are 100*m/s^2,
The scale calculation is only for the device
with the range of 3, 6, 12, and 24g,
but some other chips have a range of 2, 4, 6, and 8g.

Modified the formula to a scale list.
The scales in the list are calculated by 1/sensitivity*9.8.
The new units after the application of scale are m/s^2.

Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/bmi088-accel-core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
index a06dae5c971d..9300313b63cb 100644
--- a/drivers/iio/accel/bmi088-accel-core.c
+++ b/drivers/iio/accel/bmi088-accel-core.c
@@ -119,6 +119,7 @@ struct bmi088_accel_chip_info {
 	u8 chip_id;
 	const struct iio_chan_spec *channels;
 	int num_channels;
+	const int scale_table[4][2];
 };
 
 struct bmi088_accel_data {
@@ -280,6 +281,7 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
 	struct bmi088_accel_data *data = iio_priv(indio_dev);
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
+	int reg;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -330,13 +332,12 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
 				return ret;
 
 			ret = regmap_read(data->regmap,
-					  BMI088_ACCEL_REG_ACC_RANGE, val);
+					  BMI088_ACCEL_REG_ACC_RANGE, &reg);
 			if (ret)
 				goto out_read_raw_pm_put;
-
-			*val2 = 15 - (*val & 0x3);
-			*val = 3 * 980;
-			ret = IIO_VAL_FRACTIONAL_LOG2;
+			*val = data->chip_info->scale_table[reg&0x03][0];
+			*val2 = data->chip_info->scale_table[reg&0x03][1];
+			ret = IIO_VAL_INT_PLUS_MICRO;
 
 			goto out_read_raw_pm_put;
 		default:
@@ -432,6 +433,7 @@ static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
 		.chip_id = 0x1E,
 		.channels = bmi088_accel_channels,
 		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
+		.scale_table = {{0, 897}, {0, 1795}, {0, 3590}, {0, 7179}},
 	},
 };
 
-- 
2.25.1

Re: [PATCH V2 1/6] iio: accel: bmi088: Modified the scale calculate

[Alexandru Ardelean]

On Tue, May 10, 2022 at 5:17 PM LI Qingwu
<Qing-wu.Li@leica-geosystems.com.cn> wrote:
>
> The units after application of scale are 100*m/s^2,
> The scale calculation is only for the device
> with the range of 3, 6, 12, and 24g,
> but some other chips have a range of 2, 4, 6, and 8g.
>
> Modified the formula to a scale list.
> The scales in the list are calculated by 1/sensitivity*9.8.
> The new units after the application of scale are m/s^2.
>

Strictly on the code:

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

On the change of IIO_VAL_FRACTIONAL_LOG2 -> IIO_VAL_INT_PLUS_MICRO  is
still up for discussion.

> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/iio/accel/bmi088-accel-core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> index a06dae5c971d..9300313b63cb 100644
> --- a/drivers/iio/accel/bmi088-accel-core.c
> +++ b/drivers/iio/accel/bmi088-accel-core.c
> @@ -119,6 +119,7 @@ struct bmi088_accel_chip_info {
>         u8 chip_id;
>         const struct iio_chan_spec *channels;
>         int num_channels;
> +       const int scale_table[4][2];
>  };
>
>  struct bmi088_accel_data {
> @@ -280,6 +281,7 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
>         struct bmi088_accel_data *data = iio_priv(indio_dev);
>         struct device *dev = regmap_get_device(data->regmap);
>         int ret;
> +       int reg;
>
>         switch (mask) {
>         case IIO_CHAN_INFO_RAW:
> @@ -330,13 +332,12 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
>                                 return ret;
>
>                         ret = regmap_read(data->regmap,
> -                                         BMI088_ACCEL_REG_ACC_RANGE, val);
> +                                         BMI088_ACCEL_REG_ACC_RANGE, &reg);
>                         if (ret)
>                                 goto out_read_raw_pm_put;
> -
> -                       *val2 = 15 - (*val & 0x3);
> -                       *val = 3 * 980;
> -                       ret = IIO_VAL_FRACTIONAL_LOG2;
> +                       *val = data->chip_info->scale_table[reg&0x03][0];
> +                       *val2 = data->chip_info->scale_table[reg&0x03][1];
> +                       ret = IIO_VAL_INT_PLUS_MICRO;
>
>                         goto out_read_raw_pm_put;
>                 default:
> @@ -432,6 +433,7 @@ static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
>                 .chip_id = 0x1E,
>                 .channels = bmi088_accel_channels,
>                 .num_channels = ARRAY_SIZE(bmi088_accel_channels),
> +               .scale_table = {{0, 897}, {0, 1795}, {0, 3590}, {0, 7179}},
>         },
>  };
>
> --
> 2.25.1
>

Re: [PATCH V2 1/6] iio: accel: bmi088: Modified the scale calculate

[Jonathan Cameron]

On Thu, 12 May 2022 10:14:47 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Tue, May 10, 2022 at 5:17 PM LI Qingwu
> <Qing-wu.Li@leica-geosystems.com.cn> wrote:
> >
> > The units after application of scale are 100*m/s^2,
> > The scale calculation is only for the device
> > with the range of 3, 6, 12, and 24g,
> > but some other chips have a range of 2, 4, 6, and 8g.
> >
> > Modified the formula to a scale list.
> > The scales in the list are calculated by 1/sensitivity*9.8.
> > The new units after the application of scale are m/s^2.
> >  
> 
> Strictly on the code:
> 
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> 
> On the change of IIO_VAL_FRACTIONAL_LOG2 -> IIO_VAL_INT_PLUS_MICRO  is
> still up for discussion.
It's fine as far as I'm concerned.

Jonathan

> 
> > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> > ---
> >  drivers/iio/accel/bmi088-accel-core.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> > index a06dae5c971d..9300313b63cb 100644
> > --- a/drivers/iio/accel/bmi088-accel-core.c
> > +++ b/drivers/iio/accel/bmi088-accel-core.c
> > @@ -119,6 +119,7 @@ struct bmi088_accel_chip_info {
> >         u8 chip_id;
> >         const struct iio_chan_spec *channels;
> >         int num_channels;
> > +       const int scale_table[4][2];
> >  };
> >
> >  struct bmi088_accel_data {
> > @@ -280,6 +281,7 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
> >         struct bmi088_accel_data *data = iio_priv(indio_dev);
> >         struct device *dev = regmap_get_device(data->regmap);
> >         int ret;
> > +       int reg;
> >
> >         switch (mask) {
> >         case IIO_CHAN_INFO_RAW:
> > @@ -330,13 +332,12 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
> >                                 return ret;
> >
> >                         ret = regmap_read(data->regmap,
> > -                                         BMI088_ACCEL_REG_ACC_RANGE, val);
> > +                                         BMI088_ACCEL_REG_ACC_RANGE, &reg);
> >                         if (ret)
> >                                 goto out_read_raw_pm_put;
> > -
> > -                       *val2 = 15 - (*val & 0x3);
> > -                       *val = 3 * 980;
> > -                       ret = IIO_VAL_FRACTIONAL_LOG2;
> > +                       *val = data->chip_info->scale_table[reg&0x03][0];
> > +                       *val2 = data->chip_info->scale_table[reg&0x03][1];
> > +                       ret = IIO_VAL_INT_PLUS_MICRO;
> >
> >                         goto out_read_raw_pm_put;
> >                 default:
> > @@ -432,6 +433,7 @@ static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
> >                 .chip_id = 0x1E,
> >                 .channels = bmi088_accel_channels,
> >                 .num_channels = ARRAY_SIZE(bmi088_accel_channels),
> > +               .scale_table = {{0, 897}, {0, 1795}, {0, 3590}, {0, 7179}},
> >         },
> >  };
> >
> > --
> > 2.25.1
> >  

Re: [PATCH V2 1/6] iio: accel: bmi088: Modified the scale calculate

[Jonathan Cameron]

On Tue, 10 May 2022 14:17:48 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> The units after application of scale are 100*m/s^2,
> The scale calculation is only for the device
> with the range of 3, 6, 12, and 24g,
> but some other chips have a range of 2, 4, 6, and 8g.
> 
> Modified the formula to a scale list.
> The scales in the list are calculated by 1/sensitivity*9.8.
> The new units after the application of scale are m/s^2.
> 
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/iio/accel/bmi088-accel-core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> index a06dae5c971d..9300313b63cb 100644
> --- a/drivers/iio/accel/bmi088-accel-core.c
> +++ b/drivers/iio/accel/bmi088-accel-core.c
> @@ -119,6 +119,7 @@ struct bmi088_accel_chip_info {
>  	u8 chip_id;
>  	const struct iio_chan_spec *channels;
>  	int num_channels;
> +	const int scale_table[4][2];
>  };
>  
>  struct bmi088_accel_data {
> @@ -280,6 +281,7 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
>  	struct bmi088_accel_data *data = iio_priv(indio_dev);
>  	struct device *dev = regmap_get_device(data->regmap);
>  	int ret;
> +	int reg;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -330,13 +332,12 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
>  				return ret;
>  
>  			ret = regmap_read(data->regmap,
> -					  BMI088_ACCEL_REG_ACC_RANGE, val);
> +					  BMI088_ACCEL_REG_ACC_RANGE, &reg);
>  			if (ret)
>  				goto out_read_raw_pm_put;
> -
> -			*val2 = 15 - (*val & 0x3);
> -			*val = 3 * 980;
> -			ret = IIO_VAL_FRACTIONAL_LOG2;
> +			*val = data->chip_info->scale_table[reg&0x03][0];
> +			*val2 = data->chip_info->scale_table[reg&0x03][1];

Spaces needed around the &
The 0x03 should be a define - something like BMIO088_ACCEL_ACC_RANGE_MSK

Also, this driver doesn't yet use FIELD_PREP() / FIELD_GET()
but this is a good example of where using FIELD_GET() will make the logic
clearer.


> +			ret = IIO_VAL_INT_PLUS_MICRO;
>  
>  			goto out_read_raw_pm_put;
>  		default:
> @@ -432,6 +433,7 @@ static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
>  		.chip_id = 0x1E,
>  		.channels = bmi088_accel_channels,
>  		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
> +		.scale_table = {{0, 897}, {0, 1795}, {0, 3590}, {0, 7179}},
>  	},
>  };
>  

[PATCH V2 2/6] iio: accel: bmi088: Make it possible to config scales

[LI Qingwu]

The sensor can set the scales by writing the range register 0x41,
The current driver has no interface to configure it.
The commit adds the interface for config the scales.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/bmi088-accel-core.c | 32 ++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
index 9300313b63cb..8fee1d02e773 100644
--- a/drivers/iio/accel/bmi088-accel-core.c
+++ b/drivers/iio/accel/bmi088-accel-core.c
@@ -237,6 +237,21 @@ static int bmi088_accel_set_sample_freq(struct bmi088_accel_data *data, int val)
 				  BMI088_ACCEL_MODE_ODR_MASK, regval);
 }
 
+static int bmi088_accel_set_scale(struct bmi088_accel_data *data, int val, int val2)
+{
+	unsigned int i;
+
+	for (i = 0; i < 4; i++)
+		if (val == data->chip_info->scale_table[i][0] &&
+		    val2 == data->chip_info->scale_table[i][1])
+			break;
+
+	if (i >= 4)
+		return -EINVAL;
+
+	return regmap_write(data->regmap, BMI088_ACCEL_REG_ACC_RANGE, i);
+}
+
 static int bmi088_accel_get_temp(struct bmi088_accel_data *data, int *val)
 {
 	int ret;
@@ -368,7 +383,13 @@ static int bmi088_accel_read_avail(struct iio_dev *indio_dev,
 			     const int **vals, int *type, int *length,
 			     long mask)
 {
+	struct bmi088_accel_data *data = iio_priv(indio_dev);
 	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (const int *)data->chip_info->scale_table;
+		*length = 8;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*type = IIO_VAL_INT_PLUS_MICRO;
 		*vals = bmi088_sample_freqs;
@@ -388,6 +409,14 @@ static int bmi088_accel_write_raw(struct iio_dev *indio_dev,
 	int ret;
 
 	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret)
+			return ret;
+		ret = bmi088_accel_set_scale(data, val, val2);
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_put_autosuspend(dev);
+		return ret;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		ret = pm_runtime_resume_and_get(dev);
 		if (ret)
@@ -409,7 +438,8 @@ static int bmi088_accel_write_raw(struct iio_dev *indio_dev,
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
 				BIT(IIO_CHAN_INFO_SAMP_FREQ), \
-	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+				BIT(IIO_CHAN_INFO_SCALE), \
 	.scan_index = AXIS_##_axis, \
 }
 
-- 
2.25.1

Re: [PATCH V2 2/6] iio: accel: bmi088: Make it possible to config scales

[Jonathan Cameron]

On Tue, 10 May 2022 14:17:49 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> The sensor can set the scales by writing the range register 0x41,
> The current driver has no interface to configure it.
> The commit adds the interface for config the scales.
> 
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
Hi.

A few minor requested changes inline,

Thanks,

Jonathan

> ---
>  drivers/iio/accel/bmi088-accel-core.c | 32 ++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> index 9300313b63cb..8fee1d02e773 100644
> --- a/drivers/iio/accel/bmi088-accel-core.c
> +++ b/drivers/iio/accel/bmi088-accel-core.c
> @@ -237,6 +237,21 @@ static int bmi088_accel_set_sample_freq(struct bmi088_accel_data *data, int val)
>  				  BMI088_ACCEL_MODE_ODR_MASK, regval);
>  }
>  
> +static int bmi088_accel_set_scale(struct bmi088_accel_data *data, int val, int val2)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < 4; i++)
> +		if (val == data->chip_info->scale_table[i][0] &&
> +		    val2 == data->chip_info->scale_table[i][1])
> +			break;
> +
> +	if (i >= 4)
== 4

If it's > 4 something very odd happened :)

> +		return -EINVAL;
> +
> +	return regmap_write(data->regmap, BMI088_ACCEL_REG_ACC_RANGE, i);
> +}
> +
>  static int bmi088_accel_get_temp(struct bmi088_accel_data *data, int *val)
>  {
>  	int ret;
> @@ -368,7 +383,13 @@ static int bmi088_accel_read_avail(struct iio_dev *indio_dev,
>  			     const int **vals, int *type, int *length,
>  			     long mask)
>  {
> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
>  	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (const int *)data->chip_info->scale_table;
> +		*length = 8;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		*type = IIO_VAL_INT_PLUS_MICRO;
>  		*vals = bmi088_sample_freqs;
> @@ -388,6 +409,14 @@ static int bmi088_accel_write_raw(struct iio_dev *indio_dev,
>  	int ret;
>  
>  	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret)
> +			return ret;

Blank line here.  That separates a functional call and it's error handling from
what happens next and makes the code a tiny bit easier to read.

Also consistent with SAMP_FREQ block that follows.

> +		ret = bmi088_accel_set_scale(data, val, val2);
> +		pm_runtime_mark_last_busy(dev);
> +		pm_runtime_put_autosuspend(dev);
> +		return ret;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		ret = pm_runtime_resume_and_get(dev);
>  		if (ret)
> @@ -409,7 +438,8 @@ static int bmi088_accel_write_raw(struct iio_dev *indio_dev,
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>  				BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> -	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> +				BIT(IIO_CHAN_INFO_SCALE), \
>  	.scan_index = AXIS_##_axis, \
>  }
>  

[PATCH V2 3/6] iio: accel: bmi088: modified the device name

[LI Qingwu]

It is possible to have multiple sensors connected on the same platform,
To support multiple sensors, the commit makes it possible to obtain the
device name by reading the chip ID instead of the device-tree name.
To be compatible with previous versions, renam bmi088a to bmi088-accel.

Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/bmi088-accel-core.c | 6 +++---
 drivers/iio/accel/bmi088-accel-spi.c  | 4 +---
 drivers/iio/accel/bmi088-accel.h      | 2 +-
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
index 8fee1d02e773..de2385e4dad5 100644
--- a/drivers/iio/accel/bmi088-accel-core.c
+++ b/drivers/iio/accel/bmi088-accel-core.c
@@ -459,7 +459,7 @@ static const struct iio_chan_spec bmi088_accel_channels[] = {
 
 static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
 	[0] = {
-		.name = "bmi088a",
+		.name = "bmi088-accel",
 		.chip_id = 0x1E,
 		.channels = bmi088_accel_channels,
 		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
@@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct bmi088_accel_data *data)
 }
 
 int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
-	int irq, const char *name, bool block_supported)
+	int irq, bool block_supported)
 {
 	struct bmi088_accel_data *data;
 	struct iio_dev *indio_dev;
@@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
 
 	indio_dev->channels = data->chip_info->channels;
 	indio_dev->num_channels = data->chip_info->num_channels;
-	indio_dev->name = name ? name : data->chip_info->name;
+	indio_dev->name = data->chip_info->name;
 	indio_dev->available_scan_masks = bmi088_accel_scan_masks;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bmi088_accel_info;
diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c
index dd1e3f6cf211..0fed0081e1fd 100644
--- a/drivers/iio/accel/bmi088-accel-spi.c
+++ b/drivers/iio/accel/bmi088-accel-spi.c
@@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = {
 static int bmi088_accel_probe(struct spi_device *spi)
 {
 	struct regmap *regmap;
-	const struct spi_device_id *id = spi_get_device_id(spi);
 
 	regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus,
 			spi, &bmi088_regmap_conf);
@@ -52,8 +51,7 @@ static int bmi088_accel_probe(struct spi_device *spi)
 		return PTR_ERR(regmap);
 	}
 
-	return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, id->name,
-				       true);
+	return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, true);
 }
 
 static int bmi088_accel_remove(struct spi_device *spi)
diff --git a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h
index 5c25f16b672c..c32afe9606a8 100644
--- a/drivers/iio/accel/bmi088-accel.h
+++ b/drivers/iio/accel/bmi088-accel.h
@@ -12,7 +12,7 @@ extern const struct regmap_config bmi088_regmap_conf;
 extern const struct dev_pm_ops bmi088_accel_pm_ops;
 
 int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
-			    const char *name, bool block_supported);
+			    bool block_supported);
 int bmi088_accel_core_remove(struct device *dev);
 
 #endif /* BMI088_ACCEL_H */
-- 
2.25.1

Re: [PATCH V2 3/6] iio: accel: bmi088: modified the device name

[Alexandru Ardelean]

On Tue, May 10, 2022 at 5:18 PM LI Qingwu
<Qing-wu.Li@leica-geosystems.com.cn> wrote:
>
> It is possible to have multiple sensors connected on the same platform,
> To support multiple sensors, the commit makes it possible to obtain the
> device name by reading the chip ID instead of the device-tree name.
> To be compatible with previous versions, renam bmi088a to bmi088-accel.

// my spellcheck in GMail found this :p

typo: renam -> rename

I also have a comment about a duplication that is highlighted by this change.

You can disregard my comment about the duplication and leave this change as-is.

>
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/iio/accel/bmi088-accel-core.c | 6 +++---
>  drivers/iio/accel/bmi088-accel-spi.c  | 4 +---
>  drivers/iio/accel/bmi088-accel.h      | 2 +-
>  3 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> index 8fee1d02e773..de2385e4dad5 100644
> --- a/drivers/iio/accel/bmi088-accel-core.c
> +++ b/drivers/iio/accel/bmi088-accel-core.c
> @@ -459,7 +459,7 @@ static const struct iio_chan_spec bmi088_accel_channels[] = {
>
>  static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
>         [0] = {
> -               .name = "bmi088a",
> +               .name = "bmi088-accel",
>                 .chip_id = 0x1E,
>                 .channels = bmi088_accel_channels,
>                 .num_channels = ARRAY_SIZE(bmi088_accel_channels),
> @@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct bmi088_accel_data *data)
>  }
>
>  int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
> -       int irq, const char *name, bool block_supported)
> +       int irq, bool block_supported)
>  {
>         struct bmi088_accel_data *data;
>         struct iio_dev *indio_dev;
> @@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
>
>         indio_dev->channels = data->chip_info->channels;
>         indio_dev->num_channels = data->chip_info->num_channels;
> -       indio_dev->name = name ? name : data->chip_info->name;
> +       indio_dev->name = data->chip_info->name;

(with this change) i can better see, a bit of duplication between the
spi_device table and the chip_info table

this was not introduced by this change, but it was made a bit more
obvious by this change;

one way to address this, is to remove the `const char *name;` and
continue using the `name` provided as a parameter from
bmi088_accel_core_probe();
(apologies if I seem to have changed my mind (from the previous
changeset), but I did not see it too well before)

and we can convert

enum {
   ID_BMI088,
   ID_BMI085,
   ...
};

 static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
         [ID_BMI088] = {
                 .chip_id = 0x1E,
                 .channels = bmi088_accel_channels,
                .num_channels = ARRAY_SIZE(bmi088_accel_channels),
        },
         [ID_BMI085] = {
         ........

>         indio_dev->available_scan_masks = bmi088_accel_scan_masks;
>         indio_dev->modes = INDIO_DIRECT_MODE;
>         indio_dev->info = &bmi088_accel_info;
> diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c
> index dd1e3f6cf211..0fed0081e1fd 100644
> --- a/drivers/iio/accel/bmi088-accel-spi.c
> +++ b/drivers/iio/accel/bmi088-accel-spi.c
> @@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = {
>  static int bmi088_accel_probe(struct spi_device *spi)
>  {
>         struct regmap *regmap;
> -       const struct spi_device_id *id = spi_get_device_id(spi);
>
>         regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus,
>                         spi, &bmi088_regmap_conf);
> @@ -52,8 +51,7 @@ static int bmi088_accel_probe(struct spi_device *spi)
>                 return PTR_ERR(regmap);
>         }
>
> -       return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, id->name,
> -                                      true);
> +       return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, true);
>  }
>
>  static int bmi088_accel_remove(struct spi_device *spi)
> diff --git a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h
> index 5c25f16b672c..c32afe9606a8 100644
> --- a/drivers/iio/accel/bmi088-accel.h
> +++ b/drivers/iio/accel/bmi088-accel.h
> @@ -12,7 +12,7 @@ extern const struct regmap_config bmi088_regmap_conf;
>  extern const struct dev_pm_ops bmi088_accel_pm_ops;
>
>  int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
> -                           const char *name, bool block_supported);
> +                           bool block_supported);
>  int bmi088_accel_core_remove(struct device *dev);
>
>  #endif /* BMI088_ACCEL_H */
> --
> 2.25.1
>

RE: [PATCH V2 3/6] iio: accel: bmi088: modified the device name

[LI Qingwu]

> -----Original Message-----
> From: Alexandru Ardelean <ardeleanalex@gmail.com>
> Sent: Thursday, May 12, 2022 3:32 PM
> To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; mchehab+huawei@kernel.org; linux-iio
> <linux-iio@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Rob Herring
> <robh+dt@kernel.org>; Mike Looijmans <mike.looijmans@topic.nl>; devicetree
> <devicetree@vger.kernel.org>
> Subject: Re: [PATCH V2 3/6] iio: accel: bmi088: modified the device name
> 
> This email is not from Hexagon’s Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
> 
> 
> On Tue, May 10, 2022 at 5:18 PM LI Qingwu
> <Qing-wu.Li@leica-geosystems.com.cn> wrote:
> >
> > It is possible to have multiple sensors connected on the same
> > platform, To support multiple sensors, the commit makes it possible to
> > obtain the device name by reading the chip ID instead of the device-tree
> name.
> > To be compatible with previous versions, renam bmi088a to bmi088-accel.
> 
> // my spellcheck in GMail found this :p
> 
> typo: renam -> rename
> 
> I also have a comment about a duplication that is highlighted by this change.
> 
> You can disregard my comment about the duplication and leave this change
> as-is.
> 
> >
> > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> > ---
> >  drivers/iio/accel/bmi088-accel-core.c | 6 +++---
> > drivers/iio/accel/bmi088-accel-spi.c  | 4 +---
> >  drivers/iio/accel/bmi088-accel.h      | 2 +-
> >  3 files changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bmi088-accel-core.c
> > b/drivers/iio/accel/bmi088-accel-core.c
> > index 8fee1d02e773..de2385e4dad5 100644
> > --- a/drivers/iio/accel/bmi088-accel-core.c
> > +++ b/drivers/iio/accel/bmi088-accel-core.c
> > @@ -459,7 +459,7 @@ static const struct iio_chan_spec
> > bmi088_accel_channels[] = {
> >
> >  static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
> >         [0] = {
> > -               .name = "bmi088a",
> > +               .name = "bmi088-accel",
> >                 .chip_id = 0x1E,
> >                 .channels = bmi088_accel_channels,
> >                 .num_channels = ARRAY_SIZE(bmi088_accel_channels),
> > @@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct
> > bmi088_accel_data *data)  }
> >
> >  int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
> > -       int irq, const char *name, bool block_supported)
> > +       int irq, bool block_supported)
> >  {
> >         struct bmi088_accel_data *data;
> >         struct iio_dev *indio_dev;
> > @@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev,
> > struct regmap *regmap,
> >
> >         indio_dev->channels = data->chip_info->channels;
> >         indio_dev->num_channels = data->chip_info->num_channels;
> > -       indio_dev->name = name ? name : data->chip_info->name;
> > +       indio_dev->name = data->chip_info->name;
> 
> (with this change) i can better see, a bit of duplication between the spi_device
> table and the chip_info table
> 
> this was not introduced by this change, but it was made a bit more obvious by
> this change;
> 
> one way to address this, is to remove the `const char *name;` and continue
> using the `name` provided as a parameter from bmi088_accel_core_probe();
> (apologies if I seem to have changed my mind (from the previous changeset), but
> I did not see it too well before)
> 
> and we can convert
> 
> enum {
>    ID_BMI088,
>    ID_BMI085,
>    ...
> };
> 
>  static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
>          [ID_BMI088] = {
>                  .chip_id = 0x1E,
>                  .channels = bmi088_accel_channels,
>                 .num_channels = ARRAY_SIZE(bmi088_accel_channels),
>         },
>          [ID_BMI085] = {
>          ........
> 
Thanks Ardelean,

There is a case where some different sensors are connected to one system, 
For user space is nice if can detect which sensor is present, if using the name in spi_device table, 
the name may be inconsistent with the connected sensor. What's your opinion?


> >         indio_dev->available_scan_masks = bmi088_accel_scan_masks;
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >         indio_dev->info = &bmi088_accel_info; diff --git
> > a/drivers/iio/accel/bmi088-accel-spi.c
> > b/drivers/iio/accel/bmi088-accel-spi.c
> > index dd1e3f6cf211..0fed0081e1fd 100644
> > --- a/drivers/iio/accel/bmi088-accel-spi.c
> > +++ b/drivers/iio/accel/bmi088-accel-spi.c
> > @@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = {
> > static int bmi088_accel_probe(struct spi_device *spi)  {
> >         struct regmap *regmap;
> > -       const struct spi_device_id *id = spi_get_device_id(spi);
> >
> >         regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus,
> >                         spi, &bmi088_regmap_conf); @@ -52,8 +51,7
> @@
> > static int bmi088_accel_probe(struct spi_device *spi)
> >                 return PTR_ERR(regmap);
> >         }
> >
> > -       return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq,
> id->name,
> > -                                      true);
> > +       return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq,
> > + true);
> >  }
> >
> >  static int bmi088_accel_remove(struct spi_device *spi) diff --git
> > a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h
> > index 5c25f16b672c..c32afe9606a8 100644
> > --- a/drivers/iio/accel/bmi088-accel.h
> > +++ b/drivers/iio/accel/bmi088-accel.h
> > @@ -12,7 +12,7 @@ extern const struct regmap_config
> > bmi088_regmap_conf;  extern const struct dev_pm_ops
> > bmi088_accel_pm_ops;
> >
> >  int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
> int irq,
> > -                           const char *name, bool block_supported);
> > +                           bool block_supported);
> >  int bmi088_accel_core_remove(struct device *dev);
> >
> >  #endif /* BMI088_ACCEL_H */
> > --
> > 2.25.1
> >

Re: [PATCH V2 3/6] iio: accel: bmi088: modified the device name

[Alexandru Ardelean]

On Thu, May 12, 2022 at 7:18 PM LI Qingwu
<qing-wu.li@leica-geosystems.com.cn> wrote:
>
>
> > -----Original Message-----
> > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > Sent: Thursday, May 12, 2022 3:32 PM
> > To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> > <lars@metafoo.de>; mchehab+huawei@kernel.org; linux-iio
> > <linux-iio@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Rob Herring
> > <robh+dt@kernel.org>; Mike Looijmans <mike.looijmans@topic.nl>; devicetree
> > <devicetree@vger.kernel.org>
> > Subject: Re: [PATCH V2 3/6] iio: accel: bmi088: modified the device name
> >
> > This email is not from Hexagon’s Office 365 instance. Please be careful while
> > clicking links, opening attachments, or replying to this email.
> >
> >
> > On Tue, May 10, 2022 at 5:18 PM LI Qingwu
> > <Qing-wu.Li@leica-geosystems.com.cn> wrote:
> > >
> > > It is possible to have multiple sensors connected on the same
> > > platform, To support multiple sensors, the commit makes it possible to
> > > obtain the device name by reading the chip ID instead of the device-tree
> > name.
> > > To be compatible with previous versions, renam bmi088a to bmi088-accel.
> >
> > // my spellcheck in GMail found this :p
> >
> > typo: renam -> rename
> >
> > I also have a comment about a duplication that is highlighted by this change.
> >
> > You can disregard my comment about the duplication and leave this change
> > as-is.
> >
> > >
> > > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> > > ---
> > >  drivers/iio/accel/bmi088-accel-core.c | 6 +++---
> > > drivers/iio/accel/bmi088-accel-spi.c  | 4 +---
> > >  drivers/iio/accel/bmi088-accel.h      | 2 +-
> > >  3 files changed, 5 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/bmi088-accel-core.c
> > > b/drivers/iio/accel/bmi088-accel-core.c
> > > index 8fee1d02e773..de2385e4dad5 100644
> > > --- a/drivers/iio/accel/bmi088-accel-core.c
> > > +++ b/drivers/iio/accel/bmi088-accel-core.c
> > > @@ -459,7 +459,7 @@ static const struct iio_chan_spec
> > > bmi088_accel_channels[] = {
> > >
> > >  static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
> > >         [0] = {
> > > -               .name = "bmi088a",
> > > +               .name = "bmi088-accel",
> > >                 .chip_id = 0x1E,
> > >                 .channels = bmi088_accel_channels,
> > >                 .num_channels = ARRAY_SIZE(bmi088_accel_channels),
> > > @@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct
> > > bmi088_accel_data *data)  }
> > >
> > >  int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
> > > -       int irq, const char *name, bool block_supported)
> > > +       int irq, bool block_supported)
> > >  {
> > >         struct bmi088_accel_data *data;
> > >         struct iio_dev *indio_dev;
> > > @@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev,
> > > struct regmap *regmap,
> > >
> > >         indio_dev->channels = data->chip_info->channels;
> > >         indio_dev->num_channels = data->chip_info->num_channels;
> > > -       indio_dev->name = name ? name : data->chip_info->name;
> > > +       indio_dev->name = data->chip_info->name;
> >
> > (with this change) i can better see, a bit of duplication between the spi_device
> > table and the chip_info table
> >
> > this was not introduced by this change, but it was made a bit more obvious by
> > this change;
> >
> > one way to address this, is to remove the `const char *name;` and continue
> > using the `name` provided as a parameter from bmi088_accel_core_probe();
> > (apologies if I seem to have changed my mind (from the previous changeset), but
> > I did not see it too well before)
> >
> > and we can convert
> >
> > enum {
> >    ID_BMI088,
> >    ID_BMI085,
> >    ...
> > };
> >
> >  static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
> >          [ID_BMI088] = {
> >                  .chip_id = 0x1E,
> >                  .channels = bmi088_accel_channels,
> >                 .num_channels = ARRAY_SIZE(bmi088_accel_channels),
> >         },
> >          [ID_BMI085] = {
> >          ........
> >
> Thanks Ardelean,
>
> There is a case where some different sensors are connected to one system,
> For user space is nice if can detect which sensor is present, if using the name in spi_device table,
> the name may be inconsistent with the connected sensor. What's your opinion?

I'd let Jonathan have the last word on this.

Typically, what I've seen (in practice), that when you write a
device-tree for a board and you add (for example) BMI088 for board
RevA, but on RevB it is decided to go with BMI085, then some people
want to enforce this from the device-tree.

So, if in the DT someone specifies bmi088-accel (as compatible
string), then it could fail on probe if it detects bmi085-accel.
Right now, this isn't happening with the driver (as it is now).

On the other hand, some other people want this flexibility where if
you specify bmi088-accel, bmi085-accel or bmi090-accel, then they like
the interchange-ability at the device-tree level.

I don't know which is better. I'm not sure this was enforced (or maybe
I missed it).

But a more common practice (that I remember in some IIO drivers) is
that the chip_info table (or a chip_id value)  can be added to
spi_device_table.
When the DT tries to probe the driver and reads a different ID (from
SPI) it would fail the probe.

These are just some thoughts. Not a final conclusion.

>
>
> > >         indio_dev->available_scan_masks = bmi088_accel_scan_masks;
> > >         indio_dev->modes = INDIO_DIRECT_MODE;
> > >         indio_dev->info = &bmi088_accel_info; diff --git
> > > a/drivers/iio/accel/bmi088-accel-spi.c
> > > b/drivers/iio/accel/bmi088-accel-spi.c
> > > index dd1e3f6cf211..0fed0081e1fd 100644
> > > --- a/drivers/iio/accel/bmi088-accel-spi.c
> > > +++ b/drivers/iio/accel/bmi088-accel-spi.c
> > > @@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = {
> > > static int bmi088_accel_probe(struct spi_device *spi)  {
> > >         struct regmap *regmap;
> > > -       const struct spi_device_id *id = spi_get_device_id(spi);
> > >
> > >         regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus,
> > >                         spi, &bmi088_regmap_conf); @@ -52,8 +51,7
> > @@
> > > static int bmi088_accel_probe(struct spi_device *spi)
> > >                 return PTR_ERR(regmap);
> > >         }
> > >
> > > -       return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq,
> > id->name,
> > > -                                      true);
> > > +       return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq,
> > > + true);
> > >  }
> > >
> > >  static int bmi088_accel_remove(struct spi_device *spi) diff --git
> > > a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h
> > > index 5c25f16b672c..c32afe9606a8 100644
> > > --- a/drivers/iio/accel/bmi088-accel.h
> > > +++ b/drivers/iio/accel/bmi088-accel.h
> > > @@ -12,7 +12,7 @@ extern const struct regmap_config
> > > bmi088_regmap_conf;  extern const struct dev_pm_ops
> > > bmi088_accel_pm_ops;
> > >
> > >  int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
> > int irq,
> > > -                           const char *name, bool block_supported);
> > > +                           bool block_supported);
> > >  int bmi088_accel_core_remove(struct device *dev);
> > >
> > >  #endif /* BMI088_ACCEL_H */
> > > --
> > > 2.25.1
> > >

Re: [PATCH V2 3/6] iio: accel: bmi088: modified the device name

[Jonathan Cameron]

On Fri, 13 May 2022 09:20:48 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Thu, May 12, 2022 at 7:18 PM LI Qingwu
> <qing-wu.li@leica-geosystems.com.cn> wrote:
> >
> >  
> > > -----Original Message-----
> > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > Sent: Thursday, May 12, 2022 3:32 PM
> > > To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> > > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> > > <lars@metafoo.de>; mchehab+huawei@kernel.org; linux-iio
> > > <linux-iio@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Rob Herring
> > > <robh+dt@kernel.org>; Mike Looijmans <mike.looijmans@topic.nl>; devicetree
> > > <devicetree@vger.kernel.org>
> > > Subject: Re: [PATCH V2 3/6] iio: accel: bmi088: modified the device name
> > >
> > > This email is not from Hexagon’s Office 365 instance. Please be careful while
> > > clicking links, opening attachments, or replying to this email.
> > >
> > >
> > > On Tue, May 10, 2022 at 5:18 PM LI Qingwu
> > > <Qing-wu.Li@leica-geosystems.com.cn> wrote:  
> > > >
> > > > It is possible to have multiple sensors connected on the same
> > > > platform, To support multiple sensors, the commit makes it possible to
> > > > obtain the device name by reading the chip ID instead of the device-tree  
> > > name.  
> > > > To be compatible with previous versions, renam bmi088a to bmi088-accel.  
> > >
> > > // my spellcheck in GMail found this :p
> > >
> > > typo: renam -> rename
> > >
> > > I also have a comment about a duplication that is highlighted by this change.
> > >
> > > You can disregard my comment about the duplication and leave this change
> > > as-is.
> > >  
> > > >
> > > > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> > > > ---
> > > >  drivers/iio/accel/bmi088-accel-core.c | 6 +++---
> > > > drivers/iio/accel/bmi088-accel-spi.c  | 4 +---
> > > >  drivers/iio/accel/bmi088-accel.h      | 2 +-
> > > >  3 files changed, 5 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/accel/bmi088-accel-core.c
> > > > b/drivers/iio/accel/bmi088-accel-core.c
> > > > index 8fee1d02e773..de2385e4dad5 100644
> > > > --- a/drivers/iio/accel/bmi088-accel-core.c
> > > > +++ b/drivers/iio/accel/bmi088-accel-core.c
> > > > @@ -459,7 +459,7 @@ static const struct iio_chan_spec
> > > > bmi088_accel_channels[] = {
> > > >
> > > >  static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
> > > >         [0] = {
> > > > -               .name = "bmi088a",
> > > > +               .name = "bmi088-accel",
> > > >                 .chip_id = 0x1E,
> > > >                 .channels = bmi088_accel_channels,
> > > >                 .num_channels = ARRAY_SIZE(bmi088_accel_channels),
> > > > @@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct
> > > > bmi088_accel_data *data)  }
> > > >
> > > >  int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
> > > > -       int irq, const char *name, bool block_supported)
> > > > +       int irq, bool block_supported)
> > > >  {
> > > >         struct bmi088_accel_data *data;
> > > >         struct iio_dev *indio_dev;
> > > > @@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev,
> > > > struct regmap *regmap,
> > > >
> > > >         indio_dev->channels = data->chip_info->channels;
> > > >         indio_dev->num_channels = data->chip_info->num_channels;
> > > > -       indio_dev->name = name ? name : data->chip_info->name;
> > > > +       indio_dev->name = data->chip_info->name;  
> > >
> > > (with this change) i can better see, a bit of duplication between the spi_device
> > > table and the chip_info table
> > >
> > > this was not introduced by this change, but it was made a bit more obvious by
> > > this change;
> > >
> > > one way to address this, is to remove the `const char *name;` and continue
> > > using the `name` provided as a parameter from bmi088_accel_core_probe();
> > > (apologies if I seem to have changed my mind (from the previous changeset), but
> > > I did not see it too well before)
> > >
> > > and we can convert
> > >
> > > enum {
> > >    ID_BMI088,
> > >    ID_BMI085,
> > >    ...
> > > };
> > >
> > >  static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
> > >          [ID_BMI088] = {
> > >                  .chip_id = 0x1E,
> > >                  .channels = bmi088_accel_channels,
> > >                 .num_channels = ARRAY_SIZE(bmi088_accel_channels),
> > >         },
> > >          [ID_BMI085] = {
> > >          ........
> > >  
> > Thanks Ardelean,
> >
> > There is a case where some different sensors are connected to one system,
> > For user space is nice if can detect which sensor is present, if using the name in spi_device table,
> > the name may be inconsistent with the connected sensor. What's your opinion?  
> 
> I'd let Jonathan have the last word on this.
> 
> Typically, what I've seen (in practice), that when you write a
> device-tree for a board and you add (for example) BMI088 for board
> RevA, but on RevB it is decided to go with BMI085, then some people
> want to enforce this from the device-tree.

In my view the device tree should be using fallbacks for this, so
actually always list the real device first then have other compatible entries.
Thus if it turns out there is something unique that means we can't
just the whoami value, we have a more precise description (this
has happened with IIRC the mpu6050 supported parts).

> 
> So, if in the DT someone specifies bmi088-accel (as compatible
> string), then it could fail on probe if it detects bmi085-accel.
> Right now, this isn't happening with the driver (as it is now).
> 
> On the other hand, some other people want this flexibility where if
> you specify bmi088-accel, bmi085-accel or bmi090-accel, then they like
> the interchange-ability at the device-tree level.
> 
> I don't know which is better. I'm not sure this was enforced (or maybe
> I missed it).
> 
> But a more common practice (that I remember in some IIO drivers) is
> that the chip_info table (or a chip_id value)  can be added to
> spi_device_table.
> When the DT tries to probe the driver and reads a different ID (from
> SPI) it would fail the probe.

So we've had a few threads about this recently and some advice
from the device tree maintainers.  Where there is a Whoami or similar
register, the expectation is that a driver should at most 'warn' about
an incorrect device tree binding rather than failing the probe.
what it does then is dependent on whether the driver has info on the
chip id value found.  If it matches a different one then we can hopefully
assume that is the correct part (this isn't always true as we have
had device with the same ID that weren't compatible).  If it is unknown
then the assumption should be that the devicetree is correct and the
part is compatible so we should carry on anyway with whatever was
specified.

Idea being that if a new truly compatible part is released then
compatible = "bosch,newpart", "bosch,oldpart" should work
even though we don't know the chip id for the new part.


> 
> These are just some thoughts. Not a final conclusion.
> 
> >
> >  
> > > >         indio_dev->available_scan_masks = bmi088_accel_scan_masks;
> > > >         indio_dev->modes = INDIO_DIRECT_MODE;
> > > >         indio_dev->info = &bmi088_accel_info; diff --git
> > > > a/drivers/iio/accel/bmi088-accel-spi.c
> > > > b/drivers/iio/accel/bmi088-accel-spi.c
> > > > index dd1e3f6cf211..0fed0081e1fd 100644
> > > > --- a/drivers/iio/accel/bmi088-accel-spi.c
> > > > +++ b/drivers/iio/accel/bmi088-accel-spi.c
> > > > @@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = {
> > > > static int bmi088_accel_probe(struct spi_device *spi)  {
> > > >         struct regmap *regmap;
> > > > -       const struct spi_device_id *id = spi_get_device_id(spi);
> > > >
> > > >         regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus,
> > > >                         spi, &bmi088_regmap_conf); @@ -52,8 +51,7  
> > > @@  
> > > > static int bmi088_accel_probe(struct spi_device *spi)
> > > >                 return PTR_ERR(regmap);
> > > >         }
> > > >
> > > > -       return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq,  
> > > id->name,  
> > > > -                                      true);
> > > > +       return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq,
> > > > + true);
> > > >  }
> > > >
> > > >  static int bmi088_accel_remove(struct spi_device *spi) diff --git
> > > > a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h
> > > > index 5c25f16b672c..c32afe9606a8 100644
> > > > --- a/drivers/iio/accel/bmi088-accel.h
> > > > +++ b/drivers/iio/accel/bmi088-accel.h
> > > > @@ -12,7 +12,7 @@ extern const struct regmap_config
> > > > bmi088_regmap_conf;  extern const struct dev_pm_ops
> > > > bmi088_accel_pm_ops;
> > > >
> > > >  int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,  
> > > int irq,  
> > > > -                           const char *name, bool block_supported);
> > > > +                           bool block_supported);
> > > >  int bmi088_accel_core_remove(struct device *dev);
> > > >
> > > >  #endif /* BMI088_ACCEL_H */
> > > > --
> > > > 2.25.1
> > > >  

Re: [PATCH V2 3/6] iio: accel: bmi088: modified the device name

[Jonathan Cameron]

On Tue, 10 May 2022 14:17:50 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> It is possible to have multiple sensors connected on the same platform,
> To support multiple sensors, the commit makes it possible to obtain the
> device name by reading the chip ID instead of the device-tree name.
> To be compatible with previous versions, renam bmi088a to bmi088-accel.
rename

This is technically a userspace ABI change.  However, the value changed was a fallback
anyway, so number of cases where you would actually get it are very few.

Hence I'm not that worried about the change.  I would add that it is an ABI change
and ideally figure out under what circumstances it matters (I 'think' it might be
when using fallback compatibles - but you should check).

> 
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/iio/accel/bmi088-accel-core.c | 6 +++---
>  drivers/iio/accel/bmi088-accel-spi.c  | 4 +---
>  drivers/iio/accel/bmi088-accel.h      | 2 +-
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> index 8fee1d02e773..de2385e4dad5 100644
> --- a/drivers/iio/accel/bmi088-accel-core.c
> +++ b/drivers/iio/accel/bmi088-accel-core.c
> @@ -459,7 +459,7 @@ static const struct iio_chan_spec bmi088_accel_channels[] = {
>  
>  static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
>  	[0] = {
> -		.name = "bmi088a",
> +		.name = "bmi088-accel",
>  		.chip_id = 0x1E,
>  		.channels = bmi088_accel_channels,
>  		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
> @@ -524,7 +524,7 @@ static int bmi088_accel_chip_init(struct bmi088_accel_data *data)
>  }
>  
>  int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
> -	int irq, const char *name, bool block_supported)
> +	int irq, bool block_supported)
>  {
>  	struct bmi088_accel_data *data;
>  	struct iio_dev *indio_dev;
> @@ -545,7 +545,7 @@ int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
>  
>  	indio_dev->channels = data->chip_info->channels;
>  	indio_dev->num_channels = data->chip_info->num_channels;
> -	indio_dev->name = name ? name : data->chip_info->name;
> +	indio_dev->name = data->chip_info->name;
>  	indio_dev->available_scan_masks = bmi088_accel_scan_masks;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &bmi088_accel_info;
> diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c
> index dd1e3f6cf211..0fed0081e1fd 100644
> --- a/drivers/iio/accel/bmi088-accel-spi.c
> +++ b/drivers/iio/accel/bmi088-accel-spi.c
> @@ -42,7 +42,6 @@ static struct regmap_bus bmi088_regmap_bus = {
>  static int bmi088_accel_probe(struct spi_device *spi)
>  {
>  	struct regmap *regmap;
> -	const struct spi_device_id *id = spi_get_device_id(spi);
>  
>  	regmap = devm_regmap_init(&spi->dev, &bmi088_regmap_bus,
>  			spi, &bmi088_regmap_conf);
> @@ -52,8 +51,7 @@ static int bmi088_accel_probe(struct spi_device *spi)
>  		return PTR_ERR(regmap);
>  	}
>  
> -	return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, id->name,
> -				       true);
> +	return bmi088_accel_core_probe(&spi->dev, regmap, spi->irq, true);
>  }
>  
>  static int bmi088_accel_remove(struct spi_device *spi)
> diff --git a/drivers/iio/accel/bmi088-accel.h b/drivers/iio/accel/bmi088-accel.h
> index 5c25f16b672c..c32afe9606a8 100644
> --- a/drivers/iio/accel/bmi088-accel.h
> +++ b/drivers/iio/accel/bmi088-accel.h
> @@ -12,7 +12,7 @@ extern const struct regmap_config bmi088_regmap_conf;
>  extern const struct dev_pm_ops bmi088_accel_pm_ops;
>  
>  int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
> -			    const char *name, bool block_supported);
> +			    bool block_supported);
>  int bmi088_accel_core_remove(struct device *dev);
>  
>  #endif /* BMI088_ACCEL_H */

[PATCH V2 4/6] iio: accel: bmi088: Add support for bmi085 accel

[LI Qingwu]

Add supports for BMI085, an Inertial Measurement Unit,
with an accelerometer and gyroscope.
The commit adds the accelerometer driver for the SPI interface.
The gyroscope part is already supported by the BMG160 driver.
Different from BMI088, the BMI085 accelerometer has the range of
+/-2, 4, 6, and 8g.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/bmi088-accel-core.c | 7 +++++++
 drivers/iio/accel/bmi088-accel-spi.c  | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
index de2385e4dad5..13bb3d96a3a6 100644
--- a/drivers/iio/accel/bmi088-accel-core.c
+++ b/drivers/iio/accel/bmi088-accel-core.c
@@ -465,6 +465,13 @@ static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
 		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
 		.scale_table = {{0, 897}, {0, 1795}, {0, 3590}, {0, 7179}},
 	},
+	[1] = {
+		.name = "bmi085-accel",
+		.chip_id = 0x1F,
+		.channels = bmi088_accel_channels,
+		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
+		.scale_table = {{0, 598}, {0, 1196}, {0, 2393}, {0, 4785}},
+	},
 };
 
 static const struct iio_info bmi088_accel_info = {
diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c
index 0fed0081e1fd..e7a1daab8f3c 100644
--- a/drivers/iio/accel/bmi088-accel-spi.c
+++ b/drivers/iio/accel/bmi088-accel-spi.c
@@ -61,6 +61,7 @@ static int bmi088_accel_remove(struct spi_device *spi)
 
 static const struct spi_device_id bmi088_accel_id[] = {
 	{"bmi088-accel", },
+	{"bmi085-accel", },
 	{}
 };
 MODULE_DEVICE_TABLE(spi, bmi088_accel_id);
-- 
2.25.1

Re: [PATCH V2 4/6] iio: accel: bmi088: Add support for bmi085 accel

[Jonathan Cameron]

On Tue, 10 May 2022 14:17:51 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> Add supports for BMI085, an Inertial Measurement Unit,
> with an accelerometer and gyroscope.
> The commit adds the accelerometer driver for the SPI interface.
> The gyroscope part is already supported by the BMG160 driver.
> Different from BMI088, the BMI085 accelerometer has the range of
> +/-2, 4, 6, and 8g.
> 
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>

Hi,

A request to change the ordering of id table entries below.

> ---
>  drivers/iio/accel/bmi088-accel-core.c | 7 +++++++
>  drivers/iio/accel/bmi088-accel-spi.c  | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> index de2385e4dad5..13bb3d96a3a6 100644
> --- a/drivers/iio/accel/bmi088-accel-core.c
> +++ b/drivers/iio/accel/bmi088-accel-core.c
> @@ -465,6 +465,13 @@ static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
>  		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
>  		.scale_table = {{0, 897}, {0, 1795}, {0, 3590}, {0, 7179}},
>  	},
> +	[1] = {
> +		.name = "bmi085-accel",
> +		.chip_id = 0x1F,

Probably makes sense to keep these entries in order of chip_id - so leave the
order as you have it for this array.

> +		.channels = bmi088_accel_channels,
> +		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
> +		.scale_table = {{0, 598}, {0, 1196}, {0, 2393}, {0, 4785}},
> +	},
>  };
>  
>  static const struct iio_info bmi088_accel_info = {
> diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c
> index 0fed0081e1fd..e7a1daab8f3c 100644
> --- a/drivers/iio/accel/bmi088-accel-spi.c
> +++ b/drivers/iio/accel/bmi088-accel-spi.c
> @@ -61,6 +61,7 @@ static int bmi088_accel_remove(struct spi_device *spi)
>  
>  static const struct spi_device_id bmi088_accel_id[] = {
>  	{"bmi088-accel", },
> +	{"bmi085-accel", },

Alphabetical / numeric order preferred.

>  	{}
>  };
>  MODULE_DEVICE_TABLE(spi, bmi088_accel_id);

[PATCH V2 5/6] iio: accel: bmi088: Add support for bmi090l accel

[LI Qingwu]

Add supports for BMI090L, it's a high-performance Inertial
Measurement Unit, with an accelerometer and gyroscope.
The commit adds the accelerometer driver for the SPI interface.
The gyroscope part is already supported by the BMG160 driver.
Same as BMI088, BMI090L have the range of +/-3, 6, 12, and 24g.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/bmi088-accel-core.c | 7 +++++++
 drivers/iio/accel/bmi088-accel-spi.c  | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
index 13bb3d96a3a6..6d44e97b4906 100644
--- a/drivers/iio/accel/bmi088-accel-core.c
+++ b/drivers/iio/accel/bmi088-accel-core.c
@@ -472,6 +472,13 @@ static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
 		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
 		.scale_table = {{0, 598}, {0, 1196}, {0, 2393}, {0, 4785}},
 	},
+	[2] = {
+		.name = "bmi090l-accel",
+		.chip_id = 0x1A,
+		.channels = bmi088_accel_channels,
+		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
+		.scale_table = {{0, 897}, {0, 1795}, {0, 3590}, {0, 7179}},
+	},
 };
 
 static const struct iio_info bmi088_accel_info = {
diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c
index e7a1daab8f3c..58be73ebd2dd 100644
--- a/drivers/iio/accel/bmi088-accel-spi.c
+++ b/drivers/iio/accel/bmi088-accel-spi.c
@@ -62,6 +62,7 @@ static int bmi088_accel_remove(struct spi_device *spi)
 static const struct spi_device_id bmi088_accel_id[] = {
 	{"bmi088-accel", },
 	{"bmi085-accel", },
+	{"bmi090l-accel", },
 	{}
 };
 MODULE_DEVICE_TABLE(spi, bmi088_accel_id);
-- 
2.25.1

Re: [PATCH V2 5/6] iio: accel: bmi088: Add support for bmi090l accel

[Jonathan Cameron]

On Tue, 10 May 2022 14:17:52 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> Add supports for BMI090L, it's a high-performance Inertial
> Measurement Unit, with an accelerometer and gyroscope.
> The commit adds the accelerometer driver for the SPI interface.
> The gyroscope part is already supported by the BMG160 driver.
> Same as BMI088, BMI090L have the range of +/-3, 6, 12, and 24g.
> 
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/iio/accel/bmi088-accel-core.c | 7 +++++++
>  drivers/iio/accel/bmi088-accel-spi.c  | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> index 13bb3d96a3a6..6d44e97b4906 100644
> --- a/drivers/iio/accel/bmi088-accel-core.c
> +++ b/drivers/iio/accel/bmi088-accel-core.c
> @@ -472,6 +472,13 @@ static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
>  		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
>  		.scale_table = {{0, 598}, {0, 1196}, {0, 2393}, {0, 4785}},
>  	},
> +	[2] = {
> +		.name = "bmi090l-accel",
> +		.chip_id = 0x1A,

Either order by chip_id or by name. I don't mind which but we do want some
logical ordering in this table so we know where to put future entries.

Thanks,

Jonathan

> +		.channels = bmi088_accel_channels,
> +		.num_channels = ARRAY_SIZE(bmi088_accel_channels),
> +		.scale_table = {{0, 897}, {0, 1795}, {0, 3590}, {0, 7179}},
> +	},
>  };
>  
>  static const struct iio_info bmi088_accel_info = {
> diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c
> index e7a1daab8f3c..58be73ebd2dd 100644
> --- a/drivers/iio/accel/bmi088-accel-spi.c
> +++ b/drivers/iio/accel/bmi088-accel-spi.c
> @@ -62,6 +62,7 @@ static int bmi088_accel_remove(struct spi_device *spi)
>  static const struct spi_device_id bmi088_accel_id[] = {
>  	{"bmi088-accel", },
>  	{"bmi085-accel", },
> +	{"bmi090l-accel", },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(spi, bmi088_accel_id);

[PATCH V2 6/6] dt-bindings: iio: accel: Add bmi085 and bmi090l bindings

[LI Qingwu]

Adds the device-tree bindings for the Bosch
BMI085 and BMI090L IMU, the accelerometer part.

Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
index 911a1ae9c83f..4290f5f88a8f 100644
--- a/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
@@ -18,6 +18,8 @@ properties:
   compatible:
     enum:
       - bosch,bmi088-accel
+      - bosch,bmi085-accel
+      - bosch,bmi090l-accel
 
   reg:
     maxItems: 1
-- 
2.25.1

Re: [PATCH V2 6/6] dt-bindings: iio: accel: Add bmi085 and bmi090l bindings

[Alexandru Ardelean]

On Tue, May 10, 2022 at 5:18 PM LI Qingwu
<Qing-wu.Li@leica-geosystems.com.cn> wrote:
>
> Adds the device-tree bindings for the Bosch
> BMI085 and BMI090L IMU, the accelerometer part.
>

I think some datasheet links could be added to this file for the new devices.

The BMI088 has a link to its datasheet.

> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> index 911a1ae9c83f..4290f5f88a8f 100644
> --- a/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> @@ -18,6 +18,8 @@ properties:
>    compatible:
>      enum:
>        - bosch,bmi088-accel
> +      - bosch,bmi085-accel
> +      - bosch,bmi090l-accel
>
>    reg:
>      maxItems: 1
> --
> 2.25.1
>

Re: [PATCH V2 6/6] dt-bindings: iio: accel: Add bmi085 and bmi090l bindings

[Jonathan Cameron]

On Thu, 12 May 2022 10:32:55 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Tue, May 10, 2022 at 5:18 PM LI Qingwu
> <Qing-wu.Li@leica-geosystems.com.cn> wrote:
> >
> > Adds the device-tree bindings for the Bosch
> > BMI085 and BMI090L IMU, the accelerometer part.
> >  
> 
> I think some datasheet links could be added to this file for the new devices.
> 
> The BMI088 has a link to its datasheet.
> 
> > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> > ---
> >  Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> > index 911a1ae9c83f..4290f5f88a8f 100644
> > --- a/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> > +++ b/Documentation/devicetree/bindings/iio/accel/bosch,bmi088.yaml
> > @@ -18,6 +18,8 @@ properties:
> >    compatible:
> >      enum:
> >        - bosch,bmi088-accel
> > +      - bosch,bmi085-accel
> > +      - bosch,bmi090l-accel

Alphabetical /numeric order preferred. Also, fun question of compatible
fallbacks as per the previous email.  Other than ID you state the bmi080l
has same scales etc as the bmi088 so that one should definitely have
a fallback to bmi088.

The bmi085 is a little less obvious.  We can detect the difference by
the chip id though so a fallback compatible probably makes sense for
that one as well.  DT maintainers - I'll go with whatever you recommend
on that front.

Also, driver doesn't currently have an of_id_table so relies on the fallback
handling of the spi core. Please add an explicit table to the driver.

Thanks,

Jonathan

> >
> >    reg:
> >      maxItems: 1
> > --
> > 2.25.1
> >