subject:"\[PATCH 2\/2\] misc\: c2c\: Add C2C\(Chip to Chip\) device driver for EXYNOS"

Re: [PATCH 2/2] misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS

2012-02-06 Thread Mark Brown

On Sat, Feb 04, 2012 at 05:15:03PM +0900, Kisang Lee wrote:
> Cc: Arnd Bergmann  arndb.de>
> Cc: Greg Kroah-Hartman  kroah.com>
> 
> Signed-off-by: Kisang Lee 

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?

> + 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.

> + 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.

> +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?

> + 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?

> + /* 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?

> +#ifdef CONFIG_PM
> +static int samsung_c2c_suspend(struct platform_device *dev, pm_message_t pm)
> +{
> + /* Nothing here yet */
> + return 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.

> +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.

> +static int __init samsung_c2c_init(void)
> +{
> + return platform_driver_register(&samsung_c2c_driver);
> +}
> +module_init(samsung_c2c_init);

module_platform_driver().
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS

2012-02-04 Thread Russell King - ARM Linux

On Sat, Feb 04, 2012 at 05:15:03PM +0900, Kisang Lee wrote:
> Cc: Arnd Bergmann  arndb.de>
> Cc: Greg Kroah-Hartman  kroah.com>
> 
> Signed-off-by: Kisang Lee 

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.

> ---
>  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
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 6a1a092..2ec3f48 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -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
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 3e1d801..bd96ed7 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -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/
> diff --git a/drivers/misc/c2c/Kconfig b/drivers/misc/c2c/Kconfig
> new file mode 100644
> index 000..cd13078
> --- /dev/null
> +++ b/drivers/misc/c2c/Kconfig
> @@ -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.
> diff --git a/drivers/misc/c2c/Makefile b/drivers/misc/c2c/Makefile
> new file mode 100644
> index 000..1af7d3a
> --- /dev/null
> +++ b/drivers/misc/c2c/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for C2C(Chip to Chip) device
> +#
> +
> +obj-$(CONFIG_SAMSUNG_C2C)+= samsung-c2c.o
> diff --git a/drivers/misc/c2c/samsung-c2c.c b/drivers/misc/c2c/samsung-c2c.c
> new file mode 100644
> index 000..ee08ac6
> --- /dev/null
> +++ b/drivers/misc/c2c/samsung-c2c.c
> @@ -0,0 +1,500 @@
> +/*
> + * Samsung C2C driver
> + *
> + * Copyright (C) 2011 Samsung Electronics Co.Ltd
> + * Author: Kisang Lee 
> + *
> + * 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 
> +#include 
> +#include 
> +#include 

I can find nothing related to the input layer in this file - is this
header required?

> +#include 
> +#include 
> +#include 
> +#include 

I can find nothing which declares a misc_device in this file - is this
header required?

> +#include 
> +#include 
> +#include 
> +#include 

You appear to use nothing from this header, please remove it.  Drivers
should not depend on the type of platform they're running on.

> +
> +#include 
> +#include 
> +#include 

Is plat/cpu.h necessary?

> +
> +#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_FR

Re: [PATCH 2/2] misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS

2012-02-04 Thread Sylwester Nawrocki

Hi,

On 02/04/2012 09:15 AM, Kisang Lee wrote:
> Cc: Arnd Bergmann  arndb.de>
> Cc: Greg Kroah-Hartman  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 ?

> Signed-off-by: Kisang Lee
> ---
>   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
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 6a1a092..2ec3f48 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -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
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 3e1d801..bd96ed7 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -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/
> diff --git a/drivers/misc/c2c/Kconfig b/drivers/misc/c2c/Kconfig
> new file mode 100644
> index 000..cd13078
> --- /dev/null
> +++ b/drivers/misc/c2c/Kconfig
> @@ -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.

> + help
> +   It is for supporting C2C driver.
> diff --git a/drivers/misc/c2c/Makefile b/drivers/misc/c2c/Makefile
> new file mode 100644
> index 000..1af7d3a
> --- /dev/null
> +++ b/drivers/misc/c2c/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for C2C(Chip to Chip) device
^^^
White-space after C2C ?
> +#
> +
> +obj-$(CONFIG_SAMSUNG_C2C)+= samsung-c2c.o
> diff --git a/drivers/misc/c2c/samsung-c2c.c b/drivers/misc/c2c/samsung-c2c.c
> new file mode 100644
> index 000..ee08ac6
> --- /dev/null
> +++ b/drivers/misc/c2c/samsung-c2c.c
> @@ -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.

> + * Author: Kisang Lee
> + *
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#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.

> + 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",

nit: You could use "%#x" notation here and anywhere else for
hex numbers, that's one charac

[PATCH 2/2] misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS

2012-02-04 Thread Kisang Lee

Cc: Arnd Bergmann  arndb.de>
Cc: Greg Kroah-Hartman  kroah.com>

Signed-off-by: Kisang Lee 
---
 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

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 6a1a092..2ec3f48 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -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
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 3e1d801..bd96ed7 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -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/
diff --git a/drivers/misc/c2c/Kconfig b/drivers/misc/c2c/Kconfig
new file mode 100644
index 000..cd13078
--- /dev/null
+++ b/drivers/misc/c2c/Kconfig
@@ -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.
diff --git a/drivers/misc/c2c/Makefile b/drivers/misc/c2c/Makefile
new file mode 100644
index 000..1af7d3a
--- /dev/null
+++ b/drivers/misc/c2c/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for C2C(Chip to Chip) device
+#
+
+obj-$(CONFIG_SAMSUNG_C2C)  += samsung-c2c.o
diff --git a/drivers/misc/c2c/samsung-c2c.c b/drivers/misc/c2c/samsung-c2c.c
new file mode 100644
index 000..ee08ac6
--- /dev/null
+++ b/drivers/misc/c2c/samsung-c2c.c
@@ -0,0 +1,500 @@
+/*
+ * Samsung C2C driver
+ *
+ * Copyright (C) 2011 Samsung Electronics Co.Ltd
+ * Author: Kisang Lee 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+#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",
+   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);
+
+   switch (ops_num) {
+   case 1:
+   c2c_reset_ops();
+

Re: [PATCH 2/2] misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS

Re: [PATCH 2/2] misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS

Re: [PATCH 2/2] misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS

[PATCH 2/2] misc: c2c: Add C2C(Chip to Chip) device driver for EXYNOS

4 matches

Site Navigation

Mail list logo

Footer information