| Message ID | 20211103204654.223699-2-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: max9271: Fix pclk_detect silent failure | expand |
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@jmondi.org> > --- > 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 error? Maybe dev_info() is a better choice if you want to increase verbosity? > "%s: register 0x%02x read failed (%d)\n", > __func__, reg, ret); > > -- > 2.33.1 >
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@jmondi.org> > > --- > > 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
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@jmondi.org> > > > --- > > > 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
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, "%s: register 0x%02x read failed (%d)\n", __func__, reg, ret);
Read errors were silently going ignored. Fail louder to make sure such errors are visible. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/max9271.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)