[PATCHv6 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean

[PATCHv6 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean

[Chanwoo Choi]

This patchset support Exynos3250 ADC (Analog Digital Converter) because
Exynos3250 has additional special clock for ADC IP.

Changes from v5:
- Add acked message by Kukjin Kim
- Add reviewed messgae by Tomasz Figa
- Fix typo (for for -> for)

Changes from v4:
- Use 'exynos_adc_data' structure instead of 'exynos_adc_ops' structure
  and remove enum variable of ADC version
- Fix wrong name of special clock (sclk_tsadc -> sclk_adc)
- Add reviewed message by Naveen Krishna Chatradhi
- Add functions for ADC clock control

Changes from v3:
- Add new 'exynos_adc_ops' structure to improve readability according to
 Tomasz Figa comment[1]
 [1] https://lkml.org/lkml/2014/4/16/238
- Add new 'exynos3250-adc-v2' compatible string to support Exynos3250 ADC
- Fix wrong compaitlbe string of ADC in Exynos3250 dtsi file

Changes from v2:
- Check return value of clock function to deal with error exception
- Fix minor coding style to improve readability

Changes from v1:
- Add new "samsung,exynos-adc-v3" compatible to support Exynos3250 ADC
- Add a patch about DT binding documentation

Chanwoo Choi (4):
  iio: adc: exynos_adc: Add exynos_adc_data structure to improve
    readability
  iio: adc: exynos_adc: Control special clock of ADC to support
    Exynos3250 ADC
  iio: devicetree: Add DT binding documentation for Exynos3250 ADC
  ARM: dts: Fix wrong compatible string for Exynos3250 ADC

 .../devicetree/bindings/arm/samsung/exynos-adc.txt |  25 +-
 arch/arm/boot/dts/exynos3250.dtsi                  |   4 +-
 drivers/iio/adc/exynos_adc.c                       | 326 +++++++++++++++------
 3 files changed, 267 insertions(+), 88 deletions(-)

-- 
1.8.0

[PATCHv6 1/4] iio: adc: exynos_adc: Add exynos_adc_data structure to improve readability

[Chanwoo Choi]

This patchset add 'exynos_adc_data' structure which includes some functions
to control ADC operation and specific data according to ADC version (v1 or v2).

Signed-off-by: Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Reviewed-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Reviewed-by: Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iio/adc/exynos_adc.c | 226 ++++++++++++++++++++++++++++---------------
 1 file changed, 147 insertions(+), 79 deletions(-)

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 010578f..00d67fd 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -39,11 +39,6 @@
 #include <linux/iio/machine.h>
 #include <linux/iio/driver.h>
 
-enum adc_version {
-	ADC_V1,
-	ADC_V2
-};
-
 /* EXYNOS4412/5250 ADC_V1 registers definitions */
 #define ADC_V1_CON(x)		((x) + 0x00)
 #define ADC_V1_DLY(x)		((x) + 0x08)
@@ -85,6 +80,7 @@ enum adc_version {
 #define EXYNOS_ADC_TIMEOUT	(msecs_to_jiffies(100))
 
 struct exynos_adc {
+	struct exynos_adc_data	*data;
 	void __iomem		*regs;
 	void __iomem		*enable_reg;
 	struct clk		*clk;
@@ -97,43 +93,139 @@ struct exynos_adc {
 	unsigned int            version;
 };
 
-static const struct of_device_id exynos_adc_match[] = {
-	{ .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
-	{ .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
-	{},
+struct exynos_adc_data {
+	int num_channels;
+
+	void (*init_hw)(struct exynos_adc *info);
+	void (*exit_hw)(struct exynos_adc *info);
+	void (*clear_irq)(struct exynos_adc *info);
+	void (*start_conv)(struct exynos_adc *info, unsigned long addr);
 };
-MODULE_DEVICE_TABLE(of, exynos_adc_match);
 
-static inline unsigned int exynos_adc_get_version(struct platform_device *pdev)
+static void exynos_adc_v1_init_hw(struct exynos_adc *info)
 {
-	const struct of_device_id *match;
+	u32 con1;
 
-	match = of_match_node(exynos_adc_match, pdev->dev.of_node);
-	return (unsigned int)match->data;
+	writel(1, info->enable_reg);
+
+	/* set default prescaler values and Enable prescaler */
+	con1 =  ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
+
+	/* Enable 12-bit ADC resolution */
+	con1 |= ADC_V1_CON_RES;
+	writel(con1, ADC_V1_CON(info->regs));
+}
+
+static void exynos_adc_v1_exit_hw(struct exynos_adc *info)
+{
+	u32 con;
+
+	writel(0, info->enable_reg);
+
+	con = readl(ADC_V1_CON(info->regs));
+	con |= ADC_V1_CON_STANDBY;
+	writel(con, ADC_V1_CON(info->regs));
+}
+
+static void exynos_adc_v1_clear_irq(struct exynos_adc *info)
+{
+	writel(1, ADC_V1_INTCLR(info->regs));
 }
 
-static void exynos_adc_hw_init(struct exynos_adc *info)
+static void exynos_adc_v1_start_conv(struct exynos_adc *info,
+				     unsigned long addr)
+{
+	u32 con1;
+
+	writel(addr, ADC_V1_MUX(info->regs));
+
+	con1 = readl(ADC_V1_CON(info->regs));
+	writel(con1 | ADC_CON_EN_START, ADC_V1_CON(info->regs));
+}
+
+static struct exynos_adc_data const exynos_adc_v1_data = {
+	.num_channels	= MAX_ADC_V1_CHANNELS,
+
+	.init_hw	= exynos_adc_v1_init_hw,
+	.exit_hw	= exynos_adc_v1_exit_hw,
+	.clear_irq	= exynos_adc_v1_clear_irq,
+	.start_conv	= exynos_adc_v1_start_conv,
+};
+
+static void exynos_adc_v2_init_hw(struct exynos_adc *info)
 {
 	u32 con1, con2;
 
-	if (info->version == ADC_V2) {
-		con1 = ADC_V2_CON1_SOFT_RESET;
-		writel(con1, ADC_V2_CON1(info->regs));
+	writel(1, info->enable_reg);
 
-		con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
-			ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
-		writel(con2, ADC_V2_CON2(info->regs));
+	con1 = ADC_V2_CON1_SOFT_RESET;
+	writel(con1, ADC_V2_CON1(info->regs));
 
-		/* Enable interrupts */
-		writel(1, ADC_V2_INT_EN(info->regs));
-	} else {
-		/* set default prescaler values and Enable prescaler */
-		con1 =  ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
+	con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
+		ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
+	writel(con2, ADC_V2_CON2(info->regs));
 
-		/* Enable 12-bit ADC resolution */
-		con1 |= ADC_V1_CON_RES;
-		writel(con1, ADC_V1_CON(info->regs));
-	}
+	/* Enable interrupts */
+	writel(1, ADC_V2_INT_EN(info->regs));
+}
+
+static void exynos_adc_v2_exit_hw(struct exynos_adc *info)
+{
+	u32 con;
+
+	writel(0, info->enable_reg);
+
+	con = readl(ADC_V2_CON1(info->regs));
+	con &= ~ADC_CON_EN_START;
+	writel(con, ADC_V2_CON1(info->regs));
+}
+
+static void exynos_adc_v2_clear_irq(struct exynos_adc *info)
+{
+	writel(1, ADC_V2_INT_ST(info->regs));
+}
+
+static void exynos_adc_v2_start_conv(struct exynos_adc *info,
+				     unsigned long addr)
+{
+	u32 con1, con2;
+
+	con2 = readl(ADC_V2_CON2(info->regs));
+	con2 &= ~ADC_V2_CON2_ACH_MASK;
+	con2 |= ADC_V2_CON2_ACH_SEL(addr);
+	writel(con2, ADC_V2_CON2(info->regs));
+
+	con1 = readl(ADC_V2_CON1(info->regs));
+	writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs));
+}
+
+static struct exynos_adc_data const exynos_adc_v2_data = {
+	.num_channels	= MAX_ADC_V2_CHANNELS,
+
+	.init_hw	= exynos_adc_v2_init_hw,
+	.exit_hw	= exynos_adc_v2_exit_hw,
+	.clear_irq	= exynos_adc_v2_clear_irq,
+	.start_conv	= exynos_adc_v2_start_conv,
+};
+
+static const struct of_device_id exynos_adc_match[] = {
+	{
+		.compatible = "samsung,exynos-adc-v1",
+		.data = (void *)&exynos_adc_v1_data,
+	}, {
+		.compatible = "samsung,exynos-adc-v2",
+		.data = (void *)&exynos_adc_v2_data,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, exynos_adc_match);
+
+static struct exynos_adc_data *exynos_adc_get_data(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+
+	match = of_match_node(exynos_adc_match, pdev->dev.of_node);
+	return (struct exynos_adc_data *)match->data;
 }
 
 static int exynos_read_raw(struct iio_dev *indio_dev,
@@ -144,7 +236,6 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
 {
 	struct exynos_adc *info = iio_priv(indio_dev);
 	unsigned long timeout;
-	u32 con1, con2;
 	int ret;
 
 	if (mask != IIO_CHAN_INFO_RAW)
@@ -154,28 +245,15 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
 	reinit_completion(&info->completion);
 
 	/* Select the channel to be used and Trigger conversion */
-	if (info->version == ADC_V2) {
-		con2 = readl(ADC_V2_CON2(info->regs));
-		con2 &= ~ADC_V2_CON2_ACH_MASK;
-		con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
-		writel(con2, ADC_V2_CON2(info->regs));
-
-		con1 = readl(ADC_V2_CON1(info->regs));
-		writel(con1 | ADC_CON_EN_START,
-				ADC_V2_CON1(info->regs));
-	} else {
-		writel(chan->address, ADC_V1_MUX(info->regs));
-
-		con1 = readl(ADC_V1_CON(info->regs));
-		writel(con1 | ADC_CON_EN_START,
-				ADC_V1_CON(info->regs));
-	}
+	if (info->data->start_conv)
+		info->data->start_conv(info, chan->address);
 
 	timeout = wait_for_completion_timeout
 			(&info->completion, EXYNOS_ADC_TIMEOUT);
 	if (timeout == 0) {
 		dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
-		exynos_adc_hw_init(info);
+		if (info->data->init_hw)
+			info->data->init_hw(info);
 		ret = -ETIMEDOUT;
 	} else {
 		*val = info->value;
@@ -193,13 +271,11 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
 	struct exynos_adc *info = (struct exynos_adc *)dev_id;
 
 	/* Read value */
-	info->value = readl(ADC_V1_DATX(info->regs)) &
-						ADC_DATX_MASK;
+	info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
+
 	/* clear irq */
-	if (info->version == ADC_V2)
-		writel(1, ADC_V2_INT_ST(info->regs));
-	else
-		writel(1, ADC_V1_INTCLR(info->regs));
+	if (info->data->clear_irq)
+		info->data->clear_irq(info);
 
 	complete(&info->completion);
 
@@ -277,6 +353,12 @@ static int exynos_adc_probe(struct platform_device *pdev)
 
 	info = iio_priv(indio_dev);
 
+	info->data = exynos_adc_get_data(pdev);
+	if (!info->data) {
+		dev_err(&pdev->dev, "failed getting exynos_adc_data\n");
+		return -EINVAL;
+	}
+
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	info->regs = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(info->regs))
@@ -319,10 +401,6 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_disable_reg;
 
-	writel(1, info->enable_reg);
-
-	info->version = exynos_adc_get_version(pdev);
-
 	platform_set_drvdata(pdev, indio_dev);
 
 	indio_dev->name = dev_name(&pdev->dev);
@@ -331,11 +409,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	indio_dev->info = &exynos_adc_iio_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = exynos_adc_iio_channels;
-
-	if (info->version == ADC_V1)
-		indio_dev->num_channels = MAX_ADC_V1_CHANNELS;
-	else
-		indio_dev->num_channels = MAX_ADC_V2_CHANNELS;
+	indio_dev->num_channels = info->data->num_channels;
 
 	ret = request_irq(info->irq, exynos_adc_isr,
 					0, dev_name(&pdev->dev), info);
@@ -349,7 +423,8 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_irq;
 
-	exynos_adc_hw_init(info);
+	if (info->data->init_hw)
+		info->data->init_hw(info);
 
 	ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
 	if (ret < 0) {
@@ -366,7 +441,8 @@ err_of_populate:
 err_irq:
 	free_irq(info->irq, info);
 err_disable_clk:
-	writel(0, info->enable_reg);
+	if (info->data->exit_hw)
+		info->data->exit_hw(info);
 	clk_disable_unprepare(info->clk);
 err_disable_reg:
 	regulator_disable(info->vdd);
@@ -382,7 +458,8 @@ static int exynos_adc_remove(struct platform_device *pdev)
 				exynos_adc_remove_devices);
 	iio_device_unregister(indio_dev);
 	free_irq(info->irq, info);
-	writel(0, info->enable_reg);
+	if (info->data->exit_hw)
+		info->data->exit_hw(info);
 	clk_disable_unprepare(info->clk);
 	regulator_disable(info->vdd);
 
@@ -394,19 +471,10 @@ static int exynos_adc_suspend(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct exynos_adc *info = iio_priv(indio_dev);
-	u32 con;
 
-	if (info->version == ADC_V2) {
-		con = readl(ADC_V2_CON1(info->regs));
-		con &= ~ADC_CON_EN_START;
-		writel(con, ADC_V2_CON1(info->regs));
-	} else {
-		con = readl(ADC_V1_CON(info->regs));
-		con |= ADC_V1_CON_STANDBY;
-		writel(con, ADC_V1_CON(info->regs));
-	}
+	if (info->data->exit_hw)
+		info->data->exit_hw(info);
 
-	writel(0, info->enable_reg);
 	clk_disable_unprepare(info->clk);
 	regulator_disable(info->vdd);
 
@@ -427,8 +495,8 @@ static int exynos_adc_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	writel(1, info->enable_reg);
-	exynos_adc_hw_init(info);
+	if (info->data->init_hw)
+		info->data->init_hw(info);
 
 	return 0;
 }
-- 
1.8.0

Re: [PATCHv6 1/4] iio: adc: exynos_adc: Add exynos_adc_data structure to improve readability

[Arnd Bergmann]

On Friday 18 July 2014 14:59:43 Chanwoo Choi wrote:
> This patchset add 'exynos_adc_data' structure which includes some functions
> to control ADC operation and specific data according to ADC version (v1 or v2).
> 

This new structure makes a lot of sense for covering the exynos specific
versions, but it will likely give a little more complexity for the
older models. We'll have to deal with that later then, no need to
hold up your patch. Interestingly, the version numbers seem weird. The
old driver uses

	{
                .name           = "s3c24xx-adc",
                .driver_data    = TYPE_ADCV1,
        }, {
                .name           = "s3c2443-adc",
                .driver_data    = TYPE_ADCV11,
        }, {
                .name           = "s3c2416-adc",
                .driver_data    = TYPE_ADCV12,
        }, {
                .name           = "s3c64xx-adc",
                .driver_data    = TYPE_ADCV2,
        }, {
                .name           = "samsung-adc-v3",
                .driver_data    = TYPE_ADCV3,
	}

Where TYPE_ADCV3 seems to be the same as the new ADC_V1 used in this
driver. Do you have an explanation for that?

	Arnd

Re: [PATCHv6 1/4] iio: adc: exynos_adc: Add exynos_adc_data structure to improve readability

[Naveen Krishna Ch]

Hello Arnd,

On 18 July 2014 15:12, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Friday 18 July 2014 14:59:43 Chanwoo Choi wrote:
>> This patchset add 'exynos_adc_data' structure which includes some functions
>> to control ADC operation and specific data according to ADC version (v1 or v2).
>>
>
> This new structure makes a lot of sense for covering the exynos specific
> versions, but it will likely give a little more complexity for the
> older models. We'll have to deal with that later then, no need to
> hold up your patch. Interestingly, the version numbers seem weird. The
> old driver uses
>
>         {
>                 .name           = "s3c24xx-adc",
>                 .driver_data    = TYPE_ADCV1,
>         }, {
>                 .name           = "s3c2443-adc",
>                 .driver_data    = TYPE_ADCV11,
>         }, {
>                 .name           = "s3c2416-adc",
>                 .driver_data    = TYPE_ADCV12,
>         }, {
>                 .name           = "s3c64xx-adc",
>                 .driver_data    = TYPE_ADCV2,
>         }, {
>                 .name           = "samsung-adc-v3",
>                 .driver_data    = TYPE_ADCV3,
>         }
>
> Where TYPE_ADCV3 seems to be the same as the new ADC_V1 used in this
> driver. Do you have an explanation for that?

As per suggestion from Doug Anderson,
I've implemented IIO based ADC driver to work with Exynos5250.
keeping the plat-samsung/adc.c unchanged.

Assuming Exynos5250 is the one using the driver for the first time.
i've named it v1 and so on.

Now, This seems to cause a lot of confusion.

>
>         Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Shine bright,
(: Nav :)

Re: [PATCHv6 1/4] iio: adc: exynos_adc: Add exynos_adc_data structure to improve readability

[Arnd Bergmann]

On Friday 18 July 2014 15:41:27 Naveen Krishna Ch wrote:
> >
> >         {
> >                 .name           = "s3c24xx-adc",
> >                 .driver_data    = TYPE_ADCV1,
> >         }, {
> >                 .name           = "s3c2443-adc",
> >                 .driver_data    = TYPE_ADCV11,
> >         }, {
> >                 .name           = "s3c2416-adc",
> >                 .driver_data    = TYPE_ADCV12,
> >         }, {
> >                 .name           = "s3c64xx-adc",
> >                 .driver_data    = TYPE_ADCV2,
> >         }, {
> >                 .name           = "samsung-adc-v3",
> >                 .driver_data    = TYPE_ADCV3,
> >         }
> >
> > Where TYPE_ADCV3 seems to be the same as the new ADC_V1 used in this
> > driver. Do you have an explanation for that?
> 
> As per suggestion from Doug Anderson,
> I've implemented IIO based ADC driver to work with Exynos5250.
> keeping the plat-samsung/adc.c unchanged.
> 
> Assuming Exynos5250 is the one using the driver for the first time.
> i've named it v1 and so on.
> 
> Now, This seems to cause a lot of confusion.

Ah, so the version numbers don't come from Samsung hardware
documents but are just counting the versions we have drivers
for?

In this case, I guess using the first SoC that had a particular
version would have been better, and we should probably do that
when we add support for the older hardware in this driver.

	Arnd

[PATCHv6 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Chanwoo Choi]

This patch control special clock for ADC in Exynos series's FSYS block.
If special clock of ADC is registerd on clock list of common clk framework,
Exynos ADC drvier have to control this clock.

Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
- 'adc' clock: bus clock for ADC

Exynos3250 has additional 'sclk_adc' clock as following:
- 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC

Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
clock in FSYS_BLK.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
---
 drivers/iio/adc/exynos_adc.c | 112 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 101 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 00d67fd..b63e882 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -81,9 +81,11 @@
 
 struct exynos_adc {
 	struct exynos_adc_data	*data;
+	struct device		*dev;
 	void __iomem		*regs;
 	void __iomem		*enable_reg;
 	struct clk		*clk;
+	struct clk		*sclk;
 	unsigned int		irq;
 	struct regulator	*vdd;
 
@@ -95,6 +97,7 @@ struct exynos_adc {
 
 struct exynos_adc_data {
 	int num_channels;
+	bool needs_sclk;
 
 	void (*init_hw)(struct exynos_adc *info);
 	void (*exit_hw)(struct exynos_adc *info);
@@ -102,6 +105,66 @@ struct exynos_adc_data {
 	void (*start_conv)(struct exynos_adc *info, unsigned long addr);
 };
 
+static void exynos_adc_unprepare_clk(struct exynos_adc *info)
+{
+	if (info->data->needs_sclk)
+		clk_unprepare(info->sclk);
+	clk_unprepare(info->clk);
+}
+
+static int exynos_adc_prepare_clk(struct exynos_adc *info)
+{
+	int ret;
+
+	ret = clk_prepare(info->clk);
+	if (ret) {
+		dev_err(info->dev, "failed preparing adc clock: %d\n", ret);
+		return ret;
+	}
+
+	if (info->data->needs_sclk) {
+		ret = clk_prepare(info->sclk);
+		if (ret) {
+			clk_unprepare(info->clk);
+			dev_err(info->dev,
+				"failed preparing sclk_adc clock: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void exynos_adc_disable_clk(struct exynos_adc *info)
+{
+	if (info->data->needs_sclk)
+		clk_disable(info->sclk);
+	clk_disable(info->clk);
+}
+
+static int exynos_adc_enable_clk(struct exynos_adc *info)
+{
+	int ret;
+
+	ret = clk_enable(info->clk);
+	if (ret) {
+		dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
+		return ret;
+	}
+
+	if (info->data->needs_sclk) {
+		ret = clk_enable(info->sclk);
+		if (ret) {
+			clk_disable(info->clk);
+			dev_err(info->dev,
+				"failed enabling sclk_adc clock: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static void exynos_adc_v1_init_hw(struct exynos_adc *info)
 {
 	u32 con1;
@@ -199,13 +262,20 @@ static void exynos_adc_v2_start_conv(struct exynos_adc *info,
 	writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs));
 }
 
+#define __EXYNOS_ADC_V2_DATA				\
+	.num_channels	= MAX_ADC_V2_CHANNELS,		\
+	.init_hw	= exynos_adc_v2_init_hw,	\
+	.exit_hw	= exynos_adc_v2_exit_hw,	\
+	.clear_irq	= exynos_adc_v2_clear_irq,	\
+	.start_conv	= exynos_adc_v2_start_conv,	\
+
 static struct exynos_adc_data const exynos_adc_v2_data = {
-	.num_channels	= MAX_ADC_V2_CHANNELS,
+	__EXYNOS_ADC_V2_DATA
+};
 
-	.init_hw	= exynos_adc_v2_init_hw,
-	.exit_hw	= exynos_adc_v2_exit_hw,
-	.clear_irq	= exynos_adc_v2_clear_irq,
-	.start_conv	= exynos_adc_v2_start_conv,
+static struct exynos_adc_data const exynos3250_adc_v2_data = {
+	__EXYNOS_ADC_V2_DATA
+	.needs_sclk = true,
 };
 
 static const struct of_device_id exynos_adc_match[] = {
@@ -215,6 +285,9 @@ static const struct of_device_id exynos_adc_match[] = {
 	}, {
 		.compatible = "samsung,exynos-adc-v2",
 		.data = (void *)&exynos_adc_v2_data,
+	}, {
+		.compatible = "samsung,exynos3250-adc-v2",
+		.data = (void *)&exynos3250_adc_v2_data,
 	},
 	{},
 };
@@ -376,6 +449,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	}
 
 	info->irq = irq;
+	info->dev = &pdev->dev;
 
 	init_completion(&info->completion);
 
@@ -386,6 +460,16 @@ static int exynos_adc_probe(struct platform_device *pdev)
 		return PTR_ERR(info->clk);
 	}
 
+	if (info->data->needs_sclk) {
+		info->sclk = devm_clk_get(&pdev->dev, "sclk_adc");
+		if (IS_ERR(info->sclk)) {
+			dev_err(&pdev->dev,
+				"failed getting sclk_adc, err = %ld\n",
+				PTR_ERR(info->sclk));
+			return PTR_ERR(info->sclk);
+		}
+	}
+
 	info->vdd = devm_regulator_get(&pdev->dev, "vdd");
 	if (IS_ERR(info->vdd)) {
 		dev_err(&pdev->dev, "failed getting regulator, err = %ld\n",
@@ -397,10 +481,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(info->clk);
+	ret = exynos_adc_prepare_clk(info);
 	if (ret)
 		goto err_disable_reg;
 
+	ret = exynos_adc_enable_clk(info);
+	if (ret)
+		goto err_unprepare_clk;
+
 	platform_set_drvdata(pdev, indio_dev);
 
 	indio_dev->name = dev_name(&pdev->dev);
@@ -443,7 +531,9 @@ err_irq:
 err_disable_clk:
 	if (info->data->exit_hw)
 		info->data->exit_hw(info);
-	clk_disable_unprepare(info->clk);
+	exynos_adc_disable_clk(info);
+err_unprepare_clk:
+	exynos_adc_unprepare_clk(info);
 err_disable_reg:
 	regulator_disable(info->vdd);
 	return ret;
@@ -460,7 +550,8 @@ static int exynos_adc_remove(struct platform_device *pdev)
 	free_irq(info->irq, info);
 	if (info->data->exit_hw)
 		info->data->exit_hw(info);
-	clk_disable_unprepare(info->clk);
+	exynos_adc_disable_clk(info);
+	exynos_adc_unprepare_clk(info);
 	regulator_disable(info->vdd);
 
 	return 0;
@@ -474,8 +565,7 @@ static int exynos_adc_suspend(struct device *dev)
 
 	if (info->data->exit_hw)
 		info->data->exit_hw(info);
-
-	clk_disable_unprepare(info->clk);
+	exynos_adc_disable_clk(info);
 	regulator_disable(info->vdd);
 
 	return 0;
@@ -491,7 +581,7 @@ static int exynos_adc_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(info->clk);
+	ret = exynos_adc_enable_clk(info);
 	if (ret)
 		return ret;
 
-- 
1.8.0

Re: [PATCHv6 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Arnd Bergmann]

On Friday 18 July 2014 14:59:44 Chanwoo Choi wrote:
> This patch control special clock for ADC in Exynos series's FSYS block.
> If special clock of ADC is registerd on clock list of common clk framework,
> Exynos ADC drvier have to control this clock.
> 
> Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
> - 'adc' clock: bus clock for ADC
> 
> Exynos3250 has additional 'sclk_adc' clock as following:
> - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
> 
> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
> clock in FSYS_BLK.

Do you know if any of the older ADC blocks have an "sclk" input as well?

Further, why is it called "sclk_adc" rather than just "sclk"?

> @@ -199,13 +262,20 @@ static void exynos_adc_v2_start_conv(struct exynos_adc *info,
>         writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs));
>  }
>  
> +#define __EXYNOS_ADC_V2_DATA                           \
> +       .num_channels   = MAX_ADC_V2_CHANNELS,          \
> +       .init_hw        = exynos_adc_v2_init_hw,        \
> +       .exit_hw        = exynos_adc_v2_exit_hw,        \
> +       .clear_irq      = exynos_adc_v2_clear_irq,      \
> +       .start_conv     = exynos_adc_v2_start_conv,     \
> +
>  static struct exynos_adc_data const exynos_adc_v2_data = {
> -       .num_channels   = MAX_ADC_V2_CHANNELS,
> +       __EXYNOS_ADC_V2_DATA
> +};
>  
> -       .init_hw        = exynos_adc_v2_init_hw,
> -       .exit_hw        = exynos_adc_v2_exit_hw,
> -       .clear_irq      = exynos_adc_v2_clear_irq,
> -       .start_conv     = exynos_adc_v2_start_conv,
> +static struct exynos_adc_data const exynos3250_adc_v2_data = {
> +       __EXYNOS_ADC_V2_DATA
> +       .needs_sclk = true,
>  };

I think the macro hurts readability. Please just duplicate the definition
here.

>  static const struct of_device_id exynos_adc_match[] = {
> @@ -215,6 +285,9 @@ static const struct of_device_id exynos_adc_match[] = {
>         }, {
>                 .compatible = "samsung,exynos-adc-v2",
>                 .data = (void *)&exynos_adc_v2_data,
> +       }, {
> +               .compatible = "samsung,exynos3250-adc-v2",
> +               .data = (void *)&exynos3250_adc_v2_data,
>         },
>         {},

Remove the '(void *)' cast here and mark the structure as 'const'.
We intentionally use a 'const void *' type here to verify that
the driver doesn't modify the per-device type data at runtime,
which would be bad if you ever have multiple device instances.

	Arnd

Re: [PATCHv6 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Chanwoo Choi]

Hi Arnd,

On 07/18/2014 06:47 PM, Arnd Bergmann wrote:
> On Friday 18 July 2014 14:59:44 Chanwoo Choi wrote:
>> This patch control special clock for ADC in Exynos series's FSYS block.
>> If special clock of ADC is registerd on clock list of common clk framework,
>> Exynos ADC drvier have to control this clock.
>>
>> Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
>> - 'adc' clock: bus clock for ADC
>>
>> Exynos3250 has additional 'sclk_adc' clock as following:
>> - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
>>
>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
>> clock in FSYS_BLK.
> 
> Do you know if any of the older ADC blocks have an "sclk" input as well?

No, I didn't check older ADC blocks. I only checked it on Exynos3250,
Exynos4210/4212/4412, Exynos5250/5420.

> 
> Further, why is it called "sclk_adc" rather than just "sclk"?

The sclk means 'special clock' in Exynos TRM. Exynos SoC has varisou sclk clocks.
'sclk_adc' is only used for ADC IP.

> 
>> @@ -199,13 +262,20 @@ static void exynos_adc_v2_start_conv(struct exynos_adc *info,
>>         writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs));
>>  }
>>  
>> +#define __EXYNOS_ADC_V2_DATA                           \
>> +       .num_channels   = MAX_ADC_V2_CHANNELS,          \
>> +       .init_hw        = exynos_adc_v2_init_hw,        \
>> +       .exit_hw        = exynos_adc_v2_exit_hw,        \
>> +       .clear_irq      = exynos_adc_v2_clear_irq,      \
>> +       .start_conv     = exynos_adc_v2_start_conv,     \
>> +
>>  static struct exynos_adc_data const exynos_adc_v2_data = {
>> -       .num_channels   = MAX_ADC_V2_CHANNELS,
>> +       __EXYNOS_ADC_V2_DATA
>> +};
>>  
>> -       .init_hw        = exynos_adc_v2_init_hw,
>> -       .exit_hw        = exynos_adc_v2_exit_hw,
>> -       .clear_irq      = exynos_adc_v2_clear_irq,
>> -       .start_conv     = exynos_adc_v2_start_conv,
>> +static struct exynos_adc_data const exynos3250_adc_v2_data = {
>> +       __EXYNOS_ADC_V2_DATA
>> +       .needs_sclk = true,
>>  };
> 
> I think the macro hurts readability. Please just duplicate the definition
> here.

OK, I'll fix it.

> 
>>  static const struct of_device_id exynos_adc_match[] = {
>> @@ -215,6 +285,9 @@ static const struct of_device_id exynos_adc_match[] = {
>>         }, {
>>                 .compatible = "samsung,exynos-adc-v2",
>>                 .data = (void *)&exynos_adc_v2_data,
>> +       }, {
>> +               .compatible = "samsung,exynos3250-adc-v2",
>> +               .data = (void *)&exynos3250_adc_v2_data,
>>         },
>>         {},
> 
> Remove the '(void *)' cast here and mark the structure as 'const'.
> We intentionally use a 'const void *' type here to verify that
> the driver doesn't modify the per-device type data at runtime,
> which would be bad if you ever have multiple device instances.

OK, I'll remove it.

Thanks,
Chanwoo Choi

Re: [PATCHv6 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Arnd Bergmann]

On Friday 18 July 2014 19:00:48 Chanwoo Choi wrote:
> On 07/18/2014 06:47 PM, Arnd Bergmann wrote:
> > 
> > Further, why is it called "sclk_adc" rather than just "sclk"?
> 
> The sclk means 'special clock' in Exynos TRM. Exynos SoC has varisou sclk clocks.
> 'sclk_adc' is only used for ADC IP.

But that sounds like sclk_adc is the name of the global name
of the clock signal coming out of the clock controller.

I still think it would be best to name it 'sclk' as the input
for the adc. It shouldn't rely on a particular name of the
clock controller.

	Arnd

Re: [PATCHv6 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Chanwoo Choi]

On Fri, Jul 18, 2014 at 8:14 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 18 July 2014 19:00:48 Chanwoo Choi wrote:
>> On 07/18/2014 06:47 PM, Arnd Bergmann wrote:
>> >
>> > Further, why is it called "sclk_adc" rather than just "sclk"?
>>
>> The sclk means 'special clock' in Exynos TRM. Exynos SoC has varisou sclk clocks.
>> 'sclk_adc' is only used for ADC IP.
>
> But that sounds like sclk_adc is the name of the global name
> of the clock signal coming out of the clock controller.
>
> I still think it would be best to name it 'sclk' as the input
> for the adc. It shouldn't rely on a particular name of the
> clock controller.

I think 'sclk' is too common name. 'sclk' don't include specific device name.
As I know, usual clock name includes the name of IP or includes the
specific meaning for each IP.

In my opinion, I think sclk_adc is better than 'sclk'.

Thanks,
Chanwoo Choi

Re: [PATCHv6 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Arnd Bergmann]

On Saturday 19 July 2014 00:15:35 Chanwoo Choi wrote:
> On Fri, Jul 18, 2014 at 8:14 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 18 July 2014 19:00:48 Chanwoo Choi wrote:
> >> On 07/18/2014 06:47 PM, Arnd Bergmann wrote:
> >> >
> >> > Further, why is it called "sclk_adc" rather than just "sclk"?
> >>
> >> The sclk means 'special clock' in Exynos TRM. Exynos SoC has varisou sclk clocks.
> >> 'sclk_adc' is only used for ADC IP.
> >
> > But that sounds like sclk_adc is the name of the global name
> > of the clock signal coming out of the clock controller.
> >
> > I still think it would be best to name it 'sclk' as the input
> > for the adc. It shouldn't rely on a particular name of the
> > clock controller.
> 
> I think 'sclk' is too common name. 'sclk' don't include specific device name.
> As I know, usual clock name includes the name of IP or includes the
> specific meaning for each IP.

No, normally it does not include the name of the IP, that's my whole point.
Including the name of the IP is completely pointless because that is
implied by the fact that it's being used by that particular IP.

Ideally you would find the data sheet for the ADC IP block and figure
out what this clock is used for, then find the right name for that.
In a lot of cases, we are actually better off not naming the clocks
at all but simply enumerating them if nobody knows what they are good
for. In that case, you would simply have the first clock and the second
clock of the ADC part and enable them both.

	Arnd

Re: [PATCHv6 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Chanwoo Choi]

On Sat, Jul 19, 2014 at 12:23 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 19 July 2014 00:15:35 Chanwoo Choi wrote:
>> On Fri, Jul 18, 2014 at 8:14 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Friday 18 July 2014 19:00:48 Chanwoo Choi wrote:
>> >> On 07/18/2014 06:47 PM, Arnd Bergmann wrote:
>> >> >
>> >> > Further, why is it called "sclk_adc" rather than just "sclk"?
>> >>
>> >> The sclk means 'special clock' in Exynos TRM. Exynos SoC has varisou sclk clocks.
>> >> 'sclk_adc' is only used for ADC IP.
>> >
>> > But that sounds like sclk_adc is the name of the global name
>> > of the clock signal coming out of the clock controller.
>> >
>> > I still think it would be best to name it 'sclk' as the input
>> > for the adc. It shouldn't rely on a particular name of the
>> > clock controller.
>>
>> I think 'sclk' is too common name. 'sclk' don't include specific device name.
>> As I know, usual clock name includes the name of IP or includes the
>> specific meaning for each IP.
>
> No, normally it does not include the name of the IP, that's my whole point.
> Including the name of the IP is completely pointless because that is
> implied by the fact that it's being used by that particular IP.
>
> Ideally you would find the data sheet for the ADC IP block and figure
> out what this clock is used for, then find the right name for that.

I mentioned the meaning of clocks ('adc', 'sclk_adc') as following in
patch description.
But, you think that need the more proper name instead of 'sclk_adc' to mean the
correct operation of 'sclk_adc'. I'll check once again the meaning of
'sclk_adc' in data sheet
and renaming it.

  Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
  - 'adc' clock: bus clock for ADC

  Exynos3250 has additional 'sclk_adc' clock as following:
  - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC

> In a lot of cases, we are actually better off not naming the clocks
> at all but simply enumerating them if nobody knows what they are good
> for. In that case, you would simply have the first clock and the second
> clock of the ADC part and enable them both.

Thanks for your  review.
Chanwoo Choi

Re: [PATCHv6 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Arnd Bergmann]

On Saturday 19 July 2014 01:11:53 Chanwoo Choi wrote:
> 
>   Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
>   - 'adc' clock: bus clock for ADC
> 
>   Exynos3250 has additional 'sclk_adc' clock as following:
>   - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC

The bus clock is often named "apb_pclk", but it's too late to change
that now, and it's not clear if the device is actually on APB.

The problem with "sclk_adc" is that it's very close to the name used
in the clock provider, with is CLK_SCLK_TSADC. Please try to avoid
the ambiguity here and make it clear that they are not referring
to the same thing. "internal" or "special" might be good if you want
to avoid "sclk".

	Arnd

Re: [PATCHv6 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC

[Chanwoo Choi]

On Sat, Jul 19, 2014 at 1:31 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Saturday 19 July 2014 01:11:53 Chanwoo Choi wrote:
>>
>>   Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
>>   - 'adc' clock: bus clock for ADC
>>
>>   Exynos3250 has additional 'sclk_adc' clock as following:
>>   - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
>
> The bus clock is often named "apb_pclk", but it's too late to change
> that now, and it's not clear if the device is actually on APB.
>
> The problem with "sclk_adc" is that it's very close to the name used
> in the clock provider, with is CLK_SCLK_TSADC. Please try to avoid
> the ambiguity here and make it clear that they are not referring
> to the same thing. "internal" or "special" might be good if you want
> to avoid "sclk".

OK, thanks for your feedback.
I'll try to find proper name.

Chanwoo Choi

[PATCHv6 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Chanwoo Choi]

This patch add DT binding documentation for Exynos3250 ADC IP. Exynos3250 has
special clock ('sclk_adc') for ADC which provide clock to internal ADC.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Reviewed-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
Acked-by: Kukjin Kim <kgene.kim@samsung.com>
---
 .../devicetree/bindings/arm/samsung/exynos-adc.txt | 25 ++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
index 832fe8c..26232f9 100644
--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
@@ -14,14 +14,21 @@ Required properties:
 				for exynos4412/5250 controllers.
 			Must be "samsung,exynos-adc-v2" for
 				future controllers.
+			Must be "samsung,exynos3250-adc-v2" for
+				controllers compatible with ADC of Exynos3250.
 - reg:			Contains ADC register address range (base address and
 			length) and the address of the phy enable register.
 - interrupts: 		Contains the interrupt information for the timer. The
 			format is being dependent on which interrupt controller
 			the Samsung device uses.
 - #io-channel-cells = <1>; As ADC has multiple outputs
-- clocks		From common clock binding: handle to adc clock.
-- clock-names		From common clock binding: Shall be "adc".
+- clocks		From common clock bindings: handles to clocks specified
+			in "clock-names" property, in the same order.
+- clock-names		From common clock bindings: list of clock input names
+			used by ADC block:
+			- "adc" : ADC bus clock
+			- "sclk_adc" : ADC special clock (only for Exynos3250
+				       and compatible ADC block)
 - vdd-supply		VDD input supply.
 
 Note: child nodes can be added for auto probing from device tree.
@@ -41,6 +48,20 @@ adc: adc@12D10000 {
 	vdd-supply = <&buck5_reg>;
 };
 
+Example: adding device info in dtsi file for Exynos3250 with additional sclk
+
+adc: adc@126C0000 {
+	compatible = "samsung,exynos3250-adc-v2";
+	reg = <0x126C0000 0x100>, <0x10020718 0x4>;
+	interrupts = <0 137 0>;
+	#io-channel-cells = <1>;
+	io-channel-ranges;
+
+	clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
+	clock-names = "adc", "sclk_adc";
+
+	vdd-supply = <&buck5_reg>;
+};
 
 Example: Adding child nodes in dts file
 
-- 
1.8.0

Re: [PATCHv6 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Arnd Bergmann]

On Friday 18 July 2014 14:59:45 Chanwoo Choi wrote:
>                         Must be "samsung,exynos-adc-v2" for
>                                 future controllers.

It would be good to change 'future controllers' to something else now.
Presumably that word was used before the actual products were announced,
but now they are publically known.

> +                       Must be "samsung,exynos3250-adc-v2" for
> +                               controllers compatible with ADC of Exynos3250.

Doesn't this version have a specific name as well? The ADC block
seems to use version numbers, so better put those in here to avoid
confusion when another Exynos7890 comes out that uses the same
ADC as exynos3250.

	Arnd

Re: [PATCHv6 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Chanwoo Choi]

On Fri, Jul 18, 2014 at 6:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 18 July 2014 14:59:45 Chanwoo Choi wrote:
>>                         Must be "samsung,exynos-adc-v2" for
>>                                 future controllers.
>
> It would be good to change 'future controllers' to something else now.
> Presumably that word was used before the actual products were announced,
> but now they are publically known.
>
>> +                       Must be "samsung,exynos3250-adc-v2" for
>> +                               controllers compatible with ADC of Exynos3250.
>
> Doesn't this version have a specific name as well? The ADC block
> seems to use version numbers, so better put those in here to avoid
> confusion when another Exynos7890 comes out that uses the same
> ADC as exynos3250.

If don't add new compatible including specific exynos version,
I would add new 'adc-needs-sclk' property with existing 'exynos-adc-v2'
compatible name.


Dear Naveen, Tomasz,

If existing exynos-adc driver add just one property for 'sclk_adc'
as following, exynos-adc could not include the exynos version
in compatible name.

I need your opinion about it.

                adc: adc@126C0000 {
                        compatible = "samsung,exynos-adc-v2";
                        reg = <0x126C0000 0x100>, <0x10020718 0x4>;
                        interrupts = <0 137 0>;
                        clock-names = "adc", "sclk_adc";
                        clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
+                        adc-needs-sclk;
                        #io-channel-cells = <1>;
                        io-channel-ranges;
                }

Thanks,
Chanwoo Choi

Re: [PATCHv6 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Arnd Bergmann]

On Saturday 19 July 2014 01:23:15 Chanwoo Choi wrote:
> If don't add new compatible including specific exynos version,
> I would add new 'adc-needs-sclk' property with existing 'exynos-adc-v2'
> compatible name.
> 
> 
> Dear Naveen, Tomasz,
> 
> If existing exynos-adc driver add just one property for 'sclk_adc'
> as following, exynos-adc could not include the exynos version
> in compatible name.
> 
> I need your opinion about it.
> 
>                 adc: adc@126C0000 {
>                         compatible = "samsung,exynos-adc-v2";
>                         reg = <0x126C0000 0x100>, <0x10020718 0x4>;
>                         interrupts = <0 137 0>;
>                         clock-names = "adc", "sclk_adc";
>                         clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
> +                        adc-needs-sclk;
>                         #io-channel-cells = <1>;
>                         io-channel-ranges;
>                 }

How about just making it an optional clock? That would be much
easier because then you can simply see if the clock itself is
there and use it, or otherwise ignore it.

	Arnd

Re: [PATCHv6 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Chanwoo Choi]

On Sat, Jul 19, 2014 at 1:33 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 19 July 2014 01:23:15 Chanwoo Choi wrote:
>> If don't add new compatible including specific exynos version,
>> I would add new 'adc-needs-sclk' property with existing 'exynos-adc-v2'
>> compatible name.
>>
>>
>> Dear Naveen, Tomasz,
>>
>> If existing exynos-adc driver add just one property for 'sclk_adc'
>> as following, exynos-adc could not include the exynos version
>> in compatible name.
>>
>> I need your opinion about it.
>>
>>                 adc: adc@126C0000 {
>>                         compatible = "samsung,exynos-adc-v2";
>>                         reg = <0x126C0000 0x100>, <0x10020718 0x4>;
>>                         interrupts = <0 137 0>;
>>                         clock-names = "adc", "sclk_adc";
>>                         clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
>> +                        adc-needs-sclk;
>>                         #io-channel-cells = <1>;
>>                         io-channel-ranges;
>>                 }
>
> How about just making it an optional clock? That would be much
> easier because then you can simply see if the clock itself is
> there and use it, or otherwise ignore it.

The v1 of this patchset[1] got the clock of 'sclk_adc'  but if the dt node
of ADC in dtsi file didn't include 'sclk_adc', print just warning message
without stopping probe as following:

 [1] https://lkml.org/lkml/2014/4/10/710

+       info->sclk = devm_clk_get(&pdev->dev, "sclk_adc");
+       if (IS_ERR(info->sclk)) {
+               dev_warn(&pdev->dev, "failed getting sclk clock, err = %ld\n",
+                                                       PTR_ERR(info->sclk));
+               info->sclk = NULL;
+       }

But, Tomasz Figa suggested the method[2] of this patchset(v6).
 [2] https://lkml.org/lkml/2014/4/11/189

Thanks,
Chanwoo Choi

Re: [PATCHv6 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Arnd Bergmann]

On Saturday 19 July 2014 02:02:09 Chanwoo Choi wrote:
> On Sat, Jul 19, 2014 at 1:33 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Saturday 19 July 2014 01:23:15 Chanwoo Choi wrote:
> >> If don't add new compatible including specific exynos version,
> >> I would add new 'adc-needs-sclk' property with existing 'exynos-adc-v2'
> >> compatible name.

What I actually meant is using compatible="exynos-adc-v2.1" or similar
rather than "exynos3250-adc". However, as you already explained, the
version numbers are apparently just made up, so using "exynos3250-adc"
is actually better here. If a future exynos7890 uses the same clocks
as exynos3250, it can simply use the same "exynos3250-adc" string here.

> >> Dear Naveen, Tomasz,
> >>
> >> If existing exynos-adc driver add just one property for 'sclk_adc'
> >> as following, exynos-adc could not include the exynos version
> >> in compatible name.
> >>
> >> I need your opinion about it.
> >>
> >>                 adc: adc@126C0000 {
> >>                         compatible = "samsung,exynos-adc-v2";
> >>                         reg = <0x126C0000 0x100>, <0x10020718 0x4>;
> >>                         interrupts = <0 137 0>;
> >>                         clock-names = "adc", "sclk_adc";
> >>                         clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
> >> +                        adc-needs-sclk;
> >>                         #io-channel-cells = <1>;
> >>                         io-channel-ranges;
> >>                 }
> >
> > How about just making it an optional clock? That would be much
> > easier because then you can simply see if the clock itself is
> > there and use it, or otherwise ignore it.
> 
> The v1 of this patchset[1] got the clock of 'sclk_adc'  but if the dt node
> of ADC in dtsi file didn't include 'sclk_adc', print just warning message
> without stopping probe as following:
> 
>  [1] https://lkml.org/lkml/2014/4/10/710
> 
> +       info->sclk = devm_clk_get(&pdev->dev, "sclk_adc");
> +       if (IS_ERR(info->sclk)) {
> +               dev_warn(&pdev->dev, "failed getting sclk clock, err = %ld\n",
> +                                                       PTR_ERR(info->sclk));
> +               info->sclk = NULL;
> +       }
> 
> But, Tomasz Figa suggested the method[2] of this patchset(v6).
>  [2] https://lkml.org/lkml/2014/4/11/189

Yes, your current version is certainly better than this, but another way
to address Tomasz' comment would be to change the binding to list the "sclk"
as optional for any device and make the code silently ignore missing sclk
entries, like:


	info->sclk = devm_clk_get(&pdev->dev, "sclk");
	if (IS_ERR(info->sclk)) {
		switch (PTR_ERR(info->sclk)) {
		case -EPROBE_DEFER:
			/* silently return error so we can retry */
			return -EPROBE_DEFER:
		case -ENOENT:
			/* silently ignore missing optional clk */
			info->sclk = NULL;
			break;
		default:
			/* any other error: clk is defined by doesn't work  */
			dev_err(&pdev->dev, "failed getting sclk clock, err = %ld\n",
				PTR_ERR(info->sclk));
			return PTR_ERR(info->sclk));
		}
	}

One more comment about the name: Both in the code you use "sclk" as the
name, so presumably that is the actual name of the clk as known to this
driver, and it makes sense to use clock-names="sclk" as well, if you want
to have any name.

	Arnd

Re: [PATCHv6 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Chanwoo Choi]

On 07/19/2014 03:48 AM, Arnd Bergmann wrote:
> On Saturday 19 July 2014 02:02:09 Chanwoo Choi wrote:
>> On Sat, Jul 19, 2014 at 1:33 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>> On Saturday 19 July 2014 01:23:15 Chanwoo Choi wrote:
>>>> If don't add new compatible including specific exynos version,
>>>> I would add new 'adc-needs-sclk' property with existing 'exynos-adc-v2'
>>>> compatible name.
> 
> What I actually meant is using compatible="exynos-adc-v2.1" or similar
> rather than "exynos3250-adc". However, as you already explained, the
> version numbers are apparently just made up, so using "exynos3250-adc"
> is actually better here. If a future exynos7890 uses the same clocks
> as exynos3250, it can simply use the same "exynos3250-adc" string here.

OK, I'll use "exynos3250-adc" compatible string instead of "exynos3250-adc-v2".

> 
>>>> Dear Naveen, Tomasz,
>>>>
>>>> If existing exynos-adc driver add just one property for 'sclk_adc'
>>>> as following, exynos-adc could not include the exynos version
>>>> in compatible name.
>>>>
>>>> I need your opinion about it.
>>>>
>>>>                 adc: adc@126C0000 {
>>>>                         compatible = "samsung,exynos-adc-v2";
>>>>                         reg = <0x126C0000 0x100>, <0x10020718 0x4>;
>>>>                         interrupts = <0 137 0>;
>>>>                         clock-names = "adc", "sclk_adc";
>>>>                         clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
>>>> +                        adc-needs-sclk;
>>>>                         #io-channel-cells = <1>;
>>>>                         io-channel-ranges;
>>>>                 }
>>>
>>> How about just making it an optional clock? That would be much
>>> easier because then you can simply see if the clock itself is
>>> there and use it, or otherwise ignore it.
>>
>> The v1 of this patchset[1] got the clock of 'sclk_adc'  but if the dt node
>> of ADC in dtsi file didn't include 'sclk_adc', print just warning message
>> without stopping probe as following:
>>
>>  [1] https://lkml.org/lkml/2014/4/10/710
>>
>> +       info->sclk = devm_clk_get(&pdev->dev, "sclk_adc");
>> +       if (IS_ERR(info->sclk)) {
>> +               dev_warn(&pdev->dev, "failed getting sclk clock, err = %ld\n",
>> +                                                       PTR_ERR(info->sclk));
>> +               info->sclk = NULL;
>> +       }
>>
>> But, Tomasz Figa suggested the method[2] of this patchset(v6).
>>  [2] https://lkml.org/lkml/2014/4/11/189
> 
> Yes, your current version is certainly better than this, but another way
> to address Tomasz' comment would be to change the binding to list the "sclk"
> as optional for any device and make the code silently ignore missing sclk
> entries, like:
> 
> 
> 	info->sclk = devm_clk_get(&pdev->dev, "sclk");
> 	if (IS_ERR(info->sclk)) {
> 		switch (PTR_ERR(info->sclk)) {
> 		case -EPROBE_DEFER:
> 			/* silently return error so we can retry */
> 			return -EPROBE_DEFER:
> 		case -ENOENT:
> 			/* silently ignore missing optional clk */
> 			info->sclk = NULL;
> 			break;
> 		default:
> 			/* any other error: clk is defined by doesn't work  */
> 			dev_err(&pdev->dev, "failed getting sclk clock, err = %ld\n",
> 				PTR_ERR(info->sclk));
> 			return PTR_ERR(info->sclk));
> 		}
> 	}

I tested this patch suggested by you.
But, devm_clk_get returns always '-ENOENT' on follwong two cases:

Case 1.
ADC dt node in exynos3250.dtsi don't include 'sclk' clock as following:

                adc: adc@126C0000 {
                        compatible = "samsung,exynos3250-adc";
                        reg = <0x126C0000 0x100>, <0x10020718 0x4>;
                        interrupts = <0 137 0>;
                        clock-names = "adc";
                        clocks = <&cmu CLK_TSADC>;
                        #io-channel-cells = <1>;
                        io-channel-ranges;
		};

Case 2.
ADC dt node in exynos3250.dtsi don't include 'sclk' clock
but, exynos3250 clock controller don't support CLK_SCLK_TSADC clock as following:

                adc: adc@126C0000 {
                        compatible = "samsung,exynos3250-adc";
                        reg = <0x126C0000 0x100>, <0x10020718 0x4>;
                        interrupts = <0 137 0>;
                        clock-names = "adc", "sclk";
                        clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
                        #io-channel-cells = <1>;
                        io-channel-ranges;
		};


So, I think exynos-adc needs to use 'needs_sclk' field suggested by Tomasz Figa.

> 
> One more comment about the name: Both in the code you use "sclk" as the
> name, so presumably that is the actual name of the clk as known to this
> driver, and it makes sense to use clock-names="sclk" as well, if you want
> to have any name.

OK, I'll use 'sclk' clock name for special clock of ADC.

Thanks,
Chanwoo Choi

Re: [PATCHv6 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Arnd Bergmann]

On Monday 21 July 2014 10:52:28 Chanwoo Choi wrote:
> > Yes, your current version is certainly better than this, but another way
> > to address Tomasz' comment would be to change the binding to list the "sclk"
> > as optional for any device and make the code silently ignore missing sclk
> > entries, like:
> > 
> > 
> >       info->sclk = devm_clk_get(&pdev->dev, "sclk");
> >       if (IS_ERR(info->sclk)) {
> >               switch (PTR_ERR(info->sclk)) {
> >               case -EPROBE_DEFER:
> >                       /* silently return error so we can retry */
> >                       return -EPROBE_DEFER:
> >               case -ENOENT:
> >                       /* silently ignore missing optional clk */
> >                       info->sclk = NULL;
> >                       break;
> >               default:
> >                       /* any other error: clk is defined by doesn't work  */
> >                       dev_err(&pdev->dev, "failed getting sclk clock, err = %ld\n",
> >                               PTR_ERR(info->sclk));
> >                       return PTR_ERR(info->sclk));
> >               }
> >       }
> 
> I tested this patch suggested by you.
> But, devm_clk_get returns always '-ENOENT' on follwong two cases:
> 
> Case 1.
> ADC dt node in exynos3250.dtsi don't include 'sclk' clock as following:
> 
>                 adc: adc@126C0000 {
>                         compatible = "samsung,exynos3250-adc";
>                         reg = <0x126C0000 0x100>, <0x10020718 0x4>;
>                         interrupts = <0 137 0>;
>                         clock-names = "adc";
>                         clocks = <&cmu CLK_TSADC>;
>                         #io-channel-cells = <1>;
>                         io-channel-ranges;
>                 };
> 
> Case 2.
> ADC dt node in exynos3250.dtsi don't include 'sclk' clock
> but, exynos3250 clock controller don't support CLK_SCLK_TSADC clock as following:
> 
>                 adc: adc@126C0000 {
>                         compatible = "samsung,exynos3250-adc";
>                         reg = <0x126C0000 0x100>, <0x10020718 0x4>;
>                         interrupts = <0 137 0>;
>                         clock-names = "adc", "sclk";
>                         clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
>                         #io-channel-cells = <1>;
>                         io-channel-ranges;
>                 };

But neither of those cases is actually a correct DT representation for
exynos3250: The first case describes an ADC that doesn't need a second
clock, so if the hardware actually needs it to function, it is clearly
unsufficiently described. The second case is even worse because it refers
to a clock that isn't there. Actually that should probably return a different
error code, but that's a different matter.

> So, I think exynos-adc needs to use 'needs_sclk' field suggested by Tomasz Figa.

I don't mind you adding a field to the data, especially since you already
need to have separate structures to encode the different number of channels.
However, I don't see it as necessary either. What you do here is just
checking the DT representation for correctness and consistency. It's not
harmful but we don't normally do that because passing incorrect DT blobs
can cause an infinite number of other problems that we don't check for.

	Arnd

Re: [PATCHv6 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Tomasz Figa]

On 21.07.2014 10:00, Arnd Bergmann wrote:
> On Monday 21 July 2014 10:52:28 Chanwoo Choi wrote:
>>> Yes, your current version is certainly better than this, but another way
>>> to address Tomasz' comment would be to change the binding to list the "sclk"
>>> as optional for any device and make the code silently ignore missing sclk
>>> entries, like:
>>>
>>>
>>>       info->sclk = devm_clk_get(&pdev->dev, "sclk");
>>>       if (IS_ERR(info->sclk)) {
>>>               switch (PTR_ERR(info->sclk)) {
>>>               case -EPROBE_DEFER:
>>>                       /* silently return error so we can retry */
>>>                       return -EPROBE_DEFER:
>>>               case -ENOENT:
>>>                       /* silently ignore missing optional clk */
>>>                       info->sclk = NULL;
>>>                       break;
>>>               default:
>>>                       /* any other error: clk is defined by doesn't work  */
>>>                       dev_err(&pdev->dev, "failed getting sclk clock, err = %ld\n",
>>>                               PTR_ERR(info->sclk));
>>>                       return PTR_ERR(info->sclk));
>>>               }
>>>       }
>>
>> I tested this patch suggested by you.
>> But, devm_clk_get returns always '-ENOENT' on follwong two cases:
>>
>> Case 1.
>> ADC dt node in exynos3250.dtsi don't include 'sclk' clock as following:
>>
>>                 adc: adc@126C0000 {
>>                         compatible = "samsung,exynos3250-adc";
>>                         reg = <0x126C0000 0x100>, <0x10020718 0x4>;
>>                         interrupts = <0 137 0>;
>>                         clock-names = "adc";
>>                         clocks = <&cmu CLK_TSADC>;
>>                         #io-channel-cells = <1>;
>>                         io-channel-ranges;
>>                 };
>>
>> Case 2.
>> ADC dt node in exynos3250.dtsi don't include 'sclk' clock
>> but, exynos3250 clock controller don't support CLK_SCLK_TSADC clock as following:
>>
>>                 adc: adc@126C0000 {
>>                         compatible = "samsung,exynos3250-adc";
>>                         reg = <0x126C0000 0x100>, <0x10020718 0x4>;
>>                         interrupts = <0 137 0>;
>>                         clock-names = "adc", "sclk";
>>                         clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
>>                         #io-channel-cells = <1>;
>>                         io-channel-ranges;
>>                 };
> 
> But neither of those cases is actually a correct DT representation for
> exynos3250: The first case describes an ADC that doesn't need a second
> clock, so if the hardware actually needs it to function, it is clearly
> unsufficiently described. The second case is even worse because it refers
> to a clock that isn't there. Actually that should probably return a different
> error code, but that's a different matter.
> 
>> So, I think exynos-adc needs to use 'needs_sclk' field suggested by Tomasz Figa.
> 
> I don't mind you adding a field to the data, especially since you already
> need to have separate structures to encode the different number of channels.
> However, I don't see it as necessary either. What you do here is just
> checking the DT representation for correctness and consistency. It's not
> harmful but we don't normally do that because passing incorrect DT blobs
> can cause an infinite number of other problems that we don't check for.

I believe we should be enforcing as much correctness on DT data as
possible without too much burden in the code. Otherwise how we are
supposed to know if an error is caused by wrong/missing data in DT, bug
in the driver or who knows else?

In this case making this clock required depending on compatible string
doesn't seem too bothersome to me.

Best regards,
Tomasz

Re: [PATCHv6 3/4] iio: devicetree: Add DT binding documentation for Exynos3250 ADC

[Arnd Bergmann]

On Monday 21 July 2014 12:38:55 Tomasz Figa wrote:
> 
> I believe we should be enforcing as much correctness on DT data as
> possible without too much burden in the code. Otherwise how we are
> supposed to know if an error is caused by wrong/missing data in DT, bug
> in the driver or who knows else?
> 
> In this case making this clock required depending on compatible string
> doesn't seem too bothersome to me.

It makes some sense given that we already need the separate compatible
string (for the number of channels). Without that part, I'd probably
prefer not having the new string just for the extra clock being required,
that could be handled more easily by just making it an optional clock.

	Arnd

[PATCHv6 4/4] ARM: dts: Fix wrong compatible string for Exynos3250 ADC

[Chanwoo Choi]

This patchset fix wrong compatible string for Exynos3250 ADC. Exynos3250 SoC
need to control only special clock for ADC. Exynos SoC except for Exynos3250
has not included special clock for ADC. The exynos ADC driver can control
special clock if compatible string is 'exynos3250-adc-v2'.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
Acked-by: Kukjin Kim <kgene.kim@samsung.com>
---
 arch/arm/boot/dts/exynos3250.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/exynos3250.dtsi b/arch/arm/boot/dts/exynos3250.dtsi
index 3d3c45b..e039818 100644
--- a/arch/arm/boot/dts/exynos3250.dtsi
+++ b/arch/arm/boot/dts/exynos3250.dtsi
@@ -391,10 +391,10 @@
 		};
 
 		adc: adc@126C0000 {
-			compatible = "samsung,exynos-adc-v3";
+			compatible = "samsung,exynos3250-adc-v2";
 			reg = <0x126C0000 0x100>, <0x10020718 0x4>;
 			interrupts = <0 137 0>;
-			clock-names = "adc", "sclk_tsadc";
+			clock-names = "adc", "sclk_adc";
 			clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
 			#io-channel-cells = <1>;
 			io-channel-ranges;
-- 
1.8.0

Re: [PATCHv6 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean

[Arnd Bergmann]

On Friday 18 July 2014 14:59:42 Chanwoo Choi wrote:
> This patchset support Exynos3250 ADC (Analog Digital Converter) because
> Exynos3250 has additional special clock for ADC IP.
> 

by coincidence, I have just looked at the same driver in order to
add support for s3c24xx and s3c64xx, so we can remove the plat-samsung
adc framework they still use.  Those changes would naturally conflict
with yours, but I think it makes sense for your patches to come first.

I'll comment a bit more on the individual patches. I didn't have
interest in them earlier, so I didn't comment so far.

Another aspect that came up is the touchscreen support. Right now,
there is drivers/input/touchscreen/s3c2410_ts.c which is intimately
tied to arch/arm/plat-samsung/adc.c. The best way forward (as discussed
on IRC as few days ago) seems to be to integrate support for that into
the exynos_adc driver. Can you comment on which parts actually support
touchscreen? Did touchscreen support get dropped in the exynos hardware,
or is the driver just missing at the moment?

	Arnd

Re: [PATCHv6 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean

[Naveen Krishna Ch]

Hello Arnd,

On 18 July 2014 15:08, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 18 July 2014 14:59:42 Chanwoo Choi wrote:
>> This patchset support Exynos3250 ADC (Analog Digital Converter) because
>> Exynos3250 has additional special clock for ADC IP.
>>
>
> by coincidence, I have just looked at the same driver in order to
> add support for s3c24xx and s3c64xx, so we can remove the plat-samsung
> adc framework they still use.  Those changes would naturally conflict
> with yours, but I think it makes sense for your patches to come first.
>
> I'll comment a bit more on the individual patches. I didn't have
> interest in them earlier, so I didn't comment so far.
>
> Another aspect that came up is the touchscreen support. Right now,
> there is drivers/input/touchscreen/s3c2410_ts.c which is intimately
> tied to arch/arm/plat-samsung/adc.c. The best way forward (as discussed
> on IRC as few days ago) seems to be to integrate support for that into
> the exynos_adc driver. Can you comment on which parts actually support
> touchscreen? Did touchscreen support get dropped in the exynos hardware,
> or is the driver just missing at the moment?

SoCs from Samsung before Exynos4 had and ADC based Touch screen
and the driver under arch/arm/plat-samsung/adc.c and
drivers/input/touchscreen/s3c2410_ts.c used to work together.

Infact, the interrupt routine and priorities in plat-samsung/adc.c
driver as written keeping the Touchscreen in mind.

After Exynos4, ADC IP remained. But, the touch screen was removed in hardware.

As per suggestion from Doug Anderson, I've implemented IIO based ADC
driver started to support from exynos5250.

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



-- 
Shine bright,
(: Nav :)

Re: [PATCHv6 0/4] iio: adc: exynos_adc: Support Exynos3250 ADC and code clean

[Naveen Krishna Ch]

Hello All,

On 18 July 2014 15:43, Naveen Krishna Ch <naveenkrishna.ch-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hello Arnd,
>
> On 18 July 2014 15:08, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> On Friday 18 July 2014 14:59:42 Chanwoo Choi wrote:
>>> This patchset support Exynos3250 ADC (Analog Digital Converter) because
>>> Exynos3250 has additional special clock for ADC IP.
>>>
>>
>> by coincidence, I have just looked at the same driver in order to
>> add support for s3c24xx and s3c64xx, so we can remove the plat-samsung
>> adc framework they still use.  Those changes would naturally conflict
>> with yours, but I think it makes sense for your patches to come first.
>>
>> I'll comment a bit more on the individual patches. I didn't have
>> interest in them earlier, so I didn't comment so far.
>>
>> Another aspect that came up is the touchscreen support. Right now,
>> there is drivers/input/touchscreen/s3c2410_ts.c which is intimately
>> tied to arch/arm/plat-samsung/adc.c. The best way forward (as discussed
>> on IRC as few days ago) seems to be to integrate support for that into
>> the exynos_adc driver. Can you comment on which parts actually support
>> touchscreen? Did touchscreen support get dropped in the exynos hardware,
>> or is the driver just missing at the moment?
>
> SoCs from Samsung before Exynos4 had and ADC based Touch screen
> and the driver under arch/arm/plat-samsung/adc.c and
> drivers/input/touchscreen/s3c2410_ts.c used to work together.
>
> Infact, the interrupt routine and priorities in plat-samsung/adc.c
> driver as written keeping the Touchscreen in mind.
>
> After Exynos4, ADC IP remained. But, the touch screen was removed in hardware.

At least on Exynos4210> i'm not sure about the other Exynos4 series.
I will try to verify the user manuals.

>
> As per suggestion from Doug Anderson, I've implemented IIO based ADC
> driver started to support from exynos5250.
>
>>
>>         Arnd
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Shine bright,
> (: Nav :)



-- 
Shine bright,
(: Nav :)

[PATCH 1/2] iio: exynos-adc: add support for s3c64xx adc

[Arnd Bergmann]

The ADC in s3c64xx is almost the same as exynosv1, but
has a different 'select' method. Adding this here will be
helpful to move over the existing s3c64xx platform from the
legacy plat-samsung/adc driver to the new exynos-adc.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
[In reply to Exynos3250 ADC support, adding Heiko and Ben]

I spent way too much time this week trying to clean up the
old plat-samsung/adc.c driver as preparation for s3c64xx multiplatform
support. Eventually I figured out that all that code is much simpler
done using the new driver. This adds support for s3c64xx in
samsung-adc.c, similar code changes can be done to support the
various s3c24xx variants as well.

This first patch should be fairly straightforward but is not tested
yet. The second patch is more tricky.

Both are based on the exynos3250 patches sent by Chanwoo Choi.

diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
index 26232f98d8c5..f84e9250429b 100644
--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
@@ -11,11 +11,21 @@ New driver handles the following
 
 Required properties:
 - compatible:		Must be "samsung,exynos-adc-v1"
-				for exynos4412/5250 controllers.
+				for exynos4412/5250 and s5pv210 controllers.
 			Must be "samsung,exynos-adc-v2" for
 				future controllers.
 			Must be "samsung,exynos3250-adc-v2" for
 				controllers compatible with ADC of Exynos3250.
+			Must be "samsung,s3c2410-adc" for
+				the ADC in s3c2410 and compatibles
+			Must be "samsung,s3c2416-adc" for
+				the ADC in s3c2416 and compatibles
+			Must be "samsung,s3c2440-adc" for
+				the ADC in s3c2440 and compatibles
+			Must be "samsung,s3c2440-adc" for
+				the ADC in s3c2440 and compatibles
+			Must be "samsung,s3c2443-adc" for
+				the ADC in s3c2443 and compatibles
 - reg:			Contains ADC register address range (base address and
 			length) and the address of the phy enable register.
 - interrupts: 		Contains the interrupt information for the timer. The
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index b63e88247eb2..5f95638513d2 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -39,12 +39,16 @@
 #include <linux/iio/machine.h>
 #include <linux/iio/driver.h>
 
-/* EXYNOS4412/5250 ADC_V1 registers definitions */
+/* S3C/EXYNOS4412/5250 ADC_V1 registers definitions */
 #define ADC_V1_CON(x)		((x) + 0x00)
+#define ADC_V1_TSC(x)		((x) + 0x04)
 #define ADC_V1_DLY(x)		((x) + 0x08)
 #define ADC_V1_DATX(x)		((x) + 0x0C)
+#define ADC_V1_DATY(x)		((x) + 0x10)
+#define ADC_V1_UPDN(x)		((x) + 0x14)
 #define ADC_V1_INTCLR(x)	((x) + 0x18)
 #define ADC_V1_MUX(x)		((x) + 0x1c)
+#define ADC_V1_CLRINTPNDNUP(x)	((x) + 0x20)
 
 /* Future ADC_V2 registers definitions */
 #define ADC_V2_CON1(x)		((x) + 0x00)
@@ -60,6 +64,30 @@
 #define ADC_V1_CON_PRSCLV(x)	(((x) & 0xFF) << 6)
 #define ADC_V1_CON_STANDBY	(1u << 2)
 
+#define ADC_S3C2410_CON_SELMUX(x) (((x)&0x7)<<3)
+
+/* ADCTSC Register Bits */
+#define ADC_S3C2443_TSC_UD_SEN		(1<<8)
+#define ADC_S3C2410_TSC_YM_SEN		(1<<7)
+#define ADC_S3C2410_TSC_YP_SEN		(1<<6)
+#define ADC_S3C2410_TSC_XM_SEN		(1<<5)
+#define ADC_S3C2410_TSC_XP_SEN		(1<<4)
+#define ADC_S3C2410_TSC_PULL_UP_DISABLE	(1<<3)
+#define ADC_S3C2410_TSC_AUTO_PST	(1<<2)
+#define ADC_S3C2410_TSC_XY_PST(x)	(((x)&0x3)<<0)
+
+#define ADC_TSC_WAIT4INT (ADC_S3C2410_TSC_YM_SEN | \
+			 ADC_S3C2410_TSC_YP_SEN | \
+			 ADC_S3C2410_TSC_XP_SEN | \
+			 ADC_S3C2410_TSC_XY_PST(3))
+
+#define ADC_TSC_AUTOPST	(ADC_S3C2410_TSC_YM_SEN | \
+			 ADC_S3C2410_TSC_YP_SEN | \
+			 ADC_S3C2410_TSC_XP_SEN | \
+			 ADC_S3C2410_TSC_AUTO_PST | \
+			 ADC_S3C2410_TSC_XY_PST(0))
+
+
 /* Bit definitions for ADC_V2 */
 #define ADC_V2_CON1_SOFT_RESET	(1u << 2)
 
@@ -195,6 +223,26 @@ static void exynos_adc_v1_clear_irq(struct exynos_adc *info)
 	writel(1, ADC_V1_INTCLR(info->regs));
 }
 
+static void exynos_adc_s3c64xx_start_conv(struct exynos_adc *info,
+				     unsigned long addr)
+{
+	u32 con1;
+
+	con1 = readl(ADC_V1_CON(info->regs));
+	con1 &= ~ADC_S3C2410_CON_SELMUX(7);
+	con1 |= ADC_S3C2410_CON_SELMUX(addr);
+	writel(con1 | ADC_CON_EN_START, ADC_V1_CON(info->regs));
+}
+
+static struct exynos_adc_data const exynos_adc_s3c64xx_data = {
+	.num_channels	= MAX_ADC_V1_CHANNELS,
+
+	.init_hw	= exynos_adc_v1_init_hw,
+	.exit_hw	= exynos_adc_v1_exit_hw,
+	.clear_irq	= exynos_adc_v1_clear_irq,
+	.start_conv	= exynos_adc_s3c64xx_start_conv,
+};
+
 static void exynos_adc_v1_start_conv(struct exynos_adc *info,
 				     unsigned long addr)
 {
@@ -280,6 +328,9 @@ static struct exynos_adc_data const exynos3250_adc_v2_data = {
 
 static const struct of_device_id exynos_adc_match[] = {
 	{
+		.compatible = "samsung,s3c64100-adc",
+		.data = &exynos_adc_s3c64xx_data,
+	}, {
 		.compatible = "samsung,exynos-adc-v1",
 		.data = (void *)&exynos_adc_v1_data,
 	}, {

[PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Arnd Bergmann]

This adds support for the touchscreen on Samsung s3c64xx.
The driver is completely untested but shows roughly how
it could be done, following the example of the at91 driver.

Open questions include:

- compared to the old plat-samsung/adc driver, there is
  no support for prioritizing ts over other clients, nor
  for oversampling. From my reading of the code, the
  priorities didn't actually have any effect at all, but
  the oversampling might be needed. Maybe the original
  authors have some insight.

- I simply register the input device from the adc driver
  itself, as the at91 code does. The driver also supports
  sub-nodes, but I don't understand how they are meant
  to be used, so using those might be better.

- The new exynos_read_s3c64xx_ts() function is intentionally
  very similar to the existing exynos_read_raw() functions.
  It should probably be changed, either by merging the two
  into one, or by simplifying the exynos_read_s3c64xx_ts()
  function. This depends a bit on the answers to the questions
  above.

- We probably need to add support for platform_data as well,
  I've skipped this so far.

- Is anybody able to debug this driver on real hardware?
  While it's possible that it actually works, it's more
  likely that I made a few subtle mistakes.

Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
index e1b74828f413..4329bf3c3326 100644
--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
@@ -41,6 +41,10 @@ Required properties:
 				       and compatible ADC block)
 - vdd-supply		VDD input supply.
 
+Optional properties:
+- has-touchscreen:	If present, indicates that a touchscreen is
+			connected an usable.
+
 Note: child nodes can be added for auto probing from device tree.
 
 Example: adding device info in dtsi file
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 5f95638513d2..cf1d9f3e2492 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -34,6 +34,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/of_platform.h>
 #include <linux/err.h>
+#include <linux/input.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/machine.h>
@@ -103,6 +104,7 @@
 
 /* Bit definitions common for ADC_V1 and ADC_V2 */
 #define ADC_CON_EN_START	(1u << 0)
+#define ADC_DATX_PRESSED	(1u << 15)
 #define ADC_DATX_MASK		0xFFF
 
 #define EXYNOS_ADC_TIMEOUT	(msecs_to_jiffies(100))
@@ -110,16 +112,20 @@
 struct exynos_adc {
 	struct exynos_adc_data	*data;
 	struct device		*dev;
+	struct input_dev	*input;
 	void __iomem		*regs;
 	void __iomem		*enable_reg;
 	struct clk		*clk;
 	struct clk		*sclk;
 	unsigned int		irq;
+	unsigned int		tsirq;
 	struct regulator	*vdd;
 
 	struct completion	completion;
 
+	bool			read_ts;
 	u32			value;
+	u32			value2;
 	unsigned int            version;
 };
 
@@ -390,12 +396,61 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val,
+				int *val2,
+				long mask)
+{
+	struct exynos_adc *info = iio_priv(indio_dev);
+	unsigned long timeout;
+	int ret;
+
+	if (mask != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	mutex_lock(&indio_dev->mlock);
+	info->read_ts = 1;
+
+	reinit_completion(&info->completion);
+
+	writel(ADC_S3C2410_TSC_PULL_UP_DISABLE | ADC_TSC_AUTOPST,
+	       ADC_V1_TSC(info->regs));
+
+	/* Select the ts channel to be used and Trigger conversion */
+	info->data->start_conv(info, 0);
+
+	timeout = wait_for_completion_timeout
+			(&info->completion, EXYNOS_ADC_TIMEOUT);
+	if (timeout == 0) {
+		dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
+		if (info->data->init_hw)
+			info->data->init_hw(info);
+		ret = -ETIMEDOUT;
+	} else {
+		*val = info->value;
+		*val2 = info->value2;
+		ret = IIO_VAL_INT;
+	}
+
+	info->read_ts = 0;
+	mutex_unlock(&indio_dev->mlock);
+
+	return ret;
+}
+
 static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
 {
 	struct exynos_adc *info = (struct exynos_adc *)dev_id;
 
 	/* Read value */
-	info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
+	if (info->read_ts) {
+		info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
+		info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
+		writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
+	} else {
+		info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
+	}
 
 	/* clear irq */
 	if (info->data->clear_irq)
@@ -406,6 +461,46 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/*
+ * Here we (ab)use a threaded interrupt handler to stay running
+ * for as long as the touchscreen remains pressed, we report
+ * a new event with the latest data and then sleep until the
+ * next timer tick. This mirrors the behavior of the old
+ * driver, with much less code.
+ */
+static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
+{
+	struct exynos_adc *info = dev_id;
+	struct iio_dev *dev = dev_get_drvdata(info->dev);
+	u32 x, y;
+	bool pressed;
+	int ret;
+
+	do {
+		ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
+		if (ret == -ETIMEDOUT)
+			break;
+
+		pressed = x & y & ADC_DATX_PRESSED;
+		if (!pressed)
+			break;
+
+		input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
+		input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
+		input_report_key(info->input, BTN_TOUCH, 1);
+		input_sync(info->input);
+
+		msleep(1);
+	} while (1);
+
+	input_report_key(info->input, BTN_TOUCH, 0);
+	input_sync(info->input);
+
+	writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
+
+	return IRQ_HANDLED;
+}
+
 static int exynos_adc_reg_access(struct iio_dev *indio_dev,
 			      unsigned reg, unsigned writeval,
 			      unsigned *readval)
@@ -457,12 +552,57 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
 	return 0;
 }
 
+static int exynos_adc_ts_init(struct exynos_adc *info)
+{
+	int ret;
+
+	info->input = input_allocate_device();
+	if (!info->input)
+		return -ENOMEM;
+
+	info->input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	info->input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+
+	input_set_abs_params(info->input, ABS_X, 0, 0x3FF, 0, 0);
+	input_set_abs_params(info->input, ABS_Y, 0, 0x3FF, 0, 0);
+
+	/* data from s3c2410_ts driver */
+	info->input->name = "S3C24xx TouchScreen";
+	info->input->id.bustype = BUS_HOST;
+	info->input->id.vendor = 0xDEAD;
+	info->input->id.product = 0xBEEF;
+	info->input->id.version = 0x0200;
+
+	ret = input_register_device(info->input);
+	if (ret) {
+		input_free_device(info->input);
+		goto err;
+	}
+
+	if (info->tsirq > 0)
+		ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
+					0, "touchscreen", info);
+	if (ret < 0) {
+		dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
+							info->irq);
+		goto err_input;
+	}
+
+	return 0;
+
+err_input:
+	input_unregister_device(info->input);
+err:
+	return ret;
+}
+
 static int exynos_adc_probe(struct platform_device *pdev)
 {
 	struct exynos_adc *info = NULL;
 	struct device_node *np = pdev->dev.of_node;
 	struct iio_dev *indio_dev = NULL;
 	struct resource	*mem;
+	bool has_ts;
 	int ret = -ENODEV;
 	int irq;
 
@@ -498,8 +638,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "no irq resource?\n");
 		return irq;
 	}
-
 	info->irq = irq;
+
+	irq = platform_get_irq(pdev, 1);
+	if (irq == -EPROBE_DEFER)
+		return irq; 
+
+	info->tsirq = irq;
+
 	info->dev = &pdev->dev;
 
 	init_completion(&info->completion);
@@ -565,6 +711,12 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	if (info->data->init_hw)
 		info->data->init_hw(info);
 
+	has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
+	if (has_ts)
+		ret = exynos_adc_ts_init(info);
+	if (ret)
+		goto err_iio;
+
 	ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed adding child nodes\n");
@@ -576,6 +728,11 @@ static int exynos_adc_probe(struct platform_device *pdev)
 err_of_populate:
 	device_for_each_child(&indio_dev->dev, NULL,
 				exynos_adc_remove_devices);
+	if (has_ts) {
+		input_unregister_device(info->input);
+		free_irq(info->tsirq, info);
+	}
+err_iio:
 	iio_device_unregister(indio_dev);
 err_irq:
 	free_irq(info->irq, info);
@@ -595,9 +752,12 @@ static int exynos_adc_remove(struct platform_device *pdev)
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct exynos_adc *info = iio_priv(indio_dev);
 
+	input_free_device(info->input);	
 	device_for_each_child(&indio_dev->dev, NULL,
 				exynos_adc_remove_devices);
 	iio_device_unregister(indio_dev);
+	if (info->tsirq > 0)
+		free_irq(info->tsirq, info);
 	free_irq(info->irq, info);
 	if (info->data->exit_hw)
 		info->data->exit_hw(info);

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Jonathan Cameron]

On 18/07/14 20:29, Arnd Bergmann wrote:
> This adds support for the touchscreen on Samsung s3c64xx.
> The driver is completely untested but shows roughly how
> it could be done, following the example of the at91 driver.
>
Hi Arnd,

> Open questions include:
>
> - compared to the old plat-samsung/adc driver, there is
>    no support for prioritizing ts over other clients, nor
>    for oversampling. From my reading of the code, the
>    priorities didn't actually have any effect at all, but
>    the oversampling might be needed. Maybe the original
>    authors have some insight.
>
> - I simply register the input device from the adc driver
>    itself, as the at91 code does. The driver also supports
>    sub-nodes, but I don't understand how they are meant
>    to be used, so using those might be better.
So, the alternative you are (I think) referring to is to use
the buffered in kernel client interface.  That way a separate
touch screen driver can use the output channels provided by IIO
in much the same way you might use a regulator or similar.
Note that whilst this is similar to the simple polled interface
used for things like the iio_hwmon driver, the data flow is
quite different (clearly the polled interfce would be
inappropriate here).

Whilst we've discussed it in the past for touch screen drivers
like this, usually the hardware isn't generic enough to be
of any real use if not being used as a touch screen.  As such
it's often simpler to just have the support directly in the
driver (as you've observed the at91 driver does this).

Whilst the interface has been there a while, it's not really had
all that much use.  The original target was the simpler case
of 3D accelerometer where we have a generic iio to input
bridge driver. Time constraints meant that I haven't yet actually
formally submitted the input side of this. Whilst there are lots
of other things that can use this interface, right now nothing
actually does so.

>
> - The new exynos_read_s3c64xx_ts() function is intentionally
>    very similar to the existing exynos_read_raw() functions.
>    It should probably be changed, either by merging the two
>    into one, or by simplifying the exynos_read_s3c64xx_ts()
>    function. This depends a bit on the answers to the questions
>    above.
I'd be tempted to not bother keeping them that similar.  It's
not a generic IIO channel so simplify it where possible.
>
> - We probably need to add support for platform_data as well,
>    I've skipped this so far.
>
> - Is anybody able to debug this driver on real hardware?
>    While it's possible that it actually works, it's more
>    likely that I made a few subtle mistakes.
>
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Looks pretty good to me.  A few symantic bits and pieces and
one bug spotted.  Short and sweet.
>
> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> index e1b74828f413..4329bf3c3326 100644
> --- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> @@ -41,6 +41,10 @@ Required properties:
>   				       and compatible ADC block)
>   - vdd-supply		VDD input supply.
>
> +Optional properties:
> +- has-touchscreen:	If present, indicates that a touchscreen is
> +			connected an usable.
> +
>   Note: child nodes can be added for auto probing from device tree.
>
>   Example: adding device info in dtsi file
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index 5f95638513d2..cf1d9f3e2492 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -34,6 +34,7 @@
>   #include <linux/regulator/consumer.h>
>   #include <linux/of_platform.h>
>   #include <linux/err.h>
> +#include <linux/input.h>
Might want to make the input side optional at compile time...
I supose the existing parts are unlikely to be used much in headless
devices, but you never know.  Maybe we just leave this until someone
shouts they want to be able to avoid compiling it in.
>
>   #include <linux/iio/iio.h>
>   #include <linux/iio/machine.h>
> @@ -103,6 +104,7 @@
>
>   /* Bit definitions common for ADC_V1 and ADC_V2 */
>   #define ADC_CON_EN_START	(1u << 0)
> +#define ADC_DATX_PRESSED	(1u << 15)
>   #define ADC_DATX_MASK		0xFFF
>
>   #define EXYNOS_ADC_TIMEOUT	(msecs_to_jiffies(100))
> @@ -110,16 +112,20 @@
>   struct exynos_adc {
>   	struct exynos_adc_data	*data;
>   	struct device		*dev;
> +	struct input_dev	*input;
>   	void __iomem		*regs;
>   	void __iomem		*enable_reg;
>   	struct clk		*clk;
>   	struct clk		*sclk;
>   	unsigned int		irq;
> +	unsigned int		tsirq;
>   	struct regulator	*vdd;
>
>   	struct completion	completion;
>
> +	bool			read_ts;
>   	u32			value;
> +	u32			value2;
As I state further down, I'd rather keep a little
clear of the naming used in IIO for bits that aren't
going through IIO (less confusing!). Maybe just
have
	u32 x, y;
>   	unsigned int            version;
>   };
>
> @@ -390,12 +396,61 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>   	return ret;
>   }
>
> +static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val,
> +				int *val2,
> +				long mask)
> +{
> +	struct exynos_adc *info = iio_priv(indio_dev);
> +	unsigned long timeout;
> +	int ret;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	mutex_lock(&indio_dev->mlock);
> +	info->read_ts = 1;
> +
> +	reinit_completion(&info->completion);
> +
> +	writel(ADC_S3C2410_TSC_PULL_UP_DISABLE | ADC_TSC_AUTOPST,
> +	       ADC_V1_TSC(info->regs));
> +
> +	/* Select the ts channel to be used and Trigger conversion */
> +	info->data->start_conv(info, 0);
0 is a rather magic value.  A define perhaps?
> +
> +	timeout = wait_for_completion_timeout
> +			(&info->completion, EXYNOS_ADC_TIMEOUT);
> +	if (timeout == 0) {
> +		dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
> +		if (info->data->init_hw)
> +			info->data->init_hw(info);
> +		ret = -ETIMEDOUT;
> +	} else {
> +		*val = info->value;
> +		*val2 = info->value2;
This is definitely abuse as those two values are not intended for
different values.  If you want to do this please use different naming
and don't try to fiddle it into the IIO read raw framework.
As you've suggested above, better to simplify this code and drop the
bits cloned from the other handler.
> +		ret = IIO_VAL_INT;
> +	}
> +
> +	info->read_ts = 0;
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	return ret;
> +}
> +
>   static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>   {
>   	struct exynos_adc *info = (struct exynos_adc *)dev_id;
>
>   	/* Read value */
> -	info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> +	if (info->read_ts) {
> +		info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> +		info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
ADC_DATY_MASK would be more obviously correct.

> +		writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
Perhaps the above is cryptic enough to warrant a comment?
> +	} else {
> +		info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> +	}
>
>   	/* clear irq */
>   	if (info->data->clear_irq)
> @@ -406,6 +461,46 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>   	return IRQ_HANDLED;
>   }
>
> +/*
> + * Here we (ab)use a threaded interrupt handler to stay running
> + * for as long as the touchscreen remains pressed, we report
> + * a new event with the latest data and then sleep until the
> + * next timer tick. This mirrors the behavior of the old
> + * driver, with much less code.
> + */
> +static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> +{
> +	struct exynos_adc *info = dev_id;
> +	struct iio_dev *dev = dev_get_drvdata(info->dev);
> +	u32 x, y;
> +	bool pressed;
> +	int ret;
> +
> +	do {
> +		ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
= exynos
> +		if (ret == -ETIMEDOUT)
> +			break;
> +
> +		pressed = x & y & ADC_DATX_PRESSED;
> +		if (!pressed)
> +			break;
> +
> +		input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
> +		input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
> +		input_report_key(info->input, BTN_TOUCH, 1);
> +		input_sync(info->input);
> +
> +		msleep(1);
> +	} while (1);
> +
> +	input_report_key(info->input, BTN_TOUCH, 0);
> +	input_sync(info->input);
> +
> +	writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
> +
> +	return IRQ_HANDLED;
> +}
> +
>   static int exynos_adc_reg_access(struct iio_dev *indio_dev,
>   			      unsigned reg, unsigned writeval,
>   			      unsigned *readval)
> @@ -457,12 +552,57 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
>   	return 0;
>   }
>
> +static int exynos_adc_ts_init(struct exynos_adc *info)
> +{
> +	int ret;
> +
> +	info->input = input_allocate_device();
> +	if (!info->input)
> +		return -ENOMEM;
> +
> +	info->input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	info->input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +
> +	input_set_abs_params(info->input, ABS_X, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(info->input, ABS_Y, 0, 0x3FF, 0, 0);
> +
> +	/* data from s3c2410_ts driver */
> +	info->input->name = "S3C24xx TouchScreen";
> +	info->input->id.bustype = BUS_HOST;
> +	info->input->id.vendor = 0xDEAD;
> +	info->input->id.product = 0xBEEF;
> +	info->input->id.version = 0x0200;
> +
> +	ret = input_register_device(info->input);
> +	if (ret) {
> +		input_free_device(info->input);
> +		goto err;
> +	}
> +
> +	if (info->tsirq > 0)
> +		ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
> +					0, "touchscreen", info);
info->tsirq
(that had me really confused for a moment ;)
Also, perhaps a more specific name.  touchscreen_updown or similar as the
main interrupt is also used during touchscreen operation.
> +	if (ret < 0) {
> +		dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
> +							info->irq);
> +		goto err_input;
> +	}
> +
> +	return 0;
> +
> +err_input:
> +	input_unregister_device(info->input);
> +err:
> +	return ret;
> +}
> +
>   static int exynos_adc_probe(struct platform_device *pdev)
>   {
>   	struct exynos_adc *info = NULL;
>   	struct device_node *np = pdev->dev.of_node;
>   	struct iio_dev *indio_dev = NULL;
>   	struct resource	*mem;
> +	bool has_ts;
>   	int ret = -ENODEV;
>   	int irq;
>
> @@ -498,8 +638,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
>   		dev_err(&pdev->dev, "no irq resource?\n");
>   		return irq;
>   	}
> -
>   	info->irq = irq;
> +
> +	irq = platform_get_irq(pdev, 1);
> +	if (irq == -EPROBE_DEFER)
> +		return irq;
> +
> +	info->tsirq = irq;
> +
>   	info->dev = &pdev->dev;
>
>   	init_completion(&info->completion);
> @@ -565,6 +711,12 @@ static int exynos_adc_probe(struct platform_device *pdev)
>   	if (info->data->init_hw)
>   		info->data->init_hw(info);
>
> +	has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
> +	if (has_ts)
> +		ret = exynos_adc_ts_init(info);
> +	if (ret)
> +		goto err_iio;
> +
>   	ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
>   	if (ret < 0) {
>   		dev_err(&pdev->dev, "failed adding child nodes\n");
> @@ -576,6 +728,11 @@ static int exynos_adc_probe(struct platform_device *pdev)
>   err_of_populate:
>   	device_for_each_child(&indio_dev->dev, NULL,
>   				exynos_adc_remove_devices);
> +	if (has_ts) {
> +		input_unregister_device(info->input);
> +		free_irq(info->tsirq, info);
> +	}
> +err_iio:
>   	iio_device_unregister(indio_dev);
>   err_irq:
>   	free_irq(info->irq, info);
> @@ -595,9 +752,12 @@ static int exynos_adc_remove(struct platform_device *pdev)
>   	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>   	struct exynos_adc *info = iio_priv(indio_dev);
>
> +	input_free_device(info->input);	
>   	device_for_each_child(&indio_dev->dev, NULL,
>   				exynos_adc_remove_devices);
>   	iio_device_unregister(indio_dev);
> +	if (info->tsirq > 0)
> +		free_irq(info->tsirq, info);
>   	free_irq(info->irq, info);
>   	if (info->data->exit_hw)
>   		info->data->exit_hw(info);
>

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Jonathan Cameron]

On 20/07/14 14:49, Jonathan Cameron wrote:
> On 18/07/14 20:29, Arnd Bergmann wrote:
>> This adds support for the touchscreen on Samsung s3c64xx.
>> The driver is completely untested but shows roughly how
>> it could be done, following the example of the at91 driver.
>>
> Hi Arnd,
Cc'd linux-input and Dmitry.
>
>> Open questions include:
>>
>> - compared to the old plat-samsung/adc driver, there is
>>    no support for prioritizing ts over other clients, nor
>>    for oversampling. From my reading of the code, the
>>    priorities didn't actually have any effect at all, but
>>    the oversampling might be needed. Maybe the original
>>    authors have some insight.
>>
>> - I simply register the input device from the adc driver
>>    itself, as the at91 code does. The driver also supports
>>    sub-nodes, but I don't understand how they are meant
>>    to be used, so using those might be better.
> So, the alternative you are (I think) referring to is to use
> the buffered in kernel client interface.  That way a separate
> touch screen driver can use the output channels provided by IIO
> in much the same way you might use a regulator or similar.
> Note that whilst this is similar to the simple polled interface
> used for things like the iio_hwmon driver, the data flow is
> quite different (clearly the polled interfce would be
> inappropriate here).
>
> Whilst we've discussed it in the past for touch screen drivers
> like this, usually the hardware isn't generic enough to be
> of any real use if not being used as a touch screen.  As such
> it's often simpler to just have the support directly in the
> driver (as you've observed the at91 driver does this).
>
> Whilst the interface has been there a while, it's not really had
> all that much use.  The original target was the simpler case
> of 3D accelerometer where we have a generic iio to input
> bridge driver. Time constraints meant that I haven't yet actually
> formally submitted the input side of this. Whilst there are lots
> of other things that can use this interface, right now nothing
> actually does so.
>
>>
>> - The new exynos_read_s3c64xx_ts() function is intentionally
>>    very similar to the existing exynos_read_raw() functions.
>>    It should probably be changed, either by merging the two
>>    into one, or by simplifying the exynos_read_s3c64xx_ts()
>>    function. This depends a bit on the answers to the questions
>>    above.
> I'd be tempted to not bother keeping them that similar.  It's
> not a generic IIO channel so simplify it where possible.
>>
>> - We probably need to add support for platform_data as well,
>>    I've skipped this so far.
>>
>> - Is anybody able to debug this driver on real hardware?
>>    While it's possible that it actually works, it's more
>>    likely that I made a few subtle mistakes.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Looks pretty good to me.  A few symantic bits and pieces and
> one bug spotted.  Short and sweet.
>>
>> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> index e1b74828f413..4329bf3c3326 100644
>> --- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> @@ -41,6 +41,10 @@ Required properties:
>>                          and compatible ADC block)
>>   - vdd-supply        VDD input supply.
>>
>> +Optional properties:
>> +- has-touchscreen:    If present, indicates that a touchscreen is
>> +            connected an usable.
>> +
>>   Note: child nodes can be added for auto probing from device tree.
>>
>>   Example: adding device info in dtsi file
>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>> index 5f95638513d2..cf1d9f3e2492 100644
>> --- a/drivers/iio/adc/exynos_adc.c
>> +++ b/drivers/iio/adc/exynos_adc.c
>> @@ -34,6 +34,7 @@
>>   #include <linux/regulator/consumer.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/err.h>
>> +#include <linux/input.h>
> Might want to make the input side optional at compile time...
> I supose the existing parts are unlikely to be used much in headless
> devices, but you never know.  Maybe we just leave this until someone
> shouts they want to be able to avoid compiling it in.
>>
>>   #include <linux/iio/iio.h>
>>   #include <linux/iio/machine.h>
>> @@ -103,6 +104,7 @@
>>
>>   /* Bit definitions common for ADC_V1 and ADC_V2 */
>>   #define ADC_CON_EN_START    (1u << 0)
>> +#define ADC_DATX_PRESSED    (1u << 15)
>>   #define ADC_DATX_MASK        0xFFF
>>
>>   #define EXYNOS_ADC_TIMEOUT    (msecs_to_jiffies(100))
>> @@ -110,16 +112,20 @@
>>   struct exynos_adc {
>>       struct exynos_adc_data    *data;
>>       struct device        *dev;
>> +    struct input_dev    *input;
>>       void __iomem        *regs;
>>       void __iomem        *enable_reg;
>>       struct clk        *clk;
>>       struct clk        *sclk;
>>       unsigned int        irq;
>> +    unsigned int        tsirq;
>>       struct regulator    *vdd;
>>
>>       struct completion    completion;
>>
>> +    bool            read_ts;
>>       u32            value;
>> +    u32            value2;
> As I state further down, I'd rather keep a little
> clear of the naming used in IIO for bits that aren't
> going through IIO (less confusing!). Maybe just
> have
>      u32 x, y;
>>       unsigned int            version;
>>   };
>>
>> @@ -390,12 +396,61 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>>       return ret;
>>   }
>>
>> +static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
>> +                struct iio_chan_spec const *chan,
>> +                int *val,
>> +                int *val2,
>> +                long mask)
>> +{
>> +    struct exynos_adc *info = iio_priv(indio_dev);
>> +    unsigned long timeout;
>> +    int ret;
>> +
>> +    if (mask != IIO_CHAN_INFO_RAW)
>> +        return -EINVAL;
>> +
>> +    mutex_lock(&indio_dev->mlock);
>> +    info->read_ts = 1;
>> +
>> +    reinit_completion(&info->completion);
>> +
>> +    writel(ADC_S3C2410_TSC_PULL_UP_DISABLE | ADC_TSC_AUTOPST,
>> +           ADC_V1_TSC(info->regs));
>> +
>> +    /* Select the ts channel to be used and Trigger conversion */
>> +    info->data->start_conv(info, 0);
> 0 is a rather magic value.  A define perhaps?
>> +
>> +    timeout = wait_for_completion_timeout
>> +            (&info->completion, EXYNOS_ADC_TIMEOUT);
>> +    if (timeout == 0) {
>> +        dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
>> +        if (info->data->init_hw)
>> +            info->data->init_hw(info);
>> +        ret = -ETIMEDOUT;
>> +    } else {
>> +        *val = info->value;
>> +        *val2 = info->value2;
> This is definitely abuse as those two values are not intended for
> different values.  If you want to do this please use different naming
> and don't try to fiddle it into the IIO read raw framework.
> As you've suggested above, better to simplify this code and drop the
> bits cloned from the other handler.
>> +        ret = IIO_VAL_INT;
>> +    }
>> +
>> +    info->read_ts = 0;
>> +    mutex_unlock(&indio_dev->mlock);
>> +
>> +    return ret;
>> +}
>> +
>>   static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>>   {
>>       struct exynos_adc *info = (struct exynos_adc *)dev_id;
>>
>>       /* Read value */
>> -    info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>> +    if (info->read_ts) {
>> +        info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>> +        info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
> ADC_DATY_MASK would be more obviously correct.
>
>> +        writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
> Perhaps the above is cryptic enough to warrant a comment?
>> +    } else {
>> +        info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>> +    }
>>
>>       /* clear irq */
>>       if (info->data->clear_irq)
>> @@ -406,6 +461,46 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>>       return IRQ_HANDLED;
>>   }
>>
>> +/*
>> + * Here we (ab)use a threaded interrupt handler to stay running
>> + * for as long as the touchscreen remains pressed, we report
>> + * a new event with the latest data and then sleep until the
>> + * next timer tick. This mirrors the behavior of the old
>> + * driver, with much less code.
>> + */
>> +static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
>> +{
>> +    struct exynos_adc *info = dev_id;
>> +    struct iio_dev *dev = dev_get_drvdata(info->dev);
>> +    u32 x, y;
>> +    bool pressed;
>> +    int ret;
>> +
>> +    do {
>> +        ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> = exynos
>> +        if (ret == -ETIMEDOUT)
>> +            break;
>> +
>> +        pressed = x & y & ADC_DATX_PRESSED;
>> +        if (!pressed)
>> +            break;
>> +
>> +        input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
>> +        input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
>> +        input_report_key(info->input, BTN_TOUCH, 1);
>> +        input_sync(info->input);
>> +
>> +        msleep(1);
>> +    } while (1);
>> +
>> +    input_report_key(info->input, BTN_TOUCH, 0);
>> +    input_sync(info->input);
>> +
>> +    writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>>   static int exynos_adc_reg_access(struct iio_dev *indio_dev,
>>                     unsigned reg, unsigned writeval,
>>                     unsigned *readval)
>> @@ -457,12 +552,57 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
>>       return 0;
>>   }
>>
>> +static int exynos_adc_ts_init(struct exynos_adc *info)
>> +{
>> +    int ret;
>> +
>> +    info->input = input_allocate_device();
>> +    if (!info->input)
>> +        return -ENOMEM;
>> +
>> +    info->input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
>> +    info->input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
>> +
>> +    input_set_abs_params(info->input, ABS_X, 0, 0x3FF, 0, 0);
>> +    input_set_abs_params(info->input, ABS_Y, 0, 0x3FF, 0, 0);
>> +
>> +    /* data from s3c2410_ts driver */
>> +    info->input->name = "S3C24xx TouchScreen";
>> +    info->input->id.bustype = BUS_HOST;
>> +    info->input->id.vendor = 0xDEAD;
>> +    info->input->id.product = 0xBEEF;
>> +    info->input->id.version = 0x0200;
>> +
>> +    ret = input_register_device(info->input);
>> +    if (ret) {
>> +        input_free_device(info->input);
>> +        goto err;
>> +    }
>> +
>> +    if (info->tsirq > 0)
>> +        ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
>> +                    0, "touchscreen", info);
> info->tsirq
> (that had me really confused for a moment ;)
> Also, perhaps a more specific name.  touchscreen_updown or similar as the
> main interrupt is also used during touchscreen operation.
>> +    if (ret < 0) {
>> +        dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
>> +                            info->irq);
>> +        goto err_input;
>> +    }
>> +
>> +    return 0;
>> +
>> +err_input:
>> +    input_unregister_device(info->input);
>> +err:
>> +    return ret;
>> +}
>> +
>>   static int exynos_adc_probe(struct platform_device *pdev)
>>   {
>>       struct exynos_adc *info = NULL;
>>       struct device_node *np = pdev->dev.of_node;
>>       struct iio_dev *indio_dev = NULL;
>>       struct resource    *mem;
>> +    bool has_ts;
>>       int ret = -ENODEV;
>>       int irq;
>>
>> @@ -498,8 +638,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>           dev_err(&pdev->dev, "no irq resource?\n");
>>           return irq;
>>       }
>> -
>>       info->irq = irq;
>> +
>> +    irq = platform_get_irq(pdev, 1);
>> +    if (irq == -EPROBE_DEFER)
>> +        return irq;
>> +
>> +    info->tsirq = irq;
>> +
>>       info->dev = &pdev->dev;
>>
>>       init_completion(&info->completion);
>> @@ -565,6 +711,12 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>       if (info->data->init_hw)
>>           info->data->init_hw(info);
>>
>> +    has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
>> +    if (has_ts)
>> +        ret = exynos_adc_ts_init(info);
>> +    if (ret)
>> +        goto err_iio;
>> +
>>       ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
>>       if (ret < 0) {
>>           dev_err(&pdev->dev, "failed adding child nodes\n");
>> @@ -576,6 +728,11 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>   err_of_populate:
>>       device_for_each_child(&indio_dev->dev, NULL,
>>                   exynos_adc_remove_devices);
>> +    if (has_ts) {
>> +        input_unregister_device(info->input);
>> +        free_irq(info->tsirq, info);
>> +    }
>> +err_iio:
>>       iio_device_unregister(indio_dev);
>>   err_irq:
>>       free_irq(info->irq, info);
>> @@ -595,9 +752,12 @@ static int exynos_adc_remove(struct platform_device *pdev)
>>       struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>       struct exynos_adc *info = iio_priv(indio_dev);
>>
>> +    input_free_device(info->input);
>>       device_for_each_child(&indio_dev->dev, NULL,
>>                   exynos_adc_remove_devices);
>>       iio_device_unregister(indio_dev);
>> +    if (info->tsirq > 0)
>> +        free_irq(info->tsirq, info);
>>       free_irq(info->irq, info);
>>       if (info->data->exit_hw)
>>           info->data->exit_hw(info);
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Dmitry Torokhov]

On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
> On 20/07/14 14:49, Jonathan Cameron wrote:
> >On 18/07/14 20:29, Arnd Bergmann wrote:
> >>This adds support for the touchscreen on Samsung s3c64xx.
> >>The driver is completely untested but shows roughly how
> >>it could be done, following the example of the at91 driver.
> >>
> >Hi Arnd,
> Cc'd linux-input and Dmitry.
> >
> >>Open questions include:
> >>
> >>- compared to the old plat-samsung/adc driver, there is
> >>   no support for prioritizing ts over other clients, nor
> >>   for oversampling. From my reading of the code, the
> >>   priorities didn't actually have any effect at all, but
> >>   the oversampling might be needed. Maybe the original
> >>   authors have some insight.
> >>
> >>- I simply register the input device from the adc driver
> >>   itself, as the at91 code does. The driver also supports
> >>   sub-nodes, but I don't understand how they are meant
> >>   to be used, so using those might be better.
> >So, the alternative you are (I think) referring to is to use
> >the buffered in kernel client interface.  That way a separate
> >touch screen driver can use the output channels provided by IIO
> >in much the same way you might use a regulator or similar.
> >Note that whilst this is similar to the simple polled interface
> >used for things like the iio_hwmon driver, the data flow is
> >quite different (clearly the polled interfce would be
> >inappropriate here).
> >
> >Whilst we've discussed it in the past for touch screen drivers
> >like this, usually the hardware isn't generic enough to be
> >of any real use if not being used as a touch screen.  As such
> >it's often simpler to just have the support directly in the
> >driver (as you've observed the at91 driver does this).
> >
> >Whilst the interface has been there a while, it's not really had
> >all that much use.  The original target was the simpler case
> >of 3D accelerometer where we have a generic iio to input
> >bridge driver. Time constraints meant that I haven't yet actually
> >formally submitted the input side of this. Whilst there are lots
> >of other things that can use this interface, right now nothing
> >actually does so.
> >
> >>
> >>- The new exynos_read_s3c64xx_ts() function is intentionally
> >>   very similar to the existing exynos_read_raw() functions.
> >>   It should probably be changed, either by merging the two
> >>   into one, or by simplifying the exynos_read_s3c64xx_ts()
> >>   function. This depends a bit on the answers to the questions
> >>   above.
> >I'd be tempted to not bother keeping them that similar.  It's
> >not a generic IIO channel so simplify it where possible.
> >>
> >>- We probably need to add support for platform_data as well,
> >>   I've skipped this so far.
> >>
> >>- Is anybody able to debug this driver on real hardware?
> >>   While it's possible that it actually works, it's more
> >>   likely that I made a few subtle mistakes.
> >>
> >>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >Looks pretty good to me.  A few symantic bits and pieces and
> >one bug spotted.  Short and sweet.
> >>
> >>diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> >>index e1b74828f413..4329bf3c3326 100644
> >>--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> >>+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> >>@@ -41,6 +41,10 @@ Required properties:
> >>                         and compatible ADC block)
> >>  - vdd-supply        VDD input supply.
> >>
> >>+Optional properties:
> >>+- has-touchscreen:    If present, indicates that a touchscreen is
> >>+            connected an usable.
> >>+
> >>  Note: child nodes can be added for auto probing from device tree.
> >>
> >>  Example: adding device info in dtsi file
> >>diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> >>index 5f95638513d2..cf1d9f3e2492 100644
> >>--- a/drivers/iio/adc/exynos_adc.c
> >>+++ b/drivers/iio/adc/exynos_adc.c
> >>@@ -34,6 +34,7 @@
> >>  #include <linux/regulator/consumer.h>
> >>  #include <linux/of_platform.h>
> >>  #include <linux/err.h>
> >>+#include <linux/input.h>
> >Might want to make the input side optional at compile time...
> >I supose the existing parts are unlikely to be used much in headless
> >devices, but you never know.  Maybe we just leave this until someone
> >shouts they want to be able to avoid compiling it in.
> >>
> >>  #include <linux/iio/iio.h>
> >>  #include <linux/iio/machine.h>
> >>@@ -103,6 +104,7 @@
> >>
> >>  /* Bit definitions common for ADC_V1 and ADC_V2 */
> >>  #define ADC_CON_EN_START    (1u << 0)
> >>+#define ADC_DATX_PRESSED    (1u << 15)
> >>  #define ADC_DATX_MASK        0xFFF
> >>
> >>  #define EXYNOS_ADC_TIMEOUT    (msecs_to_jiffies(100))
> >>@@ -110,16 +112,20 @@
> >>  struct exynos_adc {
> >>      struct exynos_adc_data    *data;
> >>      struct device        *dev;
> >>+    struct input_dev    *input;
> >>      void __iomem        *regs;
> >>      void __iomem        *enable_reg;
> >>      struct clk        *clk;
> >>      struct clk        *sclk;
> >>      unsigned int        irq;
> >>+    unsigned int        tsirq;
> >>      struct regulator    *vdd;
> >>
> >>      struct completion    completion;
> >>
> >>+    bool            read_ts;
> >>      u32            value;
> >>+    u32            value2;
> >As I state further down, I'd rather keep a little
> >clear of the naming used in IIO for bits that aren't
> >going through IIO (less confusing!). Maybe just
> >have
> >     u32 x, y;
> >>      unsigned int            version;
> >>  };
> >>
> >>@@ -390,12 +396,61 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> >>      return ret;
> >>  }
> >>
> >>+static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
> >>+                struct iio_chan_spec const *chan,
> >>+                int *val,
> >>+                int *val2,
> >>+                long mask)
> >>+{
> >>+    struct exynos_adc *info = iio_priv(indio_dev);
> >>+    unsigned long timeout;
> >>+    int ret;
> >>+
> >>+    if (mask != IIO_CHAN_INFO_RAW)
> >>+        return -EINVAL;
> >>+
> >>+    mutex_lock(&indio_dev->mlock);
> >>+    info->read_ts = 1;
> >>+
> >>+    reinit_completion(&info->completion);
> >>+
> >>+    writel(ADC_S3C2410_TSC_PULL_UP_DISABLE | ADC_TSC_AUTOPST,
> >>+           ADC_V1_TSC(info->regs));
> >>+
> >>+    /* Select the ts channel to be used and Trigger conversion */
> >>+    info->data->start_conv(info, 0);
> >0 is a rather magic value.  A define perhaps?
> >>+
> >>+    timeout = wait_for_completion_timeout
> >>+            (&info->completion, EXYNOS_ADC_TIMEOUT);
> >>+    if (timeout == 0) {
> >>+        dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
> >>+        if (info->data->init_hw)
> >>+            info->data->init_hw(info);
> >>+        ret = -ETIMEDOUT;
> >>+    } else {
> >>+        *val = info->value;
> >>+        *val2 = info->value2;
> >This is definitely abuse as those two values are not intended for
> >different values.  If you want to do this please use different naming
> >and don't try to fiddle it into the IIO read raw framework.
> >As you've suggested above, better to simplify this code and drop the
> >bits cloned from the other handler.
> >>+        ret = IIO_VAL_INT;
> >>+    }
> >>+
> >>+    info->read_ts = 0;
> >>+    mutex_unlock(&indio_dev->mlock);
> >>+
> >>+    return ret;
> >>+}
> >>+
> >>  static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> >>  {
> >>      struct exynos_adc *info = (struct exynos_adc *)dev_id;
> >>
> >>      /* Read value */
> >>-    info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >>+    if (info->read_ts) {
> >>+        info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >>+        info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
> >ADC_DATY_MASK would be more obviously correct.
> >
> >>+        writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
> >Perhaps the above is cryptic enough to warrant a comment?
> >>+    } else {
> >>+        info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >>+    }
> >>
> >>      /* clear irq */
> >>      if (info->data->clear_irq)
> >>@@ -406,6 +461,46 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> >>      return IRQ_HANDLED;
> >>  }
> >>
> >>+/*
> >>+ * Here we (ab)use a threaded interrupt handler to stay running
> >>+ * for as long as the touchscreen remains pressed, we report
> >>+ * a new event with the latest data and then sleep until the
> >>+ * next timer tick. This mirrors the behavior of the old
> >>+ * driver, with much less code.
> >>+ */
> >>+static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> >>+{
> >>+    struct exynos_adc *info = dev_id;
> >>+    struct iio_dev *dev = dev_get_drvdata(info->dev);
> >>+    u32 x, y;
> >>+    bool pressed;
> >>+    int ret;
> >>+
> >>+    do {
> >>+        ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> >= exynos
> >>+        if (ret == -ETIMEDOUT)
> >>+            break;
> >>+
> >>+        pressed = x & y & ADC_DATX_PRESSED;
> >>+        if (!pressed)
> >>+            break;
> >>+
> >>+        input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
> >>+        input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
> >>+        input_report_key(info->input, BTN_TOUCH, 1);
> >>+        input_sync(info->input);
> >>+
> >>+        msleep(1);
> >>+    } while (1);

It would be nice to actually close the device even if someone is
touching screen. Please implement open/close methods and have them set a
flag that you would check here.


> >>+
> >>+    input_report_key(info->input, BTN_TOUCH, 0);
> >>+    input_sync(info->input);
> >>+
> >>+    writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
> >>+
> >>+    return IRQ_HANDLED;
> >>+}
> >>+
> >>  static int exynos_adc_reg_access(struct iio_dev *indio_dev,
> >>                    unsigned reg, unsigned writeval,
> >>                    unsigned *readval)
> >>@@ -457,12 +552,57 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
> >>      return 0;
> >>  }
> >>
> >>+static int exynos_adc_ts_init(struct exynos_adc *info)
> >>+{
> >>+    int ret;
> >>+
> >>+    info->input = input_allocate_device();
> >>+    if (!info->input)
> >>+        return -ENOMEM;
> >>+
> >>+    info->input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> >>+    info->input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> >>+
> >>+    input_set_abs_params(info->input, ABS_X, 0, 0x3FF, 0, 0);
> >>+    input_set_abs_params(info->input, ABS_Y, 0, 0x3FF, 0, 0);
> >>+
> >>+    /* data from s3c2410_ts driver */
> >>+    info->input->name = "S3C24xx TouchScreen";
> >>+    info->input->id.bustype = BUS_HOST;
> >>+    info->input->id.vendor = 0xDEAD;
> >>+    info->input->id.product = 0xBEEF;

You do not need to fill these entries with fake data.

> >>+    info->input->id.version = 0x0200;
> >>+
> >>+    ret = input_register_device(info->input);
> >>+    if (ret) {
> >>+        input_free_device(info->input);
> >>+        goto err;
> >>+    }
> >>+
> >>+    if (info->tsirq > 0)
> >>+        ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
> >>+                    0, "touchscreen", info);
> >info->tsirq
> >(that had me really confused for a moment ;)
> >Also, perhaps a more specific name.  touchscreen_updown or similar as the
> >main interrupt is also used during touchscreen operation.
> >>+    if (ret < 0) {
> >>+        dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
> >>+                            info->irq);
> >>+        goto err_input;
> >>+    }
> >>+
> >>+    return 0;
> >>+
> >>+err_input:
> >>+    input_unregister_device(info->input);
> >>+err:
> >>+    return ret;
> >>+}
> >>+
> >>  static int exynos_adc_probe(struct platform_device *pdev)
> >>  {
> >>      struct exynos_adc *info = NULL;
> >>      struct device_node *np = pdev->dev.of_node;
> >>      struct iio_dev *indio_dev = NULL;
> >>      struct resource    *mem;
> >>+    bool has_ts;
> >>      int ret = -ENODEV;
> >>      int irq;
> >>
> >>@@ -498,8 +638,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
> >>          dev_err(&pdev->dev, "no irq resource?\n");
> >>          return irq;
> >>      }
> >>-
> >>      info->irq = irq;
> >>+
> >>+    irq = platform_get_irq(pdev, 1);
> >>+    if (irq == -EPROBE_DEFER)
> >>+        return irq;
> >>+
> >>+    info->tsirq = irq;
> >>+
> >>      info->dev = &pdev->dev;
> >>
> >>      init_completion(&info->completion);
> >>@@ -565,6 +711,12 @@ static int exynos_adc_probe(struct platform_device *pdev)
> >>      if (info->data->init_hw)
> >>          info->data->init_hw(info);
> >>
> >>+    has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
> >>+    if (has_ts)
> >>+        ret = exynos_adc_ts_init(info);
> >>+    if (ret)
> >>+        goto err_iio;
> >>+
> >>      ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
> >>      if (ret < 0) {
> >>          dev_err(&pdev->dev, "failed adding child nodes\n");
> >>@@ -576,6 +728,11 @@ static int exynos_adc_probe(struct platform_device *pdev)
> >>  err_of_populate:
> >>      device_for_each_child(&indio_dev->dev, NULL,
> >>                  exynos_adc_remove_devices);
> >>+    if (has_ts) {
> >>+        input_unregister_device(info->input);
> >>+        free_irq(info->tsirq, info);

Are we sure that device is quiesced here and interrupt won't arrive
between input_unregister_device() and free_irq()? I guess if you
properly implement open/close it won't matter.

> >>+    }
> >>+err_iio:
> >>      iio_device_unregister(indio_dev);
> >>  err_irq:
> >>      free_irq(info->irq, info);
> >>@@ -595,9 +752,12 @@ static int exynos_adc_remove(struct platform_device *pdev)
> >>      struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >>      struct exynos_adc *info = iio_priv(indio_dev);
> >>
> >>+    input_free_device(info->input);

Should be unregister, not free.

> >>      device_for_each_child(&indio_dev->dev, NULL,
> >>                  exynos_adc_remove_devices);
> >>      iio_device_unregister(indio_dev);
> >>+    if (info->tsirq > 0)
> >>+        free_irq(info->tsirq, info);
> >>      free_irq(info->irq, info);
> >>      if (info->data->exit_hw)
> >>          info->data->exit_hw(info);
> >>

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Arnd Bergmann]

On Sunday 20 July 2014 13:28:42 Dmitry Torokhov wrote:
> On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
> > >>+
> > >>+    do {
> > >>+        ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> > >= exynos
> > >>+        if (ret == -ETIMEDOUT)
> > >>+            break;
> > >>+
> > >>+        pressed = x & y & ADC_DATX_PRESSED;
> > >>+        if (!pressed)
> > >>+            break;
> > >>+
> > >>+        input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
> > >>+        input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
> > >>+        input_report_key(info->input, BTN_TOUCH, 1);
> > >>+        input_sync(info->input);
> > >>+
> > >>+        msleep(1);
> > >>+    } while (1);
> 
> It would be nice to actually close the device even if someone is
> touching screen. Please implement open/close methods and have them set a
> flag that you would check here.

Ok. I think it's even better to move the request_irq() into the open function,
which will avoid the flag and defer the error handling into the actual opening,
as well as syncing the running irq with the close function.

> > >>+    /* data from s3c2410_ts driver */
> > >>+    info->input->name = "S3C24xx TouchScreen";
> > >>+    info->input->id.bustype = BUS_HOST;
> > >>+    info->input->id.vendor = 0xDEAD;
> > >>+    info->input->id.product = 0xBEEF;
> 
> You do not need to fill these entries with fake data.

