Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752425AbcLFNEb (ORCPT ); Tue, 6 Dec 2016 08:04:31 -0500 Received: from mail-wm0-f51.google.com ([74.125.82.51]:37677 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751610AbcLFNEZ (ORCPT ); Tue, 6 Dec 2016 08:04:25 -0500 Date: Tue, 6 Dec 2016 13:07:12 +0000 From: Lee Jones To: Benjamin Gaignard Cc: robh+dt@kernel.org, mark.rutland@arm.com, alexandre.torgue@st.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, thierry.reding@gmail.com, linux-pwm@vger.kernel.org, jic23@kernel.org, knaack.h@gmx.de, lars@metafoo.de, pmeerw@pmeerw.net, linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, fabrice.gasnier@st.com, gerald.baeza@st.com, arnaud.pouliquen@st.com, linus.walleij@linaro.org, linaro-kernel@lists.linaro.org, Benjamin Gaignard Subject: Re: [PATCH v4 2/7] MFD: add STM32 General Purpose Timer driver Message-ID: <20161206130712.GI25385@dell.home> References: <1481027929-13704-1-git-send-email-benjamin.gaignard@st.com> <1481027929-13704-3-git-send-email-benjamin.gaignard@st.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1481027929-13704-3-git-send-email-benjamin.gaignard@st.com> User-Agent: Mutt/1.6.2 (2016-07-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8333 Lines: 249 This is really starting to come together. Couple of nits to tend to and you'll be all set. > This hardware block could at used at same time for PWM generation > and IIO timers. > PWM and IIO timer configuration are mixed in the same registers > so we need a multi fonction driver to be able to share those registers. > > version 4: > - add a function to detect Auto Reload Register (ARR) size > - rename the structure shared with other drivers > > version 2: > - rename driver "stm32-gptimer" to be align with SoC documentation > - only keep one compatible > - use of_platform_populate() instead of devm_mfd_add_devices() > > Signed-off-by: Benjamin Gaignard > --- > drivers/mfd/Kconfig | 11 ++++++ > drivers/mfd/Makefile | 2 + > drivers/mfd/stm32-gptimer.c | 82 +++++++++++++++++++++++++++++++++++++++ This is not a timer. > include/linux/mfd/stm32-gptimer.h | 63 ++++++++++++++++++++++++++++++ > 4 files changed, 158 insertions(+) > create mode 100644 drivers/mfd/stm32-gptimer.c > create mode 100644 include/linux/mfd/stm32-gptimer.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index c6df644..a00f6b3 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1607,6 +1607,17 @@ config MFD_STW481X > in various ST Microelectronics and ST-Ericsson embedded > Nomadik series. > > +config MFD_STM32_GP_TIMER > + tristate "Support for STM32 General Purpose Timer" ... but it's not though is it. This is the parent device whose children are a PWM and a Timer. > + select MFD_CORE > + select REGMAP > + depends on ARCH_STM32 || COMPILE_TEST > + depends on OF Shouldn't this be: depends on (ARCH_STM32 && OF) || COMPILE_TEST > + help > + Select this option to enable STM32 general purpose timer > + driver used for PWM and IIO Timer. This driver allow to > + share the registers between the others drivers. > + > menu "Multimedia Capabilities Port drivers" > depends on ARCH_SA1100 > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 9834e66..86353b9 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -211,3 +211,5 @@ obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o > obj-$(CONFIG_MFD_MT6397) += mt6397-core.o > > obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o > + > +obj-$(CONFIG_MFD_STM32_GP_TIMER) += stm32-gptimer.o > diff --git a/drivers/mfd/stm32-gptimer.c b/drivers/mfd/stm32-gptimer.c > new file mode 100644 > index 0000000..6747fcb > --- /dev/null > +++ b/drivers/mfd/stm32-gptimer.c > @@ -0,0 +1,82 @@ > +/* > + * Copyright (C) STMicroelectronics 2016 > + * > + * Author: Benjamin Gaignard Nit: '\n' here. > + * License terms: GNU General Public License (GPL), version 2 > + */ > + > +#include > +#include > +#include > + > +#include Nit: Why does this need to be separate? > +static const struct regmap_config stm32_gptimer_regmap_cfg = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = sizeof(u32), > + .max_register = 0x400, > +}; > + > +static u32 stm32_gptimer_get_arr_size(struct regmap *regmap) > +{ > + u32 max_arr; > + > + /* Only the available bits will be written so when readback > + * we get the maximum value of auto reload register > + */ Incorrect format for a multi-line comment in Linux. > + regmap_write(regmap, TIM_ARR, ~0L); > + regmap_read(regmap, TIM_ARR, &max_arr); > + regmap_write(regmap, TIM_ARR, 0x0); > + > + return max_arr; > +} > + > +static int stm32_gptimer_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct stm32_gptimer *ddata; > + struct resource *res; > + void __iomem *mmio; > + > + ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL); > + if (!ddata) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + mmio = devm_ioremap_resource(dev, res); > + if (IS_ERR(mmio)) > + return PTR_ERR(mmio); > + > + ddata->regmap = devm_regmap_init_mmio_clk(dev, "clk_int", mmio, > + &stm32_gptimer_regmap_cfg); > + if (IS_ERR(ddata->regmap)) > + return PTR_ERR(ddata->regmap); > + > + ddata->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(ddata->clk)) > + return PTR_ERR(ddata->clk); > + > + ddata->max_arr = stm32_gptimer_get_arr_size(ddata->regmap); Why don't you pass in ddata, then use regmap and populate max_arr inside the function. Then make stm32_gptimer_get_arr_size() void. > + platform_set_drvdata(pdev, ddata); > + > + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > +} > + > +static const struct of_device_id stm32_gptimer_of_match[] = { > + { .compatible = "st,stm32-gptimer" }, > +}; > +MODULE_DEVICE_TABLE(of, stm32_gptimer_of_match); > + > +static struct platform_driver stm32_gptimer_driver = { > + .probe = stm32_gptimer_probe, > + .driver = { > + .name = "stm32-gptimer", > + .of_match_table = stm32_gptimer_of_match, > + }, > +}; > +module_platform_driver(stm32_gptimer_driver); > + > +MODULE_DESCRIPTION("STMicroelectronics STM32 General Purpose Timer"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/mfd/stm32-gptimer.h b/include/linux/mfd/stm32-gptimer.h > new file mode 100644 > index 0000000..bf0a595 > --- /dev/null > +++ b/include/linux/mfd/stm32-gptimer.h > @@ -0,0 +1,63 @@ > +/* > + * Copyright (C) STMicroelectronics 2016 > + * > + * Author: Benjamin Gaignard Nit: '\n' > + * License terms: GNU General Public License (GPL), version 2 > + */ > + > +#ifndef _LINUX_STM32_GPTIMER_H_ > +#define _LINUX_STM32_GPTIMER_H_ > + > +#include > +#include > + > +#define TIM_CR1 0x00 /* Control Register 1 */ > +#define TIM_CR2 0x04 /* Control Register 2 */ > +#define TIM_SMCR 0x08 /* Slave mode control reg */ > +#define TIM_DIER 0x0C /* DMA/interrupt register */ > +#define TIM_SR 0x10 /* Status register */ > +#define TIM_EGR 0x14 /* Event Generation Reg */ > +#define TIM_CCMR1 0x18 /* Capt/Comp 1 Mode Reg */ > +#define TIM_CCMR2 0x1C /* Capt/Comp 2 Mode Reg */ > +#define TIM_CCER 0x20 /* Capt/Comp Enable Reg */ > +#define TIM_PSC 0x28 /* Prescaler */ > +#define TIM_ARR 0x2c /* Auto-Reload Register */ > +#define TIM_CCR1 0x34 /* Capt/Comp Register 1 */ > +#define TIM_CCR2 0x38 /* Capt/Comp Register 2 */ > +#define TIM_CCR3 0x3C /* Capt/Comp Register 3 */ > +#define TIM_CCR4 0x40 /* Capt/Comp Register 4 */ > +#define TIM_BDTR 0x44 /* Break and Dead-Time Reg */ > + > +#define TIM_CR1_CEN BIT(0) /* Counter Enable */ > +#define TIM_CR1_ARPE BIT(7) /* Auto-reload Preload Ena */ > +#define TIM_CR2_MMS (BIT(4) | BIT(5) | BIT(6)) /* Master mode selection */ > +#define TIM_SMCR_SMS (BIT(0) | BIT(1) | BIT(2)) /* Slave mode selection */ > +#define TIM_SMCR_TS (BIT(4) | BIT(5) | BIT(6)) /* Trigger selection */ > +#define TIM_DIER_UIE BIT(0) /* Update interrupt */ > +#define TIM_SR_UIF BIT(0) /* Update interrupt flag */ > +#define TIM_EGR_UG BIT(0) /* Update Generation */ > +#define TIM_CCMR_PE BIT(3) /* Channel Preload Enable */ > +#define TIM_CCMR_M1 (BIT(6) | BIT(5)) /* Channel PWM Mode 1 */ > +#define TIM_CCER_CC1E BIT(0) /* Capt/Comp 1 out Ena */ > +#define TIM_CCER_CC1P BIT(1) /* Capt/Comp 1 Polarity */ > +#define TIM_CCER_CC1NE BIT(2) /* Capt/Comp 1N out Ena */ > +#define TIM_CCER_CC1NP BIT(3) /* Capt/Comp 1N Polarity */ > +#define TIM_CCER_CC2E BIT(4) /* Capt/Comp 2 out Ena */ > +#define TIM_CCER_CC3E BIT(8) /* Capt/Comp 3 out Ena */ > +#define TIM_CCER_CC4E BIT(12) /* Capt/Comp 4 out Ena */ > +#define TIM_CCER_CCXE (BIT(0) | BIT(4) | BIT(8) | BIT(12)) > +#define TIM_BDTR_BKE BIT(12) /* Break input enable */ > +#define TIM_BDTR_BKP BIT(13) /* Break input polarity */ > +#define TIM_BDTR_AOE BIT(14) /* Automatic Output Enable */ > +#define TIM_BDTR_MOE BIT(15) /* Main Output Enable */ > + > +#define MAX_TIM_PSC 0xFFFF > +#define TIM_CR2_MMS_SHIFT 4 > +#define TIM_SMCR_TS_SHIFT 4 > + > +struct stm32_gptimer { > + struct clk *clk; > + struct regmap *regmap; > + u32 max_arr; > +}; > +#endif -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog