> -----Original Message----- > From: Biju Das > Sent: Monday, July 8, 2024 6:37 AM > Subject: RE: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() > > Hi Andi and Claudiu, > > > -----Original Message----- > > From: Andi Shyti <andi.shyti@xxxxxxxxxx> > > Sent: Thursday, July 4, 2024 11:43 PM > > Subject: Re: [PATCH v2 04/12] i2c: riic: Use > > pm_runtime_resume_and_get() > > > > Hi 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) { > > Checking ret will lead to imbalance. It should be ret < 0 as ret = 1 corresponds to RPM_ACTIVE and > the API does not call put() when ret = 1; see [1] and [2] > > [1] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L778 > > [2] https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/pm_runtime.h#L431 I got confused. the code for pm_runtime_resume_and_get() seems correct. https://elixir.bootlin.com/linux/latest/source/include/linux/pm_runtime.h#L436 > > > Another question, pm_runtime_put [3] can also return error, don't we need to propagate that as well > to caller?? > > [3] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L1086 > > Cheers, > Biju