Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751130AbdCNLFy (ORCPT ); Tue, 14 Mar 2017 07:05:54 -0400 Received: from mail-wm0-f47.google.com ([74.125.82.47]:33603 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751105AbdCNLFu (ORCPT ); Tue, 14 Mar 2017 07:05:50 -0400 Date: Tue, 14 Mar 2017 11:05:45 +0000 From: Lee Jones To: Dmitry Torokhov Cc: Stefan Wahren , Ksenija Stanojevic , linux-input@vger.kernel.org, pmeerw@pmeerw.net, jic23@kernel.org, knaack.h@gmx.de, lars@metafoo.de, harald@ccbib.org, linux-kernel@vger.kernel.org, fabio.estevam@freescale.com, linux-iio@vger.kernel.org, marex@denx.de Subject: Re: [PATCH v14 1/5] mfd: mxs-lradc: Add support for mxs-lradc MFD Message-ID: <20170314110545.54jz53xx36vsc3bo@dell> References: <282870591.38324.1488399050890@email.1und1.de> <20170301204855.GG30349@dtor-ws> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170301204855.GG30349@dtor-ws> User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14280 Lines: 399 On Wed, 01 Mar 2017, Dmitry Torokhov wrote: > On Wed, Mar 01, 2017 at 09:10:50PM +0100, Stefan Wahren wrote: > > > Ksenija Stanojevic hat am 1. März 2017 um 19:02 geschrieben: > > > > > > > > > Add core files for low resolution analog-to-digital converter (mxs-lradc) > > > MFD driver. > > > > > > Signed-off-by: Ksenija Stanojevic > > > Acked-by: Lee Jones > > > --- > > > Changes in v14: > > > - none > > > > > > Changes in v13: > > > - none > > > > > > Changes in v12: > > > - use BIT macro > > > > > > Changes in v11: > > > - create static struct mfd_cells > > > - don't set platform data in mfd cells, set driver data instead > > > - remove mxs_lradc_reg_* functions, use writel function instead > > > > > > Changes in v10: > > > - fetch base address from DT > > > - add a NULL check for of_match_device > > > > > > Changes in v9: > > > - improve commit message. > > > > > > Changes in v8: > > > - rebase onto 4.9-rc1 > > > > > > Changes in v7: > > > - define macros ADC_CELL and TSC_CELL > > > - remove one cell and dynamically set them in the switch() > > > - fail in the touchscreen driver instead of mfd driver if > > > hardware doesn't contain a touchscreen > > > > > > Changes in v6: > > > - update copyright > > > - add kernel-doc header for struct mxs-lradc > > > - add error message > > > - change EINVAL to ENODEV > > > - use PLATFORM_DEVID_NONE instead -1 > > > - cosmetic fixes > > > > > > Changes in v5: > > > - use DEFINE_RES_MEM > > > - don't pass ioreammaped adress to platform cells > > > - move comment outside of struct mxs_lradc > > > - change type of argument in mxs_lradc_reg_set, mxs_lradc_reg_clear, > > > mxs_lradc_reg_wrt (struct mxs_lradc * -> void __iomem *) > > > > > > Changes in v4: > > > - update copyright > > > - use DEFINE_RES_IRQ_NAMED > > > - remove mxs_lradc_add_device function > > > - use struct mfd_cell in static form > > > - improve spacing > > > - remove unnecessary comment > > > - remove platform_get_irq > > > - remove touch_ret and use ret instead > > > - rename use_touchscreen to touchscreen_wire > > > - use goto statements > > > - remove irq[13], irq_count and irq_name from struct mxs_lradc > > > - remove all defines from inside the struct definition > > > > > > 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 | 264 ++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/mfd/mxs-lradc.h | 187 ++++++++++++++++++++++++++++++ > > > 4 files changed, 469 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 55ecdfb..8bbc91b 100644 > > > --- a/drivers/mfd/Kconfig > > > +++ b/drivers/mfd/Kconfig > > > @@ -344,6 +344,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 > > > + 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_MX25_TSADC > > > tristate "Freescale i.MX25 integrated Touchscreen and ADC unit" > > > select REGMAP_MMIO > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > > index 31ce076..790698a 100644 > > > --- a/drivers/mfd/Makefile > > > +++ b/drivers/mfd/Makefile > > > @@ -215,3 +215,4 @@ obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o > > > obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o > > > > > > obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.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..2ae8487 > > > --- /dev/null > > > +++ b/drivers/mfd/mxs-lradc.c > > > @@ -0,0 +1,264 @@ > > > +/* > > > + * Freescale MXS Low Resolution Analog-to-Digital Converter driver > > > + * > > > + * Copyright (c) 2012 DENX Software Engineering, GmbH. > > > + * Copyright (c) 2017 Ksenija Stanojevic > > > + * > > > + * Authors: > > > + * Marek Vasut > > > + * Ksenija Stanojevic > > > + * > > > + * 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 > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#define ADC_CELL 0 > > > +#define TSC_CELL 1 > > > +#define RES_MEM 0 > > > + > > > +enum mx23_lradc_irqs { > > > + MX23_LRADC_TS_IRQ = 0, > > > + MX23_LRADC_CH0_IRQ, > > > + MX23_LRADC_CH1_IRQ, > > > + MX23_LRADC_CH2_IRQ, > > > + MX23_LRADC_CH3_IRQ, > > > + MX23_LRADC_CH4_IRQ, > > > + MX23_LRADC_CH5_IRQ, > > > + MX23_LRADC_CH6_IRQ, > > > + MX23_LRADC_CH7_IRQ, > > > +}; > > > + > > > +enum mx28_lradc_irqs { > > > + MX28_LRADC_TS_IRQ = 0, > > > + MX28_LRADC_TRESH0_IRQ, > > > + MX28_LRADC_TRESH1_IRQ, > > > + MX28_LRADC_CH0_IRQ, > > > + MX28_LRADC_CH1_IRQ, > > > + MX28_LRADC_CH2_IRQ, > > > + MX28_LRADC_CH3_IRQ, > > > + MX28_LRADC_CH4_IRQ, > > > + MX28_LRADC_CH5_IRQ, > > > + MX28_LRADC_CH6_IRQ, > > > + MX28_LRADC_CH7_IRQ, > > > + MX28_LRADC_BUTTON0_IRQ, > > > + MX28_LRADC_BUTTON1_IRQ, > > > +}; > > > + > > > +static struct resource mx23_adc_resources[] = { > > > + DEFINE_RES_MEM(0x0, 0x0), > > > + DEFINE_RES_IRQ_NAMED(MX23_LRADC_CH0_IRQ, "mxs-lradc-channel0"), > > > + DEFINE_RES_IRQ_NAMED(MX23_LRADC_CH1_IRQ, "mxs-lradc-channel1"), > > > + DEFINE_RES_IRQ_NAMED(MX23_LRADC_CH2_IRQ, "mxs-lradc-channel2"), > > > + DEFINE_RES_IRQ_NAMED(MX23_LRADC_CH3_IRQ, "mxs-lradc-channel3"), > > > + DEFINE_RES_IRQ_NAMED(MX23_LRADC_CH4_IRQ, "mxs-lradc-channel4"), > > > + DEFINE_RES_IRQ_NAMED(MX23_LRADC_CH5_IRQ, "mxs-lradc-channel5"), > > > +}; > > > + > > > +static struct resource mx23_touchscreen_resources[] = { > > > + DEFINE_RES_MEM(0x0, 0x0), > > > + DEFINE_RES_IRQ_NAMED(MX23_LRADC_TS_IRQ, "mxs-lradc-touchscreen"), > > > + DEFINE_RES_IRQ_NAMED(MX23_LRADC_CH6_IRQ, "mxs-lradc-channel6"), > > > + DEFINE_RES_IRQ_NAMED(MX23_LRADC_CH7_IRQ, "mxs-lradc-channel7"), > > > +}; > > > + > > > +static struct resource mx28_adc_resources[] = { > > > + DEFINE_RES_MEM(0x0, 0x0), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_TRESH0_IRQ, "mxs-lradc-thresh0"), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_TRESH1_IRQ, "mxs-lradc-thresh1"), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_CH0_IRQ, "mxs-lradc-channel0"), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_CH1_IRQ, "mxs-lradc-channel1"), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_CH2_IRQ, "mxs-lradc-channel2"), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_CH3_IRQ, "mxs-lradc-channel3"), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_CH4_IRQ, "mxs-lradc-channel4"), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_CH5_IRQ, "mxs-lradc-channel5"), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_BUTTON0_IRQ, "mxs-lradc-button0"), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_BUTTON1_IRQ, "mxs-lradc-button1"), > > > +}; > > > + > > > +static struct resource mx28_touchscreen_resources[] = { > > > + DEFINE_RES_MEM(0x0, 0x0), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_TS_IRQ, "mxs-lradc-touchscreen"), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_CH6_IRQ, "mxs-lradc-channel6"), > > > + DEFINE_RES_IRQ_NAMED(MX28_LRADC_CH7_IRQ, "mxs-lradc-channel7"), > > > +}; > > > + > > > +static struct mfd_cell mx23_cells[] = { > > > + { > > > + .name = "mxs-lradc-adc", > > > + .resources = mx23_adc_resources, > > > + .num_resources = ARRAY_SIZE(mx23_adc_resources), > > > + }, > > > + { > > > + .name = "mxs-lradc-ts", > > > + .resources = mx23_touchscreen_resources, > > > + .num_resources = ARRAY_SIZE(mx23_touchscreen_resources), > > > + }, > > > +}; > > > + > > > +static struct mfd_cell mx28_cells[] = { > > > + { > > > + .name = "mxs-lradc-adc", > > > + .resources = mx28_adc_resources, > > > + .num_resources = ARRAY_SIZE(mx28_adc_resources), > > > + }, > > > + { > > > + .name = "mxs-lradc-ts", > > > + .resources = mx28_touchscreen_resources, > > > + .num_resources = ARRAY_SIZE(mx28_touchscreen_resources), > > > + } > > > +}; > > > + > > > +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; > > > + struct device *dev = &pdev->dev; > > > + struct device_node *node = dev->of_node; > > > + struct mxs_lradc *lradc; > > > + struct mfd_cell *cells = NULL; > > > + struct resource *res; > > > + int ret = 0; > > > + u32 ts_wires = 0; > > > + > > > + lradc = devm_kzalloc(&pdev->dev, sizeof(*lradc), GFP_KERNEL); > > > + if (!lradc) > > > + return -ENOMEM; > > > + > > > + of_id = of_match_device(mxs_lradc_dt_ids, &pdev->dev); > > > + if (!of_id) > > > + return -EINVAL; > > > + > > > + lradc->soc = (enum mxs_lradc_id)of_id->data; > > > + > > > + 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); > > > + } > > > + > > > + ret = clk_prepare_enable(lradc->clk); > > > + if (ret) { > > > + dev_err(dev, "Failed to enable the delay unit clock\n"); > > > + return ret; > > > + } > > > + > > > + ret = of_property_read_u32(node, "fsl,lradc-touchscreen-wires", > > > + &ts_wires); > > > + > > > + if (!ret) { > > > + lradc->buffer_vchans = BUFFER_VCHANS_LIMITED; > > > + > > > + switch (ts_wires) { > > > + case 4: > > > + lradc->touchscreen_wire = MXS_LRADC_TOUCHSCREEN_4WIRE; > > > + break; > > > + case 5: > > > + if (lradc->soc == IMX28_LRADC) { > > > + lradc->touchscreen_wire = > > > + MXS_LRADC_TOUCHSCREEN_5WIRE; > > > + break; > > > + } > > > + /* fall through to an error message for i.MX23 */ > > > + default: > > > + dev_err(&pdev->dev, > > > + "Unsupported number of touchscreen wires (%d)\n" > > > + , ts_wires); > > > + ret = -EINVAL; > > > + goto err_clk; > > > + } > > > + } else { > > > + lradc->buffer_vchans = BUFFER_VCHANS_ALL; > > > + } > > > + > > > + platform_set_drvdata(pdev, lradc); > > > + > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + if (!res) > > > + return -ENOMEM; > > > + > > > + switch (lradc->soc) { > > > + case IMX23_LRADC: > > > + mx23_adc_resources[RES_MEM] = *res; > > > + mx23_touchscreen_resources[RES_MEM] = *res; > > > + cells = mx23_cells; > > > + break; > > > + case IMX28_LRADC: > > > + mx28_adc_resources[RES_MEM] = *res; > > > + mx28_touchscreen_resources[RES_MEM] = *res; > > > + cells = mx28_cells; > > > + break; > > > + default: > > > + dev_err(dev, "Unsupported SoC\n"); > > > + ret = -ENODEV; > > > + goto err_clk; > > > + } > > > + > > > + ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, > > > + &cells[ADC_CELL], 1, NULL, 0, NULL); > > > + if (ret) { > > > + dev_err(&pdev->dev, "Failed to add the ADC subdevice\n"); > > > + goto err_clk; > > > + } > > > + > > > + ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, > > > + &cells[TSC_CELL], 1, NULL, 0, NULL); > > > + if (ret) { > > > + dev_err(&pdev->dev, > > > + "Failed to add the touchscreen subdevice\n"); > > > + goto err_clk; > > > + } > > > > Since touchscreen is optional it doesn't make to try to register it, notice in the touchscreen driver there is no touchscreen, give the user a confusing error and finally disable the clock. > > > > Here are my suggestions: > > > > 1) Use the solution from Patch V6 but with an explaining comment that touchscreen is optional > > > > if (!lradc->touchscreen_wire) > > return 0; > > > > 2) Return a special error from touchscreen registration so mfd knowns this result is expected. > > > > I'm open for better solutions. > > > > @Lee: What's your opinion? > > I am not Lee, but given that this module already parses touchscreen > properties and knows whether there is touchscreen or not, #1 seems > simple and logical. Sorry for the delay, I'm currently behind on my mail due to vacation and conferences. +1 -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog