[PATCH 1/3] pwm: Add mc13xxx pwm driver.

[PATCH 1/3] pwm: Add mc13xxx pwm driver.

[Denis Carikli]

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: devicetree@vger.kernel.org
Cc: linux-pwm@vger.kernel.org
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
 .../devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt    |   19 ++
 drivers/mfd/mc13xxx-core.c                         |   11 ++
 drivers/pwm/Kconfig                                |    6 +
 drivers/pwm/Makefile                               |    1 +
 drivers/pwm/pwm-mc13xxx.c                          |  205 ++++++++++++++++++++
 include/linux/mfd/mc13783.h                        |    2 +
 6 files changed, 244 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt
 create mode 100644 drivers/pwm/pwm-mc13xxx.c

diff --git a/Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt b/Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt
new file mode 100644
index 0000000..a1394d0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt
@@ -0,0 +1,19 @@
+Freescale mc13xxx series PWM drivers.
+
+Supported PWMs:
+mc13892 series
+mc34708 series
+
+Required properties:
+- compatible: "fsl,mc13892-pwm" or "fsl,mc34708-pwm"
+- mfd: phandle to mc13xxx mfd node.
+- #pwm-cells: should be 2. See pwm.txt in this directory for a description of
+  the cells format.
+
+Example:
+
+mc13xxx_pwm: pwm {
+	compatible = "fsl,mc34708-pwm";
+	mfd = <&pmic>;
+	#pwm-cells = <2>;
+};
diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index dbbf8ee..e250f16 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -133,6 +133,8 @@
 
 #define MC13XXX_ADC2		45
 
