Hi Uwe:
On Tue, Feb 11, 2025 at 11:36 PM Uwe Kleine-König <ukleinek@xxxxxxxxxx> wrote:
>
> Hello,
>
> On Tue, Feb 11, 2025 at 02:02:03PM +0600, Binbin Zhou wrote:
> > On Tue, Feb 11, 2025 at 12:26 AM Uwe Kleine-König <ukleinek@xxxxxxxxxx> wrote:
> > > On Tue, Dec 10, 2024 at 08:37:06PM +0800, Binbin Zhou wrote:
> > > > +static int pwm_loongson_suspend(struct device *dev)
> > > > +{
> > > > + struct pwm_chip *chip = dev_get_drvdata(dev);
> > > > + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > > > +
> > > > + ddata->lss.ctrl = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL);
> > > > + ddata->lss.duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY);
> > > > + ddata->lss.period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD);
> > > > +
> > > > + clk_disable_unprepare(ddata->clk);
> > > > +
> > > > + return 0;
> > >
> > > Is this needed assuming that before suspend the consumer stopped the
> > > PWM?
> >
> > Actually, I don't quite understand the problem you're pointing out. It
> > seems to me that the register and clk operations are required
> > regardless of the state of the pwm.
> > At least from the experimental results, the logic is now as expected.
> > Of course, I may be missing some critical information.
>
> When a PWM goes into suspend it's expected that its consumer already
> disabled it.
>
> Until I come around to do that properly in the core for all drivers, I
> think the right approach in a driver is:
>
> for (i = 0; i < chip->npwm; ++i) {
> if (chip->pwms[i].state.enabled)
> return -EBUSY;
> }
OK, I will add the approach into pwm_loongson_suspend().
Since our pwm is single channel, it can be changed to:
+ struct pwm_device *pwm = &chip->pwms[0];
+
+ if (pwm->state.enabled)
+ return -EBUSY;
>
> and if you then know that all PWMs are disabled, maybe you don't need to
> store all the registers you did in pwm_loongson_suspend()?
>
> Best regards
> Uwe
--
Thanks.
Binbin
