> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@xxxxxxxxx>
> Sent: Wednesday, June 26, 2024 7:14 AM
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; Chris Brandt <Chris.Brandt@xxxxxxxxxxx>;
> andi.shyti@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> geert+renesas@xxxxxxxxx; magnus.damm@xxxxxxxxx; mturquette@xxxxxxxxxxxx; sboyd@xxxxxxxxxx;
> p.zabel@xxxxxxxxxxxxxx; wsa+renesas@xxxxxxxxxxxxxxxxxxxx
> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; Claudiu Beznea
> <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()
>
> Hi, Biju,
>
> On 25.06.2024 18:53, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@xxxxxxxxx>
> >> Sent: Tuesday, June 25, 2024 1:14 PM
> >> Subject: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >>
> >> pm_runtime_get_sync() may return with error. In case it returns with
> >> error
> >> dev->power.usage_count needs to be decremented.
> >> dev->pm_runtime_resume_and_get()
> >> takes care of this. Thus use it.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >> ---
> >>
> >> 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);
> >
> > As at the moment we don't know how to reproduce this error condition
> > Can we use WARN_ON_ONCE() instead to catch detailed error condition here??
>
> [1] states "So, naturally, use of WARN_ON() is also now discouraged much of the time". I've go with
> dev_err() or something similar.
WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once??
Currently we don't know how to trigger pm_runtime_resume_and_get() error
condition in our setup using a testapp and we are expecting an error may
happen in future. If at all there is an error in future, we need detailed
error info so that we can handle it and fix the bug.
Cheers,
Biju
>
> Thank you,
> Claudiu Beznea
>
> [1] https://lwn.net/Articles/969923/
>
> >
> > Cheers,
> > Biju
> >
> >> + 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.2
> >>
> >
