[PATCH v8 0/2] pwm: add support for atmel-hlcdc-pwm device

[PATCH v8 0/2] pwm: add support for atmel-hlcdc-pwm device

[Boris Brezillon]

Hi Thierry,

This patch series adds support for the atmel-hlcdc-pwm device provided
by the atmel-hlcdc MFD driver.

It depends on this series [1] implementing the MFD driver.

Best Regards,

Boris

[1]http://www.mail-archive.com/devicetree@vger.kernel.org/msg44979.html

Changes since v7:
 - addressed several coding style issues

Boris Brezillon (2):
  pwm: add support for atmel-hlcdc-pwm device
  pwm: add DT bindings documentation for atmel-hlcdc-pwm driver

 .../devicetree/bindings/pwm/atmel-hlcdc-pwm.txt    |  50 +++++
 drivers/pwm/Kconfig                                |   9 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-atmel-hlcdc.c                      | 248 +++++++++++++++++++++
 4 files changed, 308 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt
 create mode 100644 drivers/pwm/pwm-atmel-hlcdc.c

-- 
1.9.1

[PATCH v8 1/2] pwm: add support for atmel-hlcdc-pwm device

[Boris Brezillon]

The HLCDC IP available in some Atmel SoCs (i.e. sam9x5i.e. at91sam9n12,
at91sam9x5 family or sama5d3 family) provide a PWM device.

This driver add support for a PWM chip exposing a single PWM device (which
will most likely be used to drive a backlight device).

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Tested-by: Anthony Harivel <anthony.harivel@emtrion.de>
Tested-by: Ludovic Desroches <ludovic.desroches@atmel.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/Kconfig           |   9 ++
 drivers/pwm/Makefile          |   1 +
 drivers/pwm/pwm-atmel-hlcdc.c | 248 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 drivers/pwm/pwm-atmel-hlcdc.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b800783..9bb331b 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -50,6 +50,15 @@ config PWM_ATMEL
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-atmel.
 
+config PWM_ATMEL_HLCDC_PWM
+	tristate "Atmel HLCDC PWM support"
+	select MFD_ATMEL_HLCDC
+	help
+	  Generic PWM framework driver for Atmel HLCDC PWM.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-atmel.
+
 config PWM_ATMEL_TCB
 	tristate "Atmel TC Block PWM support"
 	depends on ATMEL_TCLIB && OF
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index f8c577d..eb0aae5 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_PWM)		+= core.o
 obj-$(CONFIG_PWM_SYSFS)		+= sysfs.o
 obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
 obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
+obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
 obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
