Re: [PATCH v5 1/6] mfd: fsl imx25 Touchscreen ADC driver
Hi, On Tue, Feb 17, 2015 at 08:14:38PM +0100, Uwe Kleine-König wrote: > Hello Markus, > > On Sat, Jan 24, 2015 at 03:01:38PM +0100, Markus Pargmann wrote: > > +config MFD_MX25_TSADC > > + tristate "Freescale i.MX25 integrated Touchscreen and ADC unit" > > + select REGMAP_MMIO > > + depends on SOC_IMX25 > Can you make that: > > depends on SOC_IMX25 || COMPILE_TEST > > ? Yes, changed. > > > + * The code contained herein is licensed under the GNU General Public > > + * License. You may obtain a copy of the GNU General Public License > > + * Version 2 or later at the following locations: > Which version of the GPL is your code licensed under? GPLv2 as described by MODULE_LICENSE. Will change the header accordingly. > > > +static int mx25_tsadc_setup_irq(struct platform_device *pdev, > > + struct mx25_tsadc *tsadc) > > [...] > > + irq_set_chained_handler(irq, mx25_tsadc_irq_handler); > > + irq_set_handler_data(irq, tsadc); > Do you need to reverse the order of these two calls to prevent the > handler being called while handler_data is still unset? Yes that's probably better although the subdevices which may cause interrupts here are not populated at this point. Thanks, Markus -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | signature.asc Description: Digital signature
Re: [PATCH v5 1/6] mfd: fsl imx25 Touchscreen ADC driver
On Wed, Feb 18, 2015 at 12:08:44PM +, Lee Jones wrote:
> On Wed, 18 Feb 2015, Fabio Estevam wrote:
>
> > On Wed, Feb 18, 2015 at 6:01 AM, Lee Jones wrote:
> > > On Tue, 17 Feb 2015, Fabio Estevam wrote:
> > >
> > >> On Mon, Feb 16, 2015 at 11:38 AM, Lee Jones wrote:
> > >>
> > >> >> +static int mx25_tsadc_setup_irq(struct platform_device *pdev,
> > >> >> + struct mx25_tsadc *tsadc)
> > >> >> +{
> > >> >> + struct device *dev = &pdev->dev;
> > >> >> + struct device_node *np = dev->of_node;
> > >> >> + int irq;
> > >> >> +
> > >> >> + irq = platform_get_irq(pdev, 0);
> > >> >> + if (irq < 0) {
> > >> >
> > >> > What if 0 is returned?
> > >>
> > >> Then imx25.dtsi would be passing irq=0 for the ADC, which would be
> > >> totally wrong.
> > >
> > > Exactly, so it should be <=.
> >
> > imx25.dtsi passes interrupts = <46>; for the touch screen controller,
> > so the irq number will never be zero.
>
> It doesn't matter what happens to be passed at the moment. The
> correct thing to do is enforce correct/full error checking. Yes <0 is
> an error, but so is =0, so encompass it in the checks.
I now changed all irq < 0 checks to irq <= 0 under the assumption that
irq 0 is always an error.
Thanks,
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
signature.asc
Description: Digital signature
Re: [PATCH v5 1/6] mfd: fsl imx25 Touchscreen ADC driver
On Wed, 18 Feb 2015, Fabio Estevam wrote:
> On Wed, Feb 18, 2015 at 6:01 AM, Lee Jones wrote:
> > On Tue, 17 Feb 2015, Fabio Estevam wrote:
> >
> >> On Mon, Feb 16, 2015 at 11:38 AM, Lee Jones wrote:
> >>
> >> >> +static int mx25_tsadc_setup_irq(struct platform_device *pdev,
> >> >> + struct mx25_tsadc *tsadc)
> >> >> +{
> >> >> + struct device *dev = &pdev->dev;
> >> >> + struct device_node *np = dev->of_node;
> >> >> + int irq;
> >> >> +
> >> >> + irq = platform_get_irq(pdev, 0);
> >> >> + if (irq < 0) {
> >> >
> >> > What if 0 is returned?
> >>
> >> Then imx25.dtsi would be passing irq=0 for the ADC, which would be
> >> totally wrong.
> >
> > Exactly, so it should be <=.
>
> imx25.dtsi passes interrupts = <46>; for the touch screen controller,
> so the irq number will never be zero.
It doesn't matter what happens to be passed at the moment. The
correct thing to do is enforce correct/full error checking. Yes <0 is
an error, but so is =0, so encompass it in the checks.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/6] mfd: fsl imx25 Touchscreen ADC driver
On Wed, Feb 18, 2015 at 6:01 AM, Lee Jones wrote:
> On Tue, 17 Feb 2015, Fabio Estevam wrote:
>
>> On Mon, Feb 16, 2015 at 11:38 AM, Lee Jones wrote:
>>
>> >> +static int mx25_tsadc_setup_irq(struct platform_device *pdev,
>> >> + struct mx25_tsadc *tsadc)
>> >> +{
>> >> + struct device *dev = &pdev->dev;
>> >> + struct device_node *np = dev->of_node;
>> >> + int irq;
>> >> +
>> >> + irq = platform_get_irq(pdev, 0);
>> >> + if (irq < 0) {
>> >
>> > What if 0 is returned?
>>
>> Then imx25.dtsi would be passing irq=0 for the ADC, which would be
>> totally wrong.
>
> Exactly, so it should be <=.
imx25.dtsi passes interrupts = <46>; for the touch screen controller,
so the irq number will never be zero.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/6] mfd: fsl imx25 Touchscreen ADC driver
On Tue, 17 Feb 2015, Fabio Estevam wrote:
> On Mon, Feb 16, 2015 at 11:38 AM, Lee Jones wrote:
>
> >> +static int mx25_tsadc_setup_irq(struct platform_device *pdev,
> >> + struct mx25_tsadc *tsadc)
> >> +{
> >> + struct device *dev = &pdev->dev;
> >> + struct device_node *np = dev->of_node;
> >> + int irq;
> >> +
> >> + irq = platform_get_irq(pdev, 0);
> >> + if (irq < 0) {
> >
> > What if 0 is returned?
>
> Then imx25.dtsi would be passing irq=0 for the ADC, which would be
> totally wrong.
Exactly, so it should be <=.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/6] mfd: fsl imx25 Touchscreen ADC driver
Hello Markus, On Sat, Jan 24, 2015 at 03:01:38PM +0100, Markus Pargmann wrote: > +config MFD_MX25_TSADC > + tristate "Freescale i.MX25 integrated Touchscreen and ADC unit" > + select REGMAP_MMIO > + depends on SOC_IMX25 Can you make that: depends on SOC_IMX25 || COMPILE_TEST ? > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: Which version of the GPL is your code licensed under? > +static int mx25_tsadc_setup_irq(struct platform_device *pdev, > + struct mx25_tsadc *tsadc) > [...] > + irq_set_chained_handler(irq, mx25_tsadc_irq_handler); > + irq_set_handler_data(irq, tsadc); Do you need to reverse the order of these two calls to prevent the handler being called while handler_data is still unset? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/6] mfd: fsl imx25 Touchscreen ADC driver
On Mon, Feb 16, 2015 at 11:38 AM, Lee Jones wrote:
>> +static int mx25_tsadc_setup_irq(struct platform_device *pdev,
>> + struct mx25_tsadc *tsadc)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + int irq;
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq < 0) {
>
> What if 0 is returned?
Then imx25.dtsi would be passing irq=0 for the ADC, which would be
totally wrong.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/6] mfd: fsl imx25 Touchscreen ADC driver
On Sat, 24 Jan 2015, Markus Pargmann wrote:
> This is the core driver for imx25 touchscreen/adc driver. The module
> has one shared ADC and two different conversion queues which use the
> ADC. The two queues are identical. Both can be used for general purpose
> ADC but one is meant to be used for touchscreens.
>
> This driver is the core which manages the central components and
> registers of the TSC/ADC unit. It manages the IRQs and forwards them to
> the correct components.
>
> Signed-off-by: Markus Pargmann
> Signed-off-by: Denis Carikli
> Acked-by: Jonathan Cameron
> ---
>
> Notes:
> Changes in v5:
> - Remove ifdef CONFIG_OF as this driver is only for DT usage
> - Remove module owner
> - Add Kconfig dependencies ARCH_MX25 and OF
>
> @Jonathan Cameron:
> I left your acked-by on the patch as these were small changes. If it
> should be
> removed, please say so. Thanks
>
> drivers/mfd/Kconfig | 10 +++
> drivers/mfd/Makefile| 2 +
> drivers/mfd/fsl-imx25-tsadc.c | 167
>
> include/linux/mfd/imx25-tsadc.h | 140 +
> 4 files changed, 319 insertions(+)
> create mode 100644 drivers/mfd/fsl-imx25-tsadc.c
> create mode 100644 include/linux/mfd/imx25-tsadc.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 2e6b7311fabc..44fc15598a6a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -232,6 +232,16 @@ config MFD_MC13XXX_I2C
> help
> Select this if your MC13xxx is connected via an I2C bus.
>
> +config MFD_MX25_TSADC
> + tristate "Freescale i.MX25 integrated Touchscreen and ADC unit"
> + select REGMAP_MMIO
> + depends on SOC_IMX25
> + depends on OF
> + help
> + Enable support for the integrated Touchscreen and ADC unit of the
> + i.MX25 processors. They consist of a conversion queue for general
> + purpose ADC and a queue for Touchscreens.
> +
> config MFD_HI6421_PMIC
> tristate "HiSilicon Hi6421 PMU/Codec IC"
> depends on OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 53467e211381..3feeb29f5938 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -78,6 +78,8 @@ obj-$(CONFIG_TWL4030_POWER)+= twl4030-power.o
> obj-$(CONFIG_MFD_TWL4030_AUDIO) += twl4030-audio.o
> obj-$(CONFIG_TWL6040_CORE) += twl6040.o
>
> +obj-$(CONFIG_MFD_MX25_TSADC) += fsl-imx25-tsadc.o
> +
> obj-$(CONFIG_MFD_MC13XXX)+= mc13xxx-core.o
> obj-$(CONFIG_MFD_MC13XXX_SPI)+= mc13xxx-spi.o
> obj-$(CONFIG_MFD_MC13XXX_I2C)+= mc13xxx-i2c.o
> diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
> new file mode 100644
> index ..8e4013d57500
> --- /dev/null
> +++ b/drivers/mfd/fsl-imx25-tsadc.c
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright 2014 Markus Pargmann, Pengutronix
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
[...]
> +static int mx25_tsadc_setup_irq(struct platform_device *pdev,
> + struct mx25_tsadc *tsadc)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + int irq;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
What if 0 is returned?
> + dev_err(dev, "Failed to get irq\n");
> + return irq;
> + }
> +
> + tsadc->domain = irq_domain_add_simple(np, 2, 0, &mx25_tsadc_domain_ops,
> + tsadc);
> + if (!tsadc->domain) {
> + dev_err(dev, "Failed to add irq domain\n");
> + return -ENOMEM;
> + }
> +
> + irq_set_chained_handler(irq, mx25_tsadc_irq_handler);
> + irq_set_handler_data(irq, tsadc);
> +
> + return 0;
> +}
> diff --git a/include/linux/mfd/imx25-tsadc.h b/include/linux/mfd/imx25-tsadc.h
> new file mode 100644
> index ..8086f18847fe
> --- /dev/null
> +++ b/include/linux/mfd/imx25-tsadc.h
> @@ -0,0 +1,140 @@
> +#ifndef _LINUX_INCLUDE_INPUT_IMX25_TSADC_H_
> +#define _LINUX_INCLUDE_INPUT_IMX25_TSADC_H_
INPUT?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/6] mfd: fsl imx25 Touchscreen ADC driver
This is the core driver for imx25 touchscreen/adc driver. The module
has one shared ADC and two different conversion queues which use the
ADC. The two queues are identical. Both can be used for general purpose
ADC but one is meant to be used for touchscreens.
This driver is the core which manages the central components and
registers of the TSC/ADC unit. It manages the IRQs and forwards them to
the correct components.
Signed-off-by: Markus Pargmann
Signed-off-by: Denis Carikli
Acked-by: Jonathan Cameron
---
Notes:
Changes in v5:
- Remove ifdef CONFIG_OF as this driver is only for DT usage
- Remove module owner
- Add Kconfig dependencies ARCH_MX25 and OF
@Jonathan Cameron:
I left your acked-by on the patch as these were small changes. If it should
be
removed, please say so. Thanks
drivers/mfd/Kconfig | 10 +++
drivers/mfd/Makefile| 2 +
drivers/mfd/fsl-imx25-tsadc.c | 167
include/linux/mfd/imx25-tsadc.h | 140 +
4 files changed, 319 insertions(+)
create mode 100644 drivers/mfd/fsl-imx25-tsadc.c
create mode 100644 include/linux/mfd/imx25-tsadc.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 2e6b7311fabc..44fc15598a6a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -232,6 +232,16 @@ config MFD_MC13XXX_I2C
help
Select this if your MC13xxx is connected via an I2C bus.
+config MFD_MX25_TSADC
+ tristate "Freescale i.MX25 integrated Touchscreen and ADC unit"
+ select REGMAP_MMIO
+ depends on SOC_IMX25
+ depends on OF
+ help
+ Enable support for the integrated Touchscreen and ADC unit of the
+ i.MX25 processors. They consist of a conversion queue for general
+ purpose ADC and a queue for Touchscreens.
+
config MFD_HI6421_PMIC
tristate "HiSilicon Hi6421 PMU/Codec IC"
depends on OF
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 53467e211381..3feeb29f5938 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -78,6 +78,8 @@ obj-$(CONFIG_TWL4030_POWER)+= twl4030-power.o
obj-$(CONFIG_MFD_TWL4030_AUDIO)+= twl4030-audio.o
obj-$(CONFIG_TWL6040_CORE) += twl6040.o
+obj-$(CONFIG_MFD_MX25_TSADC) += fsl-imx25-tsadc.o
+
obj-$(CONFIG_MFD_MC13XXX) += mc13xxx-core.o
obj-$(CONFIG_MFD_MC13XXX_SPI) += mc13xxx-spi.o
obj-$(CONFIG_MFD_MC13XXX_I2C) += mc13xxx-i2c.o
diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
new file mode 100644
index ..8e4013d57500
--- /dev/null
+++ b/drivers/mfd/fsl-imx25-tsadc.c
@@ -0,0 +1,167 @@
+/*
+ * Copyright 2014 Markus Pargmann, Pengutronix
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+static struct regmap_config mx25_tsadc_regmap_config = {
+ .fast_io = true,
+ .max_register = 8,
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+};
+
+static void mx25_tsadc_irq_handler(u32 irq, struct irq_desc *desc)
+{
+ struct mx25_tsadc *tsadc = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_get_chip(irq);
+ u32 status;
+
+ chained_irq_enter(chip, desc);
+
+ regmap_read(tsadc->regs, MX25_TSC_TGSR, &status);
+
+ if (status & MX25_TGSR_GCQ_INT)
+ generic_handle_irq(irq_find_mapping(tsadc->domain, 1));
+
+ if (status & MX25_TGSR_TCQ_INT)
+ generic_handle_irq(irq_find_mapping(tsadc->domain, 0));
+
+ chained_irq_exit(chip, desc);
+}
+
+static int mx25_tsadc_domain_map(struct irq_domain *d, unsigned int irq,
+irq_hw_number_t hwirq)
+{
+ struct mx25_tsadc *tsadc = d->host_data;
+
+ irq_set_chip_data(irq, tsadc);
+ irq_set_chip_and_handler(irq, &dummy_irq_chip,
+handle_level_irq);
+ set_irq_flags(irq, IRQF_VALID);
+
+ return 0;
+}
+
+static struct irq_domain_ops mx25_tsadc_domain_ops = {
+ .map = mx25_tsadc_domain_map,
+ .xlate = irq_domain_xlate_onecell,
+};
+
+static int mx25_tsadc_setup_irq(struct platform_device *pdev,
+ struct mx25_tsadc *tsadc)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ int irq;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(dev, "Failed to get irq\n");
+ return irq;
+ }
+
+ tsadc->domain = irq_domain_add_simple(np, 2, 0, &mx25_tsadc_domain_ops,
+
