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

[Gary Guo]

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@xxxxxxxx/
>> 
>> 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

Best,
Gary