Ok, I wondered about this, but didn't want to change too much from
the old driver (I changed the version number).

> > >>+    info->input->id.version = 0x0200;

Do I need this?

> > >>@@ -576,6 +728,11 @@ static int exynos_adc_probe(struct platform_device *pdev)
> > >>  err_of_populate:
> > >>      device_for_each_child(&indio_dev->dev, NULL,
> > >>                  exynos_adc_remove_devices);
> > >>+    if (has_ts) {
> > >>+        input_unregister_device(info->input);
> > >>+        free_irq(info->tsirq, info);
> 
> Are we sure that device is quiesced here and interrupt won't arrive
> between input_unregister_device() and free_irq()? I guess if you
> properly implement open/close it won't matter.
 
Should be fixed now.

> > >>+err_iio:
> > >>      iio_device_unregister(indio_dev);
> > >>  err_irq:
> > >>      free_irq(info->irq, info);
> > >>@@ -595,9 +752,12 @@ static int exynos_adc_remove(struct platform_device *pdev)
> > >>      struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > >>      struct exynos_adc *info = iio_priv(indio_dev);
> > >>
> > >>+    input_free_device(info->input);
> 
> Should be unregister, not free.

Ok.

Thanks a lot for the review! I'll send out the new version after some build testing.

	Arnd

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Arnd Bergmann]

