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

[Guenter Roeck]

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

Let's agree to disagree, and keep the code in the pwm subsystem.

Thanks,
Guenter