| Message ID | 20241028142000.1058149-6-aren@peacevolution.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: stk3310: support powering off during suspend | expand |
On Mon, Oct 28, 2024 at 10:19:59AM -0400, Aren Moynihan wrote: > If the chip isn't powered, this call is likely to return an error. > Without a log here the driver will silently fail to probe. Common errors > are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus > isn't powered). The commit message does not explain why dev_err_probe() has been chosen and not simple dev_err().
On Mon, Oct 28, 2024 at 04:45:35PM +0200, Andy Shevchenko wrote: > On Mon, Oct 28, 2024 at 10:19:59AM -0400, Aren Moynihan wrote: > > If the chip isn't powered, this call is likely to return an error. > > Without a log here the driver will silently fail to probe. Common errors > > are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus > > isn't powered). > > The commit message does not explain why dev_err_probe() has been chosen > and not simple dev_err(). This function is only called from stk3310_probe, and this condition should propagate it's error, so it fits what dev_err_probe is designed for. dev_err would be pretty much equivalent just longer, like this: if (ret < 0) { dev_err(&client->dev, "failed to read chip it: %d\n", ret); return ret; } Regards - Aren
On Mon, Oct 28, 2024 at 11:29:35AM -0400, Aren wrote: > On Mon, Oct 28, 2024 at 04:45:35PM +0200, Andy Shevchenko wrote: > > On Mon, Oct 28, 2024 at 10:19:59AM -0400, Aren Moynihan wrote: > > > If the chip isn't powered, this call is likely to return an error. > > > Without a log here the driver will silently fail to probe. Common errors > > > are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus > > > isn't powered). > > > > The commit message does not explain why dev_err_probe() has been chosen > > and not simple dev_err(). > > This function is only called from stk3310_probe, and this condition > should propagate it's error, so it fits what dev_err_probe is designed > for. dev_err would be pretty much equivalent just longer, like this: > > if (ret < 0) { > dev_err(&client->dev, "failed to read chip it: %d\n", ret); > return ret; > } It's fine, the problem is the commit message in the patch. Please, update the commit message accordingly.
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c index 968a2cc59d09..d314659e7dc2 100644 --- a/drivers/iio/light/stk3310.c +++ b/drivers/iio/light/stk3310.c @@ -508,7 +508,7 @@ static int stk3310_init(struct iio_dev *indio_dev) ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid); if (ret < 0) - return ret; + return dev_err_probe(&client->dev, ret, "failed to read chip id\n"); ret = stk3310_check_chip_id(chipid); if (ret < 0)
If the chip isn't powered, this call is likely to return an error. Without a log here the driver will silently fail to probe. Common errors are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus isn't powered). Signed-off-by: Aren Moynihan <aren@peacevolution.org> --- Notes: Changes in v2: - use dev_err_probe drivers/iio/light/stk3310.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)