* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
@ 2013-01-21 20:05 Marek Vasut
2013-01-21 20:05 ` [PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC Marek Vasut
2013-01-21 20:36 ` [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver Michał Mirosław
0 siblings, 2 replies; 14+ messages in thread
From: Marek Vasut @ 2013-01-21 20:05 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC
2013-01-21 20:05 [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver Marek Vasut
@ 2013-01-21 20:05 ` Marek Vasut
2013-01-22 12:06 ` Jonathan Cameron
2013-01-21 20:36 ` [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver Michał Mirosław
1 sibling, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2013-01-21 20:05 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC
2013-01-21 20:05 ` [PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC Marek Vasut
@ 2013-01-22 12:06 ` Jonathan Cameron
2013-01-22 12:41 ` Shawn Guo
0 siblings, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2013-01-22 12:06 UTC (permalink / raw)
To: linux-arm-kernel
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";
> };
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC
2013-01-22 12:06 ` Jonathan Cameron
@ 2013-01-22 12:41 ` Shawn Guo
2013-01-22 13:02 ` Jonathan Cameron
0 siblings, 1 reply; 14+ messages in thread
From: Shawn Guo @ 2013-01-22 12:41 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC
2013-01-22 12:41 ` Shawn Guo
@ 2013-01-22 13:02 ` Jonathan Cameron
0 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2013-01-22 13:02 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
2013-01-21 20:05 [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver Marek Vasut
2013-01-21 20:05 ` [PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC Marek Vasut
@ 2013-01-21 20:36 ` Michał Mirosław
2013-01-21 21:00 ` Marek Vasut
1 sibling, 1 reply; 14+ messages in thread
From: Michał Mirosław @ 2013-01-21 20:36 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
2013-01-21 20:36 ` [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver Michał Mirosław
@ 2013-01-21 21:00 ` Marek Vasut
2013-01-21 21:19 ` Michał Mirosław
0 siblings, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2013-01-21 21:00 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
2013-01-21 21:00 ` Marek Vasut
@ 2013-01-21 21:19 ` Michał Mirosław
2013-01-21 21:32 ` Marek Vasut
0 siblings, 1 reply; 14+ messages in thread
From: Michał Mirosław @ 2013-01-21 21:19 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
2013-01-21 21:19 ` Michał Mirosław
@ 2013-01-21 21:32 ` Marek Vasut
2013-01-21 21:37 ` Lars-Peter Clausen
2013-01-21 21:48 ` Michał Mirosław
0 siblings, 2 replies; 14+ messages in thread
From: Marek Vasut @ 2013-01-21 21:32 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
2013-01-21 21:32 ` Marek Vasut
@ 2013-01-21 21:37 ` Lars-Peter Clausen
2013-01-21 21:49 ` Marek Vasut
2013-01-21 21:48 ` Michał Mirosław
1 sibling, 1 reply; 14+ messages in thread
From: Lars-Peter Clausen @ 2013-01-21 21:37 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
2013-01-21 21:37 ` Lars-Peter Clausen
@ 2013-01-21 21:49 ` Marek Vasut
2013-01-22 12:09 ` Jonathan Cameron
0 siblings, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2013-01-21 21:49 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
2013-01-21 21:49 ` Marek Vasut
@ 2013-01-22 12:09 ` Jonathan Cameron
0 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2013-01-22 12:09 UTC (permalink / raw)
To: linux-arm-kernel
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 ;)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
2013-01-21 21:32 ` Marek Vasut
2013-01-21 21:37 ` Lars-Peter Clausen
@ 2013-01-21 21:48 ` Michał Mirosław
2013-01-21 21:53 ` Marek Vasut
1 sibling, 1 reply; 14+ messages in thread
From: Michał Mirosław @ 2013-01-21 21:48 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
2013-01-21 21:48 ` Michał Mirosław
@ 2013-01-21 21:53 ` Marek Vasut
0 siblings, 0 replies; 14+ messages in thread
From: Marek Vasut @ 2013-01-21 21:53 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-01-22 13:02 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-21 20:05 [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver Marek Vasut
2013-01-21 20:05 ` [PATCH 2/2] ARM: mxs: Add OF props for MX23 LRADC Marek Vasut
2013-01-22 12:06 ` Jonathan Cameron
2013-01-22 12:41 ` Shawn Guo
2013-01-22 13:02 ` Jonathan Cameron
2013-01-21 20:36 ` [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver Michał Mirosław
2013-01-21 21:00 ` Marek Vasut
2013-01-21 21:19 ` Michał Mirosław
2013-01-21 21:32 ` Marek Vasut
2013-01-21 21:37 ` Lars-Peter Clausen
2013-01-21 21:49 ` Marek Vasut
2013-01-22 12:09 ` Jonathan Cameron
2013-01-21 21:48 ` Michał Mirosław
2013-01-21 21:53 ` Marek Vasut
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).