Re: [PATCH] iio: imu: bmi270: use dev_warn for unexpected chip id

[Jonathan Cameron]

On Mon, 16 Mar 2026 20:11:48 -0300
Rodrigo Gobbi <rodrigo.gobbi.7@xxxxxxxxx> wrote:

> An unexpected chip id read from hardware indicates a potential
> failure for detecting id, which needs a more appropriate level
> of verbosity.
> 
> Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@xxxxxxxxx>

I've always been a bit flexible on the level of these warnings, but the
counter argument is that we are only not failing probe on these mismatches
to enable dt fallback compatibles. Those enable newer fully backwards
compatible parts to work on older kernels by specifying both the new
compatible and an older one. IIRC there is a similar concept in ACPI but
it's not as commonly used.

For those it is arguable we should be silent, but a compromise to help
with debugging simple 'wrong device' cases was to print a message.

Sometimes we have had device classes where reads of all 0s or all 1s
(typical failure to communicate cases) resulted in an error but all
else is accepted. That only works if we have some info from the
manufacturer that they definitely will never use those IDs (or really
strong gut feeling that they have more sense on SPI at least -
i2c at least has detectable indications of no one is replying).

Anyhow, generally I let the original author pick if they want
dev_info() or dev_warn(),  Maybe we should have a rule on it, but
then we get into questions on whether making it more noisy (e.g.
dev_warn()) might be perceived as a minor regression and get users
calling up the help desk after a kernel upgrade.

So my feeling is probably to leave this one alone though I'm open
to a wider discussion.

Jonathan


> ---
> I was exploring Bosch BMI270 driver and noticed a possible dev_info usage that might
> not be appropriate it regarding its verbosity level. Let's consider that probe definition
> at [1], used by [2] spi version and [3] i2 version.
> The i2c or spi callers will fill chip_info and then, probe will try to validate it at _init function:
> 
> // drivers/iio/imu/bmi270/bmi270_core.c
> int bmi270_core_probe(struct device *dev, struct regmap *regmap,
> 		      const struct bmi270_chip_info *chip_info)          
>     data->chip_info = chip_info;
> ...
> 	ret = bmi270_chip_init(data);
> ....
> 
> static int bmi270_chip_init(struct bmi270_data *data)
> {
> 	int ret;
> 
> 	ret = bmi270_validate_chip_id(data);
> 	if (ret)
>         return ret;
> ....
> 
> from init, chipid will be read from hardware but if the value is not expected with
> the one from the caller, i2c or spi, it will trigger a dev_info and not a warning:
> 
> // valid ids for i2c and spi
> #define BMI260_CHIP_ID_VAL				0x27
> #define BMI270_CHIP_ID_VAL				0x24
> 
> static int bmi270_validate_chip_id(struct bmi270_data *data)
> {
> 	int chip_id;
> 	int ret;
> 	struct device *dev = data->dev;
> 	struct regmap *regmap = data->regmap;
> 
> 	ret = regmap_read(regmap, BMI270_CHIP_ID_REG, &chip_id);
> 	if (ret)
> 		return dev_err_probe(dev, ret, "Failed to read chip id");
> 
> ....
> 	if (chip_id != data->chip_info->chip_id)
> 		dev_info(dev, "Unexpected chip id 0x%x", chip_id);
> 
> 	if (chip_id == bmi260_chip_info.chip_id)
> 		data->chip_info = &bmi260_chip_info;
> 	else if (chip_id == bmi270_chip_info.chip_id)
> 		data->chip_info = &bmi270_chip_info;
> 
> 	return 0;
> }
> 
> The chip_info will be correct due the caller matching DT or ACPI before, and here,
> driver is only double-checking the value at hardware and printing if it is not matching.
> Printing as info can be confusing, since this is more like a warning than info. 
> I don't have the hw here to test it, but I'm suggesting to just change the level
> of that msg.
> Tks and regards.
> 
> [1] https://github.com/torvalds/linux/blob/2d1373e4246da3b58e1df058374ed6b101804e07/drivers/iio/imu/bmi270/bmi270_core.c#L1599
> [2] https://github.com/torvalds/linux/blob/2d1373e4246da3b58e1df058374ed6b101804e07/drivers/iio/imu/bmi270/bmi270_spi.c#L65
> [3] https://github.com/torvalds/linux/blob/2d1373e4246da3b58e1df058374ed6b101804e07/drivers/iio/imu/bmi270/bmi270_i2c.c#L32
> ---
>  drivers/iio/imu/bmi270/bmi270_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> index 2ad230788532..a2f90ac22873 100644
> --- a/drivers/iio/imu/bmi270/bmi270_core.c
> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> @@ -1473,7 +1473,7 @@ static int bmi270_validate_chip_id(struct bmi270_data *data)
>  		return -ENODEV;
>  
>  	if (chip_id != data->chip_info->chip_id)
> -		dev_info(dev, "Unexpected chip id 0x%x", chip_id);
> +		dev_warn(dev, "Unexpected chip id 0x%x", chip_id);
>  
>  	if (chip_id == bmi260_chip_info.chip_id)
>  		data->chip_info = &bmi260_chip_info;