On Monday 21 July 2014 12:23:58 Arnd Bergmann wrote:
> 
> Thanks a lot for the review! I'll send out the new version after some build testing.

Here are the changes I did to the adc driver based on the review comments.
I'll start a new thread for the updated version that includes the changes.

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 3d2caea05977..823d7ebc7f52 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -67,6 +67,9 @@
 
 #define ADC_S3C2410_CON_SELMUX(x) (((x)&0x7)<<3)
 
+/* touch screen always uses channel 0 */
+#define ADC_S3C2410_MUX_TS	0
+
 /* ADCTSC Register Bits */
 #define ADC_S3C2443_TSC_UD_SEN		(1u << 8)
 #define ADC_S3C2410_TSC_YM_SEN		(1u << 7)
@@ -106,6 +109,7 @@
 #define ADC_CON_EN_START	(1u << 0)
 #define ADC_DATX_PRESSED	(1u << 15)
 #define ADC_DATX_MASK		0xFFF
+#define ADC_DATY_MASK		0xFFF
 
 #define EXYNOS_ADC_TIMEOUT	(msecs_to_jiffies(100))
 
@@ -123,10 +127,12 @@ struct exynos_adc {
 
 	struct completion	completion;
 
-	bool			read_ts;
 	u32			value;
-	u32			value2;
 	unsigned int            version;
+
+	bool			read_ts;
+	u32			ts_x;
+	u32			ts_y;
 };
 
 struct exynos_adc_data {
@@ -396,21 +402,14 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
-				struct iio_chan_spec const *chan,
-				int *val,
-				int *val2,
-				long mask)
+static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev, int *x, int *y)
 {
 	struct exynos_adc *info = iio_priv(indio_dev);
 	unsigned long timeout;
 	int ret;
 
-	if (mask != IIO_CHAN_INFO_RAW)
-		return -EINVAL;
-
 	mutex_lock(&indio_dev->mlock);
-	info->read_ts = 1;
+	info->read_ts = true;
 
 	reinit_completion(&info->completion);
 
@@ -418,7 +417,7 @@ static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
 	       ADC_V1_TSC(info->regs));
 
 	/* Select the ts channel to be used and Trigger conversion */
