diff mbox series

[1/2] iio: pressure: bmp280: Allow multiple chips id per family of devices

Message ID 8856287079b5dc1099f5b98a0168f008fbb8debd.1692305434.git.ang.iglesiasg@gmail.com (mailing list archive)
State Superseded
Headers show
Series iio: pressure: bmp280: Add support for BMP390 | expand

Commit Message

Angel Iglesias Aug. 17, 2023, 9:05 p.m. UTC 
Improve device detection in certain chip families known to have various
chip ids.

Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>

Comments

andriy.shevchenko@linux.intel.com Aug. 18, 2023, 11:19 a.m. UTC | #1 
On Thu, Aug 17, 2023 at 11:05:21PM +0200, Angel Iglesias wrote:
> Improve device detection in certain chip families known to have various
> chip ids.

...

> +	ret = -EINVAL;

Why do you need this...

> +	for (i = 0; i < data->chip_info->num_chip_id; i++) {
> +		if (chip_id == data->chip_info->chip_id[i]) {

> +			ret = 0;

..and this...

> +			break;
> +		}
> +	}

> +	if (ret) {

...and this?

You can simply do

	for (i = 0; i < data->chip_info->num_chip_id; i++) {
		if (chip_id == data->chip_info->chip_id[i])
			break;
	}
	if (i < data->chip_info->num_chip_id) {

...

> +		// 0x<id>, so four chars per number plus one space + ENDL
> +		size_t nbuf = 5*data->chip_info->num_chip_id*sizeof(char);

Besides lack of spaces...

> +		char *buf = kmalloc(nbuf, GFP_KERNEL);

...this at least should be kmalloc_array() and on top maybe something from
overflow.h will be needed.

> +		if (!buf)
> +			return ret;
> +
> +		for (i = 0; i < data->chip_info->num_chip_id; i++)
> +			snprintf(&buf[i*5], nbuf, "0x%x ", data->chip_info->chip_id[i]);
> +		buf[nbuf-1] = '\0';
> +
> +		dev_err(dev, "bad chip id: expected [ %s ] got 0x%x\n", buf, chip_id);
> +		kfree(buf);
> +		return ret;
>  	}

...

> -	const unsigned int chip_id;

Yeah, this const makes a little sense...

> +	const unsigned int *chip_id;

...but not this :-)

What I'm wondering is why it's int and not u8 / u16
(as it seems only a byte value there).

> +	int num_chip_id;

unsigned.
Angel Iglesias Aug. 18, 2023, 3:52 p.m. UTC | #2 
On Fri, 2023-08-18 at 14:19 +0300, Andy Shevchenko wrote:
> On Thu, Aug 17, 2023 at 11:05:21PM +0200, Angel Iglesias wrote:
> > Improve device detection in certain chip families known to have various
> > chip ids.
> 
> ...
> 
> > +       ret = -EINVAL;
> 
> Why do you need this...
> 
> > +       for (i = 0; i < data->chip_info->num_chip_id; i++) {
> > +               if (chip_id == data->chip_info->chip_id[i]) {
> 
> > +                       ret = 0;
> 
> ..and this...
> 
> > +                       break;
> > +               }
> > +       }
> 
> > +       if (ret) {
> 
> ...and this?
> 
> You can simply do
> 
>         for (i = 0; i < data->chip_info->num_chip_id; i++) {
>                 if (chip_id == data->chip_info->chip_id[i])
>                         break;
>         }
>         if (i < data->chip_info->num_chip_id) {
> 

Got it, much cleaner also.

> ...
> 
> > +               // 0x<id>, so four chars per number plus one space + ENDL
> > +               size_t nbuf = 5*data->chip_info->num_chip_id*sizeof(char);
> 
> Besides lack of spaces...
> 
> > +               char *buf = kmalloc(nbuf, GFP_KERNEL);
> 
> ...this at least should be kmalloc_array() and on top maybe something from
> overflow.h will be needed.

Sure, I'll give a look. I didn't want to do string manipulations on a kernel
driver but couldn't found a way to log the error meaningfully in one entry.

> > +               if (!buf)
> > +                       return ret;
> > +
> > +               for (i = 0; i < data->chip_info->num_chip_id; i++)
> > +                       snprintf(&buf[i*5], nbuf, "0x%x ", data->chip_info-
> > >chip_id[i]);
> > +               buf[nbuf-1] = '\0';
> > +
> > +               dev_err(dev, "bad chip id: expected [ %s ] got 0x%x\n", buf,
> > chip_id);
> > +               kfree(buf);
> > +               return ret;
> >         }
> 
> ...
> 
> > -       const unsigned int chip_id;
> 
> Yeah, this const makes a little sense...
> 
> > +       const unsigned int *chip_id;
> 
> ...but not this :-)