new file mode 100644
index 0000000..c7442d5
--- /dev/null
+++ b/drivers/pwm/pwm-atmel-hlcdc.c
@@ -0,0 +1,248 @@
+/*
+ * Copyright (C) 2014 Free Electrons
+ * Copyright (C) 2014 Atmel
+ *
+ * Author: Boris BREZILLON <boris.brezillon@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * 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/clk.h>
+#include <linux/delay.h>
+#include <linux/mfd/atmel-hlcdc.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define ATMEL_HLCDC_PWMCVAL_MASK	GENMASK(15, 8)
+#define ATMEL_HLCDC_PWMCVAL(x)		(((x) << 8) & ATMEL_HLCDC_PWMCVAL_MASK)
+#define ATMEL_HLCDC_PWMPOL		BIT(4)
+#define ATMEL_HLCDC_PWMPS_MASK		GENMASK(2, 0)
+#define ATMEL_HLCDC_PWMPS_MAX		0x6
+#define ATMEL_HLCDC_PWMPS(x)		((x) & ATMEL_HLCDC_PWMPS_MASK)
+
+struct atmel_hlcdc_pwm {
+	struct pwm_chip chip;
+	struct atmel_hlcdc *hlcdc;
+	struct clk *cur_clk;
+};
+
+static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct atmel_hlcdc_pwm, chip);
+}
+
+static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
+				  struct pwm_device *pwm,
+				  int duty_ns, int period_ns)
+{
+	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
+	struct atmel_hlcdc *hlcdc = chip->hlcdc;
+	struct clk *new_clk = hlcdc->slow_clk;
+	u64 pwmcval = duty_ns * 256;
+	unsigned long clk_freq;
+	u64 clk_period_ns;
+	u32 pwmcfg;
+	int pres;
+
+	clk_freq = clk_get_rate(new_clk);
+	clk_period_ns = (u64)NSEC_PER_SEC * 256;
+	do_div(clk_period_ns, clk_freq);
+
+	if (clk_period_ns > period_ns) {
+		new_clk = hlcdc->sys_clk;
+		clk_freq = clk_get_rate(new_clk);
+		clk_period_ns = (u64)NSEC_PER_SEC * 256;
+		do_div(clk_period_ns, clk_freq);
+	}
+
+	for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++)
+		if ((clk_period_ns << pres) >= period_ns)
+			break;
+
+	if (pres > ATMEL_HLCDC_PWMPS_MAX)
+		return -EINVAL;
+
+	pwmcfg = ATMEL_HLCDC_PWMPS(pres);
+
+	if (new_clk != chip->cur_clk) {
+		u32 gencfg = 0;
+		int ret;
+
+		ret = clk_prepare_enable(new_clk);
+		if (ret)
+			return ret;
+
+		clk_disable_unprepare(chip->cur_clk);
+		chip->cur_clk = new_clk;
+
+		if (new_clk == hlcdc->sys_clk)
+			gencfg = ATMEL_HLCDC_CLKPWMSEL;
+
+		regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(0),
+				   ATMEL_HLCDC_CLKPWMSEL, gencfg);
+	}
+
+	do_div(pwmcval, period_ns);
+
+	/*
+	 * The PWM duty cycle is configurable from 0/256 to 255/256 of the
+	 * period cycle. Hence we can't set a duty cycle occupying the
+	 * whole period cycle if we're asked to.
+	 * Set it to 255 if pwmcval is greater than 256.
+	 */
+	if (pwmcval > 255)
+		pwmcval = 255;
+
+	pwmcfg |= ATMEL_HLCDC_PWMCVAL(pwmcval);
+
+	regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(6),
+			   ATMEL_HLCDC_PWMCVAL_MASK | ATMEL_HLCDC_PWMPS_MASK,
+			   pwmcfg);
+
+	return 0;
+}
+
+static int atmel_hlcdc_pwm_set_polarity(struct pwm_chip *c,
+					struct pwm_device *pwm,
+					enum pwm_polarity polarity)
+{
+	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
+	struct atmel_hlcdc *hlcdc = chip->hlcdc;
+	u32 cfg = 0;
+
+	if (polarity == PWM_POLARITY_NORMAL)
+		cfg = ATMEL_HLCDC_PWMPOL;
+
+	regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(6),
+			   ATMEL_HLCDC_PWMPOL, cfg);
+
+	return 0;
+}
+
+static int atmel_hlcdc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm)
+{
+	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
+	struct atmel_hlcdc *hlcdc = chip->hlcdc;
+	u32 status;
+	int ret;
+
+	regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN, ATMEL_HLCDC_PWM);
+
+	do {
+		usleep_range(1, 10);
+		ret = regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status);
+		if (ret)
+			return ret;
+	} while ((status & ATMEL_HLCDC_PWM) == 0);
+
+	return 0;
+}
+
+static void atmel_hlcdc_pwm_disable(struct pwm_chip *c,
+				    struct pwm_device *pwm)
+{
+	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
+	struct atmel_hlcdc *hlcdc = chip->hlcdc;
+	u32 status;
+	int ret;
+
+	regmap_write(hlcdc->regmap, ATMEL_HLCDC_DIS, ATMEL_HLCDC_PWM);
+
+	do {
+		usleep_range(1, 10);
+		ret = regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status);
+		if (ret)
+			return;
+	} while ((status & ATMEL_HLCDC_PWM) != 0);
+}
+
+static const struct pwm_ops atmel_hlcdc_pwm_ops = {
+	.config = atmel_hlcdc_pwm_config,
+	.set_polarity = atmel_hlcdc_pwm_set_polarity,
+	.enable = atmel_hlcdc_pwm_enable,
+	.disable = atmel_hlcdc_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct atmel_hlcdc_pwm *chip;
+	struct atmel_hlcdc *hlcdc;
+	int ret;
+
+	hlcdc = dev_get_drvdata(dev->parent);
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	ret = clk_prepare_enable(hlcdc->periph_clk);
+	if (ret)
+		return ret;
+
+	chip->hlcdc = hlcdc;
+	chip->chip.ops = &atmel_hlcdc_pwm_ops;
+	chip->chip.dev = dev;
+	chip->chip.base = -1;
+	chip->chip.npwm = 1;
+	chip->chip.of_xlate = of_pwm_xlate_with_flags;
+	chip->chip.of_pwm_n_cells = 3;
+	chip->chip.can_sleep = 1;
+
+	ret = pwmchip_add(&chip->chip);
+	if (ret) {
+		clk_disable_unprepare(hlcdc->periph_clk);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+}
+
+static int atmel_hlcdc_pwm_remove(struct platform_device *pdev)
+{
+	struct atmel_hlcdc_pwm *chip = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&chip->chip);
+	if (ret)
+		return ret;
+
+	clk_disable_unprepare(chip->hlcdc->periph_clk);
+
+	return 0;
+}
+
+static const struct of_device_id atmel_hlcdc_pwm_dt_ids[] = {
+	{ .compatible = "atmel,hlcdc-pwm" },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver atmel_hlcdc_pwm_driver = {
+	.driver = {
+		.name = "atmel-hlcdc-pwm",
+		.of_match_table = atmel_hlcdc_pwm_dt_ids,
+	},
+	.probe = atmel_hlcdc_pwm_probe,
+	.remove = atmel_hlcdc_pwm_remove,
+};
+module_platform_driver(atmel_hlcdc_pwm_driver);
+
+MODULE_ALIAS("platform:atmel-hlcdc-pwm");
+MODULE_AUTHOR("Boris Brezillon <boris.brezillon@free-electrons.com>");
+MODULE_DESCRIPTION("Atmel HLCDC PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v8 2/2] pwm: add DT bindings documentation for atmel-hlcdc-pwm driver

[Boris Brezillon]

The HLCDC IP available in some Atmel SoCs (i.e. sam9x5i.e. at91sam9n12,
at91sam9x5 family or sama5d3 family) provide a PWM device.

The DT bindings used for this PWM device is following the default 3 cells
bindings described in Documentation/devicetree/bindings/pwm/pwm.txt.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 .../devicetree/bindings/pwm/atmel-hlcdc-pwm.txt    | 50 ++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt
new file mode 100644
index 0000000..9f4f83a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt
@@ -0,0 +1,50 @@
+Device-Tree bindings for Atmel's HLCDC (High LCD Controller) PWM driver
+
+The Atmel HLCDC PWM is subdevice of the HLCDC MFD device.
+See ../mfd/atmel-hlcdc.txt for more details.
+
+Required properties:
+ - compatible: value should be one of the following:
+   "atmel,hlcdc-pwm"
+ - pinctr-names: the pin control state names. Should contain "default".
+ - pinctrl-0: should contain the pinctrl states described by pinctrl
+   default.
+ - #pwm-cells: should be set to 3. This PWM chip use the default 3 cells
+   bindings defined in Documentation/devicetree/bindings/pwm/pwm.txt.
+
+Example:
+
+	hlcdc: hlcdc@f0030000 {
+		compatible = "atmel,sama5d3-hlcdc";
+		reg = <0xf0030000 0x2000>;
+		clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>;
+		clock-names = "periph_clk","sys_clk", "slow_clk";
+		status = "disabled";
+
+		hlcdc-display-controller {
+			compatible = "atmel,hlcdc-display-controller";
+			interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb888>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0>;
+
+				hlcdc_panel_output: endpoint@0 {
+					reg = <0>;
+					remote-endpoint = <&panel_input>;
+				};
+			};
+		};
+
+		hlcdc_pwm: hlcdc-pwm {
+			compatible = "atmel,hlcdc-pwm";
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_lcd_pwm>;
+			#pwm-cells = <3>;
+		};
+	};
-- 
1.9.1

Re: [PATCH v8 1/2] pwm: add support for atmel-hlcdc-pwm device

[Thierry Reding]

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

On Mon, Oct 06, 2014 at 04:07:02PM +0200, Boris Brezillon wrote:
> The HLCDC IP available in some Atmel SoCs (i.e. sam9x5i.e. at91sam9n12,
> at91sam9x5 family or sama5d3 family) provide a PWM device.
> 
> This driver add support for a PWM chip exposing a single PWM device (which
> will most likely be used to drive a backlight device).
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Tested-by: Anthony Harivel <anthony.harivel@emtrion.de>
> Tested-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>  drivers/pwm/Kconfig           |   9 ++
>  drivers/pwm/Makefile          |   1 +
>  drivers/pwm/pwm-atmel-hlcdc.c | 248 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 258 insertions(+)
>  create mode 100644 drivers/pwm/pwm-atmel-hlcdc.c

Just noticed a couple more things.

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b800783..9bb331b 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -50,6 +50,15 @@ config PWM_ATMEL
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-atmel.
>  
> +config PWM_ATMEL_HLCDC_PWM
> +	tristate "Atmel HLCDC PWM support"
> +	select MFD_ATMEL_HLCDC
> +	help
> +	  Generic PWM framework driver for Atmel HLCDC PWM.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-atmel.

This should be "pwm-atmel-hlcdc".

> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
[...]
> +static int atmel_hlcdc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm)
> +{
> +	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
> +	struct atmel_hlcdc *hlcdc = chip->hlcdc;
> +	u32 status;
> +	int ret;
> +
> +	regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN, ATMEL_HLCDC_PWM);