+struct mc13xxx *mc13xxx_data;
+
 void mc13xxx_lock(struct mc13xxx *mc13xxx)
 {
 	if (!mutex_trylock(&mc13xxx->lock)) {
@@ -639,6 +641,12 @@ static inline int mc13xxx_probe_flags_dt(struct mc13xxx *mc13xxx)
 }
 #endif
 
+struct mc13xxx *get_mc13xxx(void)
+{
+	return mc13xxx_data;
+}
+EXPORT_SYMBOL_GPL(get_mc13xxx);
+
 int mc13xxx_common_init(struct mc13xxx *mc13xxx,
 		struct mc13xxx_platform_data *pdata, int irq)
 {
@@ -706,6 +714,9 @@ err_revision:
 		mc13xxx_add_subdevice(mc13xxx, "%s-pwrbutton");
 	}
 
+	/* Linux will not have to handle more than one mc13xxx pmic. */
+	mc13xxx_data = mc13xxx;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(mc13xxx_common_init);
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index eece329..a959ecd 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -100,6 +100,12 @@ config PWM_LPC32XX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-lpc32xx.
 
+config PWM_MC13XXX
+	tristate "MC13XXX PWM support"
+	depends on MFD_MC13XXX
+	help
+	  Generic PWM framework driver for Freescale MC13XXX pmic.
+
 config PWM_MXS
 	tristate "Freescale MXS PWM support"
 	depends on ARCH_MXS && OF
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 8b754e4..19f67d7 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
+obj-$(CONFIG_PWM_MC13XXX)	+= pwm-mc13xxx.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
diff --git a/drivers/pwm/pwm-mc13xxx.c b/drivers/pwm/pwm-mc13xxx.c
new file mode 100644
index 0000000..e60e4d8
--- /dev/null
+++ b/drivers/pwm/pwm-mc13xxx.c
@@ -0,0 +1,205 @@
+/*
+ * Copyright 2013 Eukréa Electromatique <denis@eukrea.com>
+ *
+ * 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.
+ */
+
+
+#include <linux/err.h>
+#include <linux/mfd/mc13783.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+#define MHz(x) (1000*1000*x)
+
+#define MC13XXX_REG_PWM_CTL		55
+#define MC13XXX_BASE_CLK_FREQ		(MHz(2) / 32)
+#define MC13XXX_PWM1CLKDIV_SHIFT	6
+#define MC13XXX_PWM2DUTY_SHIFT		12
+#define MC13XXX_PWMDUTYDIVISOR		32
+#define MC13XXX_PWMCLKDIVISOR		64
+#define MC13XXX_PWM_REG_SIZE		0x3f
+#define MC13XXX_MIN_PERIOD_NS		(NSEC_PER_SEC / MC13XXX_BASE_CLK_FREQ)
+#define MC13XXX_MAX_PERIOD_NS		(MC13XXX_PWMCLKDIVISOR * MC13XXX_MIN_PERIOD_NS)
+
+struct mc13xxx_pwm_chip {
+	struct pwm_chip pwm_chip;
+	struct mc13xxx *mcdev;
+};
+
+static inline
+struct mc13xxx_pwm_chip *to_mc13xxx_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct mc13xxx_pwm_chip, pwm_chip);
+}
+
+static int pwm_mc13xxx_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			      int duty_ns, int period_ns)
+{
+	struct mc13xxx_pwm_chip *mc13xxx_chip = to_mc13xxx_chip(chip);
+	struct mc13xxx *mcdev = mc13xxx_chip->mcdev;
+	struct device *dev =  mc13xxx_chip->pwm_chip.dev;
+	u32 offset;
+	u32 mask = MC13XXX_PWM_REG_SIZE;
+	u32 val;
+	int ret;
+	int duty_cycle;
+	int min_duty_cycle;
+	int period_cycle;
+	unsigned long period = period_ns;
+
+	dev_dbg(dev, "requested duty_ns=%d and period_ns=%d\n",
+		duty_ns, period_ns);
+
+	/* period */
+	if (period < MC13XXX_MIN_PERIOD_NS) {
+		dev_warn(dev, "period was under the range.\n");
+		period = MC13XXX_MIN_PERIOD_NS;
+	}
+
+	if (period > MC13XXX_MAX_PERIOD_NS) {
+		dev_warn(dev, "period was over the range.\n");
+		period = MC13XXX_MAX_PERIOD_NS;
+	}
+
+	period_cycle = DIV_ROUND_UP(NSEC_PER_SEC, period);
+	period_cycle = DIV_ROUND_UP(MC13XXX_BASE_CLK_FREQ, period_cycle);
+	period_cycle--;
+
+	offset = MC13XXX_REG_PWM_CTL + pwm->hwpwm * MC13XXX_PWM2DUTY_SHIFT +
+		MC13XXX_PWM1CLKDIV_SHIFT;
+
+	mc13xxx_lock(mcdev);
+	ret = mc13xxx_reg_read(mcdev, offset, &val);
+	val &= ~mask;
+	val += (period_cycle & mask);
+	ret = mc13xxx_reg_write(mcdev, offset, val);
+	mc13xxx_unlock(mcdev);
+
+	/* duty cycle */
+	min_duty_cycle = DIV_ROUND_UP(period, 32);
+
+	if (duty_ns < min_duty_cycle) {
+		dev_warn(dev, "duty cycle is under the range.\n");
+		duty_cycle = 0;
+	} else if (duty_ns > period) {
+		dev_warn(dev, "duty cycle is over the range.\n");
+		duty_cycle = 32;
+	} else {
+		duty_cycle = 32 * period;
+		duty_cycle = DIV_ROUND_UP(period, duty_ns);
+		duty_cycle--;
+		duty_cycle = 32 - duty_cycle;
+	}
+
+	offset = MC13XXX_REG_PWM_CTL + pwm->hwpwm * MC13XXX_PWM2DUTY_SHIFT;
+
+	mc13xxx_lock(mcdev);
+	ret = mc13xxx_reg_read(mcdev, offset, &val);
+	val &= ~mask;
+	val += (duty_cycle & mask);
+	ret = mc13xxx_reg_write(mcdev, offset, val);
+	mc13xxx_unlock(mcdev);
+
+	return 0;
+}
+
+static int pwm_mc13xxx_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	/* The hardware doesn't need an enable */
+
+	return 0;
+}
+
+static void pwm_mc13xxx_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	/* The hardware doesn't need a disable */
+}
+
+static const struct pwm_ops pwm_mc13xxx_ops = {
+	.enable		= pwm_mc13xxx_enable,
+	.disable	= pwm_mc13xxx_disable,
+	.config		= pwm_mc13xxx_config,
+	.owner		= THIS_MODULE,
+};
+
+static int pwm_mc13xxx_probe(struct platform_device *pdev)
+{
+	struct mc13xxx_pwm_chip *chip;
+	struct mc13xxx *mcdev;
+	int err;
+
+	mcdev = get_mc13xxx();
+	if (!mcdev) {
+		dev_err(&pdev->dev, "failed to find mc13xxx pmic.\n");
+		return -EINVAL;
+	}
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	chip->pwm_chip.dev = &pdev->dev;
+	chip->pwm_chip.ops = &pwm_mc13xxx_ops;
+	chip->pwm_chip.base = pdev->id;
+	chip->pwm_chip.npwm = 2;
+	chip->mcdev = mcdev;
+
+	err = pwmchip_add(&chip->pwm_chip);
+	if (err < 0)
+		return err;
+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+}
+
+static int pwm_mc13xxx_remove(struct platform_device *pdev)
+{
+	struct mc13xxx_pwm_chip *chip = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&chip->pwm_chip);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id pwm_mc13xxx_of_match[] = {
+	{ .compatible = "fsl,mc13892-pwm" },
+	{ .compatible = "fsl,mc34708-pwm" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pwm_mc13xxx_of_match);
+#endif
+
+static struct platform_driver pwm_mc13xxx_driver = {
+	.driver		= {
+		.name	= "mc13xxx-pwm",
+		.of_match_table = of_match_ptr(pwm_mc13xxx_of_match),
+		.owner	= THIS_MODULE,
+	},
+	.probe		= pwm_mc13xxx_probe,
+	.remove		= pwm_mc13xxx_remove,
+};
+
+module_platform_driver(pwm_mc13xxx_driver);
+
+MODULE_AUTHOR("Denis Carikli <denis@eukrea.com>");
+MODULE_DESCRIPTION("mc13xxx Pulse Width Modulation Driver");
+MODULE_ALIAS("platform:mc13xxx-pwm");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/mc13783.h b/include/linux/mfd/mc13783.h
index a8eeda7..ec934e8 100644
--- a/include/linux/mfd/mc13783.h
+++ b/include/linux/mfd/mc13783.h
@@ -88,4 +88,6 @@
 #define MC13783_IRQ_AHSSHORT	45
 #define MC13783_NUM_IRQ		MC13XXX_NUM_IRQ
 
+struct mc13xxx *get_mc13xxx(void);
+
 #endif /* ifndef __LINUX_MFD_MC13783_H */
-- 
1.7.9.5

[PATCH 2/3] ARM: dts: mbimxsd53 Add backlight and LCD regulator.

[Denis Carikli]

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: devicetree@vger.kernel.org
Cc: linux-pwm@vger.kernel.org
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
 arch/arm/boot/dts/imx53-eukrea-cpuimx53.dtsi       |    7 +++
 .../imx53-eukrea-mbimxsd53-baseboard-cmo-qvga.dts  |   64 ++++++++++++++++++++
 2 files changed, 71 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx53-eukrea-mbimxsd53-baseboard-cmo-qvga.dts

diff --git a/arch/arm/boot/dts/imx53-eukrea-cpuimx53.dtsi b/arch/arm/boot/dts/imx53-eukrea-cpuimx53.dtsi
index cb40cf3..bad044f 100644
--- a/arch/arm/boot/dts/imx53-eukrea-cpuimx53.dtsi
+++ b/arch/arm/boot/dts/imx53-eukrea-cpuimx53.dtsi
@@ -35,6 +35,13 @@
 	pinctrl-0 = <&pinctrl_i2c1>;
 	status = "okay";
 
+	pmic: mc34708@8 {
+		compatible = "fsl,mc34708";
+		reg = <0x8>;
+		interrupt-parent = <&gpio4>;
+		interrupts = <28 0x4>;
+	};
+
 	pcf8563@51 {
 		compatible = "nxp,pcf8563";
 		reg = <0x51>;
diff --git a/arch/arm/boot/dts/imx53-eukrea-mbimxsd53-baseboard-cmo-qvga.dts b/arch/arm/boot/dts/imx53-eukrea-mbimxsd53-baseboard-cmo-qvga.dts
new file mode 100644
index 0000000..b9eceae
--- /dev/null
+++ b/arch/arm/boot/dts/imx53-eukrea-mbimxsd53-baseboard-cmo-qvga.dts
@@ -0,0 +1,64 @@
+/*
+ * Copyright 2013 Eukréa Electromatique <denis@eukrea.com>
+ *
+ * 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.
+ */
+
+#include "imx53-eukrea-mbimxsd53-baseboard.dts"
+
+/ {
+	backlight {
+		compatible = "pwm-backlight";
+		power-supply = <&reg_lvds_backlight_3v3>;
+		pwms = <&mc13xxx_pwm 0 16000>;
+		brightness-levels = <0 8 16 32 64 128 255>;
+		default-brightness-level = <7>;
+	};
+
+	mc13xxx_pwm: pwm {
+		compatible = "fsl,mc34708-pwm";
+		mfd = <&pmic>;
+		#pwm-cells = <2>;
+	};
+
+	reg_lvds_backlight_3v3: lvds-backlight-en {
+		  compatible = "regulator-fixed";
+		  pinctrl-names = "default";
+		  pinctrl-0 = <&pinctrl_reg_lvds_backlight>;
+		  regulator-name = "lvds-3v3";
+		  regulator-min-microvolt = <3300000>;
+		  regulator-max-microvolt = <3300000>;
+		  gpios = <&gpio4 4 0>;
+		  enable-active-high;
+	};
+
+	reg_lcd_3v3: lcd-en {
+		  compatible = "regulator-fixed";
+		  pinctrl-names = "default";
+		  pinctrl-0 = <&pinctrl_reg_lcd_3v3>;
+		  regulator-name = "lcd-3v3";
+		  regulator-min-microvolt = <3300000>;
+		  regulator-max-microvolt = <3300000>;
+		  gpios = <&gpio4 20 0>;
+		  enable-active-high;
+	  };
+};
+
+&iomuxc {
+	imx53-eukrea {
+		pinctrl_reg_lcd_3v3: reg_lcd_3v3grp {
+			fsl,pins = <MX53_PAD_DI0_PIN4__GPIO4_20 0x80000000>;
+		};
+
+		pinctrl_reg_lvds_backlight: reg_lvds_backlightgrp {
+			fsl,pins = <MX53_PAD_GPIO_14__GPIO4_4 0x80000000>;
+		};
+	};
+};
-- 
1.7.9.5

[PATCH 3/3] ARM: imx_v6_v7_defconfig: Enable mc13xxx-pwm support.

[Denis Carikli]

The eukrea mbimx53sd has its display backlighgt
  connected to the pwm of its mc13xxx, so we turn
  it on.

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: devicetree@vger.kernel.org
Cc: linux-pwm@vger.kernel.org
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
 arch/arm/configs/imx_v6_v7_defconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index d479049..4bd1b13 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -241,6 +241,7 @@ CONFIG_COMMON_CLK_DEBUG=y
 # CONFIG_IOMMU_SUPPORT is not set
 CONFIG_PWM=y
 CONFIG_PWM_IMX=y
+CONFIG_PWM_MC13XXX=y
 CONFIG_EXT2_FS=y
 CONFIG_EXT2_FS_XATTR=y
 CONFIG_EXT2_FS_POSIX_ACL=y
-- 
1.7.9.5

Re: [PATCH 1/3] pwm: Add mc13xxx pwm driver.

[Alexander Shiyan]

Hello.

...
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
>  .../devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt    |   19 ++
>  drivers/mfd/mc13xxx-core.c                         |   11 ++
>  drivers/pwm/Kconfig                                |    6 +
>  drivers/pwm/Makefile                               |    1 +
>  drivers/pwm/pwm-mc13xxx.c                          |  205 ++++++++++++++++++++
>  include/linux/mfd/mc13783.h                        |    2 +
>  6 files changed, 244 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt
>  create mode 100644 drivers/pwm/pwm-mc13xxx.c
> 
> diff --git a/Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt b/Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt
> new file mode 100644
> index 0000000..a1394d0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt
> @@ -0,0 +1,19 @@
> +Freescale mc13xxx series PWM drivers.
> +
> +Supported PWMs:
> +mc13892 series
> +mc34708 series

MC13892 does not have a dedicated PWM.
You can probably use the existing driver mc13xxx-led driver.

---

Re: [PATCH 1/3] pwm: Add mc13xxx pwm driver.

[Philippe Rétornaz]

Hi

> +struct mc13xxx *get_mc13xxx(void)
> +{
> +	return mc13xxx_data;
> +}
> +EXPORT_SYMBOL_GPL(get_mc13xxx);
> +
>   int mc13xxx_common_init(struct mc13xxx *mc13xxx,
>   		struct mc13xxx_platform_data *pdata, int irq)
>   {
> @@ -706,6 +714,9 @@ err_revision:
>   		mc13xxx_add_subdevice(mc13xxx, "%s-pwrbutton");
>   	}
>
> +	/* Linux will not have to handle more than one mc13xxx pmic. */
> +	mc13xxx_data = mc13xxx;
> +

Why using a such hack instead of an MFD subdevice ?

Regards,

Philippe

Re: [PATCH 1/3] pwm: Add mc13xxx pwm driver.

[Thierry Reding]

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

On Wed, Nov 27, 2013 at 04:58:33PM +0100, Philippe Rétornaz wrote:
> Hi
> 
> >+struct mc13xxx *get_mc13xxx(void)
> >+{
> >+	return mc13xxx_data;
> >+}
> >+EXPORT_SYMBOL_GPL(get_mc13xxx);
> >+
> >  int mc13xxx_common_init(struct mc13xxx *mc13xxx,
> >  		struct mc13xxx_platform_data *pdata, int irq)
> >  {
> >@@ -706,6 +714,9 @@ err_revision:
> >  		mc13xxx_add_subdevice(mc13xxx, "%s-pwrbutton");
> >  	}
> >
> >+	/* Linux will not have to handle more than one mc13xxx pmic. */
> >+	mc13xxx_data = mc13xxx;
> >+
> 
> Why using a such hack instead of an MFD subdevice ?

I agree, let's not do this please.

Thierry

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

Re: [PATCH 3/3] ARM: imx_v6_v7_defconfig: Enable mc13xxx-pwm support.

[Thierry Reding]

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

On Wed, Nov 27, 2013 at 04:39:31PM +0100, Denis Carikli wrote:
> The eukrea mbimx53sd has its display backlighgt
>   connected to the pwm of its mc13xxx, so we turn
>   it on.

This is weirdly formatted. Also check the spelling: "backlighgt" ->
"backlight", "pwm" -> "PWM", "eukrea" -> "Eukrea".

Also, perhaps: ", so we enable the PWM driver."

Thierry

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

Re: [PATCH 2/3] ARM: dts: mbimxsd53 Add backlight and LCD regulator.

[Thierry Reding]

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

On Wed, Nov 27, 2013 at 04:39:30PM +0100, Denis Carikli wrote:
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-pwm@vger.kernel.org
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
>  arch/arm/boot/dts/imx53-eukrea-cpuimx53.dtsi       |    7 +++
>  .../imx53-eukrea-mbimxsd53-baseboard-cmo-qvga.dts  |   64 ++++++++++++++++++++
>  2 files changed, 71 insertions(+)
>  create mode 100644 arch/arm/boot/dts/imx53-eukrea-mbimxsd53-baseboard-cmo-qvga.dts

This could probably use a more verbose commit description.

> diff --git a/arch/arm/boot/dts/imx53-eukrea-mbimxsd53-baseboard-cmo-qvga.dts b/arch/arm/boot/dts/imx53-eukrea-mbimxsd53-baseboard-cmo-qvga.dts
[...]
> +#include "imx53-eukrea-mbimxsd53-baseboard.dts"
> +
> +/ {
> +	backlight {
> +		compatible = "pwm-backlight";
> +		power-supply = <&reg_lvds_backlight_3v3>;
> +		pwms = <&mc13xxx_pwm 0 16000>;
> +		brightness-levels = <0 8 16 32 64 128 255>;
> +		default-brightness-level = <7>;
> +	};
> +
> +	mc13xxx_pwm: pwm {
> +		compatible = "fsl,mc34708-pwm";
> +		mfd = <&pmic>;
> +		#pwm-cells = <2>;
> +	};
> +
> +	reg_lvds_backlight_3v3: lvds-backlight-en {
> +		  compatible = "regulator-fixed";
> +		  pinctrl-names = "default";
> +		  pinctrl-0 = <&pinctrl_reg_lvds_backlight>;
> +		  regulator-name = "lvds-3v3";
> +		  regulator-min-microvolt = <3300000>;
> +		  regulator-max-microvolt = <3300000>;
> +		  gpios = <&gpio4 4 0>;
> +		  enable-active-high;
> +	};
> +
> +	reg_lcd_3v3: lcd-en {
> +		  compatible = "regulator-fixed";
> +		  pinctrl-names = "default";
> +		  pinctrl-0 = <&pinctrl_reg_lcd_3v3>;
> +		  regulator-name = "lcd-3v3";
> +		  regulator-min-microvolt = <3300000>;
> +		  regulator-max-microvolt = <3300000>;
> +		  gpios = <&gpio4 20 0>;
> +		  enable-active-high;
> +	  };

These both look like purely enable signals, in which case it might be
more appropriate to not use regulators, but rather pass them to the
backlight via the enable-gpios property. At least for the backlight
enable pin.

The LCD enable pin is a slightly different story because you can't
currently wire up panels via DT. However since we should be describing
hardware here that's something I think we'll have to live with for now
and not work around the lack of bindings by tweaking things using
existing bindings.

Also the indentation in the above mixes tabs and spaces.

Thierry

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

Re: [PATCH 1/3] pwm: Add mc13xxx pwm driver.

[Thierry Reding]

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

On Wed, Nov 27, 2013 at 04:39:29PM +0100, Denis Carikli wrote:
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-pwm@vger.kernel.org
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
>  .../devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt    |   19 ++
>  drivers/mfd/mc13xxx-core.c                         |   11 ++
>  drivers/pwm/Kconfig                                |    6 +
>  drivers/pwm/Makefile                               |    1 +
>  drivers/pwm/pwm-mc13xxx.c                          |  205 ++++++++++++++++++++
>  include/linux/mfd/mc13783.h                        |    2 +
>  6 files changed, 244 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt
>  create mode 100644 drivers/pwm/pwm-mc13xxx.c

I'll say the same thing I always say: please use "PWM" in prose instead
of "pwm". It's an abbreviation, so it should be uppercased.

> diff --git a/Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt b/Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt
> new file mode 100644
> index 0000000..a1394d0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/fsl,mc13xxx-pwm.txt
> @@ -0,0 +1,19 @@
> +Freescale mc13xxx series PWM drivers.

The binding doesn't describe drivers, so this should be something like:

	Freescale mc13xxx series PWM controllers

> +Supported PWMs:

Similarily this should be: "Supported PWM controllers:"

> +mc13892 series
> +mc34708 series

And since this is a list it would make sense to prefix each item with a
dash.

> +
> +Required properties:
> +- compatible: "fsl,mc13892-pwm" or "fsl,mc34708-pwm"
> +- mfd: phandle to mc13xxx mfd node.

What's that doing here? If this is a function of the mc13xxx device,
then it should be a child node of the mc13xxx node.

> diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> index dbbf8ee..e250f16 100644
> --- a/drivers/mfd/mc13xxx-core.c
> +++ b/drivers/mfd/mc13xxx-core.c
> @@ -133,6 +133,8 @@
>  
>  #define MC13XXX_ADC2		45
>  
> +struct mc13xxx *mc13xxx_data;
> +
>  void mc13xxx_lock(struct mc13xxx *mc13xxx)
>  {
>  	if (!mutex_trylock(&mc13xxx->lock)) {
> @@ -639,6 +641,12 @@ static inline int mc13xxx_probe_flags_dt(struct mc13xxx *mc13xxx)
>  }
>  #endif
>  
> +struct mc13xxx *get_mc13xxx(void)
> +{
> +	return mc13xxx_data;
> +}
> +EXPORT_SYMBOL_GPL(get_mc13xxx);

As mentioned elsewhere, this is horrible.

>  int mc13xxx_common_init(struct mc13xxx *mc13xxx,
>  		struct mc13xxx_platform_data *pdata, int irq)
>  {
> @@ -706,6 +714,9 @@ err_revision:
>  		mc13xxx_add_subdevice(mc13xxx, "%s-pwrbutton");
>  	}
>  
> +	/* Linux will not have to handle more than one mc13xxx pmic. */
> +	mc13xxx_data = mc13xxx;

Who says that Linux will never have to handle more than one? Even if you
could guarantee that, it still sets a bad example.

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index eece329..a959ecd 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -100,6 +100,12 @@ config PWM_LPC32XX
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-lpc32xx.
>  
> +config PWM_MC13XXX
> +	tristate "MC13XXX PWM support"
> +	depends on MFD_MC13XXX
> +	help
> +	  Generic PWM framework driver for Freescale MC13XXX pmic.

PMIC is an abbreviation too, so should be all uppercase.

> diff --git a/drivers/pwm/pwm-mc13xxx.c b/drivers/pwm/pwm-mc13xxx.c
[...]
> +#include <linux/err.h>
> +#include <linux/mfd/mc13783.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>

Can this please be sorted alphabetically?

> +
> +#define MHz(x) (1000*1000*x)

Please drop this. You use it exactly once, ...

> +#define MC13XXX_REG_PWM_CTL		55
> +#define MC13XXX_BASE_CLK_FREQ		(MHz(2) / 32)

So you can just as well write (2000000 / 32) here.

> +#define MC13XXX_PWM1CLKDIV_SHIFT	6
> +#define MC13XXX_PWM2DUTY_SHIFT		12
> +#define MC13XXX_PWMDUTYDIVISOR		32
> +#define MC13XXX_PWMCLKDIVISOR		64

This list is totally confusing. I can't tell which of these is a
register and which isn't. I would at least expect registers to have
hexadecimal offsets. Better yet, a comment should explain what the
registers look like.

> +#define MC13XXX_PWM_REG_SIZE		0x3f

This doesn't seem to be a size at all. It's used as a mask, so why not
call it MC13XXX_PWM_REG_MASK?

> +static int pwm_mc13xxx_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      int duty_ns, int period_ns)
> +{
> +	struct mc13xxx_pwm_chip *mc13xxx_chip = to_mc13xxx_chip(chip);
> +	struct mc13xxx *mcdev = mc13xxx_chip->mcdev;
> +	struct device *dev =  mc13xxx_chip->pwm_chip.dev;
> +	u32 offset;
> +	u32 mask = MC13XXX_PWM_REG_SIZE;
> +	u32 val;
> +	int ret;
> +	int duty_cycle;
> +	int min_duty_cycle;
> +	int period_cycle;
> +	unsigned long period = period_ns;

Many of these can go on a single line. Also it looks like you can reuse
some of them and thereby reduce the number of local variables. Having
too many of them makes it very difficult to follow what the code is
doing.

> +
> +	dev_dbg(dev, "requested duty_ns=%d and period_ns=%d\n",
> +		duty_ns, period_ns);

This can go away.

> +
> +	/* period */
> +	if (period < MC13XXX_MIN_PERIOD_NS) {
> +		dev_warn(dev, "period was under the range.\n");
> +		period = MC13XXX_MIN_PERIOD_NS;
> +	}
> +
> +	if (period > MC13XXX_MAX_PERIOD_NS) {
> +		dev_warn(dev, "period was over the range.\n");
> +		period = MC13XXX_MAX_PERIOD_NS;
> +	}

Shouldn't both of these be an error instead?

> +
> +	period_cycle = DIV_ROUND_UP(NSEC_PER_SEC, period);
> +	period_cycle = DIV_ROUND_UP(MC13XXX_BASE_CLK_FREQ, period_cycle);
> +	period_cycle--;

You can save one line here by turning this one into a - 1 appended to
the previous line.

> +
> +	offset = MC13XXX_REG_PWM_CTL + pwm->hwpwm * MC13XXX_PWM2DUTY_SHIFT +
> +		MC13XXX_PWM1CLKDIV_SHIFT;

_SHIFT is usually for bitfields, not for register offsets. Perhaps it
would also be a good idea to move this computation into a macro or
static inline function, something like:

	#define MC13XXX_PWM_BASE(pwm) (0x37 + ((pwm) * 0xc))

And then use offsets to the individual registers:

	#define MC13XXX_PWM_CTL 0x00
	#define MC13XXX_PWM_CLKDIV 0x06

Then it becomes much easier to use these:

	unsigned int base = MC13XXX_PWM_BASE(pwm->hwpwm);

	ret = mc13xxx_reg_read(mcdev, base + MC13XXX_PWM_CTL, &val);

> +
> +	mc13xxx_lock(mcdev);
> +	ret = mc13xxx_reg_read(mcdev, offset, &val);
> +	val &= ~mask;

Why aren't you using the #define here directly?

> +	val += (period_cycle & mask);
> +	ret = mc13xxx_reg_write(mcdev, offset, val);
> +	mc13xxx_unlock(mcdev);
> +
> +	/* duty cycle */
> +	min_duty_cycle = DIV_ROUND_UP(period, 32);
> +
> +	if (duty_ns < min_duty_cycle) {
> +		dev_warn(dev, "duty cycle is under the range.\n");
> +		duty_cycle = 0;

Again, I think this should be an error and the function should fail when
this happens.

> +	} else if (duty_ns > period) {

The core code already checks for this.

> +		dev_warn(dev, "duty cycle is over the range.\n");
> +		duty_cycle = 32;
> +	} else {
> +		duty_cycle = 32 * period;
> +		duty_cycle = DIV_ROUND_UP(period, duty_ns);

Are you sure this is doing what it's supposed to? The second line
overwrites the first one. Also

Do you have a link to the manual for this controller? I'm very confused
by all these restrictions and the computations.

> +		duty_cycle--;
> +		duty_cycle = 32 - duty_cycle;
> +	}
> +
> +	offset = MC13XXX_REG_PWM_CTL + pwm->hwpwm * MC13XXX_PWM2DUTY_SHIFT;
> +
> +	mc13xxx_lock(mcdev);
> +	ret = mc13xxx_reg_read(mcdev, offset, &val);
> +	val &= ~mask;
> +	val += (duty_cycle & mask);

That's odd. The mask here is 0x3f (63), but the maximum value for the
duty-cycle is 32 by the above computations, so masking is completely
unnecessary.

Also, you don't need any parentheses around (duty_cycle & mask). And
since you're modifying a bitfield, the idiomatic way to write it is with
a | instead of a +:

	val &= ~mask;
	val |= duty_cycle & mask;

> +	ret = mc13xxx_reg_write(mcdev, offset, val);
> +	mc13xxx_unlock(mcdev);

Shouldn't the lock protect both register writes as a whole? Otherwise
there's still the potential that two such accesses will be interleaved
and cause undefined behaviour.

> +	return 0;

This returns success no matter what mc13xxx_reg_write() returned.

> +}
> +
> +static int pwm_mc13xxx_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	/* The hardware doesn't need an enable */
> +
> +	return 0;
> +}

That's not really compatible with the semantics of the PWM subsystem. It
must be possible to configure the PWM without enabling it. If the
hardware doesn't have a separate bit to do that, then you'll need to put
most of the .config() code into .enable(). You could for example compute
the register values in advance, in .config(), but only write the
registers in .enable().

> +static void pwm_mc13xxx_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	/* The hardware doesn't need a disable */
> +}

That's unfortunate. So the only way to "disable" the PWM is by setting
the duty-cycle to 0? In that case it might be good to do that here.

> +
> +static const struct pwm_ops pwm_mc13xxx_ops = {
> +	.enable		= pwm_mc13xxx_enable,
> +	.disable	= pwm_mc13xxx_disable,
> +	.config		= pwm_mc13xxx_config,
> +	.owner		= THIS_MODULE,
> +};

No tabs for aligning please, just use a single space on either side of
the assignment operator.

> +static int pwm_mc13xxx_probe(struct platform_device *pdev)
> +{
> +	struct mc13xxx_pwm_chip *chip;
> +	struct mc13xxx *mcdev;
> +	int err;
> +
> +	mcdev = get_mc13xxx();

I've mentioned this before, this needs to go away. The usual way to do
that is to make the PWM device a child of the MFD device and refer to
the MFD device via pdev->dev.parent.

> +	if (!mcdev) {
> +		dev_err(&pdev->dev, "failed to find mc13xxx pmic.\n");
> +		return -EINVAL;
> +	}
> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (chip == NULL) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}

Don't use error messages for allocation failures. And while at it, can
you change "chip == NULL" to "!chip", please?

> +
> +	chip->pwm_chip.dev = &pdev->dev;
> +	chip->pwm_chip.ops = &pwm_mc13xxx_ops;
> +	chip->pwm_chip.base = pdev->id;

No. This should always be -1 for new drivers.

> +static int pwm_mc13xxx_remove(struct platform_device *pdev)
> +{
> +	struct mc13xxx_pwm_chip *chip = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = pwmchip_remove(&chip->pwm_chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

This can be just:

	return pwmchip_remove(&chip->pwm_chip);

By the way, that pwm_ prefix on the PWM chip field is somewhat redundant
since the driver implements only one chip. I suggest you drop it.

> +#ifdef CONFIG_OF
> +static const struct of_device_id pwm_mc13xxx_of_match[] = {
> +	{ .compatible = "fsl,mc13892-pwm" },
> +	{ .compatible = "fsl,mc34708-pwm" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, pwm_mc13xxx_of_match);
> +#endif
> +
> +static struct platform_driver pwm_mc13xxx_driver = {
> +	.driver		= {
> +		.name	= "mc13xxx-pwm",
> +		.of_match_table = of_match_ptr(pwm_mc13xxx_of_match),
> +		.owner	= THIS_MODULE,

.owner is assigned automatically by the core, so this can be dropped.

> +	},
> +	.probe		= pwm_mc13xxx_probe,
> +	.remove		= pwm_mc13xxx_remove,

Please don't use tabs for alignment here. A single space on each side of
the = will do just fine. For the whole structure, not just these two
fields.

> +};
> +
> +module_platform_driver(pwm_mc13xxx_driver);

No blank line between the above two lines.

Thierry

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