[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Marek Vasut]

This patch adds support for i.MX23 into the LRADC driver. The LRADC
block on MX23 is not much different from the one on MX28, thus this
is only a few changes fixing the parts that are specific to MX23.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/staging/iio/adc/Kconfig     |    4 +--
 drivers/staging/iio/adc/mxs-lradc.c |   56 +++++++++++++++++++++++++++++------
 2 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
index fb8c239..7b2a01d 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -119,12 +119,12 @@ config LPC32XX_ADC
 	  via sysfs.
 
 config MXS_LRADC
-	tristate "Freescale i.MX28 LRADC"
+	tristate "Freescale i.MX23/i.MX28 LRADC"
 	depends on ARCH_MXS
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 	help
-	  Say yes here to build support for i.MX28 LRADC convertor
+	  Say yes here to build support for i.MX23/i.MX28 LRADC convertor
 	  built into these chips.
 
 	  To compile this driver as a module, choose M here: the
diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 829d043..bea8372 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -76,7 +76,24 @@
  */
 #define LRADC_TS_SAMPLE_AMOUNT		4
 
-static const char * const mxs_lradc_irq_name[] = {
+enum mxs_lradc_id {
+	IMX23_LRADC,
+	IMX28_LRADC,
+};
+
+static const char * const mx23_lradc_irq_names[] = {
+	"mxs-lradc-touchscreen",
+	"mxs-lradc-channel0",
+	"mxs-lradc-channel1",
+	"mxs-lradc-channel2",
+	"mxs-lradc-channel3",
+	"mxs-lradc-channel4",
+	"mxs-lradc-channel5",
+	"mxs-lradc-channel6",
+	"mxs-lradc-channel7",
+};
+
+static const char * const mx28_lradc_irq_names[] = {
 	"mxs-lradc-touchscreen",
 	"mxs-lradc-thresh0",
 	"mxs-lradc-thresh1",
@@ -92,6 +109,22 @@ static const char * const mxs_lradc_irq_name[] = {
 	"mxs-lradc-button1",
 };
 
+struct mxs_lradc_of_config {
+	const int		irq_count;
+	const char * const	*irq_name;
+};
+
+static const struct mxs_lradc_of_config const mxs_lradc_of_config[] = {
+	[IMX23_LRADC] = {
+		.irq_count	= ARRAY_SIZE(mx23_lradc_irq_names),
+		.irq_name	= mx23_lradc_irq_names,
+	},
+	[IMX28_LRADC] = {
+		.irq_count	= ARRAY_SIZE(mx28_lradc_irq_names),
+		.irq_name	= mx28_lradc_irq_names,
+	},
+};
+
 enum mxs_lradc_ts {
 	MXS_LRADC_TOUCHSCREEN_NONE = 0,
 	MXS_LRADC_TOUCHSCREEN_4WIRE,
@@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
 		writel(0, lradc->base + LRADC_DELAY(i));
 }
 
+static const struct of_device_id mxs_lradc_dt_ids[] = {
+	{ .compatible = "fsl,imx23-lradc", .data = (void *)IMX23_LRADC, },
+	{ .compatible = "fsl,imx28-lradc", .data = (void *)IMX28_LRADC, },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
+
 static int mxs_lradc_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *of_id =
+		of_match_device(mxs_lradc_dt_ids, &pdev->dev);
+	const struct mxs_lradc_of_config *of_cfg =
+		&mxs_lradc_of_config[(enum mxs_lradc_id)of_id->data];
 	struct device *dev = &pdev->dev;
 	struct device_node *node = dev->of_node;
 	struct mxs_lradc *lradc;
@@ -902,7 +946,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
 				ts_wires);
 
 	/* Grab all IRQ sources */
-	for (i = 0; i < 13; i++) {
+	for (i = 0; i < of_cfg->irq_count; i++) {
 		lradc->irq[i] = platform_get_irq(pdev, i);
 		if (lradc->irq[i] < 0) {
 			ret = -EINVAL;
@@ -911,7 +955,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
 
 		ret = devm_request_irq(dev, lradc->irq[i],
 					mxs_lradc_handle_irq, 0,
-					mxs_lradc_irq_name[i], iio);
+					of_cfg->irq_name[i], iio);
 		if (ret)
 			goto err_addr;
 	}
@@ -983,12 +1027,6 @@ static int mxs_lradc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id mxs_lradc_dt_ids[] = {
-	{ .compatible = "fsl,imx28-lradc", },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
-
 static struct platform_driver mxs_lradc_driver = {
 	.driver	= {
 		.name	= DRIVER_NAME,
-- 
1.7.10.4

[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Marek Vasut]

This patch adds support for i.MX23 into the LRADC driver. The LRADC
block on MX23 is not much different from the one on MX28, thus this
is only a few changes fixing the parts that are specific to MX23.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/staging/iio/adc/Kconfig     |    4 +--
 drivers/staging/iio/adc/mxs-lradc.c |   56 +++++++++++++++++++++++++++++------
 2 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
index fb8c239..7b2a01d 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -119,12 +119,12 @@ config LPC32XX_ADC
 	  via sysfs.
 
 config MXS_LRADC
-	tristate "Freescale i.MX28 LRADC"
+	tristate "Freescale i.MX23/i.MX28 LRADC"
 	depends on ARCH_MXS
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 	help
-	  Say yes here to build support for i.MX28 LRADC convertor
+	  Say yes here to build support for i.MX23/i.MX28 LRADC convertor
 	  built into these chips.
 
 	  To compile this driver as a module, choose M here: the
diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 829d043..bea8372 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -76,7 +76,24 @@
  */
 #define LRADC_TS_SAMPLE_AMOUNT		4
 
-static const char * const mxs_lradc_irq_name[] = {
+enum mxs_lradc_id {
+	IMX23_LRADC,
+	IMX28_LRADC,
+};
+
+static const char * const mx23_lradc_irq_names[] = {
+	"mxs-lradc-touchscreen",
+	"mxs-lradc-channel0",
+	"mxs-lradc-channel1",
+	"mxs-lradc-channel2",
+	"mxs-lradc-channel3",
+	"mxs-lradc-channel4",
+	"mxs-lradc-channel5",
+	"mxs-lradc-channel6",
+	"mxs-lradc-channel7",
+};
+
+static const char * const mx28_lradc_irq_names[] = {
 	"mxs-lradc-touchscreen",
 	"mxs-lradc-thresh0",
 	"mxs-lradc-thresh1",
@@ -92,6 +109,22 @@ static const char * const mxs_lradc_irq_name[] = {
 	"mxs-lradc-button1",
 };
 
+struct mxs_lradc_of_config {
+	const int		irq_count;
+	const char * const	*irq_name;
+};
+
+static const struct mxs_lradc_of_config const mxs_lradc_of_config[] = {
+	[IMX23_LRADC] = {
+		.irq_count	= ARRAY_SIZE(mx23_lradc_irq_names),
+		.irq_name	= mx23_lradc_irq_names,
+	},
+	[IMX28_LRADC] = {
+		.irq_count	= ARRAY_SIZE(mx28_lradc_irq_names),
+		.irq_name	= mx28_lradc_irq_names,
+	},
+};
+
 enum mxs_lradc_ts {
 	MXS_LRADC_TOUCHSCREEN_NONE = 0,
 	MXS_LRADC_TOUCHSCREEN_4WIRE,
@@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
 		writel(0, lradc->base + LRADC_DELAY(i));
 }
 
+static const struct of_device_id mxs_lradc_dt_ids[] = {
+	{ .compatible = "fsl,imx23-lradc", .data = (void *)IMX23_LRADC, },
+	{ .compatible = "fsl,imx28-lradc", .data = (void *)IMX28_LRADC, },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
+
 static int mxs_lradc_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *of_id =
+		of_match_device(mxs_lradc_dt_ids, &pdev->dev);
+	const struct mxs_lradc_of_config *of_cfg =
+		&mxs_lradc_of_config[(enum mxs_lradc_id)of_id->data];
 	struct device *dev = &pdev->dev;
 	struct device_node *node = dev->of_node;
 	struct mxs_lradc *lradc;
@@ -902,7 +946,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
 				ts_wires);
 
 	/* Grab all IRQ sources */
-	for (i = 0; i < 13; i++) {
+	for (i = 0; i < of_cfg->irq_count; i++) {
 		lradc->irq[i] = platform_get_irq(pdev, i);
 		if (lradc->irq[i] < 0) {
 			ret = -EINVAL;
@@ -911,7 +955,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
 
 		ret = devm_request_irq(dev, lradc->irq[i],
 					mxs_lradc_handle_irq, 0,
-					mxs_lradc_irq_name[i], iio);
+					of_cfg->irq_name[i], iio);
 		if (ret)
 			goto err_addr;
 	}
@@ -983,12 +1027,6 @@ static int mxs_lradc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id mxs_lradc_dt_ids[] = {
-	{ .compatible = "fsl,imx28-lradc", },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
-
 static struct platform_driver mxs_lradc_driver = {
 	.driver	= {
 		.name	= DRIVER_NAME,
-- 
1.7.10.4

[PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC

[Marek Vasut]

Add interrupt mapping and compatible string for MX23 LRADC.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/boot/dts/imx23.dtsi |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/imx23.dtsi b/arch/arm/boot/dts/imx23.dtsi
index 65415c5..56afcf4 100644
--- a/arch/arm/boot/dts/imx23.dtsi
+++ b/arch/arm/boot/dts/imx23.dtsi
@@ -391,7 +391,9 @@
 			};
 
 			lradc@80050000 {
+				compatible = "fsl,imx23-lradc";
 				reg = <0x80050000 0x2000>;
+				interrupts = <36 37 38 39 40 41 42 43 44>;
 				status = "disabled";
 			};
 
-- 
1.7.10.4

[PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC

[Marek Vasut]

Add interrupt mapping and compatible string for MX23 LRADC.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/boot/dts/imx23.dtsi |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/imx23.dtsi b/arch/arm/boot/dts/imx23.dtsi
index 65415c5..56afcf4 100644
--- a/arch/arm/boot/dts/imx23.dtsi
+++ b/arch/arm/boot/dts/imx23.dtsi
@@ -391,7 +391,9 @@
 			};
 
 			lradc at 80050000 {
+				compatible = "fsl,imx23-lradc";
 				reg = <0x80050000 0x2000>;
+				interrupts = <36 37 38 39 40 41 42 43 44>;
 				status = "disabled";
 			};
 
-- 
1.7.10.4

Re: [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Michał Mirosław]

2013/1/21 Marek Vasut <marex@denx.de>:
> This patch adds support for i.MX23 into the LRADC driver. The LRADC
> block on MX23 is not much different from the one on MX28, thus this
> is only a few changes fixing the parts that are specific to MX23.
[...]
> +struct mxs_lradc_of_config {
> +       const int               irq_count;
> +       const char * const      *irq_name;
> +};
> +
> +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] = {
> +       [IMX23_LRADC] = {
> +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
> +               .irq_name       = mx23_lradc_irq_names,
> +       },
> +       [IMX28_LRADC] = {
> +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
> +               .irq_name       = mx28_lradc_irq_names,
> +       },
> +};
> +
>  enum mxs_lradc_ts {
>         MXS_LRADC_TOUCHSCREEN_NONE = 0,
>         MXS_LRADC_TOUCHSCREEN_4WIRE,
> @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
>                 writel(0, lradc->base + LRADC_DELAY(i));
>  }
>
> +static const struct of_device_id mxs_lradc_dt_ids[] = {
> +       { .compatible = "fsl,imx23-lradc", .data = (void *)IMX23_LRADC, },
> +       { .compatible = "fsl,imx28-lradc", .data = (void *)IMX28_LRADC, },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
> +

Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?

Best Regards,
Michał Mirosław

[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Michał Mirosław]

2013/1/21 Marek Vasut <marex@denx.de>:
> This patch adds support for i.MX23 into the LRADC driver. The LRADC
> block on MX23 is not much different from the one on MX28, thus this
> is only a few changes fixing the parts that are specific to MX23.
[...]
> +struct mxs_lradc_of_config {
> +       const int               irq_count;
> +       const char * const      *irq_name;
> +};
> +
> +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] = {
> +       [IMX23_LRADC] = {
> +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
> +               .irq_name       = mx23_lradc_irq_names,
> +       },
> +       [IMX28_LRADC] = {
> +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
> +               .irq_name       = mx28_lradc_irq_names,
> +       },
> +};
> +
>  enum mxs_lradc_ts {
>         MXS_LRADC_TOUCHSCREEN_NONE = 0,
>         MXS_LRADC_TOUCHSCREEN_4WIRE,
> @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
>                 writel(0, lradc->base + LRADC_DELAY(i));
>  }
>
> +static const struct of_device_id mxs_lradc_dt_ids[] = {
> +       { .compatible = "fsl,imx23-lradc", .data = (void *)IMX23_LRADC, },
> +       { .compatible = "fsl,imx28-lradc", .data = (void *)IMX28_LRADC, },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
> +

Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?

Best Regards,
Micha? Miros?aw

Re: [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Marek Vasut]

Dear Michał Mirosław,

> 2013/1/21 Marek Vasut <marex@denx.de>:
> > This patch adds support for i.MX23 into the LRADC driver. The LRADC
> > block on MX23 is not much different from the one on MX28, thus this
> > is only a few changes fixing the parts that are specific to MX23.
> 
> [...]
> 
> > +struct mxs_lradc_of_config {
> > +       const int               irq_count;
> > +       const char * const      *irq_name;
> > +};
> > +
> > +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] = {
> > +       [IMX23_LRADC] = {
> > +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
> > +               .irq_name       = mx23_lradc_irq_names,
> > +       },
> > +       [IMX28_LRADC] = {
> > +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
> > +               .irq_name       = mx28_lradc_irq_names,
> > +       },
> > +};
> > +
> > 
> >  enum mxs_lradc_ts {
> >  
> >         MXS_LRADC_TOUCHSCREEN_NONE = 0,
> >         MXS_LRADC_TOUCHSCREEN_4WIRE,
> > 
> > @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
> > *lradc)
> > 
> >                 writel(0, lradc->base + LRADC_DELAY(i));
> >  
> >  }
> > 
> > +static const struct of_device_id mxs_lradc_dt_ids[] = {
> > +       { .compatible = "fsl,imx23-lradc", .data = (void *)IMX23_LRADC,
> > }, +       { .compatible = "fsl,imx28-lradc", .data = (void
> > *)IMX28_LRADC, }, +       { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
> > +
> 
> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?

Check the register layout, it differs between MX23 and MX28, that's one reason, 
since were we to access differently placed registers, we can do it easily as in 
the SSP/I2C drivers.

Moreover, there are some features on the MX28 that are not on the MX23 (like 
voltage treshold triggers and touchbuttons), with this setup, we can easily 
check what we're running at at runtime and determine to disallow these.

>From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is much more 
convenient in the long run.

Best regards,
Marek Vasut

[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Marek Vasut]

Dear Micha? Miros?aw,

> 2013/1/21 Marek Vasut <marex@denx.de>:
> > This patch adds support for i.MX23 into the LRADC driver. The LRADC
> > block on MX23 is not much different from the one on MX28, thus this
> > is only a few changes fixing the parts that are specific to MX23.
> 
> [...]
> 
> > +struct mxs_lradc_of_config {
> > +       const int               irq_count;
> > +       const char * const      *irq_name;
> > +};
> > +
> > +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] = {
> > +       [IMX23_LRADC] = {
> > +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
> > +               .irq_name       = mx23_lradc_irq_names,
> > +       },
> > +       [IMX28_LRADC] = {
> > +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
> > +               .irq_name       = mx28_lradc_irq_names,
> > +       },
> > +};
> > +
> > 
> >  enum mxs_lradc_ts {
> >  
> >         MXS_LRADC_TOUCHSCREEN_NONE = 0,
> >         MXS_LRADC_TOUCHSCREEN_4WIRE,
> > 
> > @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
> > *lradc)
> > 
> >                 writel(0, lradc->base + LRADC_DELAY(i));
> >  
> >  }
> > 
> > +static const struct of_device_id mxs_lradc_dt_ids[] = {
> > +       { .compatible = "fsl,imx23-lradc", .data = (void *)IMX23_LRADC,
> > }, +       { .compatible = "fsl,imx28-lradc", .data = (void
> > *)IMX28_LRADC, }, +       { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
> > +
> 
> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?

Check the register layout, it differs between MX23 and MX28, that's one reason, 
since were we to access differently placed registers, we can do it easily as in 
the SSP/I2C drivers.

Moreover, there are some features on the MX28 that are not on the MX23 (like 
voltage treshold triggers and touchbuttons), with this setup, we can easily 
check what we're running at at runtime and determine to disallow these.

>From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is much more 
convenient in the long run.

Best regards,
Marek Vasut

Re: [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Michał Mirosław]

2013/1/21 Marek Vasut <marex@denx.de>:
> Dear Michał Mirosław,
>
>> 2013/1/21 Marek Vasut <marex@denx.de>:
>> > This patch adds support for i.MX23 into the LRADC driver. The LRADC
>> > block on MX23 is not much different from the one on MX28, thus this
>> > is only a few changes fixing the parts that are specific to MX23.
>>
>> [...]
>>
>> > +struct mxs_lradc_of_config {
>> > +       const int               irq_count;
>> > +       const char * const      *irq_name;
>> > +};
>> > +
>> > +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] = {
>> > +       [IMX23_LRADC] = {
>> > +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
>> > +               .irq_name       = mx23_lradc_irq_names,
>> > +       },
>> > +       [IMX28_LRADC] = {
>> > +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
>> > +               .irq_name       = mx28_lradc_irq_names,
>> > +       },
>> > +};
>> > +
>> >
>> >  enum mxs_lradc_ts {
>> >
>> >         MXS_LRADC_TOUCHSCREEN_NONE = 0,
>> >         MXS_LRADC_TOUCHSCREEN_4WIRE,
>> >
>> > @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
>> > *lradc)
>> >
>> >                 writel(0, lradc->base + LRADC_DELAY(i));
>> >
>> >  }
>> >
>> > +static const struct of_device_id mxs_lradc_dt_ids[] = {
>> > +       { .compatible = "fsl,imx23-lradc", .data = (void *)IMX23_LRADC,
>> > }, +       { .compatible = "fsl,imx28-lradc", .data = (void
>> > *)IMX28_LRADC, }, +       { /* sentinel */ }
>> > +};
>> > +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>> > +
>>
>> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
>
> Check the register layout, it differs between MX23 and MX28, that's one reason,
> since were we to access differently placed registers, we can do it easily as in
> the SSP/I2C drivers.
>
> Moreover, there are some features on the MX28 that are not on the MX23 (like
> voltage treshold triggers and touchbuttons), with this setup, we can easily
> check what we're running at at runtime and determine to disallow these.
>
> From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is much more
> convenient in the long run.

I'm asking, because you don't use this number anywhere other than in
mxs_lradc_probe()
and there only to dereference the irq-names table. After that the
structure and number
are forgotten.

Sure, it's just an insn or two, so no strong opinion here --- just curious.

Best Regards,
Michał Mirosław

[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Michał Mirosław]

2013/1/21 Marek Vasut <marex@denx.de>:
> Dear Micha? Miros?aw,
>
>> 2013/1/21 Marek Vasut <marex@denx.de>:
>> > This patch adds support for i.MX23 into the LRADC driver. The LRADC
>> > block on MX23 is not much different from the one on MX28, thus this
>> > is only a few changes fixing the parts that are specific to MX23.
>>
>> [...]
>>
>> > +struct mxs_lradc_of_config {
>> > +       const int               irq_count;
>> > +       const char * const      *irq_name;
>> > +};
>> > +
>> > +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] = {
>> > +       [IMX23_LRADC] = {
>> > +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
>> > +               .irq_name       = mx23_lradc_irq_names,
>> > +       },
>> > +       [IMX28_LRADC] = {
>> > +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
>> > +               .irq_name       = mx28_lradc_irq_names,
>> > +       },
>> > +};
>> > +
>> >
>> >  enum mxs_lradc_ts {
>> >
>> >         MXS_LRADC_TOUCHSCREEN_NONE = 0,
>> >         MXS_LRADC_TOUCHSCREEN_4WIRE,
>> >
>> > @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
>> > *lradc)
>> >
>> >                 writel(0, lradc->base + LRADC_DELAY(i));
>> >
>> >  }
>> >
>> > +static const struct of_device_id mxs_lradc_dt_ids[] = {
>> > +       { .compatible = "fsl,imx23-lradc", .data = (void *)IMX23_LRADC,
>> > }, +       { .compatible = "fsl,imx28-lradc", .data = (void
>> > *)IMX28_LRADC, }, +       { /* sentinel */ }
>> > +};
>> > +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>> > +
>>
>> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
>
> Check the register layout, it differs between MX23 and MX28, that's one reason,
> since were we to access differently placed registers, we can do it easily as in
> the SSP/I2C drivers.
>
> Moreover, there are some features on the MX28 that are not on the MX23 (like
> voltage treshold triggers and touchbuttons), with this setup, we can easily
> check what we're running at at runtime and determine to disallow these.
>
> From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is much more
> convenient in the long run.

I'm asking, because you don't use this number anywhere other than in
mxs_lradc_probe()
and there only to dereference the irq-names table. After that the
structure and number
are forgotten.

Sure, it's just an insn or two, so no strong opinion here --- just curious.

Best Regards,
Micha? Miros?aw

Re: [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Marek Vasut]

Dear Micha=C5=82 Miros=C5=82aw,

> 2013/1/21 Marek Vasut <marex@denx.de>:
> > Dear Micha=C5=82 Miros=C5=82aw,
> >=20
> >> 2013/1/21 Marek Vasut <marex@denx.de>:
> >> > This patch adds support for i.MX23 into the LRADC driver. The LRADC
> >> > block on MX23 is not much different from the one on MX28, thus this
> >> > is only a few changes fixing the parts that are specific to MX23.
> >>=20
> >> [...]
> >>=20
> >> > +struct mxs_lradc_of_config {
> >> > +       const int               irq_count;
> >> > +       const char * const      *irq_name;
> >> > +};
> >> > +
> >> > +static const struct mxs_lradc_of_config const mxs_lradc_of_config[]=
 =3D
> >> > { +       [IMX23_LRADC] =3D {
> >> > +               .irq_count      =3D ARRAY_SIZE(mx23_lradc_irq_names),
> >> > +               .irq_name       =3D mx23_lradc_irq_names,
> >> > +       },
> >> > +       [IMX28_LRADC] =3D {
> >> > +               .irq_count      =3D ARRAY_SIZE(mx28_lradc_irq_names),
> >> > +               .irq_name       =3D mx28_lradc_irq_names,
> >> > +       },
> >> > +};
> >> > +
> >> >=20
> >> >  enum mxs_lradc_ts {
> >> > =20
> >> >         MXS_LRADC_TOUCHSCREEN_NONE =3D 0,
> >> >         MXS_LRADC_TOUCHSCREEN_4WIRE,
> >> >=20
> >> > @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
> >> > *lradc)
> >> >=20
> >> >                 writel(0, lradc->base + LRADC_DELAY(i));
> >> > =20
> >> >  }
> >> >=20
> >> > +static const struct of_device_id mxs_lradc_dt_ids[] =3D {
> >> > +       { .compatible =3D "fsl,imx23-lradc", .data =3D (void
> >> > *)IMX23_LRADC, }, +       { .compatible =3D "fsl,imx28-lradc", .data=
 =3D
> >> > (void
> >> > *)IMX28_LRADC, }, +       { /* sentinel */ }
> >> > +};
> >> > +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
> >> > +
> >>=20
> >> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
> >=20
> > Check the register layout, it differs between MX23 and MX28, that's one
> > reason, since were we to access differently placed registers, we can do
> > it easily as in the SSP/I2C drivers.
> >=20
> > Moreover, there are some features on the MX28 that are not on the MX23
> > (like voltage treshold triggers and touchbuttons), with this setup, we
> > can easily check what we're running at at runtime and determine to
> > disallow these.
> >=20
> > From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
> > much more convenient in the long run.
>=20
> I'm asking, because you don't use this number anywhere other than in
> mxs_lradc_probe()
> and there only to dereference the irq-names table. After that the
> structure and number
> are forgotten.

Certainly, so far it's used only this way. But please see my argument about=
=20
register layout, that's why I went down this road of abstraction.

> Sure, it's just an insn or two, so no strong opinion here --- just curiou=
s.

I tried to put it down above ;-)

> Best Regards,
> Micha=C5=82 Miros=C5=82aw

Best regards,
Marek Vasut

[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Marek Vasut]

Dear Micha? Miros?aw,

> 2013/1/21 Marek Vasut <marex@denx.de>:
> > Dear Micha? Miros?aw,
> > 
> >> 2013/1/21 Marek Vasut <marex@denx.de>:
> >> > This patch adds support for i.MX23 into the LRADC driver. The LRADC
> >> > block on MX23 is not much different from the one on MX28, thus this
> >> > is only a few changes fixing the parts that are specific to MX23.
> >> 
> >> [...]
> >> 
> >> > +struct mxs_lradc_of_config {
> >> > +       const int               irq_count;
> >> > +       const char * const      *irq_name;
> >> > +};
> >> > +
> >> > +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] =
> >> > { +       [IMX23_LRADC] = {
> >> > +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
> >> > +               .irq_name       = mx23_lradc_irq_names,
> >> > +       },
> >> > +       [IMX28_LRADC] = {
> >> > +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
> >> > +               .irq_name       = mx28_lradc_irq_names,
> >> > +       },
> >> > +};
> >> > +
> >> > 
> >> >  enum mxs_lradc_ts {
> >> >  
> >> >         MXS_LRADC_TOUCHSCREEN_NONE = 0,
> >> >         MXS_LRADC_TOUCHSCREEN_4WIRE,
> >> > 
> >> > @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
> >> > *lradc)
> >> > 
> >> >                 writel(0, lradc->base + LRADC_DELAY(i));
> >> >  
> >> >  }
> >> > 
> >> > +static const struct of_device_id mxs_lradc_dt_ids[] = {
> >> > +       { .compatible = "fsl,imx23-lradc", .data = (void
> >> > *)IMX23_LRADC, }, +       { .compatible = "fsl,imx28-lradc", .data =
> >> > (void
> >> > *)IMX28_LRADC, }, +       { /* sentinel */ }
> >> > +};
> >> > +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
> >> > +
> >> 
> >> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
> > 
> > Check the register layout, it differs between MX23 and MX28, that's one
> > reason, since were we to access differently placed registers, we can do
> > it easily as in the SSP/I2C drivers.
> > 
> > Moreover, there are some features on the MX28 that are not on the MX23
> > (like voltage treshold triggers and touchbuttons), with this setup, we
> > can easily check what we're running at at runtime and determine to
> > disallow these.
> > 
> > From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
> > much more convenient in the long run.
> 
> I'm asking, because you don't use this number anywhere other than in
> mxs_lradc_probe()
> and there only to dereference the irq-names table. After that the
> structure and number
> are forgotten.

Certainly, so far it's used only this way. But please see my argument about 
register layout, that's why I went down this road of abstraction.

> Sure, it's just an insn or two, so no strong opinion here --- just curious.

I tried to put it down above ;-)

> Best Regards,
> Micha? Miros?aw

Best regards,
Marek Vasut

Re: [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Lars-Peter Clausen]

On 01/21/2013 10:32 PM, Marek Vasut wrote:
> Dear Michał Mirosław,
> 
>> 2013/1/21 Marek Vasut <marex@denx.de>:
>>> Dear Michał Mirosław,
>>>
>>>> 2013/1/21 Marek Vasut <marex@denx.de>:
>>>>> This patch adds support for i.MX23 into the LRADC driver. The LRADC
>>>>> block on MX23 is not much different from the one on MX28, thus this
>>>>> is only a few changes fixing the parts that are specific to MX23.
>>>>
>>>> [...]
>>>>
>>>>> +struct mxs_lradc_of_config {
>>>>> +       const int               irq_count;
>>>>> +       const char * const      *irq_name;
>>>>> +};
>>>>> +
>>>>> +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] =
>>>>> { +       [IMX23_LRADC] = {
>>>>> +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
>>>>> +               .irq_name       = mx23_lradc_irq_names,
>>>>> +       },
>>>>> +       [IMX28_LRADC] = {
>>>>> +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
>>>>> +               .irq_name       = mx28_lradc_irq_names,
>>>>> +       },
>>>>> +};
>>>>> +
>>>>>
>>>>>  enum mxs_lradc_ts {
>>>>>  
>>>>>         MXS_LRADC_TOUCHSCREEN_NONE = 0,
>>>>>         MXS_LRADC_TOUCHSCREEN_4WIRE,
>>>>>
>>>>> @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
>>>>> *lradc)
>>>>>
>>>>>                 writel(0, lradc->base + LRADC_DELAY(i));
>>>>>  
>>>>>  }
>>>>>
>>>>> +static const struct of_device_id mxs_lradc_dt_ids[] = {
>>>>> +       { .compatible = "fsl,imx23-lradc", .data = (void
>>>>> *)IMX23_LRADC, }, +       { .compatible = "fsl,imx28-lradc", .data =
>>>>> (void
>>>>> *)IMX28_LRADC, }, +       { /* sentinel */ }
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>>>>> +
>>>>
>>>> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
>>>
>>> Check the register layout, it differs between MX23 and MX28, that's one
>>> reason, since were we to access differently placed registers, we can do
>>> it easily as in the SSP/I2C drivers.
>>>
>>> Moreover, there are some features on the MX28 that are not on the MX23
>>> (like voltage treshold triggers and touchbuttons), with this setup, we
>>> can easily check what we're running at at runtime and determine to
>>> disallow these.
>>>
>>> From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
>>> much more convenient in the long run.
>>
>> I'm asking, because you don't use this number anywhere other than in
>> mxs_lradc_probe()
>> and there only to dereference the irq-names table. After that the
>> structure and number
>> are forgotten.
> 
> Certainly, so far it's used only this way. But please see my argument about 
> register layout, that's why I went down this road of abstraction.

You'll probably be better of by putting these differences into the
mxs_lradc_of_config struct as well, instead of adding switch statements here
and there throughout the code.

- Lars

[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Lars-Peter Clausen]

On 01/21/2013 10:32 PM, Marek Vasut wrote:
> Dear Micha? Miros?aw,
> 
>> 2013/1/21 Marek Vasut <marex@denx.de>:
>>> Dear Micha? Miros?aw,
>>>
>>>> 2013/1/21 Marek Vasut <marex@denx.de>:
>>>>> This patch adds support for i.MX23 into the LRADC driver. The LRADC
>>>>> block on MX23 is not much different from the one on MX28, thus this
>>>>> is only a few changes fixing the parts that are specific to MX23.
>>>>
>>>> [...]
>>>>
>>>>> +struct mxs_lradc_of_config {
>>>>> +       const int               irq_count;
>>>>> +       const char * const      *irq_name;
>>>>> +};
>>>>> +
>>>>> +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] =
>>>>> { +       [IMX23_LRADC] = {
>>>>> +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
>>>>> +               .irq_name       = mx23_lradc_irq_names,
>>>>> +       },
>>>>> +       [IMX28_LRADC] = {
>>>>> +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
>>>>> +               .irq_name       = mx28_lradc_irq_names,
>>>>> +       },
>>>>> +};
>>>>> +
>>>>>
>>>>>  enum mxs_lradc_ts {
>>>>>  
>>>>>         MXS_LRADC_TOUCHSCREEN_NONE = 0,
>>>>>         MXS_LRADC_TOUCHSCREEN_4WIRE,
>>>>>
>>>>> @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
>>>>> *lradc)
>>>>>
>>>>>                 writel(0, lradc->base + LRADC_DELAY(i));
>>>>>  
>>>>>  }
>>>>>
>>>>> +static const struct of_device_id mxs_lradc_dt_ids[] = {
>>>>> +       { .compatible = "fsl,imx23-lradc", .data = (void
>>>>> *)IMX23_LRADC, }, +       { .compatible = "fsl,imx28-lradc", .data =
>>>>> (void
>>>>> *)IMX28_LRADC, }, +       { /* sentinel */ }
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>>>>> +
>>>>
>>>> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
>>>
>>> Check the register layout, it differs between MX23 and MX28, that's one
>>> reason, since were we to access differently placed registers, we can do
>>> it easily as in the SSP/I2C drivers.
>>>
>>> Moreover, there are some features on the MX28 that are not on the MX23
>>> (like voltage treshold triggers and touchbuttons), with this setup, we
>>> can easily check what we're running at at runtime and determine to
>>> disallow these.
>>>
>>> From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
>>> much more convenient in the long run.
>>
>> I'm asking, because you don't use this number anywhere other than in
>> mxs_lradc_probe()
>> and there only to dereference the irq-names table. After that the
>> structure and number
>> are forgotten.
> 
> Certainly, so far it's used only this way. But please see my argument about 
> register layout, that's why I went down this road of abstraction.

You'll probably be better of by putting these differences into the
mxs_lradc_of_config struct as well, instead of adding switch statements here
and there throughout the code.

- Lars

Re: [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Michał Mirosław]

2013/1/21 Marek Vasut <marex@denx.de>:
> Dear Micha=C5=82 Miros=C5=82aw,
>> 2013/1/21 Marek Vasut <marex@denx.de>:
>> > Dear Micha=C5=82 Miros=C5=82aw,
>> >> 2013/1/21 Marek Vasut <marex@denx.de>:
>> >> > This patch adds support for i.MX23 into the LRADC driver. The LRADC
>> >> > block on MX23 is not much different from the one on MX28, thus this
>> >> > is only a few changes fixing the parts that are specific to MX23.
>> >>
>> >> [...]
>> >>
>> >> > +struct mxs_lradc_of_config {
>> >> > +       const int               irq_count;
>> >> > +       const char * const      *irq_name;
>> >> > +};
>> >> > +
>> >> > +static const struct mxs_lradc_of_config const mxs_lradc_of_config[=
] =3D
>> >> > { +       [IMX23_LRADC] =3D {
>> >> > +               .irq_count      =3D ARRAY_SIZE(mx23_lradc_irq_names=
),
>> >> > +               .irq_name       =3D mx23_lradc_irq_names,
>> >> > +       },
>> >> > +       [IMX28_LRADC] =3D {
>> >> > +               .irq_count      =3D ARRAY_SIZE(mx28_lradc_irq_names=
),
>> >> > +               .irq_name       =3D mx28_lradc_irq_names,
>> >> > +       },
>> >> > +};
>> >> > +
>> >> >
>> >> >  enum mxs_lradc_ts {
>> >> >
>> >> >         MXS_LRADC_TOUCHSCREEN_NONE =3D 0,
>> >> >         MXS_LRADC_TOUCHSCREEN_4WIRE,
>> >> >
>> >> > @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
>> >> > *lradc)
>> >> >
>> >> >                 writel(0, lradc->base + LRADC_DELAY(i));
>> >> >
>> >> >  }
>> >> >
>> >> > +static const struct of_device_id mxs_lradc_dt_ids[] =3D {
>> >> > +       { .compatible =3D "fsl,imx23-lradc", .data =3D (void
>> >> > *)IMX23_LRADC, }, +       { .compatible =3D "fsl,imx28-lradc", .dat=
a =3D
>> >> > (void
>> >> > *)IMX28_LRADC, }, +       { /* sentinel */ }
>> >> > +};
>> >> > +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>> >> > +
>> >>
>> >> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
>> >
>> > Check the register layout, it differs between MX23 and MX28, that's on=
e
>> > reason, since were we to access differently placed registers, we can d=
o
>> > it easily as in the SSP/I2C drivers.
>> >
>> > Moreover, there are some features on the MX28 that are not on the MX23
>> > (like voltage treshold triggers and touchbuttons), with this setup, we
>> > can easily check what we're running at at runtime and determine to
>> > disallow these.
>> >
>> > From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
>> > much more convenient in the long run.
>>
>> I'm asking, because you don't use this number anywhere other than in
>> mxs_lradc_probe()
>> and there only to dereference the irq-names table. After that the
>> structure and number
>> are forgotten.
>
> Certainly, so far it's used only this way. But please see my argument abo=
ut
> register layout, that's why I went down this road of abstraction.

