[v3,1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD

Message ID	370a02584386944b465deff5ccc48c8919b23dba.1466769907.git.ksenija.stanojevic@gmail.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-input-owner@kernel.org> From: Ksenija Stanojevic <ksenija.stanojevic@gmail.com> To: linux-kernel@vger.kernel.org Cc: lee.jones@linaro.org, dmitry.torokhov@gmail.com, linux-input@vger.kernel.org, jic23@kernel.org, knaack.h@gmx.de, lars@metafoo.de, pmeerw@pmeerw.net, marex@denx.de, linux-iio@vger.kernel.org, harald@ccbib.org, stefan.wahren@i2se.com, fabio.estevam@freescale.com, Ksenija Stanojevic <ksenija.stanojevic@gmail.com> Subject: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD Date: Fri, 24 Jun 2016 14:09:16 +0200 Message-Id: <370a02584386944b465deff5ccc48c8919b23dba.1466769907.git.ksenija.stanojevic@gmail.com> In-Reply-To: <cover.1466769907.git.ksenija.stanojevic@gmail.com> References: <cover.1466769907.git.ksenija.stanojevic@gmail.com> Sender: linux-input-owner@vger.kernel.org Precedence: bulk

Ksenija Stanojevic June 24, 2016, 12:09 p.m. UTC

Add core files for mxs-lradc MFD driver.

Note:  this patch won't compile in iio/testing without this patch:
a8f447be8056 ("mfd: Add resource managed APIs for mfd_add_devices")

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
Changes in v3:
 - add note to commit message
 - move switch statement into if(touch_ret == 0) branch
 - add MODULE_AUTHOR

Changes in v2:
 - do not change spacing in Kconfig
 - make struct mfd_cell part of struct mxs_lradc
 - use switch instead of if in mxs_lradc_irq_mask
 - use only necessary header files in mxs_lradc.h
 - use devm_mfd_add_device
 - use separate function to register mfd device
 - change licence to GPL
 - add copyright

 drivers/mfd/Kconfig           |  17 ++++
 drivers/mfd/Makefile          |   1 +
 drivers/mfd/mxs-lradc.c       | 217 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/mxs-lradc.h | 204 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 439 insertions(+)
 create mode 100644 drivers/mfd/mxs-lradc.c
 create mode 100644 include/linux/mfd/mxs-lradc.h

Stefan Wahren June 24, 2016, 9:15 p.m. UTC | #1

Hi,

> Ksenija Stanojevic <ksenija.stanojevic@gmail.com> hat am 24. Juni 2016 um
> 14:09 geschrieben:
> 
> 
> Add core files for mxs-lradc MFD driver.
> 
> Note:  this patch won't compile in iio/testing without this patch:
> a8f447be8056 ("mfd: Add resource managed APIs for mfd_add_devices")
> 
> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
> Acked-by: Jonathan Cameron <jic23@kernel.org>

i tested it successfully with a i.MX23 board (without touchscreen).

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lee Jones June 28, 2016, 4:28 p.m. UTC | #2

On Fri, 24 Jun 2016, Ksenija Stanojevic wrote:

> Add core files for mxs-lradc MFD driver.
> 
> Note:  this patch won't compile in iio/testing without this patch:
> a8f447be8056 ("mfd: Add resource managed APIs for mfd_add_devices")
> 
> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
> Changes in v3:
>  - add note to commit message
>  - move switch statement into if(touch_ret == 0) branch
>  - add MODULE_AUTHOR
> 
> Changes in v2:
>  - do not change spacing in Kconfig
>  - make struct mfd_cell part of struct mxs_lradc
>  - use switch instead of if in mxs_lradc_irq_mask
>  - use only necessary header files in mxs_lradc.h
>  - use devm_mfd_add_device
>  - use separate function to register mfd device
>  - change licence to GPL
>  - add copyright
> 
>  drivers/mfd/Kconfig           |  17 ++++
>  drivers/mfd/Makefile          |   1 +
>  drivers/mfd/mxs-lradc.c       | 217 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/mxs-lradc.h | 204 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 439 insertions(+)
>  create mode 100644 drivers/mfd/mxs-lradc.c
>  create mode 100644 include/linux/mfd/mxs-lradc.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 9ca66de..62593ad 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -271,6 +271,23 @@ config MFD_MC13XXX_I2C
>  	help
>  	  Select this if your MC13xxx is connected via an I2C bus.
>  
> +config MFD_MXS_LRADC
> +	tristate "Freescale i.MX23/i.MX28 LRADC"
> +	depends on ARCH_MXS || COMPILE_TEST
> +	select MFD_CORE
> +	select STMP_DEVICE
> +	help
> +	  Say yes here to build support for the low-resolution
> +	  analog-to-digital converter (LRADC) found on the i.MX23 and i.MX28

Low Resolution Analogue-to-Digital Converter 

> +	  processors. This driver provides common support for accessing the
> +	  device, additional drivers must be enabled in order to use the
> +	  functionality of the device:
> +		mxs-lradc-adc for ADC readings
> +		mxs-lradc-ts  for touchscreen support
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called mxs-lradc.
> +
>  config MFD_HI6421_PMIC
>  	tristate "HiSilicon Hi6421 PMU/Codec IC"
>  	depends on OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 0f230a6..ecf6399 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -198,3 +198,4 @@ intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC)	+= intel_soc_pmic_bxtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
>  obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
> +obj-$(CONFIG_MFD_MXS_LRADC)	+= mxs-lradc.o
> diff --git a/drivers/mfd/mxs-lradc.c b/drivers/mfd/mxs-lradc.c
> new file mode 100644
> index 0000000..d53a524
> --- /dev/null
> +++ b/drivers/mfd/mxs-lradc.c
> @@ -0,0 +1,217 @@
> +/*
> + * Freescale MXS LRADC driver
> + *
> + * Copyright (c) 2012 DENX Software Engineering, GmbH.


You need to update the copyright.

Authors:

> + * Marek Vasut <marex@denx.de>
> + * Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/mxs-lradc.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +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",
> +	"mxs-lradc-channel0",
> +	"mxs-lradc-channel1",
> +	"mxs-lradc-channel2",
> +	"mxs-lradc-channel3",
> +	"mxs-lradc-channel4",
> +	"mxs-lradc-channel5",
> +	"mxs-lradc-channel6",
> +	"mxs-lradc-channel7",
> +	"mxs-lradc-button0",
> +	"mxs-lradc-button1",
> +};
> +
> +struct mxs_lradc_of_config {
> +	const int		irq_count;
> +	const char * const	*irq_name;
> +};
> +
> +static const struct mxs_lradc_of_config 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,
> +	},
> +};

Please use DEFINE_RES_IRQ_NAMED() instead.

> +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_add_device(struct platform_device *pdev,
> +				struct mxs_lradc *lradc, char *name, int i)
> +{
> +	struct mfd_cell *cell;
> +
> +	cell = &lradc->cells[i];
> +	cell->name = name;
> +	cell->platform_data = lradc;
> +	cell->pdata_size = sizeof(*lradc);
> +
> +	return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);
> +}

Please don't roll your own API.

Use 'struct mfd_cell' like everyone else does.

> +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];

This implementation is wayyyy more complicated than it needs to be.

You've entangled DT probing with the MFD API.

Please use the MFD API properly.

> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct mxs_lradc *lradc;
> +	struct resource *iores;
> +	int ret = 0, touch_ret, i;
> +	u32 ts_wires = 0;
> +
> +	lradc = devm_kzalloc(&pdev->dev, sizeof(*lradc), GFP_KERNEL);
> +	if (!lradc)
> +		return -ENOMEM;
> +	lradc->soc = (enum mxs_lradc_id)of_id->data;

If you're going to pull the platform ID out like this, please pull the
*_match_* function down, just above this line.  And separate them both
from allocating memory.

> +	/* Grab the memory area */

This comment doesn't add anything.

We know what these calls do, and they don't "grab" anything.

> +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	lradc->base = devm_ioremap_resource(dev, iores);
> +	if (IS_ERR(lradc->base))
> +		return PTR_ERR(lradc->base);
> +
> +	lradc->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(lradc->clk)) {
> +		dev_err(dev, "Failed to get the delay unit clock\n");
> +		return PTR_ERR(lradc->clk);
> +	}

'\n' here.  Nice spacing makes code easier to follow.

> +	ret = clk_prepare_enable(lradc->clk);
> +	if (ret != 0) {
> +		dev_err(dev, "Failed to enable the delay unit clock\n");
> +		return ret;
> +	}
> +
> +	touch_ret = of_property_read_u32(node, "fsl,lradc-touchscreen-wires",
> +					 &ts_wires);

