[PATCH v2] pwm: add pwm driver for HiSilicon BVT SOCs

[PATCH v2] pwm: add pwm driver for HiSilicon BVT SOCs

[Jian Yuan]

From: yuanjian <yuanjian12@hisilicon.com>

Add pwm driver for HiSilicon BVT SOCs

Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
Signed-off-by: Jian Yuan <yuanjian12@hisilicon.com>
---
Change Log:
v2:
The number of PWMs is change to be probeable based on the compatible string.

 .../devicetree/bindings/pwm/pwm-hibvt.txt          |  18 ++
 drivers/pwm/Kconfig                                |  10 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-hibvt.c                            | 274 +++++++++++++++++++++
 4 files changed, 303 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
 create mode 100644 drivers/pwm/pwm-hibvt.c

diff --git a/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
new file mode 100644
index 0000000..1274119
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
@@ -0,0 +1,18 @@
+Hisilicon PWM controller
+
+Required properties:
+-compatible: should contain one soc specific compatible string and one generic compatible
+string "hisilicon, hibvt-pwm". The soc specific strings supported including:
+	"hisilicon,hi3516cv300-pwm"
+- reg: physical base address and length of the controller's registers.
+- clocks: phandle and clock specifier of the PWM reference clock.
+- resets: offset address and offset bit for reset or unreset of the controller.
+
+Example:
+	pwm: pwm@12130000 {
+
+		compatible = "hisilicon,hi3516cv300-pwm", "hisilicon,hibvt-pwm";
+		reg = <0x12130000 0x10000>;
+		clocks = <&crg_ctrl HI3516CV300_PWM_CLK>;
+		resets = <&crg_ctrl 0x38 0>;
+	};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index c182efc..3c48768 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -158,6 +158,15 @@ config PWM_FSL_FTM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-fsl-ftm.
 
+config PWM_HIBVT
+	tristate "HiSilicon BVT PWM support"
+	depends on ARCH_HISI
+	help
+	  Generic PWM framework driver for hisilicon BVT SOCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-hibvt.
+
 config PWM_IMG
 	tristate "Imagination Technologies PWM driver"
 	depends on HAS_IOMEM
@@ -438,4 +447,5 @@ config PWM_VT8500
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-vt8500.
 
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index dd35bc1..37ec39e 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
+obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
 obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