Isn't the same case as "const struct iio_chan_spec *channels" or "const int
*oversampling_temp_avail". I thoght that this meant a pointer to a constant
integer. On bmp280-core I declare the arrays with the modifiers static const.

> What I'm wondering is why it's int and not u8 / u16
> (as it seems only a byte value there).

Yeah, can be u8, as the reg width is 1 byte and this IDs are stored on one reg.
I just carried over the int type from previous versions, but it's just wasting
space :/

> 
> > +       int num_chip_id;
> 
> unsigned.
> 
Thanks for your time!

Kind regards,
Angel
andriy.shevchenko@linux.intel.com Aug. 18, 2023, 3:57 p.m. UTC | #3 
On Fri, Aug 18, 2023 at 05:52:07PM +0200, Angel Iglesias wrote:
> On Fri, 2023-08-18 at 14:19 +0300, Andy Shevchenko wrote:
> > On Thu, Aug 17, 2023 at 11:05:21PM +0200, Angel Iglesias wrote:

...

> > > -       const unsigned int chip_id;
> > 
> > Yeah, this const makes a little sense...
> > 
> > > +       const unsigned int *chip_id;
> > 
> > ...but not this :-)
> 
> Isn't the same case as "const struct iio_chan_spec *channels" or "const int
> *oversampling_temp_avail". I thoght that this meant a pointer to a constant
> integer. On bmp280-core I declare the arrays with the modifiers static const.

Yes, and that is my point:
- old code makes a little sense
- new code makes a lot of sense

> > What I'm wondering is why it's int and not u8 / u16
> > (as it seems only a byte value there).
> 
> Yeah, can be u8, as the reg width is 1 byte and this IDs are stored on one reg.
> I just carried over the int type from previous versions, but it's just wasting
> space :/
Angel Iglesias Aug. 18, 2023, 4:41 p.m. UTC | #4 
On Fri, 2023-08-18 at 18:57 +0300, Andy Shevchenko wrote:
> On Fri, Aug 18, 2023 at 05:52:07PM +0200, Angel Iglesias wrote:
> > On Fri, 2023-08-18 at 14:19 +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 17, 2023 at 11:05:21PM +0200, Angel Iglesias wrote:
> 
> ...
> 
> > > > -       const unsigned int chip_id;
> > > 
> > > Yeah, this const makes a little sense...
> > > 
> > > > +       const unsigned int *chip_id;
> > > 
> > > ...but not this :-)
> > 
> > Isn't the same case as "const struct iio_chan_spec *channels" or "const int
> > *oversampling_temp_avail". I thoght that this meant a pointer to a constant
> > integer. On bmp280-core I declare the arrays with the modifiers static
> > const.
> 
> Yes, and that is my point:
> - old code makes a little sense
> - new code makes a lot of sense

Thanks for the clarification. I initially understood the opposite :S

> > > What I'm wondering is why it's int and not u8 / u16
> > > (as it seems only a byte value there).
> > 
> > Yeah, can be u8, as the reg width is 1 byte and this IDs are stored on one
> > reg.
> > I just carried over the int type from previous versions, but it's just
> > wasting
> > space :/
>
Jonathan Cameron Aug. 28, 2023, 2:28 p.m. UTC | #5 
> +	if (ret) {
> +		// 0x<id>, so four chars per number plus one space + ENDL
/* */
diff mbox series

Patch

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 6089f3f9d8f4..67941a67e513 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -794,10 +794,12 @@  static int bmp280_chip_config(struct bmp280_data *data)
 }
 
 static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
+static const int bmp280_chip_ids[] = { BMP280_CHIP_ID };
 
 const struct bmp280_chip_info bmp280_chip_info = {
 	.id_reg = BMP280_REG_ID,
-	.chip_id = BMP280_CHIP_ID,
+	.chip_id = bmp280_chip_ids,
+	.num_chip_id = ARRAY_SIZE(bmp280_chip_ids),
 	.regmap_config = &bmp280_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
@@ -846,9 +848,12 @@  static int bme280_chip_config(struct bmp280_data *data)
 	return bmp280_chip_config(data);
 }
 
