Dear Arnd:
> -----Original Message-----
> From: Arnd Bergmann <arnd@xxxxxxxx>
> Sent: Wednesday, November 17, 2021 4:28 PM
> To: Tony Huang <tonyhuang.sunplus@xxxxxxxxx>
> Cc: robh+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; derek.kiernan@xxxxxxxxxx;
> dragan.cvetic@xxxxxxxxxx; arnd@xxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; Wells
> Lu 呂芳騰 <wells.lu@xxxxxxxxxxx>; Tony Huang 黃懷厚
> <tony.huang@xxxxxxxxxxx>
> Subject: Re: [PATCH 2/2] misc: Add iop driver for Sunplus SP7021
>
> On Wed, Nov 17, 2021 at 7:48 AM Tony Huang
> <tonyhuang.sunplus@xxxxxxxxx> wrote:
> >
> > Add iop driver for Sunplus SP7021
> >
> > Signed-off-by: Tony Huang <tony.huang@xxxxxxxxxxx>
>
> A driver like this needs a long description of multiple paragraphs to explain
> what it does, why you need a custom user space interface etc.
I will add long description.
>
> > +config SUNPLUS_IOP
> > + tristate "Sunplus IOP support"
> > + default ARCH_SUNPLUS
> > + help
> > + Sunplus I/O processor (8051) driver.
> > + Processor for I/O control, RTC wake-up proceduce management,
> > + and cooperation with CPU&PMC in power management.
> > + Need Install DQ8051, The DQ8051 bin file generated by keil C.
> > diff --git a/drivers/misc/iop/Makefile b/drivers/misc/iop/Makefile new
> > file mode 100644 index 0000000..cb67634
> > --- /dev/null
> > +++ b/drivers/misc/iop/Makefile
> > @@ -0,0 +1,13 @@
> > +#
> > +# Makefile for the IOP module drivers.
> > +#
> > +
> > + # call from kernel build system
> > +
> > + obj-$(CONFIG_SUNPLUS_IOP) += sunplus_iop.o
> > + obj-$(CONFIG_SUNPLUS_IOP) += hal_iop.o
> > + obj-$(CONFIG_SUNPLUS_IOP) += iopnormal.o
> > + obj-$(CONFIG_SUNPLUS_IOP) += iopstandby.o
>
> Remove the extra whitespace here
I will remove it.
>
> > +clean:
> > + rm -rf *.o *~ core .depend .*.cmd *.ko *.mod.c .tmp_versions
> > \ No newline at end of file
>
> Replace the explicit 'rm' command with Kbuild listings for these files.
> Most of the files are already covered by the regular 'clean' target, so you
> should only need special handling for stuff you build separately as well.
I will modify it.
>
> > diff --git a/drivers/misc/iop/hal_iop.c b/drivers/misc/iop/hal_iop.c
> > new file mode 100644 index 0000000..6a72c8b
> > --- /dev/null
> > +++ b/drivers/misc/iop/hal_iop.c
> > @@ -0,0 +1,495 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include "hal_iop.h"
>
> One fundamental rule for Linux kernel drivers is that we assume that we know
> which OS we are running on, so there is no need for abstracting the operating
> system away from the driver. Please fold your 'hal' into the top-level driver
> accordingly and remove any unneded indirections between the two. I suppose
> you could have everything in one file if after you remove the firmware from
> the driver, and no longer require any headers.
I will modify it.
>
> > +//#define DEBUG_MESSAGE
> > +//#define early_printk
> > +
> > +#define IOP_KDBG_INFO
> > +#define IOP_FUNC_DEBUG
> > +#define IOP_KDBG_ERR
> > +#ifdef IOP_KDBG_INFO
> > + #define FUNC_DEBUG() pr_info("K_IOP: %s(%d)\n", __func__,
> __LINE__)
> > +#else
> > + #define FUNC_DEBUG()
> > +#endif
> > +
> > +#ifdef IOP_FUNC_DEBUG
> > +#define DBG_INFO(fmt, args ...) pr_info("K_IOP: " fmt, ## args)
> > +#else
> > +#define DBG_INFO(fmt, args ...)
> > +#endif
> > +
> > +#ifdef IOP_KDBG_ERR
> > +#define DBG_ERR(fmt, args ...) pr_err("K_IOP: " fmt, ## args) #else
> > +#define DBG_ERR(fmt, args ...) #endif
>
> These should all go away, try using dev_dbg() / dev_info() / dev_err()
> preferably.
I will modify it.
>
> > +void hal_iop_init(void __iomem *iopbase) {
> > + struct regs_iop_t *pIopReg = (struct regs_iop_t *)iopbase;
> > + unsigned long *IOP_base_for_normal = (unsigned long
> *)SP_IOP_RESERVE_BASE;
> > + unsigned char *IOP_kernel_base;
> > + unsigned int reg;
> > +
> > + IOP_kernel_base = (unsigned char *)ioremap((unsigned
> long)IOP_base_for_normal,
> > + NORMAL_CODE_MAX_SIZE);
> > +
> > + memset((unsigned char *)IOP_kernel_base, 0,
> NORMAL_CODE_MAX_SIZE);
> > + memcpy((unsigned char *)IOP_kernel_base, IopNormalCode,
> NORMAL_CODE_MAX_SIZE);
> > + writel(0x00100010, (void __iomem *)(B_SYSTEM_BASE + 32*4*0 +
> > + 4*1));
>
> All the type casts should be removed after you use the correct types:
> 'void __iomem *'
> for MMIO tokens, and 'void *' for addressable memory. You can use the
> 'sparse' tool by running 'make C=1' to check for any mismatched __iomem
> annotations.
I will modify it.
>
> To write to an __iomem area, you can use memcpy_to_io() and memset_io()
>
> > diff --git a/drivers/misc/iop/hal_iop.h b/drivers/misc/iop/hal_iop.h
> > new file mode 100644 index 0000000..d83f9ea
> > --- /dev/null
> > +++ b/drivers/misc/iop/hal_iop.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later*/
> > +
> > +#ifndef __IOP_HAL_H__
> > +#define __IOP_HAL_H__
> > +
> > +#include <linux/types.h>
> > +#include <linux/module.h>
> > +#include "reg_iop.h"
> > +
> > +void hal_iop_init(void __iomem *iopbase); void
> > +hal_iop_load_normal_code(void __iomem *iopbase); void
> > +hal_iop_load_standby_code(void __iomem *iopbase); void
> > +hal_iop_normalmode(void __iomem *iopbase); void
> > +hal_iop_standbymode(void __iomem *iopbase); void
> > +hal_iop_get_iop_data(void __iomem *iopbase); void
> > +hal_iop_set_iop_data(void __iomem *iopbase, unsigned int num,
> > +unsigned int value); void hal_gpio_init(void __iomem *iopbase,
> > +unsigned char gpio_number); void hal_iop_suspend(void __iomem
> > +*iopbase, void __iomem *ioppmcbase); void hal_iop_shutdown(void
> > +__iomem *iopbase, void __iomem *ioppmcbase); void
> hal_iop_S1mode(void
> > +__iomem *iopbase); void hal_iop_set_reserve_base(void __iomem
> > +*iopbase); void hal_iop_set_reserve_size(void __iomem *iopbase);
>
> When you fold those into the actual driver but decide to keep the functions
> separate, try to always pass a pointer to the device instance as the first
> argument. This is the way we do object oriented programming in the kernel.
I will modify it.
>
> > +#define NORMAL_CODE_MAX_SIZE 0X10000
> > +#define STANDBY_CODE_MAX_SIZE 0x4000
> > +extern const unsigned char IopNormalCode[]; extern const unsigned
> > +char IopStandbyCode[]; extern bool iop_code_mode; extern unsigned
> > +long SP_IOP_RESERVE_BASE; extern unsigned long SP_IOP_RESERVE_SIZE;
> > +extern unsigned int RECEIVE_CODE_SIZE; extern unsigned char
> > +NormalCode[]; extern unsigned char StandbyCode[]; #endif /*
> > +__IOP_HAL_H__ */
>
> There should generally be no global variables. Instead, these should go into
> per-instance structures.
I will modify it.
>
> Regarding the naming, avoid CamelCase and use e.g. 'iop_standby_code'
> instead of 'IopStandbyCode'.
I will modify it.
>
> > diff --git a/drivers/misc/iop/iop_ioctl.h
> > b/drivers/misc/iop/iop_ioctl.h new file mode 100644 index
> > 0000000..195388b
> > --- /dev/null
> > +++ b/drivers/misc/iop/iop_ioctl.h
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later*/
> > +/*
> > + *
> > + *iop_ioctl.h
> > + *
> > + */
> > +
> > +#ifndef _IOP_IOCTL_H
> > +#define _IOP_IOCTL_H
> > +
> > +#include <linux/ioctl.h>
> > +
> > +struct ioctl_cmd {
> > + unsigned int reg;
> > + unsigned int offset;
> > + unsigned int val;
> > +};
> > +
> > +#define IOC_MAGIC 'i'
> > +
> > +#define IOCTL_VALSET _IOW(IOC_MAGIC, 1, struct ioctl_cmd) #define
> > +IOCTL_VALGET _IOR(IOC_MAGIC, 2, struct ioctl_cmd)
> > +
> > +#endif
>
> ioctl interfaces are usually the hardest part of a driver, the best way to do this
> is to completely avoid inventing your own interfaces and using what the kernel
> already has, extending the existing interfaces if possible.
>
> If you end up having to add your own ioctls, do high-level functions rather than
> low-level register access, and create a separate ioctl command for each
> action.
I will modify it.
>
> > diff --git a/drivers/misc/iop/iopnormal.c
> > b/drivers/misc/iop/iopnormal.c new file mode 100644 index
> > 0000000..a49a376
> > --- /dev/null
> > +++ b/drivers/misc/iop/iopnormal.c
> > @@ -0,0 +1,4106 @@
> > +const unsigned char IopNormalCode[] = { 0x02, 0x1E, 0x00, 0x02, 0xFF,
> > +0xFF, 0x2E, 0x05, 0x02, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xB0, 0x05,
> > +0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x02, 0xFF, 0xFF,
> > +0xFF, 0x08, 0x75, 0xCD, 0x05, 0x00, 0x09, 0x75, 0x04, 0x02, 0x8F,
> > +0x81, 0x75, 0x52, 0x78, 0x73, 0x04, 0x13, 0xEF, 0xFF, 0xE6,
>
> As I understand, this is the binary firmware that gets loaded into the iop.
>
> The way we normally handle firmware loading in the kernel is to use the
> 'request_firmware()' interface from the driver to load the binary from a file in
> the root file system.
I will try to use request_firmware().
>
>
> > diff --git a/drivers/misc/iop/reg_iop.h b/drivers/misc/iop/reg_iop.h
> > new file mode 100644 index 0000000..829aa76
> > --- /dev/null
> > +++ b/drivers/misc/iop/reg_iop.h
> > @@ -0,0 +1,93 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later*/ #ifndef __REG_IOP_H__
> > +#define __REG_IOP_H__ #ifdef CONFIG_SOC_SP7021 #include
> > +<mach/io_map.h> #endif
>
> There should not be any conditional on the type of the SOC here, the driver
> object needs to work in a kernel that includes support for multiple SoCs.
>
> Note that there are no mach/*.h headers for modern SoCs any more, so the
> platform code needs to be updated accordingly. In particular, any MMIO
> register locations need to come from the devicetree, not from a hardcoded
> header.
I will modify it.
>
> > +struct regs_iop_moon0_t {
> > + unsigned int stamp;/* 00 */
> > + unsigned int clken[10];/* 01~10 */
> > + unsigned int gclken[10];/* 11~20 */
> > + unsigned int reset[10];/* 21~30 */
> > + unsigned int sfg_cfg_mode;/* 31 */ };
>
> Drop the '_t' suffix on the structure names, those are commonly associated
> with names for typedefs, not structures.
>
> Most driver writers find it easier to define register indexes as macros rather
> than structure definitions, but if all registers are 32-bit wide, then this works as
> well.
>
> Using 'u32' as the type instead of 'unsigned int' would make this more explicit.
I will modify it.
>
> > +#ifdef IOP_KDBG_INFO
> > +#define FUNC_DEBUG() pr_info("K_IOP: %s(%d)\n", __func__, __LINE__)
> > +#else
> > +#define FUNC_DEBUG()
> > +#endif
> > +
> > +#ifdef IOP_FUNC_DEBUG
> > +#define DBG_INFO(fmt, args ...) pr_info("K_IOP: " fmt, ## args)
> > +#else
> > +#define DBG_INFO(fmt, args ...)
> > +#endif
> > +
> > +#ifdef IOP_KDBG_ERR
> > +#define DBG_ERR(fmt, args ...) pr_err("K_IOP: " fmt, ## args) #else
> > +#define DBG_ERR(fmt, args ...) #endif
> > +/* ----------------------------------------------------------------------------------------------
> */
> > +#define IOP_REG_NAME "iop"
> > +#define IOP_PMC_REG_NAME "iop_pmc"
> > +
> > +#define DEVICE_NAME "sunplus,sp7021-iop"
>
> Remove all those and open-code the correct contents in the users.
I will remove it.
>
> > +static ssize_t setgpio_store(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count) {
> > + int ret = count;
> > + unsigned char num[1];
> > + unsigned int setnum;
> > + unsigned long val;
> > + ssize_t status;
> > +
> > + DBG_INFO("iop_store_setgpio\n");
> > + num[0] = buf[0];
> > + status = kstrtoul(buf, 16, &val);
> > + if (status)
> > + return status;
> > + setnum = val;
> > + DBG_INFO("set gpio number = %x\n", IOP_GPIO);
> > + hal_gpio_init(iop->iop_regs, IOP_GPIO);
> > + return ret;
> > +}
>
> GPIO access should be handled through the gpio subsystem by creating an
> instance of a gpio_chip, which provides interfaces for both in-kernel and
> user-space users.
I will modify it.
>
> > +static ssize_t S1mode_show(struct device *dev, struct device_attribute
> *attr,
> > + char *buf)
> > +{
> > + ssize_t len = 0;
> > +
> > + hal_iop_standbymode(iop->iop_regs);
> > + mdelay(10);//Need time to update iop_data
> > + hal_iop_S1mode(iop->iop_regs);
> > + return len;
> > +}
> > +
> > +static ssize_t S1mode_store(struct device *dev, struct device_attribute
> *attr,
> > + const char *buf, size_t count) {
> > + ssize_t len = 0;
> > +
> > + return len;
> > +}
>
> Each new device attribute needs to be documented in Documentation/ABI/
I will documented for new attribute in Documentation/ABI/.
>
> Arnd