I just noticed that regmap_write() can also fail. But perhaps that's
only for I2C or the like backends and you can indeed ignore it for MMIO
backends.

> +	do {
> +		usleep_range(1, 10);
> +		ret = regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status);
> +		if (ret)
> +			return ret;
> +	} while ((status & ATMEL_HLCDC_PWM) == 0);

A slightly better loop might be to do the sleep only after you've
determined that the status bit isn't set. That way you avoid a needless
sleep if the status bit is immediately set or an error occurs during
read.

	while (true) {
		ret = regmap_read(...);
		if (ret)
			return ret;

		if (status & ATMEL_HLCDC_PWM)
			break;

		usleep_range(1, 10);
	}

> +static int atmel_hlcdc_pwm_remove(struct platform_device *pdev)
> +{
> +	struct atmel_hlcdc_pwm *chip = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = pwmchip_remove(&chip->chip);
> +	if (ret)
> +		return ret;
> +
> +	clk_disable_unprepare(chip->hlcdc->periph_clk);

You might want to call clk_disable_unprepare() regardless of whether or
not pwmchip_remove() failed. You could simply leave out the above check
for ret and instead...

> +
> +	return 0;

"return ret;" here.

Thierry

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

Re: [PATCH v8 2/2] pwm: add DT bindings documentation for atmel-hlcdc-pwm driver

[Thierry Reding]

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

On Mon, Oct 06, 2014 at 04:07:03PM +0200, Boris Brezillon wrote:
> The HLCDC IP available in some Atmel SoCs (i.e. sam9x5i.e. at91sam9n12,
> at91sam9x5 family or sama5d3 family) provide a PWM device.
> 
> The DT bindings used for this PWM device is following the default 3 cells
> bindings described in Documentation/devicetree/bindings/pwm/pwm.txt.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  .../devicetree/bindings/pwm/atmel-hlcdc-pwm.txt    | 50 ++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt
> new file mode 100644
> index 0000000..9f4f83a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt
> @@ -0,0 +1,50 @@
> +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) PWM driver
> +
> +The Atmel HLCDC PWM is subdevice of the HLCDC MFD device.
> +See ../mfd/atmel-hlcdc.txt for more details.
> +
> +Required properties:
> + - compatible: value should be one of the following:
> +   "atmel,hlcdc-pwm"
> + - pinctr-names: the pin control state names. Should contain "default".
> + - pinctrl-0: should contain the pinctrl states described by pinctrl
> +   default.
> + - #pwm-cells: should be set to 3. This PWM chip use the default 3 cells
> +   bindings defined in Documentation/devicetree/bindings/pwm/pwm.txt.

Maybe "... defined in pwm.txt in this directory."?

> +
> +Example:
> +
> +	hlcdc: hlcdc@f0030000 {
> +		compatible = "atmel,sama5d3-hlcdc";
> +		reg = <0xf0030000 0x2000>;
> +		clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>;
> +		clock-names = "periph_clk","sys_clk", "slow_clk";
> +		status = "disabled";
> +
> +		hlcdc-display-controller {
> +			compatible = "atmel,hlcdc-display-controller";
> +			interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb888>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <0>;
> +
> +				hlcdc_panel_output: endpoint@0 {
> +					reg = <0>;
> +					remote-endpoint = <&panel_input>;
> +				};
> +			};
> +		};

Perhaps leave out the display controller node in this example since it
isn't relevant and could be confusing.

Both of the above are really only minor issues, so either way:

Acked-by: Thierry Reding <thierry.reding@gmail.com>

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

Re: [PATCH v8 2/2] pwm: add DT bindings documentation for atmel-hlcdc-pwm driver

[Boris Brezillon]

On Tue, 7 Oct 2014 10:47:05 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Oct 06, 2014 at 04:07:03PM +0200, Boris Brezillon wrote:
> > The HLCDC IP available in some Atmel SoCs (i.e. sam9x5i.e. at91sam9n12,
> > at91sam9x5 family or sama5d3 family) provide a PWM device.
> > 
> > The DT bindings used for this PWM device is following the default 3 cells
> > bindings described in Documentation/devicetree/bindings/pwm/pwm.txt.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  .../devicetree/bindings/pwm/atmel-hlcdc-pwm.txt    | 50 ++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt
> > new file mode 100644
> > index 0000000..9f4f83a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/atmel-hlcdc-pwm.txt
> > @@ -0,0 +1,50 @@
> > +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) PWM driver
> > +
> > +The Atmel HLCDC PWM is subdevice of the HLCDC MFD device.
> > +See ../mfd/atmel-hlcdc.txt for more details.
> > +
> > +Required properties:
> > + - compatible: value should be one of the following:
> > +   "atmel,hlcdc-pwm"
> > + - pinctr-names: the pin control state names. Should contain "default".
> > + - pinctrl-0: should contain the pinctrl states described by pinctrl
> > +   default.
> > + - #pwm-cells: should be set to 3. This PWM chip use the default 3 cells
> > +   bindings defined in Documentation/devicetree/bindings/pwm/pwm.txt.
> 
> Maybe "... defined in pwm.txt in this directory."?

