From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E6E34C433F5 for ; Thu, 23 Sep 2021 08:34:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C719C6105A for ; Thu, 23 Sep 2021 08:34:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239945AbhIWIfa convert rfc822-to-8bit (ORCPT ); Thu, 23 Sep 2021 04:35:30 -0400 Received: from relay10.mail.gandi.net ([217.70.178.230]:41425 "EHLO relay10.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239922AbhIWIfa (ORCPT ); Thu, 23 Sep 2021 04:35:30 -0400 Received: (Authenticated sender: miquel.raynal@bootlin.com) by relay10.mail.gandi.net (Postfix) with ESMTPSA id C7910240010; Thu, 23 Sep 2021 08:33:54 +0000 (UTC) Date: Thu, 23 Sep 2021 10:33:52 +0200 From: Miquel Raynal To: Lee Jones Cc: Jonathan Cameron , Lars-Peter Clausen , Peter Meerwald-Stadler , Rob Herring , Dmitry Torokhov , bcousson@baylibre.com, Tony Lindgren , linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-omap@vger.kernel.org, Thomas Petazzoni , Vignesh Raghavendra , Lokesh Vutla , Tero Kristo , Ryan Barnett , Grygorii Strashko , Jason Reeder , Jonathan Cameron Subject: Re: [PATCH v3 35/47] mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support Message-ID: <20210923103352.40e422a7@xps13> In-Reply-To: References: <20210915155908.476767-1-miquel.raynal@bootlin.com> <20210915155908.476767-36-miquel.raynal@bootlin.com> Organization: Bootlin X-Mailer: Claws Mail 3.17.7 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org Hi Lee, lee.jones@linaro.org wrote on Wed, 22 Sep 2021 17:00:20 +0100: > On Wed, 15 Sep 2021, Miquel Raynal wrote: > > > Introduce a new compatible that has another set of driver data, > > targeting am437x SoCs with a magnetic reader instead of the > > touchscreen and a more featureful set of registers. > > > > Signed-off-by: Miquel Raynal > > Reviewed-by: Jonathan Cameron > > --- > > drivers/mfd/ti_am335x_tscadc.c | 41 ++++++++++++++++++++++++---- > > include/linux/mfd/ti_am335x_tscadc.h | 7 +++++ > > 2 files changed, 42 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c > > index 65c0fa735a71..155a8ed879b3 100644 > > --- a/drivers/mfd/ti_am335x_tscadc.c > > +++ b/drivers/mfd/ti_am335x_tscadc.c > > @@ -121,11 +121,11 @@ static int ti_tscadc_probe(struct platform_device *pdev) > > struct mfd_cell *cell; > > struct property *prop; > > const __be32 *cur; > > - bool use_tsc = false; > > + bool use_tsc = false, use_mag = false; > > u32 val; > > int err; > > int tscmag_wires = 0, adc_channels = 0, cell_idx = 0, total_channels; > > - int readouts = 0; > > + int readouts = 0, mag_tracks = 0; > > > > /* Allocate memory for device */ > > tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL); > > @@ -148,6 +148,16 @@ static int ti_tscadc_probe(struct platform_device *pdev) > > of_node_put(node); > > if (tscmag_wires) > > use_tsc = true; > > + } else { > > + /* > > + * When adding support for the magnetic reader, here is the > > + * place to look for the number of tracks used from device tree. > > + * Let's default to 0 for now. > > + */ > > + mag_tracks = 0; > > + tscmag_wires = mag_tracks * 2; > > + if (tscmag_wires) > > + use_mag = true; > > } > > > > node = of_get_child_by_name(pdev->dev.of_node, "adc"); > > @@ -209,8 +219,9 @@ static int ti_tscadc_probe(struct platform_device *pdev) > > * The TSC_ADC_Subsystem has 2 clock domains: OCP_CLK and ADC_CLK. > > * ADCs produce a 12-bit sample every 15 ADC_CLK cycles. > > * am33xx ADCs expect to capture 200ksps. > > - * We need the ADC clocks to run at 3MHz. > > - * This frequency is valid since TSC_ADC_SS controller design > > + * am47xx ADCs expect to capture 867ksps. > > + * We need ADC clocks respectively running at 3MHz and 13MHz. > > + * These frequencies are valid since TSC_ADC_SS controller design > > * assumes the OCP clock is at least 6x faster than the ADC clock. > > */ > > clk = devm_clk_get(&pdev->dev, NULL); > > @@ -238,6 +249,9 @@ static int ti_tscadc_probe(struct platform_device *pdev) > > else > > tscadc->ctrl |= CNTRLREG_TSC_4WIRE; > > } > > + } else { > > + tscadc->ctrl |= CNTRLREG_MAG_PREAMP_PWRDOWN | > > + CNTRLREG_MAG_PREAMP_BYPASS; > > } > > regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl); > > > > @@ -246,7 +260,7 @@ static int ti_tscadc_probe(struct platform_device *pdev) > > /* Enable the TSC module enable bit */ > > regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_SSENB); > > > > - /* TSC Cell */ > > + /* TSC or MAG Cell */ > > if (tscmag_wires > 0) { > > cell = &tscadc->cells[cell_idx++]; > > cell->name = tscadc->data->name_tscmag; > > @@ -334,6 +348,7 @@ static SIMPLE_DEV_PM_OPS(tscadc_pm_ops, tscadc_suspend, tscadc_resume); > > > > static const struct ti_tscadc_data tscdata = { > > .has_tsc = true, > > + .has_mag = false, > > .name_tscmag = "TI-am335x-tsc", > > .compat_tscmag = "ti,am3359-tsc", > > .name_adc = "TI-am335x-adc", > > @@ -341,11 +356,25 @@ static const struct ti_tscadc_data tscdata = { > > .target_clk_rate = TSC_ADC_CLK, > > }; > > > > +static const struct ti_tscadc_data magdata = { > > + .has_tsc = false, > > + .has_mag = true, > > Why 2 bools? > > Better to have a single u8: > > MODE_TOUCHSCREEN > MODE_MAG_STRING > MODE_NONE It's not a mode like "please use this feature", it is more like a feature that is present and we need to know which one it is. Anyway, I can use an enum for this, fine. > > > + .name_tscmag = "TI-am43xx-mag", > > + .compat_tscmag = "ti,am4372-mag", > > + .name_adc = "TI-am43xx-adc", > > + .compat_adc = "ti,am4372-adc", > > + .target_clk_rate = MAG_ADC_CLK, > > +}; > > + > > static const struct of_device_id ti_tscadc_dt_ids[] = { > > { > > .compatible = "ti,am3359-tscadc", > > .data = &tscdata, > > }, > > + { > > + .compatible = "ti,am4372-magadc", > > + .data = &magdata, > > + }, > > { } > > }; > > MODULE_DEVICE_TABLE(of, ti_tscadc_dt_ids); > > @@ -363,6 +392,6 @@ static struct platform_driver ti_tscadc_driver = { > > > > module_platform_driver(ti_tscadc_driver); > > > > -MODULE_DESCRIPTION("TI touchscreen / ADC MFD controller driver"); > > +MODULE_DESCRIPTION("TI touchscreen/magnetic reader/ADC MFD controller driver"); > > Magnetic what? Tape, disk, stripe? I guess my current knowledge of English tricked me as I thought "magnetic reader" was clear enough for everybody, but I'll make the commit logs and this module description clearer by specifying the "stripe" keyword, sure. > > > MODULE_AUTHOR("Rachna Patil "); > > MODULE_LICENSE("GPL"); > > diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h > > index 467b3bec4335..9a412e6d764b 100644 > > --- a/include/linux/mfd/ti_am335x_tscadc.h > > +++ b/include/linux/mfd/ti_am335x_tscadc.h > > @@ -106,6 +106,11 @@ > > #define CNTRLREG_TSC_8WIRE CNTRLREG_TSC_AFE_CTRL(3) > > #define CNTRLREG_TSC_ENB BIT(7) > > > > +/*Control registers bitfields for MAGADC IP */ > > +#define CNTRLREG_MAGADCENB BIT(0) > > +#define CNTRLREG_MAG_PREAMP_PWRDOWN BIT(5) > > +#define CNTRLREG_MAG_PREAMP_BYPASS BIT(6) > > + > > /* FIFO READ Register */ > > #define FIFOREAD_DATA_MASK GENMASK(11, 0) > > #define FIFOREAD_CHNLID_MASK GENMASK(19, 16) > > @@ -119,6 +124,7 @@ > > #define CHARGE_STEP 0x11 > > > > #define TSC_ADC_CLK (3 * HZ_PER_MHZ) > > +#define MAG_ADC_CLK (13 * HZ_PER_MHZ) > > #define TOTAL_STEPS 16 > > #define TOTAL_CHANNELS 8 > > #define FIFO1_THRESHOLD 19 > > @@ -141,6 +147,7 @@ > > > > struct ti_tscadc_data { > > bool has_tsc; > > + bool has_mag; > > char *name_tscmag; > > char *compat_tscmag; > > char *name_adc; > Thanks, Miquèl