Hi Uwe:
Thanks for your review.
On Tue, Feb 11, 2025 at 12:26 AM Uwe Kleine-König <ukleinek@xxxxxxxxxx> wrote:
>
> Hello Binbin,
>
> On Tue, Dec 10, 2024 at 08:37:06PM +0800, Binbin Zhou wrote:
> > +static int pwm_loongson_probe(struct platform_device *pdev)
> > +{
> > + int ret;
> > + struct pwm_chip *chip;
> > + struct pwm_loongson_ddata *ddata;
> > + struct device *dev = &pdev->dev;
> > +
> > + chip = devm_pwmchip_alloc(dev, 1, sizeof(*ddata));
> > + if (IS_ERR(chip))
> > + return PTR_ERR(chip);
> > + ddata = to_pwm_loongson_ddata(chip);
> > +
> > + ddata->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(ddata->base))
> > + return PTR_ERR(ddata->base);
> > +
> > + ddata->clk = devm_clk_get_optional_enabled(dev, NULL);
> > + if (IS_ERR(ddata->clk))
> > + return dev_err_probe(dev, PTR_ERR(ddata->clk),
> > + "failed to get pwm clock\n");
> > + if (ddata->clk) {
> > + ret = devm_clk_rate_exclusive_get(dev, ddata->clk);
> > + if (ret)
> > + return ret;
>
> Error message please. Also please make all errors start with a capital
> letter.
Ok, I will do it.
>
> > + ddata->clk_rate = clk_get_rate(ddata->clk);
> > + } else {
> > + ddata->clk_rate = LOONGSON_PWM_FREQ_DEFAULT;
> > + }
> > +
> > + /* Explicitly initialize the CTRL register */
> > + pwm_loongson_writel(ddata, 0, LOONGSON_PWM_REG_CTRL);
>
> This disables all outputs, right? Ideally the driver takes over running
> channels. Consider the bootloader initialized a display with a splash
> screen. Disabling the PWM might disable the backlight of the display
> which hurts the visual experience.
Indeed, there may be similar problems.
I don't have similar hardware at the moment, and just in case, I think
it would be more appropriate to follow the pwm settings of the
bootloader.
I'll try to drop it in the next version.
>
> > + chip->ops = &pwm_loongson_ops;
> > + chip->atomic = true;
> > + dev_set_drvdata(dev, chip);
> > +
> > + ret = devm_pwmchip_add(dev, chip);
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret, "failed to add PWM chip\n");
> > +
> > + return 0;
> > +}
> > +
> > +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.
>
> Best regards
> Uwe
>
--
Thanks.
Binbin