-	info->data->start_conv(info, 0);
+	info->data->start_conv(info, ADC_S3C2410_MUX_TS);
 
 	timeout = wait_for_completion_timeout
 			(&info->completion, EXYNOS_ADC_TIMEOUT);
@@ -428,12 +427,12 @@ static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
 			info->data->init_hw(info);
 		ret = -ETIMEDOUT;
 	} else {
-		*val = info->value;
-		*val2 = info->value2;
-		ret = IIO_VAL_INT;
+		*x = info->ts_x;
+		*y = info->ts_y;
+		ret = 0;
 	}
 
-	info->read_ts = 0;
+	info->read_ts = false;
 	mutex_unlock(&indio_dev->mlock);
 
 	return ret;
@@ -445,8 +444,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
 
 	/* Read value */
 	if (info->read_ts) {
-		info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
-		info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
+		info->ts_x = readl(ADC_V1_DATX(info->regs));
+		info->ts_y = readl(ADC_V1_DATY(info->regs));
 		writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
 	} else {
 		info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
@@ -477,7 +476,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
 	int ret;
 
 	do {
-		ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
+		ret = exynos_read_s3c64xx_ts(dev, &x, &y);
 		if (ret == -ETIMEDOUT)
 			break;
 
@@ -486,11 +485,15 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
 			break;
 
 		input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
-		input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
+		input_report_abs(info->input, ABS_Y, y & ADC_DATY_MASK);
 		input_report_key(info->input, BTN_TOUCH, 1);
 		input_sync(info->input);
 
 		msleep(1);
+
+		/* device may have been closed while touched */
+		if (!info->input->users)
+			return IRQ_HANDLED;
 	} while (1);
 
 	input_report_key(info->input, BTN_TOUCH, 0);
@@ -552,10 +555,28 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
 	return 0;
 }
 
