MIME-Version: 1.0
References: <cover.1639557112.git.tonyhuang.sunplus@gmail.com>
 <c3a3b64c38807b2f344c3df500eb4c60b885eadf.1639557112.git.tonyhuang.sunplus@gmail.com>
 <CAK8P3a0CnCK-Dfodp-jTzZApM_1NpdY7DV2fRqnO=gaju5xGwg@mail.gmail.com>
 <CAHpW4oQmtd-gG1HYZT-Dk=QAvXKsKebzcOaVtrEeoujwoL9zSg@mail.gmail.com> <316c16afbff74160b07bd74444f3b8e1@sphcmbx02.sunplus.com.tw>
In-Reply-To: <316c16afbff74160b07bd74444f3b8e1@sphcmbx02.sunplus.com.tw>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 17 Dec 2021 09:12:57 +0100
Message-ID: <CAK8P3a0cA7iL=ug6hiqWAV1-qFSoCN-R7jrXv0cqByUDJV4x0Q@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] misc: Add iop driver for Sunplus SP7021
To:     =?UTF-8?B?VG9ueSBIdWFuZyDpu4Pmh7fljpo=?= <tony.huang@sunplus.com>
Cc:     Arnd Bergmann <arnd@arndb.de>, 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>,
        Rob Herring <robh+dt@kernel.org>,
        gregkh <gregkh@linuxfoundation.org>,
        =?UTF-8?B?V2VsbHMgTHUg5ZGC6Iqz6aiw?= <wells.lu@sunplus.com>,
        =?UTF-8?B?6buD5oe35Y6a?= <tonyhuang.sunplus@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Fri, Dec 17, 2021 at 3:44 AM Tony Huang 黃懷厚 <tony.huang@sunplus.com> wrote:
>
> Dear Arnd:
>
>
>
> On Thu, Dec 16, 2021 at 2:38 AM Tony Huang <tonyhuang.sunplus@gmail.com> wrote:
> >>
> >> IOP (IO Processor) embedded inside SP7021 which is used as
> >> Processor for I/O control, RTC wake-up 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 v4:
> >>  - Addressed comments from Arnd Bergmann.
>
> >I don't think you did: I asked you specifically to add code to interact with
> >the existing in-kernel interfaces to use the functionality provided by the
> >device. Pick any (at least two) subsystems and add support, but leave
> >out any custom user space interfaces (miscdevice, debugfs, sysfs, ...)
> >for the moment.
>
>
>
> 1. IOP can run sp_iop_platform_driver_shudown() through the poweroff command
> and the kernel. Perform system power-off actions.

Do you mean that this method a) cleanly shuts down the iop before the
system is powered down, or b) the driver_shutdown() callback is used to
initiate the powerdown of the system itself?

In case of a) I would not count that as exposing functionality, what you do here
is just part of any driver. If instead you are trying to use b), this
is the wrong
way of doing it, see drivers/power/reset/ for examples of how to do it right.

> 2. Wake up the system by relying on the 8051 internal RTC wake-up mechanism
> and external GPIO input signals to wake up.

I think those should be exposed with drivers/rtc for the RTC and drivers/gpio/
for the GPIO driver, and then you can use the device tree to configure which
GPIO to use as a wakeup and how it's connected to the RTC.

> 3.So you ask me to control IOP(8051) through file_operations, not through DEVICE_ATTR

No, neither of them. Use the appropriate drivers/*/ subsystems for any
functionality that has an existing subsystem. If there is something that the iop
does that does not yet have a subsystem, that requires a more thorough design
discussion for creating a new user interface, ideally in a hardware-independent
way. You should not start with that until all the normal features (rtc, wakeup,
suspend, gpio, ...) are supported.

       Arnd