Why do you need more than one 'ret' variable?

> +	if (touch_ret == 0) {
> +		lradc->buffer_vchans = BUFFER_VCHANS_LIMITED;

'\n' here  or else we think it's related to the switch().

> +		switch (ts_wires) {
> +		case 4:
> +			lradc->use_touchscreen = MXS_LRADC_TOUCHSCREEN_4WIRE;

This is not a good name for a variable.  It looks like a bool here,
but you're initialising with a configuration option.

> +			break;
> +		case 5:
> +			if (lradc->soc == IMX28_LRADC) {
> +				lradc->use_touchscreen =
> +					MXS_LRADC_TOUCHSCREEN_5WIRE;
> +				break;
> +			}
> +			/* fall through an error message for i.MX23 */

"to an error"

> +		default:
> +			dev_err(&pdev->dev,
> +				"Unsupported number of touchscreen wires (%d)\n",
> +				ts_wires);
> +			return -EINVAL;

The clock is still enabled.

> +		}
> +	} else {
> +		lradc->buffer_vchans = BUFFER_VCHANS_ALL;
> +	}
> +
> +	lradc->irq_count = of_cfg->irq_count;
> +	lradc->irq_name = of_cfg->irq_name;
> +	for (i = 0; i < lradc->irq_count; i++) {
> +		lradc->irq[i] = platform_get_irq(pdev, i);
> +		if (lradc->irq[i] < 0) {
> +			ret = lradc->irq[i];
> +			goto err_clk;
> +		}
> +	}

You shouldn't be 'get'ing the IRQ here.

Where are you consuming these IRQs?

> +	platform_set_drvdata(pdev, lradc);
> +
> +	ret = stmp_reset_block(lradc->base);
> +	if (ret)
> +		return ret;


The clock is still enabled.


> +	ret = mxs_lradc_add_device(pdev, lradc, DRIVER_NAME_ADC, 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to add the ADC subdevice\n");
> +		goto err_clk;
> +	}

Do this using statically defined mfd_cells.