No problem, I'll change that.

> 
> > +
> > +Example:
> > +
> > +	hlcdc: hlcdc@f0030000 {
> > +		compatible = "atmel,sama5d3-hlcdc";
> > +		reg = <0xf0030000 0x2000>;
> > +		clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>;
> > +		clock-names = "periph_clk","sys_clk", "slow_clk";
> > +		status = "disabled";
> > +
> > +		hlcdc-display-controller {
> > +			compatible = "atmel,hlcdc-display-controller";
> > +			interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>;
> > +			pinctrl-names = "default";
> > +			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb888>;
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			port@0 {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				reg = <0>;
> > +
> > +				hlcdc_panel_output: endpoint@0 {
> > +					reg = <0>;
> > +					remote-endpoint = <&panel_input>;
> > +				};
> > +			};
> > +		};
> 
> Perhaps leave out the display controller node in this example since it
> isn't relevant and could be confusing.

Sure, I'll remove it.

> 
> Both of the above are really only minor issues, so either way:
> 
> Acked-by: Thierry Reding <thierry.reding@gmail.com>



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v8 1/2] pwm: add support for atmel-hlcdc-pwm device

[Boris Brezillon]

On Tue, 7 Oct 2014 10:45:16 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Oct 06, 2014 at 04:07:02PM +0200, Boris Brezillon wrote:
> > The HLCDC IP available in some Atmel SoCs (i.e. sam9x5i.e. at91sam9n12,
> > at91sam9x5 family or sama5d3 family) provide a PWM device.
> > 
> > This driver add support for a PWM chip exposing a single PWM device (which
> > will most likely be used to drive a backlight device).
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Tested-by: Anthony Harivel <anthony.harivel@emtrion.de>
> > Tested-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > Acked-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> >  drivers/pwm/Kconfig           |   9 ++
> >  drivers/pwm/Makefile          |   1 +
> >  drivers/pwm/pwm-atmel-hlcdc.c | 248 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 258 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-atmel-hlcdc.c
> 
> Just noticed a couple more things.
> 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index b800783..9bb331b 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -50,6 +50,15 @@ config PWM_ATMEL
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-atmel.
> >  
> > +config PWM_ATMEL_HLCDC_PWM
> > +	tristate "Atmel HLCDC PWM support"
> > +	select MFD_ATMEL_HLCDC
> > +	help
> > +	  Generic PWM framework driver for Atmel HLCDC PWM.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-atmel.
> 
> This should be "pwm-atmel-hlcdc".

