Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants
From: Angel Iglesias
Date: Sun Jan 01 2023 - 05:56:41 EST
On Fri, 2022-12-30 at 18:14 +0000, Jonathan Cameron wrote:
> On Mon, 26 Dec 2022 15:29:20 +0100
> Angel Iglesias <ang.iglesiasg@xxxxxxxxx> wrote:
>
> > Adds enumeration to improve handling the different supported sensors
> > on driver initialization. This avoid collisions if different variants
> > share the same device idetifier on ID register.
>
> If we get two parts with the same ID that need different handling
> then we should probably be shouting at Bosch. Still I don't mind
> tidying this up anyway as using the CHIP_IDs is messy - however...
>
> Please be careful to make sure you have responded (either by changes, or by
> replying to review to say why you aren't making changes). If you are not
> receiving
> all emails, then you can always check lore.kernel.org to make sure you didn't
> miss
> any reviews.
I've been away for the week visiting family and friends. Sorry for leaving your
reviews lingering unanswered.
Yeah, I kinda rushed this part of the patch to send it before christmas, not my
brightest call. I like Andy suggestion, but I have a few questions on handling
the regmap and the chip config using the pointers, please refer to the reply I
sent to Andy review for the details.
Thanks for your time,
Angel
> As Andy suggested, switch over to using pointers to the chip_info structure as
> the
> data element in *_device_id tables. It usually ends up neater in the end than
> messing around with an indirection via an enum value.
>
> >
> > Signed-off-by: Angel Iglesias <ang.iglesiasg@xxxxxxxxx>
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c
> > b/drivers/iio/pressure/bmp280-core.c
> > index c0aff78489b4..46959a91408f 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -186,6 +186,7 @@ struct bmp280_data {
> >
> > struct bmp280_chip_info {
> > unsigned int id_reg;
> > + const unsigned int chip_id;
> >
> > const struct iio_chan_spec *channels;
> > int num_channels;
> > @@ -907,6 +908,7 @@ static const int bmp280_oversampling_avail[] = { 1, 2,
> > 4, 8, 16 };
> >
> > static const struct bmp280_chip_info bmp280_chip_info = {
> > .id_reg = BMP280_REG_ID,
> > + .chip_id = BMP280_CHIP_ID,
> > .start_up_time = 2000,
> > .channels = bmp280_channels,
> > .num_channels = 2,
> > @@ -955,6 +957,7 @@ static int bme280_chip_config(struct bmp280_data *data)
> >
> > static const struct bmp280_chip_info bme280_chip_info = {
> > .id_reg = BMP280_REG_ID,
> > + .chip_id = BME280_CHIP_ID,
> > .start_up_time = 2000,
> > .channels = bmp280_channels,
> > .num_channels = 3,
> > @@ -1321,6 +1324,7 @@ static const int bmp380_iir_filter_coeffs_avail[] = {
> > 1, 2, 4, 8, 16, 32, 64, 12
> >
> > static const struct bmp280_chip_info bmp380_chip_info = {
> > .id_reg = BMP380_REG_ID,
> > + .chip_id = BMP380_CHIP_ID,
> > .start_up_time = 2000,
> > .channels = bmp380_channels,
> > .num_channels = 2,
> > @@ -1581,6 +1585,7 @@ static const int bmp180_oversampling_press_avail[] = {
> > 1, 2, 4, 8 };
> >
> > static const struct bmp280_chip_info bmp180_chip_info = {
> > .id_reg = BMP280_REG_ID,
> > + .chip_id = BMP180_CHIP_ID,
> > .start_up_time = 2000,
> > .channels = bmp280_channels,
> > .num_channels = 2,
> > @@ -1685,16 +1690,16 @@ int bmp280_common_probe(struct device *dev,
> > indio_dev->modes = INDIO_DIRECT_MODE;
> >
> > switch (chip) {
> > - case BMP180_CHIP_ID:
> > + case BMP180:
> > chip_info = &bmp180_chip_info;
> > break;
> > - case BMP280_CHIP_ID:
> > + case BMP280:
> > chip_info = &bmp280_chip_info;
> > break;
> > - case BME280_CHIP_ID:
> > + case BME280:
> > chip_info = &bme280_chip_info;
> > break;
> > - case BMP380_CHIP_ID:
> > + case BMP380:
> > chip_info = &bmp380_chip_info;
>
> If you use a pointer directly then no need for this switch statement at all.
>
> > break;
> > default:
> > @@ -1751,9 +1756,9 @@ 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 != chip) {
> > + if (chip_id != data->chip_info->chip_id) {
> > dev_err(dev, "bad chip id: expected %x got %x\n",
> > - chip, chip_id);
> > + data->chip_info->chip_id, chip_id);
> > return -EINVAL;
> > }
> >
> > diff --git a/drivers/iio/pressure/bmp280-i2c.c
> > b/drivers/iio/pressure/bmp280-i2c.c
> > index 14eab086d24a..59921e8cd592 100644
> > --- a/drivers/iio/pressure/bmp280-i2c.c
> > +++ b/drivers/iio/pressure/bmp280-i2c.c
> > @@ -12,14 +12,14 @@ static int bmp280_i2c_probe(struct i2c_client *client)
> > const struct i2c_device_id *id = i2c_client_get_device_id(client);
> >
> > switch (id->driver_data) {
> > - case BMP180_CHIP_ID:
> > + case BMP180:
> > regmap_config = &bmp180_regmap_config;
> > break;
> > - case BMP280_CHIP_ID:
> > - case BME280_CHIP_ID:
> > + case BMP280:
> > + case BME280:
> > regmap_config = &bmp280_regmap_config;
> > break;
> > - case BMP380_CHIP_ID:
> > + case BMP380:
> > regmap_config = &bmp380_regmap_config;
> > break;
> > default:
> > @@ -40,21 +40,21 @@ static int bmp280_i2c_probe(struct i2c_client *client)
> > }
> >
> > static const struct of_device_id bmp280_of_i2c_match[] = {
> > - { .compatible = "bosch,bmp085", .data = (void *)BMP180_CHIP_ID },
> > - { .compatible = "bosch,bmp180", .data = (void *)BMP180_CHIP_ID },
> > - { .compatible = "bosch,bmp280", .data = (void *)BMP280_CHIP_ID },
> > - { .compatible = "bosch,bme280", .data = (void *)BME280_CHIP_ID },
> > - { .compatible = "bosch,bmp380", .data = (void *)BMP380_CHIP_ID },
> > + { .compatible = "bosch,bmp085", .data = (void *)BMP180 },
> > + { .compatible = "bosch,bmp180", .data = (void *)BMP180 },
> > + { .compatible = "bosch,bmp280", .data = (void *)BMP280 },
> > + { .compatible = "bosch,bme280", .data = (void *)BME280 },
> > + { .compatible = "bosch,bmp380", .data = (void *)BMP380 },
> > { },
> > };
> > MODULE_DEVICE_TABLE(of, bmp280_of_i2c_match);
> >
> > static const struct i2c_device_id bmp280_i2c_id[] = {
> > - {"bmp085", BMP180_CHIP_ID },
> > - {"bmp180", BMP180_CHIP_ID },
> > - {"bmp280", BMP280_CHIP_ID },
> > - {"bme280", BME280_CHIP_ID },
> > - {"bmp380", BMP380_CHIP_ID },
> > + {"bmp085", BMP180 },
> > + {"bmp180", BMP180 },
> > + {"bmp280", BMP280 },
> > + {"bme280", BME280 },
> > + {"bmp380", BMP380 },
> > { },
> > };
> > MODULE_DEVICE_TABLE(i2c, bmp280_i2c_id);
> > diff --git a/drivers/iio/pressure/bmp280-spi.c
> > b/drivers/iio/pressure/bmp280-spi.c
> > index 011c68e07ebf..4a2df5b5d838 100644
> > --- a/drivers/iio/pressure/bmp280-spi.c
> > +++ b/drivers/iio/pressure/bmp280-spi.c
> > @@ -59,14 +59,14 @@ static int bmp280_spi_probe(struct spi_device *spi)
> > }
> >
> > switch (id->driver_data) {
> > - case BMP180_CHIP_ID:
> > + case BMP180:
> > regmap_config = &bmp180_regmap_config;
> > break;
> > - case BMP280_CHIP_ID:
> > - case BME280_CHIP_ID:
> > + case BMP280:
> > + case BME280:
> > regmap_config = &bmp280_regmap_config;
> > break;
> > - case BMP380_CHIP_ID:
> > + case BMP380:
> > regmap_config = &bmp380_regmap_config;
> > break;
> > default:
> > @@ -101,11 +101,11 @@ static const struct of_device_id bmp280_of_spi_match[]
> > = {
> > MODULE_DEVICE_TABLE(of, bmp280_of_spi_match);
> >
> > static const struct spi_device_id bmp280_spi_id[] = {
> > - { "bmp180", BMP180_CHIP_ID },
> > - { "bmp181", BMP180_CHIP_ID },
> > - { "bmp280", BMP280_CHIP_ID },
> > - { "bme280", BME280_CHIP_ID },
> > - { "bmp380", BMP380_CHIP_ID },
> > + { "bmp180", BMP180 },
> > + { "bmp181", BMP180 },
> > + { "bmp280", BMP280 },
> > + { "bme280", BME280 },
> > + { "bmp380", BMP380 },
> > { }
> > };
> > MODULE_DEVICE_TABLE(spi, bmp280_spi_id);
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index c791325c7416..efc31bc84708 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -191,6 +191,14 @@
> > #define BMP280_PRESS_SKIPPED 0x80000
> > #define BMP280_HUMIDITY_SKIPPED 0x8000
> >
> > +/* Enum with supported pressure sensor models */
> > +enum bmp280_variant {
> > + BMP180,
> > + BMP280,
> > + BME280,
> > + BMP380,
> > +};
> > +
> > /* Regmap configurations */
> > extern const struct regmap_config bmp180_regmap_config;
> > extern const struct regmap_config bmp280_regmap_config;
>