> +	if (!lradc->use_touchscreen)
> +		return 0;
> +
> +	ret = mxs_lradc_add_device(pdev, lradc, DRIVER_NAME_TS, 1);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Failed to add the touchscreen subdevice\n");
> +		goto err_clk;
> +	}
> +
> +	return 0;
> +
> +err_clk:
> +	clk_disable_unprepare(lradc->clk);
> +
> +	return ret;
> +}
> +
> +static int mxs_lradc_remove(struct platform_device *pdev)
> +{
> +	struct mxs_lradc *lradc = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(lradc->clk);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mxs_lradc_driver = {
> +	.driver = {
> +		.name = "mxs-lradc",
> +		.of_match_table = mxs_lradc_dt_ids,
> +	},
> +	.probe = mxs_lradc_probe,
> +	.remove = mxs_lradc_remove,
> +};
> +

This line is superfluous.

> +module_platform_driver(mxs_lradc_driver);
> +
> +MODULE_AUTHOR("Ksenija Stanojevic <ksenija.stanojevic@gmail.com>");
> +MODULE_DESCRIPTION("Freescale i.MX23/i.MX28 LRADC driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:mxs-lradc");
> diff --git a/include/linux/mfd/mxs-lradc.h b/include/linux/mfd/mxs-lradc.h
> new file mode 100644
> index 0000000..c1475c0
> --- /dev/null
> +++ b/include/linux/mfd/mxs-lradc.h
> @@ -0,0 +1,204 @@
> +/*
> + * Freescale MXS LRADC driver
> + *
> + * Copyright (c) 2012 DENX Software Engineering, GmbH.

You need to update the copyright.

Author:

> + * Marek Vasut <marex@denx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __MXS_LRADC_H
> +#define __MXS_LRADC_H

__MFD_MXS_*

> +#include <linux/bitops.h>
> +#include <linux/io.h>
> +#include <linux/stmp_device.h>
> +
> +#define DRIVER_NAME_ADC "mxs-lradc-adc"
> +#define DRIVER_NAME_TS "mxs-lradc-ts"

Don't define these.  Just use the proper names.

> +#define LRADC_MAX_DELAY_CHANS	4
> +#define LRADC_MAX_MAPPED_CHANS	8
> +#define LRADC_MAX_TOTAL_CHANS	16
> +
> +#define LRADC_DELAY_TIMER_HZ	2000
> +
> +#define LRADC_CTRL0				0x00
> +# define LRADC_CTRL0_MX28_TOUCH_DETECT_ENABLE	BIT(23)
> +# define LRADC_CTRL0_MX28_TOUCH_SCREEN_TYPE	BIT(22)
> +# define LRADC_CTRL0_MX28_YNNSW /* YM */	BIT(21)
> +# define LRADC_CTRL0_MX28_YPNSW /* YP */	BIT(20)
> +# define LRADC_CTRL0_MX28_YPPSW /* YP */	BIT(19)
> +# define LRADC_CTRL0_MX28_XNNSW /* XM */	BIT(18)
> +# define LRADC_CTRL0_MX28_XNPSW /* XM */	BIT(17)
> +# define LRADC_CTRL0_MX28_XPPSW /* XP */	BIT(16)
> +
> +# define LRADC_CTRL0_MX23_TOUCH_DETECT_ENABLE	BIT(20)
> +# define LRADC_CTRL0_MX23_YM			BIT(19)
> +# define LRADC_CTRL0_MX23_XM			BIT(18)
> +# define LRADC_CTRL0_MX23_YP			BIT(17)
> +# define LRADC_CTRL0_MX23_XP			BIT(16)
> +
> +# define LRADC_CTRL0_MX28_PLATE_MASK \
> +		(LRADC_CTRL0_MX28_TOUCH_DETECT_ENABLE | \
> +		LRADC_CTRL0_MX28_YNNSW | LRADC_CTRL0_MX28_YPNSW | \
> +		LRADC_CTRL0_MX28_YPPSW | LRADC_CTRL0_MX28_XNNSW | \
> +		LRADC_CTRL0_MX28_XNPSW | LRADC_CTRL0_MX28_XPPSW)
> +
> +# define LRADC_CTRL0_MX23_PLATE_MASK \
> +		(LRADC_CTRL0_MX23_TOUCH_DETECT_ENABLE | \
> +		LRADC_CTRL0_MX23_YM | LRADC_CTRL0_MX23_XM | \
> +		LRADC_CTRL0_MX23_YP | LRADC_CTRL0_MX23_XP)
> +
> +#define LRADC_CTRL1				0x10
> +#define LRADC_CTRL1_TOUCH_DETECT_IRQ_EN		BIT(24)
> +#define LRADC_CTRL1_LRADC_IRQ_EN(n)		(1 << ((n) + 16))
> +#define LRADC_CTRL1_MX28_LRADC_IRQ_EN_MASK	(0x1fff << 16)
> +#define LRADC_CTRL1_MX23_LRADC_IRQ_EN_MASK	(0x01ff << 16)
> +#define LRADC_CTRL1_LRADC_IRQ_EN_OFFSET		16
> +#define LRADC_CTRL1_TOUCH_DETECT_IRQ		BIT(8)
> +#define LRADC_CTRL1_LRADC_IRQ(n)		(1 << (n))
> +#define LRADC_CTRL1_MX28_LRADC_IRQ_MASK		0x1fff
> +#define LRADC_CTRL1_MX23_LRADC_IRQ_MASK		0x01ff
> +#define LRADC_CTRL1_LRADC_IRQ_OFFSET		0
> +
> +#define LRADC_CTRL2				0x20
> +#define LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET	24
> +#define LRADC_CTRL2_TEMPSENSE_PWD		BIT(15)
> +
> +#define LRADC_STATUS				0x40
> +#define LRADC_STATUS_TOUCH_DETECT_RAW		BIT(0)
> +
> +#define LRADC_CH(n)				(0x50 + (0x10 * (n)))
> +#define LRADC_CH_ACCUMULATE			BIT(29)
> +#define LRADC_CH_NUM_SAMPLES_MASK		(0x1f << 24)
> +#define LRADC_CH_NUM_SAMPLES_OFFSET		24
> +#define LRADC_CH_NUM_SAMPLES(x) \
> +				((x) << LRADC_CH_NUM_SAMPLES_OFFSET)
> +#define LRADC_CH_VALUE_MASK			0x3ffff
> +#define LRADC_CH_VALUE_OFFSET			0
> +
> +#define LRADC_DELAY(n)				(0xd0 + (0x10 * (n)))
> +#define LRADC_DELAY_TRIGGER_LRADCS_MASK		(0xffUL << 24)
> +#define LRADC_DELAY_TRIGGER_LRADCS_OFFSET	24
> +#define LRADC_DELAY_TRIGGER(x) \
> +				(((x) << LRADC_DELAY_TRIGGER_LRADCS_OFFSET) & \
> +				LRADC_DELAY_TRIGGER_LRADCS_MASK)
> +#define LRADC_DELAY_KICK			BIT(20)
> +#define LRADC_DELAY_TRIGGER_DELAYS_MASK		(0xf << 16)
> +#define LRADC_DELAY_TRIGGER_DELAYS_OFFSET	16
> +#define LRADC_DELAY_TRIGGER_DELAYS(x) \
> +				(((x) << LRADC_DELAY_TRIGGER_DELAYS_OFFSET) & \
> +				LRADC_DELAY_TRIGGER_DELAYS_MASK)
> +#define LRADC_DELAY_LOOP_COUNT_MASK		(0x1f << 11)
> +#define LRADC_DELAY_LOOP_COUNT_OFFSET		11
> +#define LRADC_DELAY_LOOP(x) \
> +				(((x) << LRADC_DELAY_LOOP_COUNT_OFFSET) & \
> +				LRADC_DELAY_LOOP_COUNT_MASK)
> +#define LRADC_DELAY_DELAY_MASK			0x7ff
> +#define LRADC_DELAY_DELAY_OFFSET		0
> +#define LRADC_DELAY_DELAY(x) \
> +				(((x) << LRADC_DELAY_DELAY_OFFSET) & \
> +				LRADC_DELAY_DELAY_MASK)
> +
> +#define LRADC_CTRL4				0x140
> +#define LRADC_CTRL4_LRADCSELECT_MASK(n)		(0xf << ((n) * 4))
> +#define LRADC_CTRL4_LRADCSELECT_OFFSET(n)	((n) * 4)
> +#define LRADC_CTRL4_LRADCSELECT(n, x) \
> +				(((x) << LRADC_CTRL4_LRADCSELECT_OFFSET(n)) & \
> +				LRADC_CTRL4_LRADCSELECT_MASK(n))
> +
> +#define LRADC_RESOLUTION			12
> +#define LRADC_SINGLE_SAMPLE_MASK		((1 << LRADC_RESOLUTION) - 1)
> +
> +enum mxs_lradc_id {
> +	IMX23_LRADC,
> +	IMX28_LRADC,
> +};
> +
> +enum mxs_lradc_ts_wires {
> +	MXS_LRADC_TOUCHSCREEN_NONE = 0,
> +	MXS_LRADC_TOUCHSCREEN_4WIRE,
> +	MXS_LRADC_TOUCHSCREEN_5WIRE,
> +};
> +
> +struct mxs_lradc {
> +	enum mxs_lradc_id	soc;
> +
> +	void __iomem		*base;
> +	struct clk		*clk;
> +
> +	int			irq[13];
> +	const char * const	*irq_name;
> +	int			irq_count;

I guess you won't be needing these.

> +#define BUFFER_VCHANS_LIMITED		0x3f
> +#define BUFFER_VCHANS_ALL		0xff

Can you remove all these defines from inside the struct definition
please?

> +	u8			buffer_vchans;
> +
> +	/*
> +	 * Certain LRADC channels are shared between touchscreen
> +	 * and/or touch-buttons and generic LRADC block. Therefore when using
> +	 * either of these, these channels are not available for the regular
> +	 * sampling. The shared channels are as follows:
> +	 *
> +	 * CH0 -- Touch button #0
> +	 * CH1 -- Touch button #1
> +	 * CH2 -- Touch screen XPUL
> +	 * CH3 -- Touch screen YPLL
> +	 * CH4 -- Touch screen XNUL
> +	 * CH5 -- Touch screen YNLR
> +	 * CH6 -- Touch screen WIPER (5-wire only)
> +	 *
> +	 * The bit fields below represents which parts of the LRADC block are
> +	 * switched into special mode of operation. These channels can not
> +	 * be sampled as regular LRADC channels. The driver will refuse any
> +	 * attempt to sample these channels.
> +	 */
> +#define CHAN_MASK_TOUCHBUTTON		(BIT(1) | BIT(0))
> +#define CHAN_MASK_TOUCHSCREEN_4WIRE	(0xf << 2)
> +#define CHAN_MASK_TOUCHSCREEN_5WIRE	(0x1f << 2)
> +	enum mxs_lradc_ts_wires	use_touchscreen;
> +	bool			use_touchbutton;
> +
> +#define MXS_LRADC_CELLS			2
> +	struct mfd_cell cells[MXS_LRADC_CELLS];
> +};
> +
> +static inline void mxs_lradc_reg_set(struct mxs_lradc *lradc, u32 val, u32 reg)
> +{
> +	writel(val, lradc->base + reg + STMP_OFFSET_REG_SET);
> +}
> +
> +static inline void mxs_lradc_reg_clear(struct mxs_lradc *lradc, u32 val,
> +				       u32 reg)
> +{
> +	writel(val, lradc->base + reg + STMP_OFFSET_REG_CLR);
> +}
> +
> +static inline void mxs_lradc_reg_wrt(struct mxs_lradc *lradc, u32 val, u32 reg)
> +{
> +	writel(val, lradc->base + reg);
> +}

Why do you need to abstract the well know, accepted writel calls?

> +static inline u32 mxs_lradc_irq_mask(struct mxs_lradc *lradc)
> +{
> +	switch (lradc->soc) {
> +	case IMX23_LRADC:
> +		return LRADC_CTRL1_MX23_LRADC_IRQ_MASK;
> +	case IMX28_LRADC:
> +		return LRADC_CTRL1_MX28_LRADC_IRQ_MASK;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +#endif /* __MXS_LRADC_H */

Ksenija Stanojevic July 1, 2016, 11:47 a.m. UTC | #3

Hi Lee,

Thank you for the review.

On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 24 Jun 2016, Ksenija Stanojevic wrote:
>
>> Add core files for mxs-lradc MFD driver.
>>
>> Note:  this patch won't compile in iio/testing without this patch:
>> a8f447be8056 ("mfd: Add resource managed APIs for mfd_add_devices")
>>
>> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
>> Acked-by: Jonathan Cameron <jic23@kernel.org>
>> ---
>> Changes in v3:
>>  - add note to commit message
>>  - move switch statement into if(touch_ret == 0) branch
>>  - add MODULE_AUTHOR
>>
>> Changes in v2:
>>  - do not change spacing in Kconfig
>>  - make struct mfd_cell part of struct mxs_lradc
>>  - use switch instead of if in mxs_lradc_irq_mask
>>  - use only necessary header files in mxs_lradc.h
>>  - use devm_mfd_add_device
>>  - use separate function to register mfd device
>>  - change licence to GPL
>>  - add copyright
>>
>>  drivers/mfd/Kconfig           |  17 ++++
>>  drivers/mfd/Makefile          |   1 +
>>  drivers/mfd/mxs-lradc.c       | 217 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/mxs-lradc.h | 204 +++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 439 insertions(+)
>>  create mode 100644 drivers/mfd/mxs-lradc.c
>>  create mode 100644 include/linux/mfd/mxs-lradc.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 9ca66de..62593ad 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -271,6 +271,23 @@ config MFD_MC13XXX_I2C
>>       help
>>         Select this if your MC13xxx is connected via an I2C bus.
>>
>> +config MFD_MXS_LRADC
>> +     tristate "Freescale i.MX23/i.MX28 LRADC"
>> +     depends on ARCH_MXS || COMPILE_TEST
>> +     select MFD_CORE
>> +     select STMP_DEVICE
>> +     help
>> +       Say yes here to build support for the low-resolution
>> +       analog-to-digital converter (LRADC) found on the i.MX23 and i.MX28
>
> Low Resolution Analogue-to-Digital Converter
>
>> +       processors. This driver provides common support for accessing the
>> +       device, additional drivers must be enabled in order to use the
>> +       functionality of the device:
>> +             mxs-lradc-adc for ADC readings
>> +             mxs-lradc-ts  for touchscreen support
>> +
>> +       This driver can also be built as a module. If so, the module will be
>> +       called mxs-lradc.
>> +
>>  config MFD_HI6421_PMIC
>>       tristate "HiSilicon Hi6421 PMU/Codec IC"
>>       depends on OF
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 0f230a6..ecf6399 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -198,3 +198,4 @@ intel-soc-pmic-objs               := intel_soc_pmic_core.o intel_soc_pmic_crc.o
>>  intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC)       += intel_soc_pmic_bxtwc.o
>>  obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
>>  obj-$(CONFIG_MFD_MT6397)     += mt6397-core.o
>> +obj-$(CONFIG_MFD_MXS_LRADC)  += mxs-lradc.o
>> diff --git a/drivers/mfd/mxs-lradc.c b/drivers/mfd/mxs-lradc.c
>> new file mode 100644
>> index 0000000..d53a524
>> --- /dev/null
>> +++ b/drivers/mfd/mxs-lradc.c
>> @@ -0,0 +1,217 @@
>> +/*
>> + * Freescale MXS LRADC driver
>> + *
>> + * Copyright (c) 2012 DENX Software Engineering, GmbH.
>
>
> You need to update the copyright.
>
> Authors:
>
>> + * Marek Vasut <marex@denx.de>
>> + * Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/device.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/mxs-lradc.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +
>> +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",
>> +     "mxs-lradc-channel0",
>> +     "mxs-lradc-channel1",
>> +     "mxs-lradc-channel2",
>> +     "mxs-lradc-channel3",
>> +     "mxs-lradc-channel4",
>> +     "mxs-lradc-channel5",
>> +     "mxs-lradc-channel6",
>> +     "mxs-lradc-channel7",
>> +     "mxs-lradc-button0",
>> +     "mxs-lradc-button1",
>> +};
>> +
>> +struct mxs_lradc_of_config {
>> +     const int               irq_count;
>> +     const char * const      *irq_name;
>> +};
>> +
>> +static const struct mxs_lradc_of_config 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,
>> +     },
>> +};
>
> Please use DEFINE_RES_IRQ_NAMED() instead.
>
>> +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_add_device(struct platform_device *pdev,
>> +                             struct mxs_lradc *lradc, char *name, int i)
>> +{
>> +     struct mfd_cell *cell;
>> +
>> +     cell = &lradc->cells[i];
>> +     cell->name = name;
>> +     cell->platform_data = lradc;
>> +     cell->pdata_size = sizeof(*lradc);
>> +
>> +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);
>> +}
>
> Please don't roll your own API.
>
> Use 'struct mfd_cell' like everyone else does.

It has been suggested in previous reviews to use separate function to
register mfd device, and to make mfd_cell allocate dynamically because
struc mxs-lradc is allocated dynamically.
But I can revrse changes and make mfd_cells allocate staticaly
wthout separate function.

>> +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];
>
> This implementation is wayyyy more complicated than it needs to be.
>
> You've entangled DT probing with the MFD API.
>
> Please use the MFD API properly.
>
>> +     struct device *dev = &pdev->dev;
>> +     struct device_node *node = dev->of_node;
>> +     struct mxs_lradc *lradc;
>> +     struct resource *iores;
>> +     int ret = 0, touch_ret, i;
>> +     u32 ts_wires = 0;
>> +
>> +     lradc = devm_kzalloc(&pdev->dev, sizeof(*lradc), GFP_KERNEL);
>> +     if (!lradc)
>> +             return -ENOMEM;
>> +     lradc->soc = (enum mxs_lradc_id)of_id->data;
>
> If you're going to pull the platform ID out like this, please pull the
> *_match_* function down, just above this line.  And separate them both
> from allocating memory.

Ok, but then I will also need to pull this line also:
const struct mxs_lradc_of_config *of_cfg =
 &mxs_lradc_of_config[(enum mxs_lradc_id)of_id->data];

>> +     /* Grab the memory area */
>
> This comment doesn't add anything.

Ok, I will remove it.

> We know what these calls do, and they don't "grab" anything.
>
>> +     iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     lradc->base = devm_ioremap_resource(dev, iores);
>> +     if (IS_ERR(lradc->base))
>> +             return PTR_ERR(lradc->base);
>> +
>> +     lradc->clk = devm_clk_get(&pdev->dev, NULL);
>> +     if (IS_ERR(lradc->clk)) {
>> +             dev_err(dev, "Failed to get the delay unit clock\n");
>> +             return PTR_ERR(lradc->clk);
>> +     }
>
> '\n' here.  Nice spacing makes code easier to follow.
>
>> +     ret = clk_prepare_enable(lradc->clk);
>> +     if (ret != 0) {
>> +             dev_err(dev, "Failed to enable the delay unit clock\n");
>> +             return ret;
>> +     }
>> +
>> +     touch_ret = of_property_read_u32(node, "fsl,lradc-touchscreen-wires",
>> +                                      &ts_wires);
>
> Why do you need more than one 'ret' variable?

Yeah, it's not really needed.

>> +     if (touch_ret == 0) {
>> +             lradc->buffer_vchans = BUFFER_VCHANS_LIMITED;
>
> '\n' here  or else we think it's related to the switch().
>
>> +             switch (ts_wires) {
>> +             case 4:
>> +                     lradc->use_touchscreen = MXS_LRADC_TOUCHSCREEN_4WIRE;
>
> This is not a good name for a variable.  It looks like a bool here,
> but you're initialising with a configuration option.

Ok, will touchscreen_wire be ok?

>> +                     break;
>> +             case 5:
>> +                     if (lradc->soc == IMX28_LRADC) {
>> +                             lradc->use_touchscreen =
>> +                                     MXS_LRADC_TOUCHSCREEN_5WIRE;
>> +                             break;
>> +                     }
>> +                     /* fall through an error message for i.MX23 */
>
> "to an error"
>
>> +             default:
>> +                     dev_err(&pdev->dev,
>> +                             "Unsupported number of touchscreen wires (%d)\n",
>> +                             ts_wires);
>> +                     return -EINVAL;
>
> The clock is still enabled.

Ok, I will use goto err_clk

>> +             }
>> +     } else {
>> +             lradc->buffer_vchans = BUFFER_VCHANS_ALL;
>> +     }
>> +
>> +     lradc->irq_count = of_cfg->irq_count;
>> +     lradc->irq_name = of_cfg->irq_name;
>> +     for (i = 0; i < lradc->irq_count; i++) {
>> +             lradc->irq[i] = platform_get_irq(pdev, i);
>> +             if (lradc->irq[i] < 0) {
>> +                     ret = lradc->irq[i];
>> +                     goto err_clk;
>> +             }
>> +     }
>
> You shouldn't be 'get'ing the IRQ here.
>
> Where are you consuming these IRQs?

There are used in adc (mxs-lradc-adc) and touchscreen (mxs-lradc-ts) part.

>> +     platform_set_drvdata(pdev, lradc);
>> +
>> +     ret = stmp_reset_block(lradc->base);
>> +     if (ret)
>> +             return ret;
>
>
> The clock is still enabled.
>
Ok, I will use goto err_clk

>> +     ret = mxs_lradc_add_device(pdev, lradc, DRIVER_NAME_ADC, 0);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to add the ADC subdevice\n");
>> +             goto err_clk;
>> +     }
>
> Do this using statically defined mfd_cells.
>
>> +     if (!lradc->use_touchscreen)
>> +             return 0;
>> +
>> +     ret = mxs_lradc_add_device(pdev, lradc, DRIVER_NAME_TS, 1);
>> +     if (ret) {
>> +             dev_err(&pdev->dev,
>> +                     "Failed to add the touchscreen subdevice\n");
>> +             goto err_clk;
>> +     }
>> +
>> +     return 0;
>> +
>> +err_clk:
>> +     clk_disable_unprepare(lradc->clk);
>> +
>> +     return ret;
>> +}
>> +
>> +static int mxs_lradc_remove(struct platform_device *pdev)
>> +{
>> +     struct mxs_lradc *lradc = platform_get_drvdata(pdev);
>> +
>> +     clk_disable_unprepare(lradc->clk);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct platform_driver mxs_lradc_driver = {
>> +     .driver = {
>> +             .name = "mxs-lradc",
>> +             .of_match_table = mxs_lradc_dt_ids,
>> +     },
>> +     .probe = mxs_lradc_probe,
>> +     .remove = mxs_lradc_remove,
>> +};
>> +
>
> This line is superfluous.
>
>> +module_platform_driver(mxs_lradc_driver);
>> +
>> +MODULE_AUTHOR("Ksenija Stanojevic <ksenija.stanojevic@gmail.com>");
>> +MODULE_DESCRIPTION("Freescale i.MX23/i.MX28 LRADC driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:mxs-lradc");
>> diff --git a/include/linux/mfd/mxs-lradc.h b/include/linux/mfd/mxs-lradc.h
>> new file mode 100644
>> index 0000000..c1475c0
>> --- /dev/null
>> +++ b/include/linux/mfd/mxs-lradc.h
>> @@ -0,0 +1,204 @@
>> +/*
>> + * Freescale MXS LRADC driver
>> + *
>> + * Copyright (c) 2012 DENX Software Engineering, GmbH.
>
> You need to update the copyright.
>
> Author:
>
>> + * Marek Vasut <marex@denx.de>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef __MXS_LRADC_H
>> +#define __MXS_LRADC_H
>
> __MFD_MXS_*
>
>> +#include <linux/bitops.h>
>> +#include <linux/io.h>
>> +#include <linux/stmp_device.h>
>> +
>> +#define DRIVER_NAME_ADC "mxs-lradc-adc"
>> +#define DRIVER_NAME_TS "mxs-lradc-ts"
>
> Don't define these.  Just use the proper names.

Ok.

>> +#define LRADC_MAX_DELAY_CHANS        4
>> +#define LRADC_MAX_MAPPED_CHANS       8
>> +#define LRADC_MAX_TOTAL_CHANS        16
>> +
>> +#define LRADC_DELAY_TIMER_HZ 2000
>> +
>> +#define LRADC_CTRL0                          0x00
>> +# define LRADC_CTRL0_MX28_TOUCH_DETECT_ENABLE        BIT(23)
>> +# define LRADC_CTRL0_MX28_TOUCH_SCREEN_TYPE  BIT(22)
>> +# define LRADC_CTRL0_MX28_YNNSW /* YM */     BIT(21)
>> +# define LRADC_CTRL0_MX28_YPNSW /* YP */     BIT(20)
>> +# define LRADC_CTRL0_MX28_YPPSW /* YP */     BIT(19)
>> +# define LRADC_CTRL0_MX28_XNNSW /* XM */     BIT(18)
>> +# define LRADC_CTRL0_MX28_XNPSW /* XM */     BIT(17)
>> +# define LRADC_CTRL0_MX28_XPPSW /* XP */     BIT(16)
>> +
>> +# define LRADC_CTRL0_MX23_TOUCH_DETECT_ENABLE        BIT(20)
>> +# define LRADC_CTRL0_MX23_YM                 BIT(19)
>> +# define LRADC_CTRL0_MX23_XM                 BIT(18)
>> +# define LRADC_CTRL0_MX23_YP                 BIT(17)
>> +# define LRADC_CTRL0_MX23_XP                 BIT(16)
>> +
>> +# define LRADC_CTRL0_MX28_PLATE_MASK \
>> +             (LRADC_CTRL0_MX28_TOUCH_DETECT_ENABLE | \
>> +             LRADC_CTRL0_MX28_YNNSW | LRADC_CTRL0_MX28_YPNSW | \
>> +             LRADC_CTRL0_MX28_YPPSW | LRADC_CTRL0_MX28_XNNSW | \
>> +             LRADC_CTRL0_MX28_XNPSW | LRADC_CTRL0_MX28_XPPSW)
>> +
>> +# define LRADC_CTRL0_MX23_PLATE_MASK \
>> +             (LRADC_CTRL0_MX23_TOUCH_DETECT_ENABLE | \
>> +             LRADC_CTRL0_MX23_YM | LRADC_CTRL0_MX23_XM | \
>> +             LRADC_CTRL0_MX23_YP | LRADC_CTRL0_MX23_XP)
>> +
>> +#define LRADC_CTRL1                          0x10
>> +#define LRADC_CTRL1_TOUCH_DETECT_IRQ_EN              BIT(24)
>> +#define LRADC_CTRL1_LRADC_IRQ_EN(n)          (1 << ((n) + 16))
>> +#define LRADC_CTRL1_MX28_LRADC_IRQ_EN_MASK   (0x1fff << 16)
>> +#define LRADC_CTRL1_MX23_LRADC_IRQ_EN_MASK   (0x01ff << 16)
>> +#define LRADC_CTRL1_LRADC_IRQ_EN_OFFSET              16
>> +#define LRADC_CTRL1_TOUCH_DETECT_IRQ         BIT(8)
>> +#define LRADC_CTRL1_LRADC_IRQ(n)             (1 << (n))
>> +#define LRADC_CTRL1_MX28_LRADC_IRQ_MASK              0x1fff
>> +#define LRADC_CTRL1_MX23_LRADC_IRQ_MASK              0x01ff
>> +#define LRADC_CTRL1_LRADC_IRQ_OFFSET         0
>> +
>> +#define LRADC_CTRL2                          0x20
>> +#define LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET     24
>> +#define LRADC_CTRL2_TEMPSENSE_PWD            BIT(15)
>> +
>> +#define LRADC_STATUS                         0x40
>> +#define LRADC_STATUS_TOUCH_DETECT_RAW                BIT(0)
>> +
>> +#define LRADC_CH(n)                          (0x50 + (0x10 * (n)))
>> +#define LRADC_CH_ACCUMULATE                  BIT(29)
>> +#define LRADC_CH_NUM_SAMPLES_MASK            (0x1f << 24)
>> +#define LRADC_CH_NUM_SAMPLES_OFFSET          24
>> +#define LRADC_CH_NUM_SAMPLES(x) \
>> +                             ((x) << LRADC_CH_NUM_SAMPLES_OFFSET)
>> +#define LRADC_CH_VALUE_MASK                  0x3ffff
>> +#define LRADC_CH_VALUE_OFFSET                        0
>> +
>> +#define LRADC_DELAY(n)                               (0xd0 + (0x10 * (n)))
>> +#define LRADC_DELAY_TRIGGER_LRADCS_MASK              (0xffUL << 24)
>> +#define LRADC_DELAY_TRIGGER_LRADCS_OFFSET    24
>> +#define LRADC_DELAY_TRIGGER(x) \
>> +                             (((x) << LRADC_DELAY_TRIGGER_LRADCS_OFFSET) & \
>> +                             LRADC_DELAY_TRIGGER_LRADCS_MASK)
>> +#define LRADC_DELAY_KICK                     BIT(20)
>> +#define LRADC_DELAY_TRIGGER_DELAYS_MASK              (0xf << 16)
>> +#define LRADC_DELAY_TRIGGER_DELAYS_OFFSET    16
>> +#define LRADC_DELAY_TRIGGER_DELAYS(x) \
>> +                             (((x) << LRADC_DELAY_TRIGGER_DELAYS_OFFSET) & \
>> +                             LRADC_DELAY_TRIGGER_DELAYS_MASK)
>> +#define LRADC_DELAY_LOOP_COUNT_MASK          (0x1f << 11)
>> +#define LRADC_DELAY_LOOP_COUNT_OFFSET                11
>> +#define LRADC_DELAY_LOOP(x) \
>> +                             (((x) << LRADC_DELAY_LOOP_COUNT_OFFSET) & \
>> +                             LRADC_DELAY_LOOP_COUNT_MASK)
>> +#define LRADC_DELAY_DELAY_MASK                       0x7ff
>> +#define LRADC_DELAY_DELAY_OFFSET             0
>> +#define LRADC_DELAY_DELAY(x) \
>> +                             (((x) << LRADC_DELAY_DELAY_OFFSET) & \
>> +                             LRADC_DELAY_DELAY_MASK)
>> +
>> +#define LRADC_CTRL4                          0x140
>> +#define LRADC_CTRL4_LRADCSELECT_MASK(n)              (0xf << ((n) * 4))
>> +#define LRADC_CTRL4_LRADCSELECT_OFFSET(n)    ((n) * 4)
>> +#define LRADC_CTRL4_LRADCSELECT(n, x) \
>> +                             (((x) << LRADC_CTRL4_LRADCSELECT_OFFSET(n)) & \
>> +                             LRADC_CTRL4_LRADCSELECT_MASK(n))
>> +
>> +#define LRADC_RESOLUTION                     12
>> +#define LRADC_SINGLE_SAMPLE_MASK             ((1 << LRADC_RESOLUTION) - 1)
>> +
>> +enum mxs_lradc_id {
>> +     IMX23_LRADC,
>> +     IMX28_LRADC,
>> +};
>> +
>> +enum mxs_lradc_ts_wires {
>> +     MXS_LRADC_TOUCHSCREEN_NONE = 0,
>> +     MXS_LRADC_TOUCHSCREEN_4WIRE,
>> +     MXS_LRADC_TOUCHSCREEN_5WIRE,
>> +};
>> +
>> +struct mxs_lradc {
>> +     enum mxs_lradc_id       soc;
>> +
>> +     void __iomem            *base;
>> +     struct clk              *clk;
>> +
>> +     int                     irq[13];
>> +     const char * const      *irq_name;
>> +     int                     irq_count;
>
> I guess you won't be needing these.
>
>> +#define BUFFER_VCHANS_LIMITED                0x3f
>> +#define BUFFER_VCHANS_ALL            0xff
>
> Can you remove all these defines from inside the struct definition
> please?

Ok.

>> +     u8                      buffer_vchans;
>> +
>> +     /*
>> +      * Certain LRADC channels are shared between touchscreen
>> +      * and/or touch-buttons and generic LRADC block. Therefore when using
>> +      * either of these, these channels are not available for the regular
>> +      * sampling. The shared channels are as follows:
>> +      *
>> +      * CH0 -- Touch button #0
>> +      * CH1 -- Touch button #1
>> +      * CH2 -- Touch screen XPUL
>> +      * CH3 -- Touch screen YPLL
>> +      * CH4 -- Touch screen XNUL
>> +      * CH5 -- Touch screen YNLR
>> +      * CH6 -- Touch screen WIPER (5-wire only)
>> +      *
>> +      * The bit fields below represents which parts of the LRADC block are
>> +      * switched into special mode of operation. These channels can not
>> +      * be sampled as regular LRADC channels. The driver will refuse any
>> +      * attempt to sample these channels.
>> +      */
>> +#define CHAN_MASK_TOUCHBUTTON                (BIT(1) | BIT(0))
>> +#define CHAN_MASK_TOUCHSCREEN_4WIRE  (0xf << 2)
>> +#define CHAN_MASK_TOUCHSCREEN_5WIRE  (0x1f << 2)
>> +     enum mxs_lradc_ts_wires use_touchscreen;
>> +     bool                    use_touchbutton;
>> +
>> +#define MXS_LRADC_CELLS                      2
>> +     struct mfd_cell cells[MXS_LRADC_CELLS];
>> +};
>> +
>> +static inline void mxs_lradc_reg_set(struct mxs_lradc *lradc, u32 val, u32 reg)
>> +{
>> +     writel(val, lradc->base + reg + STMP_OFFSET_REG_SET);
>> +}
>> +
>> +static inline void mxs_lradc_reg_clear(struct mxs_lradc *lradc, u32 val,
>> +                                    u32 reg)
>> +{
>> +     writel(val, lradc->base + reg + STMP_OFFSET_REG_CLR);
>> +}
>> +
>> +static inline void mxs_lradc_reg_wrt(struct mxs_lradc *lradc, u32 val, u32 reg)
>> +{
>> +     writel(val, lradc->base + reg);
>> +}
>
> Why do you need to abstract the well know, accepted writel calls?

It makes code more readable.

>> +static inline u32 mxs_lradc_irq_mask(struct mxs_lradc *lradc)
>> +{
>> +     switch (lradc->soc) {
>> +     case IMX23_LRADC:
>> +             return LRADC_CTRL1_MX23_LRADC_IRQ_MASK;
>> +     case IMX28_LRADC:
>> +             return LRADC_CTRL1_MX28_LRADC_IRQ_MASK;
>> +     default:
>> +             return 0;
>> +     }
>> +}
>> +
>> +#endif /* __MXS_LRADC_H */

Regards,
Ksenija
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Harald Geyer July 1, 2016, 1:10 p.m. UTC | #4

Hi Ksenija!

Ksenija Stanojević writes:
> On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> +static int mxs_lradc_add_device(struct platform_device *pdev,
> >> +                             struct mxs_lradc *lradc, char *name, int i)
> >> +{
> >> +     struct mfd_cell *cell;
> >> +
> >> +     cell = &lradc->cells[i];
> >> +     cell->name = name;
> >> +     cell->platform_data = lradc;
> >> +     cell->pdata_size = sizeof(*lradc);
> >> +
> >> +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);
> >> +}
> >
> > Please don't roll your own API.
> >
> > Use 'struct mfd_cell' like everyone else does.
> 
> It has been suggested in previous reviews to use separate function to
> register mfd device, and to make mfd_cell allocate dynamically because
> struc mxs-lradc is allocated dynamically.
> But I can revrse changes and make mfd_cells allocate staticaly
> wthout separate function.