+static const int bme280_chip_ids[] = { BME280_CHIP_ID };
+
 const struct bmp280_chip_info bme280_chip_info = {
 	.id_reg = BMP280_REG_ID,
-	.chip_id = BME280_CHIP_ID,
+	.chip_id = bme280_chip_ids,
+	.num_chip_id = ARRAY_SIZE(bme280_chip_ids),
 	.regmap_config = &bmp280_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
@@ -1220,10 +1225,12 @@  static int bmp380_chip_config(struct bmp280_data *data)
 
 static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 };
 static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128};
+static const int bmp380_chip_ids[] = { BMP380_CHIP_ID };
 
 const struct bmp280_chip_info bmp380_chip_info = {
 	.id_reg = BMP380_REG_ID,
-	.chip_id = BMP380_CHIP_ID,
+	.chip_id = bmp380_chip_ids,
+	.num_chip_id = ARRAY_SIZE(bmp380_chip_ids),
 	.regmap_config = &bmp380_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp380_channels,
@@ -1720,10 +1727,12 @@  static int bmp580_chip_config(struct bmp280_data *data)
 }
 
 static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
+static const int bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT };
 
 const struct bmp280_chip_info bmp580_chip_info = {
 	.id_reg = BMP580_REG_CHIP_ID,
-	.chip_id = BMP580_CHIP_ID,
+	.chip_id = bmp580_chip_ids,
+	.num_chip_id = ARRAY_SIZE(bmp580_chip_ids),
 	.regmap_config = &bmp580_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp380_channels,
@@ -1983,10 +1992,12 @@  static int bmp180_chip_config(struct bmp280_data *data)
 
 static const int bmp180_oversampling_temp_avail[] = { 1 };
 static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
+static const int bmp180_chip_ids[] = { BMP180_CHIP_ID };
 
 const struct bmp280_chip_info bmp180_chip_info = {
 	.id_reg = BMP280_REG_ID,
-	.chip_id = BMP180_CHIP_ID,
+	.chip_id = bmp180_chip_ids,
+	.num_chip_id = ARRAY_SIZE(bmp180_chip_ids),
 	.regmap_config = &bmp180_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
@@ -2077,7 +2088,7 @@  int bmp280_common_probe(struct device *dev,
 	struct bmp280_data *data;
 	struct gpio_desc *gpiod;
 	unsigned int chip_id;
-	int ret;
+	int ret, i;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
@@ -2142,10 +2153,30 @@  int bmp280_common_probe(struct device *dev,
 	ret = regmap_read(regmap, data->chip_info->id_reg, &chip_id);
 	if (ret < 0)
 		return ret;
-	if (chip_id != data->chip_info->chip_id) {
-		dev_err(dev, "bad chip id: expected %x got %x\n",
-			data->chip_info->chip_id, chip_id);
-		return -EINVAL;
+
+	ret = -EINVAL;
+	for (i = 0; i < data->chip_info->num_chip_id; i++) {
+		if (chip_id == data->chip_info->chip_id[i]) {
+			ret = 0;
+			break;
+		}
+	}
+
+	if (ret) {
+		// 0x<id>, so four chars per number plus one space + ENDL
+		size_t nbuf = 5*data->chip_info->num_chip_id*sizeof(char);
+		char *buf = kmalloc(nbuf, GFP_KERNEL);
+
+		if (!buf)
+			return ret;
+
+		for (i = 0; i < data->chip_info->num_chip_id; i++)
+			snprintf(&buf[i*5], nbuf, "0x%x ", data->chip_info->chip_id[i]);
+		buf[nbuf-1] = '\0';
+
+		dev_err(dev, "bad chip id: expected [ %s ] got 0x%x\n", buf, chip_id);
+		kfree(buf);
+		return ret;
 	}
 
 	if (data->chip_info->preinit) {
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 5c0563ce7572..d68745254340 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -418,7 +418,8 @@  struct bmp280_data {
 
 struct bmp280_chip_info {
 	unsigned int id_reg;
-	const unsigned int chip_id;
+	const unsigned int *chip_id;
+	int num_chip_id;
 
 	const struct regmap_config *regmap_config;