From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
pm_runtime_get_sync() may return with error. In case it returns with error
dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get()
takes care of this. Thus use it.
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v2:
- delete i2c adapter all the time in remove
drivers/i2c/busses/i2c-riic.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 83e4d5e14ab6..002b11b020fa 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -113,6 +113,8 @@ struct riic_irq_desc {
char *name;
};
+static const char * const riic_rpm_err_msg = "Failed to runtime resume";
+
static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset)
{
writeb(val, riic->base + riic->info->regs[offset]);
@@ -133,10 +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
struct riic_dev *riic = i2c_get_adapdata(adap);
struct device *dev = adap->dev.parent;
unsigned long time_left;
- int i;
+ int i, ret;
u8 start_bit;
- pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret) {
+ dev_err(dev, riic_rpm_err_msg);
+ return ret;
+ }
if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
riic->err = -EBUSY;
@@ -301,6 +307,7 @@ static const struct i2c_algorithm riic_algo = {
static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
{
+ int ret;
unsigned long rate;
int total_ticks, cks, brl, brh;
struct device *dev = riic->adapter.dev.parent;
@@ -379,7 +386,11 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
t->scl_fall_ns / (1000000000 / rate),
t->scl_rise_ns / (1000000000 / rate), cks, brl, brh);
- pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret) {
+ dev_err(dev, riic_rpm_err_msg);
+ return ret;
+ }
/* Changing the order of accessing IICRST and ICE may break things! */
riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1);
@@ -498,11 +509,18 @@ static void riic_i2c_remove(struct platform_device *pdev)
{
struct riic_dev *riic = platform_get_drvdata(pdev);
struct device *dev = &pdev->dev;
+ int ret;
- pm_runtime_get_sync(dev);
- riic_writeb(riic, 0, RIIC_ICIER);
- pm_runtime_put(dev);
i2c_del_adapter(&riic->adapter);
+
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret) {
+ dev_err(dev, riic_rpm_err_msg);
+ } else {
+ riic_writeb(riic, 0, RIIC_ICIER);
+ pm_runtime_put(dev);
+ }
+
pm_runtime_disable(dev);
}
--
2.39.2Hi Claudiu,
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> index 83e4d5e14ab6..002b11b020fa 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -113,6 +113,8 @@ struct riic_irq_desc {
> char *name;
> };
>
> +static const char * const riic_rpm_err_msg = "Failed to runtime resume";
Please, don't do this. Much clearer to write the message
explicitly.
> +
> static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset)
> {
> writeb(val, riic->base + riic->info->regs[offset]);
> @@ -133,10 +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> struct riic_dev *riic = i2c_get_adapdata(adap);
> struct device *dev = adap->dev.parent;
> unsigned long time_left;
> - int i;
> + int i, ret;
> u8 start_bit;
>
> - pm_runtime_get_sync(dev);
> + ret = pm_runtime_resume_and_get(dev);
In principle I like the error message to be always checked and I
will always approve it. Whenever there is a return value, even
when we are sure it's always '0', it needs to be checked.
I had lots of discussions in the past about this topic but I
haven't always found support. I'd love to have the ack from a
renesas maintainer here.
> + if (ret) {
> + dev_err(dev, riic_rpm_err_msg);
> + return ret;
> + }
>
> if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
> riic->err = -EBUSY;
...
> @@ -498,11 +509,18 @@ static void riic_i2c_remove(struct platform_device *pdev)
> {
> struct riic_dev *riic = platform_get_drvdata(pdev);
> struct device *dev = &pdev->dev;
> + int ret;
>
> - pm_runtime_get_sync(dev);
> - riic_writeb(riic, 0, RIIC_ICIER);
> - pm_runtime_put(dev);
> i2c_del_adapter(&riic->adapter);
You swapped the position of this. Please put the
i2c_del_adapter() at the end.
Thanks,
Andi
> +
> + ret = pm_runtime_resume_and_get(dev);
> + if (ret) {
> + dev_err(dev, riic_rpm_err_msg);
> + } else {
> + riic_writeb(riic, 0, RIIC_ICIER);
> + pm_runtime_put(dev);
> + }
> +
> pm_runtime_disable(dev);
> }
>
> --
> 2.39.2
>
Hi Andi,
On Fri, Jul 5, 2024 at 12:42 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> > index 83e4d5e14ab6..002b11b020fa 100644
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -113,6 +113,8 @@ struct riic_irq_desc {
> > char *name;
> > };
> >
> > +static const char * const riic_rpm_err_msg = "Failed to runtime resume";
>
> Please, don't do this. Much clearer to write the message
> explicitly.
And the compiler will merge all identical strings, emitting
just a single string.
>
> > +
> > static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset)
> > {
> > writeb(val, riic->base + riic->info->regs[offset]);
> > @@ -133,10 +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > struct riic_dev *riic = i2c_get_adapdata(adap);
> > struct device *dev = adap->dev.parent;
> > unsigned long time_left;
> > - int i;
> > + int i, ret;
> > u8 start_bit;
> >
> > - pm_runtime_get_sync(dev);
> > + ret = pm_runtime_resume_and_get(dev);
>
> In principle I like the error message to be always checked and I
s/message/condition/?
> will always approve it. Whenever there is a return value, even
> when we are sure it's always '0', it needs to be checked.
>
> I had lots of discussions in the past about this topic but I
> haven't always found support. I'd love to have the ack from a
> renesas maintainer here.
I don't mind checking for the error here.
>
> > + if (ret) {
> > + dev_err(dev, riic_rpm_err_msg);
Do you need to print these error messages?
AFAIU, this cannot happen anyway.
Ultimately, I expect the device driver that requested the transfer to
handle failures, and print a message when needed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 05.07.2024 10:19, Geert Uytterhoeven wrote:
> Hi Andi,
>
> On Fri, Jul 5, 2024 at 12:42 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>>> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
>>> index 83e4d5e14ab6..002b11b020fa 100644
>>> --- a/drivers/i2c/busses/i2c-riic.c
>>> +++ b/drivers/i2c/busses/i2c-riic.c
>>> @@ -113,6 +113,8 @@ struct riic_irq_desc {
>>> char *name;
>>> };
>>>
>>> +static const char * const riic_rpm_err_msg = "Failed to runtime resume";
>>
>> Please, don't do this. Much clearer to write the message
>> explicitly.
>
> And the compiler will merge all identical strings, emitting
> just a single string.
>
>>
>>> +
>>> static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset)
>>> {
>>> writeb(val, riic->base + riic->info->regs[offset]);
>>> @@ -133,10 +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>>> struct riic_dev *riic = i2c_get_adapdata(adap);
>>> struct device *dev = adap->dev.parent;
>>> unsigned long time_left;
>>> - int i;
>>> + int i, ret;
>>> u8 start_bit;
>>>
>>> - pm_runtime_get_sync(dev);
>>> + ret = pm_runtime_resume_and_get(dev);
>>
>> In principle I like the error message to be always checked and I
>
> s/message/condition/?
>
>> will always approve it. Whenever there is a return value, even
>> when we are sure it's always '0', it needs to be checked.
>>
>> I had lots of discussions in the past about this topic but I
>> haven't always found support. I'd love to have the ack from a
>> renesas maintainer here.
>
> I don't mind checking for the error here.
>
>>
>>> + if (ret) {
>>> + dev_err(dev, riic_rpm_err_msg);
>
> Do you need to print these error messages?
No. I have it here as a result of some internal reviews.
Thank you,
Claudiu Beznea
> AFAIU, this cannot happen anyway.
> Ultimately, I expect the device driver that requested the transfer to
> handle failures, and print a message when needed.
>
> Gr{oetje,eeting}s,
>
> Geert
>
Hi Geert,
> > > static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset)
> > > {
> > > writeb(val, riic->base + riic->info->regs[offset]);
> > > @@ -133,10 +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > > struct riic_dev *riic = i2c_get_adapdata(adap);
> > > struct device *dev = adap->dev.parent;
> > > unsigned long time_left;
> > > - int i;
> > > + int i, ret;
> > > u8 start_bit;
> > >
> > > - pm_runtime_get_sync(dev);
> > > + ret = pm_runtime_resume_and_get(dev);
> >
> > In principle I like the error message to be always checked and I
>
> s/message/condition/?
yes :-)
> > will always approve it. Whenever there is a return value, even
> > when we are sure it's always '0', it needs to be checked.
> >
> > I had lots of discussions in the past about this topic but I
> > haven't always found support. I'd love to have the ack from a
> > renesas maintainer here.
>
> I don't mind checking for the error here.
>
> >
> > > + if (ret) {
> > > + dev_err(dev, riic_rpm_err_msg);
>
> Do you need to print these error messages?
I don't think it's needed, indeed.
> AFAIU, this cannot happen anyway.
That's what I was saying earlier. It's just a different point of
view.
To be honest, I don't really mind.
Thanks,
Andi
> Ultimately, I expect the device driver that requested the transfer to
> handle failures, and print a message when needed.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
© 2016 - 2026 Red Hat, Inc.