I think making mfd_cells members of struct mxs-lradc will address all
review comments.

HTH,
Harald
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lee Jones July 13, 2016, 12:49 p.m. UTC | #5

On Fri, 01 Jul 2016, Harald Geyer wrote:

> Hi Ksenija!
> 
> Ksenija Stanojević writes:
> > On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > >> +static int mxs_lradc_add_device(struct platform_device *pdev,
> > >> +                             struct mxs_lradc *lradc, char *name, int i)
> > >> +{
> > >> +     struct mfd_cell *cell;
> > >> +
> > >> +     cell = &lradc->cells[i];
> > >> +     cell->name = name;
> > >> +     cell->platform_data = lradc;
> > >> +     cell->pdata_size = sizeof(*lradc);
> > >> +
> > >> +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);
> > >> +}
> > >
> > > Please don't roll your own API.
> > >
> > > Use 'struct mfd_cell' like everyone else does.
> > 
> > It has been suggested in previous reviews to use separate function to
> > register mfd device, and to make mfd_cell allocate dynamically because
> > struc mxs-lradc is allocated dynamically.
> > But I can revrse changes and make mfd_cells allocate staticaly
> > wthout separate function.
> 
> I think making mfd_cells members of struct mxs-lradc will address all
> review comments.

No, please don't do that either.