Hmm. Then is IMX23 LRADC going to just work after this series
(assuming I don't use unsupported features) or this needs more patches
to be usable?

Best Regards,
Micha=C5=82 Miros=C5=82aw

[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Michał Mirosław]

2013/1/21 Marek Vasut <marex@denx.de>:
> Dear Micha? Miros?aw,
>> 2013/1/21 Marek Vasut <marex@denx.de>:
>> > Dear Micha? Miros?aw,
>> >> 2013/1/21 Marek Vasut <marex@denx.de>:
>> >> > This patch adds support for i.MX23 into the LRADC driver. The LRADC
>> >> > block on MX23 is not much different from the one on MX28, thus this
>> >> > is only a few changes fixing the parts that are specific to MX23.
>> >>
>> >> [...]
>> >>
>> >> > +struct mxs_lradc_of_config {
>> >> > +       const int               irq_count;
>> >> > +       const char * const      *irq_name;
>> >> > +};
>> >> > +
>> >> > +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] =
>> >> > { +       [IMX23_LRADC] = {
>> >> > +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
>> >> > +               .irq_name       = mx23_lradc_irq_names,
>> >> > +       },
>> >> > +       [IMX28_LRADC] = {
>> >> > +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
>> >> > +               .irq_name       = mx28_lradc_irq_names,
>> >> > +       },
>> >> > +};
>> >> > +
>> >> >
>> >> >  enum mxs_lradc_ts {
>> >> >
>> >> >         MXS_LRADC_TOUCHSCREEN_NONE = 0,
>> >> >         MXS_LRADC_TOUCHSCREEN_4WIRE,
>> >> >
>> >> > @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
>> >> > *lradc)
>> >> >
>> >> >                 writel(0, lradc->base + LRADC_DELAY(i));
>> >> >
>> >> >  }
>> >> >
>> >> > +static const struct of_device_id mxs_lradc_dt_ids[] = {
>> >> > +       { .compatible = "fsl,imx23-lradc", .data = (void
>> >> > *)IMX23_LRADC, }, +       { .compatible = "fsl,imx28-lradc", .data =
>> >> > (void
>> >> > *)IMX28_LRADC, }, +       { /* sentinel */ }
>> >> > +};
>> >> > +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>> >> > +
>> >>
>> >> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
>> >
>> > Check the register layout, it differs between MX23 and MX28, that's one
>> > reason, since were we to access differently placed registers, we can do
>> > it easily as in the SSP/I2C drivers.
>> >
>> > Moreover, there are some features on the MX28 that are not on the MX23
>> > (like voltage treshold triggers and touchbuttons), with this setup, we
>> > can easily check what we're running at at runtime and determine to
>> > disallow these.
>> >
>> > From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
>> > much more convenient in the long run.
>>
>> I'm asking, because you don't use this number anywhere other than in
>> mxs_lradc_probe()
>> and there only to dereference the irq-names table. After that the
>> structure and number
>> are forgotten.
>
> Certainly, so far it's used only this way. But please see my argument about
> register layout, that's why I went down this road of abstraction.

