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

[Guenter Roeck]

On Tue, Oct 27, 2026 at 10:49:48AM +0100, Andrea della Porta wrote:
> Hi Guenter,
> 
> On 09:24 Mon 26 Oct     , Guenter Roeck wrote:
> > On 10/23/26 03:15, Andrea della Porta wrote:
> > > Hi,
> > > 
> > > This patch series adds fan tachometer / RPM monitoring support for the
> > > Raspberry Pi RP1 southbridge found on the Raspberry Pi 5.
> > > Please note that the patchset in [1] is a prerequisite for this one.
> > > 
> > > On the Raspberry Pi RP1, the active cooling fan speed is controlled via
> > > the RP1 PWM controller. In addition to generating PWM signals, the RP1
> > > monitors fan tachometer pulses. The pulse measurement and RPM calculation
> > > are performed autonomously by the RP1's onboard Cortex-M microcontroller
> > > core, which periodically writes the computed RPM value directly into
> > > dedicated MMIO registers located within the RP1 PWM hardware block.
> > > 
> > > During earlier discussions, Uwe Kleine-König explicitly indicated a
> > > preference to keep the core PWM controller driver (drivers/pwm/pwm-rp1.c)
> > > focused strictly on PWM duties, rather than bloating it by registering an
> > > hwmon device directly from within the PWM driver (see [2]).
> > > 
> > > Because the Linux PWM subsystem currently lacks a generic framework or
> > > consumer API for reporting hardware-calculated tachometer readings, this
> > > series implements a standalone hwmon driver (rp1-fan) using the following
> > > structure:
> > > 
> > > -  The PWM driver exposes and exports rp1_pwm_read_tachometer() within a
> > >     dedicated symbol namespace ('RP1_PWM') to read the RPM value from
> > >     the MMIO block.
> > > -  A companion DT binding (raspberrypi,rp1-fan.yaml) introduces a vendor
> > >     property, 'raspberrypi,pwm-tachometer', to link the fan node to the
> > >     PWM controller node without overloading or abusing the standard 'pwms'
> > >     property.
> > > -  The rp1-fan hwmon driver parses this phandle, establishes a device link
> > >     for proper runtime and unbind dependency management, and registers the
> > >     standard hwmon 'fan1_input' sysfs attribute.
> > > 
> > > An alternative architectural approach to the DT phandle + exported helper
> > > method would be to model the tachometer as a child device using the
> > > auxiliary bus framework (drivers/base/auxiliary.c). In that model, the RP1
> > > PWM driver would instantiate an auxiliary device for the tachometer, and
> > > the hwmon driver would bind to that auxiliary bus device without requiring
> > > a separate DT node.
> > > 
> > > However, adopting the auxiliary bus would require a more significant
> > > rework and refactoring of the existing RP1 PWM driver.
> > > 
> > > I'd appreciate any feedback on whether the current companion driver with
> > > a dedicated DT property is acceptable, or if an auxiliary bus-based
> > > refactoring of the PWM driver is the preferred upstream path.
> > > 
> > > Patch Breakdown
> > > ---------------
> > > Patch 1: pwm: rp1: Export helper to read fan tachometer RPM
> > > Patch 2: dt-bindings: hwmon: Add Raspberry Pi RP1 fan binding
> > > Patch 3: hwmon: Add driver for Raspberry Pi RP1 fan tachometer
> > 
> > This seems conceptually wrong.
> > 
> > Have you considered implementing the hwmon driver as auxiliary driver
> > and accessing the register in question through regmap ?
> 
> 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.

Guenter