Marek Vasut July 14, 2016, 3:38 p.m. UTC | #6

On 07/13/2016 02:49 PM, Lee Jones wrote:
> On Fri, 01 Jul 2016, Harald Geyer wrote:
>
>> Hi Ksenija!
>>
>> Ksenija Stanojević writes:
>>> On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
>>>>> +static int mxs_lradc_add_device(struct platform_device *pdev,
>>>>> +                             struct mxs_lradc *lradc, char *name, int i)
>>>>> +{
>>>>> +     struct mfd_cell *cell;
>>>>> +
>>>>> +     cell = &lradc->cells[i];
>>>>> +     cell->name = name;
>>>>> +     cell->platform_data = lradc;
>>>>> +     cell->pdata_size = sizeof(*lradc);
>>>>> +
>>>>> +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);
>>>>> +}
>>>>
>>>> Please don't roll your own API.
>>>>
>>>> Use 'struct mfd_cell' like everyone else does.
>>>
>>> It has been suggested in previous reviews to use separate function to
>>> register mfd device, and to make mfd_cell allocate dynamically because
>>> struc mxs-lradc is allocated dynamically.
>>> But I can revrse changes and make mfd_cells allocate staticaly
>>> wthout separate function.
>>
>> I think making mfd_cells members of struct mxs-lradc will address all
>> review comments.
>
> No, please don't do that either.
>
It'd be nice if you explained in detail why not. Otherwise this is just 
empty splat.