diff --git a/drivers/pwm/pwm-hibvt.c b/drivers/pwm/pwm-hibvt.c
new file mode 100644
index 0000000..84f617e
--- /dev/null
+++ b/drivers/pwm/pwm-hibvt.c
@@ -0,0 +1,274 @@
+/*
+ * PWM Controller Driver for HiSilicon BVT SOCs
+ *
+ * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+
+#define PWM_CFG0_ADDR(x)    (((x) * 0x20) + 0x0)
+#define PWM_CFG1_ADDR(x)    (((x) * 0x20) + 0x4)
+#define PWM_CFG2_ADDR(x)    (((x) * 0x20) + 0x8)
+#define PWM_CTRL_ADDR(x)    (((x) * 0x20) + 0xC)
+
+#define PWM_ENABLE_SHIFT    0
+#define PWM_ENABLE_MASK		BIT(0)
+
+#define PWM_POLARITY_SHIFT	1
+#define PWM_POLARITY_MASK	BIT(1)
+
+#define PWM_KEEP_SHIFT	    2
+#define PWM_KEEP_MASK	    BIT(2)
+
+#define PWM_PERIOD_MASK	    GENMASK(31, 0)
+#define PWM_DUTY_MASK	    GENMASK(31, 0)
+
+struct hibvt_pwm_chip {
+	struct pwm_chip	chip;
+	struct clk	*clk;
+	void __iomem	*mmio_base;
+	struct reset_control *rstc;
+};
+
+static int pwm_num_array[] = {8, 4};
+
+static inline
+struct hibvt_pwm_chip *to_hibvt_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct hibvt_pwm_chip, chip);
+}
+
+static void hibvt_pwm_set_bits(void __iomem *base, unsigned int offset,
+					unsigned int mask, unsigned int data)
+{
+	void __iomem *address = base + offset;
+	unsigned int value;
+
+	value = readl(address);
+	value &= ~mask;
+	value |= (data & mask);
+	writel(value, address);
+}
+
+static int hibvt_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
+	unsigned int offset;
+
+	offset = PWM_CTRL_ADDR(pwm->hwpwm);
+	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
+			PWM_ENABLE_MASK, 0x1);
+
+	return 0;
+}
+
+static void hibvt_pwm_disable(struct pwm_chip *chip,
+					struct pwm_device *pwm)
+{
+	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
+	unsigned int offset;
+
+	offset = PWM_CTRL_ADDR(pwm->hwpwm);
+	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
+			PWM_ENABLE_MASK, 0x0);
+}
+
+static int hibvt_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+					int duty_cycle_ns, int period_ns)
+{
+	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
+	unsigned int offset;
+	unsigned int freq;
+	unsigned int period_num, duty_num;
+
+	freq = div_u64(clk_get_rate(hi_pwm_chip->clk), 1000000);
+
+	period_num = div_u64(freq * period_ns, 1000);
+	duty_num = div_u64(period_num * duty_cycle_ns, period_ns);
+
+	offset = PWM_CFG0_ADDR(pwm->hwpwm);
+	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
+			PWM_PERIOD_MASK, period_num);
+
+	offset = PWM_CFG1_ADDR(pwm->hwpwm);
+	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
+			PWM_DUTY_MASK, duty_num);
+
+	return 0;
+}
+
+static int hibvt_pwm_set_polarity(struct pwm_chip *chip,
+					struct pwm_device *pwm,
+					enum pwm_polarity polarity)
+{
+	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
+	unsigned int offset;
+
+	offset = PWM_CTRL_ADDR(pwm->hwpwm);
+	if (polarity == PWM_POLARITY_INVERSED)
+		hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
+				PWM_POLARITY_MASK, (0x1 << PWM_POLARITY_SHIFT));
+	else
+		hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
+				PWM_POLARITY_MASK, (0x0 << PWM_POLARITY_SHIFT));
+
+	return 0;
+}
+
+void hibvt_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				struct pwm_state *state)
+{
+	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
+	void __iomem *base;
+	unsigned int offset;
+	unsigned int freq;
+	unsigned int period_num, duty_num;
+	unsigned int enable;
+
+	freq = div_u64(clk_get_rate(hi_pwm_chip->clk), 1000000);
+	base = hi_pwm_chip->mmio_base;
+
+	offset = PWM_CFG0_ADDR(pwm->hwpwm);
+	period_num = readl(base + offset);
+	state->period = div_u64(period_num * 1000, freq);
+
+	offset = PWM_CFG1_ADDR(pwm->hwpwm);
+	duty_num = readl(base + offset);
+	state->duty_cycle = div_u64(duty_num * 1000, freq);
+
+	offset = PWM_CTRL_ADDR(pwm->hwpwm);
+	enable = readl(base + offset);
+	state->enabled = (PWM_ENABLE_MASK & enable);
+}
+
+static struct pwm_ops hibvt_pwm_ops = {
+	.enable       = hibvt_pwm_enable,
+	.disable      = hibvt_pwm_disable,
+	.config       = hibvt_pwm_config,
+	.set_polarity = hibvt_pwm_set_polarity,
+	.get_state    = hibvt_pwm_get_state,
+
+	.owner        = THIS_MODULE,
+};
+
+static const struct of_device_id hibvt_pwm_of_match[];
+
+static int hibvt_pwm_probe(struct platform_device *pdev)
+{
+	struct hibvt_pwm_chip *pwm_chip;
+	const struct of_device_id *of_id =
+				of_match_device(hibvt_pwm_of_match, &pdev->dev);
+	struct resource *res;
+	int ret = 0;
+	int i;
+	int offset;
+	int pwm_nums;
+
+	if (!of_id)
+		return -ENODEV;
+
+	pwm_chip = devm_kzalloc(&pdev->dev, sizeof(*pwm_chip), GFP_KERNEL);
+	if (pwm_chip == NULL)
+		return -ENOMEM;
+
+	pwm_chip->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pwm_chip->clk)) {
+		dev_err(&pdev->dev, "getting clock failed with %ld\n",
+				PTR_ERR(pwm_chip->clk));
+		return PTR_ERR(pwm_chip->clk);
+	}
+
+	pwm_nums = *((int *)of_id->data);
+
+	pwm_chip->chip.ops = &hibvt_pwm_ops;
+	pwm_chip->chip.dev = &pdev->dev;
+	pwm_chip->chip.base = -1;
+	pwm_chip->chip.npwm = pwm_nums;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pwm_chip->mmio_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pwm_chip->mmio_base))
+		return PTR_ERR(pwm_chip->mmio_base);
+
+	ret = clk_prepare_enable(pwm_chip->clk);
+	if (ret < 0)
+		return ret;
+
+	pwm_chip->rstc = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(pwm_chip->rstc))
+		return PTR_ERR(pwm_chip->rstc);
+
+	reset_control_assert(pwm_chip->rstc);
+	msleep(30);
+	reset_control_deassert(pwm_chip->rstc);
+
+	ret = pwmchip_add(&pwm_chip->chip);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < pwm_nums; i++) {
+		offset = PWM_CTRL_ADDR(i);
+		hibvt_pwm_set_bits(pwm_chip->mmio_base, offset,
+				PWM_KEEP_MASK, (0x1 << PWM_KEEP_SHIFT));
+	}
+
+	platform_set_drvdata(pdev, pwm_chip);
+
+	return 0;
+}
+
+static int hibvt_pwm_remove(struct platform_device *pdev)
+{
+	struct hibvt_pwm_chip *pwm_chip;
+
+	pwm_chip = platform_get_drvdata(pdev);
+	if (pwm_chip == NULL)
+		return -ENODEV;
+
+	clk_disable_unprepare(pwm_chip->clk);
+
+	return pwmchip_remove(&pwm_chip->chip);
+}
+
+static const struct of_device_id hibvt_pwm_of_match[] = {
+	{.compatible = "hisilicon,hibvt-pwm", .data = &pwm_num_array[0]},
+	{.compatible = "hisilicon,hi3516cv300-pwm", .data = &pwm_num_array[1]},
+	{  }
+};
+MODULE_DEVICE_TABLE(of, hibvt_pwm_of_match);
+
+static struct platform_driver hibvt_pwm_driver = {
+	.driver		= {
+		.name	= "hibvt-pwm",
+		.of_match_table = hibvt_pwm_of_match,
+	},
+	.probe		= hibvt_pwm_probe,
+	.remove		= hibvt_pwm_remove,
+};
+
+module_platform_driver(hibvt_pwm_driver);
+
+MODULE_AUTHOR("yuanjian12@hisilicon.com");
+MODULE_DESCRIPTION("Hisilicon BVT SOCs PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

Re: [PATCH v2] pwm: add pwm driver for HiSilicon BVT SOCs

[Rob Herring]

On Mon, Aug 22, 2016 at 03:50:13PM +0800, Jian Yuan wrote:
> From: yuanjian <yuanjian12@hisilicon.com>
> 
> Add pwm driver for HiSilicon BVT SOCs
> 
> Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
> Signed-off-by: Jian Yuan <yuanjian12@hisilicon.com>
> ---
> Change Log:
> v2:
> The number of PWMs is change to be probeable based on the compatible string.
> 
>  .../devicetree/bindings/pwm/pwm-hibvt.txt          |  18 ++

For the binding:

Acked-by: Rob Herring <robh@kernel.org>

One comment though...

>  drivers/pwm/Kconfig                                |  10 +
>  drivers/pwm/Makefile                               |   1 +
>  drivers/pwm/pwm-hibvt.c                            | 274 +++++++++++++++++++++
>  4 files changed, 303 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
>  create mode 100644 drivers/pwm/pwm-hibvt.c

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index c182efc..3c48768 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -158,6 +158,15 @@ config PWM_FSL_FTM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-fsl-ftm.
>  
> +config PWM_HIBVT
> +	tristate "HiSilicon BVT PWM support"
> +	depends on ARCH_HISI

Add "|| COMPILE_TEST" so it can build test on other configs

> +	help
> +	  Generic PWM framework driver for hisilicon BVT SOCs.
> +

Re: [PATCH v2] pwm: add pwm driver for HiSilicon BVT SOCs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 14510 bytes --]

On Mon, Aug 22, 2016 at 03:50:13PM +0800, Jian Yuan wrote:
> From: yuanjian <yuanjian12@hisilicon.com>
> 
> Add pwm driver for HiSilicon BVT SOCs

pwm -> PWM, please. It'd be good to have more information here about
what the hardware can do, where to find it, etc.

> 
> Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
> Signed-off-by: Jian Yuan <yuanjian12@hisilicon.com>
> ---
> Change Log:
> v2:
> The number of PWMs is change to be probeable based on the compatible string.
> 
>  .../devicetree/bindings/pwm/pwm-hibvt.txt          |  18 ++
>  drivers/pwm/Kconfig                                |  10 +
>  drivers/pwm/Makefile                               |   1 +
>  drivers/pwm/pwm-hibvt.c                            | 274 +++++++++++++++++++++
>  4 files changed, 303 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
>  create mode 100644 drivers/pwm/pwm-hibvt.c
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
> new file mode 100644
> index 0000000..1274119
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
> @@ -0,0 +1,18 @@
> +Hisilicon PWM controller
> +
> +Required properties:
> +-compatible: should contain one soc specific compatible string and one generic compatible
> +string "hisilicon, hibvt-pwm". The soc specific strings supported including:

Why the generic compatible string? You've already shown in the driver
that the two versions you support aren't compatible.

Also soc -> SoC, please.

> +	"hisilicon,hi3516cv300-pwm"
> +- reg: physical base address and length of the controller's registers.
> +- clocks: phandle and clock specifier of the PWM reference clock.
> +- resets: offset address and offset bit for reset or unreset of the controller.

That's weird. Usually this should say just that the property should
contain the phandle plus a reset specifier for the controller reset. The
above description already defines what the specifier looks like, which
may not be true on all SoCs that this hardware gets integrated into.

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index c182efc..3c48768 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -158,6 +158,15 @@ config PWM_FSL_FTM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-fsl-ftm.
>  
> +config PWM_HIBVT
> +	tristate "HiSilicon BVT PWM support"
> +	depends on ARCH_HISI
> +	help
> +	  Generic PWM framework driver for hisilicon BVT SOCs.

Please use a consistent spelling for HiSilicon. Also: SOC -> SoC.

> @@ -438,4 +447,5 @@ config PWM_VT8500
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-vt8500.
>  
> +

Please don't add this extra blank line.

> diff --git a/drivers/pwm/pwm-hibvt.c b/drivers/pwm/pwm-hibvt.c
[...]
> new file mode 100644
> index 0000000..84f617e
> --- /dev/null
> +++ b/drivers/pwm/pwm-hibvt.c
> @@ -0,0 +1,274 @@
> +/*
> + * PWM Controller Driver for HiSilicon BVT SOCs
> + *
> + * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +
> +#define PWM_CFG0_ADDR(x)    (((x) * 0x20) + 0x0)
> +#define PWM_CFG1_ADDR(x)    (((x) * 0x20) + 0x4)
> +#define PWM_CFG2_ADDR(x)    (((x) * 0x20) + 0x8)
> +#define PWM_CTRL_ADDR(x)    (((x) * 0x20) + 0xC)
> +
> +#define PWM_ENABLE_SHIFT    0
> +#define PWM_ENABLE_MASK		BIT(0)
> +
> +#define PWM_POLARITY_SHIFT	1
> +#define PWM_POLARITY_MASK	BIT(1)
> +
> +#define PWM_KEEP_SHIFT	    2
> +#define PWM_KEEP_MASK	    BIT(2)
> +
> +#define PWM_PERIOD_MASK	    GENMASK(31, 0)
> +#define PWM_DUTY_MASK	    GENMASK(31, 0)

There's inconsistent padding in the above. I thought checkpatch warned
about this nowadays.

> +
> +struct hibvt_pwm_chip {
> +	struct pwm_chip	chip;
> +	struct clk	*clk;
> +	void __iomem	*mmio_base;
> +	struct reset_control *rstc;
> +};

No artificial padding with tabs above. A single space between type and
variable name is good enough. Also why the extra mmio_ prefix for the
I/O memory base address? You use the much shorter "base" for variables
elsewhere, why not in this structure?

> +
> +static int pwm_num_array[] = {8, 4};

static const unsigned, please. Or better yet, introduce a separate
structure for this type of SoC-specific data. Something like:

	struct hibvt_pwm_soc {
		unsigned int num_pwms;
	};

> +
> +static inline
> +struct hibvt_pwm_chip *to_hibvt_pwm_chip(struct pwm_chip *chip)

It's more idiomatic to put the return type on the first line as well.

> +{
> +	return container_of(chip, struct hibvt_pwm_chip, chip);
> +}
> +
> +static void hibvt_pwm_set_bits(void __iomem *base, unsigned int offset,
> +					unsigned int mask, unsigned int data)

Please align arguments on subsequent lines with the first argument on
the first line. And use u32 for mask and data since that's what readl()
and writel() use.

> +{
> +	void __iomem *address = base + offset;
> +	unsigned int value;

u32, please.

> +
> +	value = readl(address);
> +	value &= ~mask;
> +	value |= (data & mask);
> +	writel(value, address);
> +}
> +
> +static int hibvt_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
> +	unsigned int offset;
> +
> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,

I don't think the temporary variable "offset" is useful here.

> +			PWM_ENABLE_MASK, 0x1);

Also, please align this properly.

> +
> +	return 0;
> +}
> +
> +static void hibvt_pwm_disable(struct pwm_chip *chip,
> +					struct pwm_device *pwm)

The above will fit on a single line.

> +{
> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
> +	unsigned int offset;
> +
> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
> +			PWM_ENABLE_MASK, 0x0);

Again, I don't think the offset variable is any good here.

> +}
> +
> +static int hibvt_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +					int duty_cycle_ns, int period_ns)
> +{
> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
> +	unsigned int offset;
> +	unsigned int freq;
> +	unsigned int period_num, duty_num;

Why the _num suffix? You could just name them "period" and "duty". Also
you can put the last four variables above on the same line.

> +
> +	freq = div_u64(clk_get_rate(hi_pwm_chip->clk), 1000000);
> +
> +	period_num = div_u64(freq * period_ns, 1000);
> +	duty_num = div_u64(period_num * duty_cycle_ns, period_ns);
> +
> +	offset = PWM_CFG0_ADDR(pwm->hwpwm);
> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
> +			PWM_PERIOD_MASK, period_num);
> +
> +	offset = PWM_CFG1_ADDR(pwm->hwpwm);
> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
> +			PWM_DUTY_MASK, duty_num);

offset can be dropped here as well, in my opinion.

> +
> +	return 0;
> +}
> +
> +static int hibvt_pwm_set_polarity(struct pwm_chip *chip,
> +					struct pwm_device *pwm,
> +					enum pwm_polarity polarity)
> +{
> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
> +	unsigned int offset;
> +
> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
> +	if (polarity == PWM_POLARITY_INVERSED)
> +		hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
> +				PWM_POLARITY_MASK, (0x1 << PWM_POLARITY_SHIFT));
> +	else
> +		hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
> +				PWM_POLARITY_MASK, (0x0 << PWM_POLARITY_SHIFT));
> +
> +	return 0;
> +}
> +
> +void hibvt_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				struct pwm_state *state)
> +{
> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
> +	void __iomem *base;
> +	unsigned int offset;
> +	unsigned int freq;
> +	unsigned int period_num, duty_num;
> +	unsigned int enable;

I think you could trim the list of variables down a little. offset can
go away for reasons given above. And of the other four, the only one
that you need to reuse is freq, so period_num, duty_num and enable can
all be replaced by a "value" variable, for example. Also, make that an
u32, because that's what readl() returns.

> +
> +	freq = div_u64(clk_get_rate(hi_pwm_chip->clk), 1000000);
> +	base = hi_pwm_chip->mmio_base;
> +
> +	offset = PWM_CFG0_ADDR(pwm->hwpwm);
> +	period_num = readl(base + offset);
> +	state->period = div_u64(period_num * 1000, freq);
> +
> +	offset = PWM_CFG1_ADDR(pwm->hwpwm);
> +	duty_num = readl(base + offset);
> +	state->duty_cycle = div_u64(duty_num * 1000, freq);
> +
> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
> +	enable = readl(base + offset);
> +	state->enabled = (PWM_ENABLE_MASK & enable);
> +}
> +
> +static struct pwm_ops hibvt_pwm_ops = {
> +	.enable       = hibvt_pwm_enable,
> +	.disable      = hibvt_pwm_disable,
> +	.config       = hibvt_pwm_config,
> +	.set_polarity = hibvt_pwm_set_polarity,
> +	.get_state    = hibvt_pwm_get_state,

Please don't mix legacy and atomic support. If you support .get_state(),
you should implement .apply() instead of .enable()/.disable()/.config().
Also, please remove the artificial padding.

> +
> +	.owner        = THIS_MODULE,
> +};
> +
> +static const struct of_device_id hibvt_pwm_of_match[];

No need for this...

> +
> +static int hibvt_pwm_probe(struct platform_device *pdev)
> +{
> +	struct hibvt_pwm_chip *pwm_chip;
> +	const struct of_device_id *of_id =
> +				of_match_device(hibvt_pwm_of_match, &pdev->dev);

... if you use of_device_get_match_data() here.

> +	struct resource *res;
> +	int ret = 0;

No need to initialize to 0 here.

> +	int i;

unsigned int, please.

> +	int offset;
> +	int pwm_nums;

I think both of these can be dropped. See below.

> +
> +	if (!of_id)
> +		return -ENODEV;
> +
> +	pwm_chip = devm_kzalloc(&pdev->dev, sizeof(*pwm_chip), GFP_KERNEL);
> +	if (pwm_chip == NULL)
> +		return -ENOMEM;
> +
> +	pwm_chip->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pwm_chip->clk)) {
> +		dev_err(&pdev->dev, "getting clock failed with %ld\n",
> +				PTR_ERR(pwm_chip->clk));
> +		return PTR_ERR(pwm_chip->clk);
> +	}
> +
> +	pwm_nums = *((int *)of_id->data);

This becomes a lot easier to read if you use a proper structure for the
SoC specific data:

	const struct hibvt_pwm_soc *soc = of_device_get_match_data(&pdev->dev);

	pwm_chip->chip.npwm = soc->num_pwms;

> +
> +	pwm_chip->chip.ops = &hibvt_pwm_ops;
> +	pwm_chip->chip.dev = &pdev->dev;
> +	pwm_chip->chip.base = -1;
> +	pwm_chip->chip.npwm = pwm_nums;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pwm_chip->mmio_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pwm_chip->mmio_base))
> +		return PTR_ERR(pwm_chip->mmio_base);
> +
> +	ret = clk_prepare_enable(pwm_chip->clk);
> +	if (ret < 0)
> +		return ret;
> +
> +	pwm_chip->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(pwm_chip->rstc))
> +		return PTR_ERR(pwm_chip->rstc);
> +

You might want to move clk_prepare_enable() below this so you don't keep
it enabled when the reset control can't be requested.

> +	reset_control_assert(pwm_chip->rstc);
> +	msleep(30);
> +	reset_control_deassert(pwm_chip->rstc);
> +
> +	ret = pwmchip_add(&pwm_chip->chip);
> +	if (ret < 0)
> +		return ret;

You might want to assert the reset and disable and unprepare the clock
when this fails.

> +
> +	for (i = 0; i < pwm_nums; i++) {
> +		offset = PWM_CTRL_ADDR(i);
> +		hibvt_pwm_set_bits(pwm_chip->mmio_base, offset,

Things will still fit on one line if you use PWM_CTRL_ADDR(i) in place
above, so I don't think you gain anything by adding the temporary
variable.

> +				PWM_KEEP_MASK, (0x1 << PWM_KEEP_SHIFT));
> +	}
> +
> +	platform_set_drvdata(pdev, pwm_chip);
> +
> +	return 0;
> +}
> +
> +static int hibvt_pwm_remove(struct platform_device *pdev)
> +{
> +	struct hibvt_pwm_chip *pwm_chip;
> +
> +	pwm_chip = platform_get_drvdata(pdev);
> +	if (pwm_chip == NULL)
> +		return -ENODEV;

There's no way this will ever happen.

> +
> +	clk_disable_unprepare(pwm_chip->clk);

What about the reset?

> +
> +	return pwmchip_remove(&pwm_chip->chip);
> +}
> +
> +static const struct of_device_id hibvt_pwm_of_match[] = {
> +	{.compatible = "hisilicon,hibvt-pwm", .data = &pwm_num_array[0]},
> +	{.compatible = "hisilicon,hi3516cv300-pwm", .data = &pwm_num_array[1]},

Spaces after { and before }, please.

> +	{  }
> +};
> +MODULE_DEVICE_TABLE(of, hibvt_pwm_of_match);
> +
> +static struct platform_driver hibvt_pwm_driver = {
> +	.driver		= {
> +		.name	= "hibvt-pwm",
> +		.of_match_table = hibvt_pwm_of_match,
> +	},
> +	.probe		= hibvt_pwm_probe,
> +	.remove		= hibvt_pwm_remove,
> +};

No need for the artificial padding, it's not working correctly for the
.of_match_table field anyway.

> +
> +module_platform_driver(hibvt_pwm_driver);

No blank line between the "};" and "module_platform_driver(...);" line,
please.

> +
> +MODULE_AUTHOR("yuanjian12@hisilicon.com");

It's customary to put your real name here.

> +MODULE_DESCRIPTION("Hisilicon BVT SOCs PWM driver");

Again, please use consistent spelling for HiSilicon.

> +MODULE_LICENSE("GPL v2");

The header comment says "GPL v2 or later", but "GPL v2" here means GPL
v2 only.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] pwm: add pwm driver for HiSilicon BVT SOCs

[Jian Yuan]

On 2016/8/24 21:05, Thierry Reding wrote:
> On Mon, Aug 22, 2016 at 03:50:13PM +0800, Jian Yuan wrote:
>> From: yuanjian <yuanjian12@hisilicon.com>
>>
>> Add pwm driver for HiSilicon BVT SOCs
> 
> pwm -> PWM, please. It'd be good to have more information here about
> what the hardware can do, where to find it, etc.
> 
Not sure what you mean? Should I describe what the PWM or the BVT SoCs can do?
>>
>> Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
>> Signed-off-by: Jian Yuan <yuanjian12@hisilicon.com>
>> ---
>> Change Log:
>> v2:
>> The number of PWMs is change to be probeable based on the compatible string.
>>
>>  .../devicetree/bindings/pwm/pwm-hibvt.txt          |  18 ++
>>  drivers/pwm/Kconfig                                |  10 +
>>  drivers/pwm/Makefile                               |   1 +
>>  drivers/pwm/pwm-hibvt.c                            | 274 +++++++++++++++++++++
>>  4 files changed, 303 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
>>  create mode 100644 drivers/pwm/pwm-hibvt.c
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
>> new file mode 100644
>> index 0000000..1274119
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
>> @@ -0,0 +1,18 @@
>> +Hisilicon PWM controller
>> +
>> +Required properties:
>> +-compatible: should contain one soc specific compatible string and one generic compatible
>> +string "hisilicon, hibvt-pwm". The soc specific strings supported including:
> 
> Why the generic compatible string? You've already shown in the driver
> that the two versions you support aren't compatible.
>
The generic compatible string should be contained in every devicetree bindings that BVT Socs support.
But I'll add another specific compatible string to distinguish it from "hisilicon,hi3516cv300-pwm".

> Also soc -> SoC, please.
> 
>> +	"hisilicon,hi3516cv300-pwm"
>> +- reg: physical base address and length of the controller's registers.
>> +- clocks: phandle and clock specifier of the PWM reference clock.
>> +- resets: offset address and offset bit for reset or unreset of the controller.
> 
> That's weird. Usually this should say just that the property should
> contain the phandle plus a reset specifier for the controller reset. The
> above description already defines what the specifier looks like, which
> may not be true on all SoCs that this hardware gets integrated into.
> 
Ok.
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index c182efc..3c48768 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -158,6 +158,15 @@ config PWM_FSL_FTM
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-fsl-ftm.
>>  
>> +config PWM_HIBVT
>> +	tristate "HiSilicon BVT PWM support"
>> +	depends on ARCH_HISI
>> +	help
>> +	  Generic PWM framework driver for hisilicon BVT SOCs.
> 
> Please use a consistent spelling for HiSilicon. Also: SOC -> SoC.
> 
Ok.
>> @@ -438,4 +447,5 @@ config PWM_VT8500
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-vt8500.
>>  
>> +
> 
> Please don't add this extra blank line.
> 
Ok.
>> diff --git a/drivers/pwm/pwm-hibvt.c b/drivers/pwm/pwm-hibvt.c
> [...]
>> new file mode 100644
>> index 0000000..84f617e
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-hibvt.c
>> @@ -0,0 +1,274 @@
>> +/*
>> + * PWM Controller Driver for HiSilicon BVT SOCs
>> + *
>> + * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
>> + *
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/reset.h>
>> +
>> +#define PWM_CFG0_ADDR(x)    (((x) * 0x20) + 0x0)
>> +#define PWM_CFG1_ADDR(x)    (((x) * 0x20) + 0x4)
>> +#define PWM_CFG2_ADDR(x)    (((x) * 0x20) + 0x8)
>> +#define PWM_CTRL_ADDR(x)    (((x) * 0x20) + 0xC)
>> +
>> +#define PWM_ENABLE_SHIFT    0
>> +#define PWM_ENABLE_MASK		BIT(0)
>> +
>> +#define PWM_POLARITY_SHIFT	1
>> +#define PWM_POLARITY_MASK	BIT(1)
>> +
>> +#define PWM_KEEP_SHIFT	    2
>> +#define PWM_KEEP_MASK	    BIT(2)
>> +
>> +#define PWM_PERIOD_MASK	    GENMASK(31, 0)
>> +#define PWM_DUTY_MASK	    GENMASK(31, 0)
> 
> There's inconsistent padding in the above. I thought checkpatch warned
> about this nowadays.
> 
>> +
>> +struct hibvt_pwm_chip {
>> +	struct pwm_chip	chip;
>> +	struct clk	*clk;
>> +	void __iomem	*mmio_base;
>> +	struct reset_control *rstc;
>> +};
> 
> No artificial padding with tabs above. A single space between type and
> variable name is good enough. Also why the extra mmio_ prefix for the
> I/O memory base address? You use the much shorter "base" for variables
> elsewhere, why not in this structure?
> 
Ok.
>> +
>> +static int pwm_num_array[] = {8, 4};
> 
> static const unsigned, please. Or better yet, introduce a separate
> structure for this type of SoC-specific data. Something like:
> 
> 	struct hibvt_pwm_soc {
> 		unsigned int num_pwms;
> 	};
> 
Good idea.
>> +
>> +static inline
>> +struct hibvt_pwm_chip *to_hibvt_pwm_chip(struct pwm_chip *chip)
> 
> It's more idiomatic to put the return type on the first line as well.
> 
Ok.
>> +{
>> +	return container_of(chip, struct hibvt_pwm_chip, chip);
>> +}
>> +
>> +static void hibvt_pwm_set_bits(void __iomem *base, unsigned int offset,
>> +					unsigned int mask, unsigned int data)
> 
> Please align arguments on subsequent lines with the first argument on
> the first line. And use u32 for mask and data since that's what readl()
> and writel() use.
> 
Ok.
>> +{
>> +	void __iomem *address = base + offset;
>> +	unsigned int value;
> 
> u32, please.
> 
Ok.
>> +
>> +	value = readl(address);
>> +	value &= ~mask;
>> +	value |= (data & mask);
>> +	writel(value, address);
>> +}
>> +
>> +static int hibvt_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
>> +	unsigned int offset;
>> +
>> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
>> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
> 
> I don't think the temporary variable "offset" is useful here.
> 
>> +			PWM_ENABLE_MASK, 0x1);
> 
> Also, please align this properly.
> 
Ok.
>> +
>> +	return 0;
>> +}
>> +
>> +static void hibvt_pwm_disable(struct pwm_chip *chip,
>> +					struct pwm_device *pwm)
> 
> The above will fit on a single line.
> 
Ok.
>> +{
>> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
>> +	unsigned int offset;
>> +
>> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
>> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
>> +			PWM_ENABLE_MASK, 0x0);
> 
> Again, I don't think the offset variable is any good here.
> 
Ok.
>> +}
>> +
>> +static int hibvt_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> +					int duty_cycle_ns, int period_ns)
>> +{
>> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
>> +	unsigned int offset;
>> +	unsigned int freq;
>> +	unsigned int period_num, duty_num;
> 
> Why the _num suffix? You could just name them "period" and "duty". Also
> you can put the last four variables above on the same line.
> 
Ok.
>> +
>> +	freq = div_u64(clk_get_rate(hi_pwm_chip->clk), 1000000);
>> +
>> +	period_num = div_u64(freq * period_ns, 1000);
>> +	duty_num = div_u64(period_num * duty_cycle_ns, period_ns);
>> +
>> +	offset = PWM_CFG0_ADDR(pwm->hwpwm);
>> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
>> +			PWM_PERIOD_MASK, period_num);
>> +
>> +	offset = PWM_CFG1_ADDR(pwm->hwpwm);
>> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
>> +			PWM_DUTY_MASK, duty_num);
> 
> offset can be dropped here as well, in my opinion.
> 
Ok.
>> +
>> +	return 0;
>> +}
>> +
>> +static int hibvt_pwm_set_polarity(struct pwm_chip *chip,
>> +					struct pwm_device *pwm,
>> +					enum pwm_polarity polarity)
>> +{
>> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
>> +	unsigned int offset;
>> +
>> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
>> +	if (polarity == PWM_POLARITY_INVERSED)
>> +		hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
>> +				PWM_POLARITY_MASK, (0x1 << PWM_POLARITY_SHIFT));
>> +	else
>> +		hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
>> +				PWM_POLARITY_MASK, (0x0 << PWM_POLARITY_SHIFT));
>> +
>> +	return 0;
>> +}
>> +
>> +void hibvt_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> +				struct pwm_state *state)
>> +{
>> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
>> +	void __iomem *base;
>> +	unsigned int offset;
>> +	unsigned int freq;
>> +	unsigned int period_num, duty_num;
>> +	unsigned int enable;
> 
> I think you could trim the list of variables down a little. offset can
> go away for reasons given above. And of the other four, the only one
> that you need to reuse is freq, so period_num, duty_num and enable can
> all be replaced by a "value" variable, for example. Also, make that an
> u32, because that's what readl() returns.
> 
Fine, it looks like a better solution.
>> +
>> +	freq = div_u64(clk_get_rate(hi_pwm_chip->clk), 1000000);
>> +	base = hi_pwm_chip->mmio_base;
>> +
>> +	offset = PWM_CFG0_ADDR(pwm->hwpwm);
>> +	period_num = readl(base + offset);
>> +	state->period = div_u64(period_num * 1000, freq);
>> +
>> +	offset = PWM_CFG1_ADDR(pwm->hwpwm);
>> +	duty_num = readl(base + offset);
>> +	state->duty_cycle = div_u64(duty_num * 1000, freq);
>> +
>> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
>> +	enable = readl(base + offset);
>> +	state->enabled = (PWM_ENABLE_MASK & enable);
>> +}
>> +
>> +static struct pwm_ops hibvt_pwm_ops = {
>> +	.enable       = hibvt_pwm_enable,
>> +	.disable      = hibvt_pwm_disable,
>> +	.config       = hibvt_pwm_config,
>> +	.set_polarity = hibvt_pwm_set_polarity,
>> +	.get_state    = hibvt_pwm_get_state,
> 
> Please don't mix legacy and atomic support. If you support .get_state(),
> you should implement .apply() instead of .enable()/.disable()/.config().
> Also, please remove the artificial padding.
> 
I agree to implement .apply() function.
>> +
>> +	.owner        = THIS_MODULE,
>> +};
>> +
>> +static const struct of_device_id hibvt_pwm_of_match[];
> 
> No need for this...
> 
Ok.
>> +
>> +static int hibvt_pwm_probe(struct platform_device *pdev)
>> +{
>> +	struct hibvt_pwm_chip *pwm_chip;
>> +	const struct of_device_id *of_id =
>> +				of_match_device(hibvt_pwm_of_match, &pdev->dev);
> 
> ... if you use of_device_get_match_data() here.
> 
>> +	struct resource *res;
>> +	int ret = 0;
> 
> No need to initialize to 0 here.
> 
>> +	int i;
> 
> unsigned int, please.
> 
>> +	int offset;
>> +	int pwm_nums;
> 
> I think both of these can be dropped. See below.
> 
>> +
>> +	if (!of_id)
>> +		return -ENODEV;
>> +
>> +	pwm_chip = devm_kzalloc(&pdev->dev, sizeof(*pwm_chip), GFP_KERNEL);
>> +	if (pwm_chip == NULL)
>> +		return -ENOMEM;
>> +
>> +	pwm_chip->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(pwm_chip->clk)) {
>> +		dev_err(&pdev->dev, "getting clock failed with %ld\n",
>> +				PTR_ERR(pwm_chip->clk));
>> +		return PTR_ERR(pwm_chip->clk);
>> +	}
>> +
>> +	pwm_nums = *((int *)of_id->data);
> 
> This becomes a lot easier to read if you use a proper structure for the
> SoC specific data:
> 
> 	const struct hibvt_pwm_soc *soc = of_device_get_match_data(&pdev->dev);
> 
> 	pwm_chip->chip.npwm = soc->num_pwms;
> 
That's right.
>> +
>> +	pwm_chip->chip.ops = &hibvt_pwm_ops;
>> +	pwm_chip->chip.dev = &pdev->dev;
>> +	pwm_chip->chip.base = -1;
>> +	pwm_chip->chip.npwm = pwm_nums;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	pwm_chip->mmio_base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(pwm_chip->mmio_base))
>> +		return PTR_ERR(pwm_chip->mmio_base);
>> +
>> +	ret = clk_prepare_enable(pwm_chip->clk);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	pwm_chip->rstc = devm_reset_control_get(&pdev->dev, NULL);
>> +	if (IS_ERR(pwm_chip->rstc))
>> +		return PTR_ERR(pwm_chip->rstc);
>> +
> 
> You might want to move clk_prepare_enable() below this so you don't keep
> it enabled when the reset control can't be requested.
> 
Ok, I'll add it.
>> +	reset_control_assert(pwm_chip->rstc);
>> +	msleep(30);
>> +	reset_control_deassert(pwm_chip->rstc);
>> +
>> +	ret = pwmchip_add(&pwm_chip->chip);
>> +	if (ret < 0)
>> +		return ret;
> 
> You might want to assert the reset and disable and unprepare the clock
> when this fails.
> 
>> +
>> +	for (i = 0; i < pwm_nums; i++) {
>> +		offset = PWM_CTRL_ADDR(i);
>> +		hibvt_pwm_set_bits(pwm_chip->mmio_base, offset,
> 
> Things will still fit on one line if you use PWM_CTRL_ADDR(i) in place
> above, so I don't think you gain anything by adding the temporary
> variable.
> 
>> +				PWM_KEEP_MASK, (0x1 << PWM_KEEP_SHIFT));
>> +	}
>> +
>> +	platform_set_drvdata(pdev, pwm_chip);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hibvt_pwm_remove(struct platform_device *pdev)
>> +{
>> +	struct hibvt_pwm_chip *pwm_chip;
>> +
>> +	pwm_chip = platform_get_drvdata(pdev);
>> +	if (pwm_chip == NULL)
>> +		return -ENODEV;
> 
> There's no way this will ever happen.
> 
>> +
>> +	clk_disable_unprepare(pwm_chip->clk);
> 
> What about the reset?
> 
Ok.
>> +
>> +	return pwmchip_remove(&pwm_chip->chip);
>> +}
>> +
>> +static const struct of_device_id hibvt_pwm_of_match[] = {
>> +	{.compatible = "hisilicon,hibvt-pwm", .data = &pwm_num_array[0]},
>> +	{.compatible = "hisilicon,hi3516cv300-pwm", .data = &pwm_num_array[1]},
> 
> Spaces after { and before }, please.
> 
Ok.
>> +	{  }
>> +};
>> +MODULE_DEVICE_TABLE(of, hibvt_pwm_of_match);
>> +
>> +static struct platform_driver hibvt_pwm_driver = {
>> +	.driver		= {
>> +		.name	= "hibvt-pwm",
>> +		.of_match_table = hibvt_pwm_of_match,
>> +	},
>> +	.probe		= hibvt_pwm_probe,
>> +	.remove		= hibvt_pwm_remove,
>> +};
> 
> No need for the artificial padding, it's not working correctly for the
> .of_match_table field anyway.
> 
>> +
>> +module_platform_driver(hibvt_pwm_driver);
> 
> No blank line between the "};" and "module_platform_driver(...);" line,
> please.
> 
>> +
>> +MODULE_AUTHOR("yuanjian12@hisilicon.com");
> 
> It's customary to put your real name here.
> 
>> +MODULE_DESCRIPTION("Hisilicon BVT SOCs PWM driver");
> 
> Again, please use consistent spelling for HiSilicon.
> 
>> +MODULE_LICENSE("GPL v2");
> 
> The header comment says "GPL v2 or later", but "GPL v2" here means GPL
> v2 only.
> 
Ok.
> Thierry
> 

Thanks.
Jian Yuan

Re: [PATCH v2] pwm: add pwm driver for HiSilicon BVT SOCs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2965 bytes --]

On Thu, Aug 25, 2016 at 05:15:36PM +0800, Jian Yuan wrote:
> 
> 
> On 2016/8/24 21:05, Thierry Reding wrote:
> > On Mon, Aug 22, 2016 at 03:50:13PM +0800, Jian Yuan wrote:
> >> From: yuanjian <yuanjian12@hisilicon.com>
> >>
> >> Add pwm driver for HiSilicon BVT SOCs
> > 
> > pwm -> PWM, please. It'd be good to have more information here about
> > what the hardware can do, where to find it, etc.
> > 
> Not sure what you mean? Should I describe what the PWM or the BVT SoCs can do?

Some of the things you could mention here are how many channels the PWM
controller supports, if there any noteworthy limitations or extra
features. Looking at the driver the controller is pretty standard, but
you could mention that it supports signal polarity (not all controllers
do) and that it can be found on two SoCs and name them. Sometimes it's
also useful to give some information about typical use-cases (perhaps
one of the PWMs is used to control backlight of a panel with some widely
available development board?).

> >> Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
> >> Signed-off-by: Jian Yuan <yuanjian12@hisilicon.com>
> >> ---
> >> Change Log:
> >> v2:
> >> The number of PWMs is change to be probeable based on the compatible string.
> >>
> >>  .../devicetree/bindings/pwm/pwm-hibvt.txt          |  18 ++
> >>  drivers/pwm/Kconfig                                |  10 +
> >>  drivers/pwm/Makefile                               |   1 +
> >>  drivers/pwm/pwm-hibvt.c                            | 274 +++++++++++++++++++++
> >>  4 files changed, 303 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
> >>  create mode 100644 drivers/pwm/pwm-hibvt.c
> >>
> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
> >> new file mode 100644
> >> index 0000000..1274119
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
> >> @@ -0,0 +1,18 @@
> >> +Hisilicon PWM controller
> >> +
> >> +Required properties:
> >> +-compatible: should contain one soc specific compatible string and one generic compatible
> >> +string "hisilicon, hibvt-pwm". The soc specific strings supported including:
> > 
> > Why the generic compatible string? You've already shown in the driver
> > that the two versions you support aren't compatible.
> >
> The generic compatible string should be contained in every devicetree
> bindings that BVT Socs support.
> But I'll add another specific compatible string to distinguish it from
> "hisilicon,hi3516cv300-pwm".

There's no use in specifying a generic compatible string if a driver
can't do anything useful with it. If you've only got hisilicon,hibvt-pwm
in the device tree, a driver wouldn't know the target SoC and hence has
no knowledge about the number of channels. Therefore the generic
compatible string is useless.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]