Message ID | 20200908224006.25636-9-digetx@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Improvements for Tegra I2C driver | expand |
Context | Check | Description |
---|---|---|
tagr/GVS | success | 1124251 |
tagr/GVS-1122491 | pending | None |
tagr/GVS-1122491 | fail | None |
tagr/GVS | pending | 1122533 |
tagr/GVS | success | 1122533 |
tagr/GVS | pending | 1124251 |
On Wed, Sep 09, 2020 at 01:39:40AM +0300, Dmitry Osipenko wrote: > The error message prints number of vIRQ, which isn't a useful information. > In practice devm_request_irq() never fails, hence let's remove the bogus > message in order to make code cleaner. > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) I think I have seen this fail occasionally when something's wrong in the IRQ code, or when we are not properly configuring the IRQ in device tree for example. So I'd prefer if this error message stayed here. I agree that it's not a terribly useful error message, so perhaps adding the error code to it would make it more helpful? Thierry > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index a52c72135390..b813c0976c10 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -1807,10 +1807,8 @@ static int tegra_i2c_probe(struct platform_device *pdev) > > ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr, > IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c_dev); > - if (ret) { > - dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq); > + if (ret) > goto release_dma; > - } > > i2c_set_adapdata(&i2c_dev->adapter, i2c_dev); > i2c_dev->adapter.owner = THIS_MODULE; > -- > 2.27.0 >
17.09.2020 14:28, Thierry Reding пишет: > On Wed, Sep 09, 2020 at 01:39:40AM +0300, Dmitry Osipenko wrote: >> The error message prints number of vIRQ, which isn't a useful information. >> In practice devm_request_irq() never fails, hence let's remove the bogus >> message in order to make code cleaner. >> >> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) > > I think I have seen this fail occasionally when something's wrong in the > IRQ code, or when we are not properly configuring the IRQ in device tree > for example. > > So I'd prefer if this error message stayed here. I agree that it's not a > terribly useful error message, so perhaps adding the error code to it > would make it more helpful? We have dtbs_check nowadays and I assume you only saw a such problem during of hardware bring-up, correct? Realistically, this error should never happen "randomly" and even if it will happen, then you will still know that I2C driver failed because driver-core will tell you about that. Hence there is no much value in the error message, it should be more practical to remove it in order to keep the code cleaner. >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index a52c72135390..b813c0976c10 100644 >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c >> @@ -1807,10 +1807,8 @@ static int tegra_i2c_probe(struct platform_device *pdev) >> >> ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr, >> IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c_dev); >> - if (ret) { >> - dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq); >> + if (ret) >> goto release_dma; >> - } >> >> i2c_set_adapdata(&i2c_dev->adapter, i2c_dev); >> i2c_dev->adapter.owner = THIS_MODULE; >> -- >> 2.27.0 >>
On Wed, 09 Sep 2020 01:39:40 +0300, Dmitry Osipenko wrote: > The error message prints number of vIRQ, which isn't a useful information. > In practice devm_request_irq() never fails, hence let's remove the bogus > message in order to make code cleaner. > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) Tested-by: Thierry Reding <treding@nvidia.com>
On Thu, Sep 17, 2020 at 05:59:28PM +0300, Dmitry Osipenko wrote: > 17.09.2020 14:28, Thierry Reding пишет: > > On Wed, Sep 09, 2020 at 01:39:40AM +0300, Dmitry Osipenko wrote: > >> The error message prints number of vIRQ, which isn't a useful information. > >> In practice devm_request_irq() never fails, hence let's remove the bogus > >> message in order to make code cleaner. > >> > >> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/i2c/busses/i2c-tegra.c | 4 +--- > >> 1 file changed, 1 insertion(+), 3 deletions(-) > > > > I think I have seen this fail occasionally when something's wrong in the > > IRQ code, or when we are not properly configuring the IRQ in device tree > > for example. > > > > So I'd prefer if this error message stayed here. I agree that it's not a > > terribly useful error message, so perhaps adding the error code to it > > would make it more helpful? > > We have dtbs_check nowadays and I assume you only saw a such problem > during of hardware bring-up, correct? dtbs_check is far from perfect, especially since only a handful of bindings have been converted for Tegra. It's also not going to catch any functional issues. You could still have a valid combination of flags passed via DTB and still the interrupt allocation could fail because it just so happens that the particular combination wasn't valid in that particular setup. > Realistically, this error should never happen "randomly" and even if it > will happen, then you will still know that I2C driver failed because > driver-core will tell you about that. And yes, this is the type of error that you typically get during bring-up and it does obviously go away once you've fixed it and then tends not to come back. But that's exactly what happens with most errors and having error messages helps in finding what went wrong. If we drop the error message, then I may notice that the I2C driver didn't probe correctly. But in order to find out why it didn't, I have to go and add error messages to narrow down where I need to look. The whole point of having error messages in the code is to avoid having to go in and modify the code in order to troubleshoot. In the interest of moving this along I'm fine with this for now. But I suspect that we'll run into an issue with this eventually and that we'll have to add an error message here again. If that happens we can always reintroduce one, and perhaps at that time do a better job of making it actually useful. Reviewed-by: Thierry Reding <treding@nvidia.com>
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index a52c72135390..b813c0976c10 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -1807,10 +1807,8 @@ static int tegra_i2c_probe(struct platform_device *pdev) ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr, IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c_dev); - if (ret) { - dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq); + if (ret) goto release_dma; - } i2c_set_adapdata(&i2c_dev->adapter, i2c_dev); i2c_dev->adapter.owner = THIS_MODULE;