Re: [PATCH v8 2/2] misc: Add iop driver for Sunplus SP7021

[gregkh <gregkh@linuxfoundation.org>]

On Mon, Feb 07, 2022 at 08:29:40AM +0000, Tony Huang 黃懷厚 wrote:
> Dear Arnd:
> 
> > -----Original Message-----
> > From: Arnd Bergmann <arnd@arndb.de>
> > Sent: Monday, February 7, 2022 3:48 PM
> > To: Tony Huang <tonyhuang.sunplus@gmail.com>
> > Cc: Rob Herring <robh+dt@kernel.org>; DTML <devicetree@vger.kernel.org>;
> > Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Derek Kiernan
> > <derek.kiernan@xilinx.com>; Dragan Cvetic <dragan.cvetic@xilinx.com>; Arnd
> > Bergmann <arnd@arndb.de>; gregkh <gregkh@linuxfoundation.org>; Tony
> > Huang 黃懷厚 <tony.huang@sunplus.com>; Wells Lu 呂芳騰
> > <wells.lu@sunplus.com>
> > Subject: Re: [PATCH v8 2/2] misc: Add iop driver for Sunplus SP7021
> > 
> > On Mon, Feb 7, 2022 at 7:30 AM Tony Huang
> > <tonyhuang.sunplus@gmail.com> wrote:
> > >
> > > IOP(8051) embedded inside SP7021 which is used as Processor for I/O
> > > control, monitor RTC interrupt and cooperation with CPU & PMC in power
> > > management purpose.
> > > The IOP core is DQ8051, so also named IOP8051, it supports dedicated
> > > JTAG debug pins which share with SP7021.
> > > In standby mode operation, the power spec reach 400uA.
> > >
> > > Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> > > ---
> > > Changes in v8:
> > >  - Addressed comments from Greg KH.
> > >
> > >  Documentation/ABI/testing/sysfs-platform-soc@B |  28 ++
> > >  MAINTAINERS                                    |   2 +
> > >  drivers/misc/Kconfig                           |  20 ++
> > >  drivers/misc/Makefile                          |   1 +
> > >  drivers/misc/sunplus_iop.c                     | 463
> > +++++++++++++++++++++++++
> > >  5 files changed, 514 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-platform-soc@B
> > >  create mode 100644 drivers/misc/sunplus_iop.c
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-platform-soc@B
> > > b/Documentation/ABI/testing/sysfs-platform-soc@B
> > > new file mode 100644
> > > index 0000000..d26d6f5
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-platform-soc@B
> > > @@ -0,0 +1,28 @@
> > > +What:
> > /sys/devices/platform/soc@B/9c000400.iop/sp_iop_mailbox
> > > +Date:          January 2022
> > > +KernelVersion: 5.16
> > > +Contact:       Tony Huang <tonyhuang.sunplus@gmail.com>
> > > +Description:
> > > +               Show IOP's mailbox0 register data.
> > > +               Format: %x
> > > +
> > > +What:
> > /sys/devices/platform/soc@B/9c000400.iop/sp_iop_mode
> > > +Date:          January 2022
> > > +KernelVersion: 5.16
> > > +Contact:       Tony Huang <tonyhuang.sunplus@gmail.com>
> > > +Description:
> > > +               Read-Write.
> > > +
> > > +               Write this file.
> > > +               Operation mode of IOP is switched to standby mode by
> > writing
> > > +               "1" to sysfs.
> > > +               Operation mode of IOP is switched to normal mode by
> > writing
> > > +               "0" to sysfs.
> > > +               Writing of other values is invalid.
> > > +
> > > +               Read this file.
> > > +               Show operation mode of IOP. "0" is normal mode. "1" is
> > standby
> > > +               mode.
> > > +               Format: %x
> > 
> > As discussed before, I would suggest leaving out all custom attributes for now,
> > and first hooking up the driver to all the in-kernel subsystems.
> > 
> > The mailbox0 register data definitely feels like an implementation detail, not
> > something that should be exposed to user space as an interface.
> > 
> > For standby mode, this would normally be handled by the power management
> > subsystem in the kernel. not a custom interface. From your earlier description,
> > I assume this interface puts the main CPU into standby mode, not the IOP,
> > right?
> > 
> > CPU standby is handled by the cpuidle subsystem, so you need a driver in
> > drivers/cpuidle/ to replace your sysfs attribute.
> > If you plan to hook up the driver to multiple subsystems, keeping a generic
> > driver file is ok, so  you'll end up with two driver modules, with one of them
> > calling into the other, using
> > EXPORT_SYMBOL() to link between them.
> > 
> 
> The purpose of adding sysfs is only for users to debug.
> So this is not needed?

If this is only for debugging, please put it in debugfs and not sysfs.

thanks,

greg k-h