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

[Guenter Roeck]

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