Hi Jacopo, On 2021-11-04 09:30:58 +0100, Jacopo Mondi wrote: > 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. Is that not a verbose issue that should be addressed at the call sites and not the read wrapper? > > > 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... I would argue, either any register read failures are fatal or non of them are and each call site needs to decide how to deal with it. > > Thanks > j > > > > > "%s: register 0x%02x read failed (%d)\n", > > > __func__, reg, ret); > > > > > > -- > > > 2.33.1 > > > > > > > -- > > Regards, > > Niklas Söderlund -- Kind Regards, Niklas Söderlund
