Hi Niklas, On Wed, Nov 03, 2021 at 11:10:32PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2021-11-03 21:46:53 +0100, Jacopo Mondi wrote: > > Read errors were silently going ignored. Fail louder to make sure such > > errors are visible. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx> > > --- > > drivers/media/i2c/max9271.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c > > index ff86c8c4ea61..aa9ab6831574 100644 > > --- a/drivers/media/i2c/max9271.c > > +++ b/drivers/media/i2c/max9271.c > > @@ -30,7 +30,7 @@ static int max9271_read(struct max9271_device *dev, u8 reg) > > > > ret = i2c_smbus_read_byte_data(dev->client, reg); > > if (ret < 0) > > - dev_dbg(&dev->client->dev, > > + dev_err(&dev->client->dev, > > This feels a bit illogical as all call sites handles the return code and > acts accordingly. For some it's OK to fail and for others where it's not > a dev_err() is reported, for example in max9271_verify_id(). > > Will this not log error messages in situations where there really is no Yes, that's the case now with my 2/2 applied. Basically I started this as pclk_detect was silently failing due to a sporadic read error, and I was not able to start the camera stream. I went all the way down from VIN to the very end of the pipeline increasing log verbosity and then I stumbled on this one. So yes, call sites handles the error code, but most of them also fail silently making debug even more painful than usual. > error? Maybe dev_info() is a better choice if you want to increase > verbosity? Yes, we could consider this. However, one could argue that errors in accessing the bus are anyway errors which is worth reporting, then the caller might decide if they're fatal or not... Thanks j > > > "%s: register 0x%02x read failed (%d)\n", > > __func__, reg, ret); > > > > -- > > 2.33.1 > > > > -- > Regards, > Niklas Söderlund
