| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <be145af2101749aaa7749b779e9efed9@sphcmbx02.sunplus.com.tw>
Date: Wed, 9 Mar 2022 08:31:51 +0000
From: Tony Huang 黃懷厚 <tony.huang@...plus.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
Tony Huang <tonyhuang.sunplus@...il.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"derek.kiernan@...inx.com" <derek.kiernan@...inx.com>,
"Dragan.cvetic@...inx.com" <Dragan.cvetic@...inx.com>,
"arnd@...db.de" <arnd@...db.de>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC: Wells Lu 呂芳騰 <wells.lu@...plus.com>
Subject: RE: [PATCH v10 2/2] misc: Add iop driver for Sunplus SP7021
Dear Krzysztof:
I will modify the code to follow your comments
Thanks
> Subject: Re: [PATCH v10 2/2] misc: Add iop driver for Sunplus SP7021
>
> On 07/03/2022 06:25, Tony Huang wrote:
> > This driver is load 8051 bin code.
> > Processor for I/O control:
> > SP7021 has its own GPIO device driver.
> > The user can configure the gpio pin for 8051 use.
> > The 8051 support wake-up with IR key after system poweroff.
> >
> > Monitor RTC interrupt:
> > SP7021 system has its own RTC device driver (rtc-sunplus.c).
> > The user can set the RTC wake-up time through rtc-sunplus.c.
> > The IOP code does not need to increase the RTC subsystem function,
> > set the RTC register. When the linux kernel system is poweroff.
> > Only the 8051 CPU has power and can monitor the RTC wake-up interrupt.
> >
> > PMC in power management purpose:
> > Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
> > When the poweroff command is executed.
> > The 8051 has a register to control the power-on and power-off of the
> > system. If you turn off the power through the 8051
> > register(DEF_PWR_EN_0=0). The current measured by the circuit board is
> > 0.4mA only. In order to save power.
> >
> > Signed-off-by: Tony Huang <tonyhuang.sunplus@...il.com>
> > ---
> > Changes in v10:
> > - Added sp_iop_poweroff function for poweroff command.
> >
> > MAINTAINERS | 1 +
> > drivers/misc/Kconfig | 36 ++++
> > drivers/misc/Makefile | 1 +
> > drivers/misc/sunplus_iop.c | 438
> > +++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 476 insertions(+)
> > create mode 100644 drivers/misc/sunplus_iop.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index 6f336c9..11ecefa 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18246,6 +18246,7 @@ SUNPLUS IOP DRIVER
> > M: Tony Huang <tonyhuang.sunplus@...il.com>
> > S: Maintained
> > F: Documentation/devicetree/bindings/misc/sunplu-iop.yaml
> > +F: drivers/misc/sunplus_iop.c
> >
> > SUPERH
> > M: Yoshinori Sato <ysato@...rs.sourceforge.jp>
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index
> > 0f5a49f..3106f15 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -470,6 +470,42 @@ config HISI_HIKEY_USB
> > switching between the dual-role USB-C port and the USB-A host ports
> > using only one USB controller.
> >
> > +config SUNPLUS_IOP
> > + tristate "Sunplus IOP support"
> > + default ARCH_SUNPLUS
> > + help
> > + This driver is load 8051 bin code.
> > + Processor for I/O control:
> > + SP7021 has its own GPIO device driver.
> > + The user can configure the gpio pin for 8051 use.
> > + 8051 support wake-up with IR key after system poweroff.
> > +
> > + Monitor RTC interrupt:
> > + SP7021 system has its own RTC device driver (rtc-sunplus.c).
> > + The user can set the RTC wake-up time through rtc-sunplus.c.
> > + The IOP code does not need to increase the RTC subsystem function,
> > + set the RTC register. When the linux kernel system is poweroff.
> > + Only the 8051 CPU has power and can monitor the RTC wake-up
> interrupt.
> > +
> > + PMC in power management purpose:
> > + Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
> > + When the poweroff command is executed.
> > + The 8051 has a register to control the power-on and power-off of the
> system.
> > + If you turn off the power through the 8051 register(DEF_PWR_EN_0=0),
> > + The current measured by the circuit board is 0.4mA only. In order to
> save power.
> > +
> > + The IOP core is DQ8051, so also named IOP8051.
> > + Need Install DQ8051, The DQ8051 bin file generated by keil C.
> > + We will provide users with 8051 normal and standby source code.
> > + Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
> > +
> How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source-
> files-for-IOP
> > + Users can follow the operation steps to generate normal.bin and
> standby.bin.
> > + Path:
> https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP8051
> > + 26.5?How To Create 8051 bin file
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called sunplus_iop.
> > +
> > source "drivers/misc/c2port/Kconfig"
> > source "drivers/misc/eeprom/Kconfig"
> > source "drivers/misc/cb710/Kconfig"
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index
> > a086197..eafeab6 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -52,6 +52,7 @@ obj-$(CONFIG_DW_XDATA_PCIE) += dw-xdata-pcie.o
> > obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o
> > obj-$(CONFIG_OCXL) += ocxl/
> > obj-$(CONFIG_BCM_VK) += bcm-vk/
> > +obj-$(CONFIG_SUNPLUS_IOP) += sunplus_iop.o
> > obj-y += cardreader/
> > obj-$(CONFIG_PVPANIC) += pvpanic/
> > obj-$(CONFIG_HABANA_AI) += habanalabs/
> > diff --git a/drivers/misc/sunplus_iop.c b/drivers/misc/sunplus_iop.c
> > new file mode 100644 index 0000000..03301b4
> > --- /dev/null
> > +++ b/drivers/misc/sunplus_iop.c
> > @@ -0,0 +1,438 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * The IOP driver for Sunplus SP7021
> > + *
> > + * Copyright (C) 2021 Sunplus Technology Inc.
> > + *
> > + * All Rights Reserved.
> > + */
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/firmware.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_gpio.h>
> > +
>
> Except comments from Arnd also:
>
> > +enum IOP_Status_e {
> > + IOP_SUCCESS, /* successful */
> > + IOP_ERR_IOP_BUSY, /* IOP is busy */
> > +};
>
> please do not redefine true/false or ERRNO.
>
> > +
> > +/* moon0 register offset */
> > +#define IOP_CLKEN0 0x04
> > +#define IOP_RESET0 0x54
> > +
> > +/* IOP register offset */
> > +#define IOP_CONTROL 0x00
> > +#define IOP_DATA0 0x20
> > +#define IOP_DATA1 0x24
> > +#define IOP_DATA2 0x28
> > +#define IOP_DATA3 0x2c
> > +#define IOP_DATA4 0x30
> > +#define IOP_DATA5 0x34
> > +#define IOP_DATA6 0x38
> > +#define IOP_DATA7 0x3c
> > +#define IOP_DATA8 0x40
> > +#define IOP_DATA9 0x44
> > +#define IOP_DATA10 0x48
> > +#define IOP_DATA11 0x4c
> > +#define IOP_BASE_ADR_L 0x50
> > +#define IOP_BASE_ADR_H 0x54
> > +
> > +/* PMC register offset */
> > +#define IOP_PMC_TIMER 0x00
> > +#define IOP_PMC_CTRL 0x04
> > +#define IOP_XTAL27M_PASSWORD_I 0x08
> > +#define IOP_XTAL27M_PASSWORD_II 0x0c
> > +#define IOP_XTAL32K_PASSWORD_I 0x10
> > +#define IOP_XTAL32K_PASSWORD_II 0x14
> > +#define IOP_CLK27M_PASSWORD_I 0x18
> > +#define IOP_CLK27M_PASSWORD_II 0x1c
> > +#define IOP_PMC_TIMER2 0x20
> > +
> > +#define NORMAL_CODE_MAX_SIZE 0X1000 /* Max size of normal.bin
> that can be received */
> > +#define STANDBY_CODE_MAX_SIZE 0x4000 /* Max size of standby.bin
> that can be received */
>
> Empty line.
>
> > +struct sp_iop {
> > + struct device *dev;
> > + struct mutex write_lock; /* avoid parallel access */
>
> This comment is useless. It is obvious that mutex is used to avoid parallel
> access, what else could it be used?
>
> Instead, you need to describe which members/variables/parts of code are
> protected with the mutex.
>
> > + void __iomem *iop_regs;
> > + void __iomem *pmc_regs;
> > + void __iomem *moon0_regs;
> > + int irq;
> > + int gpio_wakeup;
> > + unsigned char iop_normal_code[NORMAL_CODE_MAX_SIZE];
> > + unsigned char iop_standby_code[STANDBY_CODE_MAX_SIZE];
>
> Why not using firmware? include/linux/firmware.h? Do not reimplement
> existing solutions...
>
> > + resource_size_t iop_mem_start;
> > + resource_size_t iop_mem_size;
> > + unsigned char bin_code_mode;
> > +};
> > +
> > +static struct sp_iop *iop_poweroff;
> > +
> > +static void sp_iop_run_normal_code(struct sp_iop *iop) {
> > + void __iomem *iop_kernel_base;
> > + unsigned int reg;
> > +
> > + iop_kernel_base = ioremap(iop->iop_mem_start,
> NORMAL_CODE_MAX_SIZE);
> > + memset(iop_kernel_base, 0, NORMAL_CODE_MAX_SIZE);
> > + memcpy(iop_kernel_base, iop->iop_normal_code,
> NORMAL_CODE_MAX_SIZE);
> > +
> > + writel(0x00100010, iop->moon0_regs + IOP_CLKEN0);
> > +
> > + reg = readl(iop->iop_regs + IOP_CONTROL);
> > + reg |= 0x01;
> > + writel(reg, iop->iop_regs + IOP_CONTROL);
> > +
> > + reg = readl(iop->iop_regs + IOP_CONTROL);
> > + reg &= ~(0x8000);
> > + writel(reg, iop->iop_regs + IOP_CONTROL);
> > +
> > + reg = readl(iop->iop_regs + IOP_CONTROL);
> > + reg |= 0x0200;// disable watchdog event reset IOP
> > + writel(reg, iop->iop_regs + IOP_CONTROL);
> > +
> > + reg = (iop->iop_mem_start & 0xFFFF);
> > + writel(reg, iop->iop_regs + IOP_BASE_ADR_L);
> > + reg = (iop->iop_mem_start >> 16);
> > + writel(reg, iop->iop_regs + IOP_BASE_ADR_H);
> > +
> > + reg = readl(iop->iop_regs + IOP_CONTROL);
> > + reg &= ~(0x01);
> > + writel(reg, iop->iop_regs + IOP_CONTROL);
> > + iop->bin_code_mode = 0;
> > +}
> > +
> > +static void sp_iop_run_standby_code(struct sp_iop *iop) {
> > + void __iomem *iop_kernel_base;
> > + unsigned long reg;
> > +
> > + iop_kernel_base = ioremap(iop->iop_mem_start,
> STANDBY_CODE_MAX_SIZE);
> > + memset(iop_kernel_base, 0, STANDBY_CODE_MAX_SIZE);
> > + memcpy(iop_kernel_base, iop->iop_standby_code,
> > +STANDBY_CODE_MAX_SIZE);
> > +
> > + writel(0x00100010, iop->moon0_regs + IOP_CLKEN0);
> > +
> > + reg = readl(iop->iop_regs + IOP_CONTROL);
> > + reg |= 0x01;
> > + writel(reg, iop->iop_regs + IOP_CONTROL);
> > +
> > + reg = readl(iop->iop_regs + IOP_CONTROL);
> > + reg &= ~(0x8000);
> > + writel(reg, iop->iop_regs + IOP_CONTROL);
> > +
> > + reg = readl(iop->iop_regs + IOP_CONTROL);
> > + reg |= 0x0200;// disable watchdog event reset IOP
> > + writel(reg, iop->iop_regs + IOP_CONTROL);
> > +
> > + reg = (iop->iop_mem_start & 0xFFFF);
> > + writel(reg, iop->iop_regs + IOP_BASE_ADR_L);
> > + reg = (iop->iop_mem_start >> 16);
> > + writel(reg, iop->iop_regs + IOP_BASE_ADR_H);
> > +
> > + reg = readl(iop->iop_regs + IOP_CONTROL);
> > + reg &= ~(0x01);
> > + writel(reg, iop->iop_regs + IOP_CONTROL);
> > + iop->bin_code_mode = 1;
> > +}
> > +
> > +/* 8051 informs linux kerenl. 8051 has been switched to standby.bin code.
> */
> > +#define IOP_READY 0x0004
> > +#define RISC_READY 0x0008
> > +
> > +/* System linux kernel tells 8051 which gpio pin to wake-up through. */
> > +#define WAKEUP_PIN 0xFE02
> > +
> > +/* System linux kernel tells 8051 to execute S1 or S3 mode. */
> > +#define S1 0x5331
> > +#define S3 0x5333
>
> Keep defines together, so beginning of file.
>
> > +
> > +static int sp_iop_s3mode(struct device *dev, struct sp_iop *iop) {
> > + unsigned int reg;
> > + int ret, value;
> > +
> > + /* PMC set */
> > + writel(0x00010001, iop->pmc_regs + IOP_PMC_TIMER);
> > + reg = readl(iop->pmc_regs + IOP_PMC_CTRL);
> > + /* disable system reset PMC, enalbe power down IOP Domain, enable
> gating clock 27Mhz */
> > + reg |= 0x23;
> > + writel(reg, iop->pmc_regs + IOP_PMC_CTRL);
> > +
> > + writel(0x55aa00ff, iop->pmc_regs + IOP_XTAL27M_PASSWORD_I);
> > + writel(0x00ff55aa, iop->pmc_regs + IOP_XTAL27M_PASSWORD_II);
> > + writel(0xaa00ff55, iop->pmc_regs + IOP_XTAL32K_PASSWORD_I);
> > + writel(0xff55aa00, iop->pmc_regs + IOP_XTAL32K_PASSWORD_II);
> > + writel(0xaaff0055, iop->pmc_regs + IOP_CLK27M_PASSWORD_I);
> > + writel(0x5500aaff, iop->pmc_regs + IOP_CLK27M_PASSWORD_II);
> > + writel(0x01000100, iop->pmc_regs + IOP_PMC_TIMER2);
> > +
> > + /* IOP Hardware IP reset */
> > + reg = readl(iop->moon0_regs + IOP_RESET0);
> > + reg |= 0x10;
> > + writel(reg, (iop->moon0_regs + IOP_RESET0));
> > + reg &= ~(0x10);
> > + writel(reg, (iop->moon0_regs + IOP_RESET0));
> > +
> > + writel(0x00ff0085, (iop->moon0_regs + 32 * 4 * 1 + 4 * 1));
> > +
> > + reg = readl(iop->moon0_regs + 32 * 4 * 1 + 4 * 2);
> > + reg |= 0x08000800;
> > + writel(reg, (iop->moon0_regs + 32 * 4 * 1 + 4 * 2));
> > +
> > + writel(WAKEUP_PIN, iop->iop_regs + IOP_DATA0);
> > + writel(iop->gpio_wakeup, iop->iop_regs + IOP_DATA1);
> > +
> > + ret = readl_poll_timeout(iop->iop_regs + IOP_DATA2, value,
> > + (value & IOP_READY) == IOP_READY, 1000, 10000);
> > + if (ret) {
> > + dev_err(dev, "timed out\n");
> > + return ret;
> > + }
> > +
> > + reg = RISC_READY;
> > + writel(reg, iop->iop_regs + IOP_DATA2);
> > + reg = 0x0000;
> > + writel(reg, iop->iop_regs + IOP_DATA5);
> > + reg = 0x0060;
> > + writel(reg, iop->iop_regs + IOP_DATA6);
> > +
> > + ret = readl_poll_timeout(iop->iop_regs + IOP_DATA7, value,
> > + value == 0xaaaa, 1000, 10000);
> > + if (ret) {
> > + dev_err(dev, "timed out\n");
> > + return ret;
> > + }
> > +
> > + /* 8051 bin file call Ultra low function. */
> > + writel(0xdd, iop->iop_regs + IOP_DATA1);
> > + /*
> > + * When the execution is here, the system linux kernel
> > + * is about to be powered off
> > + * The purpose of mdelay(10): Do not let the system linux
> > + * kernel continue to run other programs.
> > + */
> > + mdelay(10);
> > + return 0;
> > +}
> > +
> > +static int sp_iop_s1mode(struct device *dev, struct sp_iop *iop) {
> > + int ret, value;
> > +
> > + ret = readl_poll_timeout(iop->iop_regs + IOP_DATA2, value,
> > + (value & IOP_READY) == IOP_READY, 1000, 10000);
> > + if (ret) {
> > + dev_err(dev, "timed out\n");
> > + return ret;
> > + }
> > +
> > + writel(RISC_READY, iop->iop_regs + IOP_DATA2);
> > + writel(0x0000, iop->iop_regs + IOP_DATA5);
> > + writel(0x0060, iop->iop_regs + IOP_DATA6);
> > +
> > + ret = readl_poll_timeout(iop->iop_regs + IOP_DATA7, value,
> > + value == 0xaaaa, 1000, 10000);
>
> Hundreds of magical constants... Almost all of these should be defined or
> described.
>
> > + if (ret) {
> > + dev_err(dev, "timed out\n");
> > + return ret;
> > + }
> > +
> > + /* 8051 bin file call S1_mode function. */
> > + writel(0xee, iop->iop_regs + IOP_DATA1);
> > + /*
> > + * When the execution is here, the system linux kernel
> > + * is about to be powered off
> > + * The purpose of mdelay(10): Do not let the system linux
> > + * kernel continue to run other programs.
> > + */
> > + mdelay(10);
> > + return 0;
> > +}
> > +
> > +static int sp_iop_get_normal_code(struct device *dev, struct sp_iop
> > +*iop) {
> > + const struct firmware *fw;
> > + static const char file[] = "normal.bin";
> > + unsigned int err, i;
> > +
> > + err = request_firmware(&fw, file, dev);
> > + if (err) {
> > + dev_err(dev, "get bin file error\n");
> > + return err;
> > + }
> > +
> > + for (i = 0; i < NORMAL_CODE_MAX_SIZE; i++) {
> > + char temp;
> > +
> > + temp = fw->data[i];
> > + iop->iop_normal_code[i] = temp;
> > + }
> > + release_firmware(fw);
> > + return err;
> > +}
> > +
> > +static int sp_iop_get_standby_code(struct device *dev, struct sp_iop
> > +*iop) {
> > + const struct firmware *fw;
> > + static const char file[] = "standby.bin";
> > + unsigned int err, i;
> > +
> > + err = request_firmware(&fw, file, dev);
> > + if (err) {
> > + dev_err(dev, "get bin file error\n");
> > + return err;
> > + }
> > +
> > + for (i = 0; i < STANDBY_CODE_MAX_SIZE; i++) {
> > + char temp;
> > +
> > + temp = fw->data[i];
> > + iop->iop_standby_code[i] = temp;
> > + }
> > + release_firmware(fw);
> > + return err;
> > +}
> > +
> > +static void sp_iop_poweroff(void)
> > +{
> > + struct sp_iop *iop = iop_poweroff;
> > + unsigned int ret, value;
> > +
> > + value = readl(iop->iop_regs + IOP_DATA11);
> > + sp_iop_run_standby_code(iop);
> > +
> > + ret = readl_poll_timeout(iop->iop_regs + IOP_DATA0, value,
> > + value == 0x2222, 1000, 100000);
> > + if (ret)
> > + dev_warn(iop->dev, "timed out\n");
> > +
> > + if (value == S1)
> > + sp_iop_s1mode(iop->dev, iop);
> > + else
> > + sp_iop_s3mode(iop->dev, iop);
> > +}
> > +
> > +static int sp_iop_get_resources(struct platform_device *pdev, struct
> > +sp_iop *p_sp_iop_info) {
> > + struct resource *r;
> > +
> > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iop");
> > + p_sp_iop_info->iop_regs = devm_ioremap_resource(&pdev->dev, r);
> > + if (IS_ERR(p_sp_iop_info->iop_regs)) {
> > + dev_err(&pdev->dev, "ioremap fail\n");
> > + return PTR_ERR(p_sp_iop_info->iop_regs);
> > + }
> > +
> > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "iop_pmc");
> > + p_sp_iop_info->pmc_regs = devm_ioremap_resource(&pdev->dev, r);
> > + if (IS_ERR(p_sp_iop_info->pmc_regs)) {
> > + dev_err(&pdev->dev, "ioremap fail\n");
> > + return PTR_ERR(p_sp_iop_info->pmc_regs);
> > + }
> > +
> > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "moon0");
> > + p_sp_iop_info->moon0_regs = devm_ioremap_resource(&pdev->dev, r);
> > + if (IS_ERR(p_sp_iop_info->moon0_regs)) {
> > + dev_err(&pdev->dev, "ioremap fail\n");
> > + return PTR_ERR(p_sp_iop_info->moon0_regs);
> > + }
> > + return IOP_SUCCESS;
> > +}
> > +
> > +static int sp_iop_platform_driver_probe(struct platform_device *pdev)
> > +{
> > + int ret = -ENXIO;
> > + int rc;
> > + struct sp_iop *iop;
> > + struct device_node *memnp;
> > + struct resource mem_res;
> > +
> > + iop = devm_kzalloc(&pdev->dev, sizeof(struct sp_iop), GFP_KERNEL);
> > + if (!iop) {
> > + ret = -ENOMEM;
>
> Wrong spacing before '='.
>
> > + goto fail_kmalloc;
> > + }
> > + /* init */
>
> Remove useless comments.
>
> > + mutex_init(&iop->write_lock);
>
> Where are lock/unlock operations? What is this all code? This looks like
> half-baked solution...
>
> > + ret = sp_iop_get_resources(pdev, iop);
> > +
> > + // Get reserve address
>
> Keep consistent style. For comments /* is preferred. But definitely do not mix
> one with another.
>
> This code is difficult to read.
>
> > + memnp = of_parse_phandle(pdev->dev.of_node, "memory-region", 0);
> > + if (!memnp) {
> > + dev_err(&pdev->dev, "no memory-region node\n");
> > + return -EINVAL;
> > + }
> > +
> > + rc = of_address_to_resource(memnp, 0, &mem_res);
> > + of_node_put(memnp);
> > + if (rc) {
> > + dev_err(&pdev->dev, "failed to translate memory-region to a
> resource\n");
> > + return -EINVAL;
> > + }
> > +
> > + iop->dev = &pdev->dev;
> > + iop->iop_mem_start = mem_res.start;
> > + iop->iop_mem_size = resource_size(&mem_res);
> > +
> > + /*
> > + * 8051 has two bin files, normal.bin and standby.bin in rootfs.
> > + * Normally, 8051 executes normal.bin code. Normal.bin code size can
> exceed 16K.
> > + * When system linux kernel is shutdown, 8051 is to execute standby.bin
> code.
> > + * Standby.bin code cannot exceed 16K bytes. Because 8051 Icache size is
> 16k.
> > + * 8051 runs with standby.bin code in the Icache before the system(linux
> kernel)
> > + * is poweroff.
> > + */
> > + ret = sp_iop_get_normal_code(&pdev->dev, iop);
> > + if (ret != 0) {
> > + dev_err(&pdev->dev, "get normal code err=%d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = sp_iop_get_standby_code(&pdev->dev, iop);
> > + if (ret != 0) {
> > + dev_err(&pdev->dev, "get standby code err=%d\n", ret);
> > + return ret;
> > + }
> > +
> > + sp_iop_run_normal_code(iop);
> > + platform_set_drvdata(pdev, iop);
> > + iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node,
> "iop-wakeup", 0);
> > + iop_poweroff = iop;
> > + pm_power_off = sp_iop_poweroff;
>
> Blank line. Before every such return. Please, open some recent drivers and look
> at their code style.
>
> > + return 0;
> > +
> > +fail_kmalloc:
> > + return ret;
> > +}
> > +
> > +static int sp_iop_remove(struct platform_device *pdev) {
> > + pm_power_off = NULL;
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id sp_iop_of_match[] = {
> > + { .compatible = "sunplus,sp7021-iop" },
> > + { /* sentinel */ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, sp_iop_of_match);
> > +
> > +static struct platform_driver sp_iop_platform_driver = {
> > + .probe = sp_iop_platform_driver_probe,
> > + .remove = sp_iop_remove,
> > + .driver = {
> > + .name = "sunplus,sp7021-iop",
> > + .of_match_table = sp_iop_of_match,
> > + }
> > +};
> > +
> > +module_platform_driver(sp_iop_platform_driver);
> > +
> > +MODULE_AUTHOR("Tony Huang <tonyhuang.sunplus@...il.com>");
> > +MODULE_DESCRIPTION("Sunplus IOP Driver"); MODULE_LICENSE("GPL
> v2");
>
>
> Best regards,
> Krzysztof
Powered by blists - more mailing lists