Hmm. Then is IMX23 LRADC going to just work after this series
(assuming I don't use unsupported features) or this needs more patches
to be usable?

Best Regards,
Micha? Miros?aw

Re: [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Marek Vasut]

Dear Lars-Peter Clausen,

> On 01/21/2013 10:32 PM, Marek Vasut wrote:
> > Dear Micha=C5=82 Miros=C5=82aw,
> >=20
> >> 2013/1/21 Marek Vasut <marex@denx.de>:
> >>> Dear Micha=C5=82 Miros=C5=82aw,
> >>>=20
> >>>> 2013/1/21 Marek Vasut <marex@denx.de>:
> >>>>> This patch adds support for i.MX23 into the LRADC driver. The LRADC
> >>>>> block on MX23 is not much different from the one on MX28, thus this
> >>>>> is only a few changes fixing the parts that are specific to MX23.
> >>>>=20
> >>>> [...]
> >>>>=20
> >>>>> +struct mxs_lradc_of_config {
> >>>>> +       const int               irq_count;
> >>>>> +       const char * const      *irq_name;
> >>>>> +};
> >>>>> +
> >>>>> +static const struct mxs_lradc_of_config const mxs_lradc_of_config[]
> >>>>> =3D { +       [IMX23_LRADC] =3D {
> >>>>> +               .irq_count      =3D ARRAY_SIZE(mx23_lradc_irq_names=
),
> >>>>> +               .irq_name       =3D mx23_lradc_irq_names,
> >>>>> +       },
> >>>>> +       [IMX28_LRADC] =3D {
> >>>>> +               .irq_count      =3D ARRAY_SIZE(mx28_lradc_irq_names=
),
> >>>>> +               .irq_name       =3D mx28_lradc_irq_names,
> >>>>> +       },
> >>>>> +};
> >>>>> +
> >>>>>=20
> >>>>>  enum mxs_lradc_ts {
> >>>>> =20
> >>>>>         MXS_LRADC_TOUCHSCREEN_NONE =3D 0,
> >>>>>         MXS_LRADC_TOUCHSCREEN_4WIRE,
> >>>>>=20
> >>>>> @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
> >>>>> *lradc)
> >>>>>=20
> >>>>>                 writel(0, lradc->base + LRADC_DELAY(i));
> >>>>> =20
> >>>>>  }
> >>>>>=20
> >>>>> +static const struct of_device_id mxs_lradc_dt_ids[] =3D {
> >>>>> +       { .compatible =3D "fsl,imx23-lradc", .data =3D (void
> >>>>> *)IMX23_LRADC, }, +       { .compatible =3D "fsl,imx28-lradc", .dat=
a =3D
> >>>>> (void
> >>>>> *)IMX28_LRADC, }, +       { /* sentinel */ }
> >>>>> +};
> >>>>> +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
> >>>>> +
> >>>>=20
> >>>> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
> >>>=20
> >>> Check the register layout, it differs between MX23 and MX28, that's o=
ne
> >>> reason, since were we to access differently placed registers, we can =
do
> >>> it easily as in the SSP/I2C drivers.
> >>>=20
> >>> Moreover, there are some features on the MX28 that are not on the MX23
> >>> (like voltage treshold triggers and touchbuttons), with this setup, we
> >>> can easily check what we're running at at runtime and determine to
> >>> disallow these.
> >>>=20
> >>> From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
> >>> much more convenient in the long run.
> >>=20
> >> I'm asking, because you don't use this number anywhere other than in
> >> mxs_lradc_probe()
> >> and there only to dereference the irq-names table. After that the
> >> structure and number
> >> are forgotten.
> >=20
> > Certainly, so far it's used only this way. But please see my argument
> > about register layout, that's why I went down this road of abstraction.
>=20
> You'll probably be better of by putting these differences into the
> mxs_lradc_of_config struct as well, instead of adding switch statements
> here and there throughout the code.