Stefan Wahren Aug. 2, 2016, 4:35 p.m. UTC | #7

Hi,

> Marek Vasut <marex@denx.de> hat am 14. Juli 2016 um 17:38 geschrieben:
> 
> 
> On 07/13/2016 02:49 PM, Lee Jones wrote:
> > On Fri, 01 Jul 2016, Harald Geyer wrote:
> >
> >> Hi Ksenija!
> >>
> >> Ksenija Stanojević writes:
> >>> On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >>>>> +static int mxs_lradc_add_device(struct platform_device *pdev,
> >>>>> +                             struct mxs_lradc *lradc, char *name, int
> >>>>> i)
> >>>>> +{
> >>>>> +     struct mfd_cell *cell;
> >>>>> +
> >>>>> +     cell = &lradc->cells[i];
> >>>>> +     cell->name = name;
> >>>>> +     cell->platform_data = lradc;
> >>>>> +     cell->pdata_size = sizeof(*lradc);
> >>>>> +
> >>>>> +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0,
> >>>>> NULL);
> >>>>> +}
> >>>>
> >>>> Please don't roll your own API.
> >>>>
> >>>> Use 'struct mfd_cell' like everyone else does.
> >>>
> >>> It has been suggested in previous reviews to use separate function to
> >>> register mfd device, and to make mfd_cell allocate dynamically because
> >>> struc mxs-lradc is allocated dynamically.
> >>> But I can revrse changes and make mfd_cells allocate staticaly
> >>> wthout separate function.
> >>
> >> I think making mfd_cells members of struct mxs-lradc will address all
> >> review comments.
> >
> > No, please don't do that either.
> >
> It'd be nice if you explained in detail why not. Otherwise this is just 
> empty splat.

since there is no reply, here is my guess:

static const struct mfd_cell mxs_lradc_devs[] = {
	{
		.name = DRIVER_NAME_ADC,
	},
	{
		.name = DRIVER_NAME_TS,
	},
};

But i'm not sure if we need of_compatible defined here. The intension of this
patch series is to keep the DT binding.

@Lee: Could you please give us a feedback?

@Ksenija: Still motivated for next round?

Regards
Stefan

