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

On 14:46 Wed 23 Sep     , Gary Guo wrote:

On Wed Sep 23, 2026 at 1:51 PM BST, Andrea della Porta wrote:

quoted

Hi Gary,
thanks for your feedback!

On 20:51 Tue 22 Sep     , Gary Guo wrote:

quoted

On Fri Sep 18, 2026 at 10:59 AM BST, Andrea della Porta wrote:

<...snip...>

quoted

quoted

quoted

+++ 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...

quoted

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.

quoted

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).

quoted

So Sashiko is kinda reporting a false positive here.

quoted

+
+	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.

quoted

- 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

quoted

- 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.