+static int exynos_adc_ts_open(struct input_dev *dev)
+{
+	struct exynos_adc *info = input_get_drvdata(dev);
+
+	return request_threaded_irq(info->tsirq, NULL, exynos_ts_isr,
+				    0, "touchscreen", info);
+}
+
+static void exynos_adc_ts_close(struct input_dev *dev)
+{
+	struct exynos_adc *info = input_get_drvdata(dev);
+
+	free_irq(info->tsirq, info);
+}
+
 static int exynos_adc_ts_init(struct exynos_adc *info)
 {
 	int ret;
 
+	if (info->tsirq <= 0)
+		return -ENODEV;
+
 	info->input = input_allocate_device();
 	if (!info->input)
 		return -ENOMEM;
@@ -566,33 +587,17 @@ static int exynos_adc_ts_init(struct exynos_adc *info)
 	input_set_abs_params(info->input, ABS_X, 0, 0x3FF, 0, 0);
 	input_set_abs_params(info->input, ABS_Y, 0, 0x3FF, 0, 0);
 
-	/* data from s3c2410_ts driver */
 	info->input->name = "S3C24xx TouchScreen";
 	info->input->id.bustype = BUS_HOST;
-	info->input->id.vendor = 0xDEAD;
-	info->input->id.product = 0xBEEF;
-	info->input->id.version = 0x0200;
+	info->input->open = exynos_adc_ts_open;
+	info->input->close = exynos_adc_ts_close;
+	
+	input_set_drvdata(info->input, info);
 
 	ret = input_register_device(info->input);
-	if (ret) {
+	if (ret)
 		input_free_device(info->input);
-		goto err;
-	}
-
-	if (info->tsirq > 0)
-		ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
-					0, "touchscreen", info);
-	if (ret < 0) {
-		dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
-							info->irq);
-		goto err_input;
-	}
-
-	return 0;
 
