Re: [PATCH v3 2/7] MFD: add stm32 general purpose timer driver

[Lee Jones <lee.jones@linaro.org>]

On Fri, 02 Dec 2016, Benjamin Gaignard wrote:

> This hardware block could at used at same time for PWM generation
> and IIO timer for other IPs like DAC, ADC or other 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 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 <benjamin.gaignard@st.com>
> ---
>  drivers/mfd/Kconfig               | 10 ++++++
>  drivers/mfd/Makefile              |  2 ++
>  drivers/mfd/stm32-gptimer.c       | 73 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/stm32-gptimer.h | 62 +++++++++++++++++++++++++++++++++
>  4 files changed, 147 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..e75abcb 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1607,6 +1607,15 @@ 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"
> +	select MFD_CORE
> +	select REGMAP
> +	depends on ARCH_STM32
> +	depends on OF

"|| COMPILE_TEST"?

> +	help
> +	  Select this option to enable stm32 general purpose timer

I can see that.  Tell us more about the device and what it does.

s/stm32/STM32/

>  menu "Multimedia Capabilities Port drivers"
>  	depends on ARCH_SA1100
>  
> @@ -1644,4 +1653,5 @@ config MFD_VEXPRESS_SYSREG
>  	  on the ARM Ltd. Versatile Express board.
>  
>  endmenu
> +

Please remove this change.  It has nothing to do with the set.

>  endif
> 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..54fb95c
> --- /dev/null
> +++ b/drivers/mfd/stm32-gptimer.c
> @@ -0,0 +1,73 @@
> +/*
> + * stm32-gptimer.c

Swap this out for a description.

> + * Copyright (C) STMicroelectronics 2016

'\n'

> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/reset.h>
> +
> +#include <linux/mfd/stm32-gptimer.h>
> +
> +static const struct regmap_config stm32_gptimer_regmap_cfg = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = sizeof(u32),
> +	.max_register = 0x400,
> +	.fast_io = true,
> +};
> +
> +static int stm32_gptimer_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct stm32_gptimer_dev *mfd;

s/mfd/ddata/

> +	struct resource *res;
> +	void __iomem *mmio;
> +
> +	mfd = devm_kzalloc(dev, sizeof(*mfd), GFP_KERNEL);
> +	if (!mfd)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENOMEM;
> +

Remove this 3 lines.

devm_ioremap_resource() does the checking and error printing for you.

> +	mmio = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(mmio))
> +		return PTR_ERR(mmio);
> +
> +	mfd->regmap = devm_regmap_init_mmio_clk(dev, "clk_int", mmio,
> +						&stm32_gptimer_regmap_cfg);
> +	if (IS_ERR(mfd->regmap))
> +		return PTR_ERR(mfd->regmap);
> +
> +	mfd->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(mfd->clk))
> +		return PTR_ERR(mfd->clk);
> +
> +	platform_set_drvdata(pdev, mfd);
> +
> +	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",
> +	},

One line:

    { .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,
> +	},

Remove tabs before the '='s.

> +module_platform_driver(stm32_gptimer_driver);
> +
> +MODULE_DESCRIPTION("STMicroelectronics STM32 general purpose timer");

"General Purpose Timer"

> +MODULE_LICENSE("GPL");


"GPL v2"
> diff --git a/include/linux/mfd/stm32-gptimer.h b/include/linux/mfd/stm32-gptimer.h
> new file mode 100644
> index 0000000..f8c92de
> --- /dev/null
> +++ b/include/linux/mfd/stm32-gptimer.h
> @@ -0,0 +1,62 @@
> +/*
> + * stm32-gptimer.h
> + *
> + * Copyright (C) STMicroelectronics 2016
> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
> + * License terms:  GNU General Public License (GPL), version 2
> + */

Same comments as before.

> +#ifndef _LINUX_STM32_GPTIMER_H_
> +#define _LINUX_STM32_GPTIMER_H_
> +
> +#include <linux/clk.h>
> +#include <linux/regmap.h>
> +
> +#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_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
> +
> +struct stm32_gptimer_dev {

Drop the "_dev" or replace with ddata.

> +	/* Device data */

No need for this.
> +	struct clk *clk;
> +
> +	/* Registers mapping */

No need for this.

> +	struct regmap *regmap;
> +};
> +
> +#endif

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog