Re: [PATCH v9 2/3] pwm: rp1: Add RP1 PWM controller driver

[Andrea della Porta <andrea.porta@suse.com>]

On 14:46 Wed 23 Sep     , Gary Guo wrote:
> On Wed Sep 23, 2026 at 1:51 PM BST, Andrea della Porta wrote:
> > Hi Gary,
> > thanks for your feedback!
> >
> > On 20:51 Tue 22 Sep     , Gary Guo wrote:
> >> On Fri Sep 18, 2026 at 10:59 AM BST, Andrea della Porta wrote:

<...snip...>

> >> > +++ b/drivers/pwm/pwm-rp1.c
> >> > +
> >> > +int rp1_pwm_read_tachometer(struct device *dev)
> >> > +{
> >> > +	struct pwm_chip *chip;
> >> > +	struct rp1_pwm *rp1;
> >> > +	u32 tach_val;
> >> > +	int ret;
> >> > +
> >> > +	if (!dev)
> >> > +		return -EINVAL;
> >> > +
> >> > +	device_lock(dev);
> >> 
> >> You should use the device link mechanism for synchronizing unbind / runtime PM.
> >> That can be done in your RP1 fan driver, and it doesn't need locking on this
> >> driver.
> >
> > This does not enforce the locking though.
> 
> What locking do you think is needed?
> 
> Driver core takes care of ordering so you will never see a depended device
> going away while a dependant device is still bound. Your RP1 fan driver just
> need to make sure that it never calls this API after it's itself unbound --
> which it needs to guarantee anyway.

I think you're referring to the struct device (data) and module binary unloading
from memory, which is guaranteed not to happen when there is a dependent device
using it, but what happens when you try to unbind or suspend the device?
More on that below...

> 
> > Is it enough to just rely on
> > caller to create the device link in advance? IOW, just add a documentation
> > comment to the exported function prologue stating that the consumer is
> > responsible to sync via a device link is acceptable?
> 
> If you use pwm_get API then a device link is automatically created for you
> already (however, this automatic link does not pass runtime PM flags, so if you
> need that you still need to add link explicitly).

This driver implements only static PM ops so I think both pwm_get API or
device_link_add should deal automatically with races.
OTOH, what if the consumer obtains a reference to the PWM device via of_* API (or
other means)? Thhose calls does not create device link and we would still have unsync
critical paths.
I'm just trying to figure out whether I should design the exported function as
foolproof and caller agnostic wrt sync issues.
If relying on the caller to use pwm_get/device_link_add is enough, I'd be happy to
find out I'm just being overly paranoid, dropping all the locking to make teh code
simpler.

> 
> How this is supposed to be synchronized or documented is very hard to get right
> without seeing the user side driver -- if you already have a working version of
> the RP1 fan driver it might benefit to have it attached as a RFC patch in the

Agreed, that's why I was trying to be as consumer agnostic as possible. After all,
coupling two modules wrt their locking requirements is usually better to be avoided.
As for the sample fan driver, I have only a simple module that grab a reference
to the pwm device and call the exported tachometer function, so nothing really useful
for this discussion, yet.

> series; or an option to to drop the tachometer API and add it as part of the fan
> driver series.

That is a great advice. I think it's beneficial to discuss everything in one place,
and as a plus we won't block the pwm driver.

> 
> > Otherwise the only way I see to protect it in any scenario is via the mutex
> > I've already implemented and a second flag for the removing path (device_lock
> > will be dropped, of course).
> >
> >> 
> >> So Sashiko is kinda reporting a false positive here.
> >> 
> >> > +
> >> > +	chip = dev_get_drvdata(dev);
> >> > +	if (!chip) {
> >> > +		ret = -ENODEV;
> >> > +		goto err_dev_unlock;
> >> > +	}
> >> > +
> >> > +	rp1 = pwmchip_get_drvdata(chip);
> >> > +	if (!rp1) {
> >> > +		ret = -ENODEV;
> >> > +		goto err_dev_unlock;
> >> > +	}
> >> > +
> >> > +	mutex_lock(&rp1->lock);
> >> > +	if (!rp1->clk_enabled) {
> >> > +		ret = -EBUSY;
> >> > +		goto err_clk_unlock;
> >> > +	}
> >> > +
> >> > +	ret = regmap_read(rp1->regmap, RP1_PWM_PHASE(2), &tach_val);
> >> > +	if (ret)
> >> > +		goto err_clk_unlock;
> >> > +
> >> > +	ret = (int)tach_val;
> >> > +
> >> > +err_clk_unlock:
> >> > +	mutex_unlock(&rp1->lock);
> >> > +err_dev_unlock:
> >> > +	device_unlock(dev);
> >> > +
> >> > +	return ret;
> >> > +}
> >> > +EXPORT_SYMBOL_NS_GPL(rp1_pwm_read_tachometer, "RP1_PWM_FAN");
> >> > +
> >> > +static int rp1_pwm_probe(struct platform_device *pdev)
> >> > +{
> >> > +	struct device *dev = &pdev->dev;
> >> > +	unsigned long clk_rate;
> >> > +	struct pwm_chip *chip;
> >> > +	void __iomem	*base;
> >> > +	struct rp1_pwm *rp1;
> >> > +	int ret;
> >> > +
> >> > +	chip = devm_pwmchip_alloc(dev, RP1_PWM_NUM_PWMS, sizeof(*rp1));
> >> > +	if (IS_ERR(chip))
> >> > +		return PTR_ERR(chip);
> >> > +
> >> > +	rp1 = pwmchip_get_drvdata(chip);
> >> > +	ret = devm_mutex_init(dev, &rp1->lock);
> >> > +	if (ret)
> >> > +		return ret;
> >> > +
> >> > +	base = devm_platform_ioremap_resource(pdev, 0);
> >> > +	if (IS_ERR(base))
> >> > +		return PTR_ERR(base);
> >> > +
> >> > +	rp1->regmap = devm_regmap_init_mmio(dev, base, &rp1_pwm_regmap_config);
> >> > +	if (IS_ERR(rp1->regmap))
> >> > +		return dev_err_probe(dev, PTR_ERR(rp1->regmap), "Cannot initialize regmap\n");
> >> 
> >> You mentioned "rework regmap error paths" in cover letter.
> >> 
> >> But the issue is that regmap doesn't need be used here at all. RP1 is on PCIe
> >> so there is no need for bus abstraction, direct use of MMIO is sufficient.
> >> MMIO accessors have no error paths, so you're saying yourself from having to
> >> handle that, and also reduce the overhead by not having to go through an
> >> abstraction w/ indirect funicton calls.
> >>
> >> I suppose regmap was used when syscon was there; but it's not needed anymore.
> >
> > True, and I don't have any issue in converting back to MMIO call and drop the conditional for
> > error checking, but please consider the following, since the driver may be extended in the
> > future to support more features:
> >
> > - regmap gives you free debugfs view on the registers, which may be useful to test
> >   the new features.
> 
> Do you have any register that we want to access that is not part of the PWM
> facility, other than tachometer?

Not at the moment, no. But I don't see how this impact the debugfs usefulness.

> 
> > - regmap_write/read may still return an error in case the passed register is not in range.
> >   This will be trapped at runtime only, but could still be useful during development
> 
> I think this is rather a anti-feature. Having additional error paths for some
> thing that never happens is not a good idea, especially that you basically get 0
> coverage for these paths.

Sure. Well this is true once the code is crystallized and tested, so it's somewhat
still useful (only) during future development. But I got the point, and I agree.

> 
> You already know the shape of the register region, so the bounds checking
> provided by regmap would be better served by an ahead-of-time check:
> 
>     #define RP1_PWM_REG_MAX (RP1_PWM_DUTY(RP1_PWM_NUM_PWMS) + 4)
> 
>     struct resource *res;
>     base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>     if (IS_ERR(base))
>         return PTR_ERR(base);
> 
>     if (resource_size(res) < RP1_PWM_REG_MAX) ...

Fine for the probe method, but regmap_read/write also check for the range, for free.

> 
> Shameless plug: Rust abstractions for I/O access is actually pretty good in this
> regard in the sense that we try to prove statically that access canot fail. It
> also has PWM and platform abstractions. If you're interested in learning Rust
> this driver might be a good candidate :) If you're going to LPC we can chat
> about this there.

Now I'm tempted! :)
Although it will took far more time to upstream the driver so I think I have
to posticipate Rust for another driver.
Thanks for being so available for the LPC, really appreciated! Not sure if I
can make it this year but we'll see...

Regards,
Andrea

> 
> Best,
> Gary
> 
> > - I expect the PWM driver to be a access with very low frequency, so I guess the overhead 
> >   imposed by regmap is negligible.
> >
> > Since we already have it, I'd prefer to leave regmap if possible for the aforementioned reasons,
> > but I'm obviously open to drop it in favor of direct MMIO calls in case you or anyone else are
> > not seeing those as real benefits.
> 
>