Received-SPF: pass (google.com: domain of linux-kernel+bounces-168624-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sat, 4 May 2024 12:49:39 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: =?UTF-8?B?QmFybmFiw6FzIEN6w6ltw6Fu?= <trabarni@gmail.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>, Rob Herring <robh@kernel.org>,
 Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley
 <conor+dt@kernel.org>, linux-iio@vger.kernel.org,
 linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Danila Tikhonov
 <danila@jiaxyga.com>
Subject: Re: [PATCH 1/2] iio: imu: bmi160: add support for bmi120
Message-ID: <20240504124939.5ba414ec@jic23-huawei>
In-Reply-To: <20240504-bmi120-v1-1-478470a85058@gmail.com>
References: <20240504-bmi120-v1-0-478470a85058@gmail.com>
	<20240504-bmi120-v1-1-478470a85058@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Sat, 04 May 2024 01:45:24 +0200
Barnab=C3=A1s Cz=C3=A9m=C3=A1n <trabarni@gmail.com> wrote:

> From: Danila Tikhonov <danila@jiaxyga.com>
>=20
> Add support for bmi120 low power variant of bmi160.
>=20
> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
> Co-developed-by: Barnab=C3=A1s Cz=C3=A9m=C3=A1n <trabarni@gmail.com>
> Signed-off-by: Barnab=C3=A1s Cz=C3=A9m=C3=A1n <trabarni@gmail.com>

Hi. A comment inline that is more about us doing things wrong
in the past than the changes you are making.  However, I would
like to fix that up whilst we are here.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/bmi160/bmi160_core.c | 24 ++++++++++++++++++++----
>  drivers/iio/imu/bmi160/bmi160_i2c.c  |  3 +++
>  drivers/iio/imu/bmi160/bmi160_spi.c  |  3 +++
>  3 files changed, 26 insertions(+), 4 deletions(-)
>=20
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi16=
0/bmi160_core.c
> index a77f1a8348ff..015801ad4d9a 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -26,6 +26,7 @@
>  #include "bmi160.h"
> =20
>  #define BMI160_REG_CHIP_ID	0x00
> +#define BMI120_CHIP_ID_VAL	0xD3
>  #define BMI160_CHIP_ID_VAL	0xD1
> =20
>  #define BMI160_REG_PMU_STATUS	0x03
> @@ -112,6 +113,11 @@
>  	.ext_info =3D bmi160_ext_info,				\
>  }
> =20
> +const u8 bmi_chip_ids[] =3D {
> +	BMI120_CHIP_ID_VAL,
> +	BMI160_CHIP_ID_VAL,
> +};
> +
>  /* scan indexes follow DATA register order */
>  enum bmi160_scan_axis {
>  	BMI160_SCAN_EXT_MAGN_X =3D 0,
> @@ -704,6 +710,16 @@ static int bmi160_setup_irq(struct iio_dev *indio_de=
v, int irq,
>  	return bmi160_probe_trigger(indio_dev, irq, irq_type);
>  }
> =20
> +static int bmi160_check_chip_id(const u8 chip_id)
> +{
> +	for (int i =3D 0; i < ARRAY_SIZE(bmi_chip_ids); i++) {
> +		if (chip_id =3D=3D bmi_chip_ids[i])
> +			return 0;

This falls into a miss design issue on the original driver based on
my understanding of what we should do with chip IDs back then.

My current thinking after discussing with DT maintainers is we should
at most print a warning if we fail to match an ID.

The intent being to allow for cases exactly like this one to work with
an older kernel if a fallback compatible is in use.

So whilst you are here, please could you relax the below error path
to just do a dev_warn() or similar and return 0 anyway.

Or better yet, push that warning print to the caller...

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

For reasons given above, whilst we are hear I'd like this to change to dev_=
warn() and
carry on as if we did have a match.

>  	}
> =20
> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160=
/bmi160_i2c.c
> index a081305254db..d0ec5301ad9a 100644
> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
> @@ -37,6 +37,7 @@ static int bmi160_i2c_probe(struct i2c_client *client)
>  }
> =20
>  static const struct i2c_device_id bmi160_i2c_id[] =3D {
> +	{"bmi120", 0},
>  	{"bmi160", 0},
>  	{}
>  };
> @@ -52,12 +53,14 @@ static const struct acpi_device_id bmi160_acpi_match[=
] =3D {
>  	 * the affected devices are from 2021/2022.
>  	 */
>  	{"10EC5280", 0},
> +	{"BMI0120", 0},
>  	{"BMI0160", 0},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(acpi, bmi160_acpi_match);
> =20
>  static const struct of_device_id bmi160_of_match[] =3D {
> +	{ .compatible =3D "bosch,bmi120" },
>  	{ .compatible =3D "bosch,bmi160" },
>  	{ },
>  };
> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160=
/bmi160_spi.c
> index 8b573ea99af2..9f40500132f7 100644
> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
> @@ -34,18 +34,21 @@ static int bmi160_spi_probe(struct spi_device *spi)
>  }
> =20
>  static const struct spi_device_id bmi160_spi_id[] =3D {
> +	{"bmi120", 0},
>  	{"bmi160", 0},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(spi, bmi160_spi_id);
> =20
>  static const struct acpi_device_id bmi160_acpi_match[] =3D {
> +	{"BMI0120", 0},
>  	{"BMI0160", 0},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(acpi, bmi160_acpi_match);
> =20
>  static const struct of_device_id bmi160_of_match[] =3D {
> +	{ .compatible =3D "bosch,bmi120" },
>  	{ .compatible =3D "bosch,bmi160" },
>  	{ },
>  };
>=20