Certainly. All that is needed is in place now.

Best regards,
Marek Vasut

[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Marek Vasut]

Dear Lars-Peter Clausen,

> On 01/21/2013 10:32 PM, Marek Vasut wrote:
> > Dear Micha? Miros?aw,
> > 
> >> 2013/1/21 Marek Vasut <marex@denx.de>:
> >>> Dear Micha? Miros?aw,
> >>> 
> >>>> 2013/1/21 Marek Vasut <marex@denx.de>:
> >>>>> This patch adds support for i.MX23 into the LRADC driver. The LRADC
> >>>>> block on MX23 is not much different from the one on MX28, thus this
> >>>>> is only a few changes fixing the parts that are specific to MX23.
> >>>> 
> >>>> [...]
> >>>> 
> >>>>> +struct mxs_lradc_of_config {
> >>>>> +       const int               irq_count;
> >>>>> +       const char * const      *irq_name;
> >>>>> +};
> >>>>> +
> >>>>> +static const struct mxs_lradc_of_config const mxs_lradc_of_config[]
> >>>>> = { +       [IMX23_LRADC] = {
> >>>>> +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
> >>>>> +               .irq_name       = mx23_lradc_irq_names,
> >>>>> +       },
> >>>>> +       [IMX28_LRADC] = {
> >>>>> +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
> >>>>> +               .irq_name       = mx28_lradc_irq_names,
> >>>>> +       },
> >>>>> +};
> >>>>> +
> >>>>> 
> >>>>>  enum mxs_lradc_ts {
> >>>>>  
> >>>>>         MXS_LRADC_TOUCHSCREEN_NONE = 0,
> >>>>>         MXS_LRADC_TOUCHSCREEN_4WIRE,
> >>>>> 
> >>>>> @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
> >>>>> *lradc)
> >>>>> 
> >>>>>                 writel(0, lradc->base + LRADC_DELAY(i));
> >>>>>  
> >>>>>  }
> >>>>> 
> >>>>> +static const struct of_device_id mxs_lradc_dt_ids[] = {
> >>>>> +       { .compatible = "fsl,imx23-lradc", .data = (void
> >>>>> *)IMX23_LRADC, }, +       { .compatible = "fsl,imx28-lradc", .data =
> >>>>> (void
> >>>>> *)IMX28_LRADC, }, +       { /* sentinel */ }
> >>>>> +};
> >>>>> +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
> >>>>> +
> >>>> 
> >>>> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
> >>> 
> >>> Check the register layout, it differs between MX23 and MX28, that's one
> >>> reason, since were we to access differently placed registers, we can do
> >>> it easily as in the SSP/I2C drivers.
> >>> 
> >>> Moreover, there are some features on the MX28 that are not on the MX23
> >>> (like voltage treshold triggers and touchbuttons), with this setup, we
> >>> can easily check what we're running at at runtime and determine to
> >>> disallow these.
> >>> 
> >>> From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
> >>> much more convenient in the long run.
> >> 
> >> I'm asking, because you don't use this number anywhere other than in
> >> mxs_lradc_probe()
> >> and there only to dereference the irq-names table. After that the
> >> structure and number
> >> are forgotten.
> > 
> > Certainly, so far it's used only this way. But please see my argument
> > about register layout, that's why I went down this road of abstraction.
> 
> You'll probably be better of by putting these differences into the
> mxs_lradc_of_config struct as well, instead of adding switch statements
> here and there throughout the code.

