misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS
Subsystems (2)
PATCH 2/2 misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS
Cc Arnd Bergmann <arnd <at> arndb.de>
Cc Greg Kroah-Hartman <greg <at> kroah.com>
Signed-off-by Kisang Lee <[email protected]>
--- drivers/misc/Kconfig | 1 + drivers/misc/Makefile | 1 + drivers/misc/c2c/Kconfig | 10 + drivers/misc/c2c/Makefile | 5 + drivers/misc/c2c/samsung-c2c.c | 500 ++++++++++++++++++++++++++++++++++++++++ drivers/misc/c2c/samsung-c2c.h | 300 ++++++++++++++++++++++++ 6 files changed, 817 insertions(+), 0 deletions(-) create mode 100644 drivers/misc/c2c/Kconfig create mode 100644 drivers/misc/c2c/Makefile create mode 100644 drivers/misc/c2c/samsung-c2c.c create mode 100644 drivers/misc/c2c/samsung-c2c.h
drivers/misc/Kconfig +1 -0
@@ -516,5 +516,6 @@ source "drivers/misc/ti-st/Kconfig" source "drivers/misc/lis3lv02d/Kconfig" source "drivers/misc/carma/Kconfig" source "drivers/misc/altera-stapl/Kconfig" +source "drivers/misc/c2c/Kconfig" endif # MISC_DEVICES
drivers/misc/Makefile +1 -0
@@ -49,3 +49,4 @@ obj-y += carma/ obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o obj-$(CONFIG_ALTERA_STAPL) +=altera-stapl/ obj-$(CONFIG_MAX8997_MUIC) += max8997-muic.o +obj-$(CONFIG_SAMSUNG_C2C) += c2c/
drivers/misc/c2c/Kconfig new file +10 -0
@@ -0,0 +1,10 @@ +# +# C2C(Chip to Chip) Device +# + +config SAMSUNG_C2C + tristate "C2C Support" + depends on SOC_EXYNOS4212 || SOC_EXYNOS5250 + default n + help + It is for supporting C2C driver.
drivers/misc/c2c/Makefile new file +5 -0
@@ -0,0 +1,5 @@ +# +# Makefile for C2C(Chip to Chip) device +# + +obj-$(CONFIG_SAMSUNG_C2C) += samsung-c2c.o
drivers/misc/c2c/samsung-c2c.c new file +500 -0
500 lines not on this page · raw message
drivers/misc/c2c/samsung-c2c.h new file +300 -0
300 lines not on this page · raw message
Signature · 1 line
1.7.1
PATCH 2/2 misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS
Hi,
Quoted 2 lines · Kisang Lee
Cc Arnd Bergmann<arnd<at> arndb.de>Cc Greg Kroah-Hartman<greg<at> kroah.com>
Commit description aren't expected to be empty, especially when you're adding a new driver. There is little clue what the driver is needed for. Could you please be a little bit more verbose here ?
Quoted 40 lines
Signed-off-by Kisang Lee<[email protected]>--- drivers/misc/Kconfig | 1 + drivers/misc/Makefile | 1 + drivers/misc/c2c/Kconfig | 10 + drivers/misc/c2c/Makefile | 5 + drivers/misc/c2c/samsung-c2c.c | 500 ++++++++++++++++++++++++++++++++++++++++ drivers/misc/c2c/samsung-c2c.h | 300 ++++++++++++++++++++++++ 6 files changed, 817 insertions(+), 0 deletions(-)create mode 100644
drivers/misc/c2c/Kconfigcreate mode 100644drivers/misc/c2c/Makefilecreate mode 100644drivers/misc/c2c/samsung-c2c.ccreate mode 100644drivers/misc/c2c/samsung-c2c.hdrivers/misc/Kconfig +1
-0@@ -516,5 +516,6 @@ source "drivers/misc/ti-st/Kconfig" source "drivers/misc/lis3lv02d/Kconfig" source "drivers/misc/carma/Kconfig" source "drivers/misc/altera-stapl/Kconfig" +source "drivers/misc/c2c/Kconfig" endif # MISC_DEVICESdrivers/misc/Makefile +1
-0@@ -49,3 +49,4 @@ obj-y += carma/ obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o obj-$(CONFIG_ALTERA_STAPL) +=altera-stapl/ obj-$(CONFIG_MAX8997_MUIC) += max8997-muic.o +obj-$(CONFIG_SAMSUNG_C2C) += c2c/drivers/misc/c2c/Kconfig new file +0
-0
drivers/misc/c2c/Kconfig line 8
Quoted 8 lines
drivers/misc/c2c/Kconfig new file +8
-0@@ -0,0 +1,10 @@ +# +# C2C(Chip to Chip) Device +# + +config SAMSUNG_C2C + tristate "C2C Support" + depends on SOC_EXYNOS4212 || SOC_EXYNOS5250 + default n
The default is 'n' so you can drop this line.
drivers/misc/c2c/Makefile line 2
Quoted 10 lines
drivers/misc/c2c/Kconfig new file +2
-0+ help + It is for supporting C2C driver.drivers/misc/c2c/Makefile new file +2
-0@@ -0,0 +1,5 @@ +# +# Makefile for C2C(Chip to Chip) device
^^^
White-space after C2C ?
drivers/misc/c2c/samsung-c2c.c line 4
Quoted 13 lines
drivers/misc/c2c/Makefile new file +3
-0+# + +obj-$(CONFIG_SAMSUNG_C2C) += samsung-c2c.odrivers/misc/c2c/samsung-c2c.c new file +4
-0@@ -0,0 +1,500 @@ +/* +* Samsung C2C driver +* +* Copyright (C) 2011 Samsung Electronics Co.Ltd
2012 or 2011 - 2012 ?
I think the proper form is:
Copyright (C) 2012 Samsung Electronics Co., Ltd.
Quoted 22 lines
drivers/misc/c2c/samsung-c2c.c new file +22
-0+* Author: Kisang Lee<[email protected]> +* +* This program is free software; you can redistribute it and/or modify it +* under the terms of the GNU General Public License as published by the +* Free Software Foundation; either version 2 of the License, or (at your +* option) any later version. +*/ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/input.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/miscdevice.h> +#include <linux/platform_device.h> +#include <linux/sched.h> +#include <linux/sysfs.h> +#include <asm/mach-types.h> + +#include <mach/c2c.h>
drivers/misc/c2c/samsung-c2c.c line 36
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+#include <mach/regs-c2c.h> +#include <plat/cpu.h> + +#include "samsung-c2c.h" + +void c2c_reset_ops (void ) +{ +u32 set_clk =0 ; + +if (c2c_con.opp_mode ==C2C_OPP100 )
Would it make sense to make an effort to get rid of the global c2c_con variable ? I mean when there is more than one instance of the C2C device in the system this driver won't work as is. However if there is always only one hardware instance then there is no issue.
I would strongly recommend not relying on such a global variable though.
Quoted 17 lines
drivers/misc/c2c/samsung-c2c.c new file +17
-0+ set_clk = c2c_con.clk_opp100; + else if (c2c_con.opp_mode == C2C_OPP50) + set_clk = c2c_con.clk_opp50; + else if (c2c_con.opp_mode == C2C_OPP25) + set_clk = c2c_con.clk_opp25; + +dev_info (c2c_con.c2c_dev,"c2c_reset_ops()\n" ); +clk_set_rate (c2c_con.c2c_sclk, (set_clk +1 ) *MHZ ); +c2c_set_func_clk (set_clk); + +/* C2C block reset */ +c2c_set_reset (C2C_CLEAR ); +c2c_set_reset (C2C_SET ); + +c2c_set_clock_gating (C2C_CLEAR ); +c2c_writel (c2c_con.retention_reg,EXYNOS_C2C_IRQ_EN_SET1 ); +c2c_writel (set_clk,EXYNOS_C2C_FCLK_FREQ );
drivers/misc/c2c/samsung-c2c.c line 63
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+c2c_writel (set_clk,EXYNOS_C2C_RX_MAX_FREQ ); +c2c_set_clock_gating (C2C_SET ); +} + +static ssize_t c2c_ctrl_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int ret =0 ; + ret =sprintf (buf,"C2C State" ); + ret += sprintf(&buf[ret], "SysReg : 0x%x\n",
nit: You could use "%#x" notation here and anywhere else for hex numbers, that's one character less... :)
Quoted 20 lines
drivers/misc/c2c/samsung-c2c.c new file +20
-0+ readl(c2c_con.c2c_sysreg)); + ret +=sprintf (&buf[ret],"Port Config : 0x%x\n" , +c2c_readl (EXYNOS_C2C_PORTCONFIG )); + ret +=sprintf (&buf[ret],"FCLK_FREQ : %d\n" , +c2c_readl (EXYNOS_C2C_FCLK_FREQ )); + ret +=sprintf (&buf[ret],"RX_MAX_FREQ : %d\n" , +c2c_readl (EXYNOS_C2C_RX_MAX_FREQ )); + ret +=sprintf (&buf[ret],"IRQ_EN_SET1 : 0x%x\n" , +c2c_readl (EXYNOS_C2C_IRQ_EN_SET1 )); + ret +=sprintf (&buf[ret],"Get C2C sclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_sclk)); + ret +=sprintf (&buf[ret],"Get C2C aclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_aclk)); + +return ret; +} + +static ssize_t c2c_ctrl_store (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{
drivers/misc/c2c/samsung-c2c.c line 93
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+int ops_num, opp_val, req_clk; +sscanf (buf,"%d" ,&ops_num); + +switch (ops_num) { +case 1 : +c2c_reset_ops (); +break ; +case 2 : +case 3 : +case 4 :
What about:
case 2 ... 4:
?
Quoted 29 lines
drivers/misc/c2c/samsung-c2c.c new file +29
-0+ opp_val = ops_num -1 ; + req_clk =0 ; +dev_info (c2c_con.c2c_dev,"Set current OPP mode (%d)\n" , + opp_val); + +if (opp_val ==C2C_OPP100 ) + req_clk = c2c_con.clk_opp100; +else if (opp_val ==C2C_OPP50 ) + req_clk = c2c_con.clk_opp50; +else if (opp_val ==C2C_OPP25 ) + req_clk = c2c_con.clk_opp25; + +if (opp_val ==0 || req_clk ==1 ) { +dev_info (c2c_con.c2c_dev, +"This mode is not reserved in OPP mode.\n" ); + }else { +c2c_set_clock_gating (C2C_CLEAR ); +if (c2c_con.opp_mode> opp_val) { +/* increase case */ +clk_set_rate (c2c_con.c2c_sclk, + (req_clk +1 ) *MHZ ); +c2c_writel (req_clk,EXYNOS_C2C_FCLK_FREQ ); +c2c_set_func_clk (req_clk); +c2c_writel (req_clk,EXYNOS_C2C_RX_MAX_FREQ ); + }else if (c2c_con.opp_mode< opp_val) { +/* decrease case */ +c2c_writel (req_clk,EXYNOS_C2C_RX_MAX_FREQ ); +clk_set_rate (c2c_con.c2c_sclk, + (req_clk +1 ) *MHZ );
drivers/misc/c2c/samsung-c2c.c line 132
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+c2c_writel (req_clk,EXYNOS_C2C_FCLK_FREQ ); +c2c_set_func_clk (req_clk); + }else { +dev_info (c2c_con.c2c_dev, +"Requested same OPP mode\n" ); + } + c2c_con.opp_mode = opp_val; +c2c_set_clock_gating (C2C_SET ); + } +
How about using a local variable pointing to the device at the beginning of this function, e.g.
struct device *dev = &c2c_con.c2c_dev;
Better might be to store &c2c_con as the driver's data, for example. c2c_con.c2c_dev is used quite often and this could make code more readable.
Quoted 16 lines
drivers/misc/c2c/samsung-c2c.c new file +16
-0+dev_info (c2c_con.c2c_dev,"Get C2C sclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_sclk)); +dev_info (c2c_con.c2c_dev,"Get C2C aclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_aclk)); +break ; +default : +dev_info (c2c_con.c2c_dev,"---C2C Operation Number---\n" ); +dev_info (c2c_con.c2c_dev,"1. C2C Reset\n" ); +dev_info (c2c_con.c2c_dev,"2. Set OPP25\n" ); +dev_info (c2c_con.c2c_dev,"3. Set OPP50\n" ); +dev_info (c2c_con.c2c_dev,"4. Set OPP100\n" ); + } + +return count; +} +
drivers/misc/c2c/samsung-c2c.c
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+static DEVICE_ATTR(c2c_ctrl, 0644, c2c_ctrl_show, c2c_ctrl_store); + +static int c2c_set_sharedmem (enum c2c_shrdmem_size size,u32 addr) +{ +dev_info (c2c_con.c2c_dev,"Set BaseAddr(0x%x) and Size(%d)\n" , + addr,1 << (2 + size)); + +/* Set DRAM Base Addr& Size */ +c2c_set_shdmem_size (size); +c2c_set_base_addr ((addr>>22 ));
Superfluous parentheses.
Quoted 26 lines
drivers/misc/c2c/samsung-c2c.c new file +26
-0+ +return 0 ; +} + +static void c2c_set_interrupt (u32 genio_num,enum c2c_interrupt set_int) +{ +u32 cur_int_reg, cur_lev_reg; + + cur_int_reg =c2c_readl (EXYNOS_C2C_GENO_INT ); + cur_lev_reg =c2c_readl (EXYNOS_C2C_GENO_LEVEL ); + +switch (set_int) { +case C2C_INT_TOGGLE : + cur_int_reg&= ~(0x1 << genio_num); +c2c_writel (cur_int_reg,EXYNOS_C2C_GENO_INT ); +break ; +case C2C_INT_HIGH : + cur_int_reg |= (0x1 << genio_num); + cur_lev_reg |= (0x1 << genio_num); +c2c_writel (cur_int_reg,EXYNOS_C2C_GENO_INT ); +c2c_writel (cur_lev_reg,EXYNOS_C2C_GENO_LEVEL ); +break ; +case C2C_INT_LOW : + cur_int_reg |= (0x1 << genio_num); + cur_lev_reg&= ~(0x1 << genio_num); +c2c_writel (cur_int_reg,EXYNOS_C2C_GENO_INT );
drivers/misc/c2c/samsung-c2c.c line 194
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+c2c_writel (cur_lev_reg,EXYNOS_C2C_GENO_LEVEL ); +break ; + } +} + +static irqreturn_t c2c_sscm0_irq (int irq,void *data) +{ +/* Nothing here yet */ +return IRQ_HANDLED ; +}
Should it be just removed for now ?
Quoted 74 lines
drivers/misc/c2c/samsung-c2c.c new file +74
-0+ +static irqreturn_t c2c_sscm1_irq (int irq,void *data) +{ +u32 raw_irq, opp_val, req_clk; + raw_irq =c2c_readl (EXYNOS_C2C_IRQ_EN_STAT1 ); + +if ((raw_irq>>C2C_GENIO_OPP_INT )&1 ) {/* OPP Change */ +/* +* OPP mode GENI/O bit definition[29:27] +* OPP100 GENI/O[29:28] : 1 1 +* OPP50 GENI/O[29:28] : 1 0 +* OPP25 GENI/O[29:28] : 0 1 +* GENI[27] is only used for making interrupt. +*/ + opp_val = (c2c_readl (EXYNOS_C2C_GENO_STATUS )>>28 )&3 ; + req_clk =0 ; +dev_info (c2c_con.c2c_dev,"OPP interrupt occured (%d)\n" , + opp_val); + +if (opp_val ==C2C_OPP100 ) + req_clk = c2c_con.clk_opp100; +else if (opp_val ==C2C_OPP50 ) + req_clk = c2c_con.clk_opp50; +else if (opp_val ==C2C_OPP25 ) + req_clk = c2c_con.clk_opp25; + +if (opp_val ==0 || req_clk ==1 ) { +dev_info (c2c_con.c2c_dev, +"This mode is not reserved in OPP mode.\n" ); + }else { +if (c2c_con.opp_mode> opp_val) { +/* increase case */ +clk_set_rate (c2c_con.c2c_sclk, + (req_clk +1 ) *MHZ ); +c2c_writel (req_clk,EXYNOS_C2C_FCLK_FREQ ); +c2c_set_func_clk (req_clk); +c2c_writel (req_clk,EXYNOS_C2C_RX_MAX_FREQ ); + }else if (c2c_con.opp_mode< opp_val) { +/* decrease case */ +c2c_writel (req_clk,EXYNOS_C2C_RX_MAX_FREQ ); +clk_set_rate (c2c_con.c2c_sclk, + (req_clk +1 ) *MHZ ); +c2c_writel (req_clk,EXYNOS_C2C_FCLK_FREQ ); +c2c_set_func_clk (req_clk); + }else { +dev_info (c2c_con.c2c_dev,"Requested same OPP mode\n" ); + } + c2c_con.opp_mode = opp_val; + } + +/* Interrupt Clear */ +c2c_writel ((0x1 <<C2C_GENIO_OPP_INT ),EXYNOS_C2C_IRQ_EN_STAT1 ); + } + +return IRQ_HANDLED ; +} + +static void set_c2c_device (struct platform_device *pdev) +{ +struct exynos_c2c_platdata *pdata = pdev->dev.platform_data; +u32 default_clk; + + c2c_con.c2c_sysreg = pdata->c2c_sysreg; + c2c_con.rx_width = pdata->rx_width; + c2c_con.tx_width = pdata->tx_width; + c2c_con.clk_opp100 = pdata->clk_opp100; + c2c_con.clk_opp50 = pdata->clk_opp50; + c2c_con.clk_opp25 = pdata->clk_opp25; + c2c_con.opp_mode = pdata->default_opp_mode; + c2c_con.c2c_sclk =clk_get (&pdev->dev,"sclk_c2c" ); + c2c_con.c2c_aclk =clk_get (&pdev->dev,"aclk_c2c" ); + +/* Set clock to default mode */ +if (c2c_con.opp_mode ==C2C_OPP100 )
drivers/misc/c2c/samsung-c2c.c line 278
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+ default_clk = c2c_con.clk_opp100; + else if (c2c_con.opp_mode == C2C_OPP50) + default_clk = c2c_con.clk_opp50; + else if (c2c_con.opp_mode == C2C_OPP25) + default_clk = c2c_con.clk_opp25; + else { +dev_info (c2c_con.c2c_dev,"Default OPP mode is not selected.\n" ); + c2c_con.opp_mode =C2C_OPP50 ; + default_clk = c2c_con.clk_opp50; + }
switch statement ? e.g
switch (c2c_con.opp_mode) {
case C2C_OPP100:
default_clk = c2c_con.clk_opp100;
break;
case C2C_OPP25:
default_clk = c2c_con.clk_opp25;
break;
case C2C_OPP50:
default_clk = c2c_con.clk_opp50;
/* fall through */
default:
dev_info(c2c_con.c2c_dev, "Fallback to OPP50 mode\n");
c2c_con.opp_mode = C2C_OPP50;
}?
drivers/misc/c2c/samsung-c2c.c line 289
Quoted 11 lines
drivers/misc/c2c/samsung-c2c.c new file +11
-0+ +clk_set_rate (c2c_con.c2c_sclk, (default_clk +1 ) *MHZ ); +clk_set_rate (c2c_con.c2c_aclk, ((default_clk /2 ) +1 ) *MHZ ); + +dev_info (c2c_con.c2c_dev,"Get C2C sclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_sclk)); +dev_info (c2c_con.c2c_dev,"Get C2C aclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_aclk)); + +if (pdata->setup_gpio) + pdata->setup_gpio (pdata->rx_width, pdata->tx_width);
We should be avoiding callbacks in the platform data. These cause trouble for device tree based platforms.
Quoted 31 lines
drivers/misc/c2c/samsung-c2c.c new file +31
-0+ +c2c_set_sharedmem (pdata->shdmem_size, pdata->shdmem_addr); + +/* Set SYSREG to memdone */ +c2c_set_memdone (C2C_SET ); +c2c_set_clock_gating (C2C_CLEAR ); + +/* Set C2C clock register */ +c2c_writel (default_clk,EXYNOS_C2C_FCLK_FREQ ); +c2c_writel (default_clk,EXYNOS_C2C_RX_MAX_FREQ ); +c2c_set_func_clk (default_clk); + +/* Set C2C buswidth */ +c2c_writel (((pdata->rx_width<<4 ) | (pdata->tx_width)), +EXYNOS_C2C_PORTCONFIG ); +c2c_set_tx_buswidth (pdata->tx_width); +c2c_set_rx_buswidth (pdata->rx_width); + +/* Enable defined GENI/O Interrupt */ +c2c_writel ((0x1 <<C2C_GENIO_OPP_INT ),EXYNOS_C2C_IRQ_EN_SET1 ); + c2c_con.retention_reg = (0x1 <<C2C_GENIO_OPP_INT ); + +c2c_set_interrupt (C2C_GENIO_OPP_INT ,C2C_INT_HIGH ); + +dev_info (c2c_con.c2c_dev,"Port Config : 0x%x\n" , +c2c_readl (EXYNOS_C2C_PORTCONFIG )); +dev_info (c2c_con.c2c_dev,"FCLK_FREQ register : %d\n" , +c2c_readl (EXYNOS_C2C_FCLK_FREQ )); +dev_info (c2c_con.c2c_dev,"RX_MAX_FREQ register : %d\n" , +c2c_readl (EXYNOS_C2C_RX_MAX_FREQ )); + dev_info(c2c_con.c2c_dev, "IRQ_EN_SET1 register : 0x%x\n",
drivers/misc/c2c/samsung-c2c.c line 330
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+ c2c_readl(EXYNOS_C2C_IRQ_EN_SET1)); + +c2c_set_clock_gating (C2C_SET ); +} + +static int __devinit samsung_c2c_probe (struct platform_device *pdev) +{ +struct exynos_c2c_platdata *pdata = pdev->dev.platform_data; +struct resource *res =NULL ; +struct resource *res1 =NULL ;
Unneeded initialization.
Quoted 7 lines
drivers/misc/c2c/samsung-c2c.c new file +7
-0+int sscm_irq0, sscm_irq1; +int err =0 ; + + c2c_con.c2c_dev =&pdev->dev; + +/* resource for AP's SSCM region */ + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 );
drivers/misc/c2c/samsung-c2c.c line 347
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+if (!res) { +dev_err (&pdev->dev,"no memory resource defined(AP's SSCM)\n" ); +return -ENOENT ; + } + res =request_mem_region (res->start,resource_size (res), pdev->name); +if (!res) { +dev_err (&pdev->dev,"failded to request memory resource(AP)\n" ); +return -ENOENT ; + } + pdata->ap_sscm_addr =ioremap (res->start,resource_size (res));
What ? You're overwriting the platform data ? Please don't do that. I wonder why do you need the IO memory region address at the platform data in the first place. Instead what's needed should be copied to driver's internal data structures so platform_data can be marked with __init and discarded after the system initialization.
drivers/misc/c2c/samsung-c2c.c line 351
Quoted 4 lines
drivers/misc/c2c/samsung-c2c.c new file +4
-0+if (!pdata->ap_sscm_addr) { +dev_err (&pdev->dev,"failded to request memory resource(AP)\n" ); + goto release_ap_sscm; + }
How about using the device managed resources, i.e. devm_request_and_ioremap() in this case and...
Quoted 17 lines
drivers/misc/c2c/samsung-c2c.c new file +17
-0+ c2c_con.ap_sscm_addr = pdata->ap_sscm_addr; + +/* resource for CP's SSCM region */ + res1 =platform_get_resource (pdev,IORESOURCE_MEM ,1 ); +if (!res1) { +dev_err (&pdev->dev,"no memory resource defined(AP's SSCM)\n" ); + goto unmap_ap_sscm; + } + res1 =request_mem_region (res1->start,resource_size (res1), pdev->name); +if (!res1) { +dev_err (&pdev->dev,"failded to request memory resource(AP)\n" ); + goto unmap_ap_sscm; + } + pdata->cp_sscm_addr =ioremap (res1->start,resource_size (res1)); +if (!pdata->cp_sscm_addr) { +dev_err (&pdev->dev,"failded to request memory resource(CP)\n" ); + goto release_cp_sscm;
drivers/misc/c2c/samsung-c2c.c line 378
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+ } + c2c_con.cp_sscm_addr = pdata->cp_sscm_addr; + +/* Request IRQ for SSCM0 */ + sscm_irq0 =platform_get_irq (pdev,0 ); +if (sscm_irq0<0 ) { +dev_err (&pdev->dev,"no irq specified\n" ); + goto unmap_cp_sscm; + } + err =request_irq (sscm_irq0, c2c_sscm0_irq,0 , pdev->name, pdev);
... devm_request_irq() ?
Your error handling would then simplify significantly.
drivers/misc/c2c/samsung-c2c.c line 384
Quoted 6 lines
drivers/misc/c2c/samsung-c2c.c new file +6
-0+if (err) { +dev_err (&pdev->dev,"Can't request SSCM0 IRQ\n" ); + goto unmap_cp_sscm; + } +/* SSCM0 irq will be only used for master device */ +disable_irq (sscm_irq0);
I personally would just not request the irq for now, it's unused.
Quoted 59 lines
drivers/misc/c2c/samsung-c2c.c new file +59
-0+ +/* Request IRQ for SSCM1 */ + sscm_irq1 =platform_get_irq (pdev,1 ); +if (sscm_irq1<0 ) { +dev_err (&pdev->dev,"no irq specified\n" ); + goto release_sscm_irq0; + } + err =request_irq (sscm_irq1, c2c_sscm1_irq,1 , pdev->name, pdev); +if (err) { +dev_err (&pdev->dev,"Can't request SSCM1 IRQ\n" ); + goto release_sscm_irq0; + } + +if (err) { +dev_err (&pdev->dev,"Can't register chrdev!\n" ); + goto release_sscm_irq0; + } + +set_c2c_device (pdev); + +/* Create sysfs file for C2C debug */ + err =device_create_file (&pdev->dev,&dev_attr_c2c_ctrl); +if (err) { +dev_err (&pdev->dev,"Failed to create sysfs for C2C\n" ); + goto release_sscm_irq1; + } + +return 0 ; + +release_sscm_irq1: +free_irq (sscm_irq1, pdev); + +release_sscm_irq0: +free_irq (sscm_irq0, pdev); + +unmap_cp_sscm: +iounmap (pdata->cp_sscm_addr); + +release_cp_sscm: +release_mem_region (res1->start,resource_size (res1)); + +unmap_ap_sscm: +iounmap (pdata->ap_sscm_addr); + +release_ap_sscm: +release_mem_region (res->start,resource_size (res)); + +return err; +} + +static int __devexit samsung_c2c_remove(struct platform_device *pdev) +{ +struct exynos_c2c_platdata *pdata = pdev->dev.platform_data; +struct resource *res =NULL ; +struct resource *res1 =NULL ; +int sscm_irq0, sscm_irq1; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + res1 =platform_get_resource (pdev,IORESOURCE_MEM ,1 );
drivers/misc/c2c/samsung-c2c.c line 453
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+ sscm_irq0 =platform_get_irq (pdev,0 ); + sscm_irq1 =platform_get_irq (pdev,1 ); + +iounmap (pdata->ap_sscm_addr); +iounmap (pdata->cp_sscm_addr); +release_mem_region (res->start,resource_size (res)); +release_mem_region (res1->start,resource_size (res1)); + +free_irq (sscm_irq0, pdev); +free_irq (sscm_irq1, pdev);
If you have used the devm_* API for requesting resources everything in this function could go away.
Quoted 9 lines
drivers/misc/c2c/samsung-c2c.c new file +9
-0+ +return 0 ; +} + +#ifdef CONFIG_PM +static int samsung_c2c_suspend (struct platform_device *dev,pm_message_t pm) +{ +/* Nothing here yet */ +return 0 ;
drivers/misc/c2c/samsung-c2c.c line 472
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+} + +static int samsung_c2c_resume (struct platform_device *dev) +{ +/* Nothing here yet */ +return 0 ; +} +#else +#define samsung_c2c_suspend NULL +#define samsung_c2c_resume NULL
This #else statement is not needed when you use SET_SYSTEM_SLEEP_PM_OPS() macro for assigning the PM helpers below.
drivers/misc/c2c/samsung-c2c.c line 479
Quoted 7 lines
drivers/misc/c2c/samsung-c2c.c new file +7
-0+#endif + +static struct platform_driver samsung_c2c_driver = { + .probe = samsung_c2c_probe, + .remove =__devexit_p (samsung_c2c_remove), + .suspend = samsung_c2c_suspend, + .resume = samsung_c2c_resume,
Please don't use legacy PM callbacks, struct dev_pm_ops should be used instead, e.g.
static const struct dev_pm_ops exynos_c2c_pm_ops = {
SET_SYSTEM_SLLEP_PM_OPS(exynos_c2c_suspend, exynos_c2c_resume,
NULL)
};
drivers/misc/c2c/samsung-c2c.c line 480
Quoted 1 line
drivers/misc/c2c/samsung-c2c.c new file +1
-0+ .driver = {
.pm = &exynos_c2c_pm_ops,
drivers/misc/c2c/samsung-c2c.c line 485
Quoted 5 lines
drivers/misc/c2c/samsung-c2c.c new file +5
-0+ .name = "samsung-c2c", + .owner = THIS_MODULE, + }, +}; +
Nevertheless you PM callbacks are empty, how about dropping them for now and adding in a subsequent patch that implements proper power management ?
drivers/misc/c2c/samsung-c2c.c line 496
Quoted 11 lines
drivers/misc/c2c/samsung-c2c.c new file +11
-0+static int __init samsung_c2c_init (void ) +{ +return platform_driver_register (&samsung_c2c_driver); +} +module_init (samsung_c2c_init); + +static void __exit samsung_c2c_exit(void) +{ +platform_driver_unregister (&samsung_c2c_driver); +} +module_exit (samsung_c2c_exit);
You could use module_platform_driver macro to simplify that, e.g.
module_platform_driver(&samsung_c2c_driver);
BTW, isn't it better to just use "exynos_" prefix rather than "samsung_" ? Just in case there is other C2C driver for different SoC family ?
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +4
-0+ +MODULE_DESCRIPTION ("Samsung C2C driver" ); +MODULE_AUTHOR ("Kisang Lee<[email protected]>" ); +MODULE_LICENSE ("GPL" );drivers/misc/c2c/samsung-c2c.h new file +0
-0
drivers/misc/c2c/samsung-c2c.h line 4
Quoted 4 lines
drivers/misc/c2c/samsung-c2c.h new file +4
-0@@ -0,0 +1,300 @@ +/* +* Samsung C2C driver +* +* Copyright (C) 2011 Samsung Electronics Co.Ltd
Copyright (C) 2012 Samsung Electronics Co., Ltd.
Quoted 30 lines
drivers/misc/c2c/samsung-c2c.h new file +30
-0+* Author: Kisang Lee<[email protected]> +* +* This program is free software; you can redistribute it and/or modify it +* under the terms of the GNU General Public License as published by the +* Free Software Foundation; either version 2 of the License, or (at your +* option) any later version. +*/ +#ifndef SAMSUNG_C2C_H +#define SAMSUNG_C2C_H + +enum c2c_set_clear { + C2C_CLEAR = 0, + C2C_SET = 1, +}; + +enum c2c_interrupt { + C2C_INT_TOGGLE = 0, + C2C_INT_HIGH = 1, + C2C_INT_LOW = 2, +}; + +struct c2c_state_control { + void __iomem *ap_sscm_addr; + void __iomem *cp_sscm_addr; + struct device *c2c_dev; + + u32 rx_width; + u32 tx_width; + + u32 clk_opp100;
drivers/misc/c2c/samsung-c2c.h line 44
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.h new file +10
-0+u32 clk_opp50; +u32 clk_opp25; + +struct clk *c2c_sclk; +struct clk *c2c_aclk; + +enum c2c_opp_mode opp_mode; + +u32 retention_reg; +void __iomem *c2c_sysreg;
nit: What about not prefixing the individual struct members with "c2c",
since they already belong to struct "c2c_state_control" ?
drivers/misc/c2c/samsung-c2c.h line 52
Quoted 8 lines
drivers/misc/c2c/samsung-c2c.h new file +8
-0+}; + +static struct c2c_state_control c2c_con; + +static inline void c2c_writel (u32 val,int reg) +{ +writel (val, c2c_con.ap_sscm_addr + reg); +}
As I already pointed out, might be good to get rid of the global c2c_con variable and be passing a relevant pointer to these helpers.
--
Thanks,
Sylwester
PATCH 2/2 misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS
In reply to Kisang LeeQuoted 4 lines · Kisang Lee
Cc Arnd Bergmann <arnd <at> arndb.de>Cc Greg Kroah-Hartman <greg <at> kroah.com>Signed-off-by Kisang Lee <[email protected]>
What follows is a quick review of this driver. I think there's quite a number of issues which need to be fixed before this driver is ready, some of them are rather serious. Please take a look and try to address as many of these comments as possible.
Thanks.
Quoted 72 lines
--- drivers/misc/Kconfig | 1 + drivers/misc/Makefile | 1 + drivers/misc/c2c/Kconfig | 10 + drivers/misc/c2c/Makefile | 5 + drivers/misc/c2c/samsung-c2c.c | 500 ++++++++++++++++++++++++++++++++++++++++ drivers/misc/c2c/samsung-c2c.h | 300 ++++++++++++++++++++++++ 6 files changed, 817 insertions(+), 0 deletions(-) create mode 100644 drivers/misc/c2c/Kconfig create mode 100644 drivers/misc/c2c/Makefile create mode 100644 drivers/misc/c2c/samsung-c2c.c create mode 100644 drivers/misc/c2c/samsung-c2c.hdrivers/misc/Kconfig +1
-0@@ -516,5 +516,6 @@ source "drivers/misc/ti-st/Kconfig" source "drivers/misc/lis3lv02d/Kconfig" source "drivers/misc/carma/Kconfig" source "drivers/misc/altera-stapl/Kconfig" +source "drivers/misc/c2c/Kconfig" endif # MISC_DEVICESdrivers/misc/Makefile +1
-0@@ -49,3 +49,4 @@ obj-y += carma/ obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o obj-$(CONFIG_ALTERA_STAPL) +=altera-stapl/ obj-$(CONFIG_MAX8997_MUIC) += max8997-muic.o +obj-$(CONFIG_SAMSUNG_C2C) += c2c/drivers/misc/c2c/Kconfig new file +10
-0@@ -0,0 +1,10 @@ +# +# C2C(Chip to Chip) Device +# + +config SAMSUNG_C2C + tristate "C2C Support" + depends on SOC_EXYNOS4212 || SOC_EXYNOS5250 + default n + help + It is for supporting C2C driver.drivers/misc/c2c/Makefile new file +5
-0@@ -0,0 +1,5 @@ +# +# Makefile for C2C(Chip to Chip) device +# + +obj-$(CONFIG_SAMSUNG_C2C) += samsung-c2c.odrivers/misc/c2c/samsung-c2c.c new file +6
-0@@ -0,0 +1,500 @@ +/* +* Samsung C2C driver +* +* Copyright (C) 2011 Samsung Electronics Co.Ltd +* Author: Kisang Lee <[email protected]> +*
drivers/misc/c2c/samsung-c2c.c line 16
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+* This program is free software; you can redistribute it and/or modify it +* under the terms of the GNU General Public License as published by the +* Free Software Foundation; either version 2 of the License, or (at your +* option) any later version. +*/ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/input.h>
I can find nothing related to the input layer in this file - is this header required?
drivers/misc/c2c/samsung-c2c.c line 20
Quoted 4 lines
drivers/misc/c2c/samsung-c2c.c new file +4
-0+#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/miscdevice.h>
I can find nothing which declares a misc_device in this file - is this header required?
drivers/misc/c2c/samsung-c2c.c line 24
Quoted 4 lines
drivers/misc/c2c/samsung-c2c.c new file +4
-0+#include <linux/platform_device.h> +#include <linux/sched.h> +#include <linux/sysfs.h> +#include <asm/mach-types.h>
You appear to use nothing from this header, please remove it. Drivers should not depend on the type of platform they're running on.
drivers/misc/c2c/samsung-c2c.c line 28
Quoted 4 lines
drivers/misc/c2c/samsung-c2c.c new file +4
-0+ +#include <mach/c2c.h> +#include <mach/regs-c2c.h> +#include <plat/cpu.h>
Is plat/cpu.h necessary?
Quoted 47 lines
drivers/misc/c2c/samsung-c2c.c new file +47
-0+ +#include "samsung-c2c.h" + +void c2c_reset_ops (void ) +{ +u32 set_clk =0 ; + +if (c2c_con.opp_mode ==C2C_OPP100 ) + set_clk = c2c_con.clk_opp100; +else if (c2c_con.opp_mode ==C2C_OPP50 ) + set_clk = c2c_con.clk_opp50; +else if (c2c_con.opp_mode ==C2C_OPP25 ) + set_clk = c2c_con.clk_opp25; + +dev_info (c2c_con.c2c_dev,"c2c_reset_ops()\n" ); +clk_set_rate (c2c_con.c2c_sclk, (set_clk +1 ) *MHZ ); +c2c_set_func_clk (set_clk); + +/* C2C block reset */ +c2c_set_reset (C2C_CLEAR ); +c2c_set_reset (C2C_SET ); + +c2c_set_clock_gating (C2C_CLEAR ); +c2c_writel (c2c_con.retention_reg,EXYNOS_C2C_IRQ_EN_SET1 ); +c2c_writel (set_clk,EXYNOS_C2C_FCLK_FREQ ); +c2c_writel (set_clk,EXYNOS_C2C_RX_MAX_FREQ ); +c2c_set_clock_gating (C2C_SET ); +} + +static ssize_t c2c_ctrl_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int ret =0 ; + ret =sprintf (buf,"C2C State" ); + ret +=sprintf (&buf[ret],"SysReg : 0x%x\n" , +readl (c2c_con.c2c_sysreg)); + ret +=sprintf (&buf[ret],"Port Config : 0x%x\n" , +c2c_readl (EXYNOS_C2C_PORTCONFIG )); + ret +=sprintf (&buf[ret],"FCLK_FREQ : %d\n" , +c2c_readl (EXYNOS_C2C_FCLK_FREQ )); + ret +=sprintf (&buf[ret],"RX_MAX_FREQ : %d\n" , +c2c_readl (EXYNOS_C2C_RX_MAX_FREQ )); + ret +=sprintf (&buf[ret],"IRQ_EN_SET1 : 0x%x\n" , +c2c_readl (EXYNOS_C2C_IRQ_EN_SET1 )); + ret +=sprintf (&buf[ret],"Get C2C sclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_sclk)); + ret += sprintf(&buf[ret], "Get C2C aclk rate : %ld\n",
drivers/misc/c2c/samsung-c2c.c line 85
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+ clk_get_rate(c2c_con.c2c_aclk)); + +return ret; +} + +static ssize_t c2c_ctrl_store (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +int ops_num, opp_val, req_clk; +sscanf (buf,"%d" , &ops_num);
What if sscanf fails?
Quoted 69 lines
drivers/misc/c2c/samsung-c2c.c new file +69
-0+ +switch (ops_num) { +case 1 : +c2c_reset_ops (); +break ; +case 2 : +case 3 : +case 4 : + opp_val = ops_num -1 ; + req_clk =0 ; +dev_info (c2c_con.c2c_dev,"Set current OPP mode (%d)\n" , + opp_val); + +if (opp_val ==C2C_OPP100 ) + req_clk = c2c_con.clk_opp100; +else if (opp_val ==C2C_OPP50 ) + req_clk = c2c_con.clk_opp50; +else if (opp_val ==C2C_OPP25 ) + req_clk = c2c_con.clk_opp25; + +if (opp_val ==0 || req_clk ==1 ) { +dev_info (c2c_con.c2c_dev, +"This mode is not reserved in OPP mode.\n" ); + }else { +c2c_set_clock_gating (C2C_CLEAR ); +if (c2c_con.opp_mode > opp_val) { +/* increase case */ +clk_set_rate (c2c_con.c2c_sclk, + (req_clk +1 ) *MHZ ); +c2c_writel (req_clk,EXYNOS_C2C_FCLK_FREQ ); +c2c_set_func_clk (req_clk); +c2c_writel (req_clk,EXYNOS_C2C_RX_MAX_FREQ ); + }else if (c2c_con.opp_mode < opp_val) { +/* decrease case */ +c2c_writel (req_clk,EXYNOS_C2C_RX_MAX_FREQ ); +clk_set_rate (c2c_con.c2c_sclk, + (req_clk +1 ) *MHZ ); +c2c_writel (req_clk,EXYNOS_C2C_FCLK_FREQ ); +c2c_set_func_clk (req_clk); + }else { +dev_info (c2c_con.c2c_dev, +"Requested same OPP mode\n" ); + } + c2c_con.opp_mode = opp_val; +c2c_set_clock_gating (C2C_SET ); + } + +dev_info (c2c_con.c2c_dev,"Get C2C sclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_sclk)); +dev_info (c2c_con.c2c_dev,"Get C2C aclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_aclk)); +break ; +default : +dev_info (c2c_con.c2c_dev,"---C2C Operation Number---\n" ); +dev_info (c2c_con.c2c_dev,"1. C2C Reset\n" ); +dev_info (c2c_con.c2c_dev,"2. Set OPP25\n" ); +dev_info (c2c_con.c2c_dev,"3. Set OPP50\n" ); +dev_info (c2c_con.c2c_dev,"4. Set OPP100\n" ); + } + +return count; +} + +static DEVICE_ATTR(c2c_ctrl, 0644, c2c_ctrl_show, c2c_ctrl_store); + +static int c2c_set_sharedmem (enum c2c_shrdmem_size size,u32 addr) +{ +dev_info (c2c_con.c2c_dev,"Set BaseAddr(0x%x) and Size(%d)\n" , + addr,1 << (2 + size));
drivers/misc/c2c/samsung-c2c.c line 164
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+ +/* Set DRAM Base Addr & Size */ +c2c_set_shdmem_size (size); +c2c_set_base_addr ((addr >>22 )); + +return 0 ; +} + +static void c2c_set_interrupt (u32 genio_num,enum c2c_interrupt set_int) +{
Is this function ever called with anything other than C2C_INT_HIGH ?
Quoted 20 lines
drivers/misc/c2c/samsung-c2c.c new file +20
-0+u32 cur_int_reg, cur_lev_reg; + + cur_int_reg =c2c_readl (EXYNOS_C2C_GENO_INT ); + cur_lev_reg =c2c_readl (EXYNOS_C2C_GENO_LEVEL ); + +switch (set_int) { +case C2C_INT_TOGGLE : + cur_int_reg &= ~(0x1 << genio_num); +c2c_writel (cur_int_reg,EXYNOS_C2C_GENO_INT ); +break ; +case C2C_INT_HIGH : + cur_int_reg |= (0x1 << genio_num); + cur_lev_reg |= (0x1 << genio_num); +c2c_writel (cur_int_reg,EXYNOS_C2C_GENO_INT ); +c2c_writel (cur_lev_reg,EXYNOS_C2C_GENO_LEVEL ); +break ; +case C2C_INT_LOW : + cur_int_reg |= (0x1 << genio_num); + cur_lev_reg &= ~(0x1 << genio_num); +c2c_writel (cur_int_reg,EXYNOS_C2C_GENO_INT );
drivers/misc/c2c/samsung-c2c.c line 194
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+c2c_writel (cur_lev_reg,EXYNOS_C2C_GENO_LEVEL ); +break ; + } +} + +static irqreturn_t c2c_sscm0_irq (int irq,void *data) +{ +/* Nothing here yet */ +return IRQ_HANDLED ; +}
So is there any reason to even request this interrupt, potentially causing an interrupt storm if it's triggered? By returning IRQ_HANDLED you're telling the IRQ core that you serviced and cleared it, so if it gets triggered, you're going to get stuck here.
drivers/misc/c2c/samsung-c2c.c line 197
Quoted 3 lines
drivers/misc/c2c/samsung-c2c.c new file +3
-0+ +static irqreturn_t c2c_sscm1_irq (int irq,void *data) +{
If you passed &c2c_con into request_irq(), fixed c2c_readl etc to take a c2c_con pointer, then you don't need to keep dereferencing a global symbol. Note that the structure does nothing to optimize the generated code - the compiler _won't_ keep the base address of it in a register and use offsets. It'll just load the individual addresses to structure members.
If you want efficient code, then you have to use a pointer to the structure.
Quoted 58 lines
drivers/misc/c2c/samsung-c2c.c new file +58
-0+u32 raw_irq, opp_val, req_clk; + raw_irq =c2c_readl (EXYNOS_C2C_IRQ_EN_STAT1 ); + +if ((raw_irq >>C2C_GENIO_OPP_INT ) &1 ) {/* OPP Change */ +/* +* OPP mode GENI/O bit definition[29:27] +* OPP100 GENI/O[29:28] : 1 1 +* OPP50 GENI/O[29:28] : 1 0 +* OPP25 GENI/O[29:28] : 0 1 +* GENI[27] is only used for making interrupt. +*/ + opp_val = (c2c_readl (EXYNOS_C2C_GENO_STATUS ) >>28 ) &3 ; + req_clk =0 ; +dev_info (c2c_con.c2c_dev,"OPP interrupt occured (%d)\n" , + opp_val); + +if (opp_val ==C2C_OPP100 ) + req_clk = c2c_con.clk_opp100; +else if (opp_val ==C2C_OPP50 ) + req_clk = c2c_con.clk_opp50; +else if (opp_val ==C2C_OPP25 ) + req_clk = c2c_con.clk_opp25; + +if (opp_val ==0 || req_clk ==1 ) { +dev_info (c2c_con.c2c_dev, +"This mode is not reserved in OPP mode.\n" ); + }else { +if (c2c_con.opp_mode > opp_val) { +/* increase case */ +clk_set_rate (c2c_con.c2c_sclk, + (req_clk +1 ) *MHZ ); +c2c_writel (req_clk,EXYNOS_C2C_FCLK_FREQ ); +c2c_set_func_clk (req_clk); +c2c_writel (req_clk,EXYNOS_C2C_RX_MAX_FREQ ); + }else if (c2c_con.opp_mode < opp_val) { +/* decrease case */ +c2c_writel (req_clk,EXYNOS_C2C_RX_MAX_FREQ ); +clk_set_rate (c2c_con.c2c_sclk, + (req_clk +1 ) *MHZ ); +c2c_writel (req_clk,EXYNOS_C2C_FCLK_FREQ ); +c2c_set_func_clk (req_clk); + }else { +dev_info (c2c_con.c2c_dev,"Requested same OPP mode\n" ); + } + c2c_con.opp_mode = opp_val; + } + +/* Interrupt Clear */ +c2c_writel ((0x1 <<C2C_GENIO_OPP_INT ),EXYNOS_C2C_IRQ_EN_STAT1 ); + } + +return IRQ_HANDLED ; +} + +static void set_c2c_device (struct platform_device *pdev) +{ +struct exynos_c2c_platdata *pdata = pdev->dev.platform_data; +u32 default_clk;
drivers/misc/c2c/samsung-c2c.c line 265
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+ + c2c_con.c2c_sysreg = pdata->c2c_sysreg; + c2c_con.rx_width = pdata->rx_width; + c2c_con.tx_width = pdata->tx_width; + c2c_con.clk_opp100 = pdata->clk_opp100; + c2c_con.clk_opp50 = pdata->clk_opp50; + c2c_con.clk_opp25 = pdata->clk_opp25; + c2c_con.opp_mode = pdata->default_opp_mode; + c2c_con.c2c_sclk =clk_get (&pdev->dev,"sclk_c2c" ); + c2c_con.c2c_aclk =clk_get (&pdev->dev,"aclk_c2c" );
Where's the error checking? Nowhere in this file do you prepare and enable the clock, so I assume that these can be deleted because they don't ever actually supply any clock to the device at all.
Quoted 67 lines
drivers/misc/c2c/samsung-c2c.c new file +67
-0+ +/* Set clock to default mode */ +if (c2c_con.opp_mode ==C2C_OPP100 ) + default_clk = c2c_con.clk_opp100; +else if (c2c_con.opp_mode ==C2C_OPP50 ) + default_clk = c2c_con.clk_opp50; +else if (c2c_con.opp_mode ==C2C_OPP25 ) + default_clk = c2c_con.clk_opp25; +else { +dev_info (c2c_con.c2c_dev,"Default OPP mode is not selected.\n" ); + c2c_con.opp_mode =C2C_OPP50 ; + default_clk = c2c_con.clk_opp50; + } + +clk_set_rate (c2c_con.c2c_sclk, (default_clk +1 ) *MHZ ); +clk_set_rate (c2c_con.c2c_aclk, ((default_clk /2 ) +1 ) *MHZ ); + +dev_info (c2c_con.c2c_dev,"Get C2C sclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_sclk)); +dev_info (c2c_con.c2c_dev,"Get C2C aclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_aclk)); + +if (pdata->setup_gpio) + pdata->setup_gpio (pdata->rx_width, pdata->tx_width); + +c2c_set_sharedmem (pdata->shdmem_size, pdata->shdmem_addr); + +/* Set SYSREG to memdone */ +c2c_set_memdone (C2C_SET ); +c2c_set_clock_gating (C2C_CLEAR ); + +/* Set C2C clock register */ +c2c_writel (default_clk,EXYNOS_C2C_FCLK_FREQ ); +c2c_writel (default_clk,EXYNOS_C2C_RX_MAX_FREQ ); +c2c_set_func_clk (default_clk); + +/* Set C2C buswidth */ +c2c_writel (((pdata->rx_width <<4 ) | (pdata->tx_width)), +EXYNOS_C2C_PORTCONFIG ); +c2c_set_tx_buswidth (pdata->tx_width); +c2c_set_rx_buswidth (pdata->rx_width); + +/* Enable defined GENI/O Interrupt */ +c2c_writel ((0x1 <<C2C_GENIO_OPP_INT ),EXYNOS_C2C_IRQ_EN_SET1 ); + c2c_con.retention_reg = (0x1 <<C2C_GENIO_OPP_INT ); + +c2c_set_interrupt (C2C_GENIO_OPP_INT ,C2C_INT_HIGH ); + +dev_info (c2c_con.c2c_dev,"Port Config : 0x%x\n" , +c2c_readl (EXYNOS_C2C_PORTCONFIG )); +dev_info (c2c_con.c2c_dev,"FCLK_FREQ register : %d\n" , +c2c_readl (EXYNOS_C2C_FCLK_FREQ )); +dev_info (c2c_con.c2c_dev,"RX_MAX_FREQ register : %d\n" , +c2c_readl (EXYNOS_C2C_RX_MAX_FREQ )); +dev_info (c2c_con.c2c_dev,"IRQ_EN_SET1 register : 0x%x\n" , +c2c_readl (EXYNOS_C2C_IRQ_EN_SET1 )); + +c2c_set_clock_gating (C2C_SET ); +} + +static int __devinit samsung_c2c_probe (struct platform_device *pdev) +{ +struct exynos_c2c_platdata *pdata = pdev->dev.platform_data; +struct resource *res =NULL ; +struct resource *res1 =NULL ; +int sscm_irq0, sscm_irq1; +int err =0 ;
drivers/misc/c2c/samsung-c2c.c line 342
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+ + c2c_con.c2c_dev = &pdev->dev; + +/* resource for AP's SSCM region */ + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res) { +dev_err (&pdev->dev,"no memory resource defined(AP's SSCM)\n" ); +return -ENOENT ; + } + res =request_mem_region (res->start,resource_size (res), pdev->name);
Driver name? The parent resource already contains the device name.
drivers/misc/c2c/samsung-c2c.c line 348
Quoted 6 lines
drivers/misc/c2c/samsung-c2c.c new file +6
-0+if (!res) { +dev_err (&pdev->dev,"failded to request memory resource(AP)\n" ); +return -ENOENT ; + } + pdata->ap_sscm_addr =ioremap (res->start,resource_size (res)); +if (!pdata->ap_sscm_addr) {
This is silly. Please name 'pdata' a const and fix the build warnings that fall out from that. You really shouldn't be writing to random data that you - as a driver - don't own. And as you seem to have c2c_con.ap_sscm_addr to store it in, I see no reason for this to even be happening.
drivers/misc/c2c/samsung-c2c.c line 360
Quoted 12 lines
drivers/misc/c2c/samsung-c2c.c new file +12
-0+dev_err (&pdev->dev,"failded to request memory resource(AP)\n" ); + goto release_ap_sscm; + } + c2c_con.ap_sscm_addr = pdata->ap_sscm_addr; + +/* resource for CP's SSCM region */ + res1 =platform_get_resource (pdev,IORESOURCE_MEM ,1 ); +if (!res1) { +dev_err (&pdev->dev,"no memory resource defined(AP's SSCM)\n" ); + goto unmap_ap_sscm; + } + res1 =request_mem_region (res1->start,resource_size (res1), pdev->name);
Driver name? The parent resource already contains the device name.
drivers/misc/c2c/samsung-c2c.c line 366
Quoted 6 lines
drivers/misc/c2c/samsung-c2c.c new file +6
-0+if (!res1) { +dev_err (&pdev->dev,"failded to request memory resource(AP)\n" ); + goto unmap_ap_sscm; + } + pdata->cp_sscm_addr =ioremap (res1->start,resource_size (res1)); +if (!pdata->cp_sscm_addr) {
Same comments as for pdata->ap_sscm_addr.
Quoted 34 lines
drivers/misc/c2c/samsung-c2c.c new file +34
-0+dev_err (&pdev->dev,"failded to request memory resource(CP)\n" ); + goto release_cp_sscm; + } + c2c_con.cp_sscm_addr = pdata->cp_sscm_addr; + +/* Request IRQ for SSCM0 */ + sscm_irq0 =platform_get_irq (pdev,0 ); +if (sscm_irq0 <0 ) { +dev_err (&pdev->dev,"no irq specified\n" ); + goto unmap_cp_sscm; + } + err =request_irq (sscm_irq0, c2c_sscm0_irq,0 , pdev->name, pdev); +if (err) { +dev_err (&pdev->dev,"Can't request SSCM0 IRQ\n" ); + goto unmap_cp_sscm; + } +/* SSCM0 irq will be only used for master device */ +disable_irq (sscm_irq0); + +/* Request IRQ for SSCM1 */ + sscm_irq1 =platform_get_irq (pdev,1 ); +if (sscm_irq1 <0 ) { +dev_err (&pdev->dev,"no irq specified\n" ); + goto release_sscm_irq0; + } + err =request_irq (sscm_irq1, c2c_sscm1_irq,1 , pdev->name, pdev); +if (err) { +dev_err (&pdev->dev,"Can't request SSCM1 IRQ\n" ); + goto release_sscm_irq0; + } + +if (err) { +dev_err (&pdev->dev,"Can't register chrdev!\n" ); + goto release_sscm_irq0;
drivers/misc/c2c/samsung-c2c.c line 410
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+ } + +set_c2c_device (pdev); + +/* Create sysfs file for C2C debug */ + err =device_create_file (&pdev->dev, &dev_attr_c2c_ctrl); +if (err) { +dev_err (&pdev->dev,"Failed to create sysfs for C2C\n" ); + goto release_sscm_irq1; + }
Where do you remove this device file? It looks like an oops waiting to happen if you remove this module.
Quoted 28 lines
drivers/misc/c2c/samsung-c2c.c new file +28
-0+ +return 0 ; + +release_sscm_irq1: +free_irq (sscm_irq1, pdev); + +release_sscm_irq0: +free_irq (sscm_irq0, pdev); + +unmap_cp_sscm: +iounmap (pdata->cp_sscm_addr); + +release_cp_sscm: +release_mem_region (res1->start,resource_size (res1)); + +unmap_ap_sscm: +iounmap (pdata->ap_sscm_addr); + +release_ap_sscm: +release_mem_region (res->start,resource_size (res)); + +return err; +} + +static int __devexit samsung_c2c_remove(struct platform_device *pdev) +{ +struct exynos_c2c_platdata *pdata = pdev->dev.platform_data; +struct resource *res =NULL ;
drivers/misc/c2c/samsung-c2c.c line 448
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+struct resource *res1 =NULL ; +int sscm_irq0, sscm_irq1; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + res1 =platform_get_resource (pdev,IORESOURCE_MEM ,1 ); + sscm_irq0 =platform_get_irq (pdev,0 ); + sscm_irq1 =platform_get_irq (pdev,1 ); + +iounmap (pdata->ap_sscm_addr); +iounmap (pdata->cp_sscm_addr);
Why not use c2c_con.cp_sscm_addr ? Why not put &c2c_con into the device driver data in the probe, and retrieve it at remove and suspend/resume time?
Quoted 11 lines
drivers/misc/c2c/samsung-c2c.c new file +11
-0+release_mem_region (res->start,resource_size (res)); +release_mem_region (res1->start,resource_size (res1)); + +free_irq (sscm_irq0, pdev); +free_irq (sscm_irq1, pdev); + +return 0 ; +} + +#ifdef CONFIG_PM +static int samsung_c2c_suspend (struct platform_device *dev,pm_message_t pm)
drivers/misc/c2c/samsung-c2c.c line 469
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+{ +/* Nothing here yet */ +return 0 ; +} + +static int samsung_c2c_resume (struct platform_device *dev) +{ +/* Nothing here yet */ +return 0 ; +}
If there's nothing that you want to do here, don't provide these functions until you've thought of something to do. It just adds unnecessary patch noise.
drivers/misc/c2c/samsung-c2c.c line 479
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+#else +#define samsung_c2c_suspend NULL +#define samsung_c2c_resume NULL +#endif + +static struct platform_driver samsung_c2c_driver = { + .probe = samsung_c2c_probe, + .remove =__devexit_p (samsung_c2c_remove), + .suspend = samsung_c2c_suspend, + .resume = samsung_c2c_resume,
These two suspend/resume methods are superseded by the dev_pm_ops stuff. Please convert to use this instead.
Quoted 64 lines
drivers/misc/c2c/samsung-c2c.c new file +21
-0+ .driver = { + .name = "samsung-c2c", + .owner = THIS_MODULE, + }, +}; + +static int __init samsung_c2c_init (void ) +{ +return platform_driver_register (&samsung_c2c_driver); +} +module_init (samsung_c2c_init); + +static void __exit samsung_c2c_exit(void) +{ +platform_driver_unregister (&samsung_c2c_driver); +} +module_exit (samsung_c2c_exit); + +MODULE_DESCRIPTION ("Samsung C2C driver" ); +MODULE_AUTHOR ("Kisang Lee <[email protected]>" ); +MODULE_LICENSE ("GPL" );drivers/misc/c2c/samsung-c2c.h new file +37
-0@@ -0,0 +1,300 @@ +/* +* Samsung C2C driver +* +* Copyright (C) 2011 Samsung Electronics Co.Ltd +* Author: Kisang Lee <[email protected]> +* +* This program is free software; you can redistribute it and/or modify it +* under the terms of the GNU General Public License as published by the +* Free Software Foundation; either version 2 of the License, or (at your +* option) any later version. +*/ +#ifndef SAMSUNG_C2C_H +#define SAMSUNG_C2C_H + +enum c2c_set_clear { + C2C_CLEAR = 0, + C2C_SET = 1, +}; + +enum c2c_interrupt { + C2C_INT_TOGGLE = 0, + C2C_INT_HIGH = 1, + C2C_INT_LOW = 2, +}; + +struct c2c_state_control { + void __iomem *ap_sscm_addr; + void __iomem *cp_sscm_addr; + struct device *c2c_dev; + + u32 rx_width; + u32 tx_width; + + u32 clk_opp100; + u32 clk_opp50; + u32 clk_opp25; +
drivers/misc/c2c/samsung-c2c.h line 47
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.h new file +10
-0+struct clk *c2c_sclk; +struct clk *c2c_aclk; + +enum c2c_opp_mode opp_mode; + +u32 retention_reg; +void __iomem *c2c_sysreg; +}; + +static struct c2c_state_control c2c_con;
Why is this in a header file - and why do you only allow one of these devices in the driver (with no protection against a second one being registered.)
I see no reason why this file exists anyway - why isn't this in drivers/misc/c2c/samsung-c2c.c ?
Quoted 262 lines
drivers/misc/c2c/samsung-c2c.h new file +253
-0+ +static inline void c2c_writel (u32 val,int reg) +{ +writel (val, c2c_con.ap_sscm_addr + reg); +} + +static inline void c2c_writew (u16 val,int reg) +{ +writew (val, c2c_con.ap_sscm_addr + reg); +} + +static inline void c2c_writeb (u8 val,int reg) +{ +writeb (val, c2c_con.ap_sscm_addr + reg); +} + +static inline u32 c2c_readl (int reg) +{ +return readl (c2c_con.ap_sscm_addr + reg); +} + +static inline u16 c2c_readw (int reg) +{ +return readw (c2c_con.ap_sscm_addr + reg); +} + +static inline u8 c2c_readb (int reg) +{ +return readb (c2c_con.ap_sscm_addr + reg); +} + +static inline void c2c_writel_cp (u32 val,int reg) +{ +writel (val, c2c_con.cp_sscm_addr + reg); +} + +static inline void c2c_writew_cp (u16 val,int reg) +{ +writew (val, c2c_con.cp_sscm_addr + reg); +} + +static inline void c2c_writeb_cp (u8 val,int reg) +{ +writeb (val, c2c_con.cp_sscm_addr + reg); +} + +static inline u32 c2c_readl_cp (int reg) +{ +return readl (c2c_con.cp_sscm_addr + reg); +} + +static inline u16 c2c_readw_cp (int reg) +{ +return readw (c2c_con.cp_sscm_addr + reg); +} + +static inline u8 c2c_readb_cp (int reg) +{ +return readb (c2c_con.cp_sscm_addr + reg); +} + +static inline enum c2c_set_clear c2c_get_clock_gating (void ) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); +if (sysreg & (1 <<C2C_SYSREG_CG )) +return C2C_SET ; +else +return C2C_CLEAR ; +} + +static inline void c2c_set_clock_gating (enum c2c_set_clear val) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + +if (val ==C2C_SET ) + sysreg |= (1 <<C2C_SYSREG_CG ); +else + sysreg &= ~(1 <<C2C_SYSREG_CG ); + +writel (sysreg, c2c_con.c2c_sysreg); +} + +static inline enum c2c_set_clear c2c_get_memdone (void ) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); +if (sysreg & (1 <<C2C_SYSREG_MD )) +return C2C_SET ; +else +return C2C_CLEAR ; +} + +static inline void c2c_set_memdone (enum c2c_set_clear val) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + +if (val ==C2C_SET ) + sysreg |= (1 <<C2C_SYSREG_MD ); +else + sysreg &= ~(1 <<C2C_SYSREG_MD ); + +writel (sysreg, c2c_con.c2c_sysreg); +} + +static inline enum c2c_set_clear c2c_get_master_on (void ) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); +if (sysreg & (1 <<C2C_SYSREG_MO )) +return C2C_SET ; +else +return C2C_CLEAR ; +} + +static inline void c2c_set_master_on (enum c2c_set_clear val) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + +if (val ==C2C_SET ) + sysreg |= (1 <<C2C_SYSREG_MO ); +else + sysreg &= ~(1 <<C2C_SYSREG_MO ); + +writel (sysreg, c2c_con.c2c_sysreg); +} + +static inline u32 c2c_get_func_clk (void ) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + + sysreg &= (0x3ff <<C2C_SYSREG_FCLK ); + +return sysreg >>C2C_SYSREG_FCLK ; +} + +static inline void c2c_set_func_clk (u32 val) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + + sysreg &= ~(0x3ff <<C2C_SYSREG_FCLK ); + sysreg |= (val <<C2C_SYSREG_FCLK ); + +writel (sysreg, c2c_con.c2c_sysreg); +} + +static inline u32 c2c_get_tx_buswidth (void ) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + + sysreg &= (0x3 <<C2C_SYSREG_TXW ); + +return sysreg >>C2C_SYSREG_TXW ; +} + +static inline void c2c_set_tx_buswidth (u32 val) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + + sysreg &= ~(0x3 <<C2C_SYSREG_TXW ); + sysreg |= (val <<C2C_SYSREG_TXW ); + +writel (sysreg, c2c_con.c2c_sysreg); +} + +static inline u32 c2c_get_rx_buswidth (void ) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + + sysreg &= (0x3 <<C2C_SYSREG_RXW ); + +return sysreg >>C2C_SYSREG_RXW ; +} + +static inline void c2c_set_rx_buswidth (u32 val) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + + sysreg &= ~(0x3 <<C2C_SYSREG_RXW ); + sysreg |= (val <<C2C_SYSREG_RXW ); + +writel (sysreg, c2c_con.c2c_sysreg); +} + +static inline enum c2c_set_clear c2c_get_reset (void ) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); +if (sysreg & (1 <<C2C_SYSREG_RST )) +return C2C_SET ; +else +return C2C_CLEAR ; +} + +static inline void c2c_set_reset (enum c2c_set_clear val) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + +if (val ==C2C_SET ) + sysreg |= (1 <<C2C_SYSREG_RST ); +else + sysreg &= ~(1 <<C2C_SYSREG_RST ); + +writel (sysreg, c2c_con.c2c_sysreg); +} + +static inline void c2c_set_rtrst (enum c2c_set_clear val) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + +if (val ==C2C_SET ) + sysreg |= (1 <<C2C_SYSREG_RTRST ); +else + sysreg &= ~(1 <<C2C_SYSREG_RTRST ); + +writel (sysreg, c2c_con.c2c_sysreg); +} + +static inline u32 c2c_get_base_addr (void ) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + + sysreg &= (0x3ff <<C2C_SYSREG_BASE_ADDR ); + +return sysreg >>C2C_SYSREG_BASE_ADDR ; +} + +static inline void c2c_set_base_addr (u32 val) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + + sysreg &= ~(0x3ff <<C2C_SYSREG_BASE_ADDR ); + sysreg |= (val <<C2C_SYSREG_BASE_ADDR ); + +writel (sysreg, c2c_con.c2c_sysreg); +} + +static inline u32 c2c_get_shdmem_size (void ) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + + sysreg &= (0x7 <<C2C_SYSREG_DRAM_SIZE ); + +return sysreg >>C2C_SYSREG_DRAM_SIZE ; +} + +static inline void c2c_set_shdmem_size (u32 val) +{ +u32 sysreg =readl (c2c_con.c2c_sysreg); + + sysreg &= ~(0x7 <<C2C_SYSREG_DRAM_SIZE ); + sysreg |= (val <<C2C_SYSREG_DRAM_SIZE ); + +writel (sysreg, c2c_con.c2c_sysreg); +} + +#endif Signature · 8 lines
1.7.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 2/2 misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS
In reply to Kisang LeeQuoted 4 lines · Kisang Lee
Cc Arnd Bergmann <arnd <at> arndb.de>Cc Greg Kroah-Hartman <greg <at> kroah.com>Signed-off-by Kisang Lee <[email protected]>
What is a chip to chip driver? It looks like there is some overlap with the remoteproc work that Ohad Ben-Cohen has been doing, or I think there was also some work for communicating with things like basebands? Perhaps even UIO?
drivers/misc/c2c/samsung-c2c.c line 104
Quoted 6 lines
drivers/misc/c2c/samsung-c2c.c new file +6
-0+if (opp_val ==C2C_OPP100 ) + req_clk = c2c_con.clk_opp100; +else if (opp_val ==C2C_OPP50 ) + req_clk = c2c_con.clk_opp50; +else if (opp_val ==C2C_OPP25 ) + req_clk = c2c_con.clk_opp25;
This looks like a switch statement.
drivers/misc/c2c/samsung-c2c.c line 143
Quoted 11 lines
drivers/misc/c2c/samsung-c2c.c new file +11
-0+dev_info (c2c_con.c2c_dev,"Get C2C sclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_sclk)); +dev_info (c2c_con.c2c_dev,"Get C2C aclk rate : %ld\n" , +clk_get_rate (c2c_con.c2c_aclk)); +break ; +default : +dev_info (c2c_con.c2c_dev,"---C2C Operation Number---\n" ); +dev_info (c2c_con.c2c_dev,"1. C2C Reset\n" ); +dev_info (c2c_con.c2c_dev,"2. Set OPP25\n" ); +dev_info (c2c_con.c2c_dev,"3. Set OPP50\n" ); +dev_info (c2c_con.c2c_dev,"4. Set OPP100\n" );
These log messages probably shouldn't be in.
drivers/misc/c2c/samsung-c2c.c line 194
Quoted 5 lines
drivers/misc/c2c/samsung-c2c.c new file +5
-0+static irqreturn_t c2c_sscm0_irq (int irq,void *data) +{ +/* Nothing here yet */ +return IRQ_HANDLED ; +}
This doesn't look terribly clever - if the interrupt has been asserted and we don't do anything to handle it won't we end up spinning for ever with repeated calls to the interrupt handler?
drivers/misc/c2c/samsung-c2c.c line 104
Quoted 6 lines
drivers/misc/c2c/samsung-c2c.c new file +6
-0+if (opp_val ==C2C_OPP100 ) + req_clk = c2c_con.clk_opp100; +else if (opp_val ==C2C_OPP50 ) + req_clk = c2c_con.clk_opp50; +else if (opp_val ==C2C_OPP25 ) + req_clk = c2c_con.clk_opp25;
This and the surrounding code looks a lot like the code I commented on above - shouldn't this be factored out into a single function called from both places?
drivers/misc/c2c/samsung-c2c.c line 341
Quoted 6 lines
drivers/misc/c2c/samsung-c2c.c new file +6
-0+/* resource for AP's SSCM region */ + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res) { +dev_err (&pdev->dev,"no memory resource defined(AP's SSCM)\n" ); +return -ENOENT ; + }
devm_requets_and_ioremap() perhaps?
Quoted 6 lines
drivers/misc/c2c/samsung-c2c.c new file +6
-0+#ifdef CONFIG_PM +static int samsung_c2c_suspend (struct platform_device *dev,pm_message_t pm) +{ +/* Nothing here yet */ +return 0 ; +}
drivers/misc/c2c/samsung-c2c.c line 473
Quoted 10 lines
drivers/misc/c2c/samsung-c2c.c new file +10
-0+ +static int samsung_c2c_resume (struct platform_device *dev) +{ +/* Nothing here yet */ +return 0 ; +} +#else +#define samsung_c2c_suspend NULL +#define samsung_c2c_resume NULL +#endif
No need to include this if it doesn't do anything.
drivers/misc/c2c/samsung-c2c.c line 479
Quoted 5 lines
drivers/misc/c2c/samsung-c2c.c new file +5
-0+static struct platform_driver samsung_c2c_driver = { + .probe = samsung_c2c_probe, + .remove =__devexit_p (samsung_c2c_remove), + .suspend = samsung_c2c_suspend, + .resume = samsung_c2c_resume,
If you were including pm operations they should be in the pm_ops rather than using the platform-specific variants - this makes it easier to do things like runtime PM later.
drivers/misc/c2c/samsung-c2c.c line 490
Quoted 5 lines
drivers/misc/c2c/samsung-c2c.c new file +5
-0+static int __init samsung_c2c_init (void ) +{ +return platform_driver_register (&samsung_c2c_driver); +} +module_init (samsung_c2c_init);
module_platform_driver().