Re: [PATCH v4 2/5] MFD: ti_tscadc: Add support for TI's TSC/ADC MFDevice

2012-09-30 Thread Samuel Ortiz
Hi Rachna,

On Wed, Sep 26, 2012 at 10:50:11AM +0530, Patil, Rachna wrote:
> Add the mfd core driver which supports touchscreen
> and ADC.
> With this patch we are only adding infrastructure to
> support the MFD clients.
> 
> Signed-off-by: Patil, Rachna 
> ---
> Changes in v2:
>   Merged "[PATCH 5/5] MFD: ti_tscadc: Add check on number of i/p 
> channels",
>   patch submitted in previous version into this file.
> 
> Changes in v3:
>   No changes
> 
> Changes in v4:
>   No changes
> 
>  drivers/mfd/Kconfig  |9 ++
>  drivers/mfd/Makefile |1 +
>  drivers/mfd/ti_am335x_tscadc.c   |  193 
> ++
>  include/linux/mfd/ti_am335x_tscadc.h |  133 +++
>  4 files changed, 336 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/mfd/ti_am335x_tscadc.c
>  create mode 100644 include/linux/mfd/ti_am335x_tscadc.h
Looks good to me, I only have a few comments:

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b1a1462..e472184 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -94,6 +94,15 @@ config MFD_TI_SSP
> To compile this driver as a module, choose M here: the
> module will be called ti-ssp.
>  
> +config MFD_TI_AM335X_TSCADC
> + tristate "TI ADC / Touch Screen chip support"
> + depends on ARCH_OMAP2PLUS
- Why do you need to depend on that architecture ?
- You must select MFD_CORE.

> +static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int 
> reg)
> +{
> + return readl(tsadc->tscadc_base + reg);
> +}
> +
> +static void tscadc_writel(struct ti_tscadc_dev *tsadc, unsigned int reg,
> + unsigned int val)
> +{
> + writel(val, tsadc->tscadc_base + reg);
> +}
> +
Have you considered using the regmap MMIO API ? The benefits are not very
obvious in that case, but it's always better to use a common framework.


> +static   int __devinit ti_tscadc_probe(struct platform_device *pdev)
> +{
> + struct ti_tscadc_dev*tscadc;
> + struct resource *res;
> + struct clk  *clk;
> + struct mfd_tscadc_board *pdata = pdev->dev.platform_data;
> + int irq;
> + int err, ctrl;
> + int clk_value, clock_rate;
> +
> + if (!pdata) {
> + dev_err(&pdev->dev, "Could not find platform data\n");
> + return -EINVAL;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "no memory resource defined.\n");
> + return -EINVAL;
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "no irq ID is specified.\n");
> + return -EINVAL;
> + }
> +
> + /* Allocate memory for device */
> + tscadc = kzalloc(sizeof(struct ti_tscadc_dev), GFP_KERNEL);
devm_kzalloc ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH v4 2/5] MFD: ti_tscadc: Add support for TI's TSC/ADC MFDevice

2012-09-25 Thread Patil, Rachna
Add the mfd core driver which supports touchscreen
and ADC.
With this patch we are only adding infrastructure to
support the MFD clients.

Signed-off-by: Patil, Rachna 
---
Changes in v2:
Merged "[PATCH 5/5] MFD: ti_tscadc: Add check on number of i/p 
channels",
patch submitted in previous version into this file.

Changes in v3:
No changes

Changes in v4:
No changes

 drivers/mfd/Kconfig  |9 ++
 drivers/mfd/Makefile |1 +
 drivers/mfd/ti_am335x_tscadc.c   |  193 ++
 include/linux/mfd/ti_am335x_tscadc.h |  133 +++
 4 files changed, 336 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mfd/ti_am335x_tscadc.c
 create mode 100644 include/linux/mfd/ti_am335x_tscadc.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b1a1462..e472184 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -94,6 +94,15 @@ config MFD_TI_SSP
  To compile this driver as a module, choose M here: the
  module will be called ti-ssp.
 
+config MFD_TI_AM335X_TSCADC
+   tristate "TI ADC / Touch Screen chip support"
+   depends on ARCH_OMAP2PLUS
+   help
+ If you say yes here you get support for Texas Instruments series
+ of Touch Screen /ADC chips.
+ To compile this driver as a module, choose M here: the
+ module will be called ti_am335x_tscadc.
+
 config HTC_EGPIO
bool "HTC EGPIO support"
depends on GENERIC_HARDIRQS && GPIOLIB && ARM
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 79dd22d..bc6b2f0 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_HTC_I2CPLD)  += htc-i2cpld.o
 obj-$(CONFIG_MFD_DAVINCI_VOICECODEC)   += davinci_voicecodec.o
 obj-$(CONFIG_MFD_DM355EVM_MSP) += dm355evm_msp.o
 obj-$(CONFIG_MFD_TI_SSP)   += ti-ssp.o
+obj-$(CONFIG_MFD_TI_AM335X_TSCADC) += ti_am335x_tscadc.o
 
 obj-$(CONFIG_MFD_STA2X11)  += sta2x11-mfd.o
 obj-$(CONFIG_MFD_STMPE)+= stmpe.o
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
new file mode 100644
index 000..ae93b48
--- /dev/null
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -0,0 +1,193 @@
+/*
+ * TI Touch Screen / ADC MFD driver
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * 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 version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg)
+{
+   return readl(tsadc->tscadc_base + reg);
+}
+
+static void tscadc_writel(struct ti_tscadc_dev *tsadc, unsigned int reg,
+   unsigned int val)
+{
+   writel(val, tsadc->tscadc_base + reg);
+}
+
+static void tscadc_idle_config(struct ti_tscadc_dev *config)
+{
+   unsigned int idleconfig;
+
+   idleconfig = STEPCONFIG_YNN | STEPCONFIG_INM_ADCREFM |
+   STEPCONFIG_INP_ADCREFM | STEPCONFIG_YPN;
+
+   tscadc_writel(config, REG_IDLECONFIG, idleconfig);
+}
+
+static int __devinit ti_tscadc_probe(struct platform_device *pdev)
+{
+   struct ti_tscadc_dev*tscadc;
+   struct resource *res;
+   struct clk  *clk;
+   struct mfd_tscadc_board *pdata = pdev->dev.platform_data;
+   int irq;
+   int err, ctrl;
+   int clk_value, clock_rate;
+
+   if (!pdata) {
+   dev_err(&pdev->dev, "Could not find platform data\n");
+   return -EINVAL;
+   }
+
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   if (!res) {
+   dev_err(&pdev->dev, "no memory resource defined.\n");
+   return -EINVAL;
+   }
+
+   irq = platform_get_irq(pdev, 0);
+   if (irq < 0) {
+   dev_err(&pdev->dev, "no irq ID is specified.\n");
+   return -EINVAL;
+   }
+
+   /* Allocate memory for device */
+   tscadc = kzalloc(sizeof(struct ti_tscadc_dev), GFP_KERNEL);
+   if (!tscadc) {
+   dev_err(&pdev->dev, "failed to allocate memory.\n");
+   return -ENOMEM;
+   }
+   tscadc->dev = &pdev->dev;
+   tscadc->irq = irq;
+
+   res = request_mem_region(res->start, resource_size(res), pdev->name);
+   if (!res) {
+   dev_err(&pdev->dev, "failed to reserve registers.\n");
+   err = -EBUSY;
+   goto err