Certainly. All that is needed is in place now.

Best regards,
Marek Vasut

Re: [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Marek Vasut]

Dear Michał Mirosław,

[...]

> >> I'm asking, because you don't use this number anywhere other than in
> >> mxs_lradc_probe()
> >> and there only to dereference the irq-names table. After that the
> >> structure and number
> >> are forgotten.
> > 
> > Certainly, so far it's used only this way. But please see my argument
> > about register layout, that's why I went down this road of abstraction.
> 
> Hmm. Then is IMX23 LRADC going to just work after this series
> (assuming I don't use unsupported features) or this needs more patches
> to be usable?

It's gonna work, touchscreen will work as well. You won't have the touchbuttons, 
but when these're gonna be implemented, we will need to be careful to enable 
them only for mx28.

Best regards,
Marek Vasut

[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Marek Vasut]

Dear Micha? Miros?aw,

[...]

> >> I'm asking, because you don't use this number anywhere other than in
> >> mxs_lradc_probe()
> >> and there only to dereference the irq-names table. After that the
> >> structure and number
> >> are forgotten.
> > 
> > Certainly, so far it's used only this way. But please see my argument
> > about register layout, that's why I went down this road of abstraction.
> 
> Hmm. Then is IMX23 LRADC going to just work after this series
> (assuming I don't use unsupported features) or this needs more patches
> to be usable?

It's gonna work, touchscreen will work as well. You won't have the touchbuttons, 
but when these're gonna be implemented, we will need to be careful to enable 
them only for mx28.

Best regards,
Marek Vasut

Re: [PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC

[Jonathan Cameron]

On 01/21/2013 08:05 PM, Marek Vasut wrote:
> Add interrupt mapping and compatible string for MX23 LRADC.

Series looks fine to me, but obviously I'd like an ACK for this one
from Shawn before taking it.

Thanks,

Jonathan

> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/boot/dts/imx23.dtsi |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx23.dtsi b/arch/arm/boot/dts/imx23.dtsi
> index 65415c5..56afcf4 100644
> --- a/arch/arm/boot/dts/imx23.dtsi
> +++ b/arch/arm/boot/dts/imx23.dtsi
> @@ -391,7 +391,9 @@
>  			};
>  
>  			lradc@80050000 {
> +				compatible = "fsl,imx23-lradc";
>  				reg = <0x80050000 0x2000>;
> +				interrupts = <36 37 38 39 40 41 42 43 44>;
>  				status = "disabled";
>  			};
>  
> 

[PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC

[Jonathan Cameron]

On 01/21/2013 08:05 PM, Marek Vasut wrote:
> Add interrupt mapping and compatible string for MX23 LRADC.

Series looks fine to me, but obviously I'd like an ACK for this one
from Shawn before taking it.

Thanks,

Jonathan

> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/boot/dts/imx23.dtsi |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx23.dtsi b/arch/arm/boot/dts/imx23.dtsi
> index 65415c5..56afcf4 100644
> --- a/arch/arm/boot/dts/imx23.dtsi
> +++ b/arch/arm/boot/dts/imx23.dtsi
> @@ -391,7 +391,9 @@
>  			};
>  
>  			lradc at 80050000 {
> +				compatible = "fsl,imx23-lradc";
>  				reg = <0x80050000 0x2000>;
> +				interrupts = <36 37 38 39 40 41 42 43 44>;
>  				status = "disabled";
>  			};
>  
> 

Re: [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Jonathan Cameron]

On 01/21/2013 09:49 PM, Marek Vasut wrote:
> Dear Lars-Peter Clausen,
> 
>> On 01/21/2013 10:32 PM, Marek Vasut wrote:
>>> Dear Michał Mirosław,
>>>
>>>> 2013/1/21 Marek Vasut <marex@denx.de>:
>>>>> Dear Michał Mirosław,
>>>>>
>>>>>> 2013/1/21 Marek Vasut <marex@denx.de>:
>>>>>>> This patch adds support for i.MX23 into the LRADC driver. The LRADC
>>>>>>> block on MX23 is not much different from the one on MX28, thus this
>>>>>>> is only a few changes fixing the parts that are specific to MX23.
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +struct mxs_lradc_of_config {
>>>>>>> +       const int               irq_count;
>>>>>>> +       const char * const      *irq_name;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static const struct mxs_lradc_of_config const mxs_lradc_of_config[]
>>>>>>> = { +       [IMX23_LRADC] = {
>>>>>>> +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
>>>>>>> +               .irq_name       = mx23_lradc_irq_names,
>>>>>>> +       },
>>>>>>> +       [IMX28_LRADC] = {
>>>>>>> +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
>>>>>>> +               .irq_name       = mx28_lradc_irq_names,
>>>>>>> +       },
>>>>>>> +};
>>>>>>> +
>>>>>>>
>>>>>>>  enum mxs_lradc_ts {
>>>>>>>  
>>>>>>>         MXS_LRADC_TOUCHSCREEN_NONE = 0,
>>>>>>>         MXS_LRADC_TOUCHSCREEN_4WIRE,
>>>>>>>
>>>>>>> @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
>>>>>>> *lradc)
>>>>>>>
>>>>>>>                 writel(0, lradc->base + LRADC_DELAY(i));
>>>>>>>  
>>>>>>>  }
>>>>>>>
>>>>>>> +static const struct of_device_id mxs_lradc_dt_ids[] = {
>>>>>>> +       { .compatible = "fsl,imx23-lradc", .data = (void
>>>>>>> *)IMX23_LRADC, }, +       { .compatible = "fsl,imx28-lradc", .data =
>>>>>>> (void
>>>>>>> *)IMX28_LRADC, }, +       { /* sentinel */ }
>>>>>>> +};
>>>>>>> +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>>>>>>> +
>>>>>>
>>>>>> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
>>>>>
>>>>> Check the register layout, it differs between MX23 and MX28, that's one
>>>>> reason, since were we to access differently placed registers, we can do
>>>>> it easily as in the SSP/I2C drivers.
>>>>>
>>>>> Moreover, there are some features on the MX28 that are not on the MX23
>>>>> (like voltage treshold triggers and touchbuttons), with this setup, we
>>>>> can easily check what we're running at at runtime and determine to
>>>>> disallow these.
>>>>>
>>>>> From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
>>>>> much more convenient in the long run.
>>>>
>>>> I'm asking, because you don't use this number anywhere other than in
>>>> mxs_lradc_probe()
>>>> and there only to dereference the irq-names table. After that the
>>>> structure and number
>>>> are forgotten.
>>>
>>> Certainly, so far it's used only this way. But please see my argument
>>> about register layout, that's why I went down this road of abstraction.
>>
>> You'll probably be better of by putting these differences into the
>> mxs_lradc_of_config struct as well, instead of adding switch statements
>> here and there throughout the code.
> 
> Certainly. All that is needed is in place now.
> 
All look sane to me and Marek has answered all the questions as far as I can see.
I'll take these once I get a response from Shawn (unless someone convinces me
otherwise ;)	

[PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver

[Jonathan Cameron]

On 01/21/2013 09:49 PM, Marek Vasut wrote:
> Dear Lars-Peter Clausen,
> 
>> On 01/21/2013 10:32 PM, Marek Vasut wrote:
>>> Dear Micha? Miros?aw,
>>>
>>>> 2013/1/21 Marek Vasut <marex@denx.de>:
>>>>> Dear Micha? Miros?aw,
>>>>>
>>>>>> 2013/1/21 Marek Vasut <marex@denx.de>:
>>>>>>> This patch adds support for i.MX23 into the LRADC driver. The LRADC
>>>>>>> block on MX23 is not much different from the one on MX28, thus this
>>>>>>> is only a few changes fixing the parts that are specific to MX23.
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +struct mxs_lradc_of_config {
>>>>>>> +       const int               irq_count;
>>>>>>> +       const char * const      *irq_name;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static const struct mxs_lradc_of_config const mxs_lradc_of_config[]
>>>>>>> = { +       [IMX23_LRADC] = {
>>>>>>> +               .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
>>>>>>> +               .irq_name       = mx23_lradc_irq_names,
>>>>>>> +       },
>>>>>>> +       [IMX28_LRADC] = {
>>>>>>> +               .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
>>>>>>> +               .irq_name       = mx28_lradc_irq_names,
>>>>>>> +       },
>>>>>>> +};
>>>>>>> +
>>>>>>>
>>>>>>>  enum mxs_lradc_ts {
>>>>>>>  
>>>>>>>         MXS_LRADC_TOUCHSCREEN_NONE = 0,
>>>>>>>         MXS_LRADC_TOUCHSCREEN_4WIRE,
>>>>>>>
>>>>>>> @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
>>>>>>> *lradc)
>>>>>>>
>>>>>>>                 writel(0, lradc->base + LRADC_DELAY(i));
>>>>>>>  
>>>>>>>  }
>>>>>>>
>>>>>>> +static const struct of_device_id mxs_lradc_dt_ids[] = {
>>>>>>> +       { .compatible = "fsl,imx23-lradc", .data = (void
>>>>>>> *)IMX23_LRADC, }, +       { .compatible = "fsl,imx28-lradc", .data =
>>>>>>> (void
>>>>>>> *)IMX28_LRADC, }, +       { /* sentinel */ }
>>>>>>> +};
>>>>>>> +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>>>>>>> +
>>>>>>
>>>>>> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
>>>>>
>>>>> Check the register layout, it differs between MX23 and MX28, that's one
>>>>> reason, since were we to access differently placed registers, we can do
>>>>> it easily as in the SSP/I2C drivers.
>>>>>
>>>>> Moreover, there are some features on the MX28 that are not on the MX23
>>>>> (like voltage treshold triggers and touchbuttons), with this setup, we
>>>>> can easily check what we're running at at runtime and determine to
>>>>> disallow these.
>>>>>
>>>>> From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
>>>>> much more convenient in the long run.
>>>>
>>>> I'm asking, because you don't use this number anywhere other than in
>>>> mxs_lradc_probe()
>>>> and there only to dereference the irq-names table. After that the
>>>> structure and number
>>>> are forgotten.
>>>
>>> Certainly, so far it's used only this way. But please see my argument
>>> about register layout, that's why I went down this road of abstraction.
>>
>> You'll probably be better of by putting these differences into the
>> mxs_lradc_of_config struct as well, instead of adding switch statements
>> here and there throughout the code.
> 
> Certainly. All that is needed is in place now.
> 
All look sane to me and Marek has answered all the questions as far as I can see.
I'll take these once I get a response from Shawn (unless someone convinces me
otherwise ;)	

Re: [PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC

[Shawn Guo]

On Tue, Jan 22, 2013 at 12:06:43PM +0000, Jonathan Cameron wrote:
> On 01/21/2013 08:05 PM, Marek Vasut wrote:
> > Add interrupt mapping and compatible string for MX23 LRADC.
> 
> Series looks fine to me, but obviously I'd like an ACK for this one
> from Shawn before taking it.

Acked-by: Shawn Guo <shawn.guo@linaro.org>

[PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC

[Shawn Guo]

On Tue, Jan 22, 2013 at 12:06:43PM +0000, Jonathan Cameron wrote:
> On 01/21/2013 08:05 PM, Marek Vasut wrote:
> > Add interrupt mapping and compatible string for MX23 LRADC.
> 
> Series looks fine to me, but obviously I'd like an ACK for this one
> from Shawn before taking it.

Acked-by: Shawn Guo <shawn.guo@linaro.org>

Re: [PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC

[Jonathan Cameron]

On 01/22/2013 12:41 PM, Shawn Guo wrote:
> On Tue, Jan 22, 2013 at 12:06:43PM +0000, Jonathan Cameron wrote:
>> On 01/21/2013 08:05 PM, Marek Vasut wrote:
>>> Add interrupt mapping and compatible string for MX23 LRADC.
>>
>> Series looks fine to me, but obviously I'd like an ACK for this one
>> from Shawn before taking it.
> 
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
> 
Thanks,

both added to togreg branch of iio.git

[PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC

[Jonathan Cameron]

On 01/22/2013 12:41 PM, Shawn Guo wrote:
> On Tue, Jan 22, 2013 at 12:06:43PM +0000, Jonathan Cameron wrote:
>> On 01/21/2013 08:05 PM, Marek Vasut wrote:
>>> Add interrupt mapping and compatible string for MX23 LRADC.
>>
>> Series looks fine to me, but obviously I'd like an ACK for this one
>> from Shawn before taking it.
> 
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
> 
Thanks,

both added to togreg branch of iio.git