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

[Greg KH]

On Fri, Dec 24, 2021 at 04:35:56PM +0800, Tony Huang 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@xxxxxxxxx>
> ---
> Changes in v5:
>  - Modify sysfs read/write function.
>  - Added gpio pin for 8051 wake up linux kernel. 
> 
>  Documentation/ABI/testing/sysfs-platform-soc@B |  18 +
>  MAINTAINERS                                    |   2 +
>  drivers/misc/Kconfig                           |  12 +
>  drivers/misc/Makefile                          |   1 +
>  drivers/misc/sunplus_iop.c                     | 481 +++++++++++++++++++++++++
>  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..b0c54b5
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-soc@B
> @@ -0,0 +1,18 @@
> +What:		/sys/devices/platform/soc@B/9c000400.iop/sp_iop_mailbox
> +Date:		December 2021
> +KernelVersion:	5.15

5.15 was alread released, this can not be added to older kernels.

> +Contact:	Tony Huang <tonyhuang.sunplus@xxxxxxxxx>
> +Description:
> +		Show 8051 mailbox data.
> +
> +What:		/sys/devices/platform/soc@B/9c000400.iop/sp_iop_mode
> +Date:		December 2021
> +KernelVersion:	5.15
> +Contact:	Tony Huang <tonyhuang.sunplus@xxxxxxxxx>
> +Description:
> +		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.

You are not documenting what reading these sysfs files show.  Remember,
sysfs is "one value per file" and it looks like you are printing out
many values in the same file:

> +static ssize_t sp_iop_mailbox_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct sp_iop *iop = dev_get_drvdata(dev);
> +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> +	unsigned int MB0, MB1, MB2;
> +
> +	MB0 = readl(&p_iop_reg->iop_data0);
> +	MB1 = readl(&p_iop_reg->iop_data1);
> +	MB2 = readl(&p_iop_reg->iop_data2);
> +	return sprintf(buf, "MB0:0x%x MB1:0x%x MB2:0x%x\n", MB0, MB1, MB2);

That is a very odd "single value".

Also please use sysfs_emit().

> +static ssize_t sp_iop_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct sp_iop *iop = dev_get_drvdata(dev);
> +
> +	if (iop->mode == 0)
> +		return sprintf(buf, "normal code\n");
> +	else
> +		return sprintf(buf, "standby code\n");

2 words?