> 
> -- 
> Best regards,
> Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ksenija Stanojevic Aug. 2, 2016, 5:08 p.m. UTC | #8

Hi,

On Tue, Aug 2, 2016 at 6:35 PM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> Hi,
>
>> Marek Vasut <marex@denx.de> hat am 14. Juli 2016 um 17:38 geschrieben:
>>
>>
>> On 07/13/2016 02:49 PM, Lee Jones wrote:
>> > On Fri, 01 Jul 2016, Harald Geyer wrote:
>> >
>> >> Hi Ksenija!
>> >>
>> >> Ksenija Stanojević writes:
>> >>> On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> >>>>> +static int mxs_lradc_add_device(struct platform_device *pdev,
>> >>>>> +                             struct mxs_lradc *lradc, char *name, int
>> >>>>> i)
>> >>>>> +{
>> >>>>> +     struct mfd_cell *cell;
>> >>>>> +
>> >>>>> +     cell = &lradc->cells[i];
>> >>>>> +     cell->name = name;
>> >>>>> +     cell->platform_data = lradc;
>> >>>>> +     cell->pdata_size = sizeof(*lradc);
>> >>>>> +
>> >>>>> +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0,
>> >>>>> NULL);
>> >>>>> +}
>> >>>>
>> >>>> Please don't roll your own API.
>> >>>>
>> >>>> Use 'struct mfd_cell' like everyone else does.
>> >>>
>> >>> It has been suggested in previous reviews to use separate function to
>> >>> register mfd device, and to make mfd_cell allocate dynamically because
>> >>> struc mxs-lradc is allocated dynamically.
>> >>> But I can revrse changes and make mfd_cells allocate staticaly
>> >>> wthout separate function.
>> >>
>> >> I think making mfd_cells members of struct mxs-lradc will address all
>> >> review comments.
>> >
>> > No, please don't do that either.
>> >
>> It'd be nice if you explained in detail why not. Otherwise this is just
>> empty splat.
>
> since there is no reply, here is my guess:

Sorry for the delay, I'm currently working on it. I will post another
version soon.

> static const struct mfd_cell mxs_lradc_devs[] = {
>         {
>                 .name = DRIVER_NAME_ADC,
>         },
>         {
>                 .name = DRIVER_NAME_TS,
>         },
> };
>
> But i'm not sure if we need of_compatible defined here. The intension of this
> patch series is to keep the DT binding.

I think it needs .resources because in next version DEFINE_RES_IRQ_NAMED
will be used.

> @Lee: Could you please give us a feedback?
>
> @Ksenija: Still motivated for next round?
>
> Regards
> Stefan
>
>>
>> --
>> Best regards,
>> Marek Vasut

Regards,
Ksenija
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ksenija Stanojevic Aug. 3, 2016, 4:32 p.m. UTC | #9

Hi All,

On Tue, Aug 2, 2016 at 7:08 PM, Ksenija Stanojević
<ksenija.stanojevic@gmail.com> wrote:
> Hi,
>
> On Tue, Aug 2, 2016 at 6:35 PM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>> Hi,
>>
>>> Marek Vasut <marex@denx.de> hat am 14. Juli 2016 um 17:38 geschrieben:
>>>
>>>
>>> On 07/13/2016 02:49 PM, Lee Jones wrote:
>>> > On Fri, 01 Jul 2016, Harald Geyer wrote:
>>> >
>>> >> Hi Ksenija!
>>> >>
>>> >> Ksenija Stanojević writes:
>>> >>> On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
>>> >>>>> +static int mxs_lradc_add_device(struct platform_device *pdev,
>>> >>>>> +                             struct mxs_lradc *lradc, char *name, int
>>> >>>>> i)
>>> >>>>> +{
>>> >>>>> +     struct mfd_cell *cell;
>>> >>>>> +
>>> >>>>> +     cell = &lradc->cells[i];
>>> >>>>> +     cell->name = name;
>>> >>>>> +     cell->platform_data = lradc;
>>> >>>>> +     cell->pdata_size = sizeof(*lradc);
>>> >>>>> +
>>> >>>>> +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0,
>>> >>>>> NULL);
>>> >>>>> +}
>>> >>>>
>>> >>>> Please don't roll your own API.
>>> >>>>
>>> >>>> Use 'struct mfd_cell' like everyone else does.
>>> >>>
>>> >>> It has been suggested in previous reviews to use separate function to
>>> >>> register mfd device, and to make mfd_cell allocate dynamically because
>>> >>> struc mxs-lradc is allocated dynamically.
>>> >>> But I can revrse changes and make mfd_cells allocate staticaly
>>> >>> wthout separate function.
>>> >>
>>> >> I think making mfd_cells members of struct mxs-lradc will address all
>>> >> review comments.
>>> >
>>> > No, please don't do that either.
>>> >
>>> It'd be nice if you explained in detail why not. Otherwise this is just
>>> empty splat.
>>
>> since there is no reply, here is my guess:
>
> Sorry for the delay, I'm currently working on it. I will post another
> version soon.
>
>> static const struct mfd_cell mxs_lradc_devs[] = {
>>         {
>>                 .name = DRIVER_NAME_ADC,
>>         },
>>         {
>>                 .name = DRIVER_NAME_TS,
>>         },
>> };
>>
>> But i'm not sure if we need of_compatible defined here. The intension of this
>> patch series is to keep the DT binding.
>
> I think it needs .resources because in next version DEFINE_RES_IRQ_NAMED
> will be used.
>
>> @Lee: Could you please give us a feedback?
>>
>> @Ksenija: Still motivated for next round?

Can someone with imx23 board send me /proc/interrupts log, I
need irq numbers...

Thanks,
Ksenija
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Fabio Estevam Aug. 3, 2016, 4:35 p.m. UTC | #10

Hi Ksenija,

On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
<ksenija.stanojevic@gmail.com> wrote:

> Can someone with imx23 board send me /proc/interrupts log, I
> need irq numbers...

Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
Interrupt Sources)?
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ksenija Stanojevic Aug. 3, 2016, 4:45 p.m. UTC | #11

Hi Fabio,

On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Ksenija,
>
> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
> <ksenija.stanojevic@gmail.com> wrote:
>
>> Can someone with imx23 board send me /proc/interrupts log, I
>> need irq numbers...
>
> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
> Interrupt Sources)?

I thought it will be more reliable if I asked someone, since numbers from MX28
manual don't match with /proc/interrupts log.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marek Vasut Aug. 3, 2016, 7:13 p.m. UTC | #12

On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
> Hi Fabio,

Hi,

> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> Hi Ksenija,
>>
>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>> <ksenija.stanojevic@gmail.com> wrote:
>>
>>> Can someone with imx23 board send me /proc/interrupts log, I
>>> need irq numbers...
>>
>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>> Interrupt Sources)?
> 
> I thought it will be more reliable if I asked someone, since numbers from MX28
> manual don't match with /proc/interrupts log.
> 
What do you mean they don't match ? Please elaborate.

Ksenija Stanojevic Aug. 3, 2016, 7:34 p.m. UTC | #13

On Wed, Aug 3, 2016 at 9:13 PM, Marek Vasut <marex@denx.de> wrote:
> On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
>> Hi Fabio,
>
> Hi,
>
>> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>> Hi Ksenija,
>>>
>>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>>> <ksenija.stanojevic@gmail.com> wrote:
>>>
>>>> Can someone with imx23 board send me /proc/interrupts log, I
>>>> need irq numbers...
>>>
>>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>>> Interrupt Sources)?
>>
>> I thought it will be more reliable if I asked someone, since numbers from MX28
>> manual don't match with /proc/interrupts log.
>>
> What do you mean they don't match ? Please elaborate.

irqs don't have the same values in Table 5.1 in MX28 manual and in
/proc/interrupts
output.

For example:
touchscreen irq in MX28 manual have these values:
10 (source number) and 0x0028 (vector number)

in /proc/interrupts:
210:          0         -  10 Edge      mxs-lradc-touchscreen
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marek Vasut Aug. 3, 2016, 7:39 p.m. UTC | #14

On 08/03/2016 09:34 PM, Ksenija Stanojević wrote:
> On Wed, Aug 3, 2016 at 9:13 PM, Marek Vasut <marex@denx.de> wrote:
>> On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
>>> Hi Fabio,
>>
>> Hi,
>>
>>> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>>> Hi Ksenija,
>>>>
>>>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>>>> <ksenija.stanojevic@gmail.com> wrote:
>>>>
>>>>> Can someone with imx23 board send me /proc/interrupts log, I
>>>>> need irq numbers...
>>>>
>>>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>>>> Interrupt Sources)?
>>>
>>> I thought it will be more reliable if I asked someone, since numbers from MX28
>>> manual don't match with /proc/interrupts log.
>>>
>> What do you mean they don't match ? Please elaborate.
> 
> irqs don't have the same values in Table 5.1 in MX28 manual and in
> /proc/interrupts
> output.
> 
> For example:
> touchscreen irq in MX28 manual have these values:
> 10 (source number) and 0x0028 (vector number)
> 
> in /proc/interrupts:
> 210:          0         -  10 Edge      mxs-lradc-touchscreen

