On Fri, 16 Jun 2017, Fabrice Gasnier wrote:

It think this should be "Low Power Timer" throughout.

Or even "Low-Power Timer".  You decide.

+	depends on (ARCH_STM32 && OF) || COMPILE_TEST
+	select MFD_CORE
+	select REGMAP
+	select REGMAP_MMIO
+	select RESET_CONTROLLER
+	help
+	  Select this option to enable STM32 low power timer driver
+	  used for PWM, IIO trigger, IIO encoder and counter. This driver

"Counter"

+	  allows to share the resources between these drivers.

"Shared resources are also dealt with here."  Or similar.

This is a separate driver.  It isn't part of anything.

"STM32 Low Power Timer parent driver".

+ * Copyright (C) STMicroelectronics 2017
+ *
+ * Author: Fabrice Gasnier [off-list ref]

'\n' here.

+ * Inspired from: stm32-timers from Benjamin Gaignard

"Inspired by Benjamin Gaignard's stm32-timers driver"

+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/mfd/stm32-lptimer.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/reset.h>
+
+static const struct regmap_config stm32_lptimer_regmap_cfg = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = sizeof(u32),
+	.max_register = 0x3fc,

This should be defined.

+};
+
+static int stm32_lptimer_detect_encoder(struct stm32_lptimer *ddata)
+{
+	u32 val, enc = STM32_LPTIM_ENC;

Why have enc?  Why not just use STM32_LPTIM_ENC?

+	int ret;
+
+	/*
+	 * Quadrature encoder mode bit can only be written and read back when
+	 * LP Timer supports is.

"it"

+	 */
+	ret = regmap_update_bits(ddata->regmap, STM32_LPTIM_CFGR, enc, enc);
+	if (ret)
+		return ret;

'\n' here.

+	ret = regmap_read(ddata->regmap, STM32_LPTIM_CFGR, &val);
+	if (ret)
+		return ret;

'\n' here.

+	ret = regmap_update_bits(ddata->regmap, STM32_LPTIM_CFGR, enc, 0);
+	if (ret)
+		return ret;

'\n' here.

+	ddata->has_encoder = !!val;
+
+	return 0;
+}
+
+static int stm32_lptimer_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct stm32_lptimer *ddata;
+	struct reset_control *rst;

This looks quite a lot like 'ret'.  Just use 'reset'.

+	struct resource *res;
+	void __iomem *mmio;
+	int ret;
+
+	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, "int", mmio,

Is that what the clock is called in the datasheet?

+						  &stm32_lptimer_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);
+
+	rst = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(rst))
+		return PTR_ERR(rst);
+
+	if (rst) {
+		reset_control_assert(rst);
+		reset_control_deassert(rst);
+	}

This is odd.  Please provide a comment as to why you're doing this.

+	ret = stm32_lptimer_detect_encoder(ddata);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, ddata);
+
+	return devm_of_platform_populate(&pdev->dev);
+}
+
+static const struct of_device_id stm32_lptimer_of_match[] = {
+	{ .compatible = "st,stm32-lptimer", },
+	{ /* end node */ },

No need for the comment.

Same as the comments I provided for the *.c file above.

+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#ifndef _LINUX_STM32_LPTIMER_H_
+#define _LINUX_STM32_LPTIMER_H_
+
+#include <linux/clk.h>
+#include <linux/regmap.h>
+
+#define STM32_LPTIM_ISR		0x00	/* Interrupt and Status Reg  */
+#define STM32_LPTIM_ICR		0x04	/* Interrupt Clear Reg       */
+#define STM32_LPTIM_IER		0x08	/* Interrupt Enable Reg      */
+#define STM32_LPTIM_CFGR	0x0C	/* Configuration Reg         */
+#define STM32_LPTIM_CR		0x10	/* Control Reg               */
+#define STM32_LPTIM_CMP		0x14	/* Compare Reg               */
+#define STM32_LPTIM_ARR		0x18	/* Autoreload Reg            */
+#define STM32_LPTIM_CNT		0x1C	/* Counter Reg               */
+
+/* STM32_LPTIM_ISR - bit fields */
+#define STM32_LPTIM_CMPOK_ARROK		GENMASK(4, 3)
+#define STM32_LPTIM_ARROK		BIT(4)
+#define STM32_LPTIM_CMPOK		BIT(3)
+
+/* STM32_LPTIM_ICR - bit fields */
+#define STM32_LPTIM_CMPOKCF_ARROKCF	GENMASK(4, 3)
+
+/* STM32_LPTIM_CR - bit fields */
+#define STM32_LPTIM_CNTSTRT	BIT(2)
+#define STM32_LPTIM_ENABLE	BIT(0)
+
+/* STM32_LPTIM_CFGR - bit fields */
+#define STM32_LPTIM_ENC		BIT(24)
+#define STM32_LPTIM_COUNTMODE	BIT(23)
+#define STM32_LPTIM_WAVPOL	BIT(21)
+#define STM32_LPTIM_PRESC	GENMASK(11, 9)
+#define STM32_LPTIM_CKPOL	GENMASK(2, 1)
+
+/* STM32_LPTIM_ARR */
+#define STM32_LPTIM_MAX_ARR	0xFFFF
+
+struct stm32_lptimer {
+	struct clk *clk;
+	struct regmap *regmap;
+	bool has_encoder;
+};

Please provide a kerneldoc type header for this struct.

+#endif

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