Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1644212476.git.tonyhuang.sunplus@gmail.com> <b96f039dc071b1d32bb52fa283fd8afc6d3349cc.1644212476.git.tonyhuang.sunplus@gmail.com>
In-Reply-To: <b96f039dc071b1d32bb52fa283fd8afc6d3349cc.1644212476.git.tonyhuang.sunplus@gmail.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Mon, 7 Feb 2022 08:47:48 +0100
Message-ID: <CAK8P3a0zNeaeOzC_tPb1KDbyktLpjUJCdEu=C6t_QX4pB9TKnQ@mail.gmail.com>
Subject: Re: [PATCH v8 2/2] misc: Add iop driver for Sunplus SP7021
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>,
        =?UTF-8?B?V2VsbHMgTHUg5ZGC6Iqz6aiw?= <wells.lu@sunplus.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

         Arnd