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

["Gary Guo" <gary@garyguo.net>]

On Wed Sep 23, 2026 at 5:28 PM BST, Andrea della Porta wrote:
> 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.

of_pwm_get also registers a device link. In general you shouldn't have to worry
about synchronization when using supplier-consumer APIs, as synchronization has
been taken care for you. Regulator APIs for example also handles device links.

If this isn't true.. Then I'll be a driver core design issue and not that of
drivers :)

I think sending this part with your fan driver would be a better idea, and we
shall see if Sashiko still complains :)

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

I don't think having debugfs alone is a convincing reason to use regmap.. Most
of the users who use this driver won't care.

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

Well, you have to handle the possibility of error, and the compiler needs to
generate bound checks, so it's not free?

A probe time check is good because once you checked that the register space is
large enough, you never have to check again for accesses.

Using regmap_read/write to provide the check will give a false sense of "things
are working" when code runs past the probe, but if, say, the device tree is
messed up. And it'll fail much later..

Best,
Gary