Re: [PATCH 2/2] sbrmi: Add support for APML protocols

[Lee Jones <lee@kernel.org>]

On Tue, 14 May 2024, Guenter Roeck wrote:

> On 5/14/24 12:15, Chatradhi, Naveen Krishna wrote:
> > + @Misc and @MFD maintainers in CC
> > 
> > Hi
> > 
> > On 5/3/2024 3:56 AM, Guenter Roeck wrote:
> > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > > 
> > > 
> > > On 5/2/24 15:05, Naveen Krishna Chatradhi wrote:
> > > > From: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> > > > 
> > > > The present sbrmi module only support reporting power. However, AMD data
> > > > center processors support various system management functionality
> > > > Out-of-band over Advanced Platform Management Link APML.
> > > > 
> > > > Register a miscdevice, which creates a device /dev/sbrmiX with an IOCTL
> > > > interface for the user space to invoke the following protocols.
> > > >    - Mailbox read/write (already defined in sbrmi_mailbox_xfer())
> > > >    - CPUID read
> > > >    - MCAMSR read
> > > > 
> > > 
> > > This is not hardware monitoring functionality and would have to reside
> > > elsewhere, outside the hwmon subsystem.
> > 
> > I thought as much, please provide your opinion on the following approach.
> > 
> > Background: Present sbrmi under hwmon subsystem is probed as an i2c driver and reports power.
> > 
> > However, APML interface defines few other protocols to support OOB system management functionality.
> > 
> > As adding the core functionality of the APML interface in drivers/hwmon/sbrmi is not the correct approach.
> > 
> > We would like do the following
> > 
> > 1. Move the i2c client probe, misc device registration and rmi_mailbox_xfer() function to a drivers/misc.
> > 
> > 2. Add an MFD device with a cell, which probes the hwmon/sbrmi and continues to report power using the symbols exported by the misc/sbrmi.
> > 
> 
> afaik mfd API function must not be used outside drivers/mfd. The mfd maintainer
> is (or at least used to be) pretty strict on that. The core code of a
> multi-function device might better be implemented in drivers/mfd, with
> drivers in drivers/misc (for the misc device) and drivers/hwmon/ (for
> hwmon functionality). The misc and hwmon drivers could then communicate
> with the core using regmap.

Yes, please only use the MFD API from drivers/mfd.

There are 'offenders' that slipped by me, but in general if you need to
create an MFD then it should be located in the MFD subsystem.

> drivers/mailbox/ supports mailbox style communication. I don't know if that
> would apply to the mailbox functionality implemented here.

-- 
Lee Jones [李琼斯]