-err_input:
-	input_unregister_device(info->input);
-err:
 	return ret;
 }
 
@@ -602,7 +607,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct iio_dev *indio_dev = NULL;
 	struct resource	*mem;
-	bool has_ts;
+	bool has_ts = false;
 	int ret = -ENODEV;
 	int irq;
 
@@ -711,7 +716,10 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	if (info->data->init_hw)
 		info->data->init_hw(info);
 
-	has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
+	/* leave out any TS related code if unreachable */
+	if (IS_BUILTIN(CONFIG_INPUT) ||
+	    (IS_MODULE(CONFIG_INPUT) && config_enabled(MODULE)))
+		has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
 	if (has_ts)
 		ret = exynos_adc_ts_init(info);
 	if (ret)
@@ -752,7 +760,7 @@ static int exynos_adc_remove(struct platform_device *pdev)
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct exynos_adc *info = iio_priv(indio_dev);
 
-	input_free_device(info->input);	
+	input_unregister_device(info->input);	
 	device_for_each_child(&indio_dev->dev, NULL,
 				exynos_adc_remove_devices);
 	iio_device_unregister(indio_dev);

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Dmitry Torokhov]

On Mon, Jul 21, 2014 at 12:23:58PM +0200, Arnd Bergmann wrote:
> On Sunday 20 July 2014 13:28:42 Dmitry Torokhov wrote:
> > On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
> > > >>+
> > > >>+    do {
> > > >>+        ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> > > >= exynos
> > > >>+        if (ret == -ETIMEDOUT)
> > > >>+            break;
> > > >>+
> > > >>+        pressed = x & y & ADC_DATX_PRESSED;
> > > >>+        if (!pressed)
> > > >>+            break;
> > > >>+
> > > >>+        input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
> > > >>+        input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
> > > >>+        input_report_key(info->input, BTN_TOUCH, 1);
> > > >>+        input_sync(info->input);
> > > >>+
> > > >>+        msleep(1);
> > > >>+    } while (1);
> > 
> > It would be nice to actually close the device even if someone is
> > touching screen. Please implement open/close methods and have them set a
> > flag that you would check here.
> 
> Ok. I think it's even better to move the request_irq() into the open function,
> which will avoid the flag and defer the error handling into the actual opening,
> as well as syncing the running irq with the close function.

I do not quite like acquiring resources needed in open. I think drivers should
do all resource acquisition in probe() and leave open()/close() to
activate/quiesce devices.

> 
> > > >>+    /* data from s3c2410_ts driver */
> > > >>+    info->input->name = "S3C24xx TouchScreen";
> > > >>+    info->input->id.bustype = BUS_HOST;
> > > >>+    info->input->id.vendor = 0xDEAD;
> > > >>+    info->input->id.product = 0xBEEF;
> > 
> > You do not need to fill these entries with fake data.
> 
> Ok, I wondered about this, but didn't want to change too much from
> the old driver (I changed the version number).
> 
> > > >>+    info->input->id.version = 0x0200;
> 
> Do I need this?

Not really.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Arnd Bergmann]

On Monday 21 July 2014 07:44:42 Dmitry Torokhov wrote:
> > > 
> > > It would be nice to actually close the device even if someone is
> > > touching screen. Please implement open/close methods and have them set a
> > > flag that you would check here.
> > 
> > Ok. I think it's even better to move the request_irq() into the open function,
> > which will avoid the flag and defer the error handling into the actual opening,
> > as well as syncing the running irq with the close function.
> 
> I do not quite like acquiring resources needed in open. I think drivers should
> do all resource acquisition in probe() and leave open()/close() to
> activate/quiesce devices.

Ok, I'll move it back then. I'm not sure what I'm supposed to do
in open/close then. Isn't it enough to check info->input->users
like this?

static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
{
        struct exynos_adc *info = dev_id;
        struct iio_dev *dev = dev_get_drvdata(info->dev);
        u32 x, y;
        bool pressed;
        int ret;

        while (info->input->users) {
                ret = exynos_read_s3c64xx_ts(dev, &x, &y);
                if (ret == -ETIMEDOUT)
                        break;

                pressed = x & y & ADC_DATX_PRESSED;
                if (!pressed) {
                        input_report_key(info->input, BTN_TOUCH, 0);
                        input_sync(info->input);
                        break;
                }

                input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
                input_report_abs(info->input, ABS_Y, y & ADC_DATY_MASK);
                input_report_key(info->input, BTN_TOUCH, 1);
                input_sync(info->input);

                msleep(1);
        };

        writel(0, ADC_V1_CLRINTPNDNUP(info->regs));

        return IRQ_HANDLED;
}

I could do enable_irq()/disable_irq(), but that leaves a small
race at startup where we request the irq line and then immediately
disable it again.

	Arnd

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Dmitry Torokhov]

On Mon, Jul 21, 2014 at 05:11:27PM +0200, Arnd Bergmann wrote:
> On Monday 21 July 2014 07:44:42 Dmitry Torokhov wrote:
> > > > 
> > > > It would be nice to actually close the device even if someone is
> > > > touching screen. Please implement open/close methods and have them set a
> > > > flag that you would check here.
> > > 
> > > Ok. I think it's even better to move the request_irq() into the open function,
> > > which will avoid the flag and defer the error handling into the actual opening,
> > > as well as syncing the running irq with the close function.
> > 
> > I do not quite like acquiring resources needed in open. I think drivers should
> > do all resource acquisition in probe() and leave open()/close() to
> > activate/quiesce devices.
> 
> Ok, I'll move it back then. I'm not sure what I'm supposed to do
> in open/close then. Isn't it enough to check info->input->users
> like this?
> 
> static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> {
>         struct exynos_adc *info = dev_id;
>         struct iio_dev *dev = dev_get_drvdata(info->dev);
>         u32 x, y;
>         bool pressed;
>         int ret;
> 
>         while (info->input->users) {
>                 ret = exynos_read_s3c64xx_ts(dev, &x, &y);
>                 if (ret == -ETIMEDOUT)
>                         break;
> 
>                 pressed = x & y & ADC_DATX_PRESSED;
>                 if (!pressed) {
>                         input_report_key(info->input, BTN_TOUCH, 0);
>                         input_sync(info->input);
>                         break;
>                 }
> 
>                 input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
>                 input_report_abs(info->input, ABS_Y, y & ADC_DATY_MASK);
>                 input_report_key(info->input, BTN_TOUCH, 1);
>                 input_sync(info->input);
> 
>                 msleep(1);
>         };
> 
>         writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
> 
>         return IRQ_HANDLED;
> }
> 
> I could do enable_irq()/disable_irq(), but that leaves a small
> race at startup where we request the irq line and then immediately
> disable it again.

I think the above (or a separate flag in driver structure) coupled with
enable/disable IRQ is fine - input core can deal with calls to input_report_*
on devices that have been properly allocated but have not been registered yet.

drivers/input/keyboard/samsung-keypad.c does similar handling.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Hartmut Knaack]

Jonathan Cameron schrieb:
> On 18/07/14 20:29, Arnd Bergmann wrote:
>> This adds support for the touchscreen on Samsung s3c64xx.
>> The driver is completely untested but shows roughly how
>> it could be done, following the example of the at91 driver.
>>
> Hi Arnd,
>
>> Open questions include:
>>
>> - compared to the old plat-samsung/adc driver, there is
>>    no support for prioritizing ts over other clients, nor
>>    for oversampling. From my reading of the code, the
>>    priorities didn't actually have any effect at all, but
>>    the oversampling might be needed. Maybe the original
>>    authors have some insight.
>>
>> - I simply register the input device from the adc driver
>>    itself, as the at91 code does. The driver also supports
>>    sub-nodes, but I don't understand how they are meant
>>    to be used, so using those might be better.
> So, the alternative you are (I think) referring to is to use
> the buffered in kernel client interface.  That way a separate
> touch screen driver can use the output channels provided by IIO
> in much the same way you might use a regulator or similar.
> Note that whilst this is similar to the simple polled interface
> used for things like the iio_hwmon driver, the data flow is
> quite different (clearly the polled interfce would be
> inappropriate here).
>
> Whilst we've discussed it in the past for touch screen drivers
> like this, usually the hardware isn't generic enough to be
> of any real use if not being used as a touch screen.  As such
> it's often simpler to just have the support directly in the
> driver (as you've observed the at91 driver does this).
>
> Whilst the interface has been there a while, it's not really had
> all that much use.  The original target was the simpler case
> of 3D accelerometer where we have a generic iio to input
> bridge driver. Time constraints meant that I haven't yet actually
> formally submitted the input side of this. Whilst there are lots
> of other things that can use this interface, right now nothing
> actually does so.
>
>> - The new exynos_read_s3c64xx_ts() function is intentionally
>>    very similar to the existing exynos_read_raw() functions.
>>    It should probably be changed, either by merging the two
>>    into one, or by simplifying the exynos_read_s3c64xx_ts()
>>    function. This depends a bit on the answers to the questions
>>    above.
> I'd be tempted to not bother keeping them that similar.  It's
> not a generic IIO channel so simplify it where possible.
>> - We probably need to add support for platform_data as well,
>>    I've skipped this so far.
>>
>> - Is anybody able to debug this driver on real hardware?
>>    While it's possible that it actually works, it's more
>>    likely that I made a few subtle mistakes.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Looks pretty good to me.  A few symantic bits and pieces and
> one bug spotted.  Short and sweet.
>> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> index e1b74828f413..4329bf3c3326 100644
>> --- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> @@ -41,6 +41,10 @@ Required properties:
>>   				       and compatible ADC block)
>>   - vdd-supply		VDD input supply.
>>
>> +Optional properties:
>> +- has-touchscreen:	If present, indicates that a touchscreen is
>> +			connected an usable.
>> +
>>   Note: child nodes can be added for auto probing from device tree.
>>
>>   Example: adding device info in dtsi file
>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>> index 5f95638513d2..cf1d9f3e2492 100644
>> --- a/drivers/iio/adc/exynos_adc.c
>> +++ b/drivers/iio/adc/exynos_adc.c
>> @@ -34,6 +34,7 @@
>>   #include <linux/regulator/consumer.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/err.h>
>> +#include <linux/input.h>
> Might want to make the input side optional at compile time...
> I supose the existing parts are unlikely to be used much in headless
> devices, but you never know.  Maybe we just leave this until someone
> shouts they want to be able to avoid compiling it in.
>>   #include <linux/iio/iio.h>
>>   #include <linux/iio/machine.h>
>> @@ -103,6 +104,7 @@
>>
>>   /* Bit definitions common for ADC_V1 and ADC_V2 */
>>   #define ADC_CON_EN_START	(1u << 0)
>> +#define ADC_DATX_PRESSED	(1u << 15)
>>   #define ADC_DATX_MASK		0xFFF
>>
>>   #define EXYNOS_ADC_TIMEOUT	(msecs_to_jiffies(100))
>> @@ -110,16 +112,20 @@
>>   struct exynos_adc {
>>   	struct exynos_adc_data	*data;
>>   	struct device		*dev;
>> +	struct input_dev	*input;
>>   	void __iomem		*regs;
>>   	void __iomem		*enable_reg;
>>   	struct clk		*clk;
>>   	struct clk		*sclk;
>>   	unsigned int		irq;
>> +	unsigned int		tsirq;
>>   	struct regulator	*vdd;
>>
>>   	struct completion	completion;
>>
>> +	bool			read_ts;
>>   	u32			value;
>> +	u32			value2;
> As I state further down, I'd rather keep a little
> clear of the naming used in IIO for bits that aren't
> going through IIO (less confusing!). Maybe just
> have
> 	u32 x, y;
>>   	unsigned int            version;
>>   };
>>
>> @@ -390,12 +396,61 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>>   	return ret;
>>   }
>>
>> +static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
>> +				struct iio_chan_spec const *chan,
>> +				int *val,
>> +				int *val2,
>> +				long mask)
>> +{
>> +	struct exynos_adc *info = iio_priv(indio_dev);
>> +	unsigned long timeout;
>> +	int ret;
>> +
>> +	if (mask != IIO_CHAN_INFO_RAW)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&indio_dev->mlock);
>> +	info->read_ts = 1;
Since info->read_ts is of type bool, use true/false.
>> +
>> +	reinit_completion(&info->completion);
>> +
>> +	writel(ADC_S3C2410_TSC_PULL_UP_DISABLE | ADC_TSC_AUTOPST,
>> +	       ADC_V1_TSC(info->regs));
>> +
>> +	/* Select the ts channel to be used and Trigger conversion */
>> +	info->data->start_conv(info, 0);
> 0 is a rather magic value.  A define perhaps?
>> +
>> +	timeout = wait_for_completion_timeout
>> +			(&info->completion, EXYNOS_ADC_TIMEOUT);
>> +	if (timeout == 0) {
>> +		dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
>> +		if (info->data->init_hw)
>> +			info->data->init_hw(info);
>> +		ret = -ETIMEDOUT;
>> +	} else {
>> +		*val = info->value;
>> +		*val2 = info->value2;
> This is definitely abuse as those two values are not intended for
> different values.  If you want to do this please use different naming
> and don't try to fiddle it into the IIO read raw framework.
> As you've suggested above, better to simplify this code and drop the
> bits cloned from the other handler.
>> +		ret = IIO_VAL_INT;
>> +	}
>> +
>> +	info->read_ts = 0;
>> +	mutex_unlock(&indio_dev->mlock);
>> +
>> +	return ret;
>> +}
>> +
>>   static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>>   {
>>   	struct exynos_adc *info = (struct exynos_adc *)dev_id;
>>
>>   	/* Read value */
>> -	info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>> +	if (info->read_ts) {
>> +		info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>> +		info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
> ADC_DATY_MASK would be more obviously correct.
>
>> +		writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
> Perhaps the above is cryptic enough to warrant a comment?
>> +	} else {
>> +		info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>> +	}
>>
>>   	/* clear irq */
>>   	if (info->data->clear_irq)
>> @@ -406,6 +461,46 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>>   	return IRQ_HANDLED;
>>   }
>>
>> +/*
>> + * Here we (ab)use a threaded interrupt handler to stay running
>> + * for as long as the touchscreen remains pressed, we report
>> + * a new event with the latest data and then sleep until the
>> + * next timer tick. This mirrors the behavior of the old
>> + * driver, with much less code.
>> + */
>> +static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
>> +{
>> +	struct exynos_adc *info = dev_id;
>> +	struct iio_dev *dev = dev_get_drvdata(info->dev);
>> +	u32 x, y;
>> +	bool pressed;
>> +	int ret;
>> +
>> +	do {
>> +		ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> = exynos
>> +		if (ret == -ETIMEDOUT)
>> +			break;
>> +
>> +		pressed = x & y & ADC_DATX_PRESSED;
>> +		if (!pressed)
>> +			break;
>> +
>> +		input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
>> +		input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
>> +		input_report_key(info->input, BTN_TOUCH, 1);
>> +		input_sync(info->input);
>> +
>> +		msleep(1);
>> +	} while (1);
>> +
>> +	input_report_key(info->input, BTN_TOUCH, 0);
>> +	input_sync(info->input);
>> +
>> +	writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>   static int exynos_adc_reg_access(struct iio_dev *indio_dev,
>>   			      unsigned reg, unsigned writeval,
>>   			      unsigned *readval)
>> @@ -457,12 +552,57 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
>>   	return 0;
>>   }
>>
>> +static int exynos_adc_ts_init(struct exynos_adc *info)
>> +{
>> +	int ret;
>> +
>> +	info->input = input_allocate_device();
>> +	if (!info->input)
>> +		return -ENOMEM;
>> +
>> +	info->input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
>> +	info->input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
>> +
>> +	input_set_abs_params(info->input, ABS_X, 0, 0x3FF, 0, 0);
>> +	input_set_abs_params(info->input, ABS_Y, 0, 0x3FF, 0, 0);
>> +
>> +	/* data from s3c2410_ts driver */
>> +	info->input->name = "S3C24xx TouchScreen";
>> +	info->input->id.bustype = BUS_HOST;
>> +	info->input->id.vendor = 0xDEAD;
>> +	info->input->id.product = 0xBEEF;
>> +	info->input->id.version = 0x0200;
>> +
>> +	ret = input_register_device(info->input);
>> +	if (ret) {
>> +		input_free_device(info->input);
>> +		goto err;
Just return ret, without goto (and get rid of label err).
>> +	}
>> +
>> +	if (info->tsirq > 0)
>> +		ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
>> +					0, "touchscreen", info);
> info->tsirq
> (that had me really confused for a moment ;)
> Also, perhaps a more specific name.  touchscreen_updown or similar as the
> main interrupt is also used during touchscreen operation.
>> +	if (ret < 0) {
>> +		dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
>> +							info->irq);
>> +		goto err_input;
>> +	}
>> +
>> +	return 0;
>> +
Probably better to get rid of the labels and move the code up, as it is only used once.
>> +err_input:
>> +	input_unregister_device(info->input);
>> +err:
>> +	return ret;
>> +}
>> +
>>   static int exynos_adc_probe(struct platform_device *pdev)
>>   {
>>   	struct exynos_adc *info = NULL;
>>   	struct device_node *np = pdev->dev.of_node;
>>   	struct iio_dev *indio_dev = NULL;
>>   	struct resource	*mem;
>> +	bool has_ts;
>>   	int ret = -ENODEV;
>>   	int irq;
>>
>> @@ -498,8 +638,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>   		dev_err(&pdev->dev, "no irq resource?\n");
>>   		return irq;
>>   	}
>> -
>>   	info->irq = irq;
>> +
>> +	irq = platform_get_irq(pdev, 1);
>> +	if (irq == -EPROBE_DEFER)
>> +		return irq;
What about other possible error codes?
>> +
>> +	info->tsirq = irq;
>> +
>>   	info->dev = &pdev->dev;
>>
>>   	init_completion(&info->completion);
>> @@ -565,6 +711,12 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>   	if (info->data->init_hw)
>>   		info->data->init_hw(info);
>>
>> +	has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
>> +	if (has_ts)
>> +		ret = exynos_adc_ts_init(info);
>> +	if (ret)
>> +		goto err_iio;
>> +
>>   	ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
>>   	if (ret < 0) {
>>   		dev_err(&pdev->dev, "failed adding child nodes\n");
>> @@ -576,6 +728,11 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>   err_of_populate:
>>   	device_for_each_child(&indio_dev->dev, NULL,
>>   				exynos_adc_remove_devices);
>> +	if (has_ts) {
>> +		input_unregister_device(info->input);
>> +		free_irq(info->tsirq, info);
>> +	}
>> +err_iio:
>>   	iio_device_unregister(indio_dev);
>>   err_irq:
>>   	free_irq(info->irq, info);
>> @@ -595,9 +752,12 @@ static int exynos_adc_remove(struct platform_device *pdev)
>>   	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>   	struct exynos_adc *info = iio_priv(indio_dev);
>>
>> +	input_free_device(info->input);	
>>   	device_for_each_child(&indio_dev->dev, NULL,
>>   				exynos_adc_remove_devices);
>>   	iio_device_unregister(indio_dev);
>> +	if (info->tsirq > 0)
>> +		free_irq(info->tsirq, info);
>>   	free_irq(info->irq, info);
>>   	if (info->data->exit_hw)
>>   		info->data->exit_hw(info);
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Arnd Bergmann]

