Re: [PATCH v2 1/2] iio: imu: bmi160: add support for bmi120

[David Lechner]

On Sat, May 4, 2024 at 8:01 AM Barnabás Czémán <trabarni@xxxxxxxxx> wrote:
>
> From: Danila Tikhonov <danila@xxxxxxxxxxx>
>
> Add support for bmi120 low power variant of bmi160.
>
> Signed-off-by: Danila Tikhonov <danila@xxxxxxxxxxx>
> Co-developed-by: Barnabás Czémán <trabarni@xxxxxxxxx>
> Signed-off-by: Barnabás Czémán <trabarni@xxxxxxxxx>
> ---
>  drivers/iio/imu/bmi160/bmi160_core.c | 26 ++++++++++++++++++++------
>  drivers/iio/imu/bmi160/bmi160_i2c.c  |  3 +++
>  drivers/iio/imu/bmi160/bmi160_spi.c  |  3 +++
>  3 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index a77f1a8348ff..468aa80318fc 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -26,6 +26,7 @@
>  #include "bmi160.h"
>
>  #define BMI160_REG_CHIP_ID     0x00
> +#define BMI120_CHIP_ID_VAL     0xD3
>  #define BMI160_CHIP_ID_VAL     0xD1
>
>  #define BMI160_REG_PMU_STATUS  0x03
> @@ -112,6 +113,11 @@
>         .ext_info = bmi160_ext_info,                            \
>  }
>
> +const u8 bmi_chip_ids[] = {
> +       BMI120_CHIP_ID_VAL,
> +       BMI160_CHIP_ID_VAL,
> +};
> +
>  /* scan indexes follow DATA register order */
>  enum bmi160_scan_axis {
>         BMI160_SCAN_EXT_MAGN_X = 0,
> @@ -704,6 +710,16 @@ static int bmi160_setup_irq(struct iio_dev *indio_dev, int irq,
>         return bmi160_probe_trigger(indio_dev, irq, irq_type);
>  }
>
> +static int bmi160_check_chip_id(const u8 chip_id)
> +{
> +       for (int i = 0; i < ARRAY_SIZE(bmi_chip_ids); i++) {
> +               if (chip_id == bmi_chip_ids[i])
> +                       return 0;

It looks like this will match either chip to either ID. If we do this,
then why check the ID at all?

Another approach could be to put the chip ID as the match data in
bmi160_*_match, then you would get the right ID based on the
compatible string.

> +       }
> +
> +       return -ENODEV;
> +}
> +
>  static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
>  {
>         int ret;
> @@ -737,12 +753,10 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
>                 dev_err(dev, "Error reading chip id\n");
>                 goto disable_regulator;
>         }
> -       if (val != BMI160_CHIP_ID_VAL) {
> -               dev_err(dev, "Wrong chip id, got %x expected %x\n",
> -                       val, BMI160_CHIP_ID_VAL);
> -               ret = -ENODEV;
> -               goto disable_regulator;
> -       }
> +
> +       ret = bmi160_check_chip_id(val);
> +       if (ret)
> +               dev_warn(dev, "Chip id not found: %x\n", val);

This changes the error with probe failure to a warning, but the commit
message doesn't explain why. We always want to know why changes were
made. :-)

Should also probably be in a separate patch since changing the
behavior here is a separate change from adding support for a new chip.

>
>         ret = bmi160_set_mode(data, BMI160_ACCEL, true);
>         if (ret)

..