Re: [PATCH RFC 0/4] hwmon: Add fan tachometer support for Raspberry Pi RP1

[Guenter Roeck <linux@roeck-us.net>]

On 11/4/26 05:42, Andrea della Porta wrote:
> Hi,
> 
> On 17:54 Wed 28 Oct     , Andrea della Porta wrote:
>> On 09:13 Tue 27 Oct     , Guenter Roeck wrote:
>>> On Tue, Oct 27, 2026 at 10:51:15PM +0800, Gary Guo wrote:
>>>> On Tue Oct 27, 2026 at 9:48 PM CST, Guenter Roeck wrote:
>>>>> On Tue, Oct 27, 2026 at 10:49:48AM +0100, Andrea della Porta wrote:
>>>>>> Hi Guenter,
>>>>>>
>>>>>> Yes, I also proposed that in the cover letter because auxbus seems also a viable
>>>>>> solution.
>>>>>>
>>>>>> The current implementation is a consequence to the discussion about the companion
>>>>>> PWM driver, from here (and the revisions that followed):
>>>>>>
>>>>>> https://lore.kernel.org/all/cover.1775223441.git.andrea.porta@suse.com/
>>>>>>
>>>>>> To give a short and hopefully coherent view of the decisions, here's a breakdown:
>>>>>>
>>>>>> - first proposal: PWM driver + fan hwmon (to read the RPM), all in the same driver
>>>>>>
>>>>>> - V2: Uwe preferred to split the PWM driver from the hwmon one, so the hwmon was
>>>>>>    dropped from teh PWM driver in favor of a future (fan) driver. To accomplish this,
>>>>>>    I moved from readl/writel to regmap + syscon.
>>>>>>
>>>>>> - V8: Gary proposed to export a function to read the fan RPM as a viable
>>>>>>    solution. As a result, the regmap/syscon was dropped as it was no longer needed
>>>>>>    and the function has been exported from the PWM driver
>>>>>>
>>>>>> - new patchset for fan driver: the now independent hwmon moodle just call the
>>>>>>    exported function from thw PWM driver.
>>>>>>
>>>>>> If the auxiliary bus is indeed the preferred approach, I'd like to ensure we are
>>>>>> all aligned, as this would also require refactoring the PWM driver.
>>>>>>
>>>>>
>>>>> Let's say it that way: If you don't want to implement the auxiliary driver,
>>>>> and you insist using the exported function, I would prefer for you to go back
>>>>> to the first solution. The one thing I really dislike is conceptually broken
>>>>> code, and I consider the current approach broken. Your call, but the one thing
>>>>> I won't accept is the code with the exported function.
>>>>
>>>> I agree on the auxiliary driver part. However, I don't think EXPORT_SYMBOL_NS is
>>>> an issue, and regmap is not the right fit here:
>>>>
>>>> - Previous version of the PWM1 patch series publish regmap using
>>>>    of_syscon_register_regmap, but this cannot be undone -- which needs to happen
>>>>    on PWM device unbind (the RP1 PWM device is itself platform device, but RP1
>>>>    chip itself is PCIe device which must be removable).
>>>>
>>>> - The tachometer register is not its own separate register, but rather (re)using
>>>>    the phase control register. The phase control is not implemented in the RP1
>>>>    PWM driver on the list yet, so this is currently a simple register read -- I
>>>>    can see that RP1 PWM driver need to take special care to about the tachometer
>>>>    reading if the phase control is implemented.
>>>>
>>>>    For the two reasons above, I think the way that tachometer is read should
>>>>    really be an implementation detail of the PWM driver and not for the hwmon
>>>>    driver to worry about, so I suggested using EXPORT_SYMBOL_NS.
>>>>
>>>> - EXPORT_SYMBOL_NS is the documented way for exposing functionality from parent
>>>>    device to auxiliary drivers: https://docs.kernel.org/driver-api/auxiliary_bus.html
>>>>
>>>
>>> Let's agree to disagree, and keep the code in the pwm subsystem.
>>
>> And auxiliary bus it will be. I propose the following:
>>
>> - drivers/pwm/pwm-rp1.c: create an auxiliary device for the fan. The function to read
>>    the RPM will not be exported, but a function pointer will be passed through the
>>    private data to the fan driver. No need for regmap. To be taken by Uwe.
>>
>> - drivers/hwmon/rp1-hwmon.c: The auxiliary driver is probed against the device registered
>>    by the pwm driver. No imported function. To be taken by Guenter.
>>
>> In this way, we decouple the two drivers and we leverage the auxiliary bus paradigm.
>> Also, no DT node needed for the fan.
>>
>> Do we all agree on this approach?
> 
> A gentle reminder about this inquiry. Before refactoring both the PWM and the fan
> driver I'd like to be sure that the proposed approach is at least acceptable.
> 

This is just as bad as the exported symbol, as it creates a cross-module dependency
which I would strongly prefer to avoid. Given that this is actually suggested
in the guidance for auxiliary drivers (shiver), lets just keep the EXPORT_SYMBOL_NS
approach after all.

Guenter