On Sunday 20 July 2014 23:37:18 Hartmut Knaack wrote:
> Jonathan Cameron schrieb:
> > On 18/07/14 20:29, Arnd Bergmann wrote:
> >> - I simply register the input device from the adc driver
> >>    itself, as the at91 code does. The driver also supports
> >>    sub-nodes, but I don't understand how they are meant
> >>    to be used, so using those might be better.
> > So, the alternative you are (I think) referring to is to use
> > the buffered in kernel client interface.  That way a separate
> > touch screen driver can use the output channels provided by IIO
> > in much the same way you might use a regulator or similar.
> > Note that whilst this is similar to the simple polled interface
> > used for things like the iio_hwmon driver, the data flow is
> > quite different (clearly the polled interfce would be
> > inappropriate here).
> >
> > Whilst we've discussed it in the past for touch screen drivers
> > like this, usually the hardware isn't generic enough to be
> > of any real use if not being used as a touch screen.  As such
> > it's often simpler to just have the support directly in the
> > driver (as you've observed the at91 driver does this).

Ok, I see. That's exactly the information I was looking for.

> >
> > Whilst the interface has been there a while, it's not really had
> > all that much use.  The original target was the simpler case
> > of 3D accelerometer where we have a generic iio to input
> > bridge driver. Time constraints meant that I haven't yet actually
> > formally submitted the input side of this. Whilst there are lots
> > of other things that can use this interface, right now nothing
> > actually does so.

Ok.

> >> - The new exynos_read_s3c64xx_ts() function is intentionally
> >>    very similar to the existing exynos_read_raw() functions.
> >>    It should probably be changed, either by merging the two
> >>    into one, or by simplifying the exynos_read_s3c64xx_ts()
> >>    function. This depends a bit on the answers to the questions
> >>    above.
> > I'd be tempted to not bother keeping them that similar.  It's
> > not a generic IIO channel so simplify it where possible.

Ok.

> >> index 5f95638513d2..cf1d9f3e2492 100644
> >> --- a/drivers/iio/adc/exynos_adc.c
> >> +++ b/drivers/iio/adc/exynos_adc.c
> >> @@ -34,6 +34,7 @@
> >>   #include <linux/regulator/consumer.h>
> >>   #include <linux/of_platform.h>
> >>   #include <linux/err.h>
> >> +#include <linux/input.h>
> > Might want to make the input side optional at compile time...
> > I supose the existing parts are unlikely to be used much in headless
> > devices, but you never know.  Maybe we just leave this until someone
> > shouts they want to be able to avoid compiling it in.

I expected the input stuff to just be left out by the compiler
if CONFIG_INPUT is not set. I'll try it out and change it if necessary.

> >>   	struct completion	completion;
> >>
> >> +	bool			read_ts;
> >>   	u32			value;
> >> +	u32			value2;
> > As I state further down, I'd rather keep a little
> > clear of the naming used in IIO for bits that aren't
> > going through IIO (less confusing!). Maybe just
> > have
> > 	u32 x, y;

Ok.


> >>   	unsigned int            version;
> >>   };
> >>
> >> @@ -390,12 +396,61 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> >>   	return ret;
> >>   }
> >>
> >> +static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
> >> +				struct iio_chan_spec const *chan,
> >> +				int *val,
> >> +				int *val2,
> >> +				long mask)
> >> +{
> >> +	struct exynos_adc *info = iio_priv(indio_dev);
> >> +	unsigned long timeout;
> >> +	int ret;
> >> +
> >> +	if (mask != IIO_CHAN_INFO_RAW)
> >> +		return -EINVAL;
> >> +
> >> +	mutex_lock(&indio_dev->mlock);
> >> +	info->read_ts = 1;
> Since info->read_ts is of type bool, use true/false.

Ok

> >> +
> >> +	reinit_completion(&info->completion);
> >> +
> >> +	writel(ADC_S3C2410_TSC_PULL_UP_DISABLE | ADC_TSC_AUTOPST,
> >> +	       ADC_V1_TSC(info->regs));
> >> +
> >> +	/* Select the ts channel to be used and Trigger conversion */
> >> +	info->data->start_conv(info, 0);
> > 0 is a rather magic value.  A define perhaps?

I'm not entirely sure about why we pass 0 here, it's either channel zero
being used for touchscreen, or the channel number being ignore after
we write to the TSC register above. I copied it from the original driver,
but it might be helpful if someone with access to the specs could comment
here.

I'll add a macro for now.

> >> +
> >> +	timeout = wait_for_completion_timeout
> >> +			(&info->completion, EXYNOS_ADC_TIMEOUT);
> >> +	if (timeout == 0) {
> >> +		dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
> >> +		if (info->data->init_hw)
> >> +			info->data->init_hw(info);
> >> +		ret = -ETIMEDOUT;
> >> +	} else {
> >> +		*val = info->value;
> >> +		*val2 = info->value2;
> > This is definitely abuse as those two values are not intended for
> > different values.  If you want to do this please use different naming
> > and don't try to fiddle it into the IIO read raw framework.
> > As you've suggested above, better to simplify this code and drop the
> > bits cloned from the other handler.

Ok, adding ts_x and ts_y members.

> >> +		ret = IIO_VAL_INT;
> >> +	}
> >> +
> >> +	info->read_ts = 0;
> >> +	mutex_unlock(&indio_dev->mlock);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>   static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> >>   {
> >>   	struct exynos_adc *info = (struct exynos_adc *)dev_id;
> >>
> >>   	/* Read value */
> >> -	info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >> +	if (info->read_ts) {
> >> +		info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >> +		info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
> > ADC_DATY_MASK would be more obviously correct.

Agreed. I thought about it, but then kept it as it was in the original
driver. Will change now.

> >> +		writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
> > Perhaps the above is cryptic enough to warrant a comment?

This is also taken directly from the old driver, I have no idea what
it really does...

> >> +	/* data from s3c2410_ts driver */
> >> +	info->input->name = "S3C24xx TouchScreen";
> >> +	info->input->id.bustype = BUS_HOST;
> >> +	info->input->id.vendor = 0xDEAD;
> >> +	info->input->id.product = 0xBEEF;
> >> +	info->input->id.version = 0x0200;
> >> +
> >> +	ret = input_register_device(info->input);
> >> +	if (ret) {
> >> +		input_free_device(info->input);
> >> +		goto err;
> Just return ret, without goto (and get rid of label err).

ok

> >> +	}
> >> +
> >> +	if (info->tsirq > 0)
> >> +		ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
> >> +					0, "touchscreen", info);
> > info->tsirq
> > (that had me really confused for a moment ;)
> > Also, perhaps a more specific name.  touchscreen_updown or similar as the
> > main interrupt is also used during touchscreen operation.
> >> +	if (ret < 0) {
> >> +		dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
> >> +							info->irq);
> >> +		goto err_input;
> >> +	}
> >> +
> >> +	return 0;
> >> +
> Probably better to get rid of the labels and move the code up, as it is only used once.

Ok. I try not to mix goto and early return, so I've removed all the labels
here.

> >>   static int exynos_adc_probe(struct platform_device *pdev)
> >>   {
> >>   	struct exynos_adc *info = NULL;
> >>   	struct device_node *np = pdev->dev.of_node;
> >>   	struct iio_dev *indio_dev = NULL;
> >>   	struct resource	*mem;
> >> +	bool has_ts;
> >>   	int ret = -ENODEV;
> >>   	int irq;
> >>
> >> @@ -498,8 +638,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
> >>   		dev_err(&pdev->dev, "no irq resource?\n");
> >>   		return irq;
> >>   	}
> >> -
> >>   	info->irq = irq;
> >> +
> >> +	irq = platform_get_irq(pdev, 1);
> >> +	if (irq == -EPROBE_DEFER)
> >> +		return irq;
> What about other possible error codes?

We handle them later, in the request_threaded_irq call. In particular,
if the touchscreen is not used because either the "has-touchscreen" flag
is not set or because the input layer is not available, failing to get
the irq line should not be treated as an error.

Checking -EPROBE_DEFER however makes sense, so we get out of the probe function
early and don't have to undo and later retry everything.

Thanks everybody for the review!

	Arnd

Re: [PATCH 1/2] iio: exynos-adc: add support for s3c64xx adc

[Hartmut Knaack]

Arnd Bergmann schrieb:
> The ADC in s3c64xx is almost the same as exynosv1, but
> has a different 'select' method. Adding this here will be
> helpful to move over the existing s3c64xx platform from the
> legacy plat-samsung/adc driver to the new exynos-adc.
>
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> ---
There are some style issues, see the comments inline.
> [In reply to Exynos3250 ADC support, adding Heiko and Ben]
>
> I spent way too much time this week trying to clean up the
> old plat-samsung/adc.c driver as preparation for s3c64xx multiplatform
> support. Eventually I figured out that all that code is much simpler
> done using the new driver. This adds support for s3c64xx in
> samsung-adc.c, similar code changes can be done to support the
> various s3c24xx variants as well.
>
> This first patch should be fairly straightforward but is not tested
> yet. The second patch is more tricky.
>
> Both are based on the exynos3250 patches sent by Chanwoo Choi.
>
> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> index 26232f98d8c5..f84e9250429b 100644
> --- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> @@ -11,11 +11,21 @@ New driver handles the following
>  
>  Required properties:
>  - compatible:		Must be "samsung,exynos-adc-v1"
> -				for exynos4412/5250 controllers.
> +				for exynos4412/5250 and s5pv210 controllers.
>  			Must be "samsung,exynos-adc-v2" for
>  				future controllers.
>  			Must be "samsung,exynos3250-adc-v2" for
>  				controllers compatible with ADC of Exynos3250.
> +			Must be "samsung,s3c2410-adc" for
> +				the ADC in s3c2410 and compatibles
> +			Must be "samsung,s3c2416-adc" for
> +				the ADC in s3c2416 and compatibles
> +			Must be "samsung,s3c2440-adc" for
> +				the ADC in s3c2440 and compatibles
> +			Must be "samsung,s3c2440-adc" for
> +				the ADC in s3c2440 and compatibles
> +			Must be "samsung,s3c2443-adc" for
> +				the ADC in s3c2443 and compatibles
>  - reg:			Contains ADC register address range (base address and
>  			length) and the address of the phy enable register.
>  - interrupts: 		Contains the interrupt information for the timer. The
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index b63e88247eb2..5f95638513d2 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -39,12 +39,16 @@
>  #include <linux/iio/machine.h>
>  #include <linux/iio/driver.h>
>  
> -/* EXYNOS4412/5250 ADC_V1 registers definitions */
> +/* S3C/EXYNOS4412/5250 ADC_V1 registers definitions */
>  #define ADC_V1_CON(x)		((x) + 0x00)
> +#define ADC_V1_TSC(x)		((x) + 0x04)
>  #define ADC_V1_DLY(x)		((x) + 0x08)
>  #define ADC_V1_DATX(x)		((x) + 0x0C)
> +#define ADC_V1_DATY(x)		((x) + 0x10)
> +#define ADC_V1_UPDN(x)		((x) + 0x14)
>  #define ADC_V1_INTCLR(x)	((x) + 0x18)
>  #define ADC_V1_MUX(x)		((x) + 0x1c)
> +#define ADC_V1_CLRINTPNDNUP(x)	((x) + 0x20)
>  
>  /* Future ADC_V2 registers definitions */
>  #define ADC_V2_CON1(x)		((x) + 0x00)
> @@ -60,6 +64,30 @@
>  #define ADC_V1_CON_PRSCLV(x)	(((x) & 0xFF) << 6)
>  #define ADC_V1_CON_STANDBY	(1u << 2)
>  
Leave a whitespace around operators below.
> +#define ADC_S3C2410_CON_SELMUX(x) (((x)&0x7)<<3)
> +
> +/* ADCTSC Register Bits */
> +#define ADC_S3C2443_TSC_UD_SEN		(1<<8)
> +#define ADC_S3C2410_TSC_YM_SEN		(1<<7)
> +#define ADC_S3C2410_TSC_YP_SEN		(1<<6)
> +#define ADC_S3C2410_TSC_XM_SEN		(1<<5)
> +#define ADC_S3C2410_TSC_XP_SEN		(1<<4)
> +#define ADC_S3C2410_TSC_PULL_UP_DISABLE	(1<<3)
> +#define ADC_S3C2410_TSC_AUTO_PST	(1<<2)
> +#define ADC_S3C2410_TSC_XY_PST(x)	(((x)&0x3)<<0)
> +
> +#define ADC_TSC_WAIT4INT (ADC_S3C2410_TSC_YM_SEN | \
> +			 ADC_S3C2410_TSC_YP_SEN | \
> +			 ADC_S3C2410_TSC_XP_SEN | \
> +			 ADC_S3C2410_TSC_XY_PST(3))
> +
> +#define ADC_TSC_AUTOPST	(ADC_S3C2410_TSC_YM_SEN | \
> +			 ADC_S3C2410_TSC_YP_SEN | \
> +			 ADC_S3C2410_TSC_XP_SEN | \
> +			 ADC_S3C2410_TSC_AUTO_PST | \
> +			 ADC_S3C2410_TSC_XY_PST(0))
> +
> +
>  /* Bit definitions for ADC_V2 */
>  #define ADC_V2_CON1_SOFT_RESET	(1u << 2)
>  
> @@ -195,6 +223,26 @@ static void exynos_adc_v1_clear_irq(struct exynos_adc *info)
>  	writel(1, ADC_V1_INTCLR(info->regs));
>  }
>  
> +static void exynos_adc_s3c64xx_start_conv(struct exynos_adc *info,
> +				     unsigned long addr)
This indention could be a bit better.
> +{
> +	u32 con1;
> +
> +	con1 = readl(ADC_V1_CON(info->regs));
> +	con1 &= ~ADC_S3C2410_CON_SELMUX(7);
> +	con1 |= ADC_S3C2410_CON_SELMUX(addr);
> +	writel(con1 | ADC_CON_EN_START, ADC_V1_CON(info->regs));
> +}
> +
> +static struct exynos_adc_data const exynos_adc_s3c64xx_data = {
> +	.num_channels	= MAX_ADC_V1_CHANNELS,
> +
> +	.init_hw	= exynos_adc_v1_init_hw,
> +	.exit_hw	= exynos_adc_v1_exit_hw,
> +	.clear_irq	= exynos_adc_v1_clear_irq,
> +	.start_conv	= exynos_adc_s3c64xx_start_conv,
> +};
> +
>  static void exynos_adc_v1_start_conv(struct exynos_adc *info,
>  				     unsigned long addr)
>  {
> @@ -280,6 +328,9 @@ static struct exynos_adc_data const exynos3250_adc_v2_data = {
>  
>  static const struct of_device_id exynos_adc_match[] = {
>  	{
> +		.compatible = "samsung,s3c64100-adc",
> +		.data = &exynos_adc_s3c64xx_data,
> +	}, {
>  		.compatible = "samsung,exynos-adc-v1",
>  		.data = (void *)&exynos_adc_v1_data,
>  	}, {
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>