Re: Re: [PATCH v12 3/4] pwm: dwc: add of/platform support

[Xuyang Dong]

> 
> On Di, 2026-07-21 at 20:09 +0800, dongxuyang@xxxxxxxxxxxxxxxxxx wrote:
> > From: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> > 
> > The dwc pwm controller can be used in non-PCI systems, so allow
> > either platform or OF based probing.
> > 
> > The controller is reset only when no PWM channel is enabled.
> > Otherwise, clocks are enabled and the runtime PM state is updated
> > to reflect the active hardware configuration.
> > 
> > The DWC PWM controller does not provide a hardware polarity bit.
> > Currently, the driver only supports active-low output, which is
> > incompatible with devices requiring active-high waveforms (e.g.,
> > backlight controllers, fan speed regulators).
> > 
> > Implement polarity control by exploiting the timer's dual load
> > registers. The hardware uses:
> > - LD_CNT:  LOW period count
> > - LD_CNT2: HIGH period count
> > 
> > The total period is defined as (LD_CNT + LD_CNT2). By swapping the
> > duty cycle between these registers, we invert the polarity while
> > keeping the period unchanged:
> > - PWM_POLARITY_NORMAL:  write duty_cycle to LD_CNT2 (HIGH period)
> > - PWM_POLARITY_INVERSED: write duty_cycle to LD_CNT  (LOW period)
> > 
> > Implementation:
> > Update both apply() and get_state() to handle state->polarity
> > consistently. Since the hardware does not store polarity, get_state()
> > returns the last successfully applied software state, ensuring that
> > read-back matches what was originally set.
> > 
> > Co-developed-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>
> > Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>
> > Signed-off-by: Xiang Xu <xuxiang@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Guosheng Wang <wangguosheng@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/pwm/Kconfig        |  10 ++
> >  drivers/pwm/Makefile       |   1 +
> >  drivers/pwm/pwm-dwc-core.c | 172 ++++++++++++++++----
> >  drivers/pwm/pwm-dwc-of.c   | 312 +++++++++++++++++++++++++++++++++++++
> >  drivers/pwm/pwm-dwc.h      |  25 ++-
> >  5 files changed, 481 insertions(+), 39 deletions(-)
> >  create mode 100644 drivers/pwm/pwm-dwc-of.c
> > 
> [...]
> > diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> > new file mode 100644
> > index 000000000000..460863549274
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-dwc-of.c
> > @@ -0,0 +1,312 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DesignWare PWM Controller driver OF
> > + *
> > + * Copyright (C) 2026 SiFive, Inc.
> > + */
> > +
> > +#define DEFAULT_SYMBOL_NAMESPACE "dwc_pwm_of"
> > +
> > +#include <linux/clk.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/pwm.h>
> > +#include <linux/reset.h>
> > +
> > +#include "pwm-dwc.h"
> > +
> > +struct dwc_pwm_plat_data {
> > +	bool reset_required;
> 
> This is not needed.
> 

Hi Philipp,

We will drop it in the next version.

> > +};
> > +
> > +static int dwc_pwm_plat_probe(struct platform_device *pdev)
> > +{
> [...]
> > +	pdata = device_get_match_data(dev);
> > +	if (pdata && pdata->reset_required)
> > +		dwc->rst = devm_reset_control_get_exclusive(dev, NULL);
> > +	else
> > +		dwc->rst = devm_reset_control_array_get_optional_exclusive(dev);
> 
> Please drop reset_required from pdata and just use
> 
> 	dwc->rst = devm_reset_control_array_get_optional_exclusive(dev);
> 
> That works for a single reset as well.
> 
> Device tree validation already makes sure required reset controls are
> present. There is no need handle this in the driver as a special case.
> 

Use dwc->rst = devm_reset_control_array_get_optional_exclusive(dev);

> > +
> > +	if (IS_ERR(dwc->rst))
> > +		return dev_err_probe(dev, PTR_ERR(dwc->rst),
> > +				     "failed to get reset control\n");
> > +
> > +	ret = clk_prepare_enable(dwc->bus_clk);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to enable bus clock\n");
> > +
> > +	ret = clk_prepare_enable(dwc->clk);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable timer clock\n");
> 
> 		dev_err_probe(dev, "failed to enable timer clock\n");
> 
> prints the actual error. Same for other dev_err()s in the probe
> function.
> 

Use dev_err_probe() instead of dev_err() in the probe function.

Best regards,
Xuyang Dong