I see lradc_touch_irq in the MX28RM is IRQ 10 and it is also 10 in your
/proc/interrupts output . What am I missing ?

Ksenija Stanojevic Aug. 3, 2016, 7:45 p.m. UTC | #15

On Wed, Aug 3, 2016 at 9:39 PM, Marek Vasut <marex@denx.de> wrote:
> On 08/03/2016 09:34 PM, Ksenija Stanojević wrote:
>> On Wed, Aug 3, 2016 at 9:13 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
>>>> Hi Fabio,
>>>
>>> Hi,
>>>
>>>> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>>>> Hi Ksenija,
>>>>>
>>>>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>>>>> <ksenija.stanojevic@gmail.com> wrote:
>>>>>
>>>>>> Can someone with imx23 board send me /proc/interrupts log, I
>>>>>> need irq numbers...
>>>>>
>>>>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>>>>> Interrupt Sources)?
>>>>
>>>> I thought it will be more reliable if I asked someone, since numbers from MX28
>>>> manual don't match with /proc/interrupts log.
>>>>
>>> What do you mean they don't match ? Please elaborate.
>>
>> irqs don't have the same values in Table 5.1 in MX28 manual and in
>> /proc/interrupts
>> output.
>>
>> For example:
>> touchscreen irq in MX28 manual have these values:
>> 10 (source number) and 0x0028 (vector number)
>>
>> in /proc/interrupts:
>> 210:          0         -  10 Edge      mxs-lradc-touchscreen
>
> I see lradc_touch_irq in the MX28RM is IRQ 10 and it is also 10 in your
> /proc/interrupts output . What am I missing ?

I was referring to the number of the first column 210, that's the number that
I need and cannot find in the Table of the manual.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marek Vasut Aug. 3, 2016, 7:57 p.m. UTC | #16

On 08/03/2016 09:45 PM, Ksenija Stanojević wrote:
> On Wed, Aug 3, 2016 at 9:39 PM, Marek Vasut <marex@denx.de> wrote:
>> On 08/03/2016 09:34 PM, Ksenija Stanojević wrote:
>>> On Wed, Aug 3, 2016 at 9:13 PM, Marek Vasut <marex@denx.de> wrote:
>>>> On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
>>>>> Hi Fabio,
>>>>
>>>> Hi,
>>>>
>>>>> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>>>>> Hi Ksenija,
>>>>>>
>>>>>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>>>>>> <ksenija.stanojevic@gmail.com> wrote:
>>>>>>
>>>>>>> Can someone with imx23 board send me /proc/interrupts log, I
>>>>>>> need irq numbers...
>>>>>>
>>>>>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>>>>>> Interrupt Sources)?
>>>>>
>>>>> I thought it will be more reliable if I asked someone, since numbers from MX28
>>>>> manual don't match with /proc/interrupts log.
>>>>>
>>>> What do you mean they don't match ? Please elaborate.
>>>
>>> irqs don't have the same values in Table 5.1 in MX28 manual and in
>>> /proc/interrupts
>>> output.
>>>
>>> For example:
>>> touchscreen irq in MX28 manual have these values:
>>> 10 (source number) and 0x0028 (vector number)
>>>
>>> in /proc/interrupts:
>>> 210:          0         -  10 Edge      mxs-lradc-touchscreen
>>
>> I see lradc_touch_irq in the MX28RM is IRQ 10 and it is also 10 in your
>> /proc/interrupts output . What am I missing ?
> 
> I was referring to the number of the first column 210, that's the number that
> I need and cannot find in the Table of the manual.

The first number in /proc/interrupts is the number which Linux maps to
that particular IRQ line, but that number can be anything, it does not
(need to) match the hardware in any way. What do you need it for ?

Ksenija Stanojevic Aug. 3, 2016, 8:20 p.m. UTC | #17

On Wed, Aug 3, 2016 at 9:57 PM, Marek Vasut <marex@denx.de> wrote:
> On 08/03/2016 09:45 PM, Ksenija Stanojević wrote:
>> On Wed, Aug 3, 2016 at 9:39 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 08/03/2016 09:34 PM, Ksenija Stanojević wrote:
>>>> On Wed, Aug 3, 2016 at 9:13 PM, Marek Vasut <marex@denx.de> wrote:
>>>>> On 08/03/2016 06:45 PM, Ksenija Stanojević wrote:
>>>>>> Hi Fabio,
>>>>>
>>>>> Hi,
>>>>>
>>>>>> On Wed, Aug 3, 2016 at 6:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>>>>>> Hi Ksenija,
>>>>>>>
>>>>>>> On Wed, Aug 3, 2016 at 1:32 PM, Ksenija Stanojević
>>>>>>> <ksenija.stanojevic@gmail.com> wrote:
>>>>>>>
>>>>>>>> Can someone with imx23 board send me /proc/interrupts log, I
>>>>>>>> need irq numbers...
>>>>>>>
>>>>>>> Can't you get them from the MX23 Reference Manual (Table 5-1. i.MX23
>>>>>>> Interrupt Sources)?
>>>>>>
>>>>>> I thought it will be more reliable if I asked someone, since numbers from MX28
>>>>>> manual don't match with /proc/interrupts log.
>>>>>>
>>>>> What do you mean they don't match ? Please elaborate.
>>>>
>>>> irqs don't have the same values in Table 5.1 in MX28 manual and in
>>>> /proc/interrupts
>>>> output.
>>>>
>>>> For example:
>>>> touchscreen irq in MX28 manual have these values:
>>>> 10 (source number) and 0x0028 (vector number)
>>>>
>>>> in /proc/interrupts:
>>>> 210:          0         -  10 Edge      mxs-lradc-touchscreen
>>>
>>> I see lradc_touch_irq in the MX28RM is IRQ 10 and it is also 10 in your
>>> /proc/interrupts output . What am I missing ?
>>
>> I was referring to the number of the first column 210, that's the number that
>> I need and cannot find in the Table of the manual.
>
> The first number in /proc/interrupts is the number which Linux maps to
> that particular IRQ line, but that number can be anything, it does not
> (need to) match the hardware in any way. What do you need it for ?

Thanks, I didn't know that. Well I was using the first number to map
irq names to irq number in DEFINE_RES_IRQ_NAMED.
Otherwise I get probe failure:
probe of mxs-lradc-ts failed with error -22

It's because platform_get_irq_byname cannot find irq for the given
irq name.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lee Jones Aug. 4, 2016, 8:31 a.m. UTC | #18

On Thu, 14 Jul 2016, Marek Vasut wrote:

> On 07/13/2016 02:49 PM, Lee Jones wrote:
> > On Fri, 01 Jul 2016, Harald Geyer wrote:
> > 
> > > Hi Ksenija!
> > > 
> > > Ksenija Stanojević writes:
> > > > On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > +static int mxs_lradc_add_device(struct platform_device *pdev,
> > > > > > +                             struct mxs_lradc *lradc, char *name, int i)
> > > > > > +{
> > > > > > +     struct mfd_cell *cell;
> > > > > > +
> > > > > > +     cell = &lradc->cells[i];
> > > > > > +     cell->name = name;
> > > > > > +     cell->platform_data = lradc;
> > > > > > +     cell->pdata_size = sizeof(*lradc);
> > > > > > +
> > > > > > +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);
> > > > > > +}
> > > > > 
> > > > > Please don't roll your own API.
> > > > > 
> > > > > Use 'struct mfd_cell' like everyone else does.
> > > > 
> > > > It has been suggested in previous reviews to use separate function to
> > > > register mfd device, and to make mfd_cell allocate dynamically because
> > > > struc mxs-lradc is allocated dynamically.
> > > > But I can revrse changes and make mfd_cells allocate staticaly
> > > > wthout separate function.
> > > 
> > > I think making mfd_cells members of struct mxs-lradc will address all
> > > review comments.
> > 
> > No, please don't do that either.
> > 
> It'd be nice if you explained in detail why not. Otherwise this is just
> empty splat.

I already said what needs to be done.

"Use struct mfd_cell", in it's pure/static form.  Don't include it in
any device data struct.

"like everyone else does", look at other driver to see how they do
it.

[v3,1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD

Commit Message

Comments

Patch