Absolutely, I'll fix that.

> 
> > diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
> [...]
> > +static int atmel_hlcdc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm)
> > +{
> > +	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
> > +	struct atmel_hlcdc *hlcdc = chip->hlcdc;
> > +	u32 status;
> > +	int ret;
> > +
> > +	regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN, ATMEL_HLCDC_PWM);
> 
> I just noticed that regmap_write() can also fail. But perhaps that's
> only for I2C or the like backends and you can indeed ignore it for MMIO
> backends.

Checking for errors is always a good thing (even if in this case,
there's no reason it should fail).

> 
> > +	do {
> > +		usleep_range(1, 10);
> > +		ret = regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status);
> > +		if (ret)
> > +			return ret;
> > +	} while ((status & ATMEL_HLCDC_PWM) == 0);
> 
> A slightly better loop might be to do the sleep only after you've
> determined that the status bit isn't set. That way you avoid a needless
> sleep if the status bit is immediately set or an error occurs during
> read.
> 
> 	while (true) {
> 		ret = regmap_read(...);
> 		if (ret)
> 			return ret;
> 
> 		if (status & ATMEL_HLCDC_PWM)
> 			break;
> 
> 		usleep_range(1, 10);
> 	}

Absolutely, I'll use this code chunk in place of the previous one.

> 
> > +static int atmel_hlcdc_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct atmel_hlcdc_pwm *chip = platform_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	ret = pwmchip_remove(&chip->chip);
> > +	if (ret)
> > +		return ret;
> > +
> > +	clk_disable_unprepare(chip->hlcdc->periph_clk);
> 
> You might want to call clk_disable_unprepare() regardless of whether or
> not pwmchip_remove() failed. You could simply leave out the above check
> for ret and instead...

Are you sure of this one, if pwmchip_remove fails, then the PWM chip
might still be used. And if we disable the clock the PWM chip won't work
anymore.

Best Regards,

Boris



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v8 1/2] pwm: add support for atmel-hlcdc-pwm device

[Thierry Reding]

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

On Tue, Oct 07, 2014 at 11:14:11AM +0200, Boris Brezillon wrote:
> On Tue, 7 Oct 2014 10:45:16 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Mon, Oct 06, 2014 at 04:07:02PM +0200, Boris Brezillon wrote:
[...]
> > > +static int atmel_hlcdc_pwm_remove(struct platform_device *pdev)
> > > +{
> > > +	struct atmel_hlcdc_pwm *chip = platform_get_drvdata(pdev);
> > > +	int ret;
> > > +
> > > +	ret = pwmchip_remove(&chip->chip);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	clk_disable_unprepare(chip->hlcdc->periph_clk);
> > 
> > You might want to call clk_disable_unprepare() regardless of whether or
> > not pwmchip_remove() failed. You could simply leave out the above check
> > for ret and instead...
> 
> Are you sure of this one, if pwmchip_remove fails, then the PWM chip
> might still be used. And if we disable the clock the PWM chip won't work
> anymore.

The return value of .remove() isn't really used, so if built as a module
the PWM chip will disappear anyway. Even if not built as a module the
managed resources are going to go away anyway, so for all intents and
purposes the PWM chip will be defunct.

A long time ago there were some discussions about adding reference
counting to the PWM chip and PWM devices to better handle this situation
but it has never really proven to be an issue thus far.

Perhaps a better way to solve this would be to make pwmchip_remove() not
return an error and instead WARN in the single case where it can fail
(if one of the PWM devices it exposes is still in use). That way users
will still get an appropriate warning that something's awry and it would
play more nicely with an advanced mechanism to keep PWM devices alive
beyond the lifetime of a driver to cope with removal.

Irrespective of the above it's probably fine either way, since if
pwmchip_remove() fails you're in a pretty bad place anyway.

Thierry

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