Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants

Angel Iglesias
2023-01-01T10:56:14Z

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@gmail.com> 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@gmail.com>
> >
> > 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;
>