[PATCH v5 0/3] Add MediaTek display PWM driver

[PATCH v5 0/3] Add MediaTek display PWM driver

[YH Huang]

This patch series add the use of display PWM driver, documentation 
and device tree for Mediatek SoCs. The driver is used to support 
the backlight of the panel. This is based on v4.2-rc1.

The clock definitions (CLK_MM_DISP_PWM*) are added by James Liao's patch: 
clk: mediatek: Add subsystem clocks of MT8173

Change in v5:
1. Configure PWM output via pinctrl
2. Fix the parameter name in dtsi

YH Huang (3):
  dt-bindings: pwm: add MediaTek display PWM bindings
  pwm: add MediaTek display PWM driver support
  arm64: dts: mt8173: add MT8173 display PWM driver support node

 .../devicetree/bindings/pwm/pwm-mtk-disp.txt       |  29 +++
 arch/arm64/boot/dts/mediatek/mt8173-evb.dts        |  15 ++
 arch/arm64/boot/dts/mediatek/mt8173.dtsi           |  22 ++
 drivers/pwm/Kconfig                                |  10 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-mtk-disp.c                         | 256 +++++++++++++++++++++
 6 files changed, 333 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
 create mode 100644 drivers/pwm/pwm-mtk-disp.c

--
1.8.1.1.dirty

[PATCH v5 1/3] dt-bindings: pwm: add MediaTek display PWM bindings

[YH Huang]

Document the device-tree binding of MediatTek display PWM.
The PWM has one channel to control the backlight brightness for display.
It supports MT8173 and MT6595.

Signed-off-by: YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 .../devicetree/bindings/pwm/pwm-mtk-disp.txt       | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
new file mode 100644
index 0000000..aac29dc
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
@@ -0,0 +1,29 @@
+MediaTek display PWM controller
+
+Required properties:
+ - compatible: should be "mediatek,<name>-disp-pwm":
+   - "mediatek,mt8173-disp-pwm": found on mt8173 SoC.
+   - "mediatek,mt6595-disp-pwm": found on mt6595 SoC.
+ - reg: physical base address and length of the controller's registers.
+ - #pwm-cells: must be 2. See pwm.txt in this directory for a description of
+   the cell format.
+ - clocks: phandle and clock specifier of the PWM reference clock.
+ - clock-names: must contain the following:
+   - "main": clock used to generate PWM signals.
+   - "mm": sync signals from the modules of mmsys.
+ - pinctrl-names: Must contain a "default" entry.
+ - pinctrl-0: One property must exist for each entry in pinctrl-names.
+   See pinctrl/pinctrl-bindings.txt for details of the property values.
+
+Example:
+	pwm0: pwm@1401e000 {
+		compatible = "mediatek,mt8173-disp-pwm",
+			     "mediatek,mt6595-disp-pwm";
+		reg = <0 0x1401e000 0 0x1000>;
+		#pwm-cells = <2>;
+		clocks = <&mmsys CLK_MM_DISP_PWM026M>,
+			 <&mmsys CLK_MM_DISP_PWM0MM>;
+		clock-names = "main", "mm";
+		pinctrl-names = "default";
+		pinctrl-0 = <&disp_pwm0_pins>;
+	};
-- 
1.8.1.1.dirty

Re: [PATCH v5 1/3] dt-bindings: pwm: add MediaTek display PWM bindings

[Daniel Kurtz]

On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang@mediatek.com> wrote:
> Document the device-tree binding of MediatTek display PWM.
> The PWM has one channel to control the backlight brightness for display.
> It supports MT8173 and MT6595.
>
> Signed-off-by: YH Huang <yh.huang@mediatek.com>
> ---
>  .../devicetree/bindings/pwm/pwm-mtk-disp.txt       | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> new file mode 100644
> index 0000000..aac29dc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> @@ -0,0 +1,29 @@
> +MediaTek display PWM controller
> +
> +Required properties:
> + - compatible: should be "mediatek,<name>-disp-pwm":
> +   - "mediatek,mt8173-disp-pwm": found on mt8173 SoC.
> +   - "mediatek,mt6595-disp-pwm": found on mt6595 SoC.
> + - reg: physical base address and length of the controller's registers.
> + - #pwm-cells: must be 2. See pwm.txt in this directory for a description of
> +   the cell format.
> + - clocks: phandle and clock specifier of the PWM reference clock.
> + - clock-names: must contain the following:
> +   - "main": clock used to generate PWM signals.
> +   - "mm": sync signals from the modules of mmsys.
> + - pinctrl-names: Must contain a "default" entry.
> + - pinctrl-0: One property must exist for each entry in pinctrl-names.
> +   See pinctrl/pinctrl-bindings.txt for details of the property values.
> +
> +Example:
> +       pwm0: pwm@1401e000 {
> +               compatible = "mediatek,mt8173-disp-pwm",
> +                            "mediatek,mt6595-disp-pwm";
> +               reg = <0 0x1401e000 0 0x1000>;
> +               #pwm-cells = <2>;
> +               clocks = <&mmsys CLK_MM_DISP_PWM026M>,
> +                        <&mmsys CLK_MM_DISP_PWM0MM>;
> +               clock-names = "main", "mm";
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&disp_pwm0_pins>;
> +       };

Please show an example consumer of the pwm phandle to show how to set
the two properties required by the #pwm-cells.
Although the pwm-specifier typically encodes the chip-relative PWM
number and the PWM period in nanoseconds, it is technically controller
specific.

In fact, since the mtk-disp-pwm does not have a chip-relative PWM
number, could we in fact set #pwm-cells = <1>, and only specify the
requested PWM period?

-Dan


> --
> 1.8.1.1.dirty
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v5 1/3] dt-bindings: pwm: add MediaTek display PWM bindings

[YH Huang]

On Mon, 2015-07-13 at 18:20 +0800, Daniel Kurtz wrote:
> On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> > Document the device-tree binding of MediatTek display PWM.
> > The PWM has one channel to control the backlight brightness for display.
> > It supports MT8173 and MT6595.
> >
> > Signed-off-by: YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> >  .../devicetree/bindings/pwm/pwm-mtk-disp.txt       | 29 ++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> > new file mode 100644
> > index 0000000..aac29dc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> > @@ -0,0 +1,29 @@
> > +MediaTek display PWM controller
> > +
> > +Required properties:
> > + - compatible: should be "mediatek,<name>-disp-pwm":
> > +   - "mediatek,mt8173-disp-pwm": found on mt8173 SoC.
> > +   - "mediatek,mt6595-disp-pwm": found on mt6595 SoC.
> > + - reg: physical base address and length of the controller's registers.
> > + - #pwm-cells: must be 2. See pwm.txt in this directory for a description of
> > +   the cell format.
> > + - clocks: phandle and clock specifier of the PWM reference clock.
> > + - clock-names: must contain the following:
> > +   - "main": clock used to generate PWM signals.
> > +   - "mm": sync signals from the modules of mmsys.
> > + - pinctrl-names: Must contain a "default" entry.
> > + - pinctrl-0: One property must exist for each entry in pinctrl-names.
> > +   See pinctrl/pinctrl-bindings.txt for details of the property values.
> > +
> > +Example:
> > +       pwm0: pwm@1401e000 {
> > +               compatible = "mediatek,mt8173-disp-pwm",
> > +                            "mediatek,mt6595-disp-pwm";
> > +               reg = <0 0x1401e000 0 0x1000>;
> > +               #pwm-cells = <2>;
> > +               clocks = <&mmsys CLK_MM_DISP_PWM026M>,
> > +                        <&mmsys CLK_MM_DISP_PWM0MM>;
> > +               clock-names = "main", "mm";
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&disp_pwm0_pins>;
> > +       };
> 
> Please show an example consumer of the pwm phandle to show how to set
> the two properties required by the #pwm-cells.
> Although the pwm-specifier typically encodes the chip-relative PWM
> number and the PWM period in nanoseconds, it is technically controller
> specific.
> 
> In fact, since the mtk-disp-pwm does not have a chip-relative PWM
> number, could we in fact set #pwm-cells = <1>, and only specify the
> requested PWM period?
OK. I will add the example like the following.

backlight_lcd: backlight_lcd {
	compatible = "pwm-backlight";
	pwms = <&pwm0 0 1000000>;
	brightness-levels = <
		  0  16  32  48  64  80  96 112
		128 144 160 176 192 208 224 240
		255
	>;
	default-brightness-level = <9>;
	power-supply = <&mt6397_vio18_reg>;
	enable-gpios = <&pio 95 GPIO_ACTIVE_HIGH>;
};

The core in PWM driver can only take two or three cells.
In our case, we don't need to set polarity of PWM.
So it should be 2.

Regards,
YH Huang

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[YH Huang]

Add display PWM driver support to modify backlight for MT8173 and MT6595.
The PWM has one channel to control the brightness of the display.
When the (high_width / period) is closer to 1, the screen is brighter;
otherwise, it is darker.

Signed-off-by: YH Huang <yh.huang@mediatek.com>
---
 drivers/pwm/Kconfig        |  10 ++
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-mtk-disp.c | 256 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 267 insertions(+)
 create mode 100644 drivers/pwm/pwm-mtk-disp.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b1541f4..f5b03a4 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -211,6 +211,16 @@ config PWM_LPSS_PLATFORM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-lpss-platform.
 
+config PWM_MTK_DISP
+	tristate "MediaTek display PWM driver"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	help
+	  Generic PWM framework driver for MediaTek disp-pwm device.
+	  The PWM is used to control the backlight brightness for display.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-mtk-disp.
+
 config PWM_MXS
 	tristate "Freescale MXS PWM support"
 	depends on ARCH_MXS && OF
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index ec50eb5..99c9e75 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
 obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
 obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
+obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.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-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
new file mode 100644
index 0000000..1f17cee
--- /dev/null
+++ b/drivers/pwm/pwm-mtk-disp.c
@@ -0,0 +1,256 @@
+/*
+ * MediaTek display pulse-width-modulation controller driver.
+ * Copyright (c) 2015 MediaTek Inc.
+ * Author: YH Huang <yh.huang@mediatek.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.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pwm.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define DISP_PWM_EN		0
+#define PWM_ENABLE_MASK		BIT(0)
+
+#define DISP_PWM_COMMIT		BIT(3)
+#define PWM_COMMIT_MASK		BIT(0)
+
+#define DISP_PWM_CON_0		BIT(4)
+#define PWM_CLKDIV_SHIFT	16
+#define PWM_CLKDIV_MAX		0x3ff
+#define PWM_CLKDIV_MASK		(PWM_CLKDIV_MAX << PWM_CLKDIV_SHIFT)
+
+#define DISP_PWM_CON_1		0x14
+#define PWM_PERIOD_MASK		0xfff
+/* Shift log2(PWM_PERIOD_MASK + 1) as divisor */
+#define PWM_PERIOD_BIT_SHIFT	12
+
+#define PWM_HIGH_WIDTH_SHIFT	16
+#define PWM_HIGH_WIDTH_MASK	(0x1fff << PWM_HIGH_WIDTH_SHIFT)
+
+struct mtk_disp_pwm {
+	struct pwm_chip chip;
+	struct device *dev;
+	struct clk *clk_main;
+	struct clk *clk_mm;
+	void __iomem *base;
+};
+
+static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct mtk_disp_pwm, chip);
+}
+
+static void mtk_disp_pwm_update_bits(void __iomem *address, u32 mask, u32 value)
+{
+	u32 val;
+
+	val = readl(address);
+	val &= ~mask;
+	val |= value;
+	writel(val, address);
+}
+
+static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			       int duty_ns, int period_ns)
+{
+	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
+	u64 div, rate;
+	u32 clk_div, period, high_width, value;
+
+	/*
+	 * Find period, high_width and clk_div to suit duty_ns and period_ns.
+	 * Calculate proper div value to keep period value in the bound.
+	 *
+	 * period_ns = 10^9 * (clk_div + 1) * (period + 1) / PWM_CLK_RATE
+	 * duty_ns = 10^9 * (clk_div + 1) * high_width / PWM_CLK_RATE
+	 *
+	 * period = (PWM_CLK_RATE * period_ns) / (10^9 * (clk_div + 1)) - 1
+	 * high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
+	 */
+	rate = clk_get_rate(mdp->clk_main);
+	clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
+			  PWM_PERIOD_BIT_SHIFT;
+	if (clk_div > PWM_CLKDIV_MAX)
+		return -EINVAL;
+
+	div = NSEC_PER_SEC * (clk_div + 1);
+	period = div64_u64(rate * period_ns, div);
+	if (period > 0)
+		period--;
+
+	high_width = div64_u64(rate * duty_ns, div);
+
+	mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_0,
+				 PWM_CLKDIV_MASK, clk_div << PWM_CLKDIV_SHIFT);
+
+	value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
+	mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_1,
+				 PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
+
+	mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
+				 PWM_COMMIT_MASK, 1);
+	mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
+				 PWM_COMMIT_MASK, 0);
+
+	return 0;
+}
+
+static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
+
+	mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
+				 PWM_ENABLE_MASK, 1);
+
+	return 0;
+}
+
+static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
+
+	mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
+				 PWM_ENABLE_MASK, 0);
+}
+
+static const struct pwm_ops mtk_disp_pwm_ops = {
+	.config = mtk_disp_pwm_config,
+	.enable = mtk_disp_pwm_enable,
+	.disable = mtk_disp_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int mtk_disp_pwm_probe(struct platform_device *pdev)
+{
+	struct mtk_disp_pwm *mdp;
+	struct resource *r;
+	int ret;
+
+	mdp = devm_kzalloc(&pdev->dev, sizeof(*mdp), GFP_KERNEL);
+	if (!mdp)
+		return -ENOMEM;
+
+	mdp->dev = &pdev->dev;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mdp->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(mdp->base))
+		return PTR_ERR(mdp->base);
+
+	mdp->clk_main = devm_clk_get(&pdev->dev, "main");
+	if (IS_ERR(mdp->clk_main))
+		return PTR_ERR(mdp->clk_main);
+
+	mdp->clk_mm = devm_clk_get(&pdev->dev, "mm");
+	if (IS_ERR(mdp->clk_mm))
+		return PTR_ERR(mdp->clk_mm);
+
+	ret = clk_prepare_enable(mdp->clk_main);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_prepare_enable(mdp->clk_mm);
+	if (ret < 0)
+		goto disable_clk_main;
+
+	platform_set_drvdata(pdev, mdp);
+
+	mdp->chip.dev = &pdev->dev;
+	mdp->chip.ops = &mtk_disp_pwm_ops;
+	mdp->chip.base = -1;
+	mdp->chip.npwm = 1;
+
+	ret = pwmchip_add(&mdp->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+		goto disable_clk_mm;
+	}
+
+	return 0;
+
+disable_clk_mm:
+	clk_disable_unprepare(mdp->clk_mm);
+disable_clk_main:
+	clk_disable_unprepare(mdp->clk_main);
+	return ret;
+}
+
+static int mtk_disp_pwm_remove(struct platform_device *pdev)
+{
+	struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev);
+	int ret = pwmchip_remove(&mdp->chip);
+
+	clk_disable_unprepare(mdp->clk_main);
+	clk_disable_unprepare(mdp->clk_mm);
+
+	return ret;
+}
+
+static const struct of_device_id mtk_disp_pwm_of_match[] = {
+	{ .compatible = "mediatek,mt8173-disp-pwm" },
+	{ .compatible = "mediatek,mt6595-disp-pwm" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mtk_disp_pwm_of_match);
+
+#ifdef CONFIG_PM_SLEEP
+static int mtk_disp_pwm_suspend(struct device *dev)
+{
+	struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(mdp->clk_main);
+	clk_disable_unprepare(mdp->clk_mm);
+
+	return 0;
+}
+
+static int mtk_disp_pwm_resume(struct device *dev)
+{
+	struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(mdp->clk_main);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_prepare_enable(mdp->clk_mm);
+	if (ret < 0) {
+		clk_disable_unprepare(mdp->clk_main);
+		return ret;
+	}
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mtk_disp_pwm_pm_ops, mtk_disp_pwm_suspend,
+			 mtk_disp_pwm_resume);
+
+static struct platform_driver mtk_disp_pwm_driver = {
+	.driver = {
+		.name = "mediatek-disp-pwm",
+		.pm = &mtk_disp_pwm_pm_ops,
+		.of_match_table = mtk_disp_pwm_of_match,
+	},
+	.probe = mtk_disp_pwm_probe,
+	.remove = mtk_disp_pwm_remove,
+};
+module_platform_driver(mtk_disp_pwm_driver);
+
+MODULE_AUTHOR("YH Huang <yh.huang@mediatek.com>");
+MODULE_DESCRIPTION("MediaTek SoC display PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.1.1.dirty

Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[Daniel Kurtz]

On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang@mediatek.com> wrote:
> Add display PWM driver support to modify backlight for MT8173 and MT6595.
> The PWM has one channel to control the brightness of the display.
> When the (high_width / period) is closer to 1, the screen is brighter;
> otherwise, it is darker.
>
> Signed-off-by: YH Huang <yh.huang@mediatek.com>
> ---
>  drivers/pwm/Kconfig        |  10 ++
>  drivers/pwm/Makefile       |   1 +
>  drivers/pwm/pwm-mtk-disp.c | 256 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 267 insertions(+)
>  create mode 100644 drivers/pwm/pwm-mtk-disp.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f4..f5b03a4 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -211,6 +211,16 @@ config PWM_LPSS_PLATFORM
>           To compile this driver as a module, choose M here: the module
>           will be called pwm-lpss-platform.
>
> +config PWM_MTK_DISP
> +       tristate "MediaTek display PWM driver"
> +       depends on ARCH_MEDIATEK || COMPILE_TEST
> +       help
> +         Generic PWM framework driver for MediaTek disp-pwm device.
> +         The PWM is used to control the backlight brightness for display.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called pwm-mtk-disp.
> +
>  config PWM_MXS
>         tristate "Freescale MXS PWM support"
>         depends on ARCH_MXS && OF
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ec50eb5..99c9e75 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_LPC32XX)     += pwm-lpc32xx.o
>  obj-$(CONFIG_PWM_LPSS)         += pwm-lpss.o
>  obj-$(CONFIG_PWM_LPSS_PCI)     += pwm-lpss-pci.o
>  obj-$(CONFIG_PWM_LPSS_PLATFORM)        += pwm-lpss-platform.o
> +obj-$(CONFIG_PWM_MTK_DISP)     += pwm-mtk-disp.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-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> new file mode 100644
> index 0000000..1f17cee
> --- /dev/null
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -0,0 +1,256 @@
> +/*
> + * MediaTek display pulse-width-modulation controller driver.
> + * Copyright (c) 2015 MediaTek Inc.
> + * Author: YH Huang <yh.huang@mediatek.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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pwm.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define DISP_PWM_EN            0

The "DISP_PWM_*" are register offsets, so use a hex value, like this:

#define DISP_PWM_EN                      0x00

Use BIT() for register *fields*, that is, the individual bits of a register.

> +#define PWM_ENABLE_MASK                BIT(0)
> +
> +#define DISP_PWM_COMMIT                BIT(3)

#define DISP_PWM_COMMIT            0x08

> +#define PWM_COMMIT_MASK                BIT(0)
> +
> +#define DISP_PWM_CON_0         BIT(4)

#define DISP_PWM_COMMIT            0x10

> +#define PWM_CLKDIV_SHIFT       16
> +#define PWM_CLKDIV_MAX         0x3ff
> +#define PWM_CLKDIV_MASK                (PWM_CLKDIV_MAX << PWM_CLKDIV_SHIFT)
> +
> +#define DISP_PWM_CON_1         0x14
> +#define PWM_PERIOD_MASK                0xfff
> +/* Shift log2(PWM_PERIOD_MASK + 1) as divisor */
> +#define PWM_PERIOD_BIT_SHIFT   12
> +
> +#define PWM_HIGH_WIDTH_SHIFT   16
> +#define PWM_HIGH_WIDTH_MASK    (0x1fff << PWM_HIGH_WIDTH_SHIFT)
> +
> +struct mtk_disp_pwm {
> +       struct pwm_chip chip;
> +       struct device *dev;

I don't think "dev" is actually used.  And, if needed, it can be
extracted from "chip".

> +       struct clk *clk_main;
> +       struct clk *clk_mm;
> +       void __iomem *base;
> +};
> +
> +static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)
> +{
> +       return container_of(chip, struct mtk_disp_pwm, chip);
> +}
> +
> +static void mtk_disp_pwm_update_bits(void __iomem *address, u32 mask, u32 value)

Take "struct mtk_disp_pwm *mdp" as a param and extract mdp->base,
rather than pass the raw iomem address.

> +{
> +       u32 val;
> +
> +       val = readl(address);
> +       val &= ~mask;
> +       val |= value;
> +       writel(val, address);
> +}
> +
> +static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +                              int duty_ns, int period_ns)
> +{
> +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> +       u64 div, rate;
> +       u32 clk_div, period, high_width, value;
> +
> +       /*
> +        * Find period, high_width and clk_div to suit duty_ns and period_ns.
> +        * Calculate proper div value to keep period value in the bound.
> +        *
> +        * period_ns = 10^9 * (clk_div + 1) * (period + 1) / PWM_CLK_RATE
> +        * duty_ns = 10^9 * (clk_div + 1) * high_width / PWM_CLK_RATE
> +        *
> +        * period = (PWM_CLK_RATE * period_ns) / (10^9 * (clk_div + 1)) - 1
> +        * high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
> +        */
> +       rate = clk_get_rate(mdp->clk_main);
> +       clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
> +                         PWM_PERIOD_BIT_SHIFT;
> +       if (clk_div > PWM_CLKDIV_MAX)
> +               return -EINVAL;
> +
> +       div = NSEC_PER_SEC * (clk_div + 1);
> +       period = div64_u64(rate * period_ns, div);
> +       if (period > 0)
> +               period--;
> +
> +       high_width = div64_u64(rate * duty_ns, div);
> +
> +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_0,
> +                                PWM_CLKDIV_MASK, clk_div << PWM_CLKDIV_SHIFT);
> +
> +       value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
> +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_1,
> +                                PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
> +
> +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
> +                                PWM_COMMIT_MASK, 1);
> +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
> +                                PWM_COMMIT_MASK, 0);
> +
> +       return 0;
> +}
> +
> +static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> +
> +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
> +                                PWM_ENABLE_MASK, 1);
> +
> +       return 0;
> +}
> +
> +static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> +
> +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
> +                                PWM_ENABLE_MASK, 0);
> +}
> +
> +static const struct pwm_ops mtk_disp_pwm_ops = {
> +       .config = mtk_disp_pwm_config,
> +       .enable = mtk_disp_pwm_enable,
> +       .disable = mtk_disp_pwm_disable,
> +       .owner = THIS_MODULE,
> +};
> +
> +static int mtk_disp_pwm_probe(struct platform_device *pdev)
> +{
> +       struct mtk_disp_pwm *mdp;
> +       struct resource *r;
> +       int ret;
> +
> +       mdp = devm_kzalloc(&pdev->dev, sizeof(*mdp), GFP_KERNEL);
> +       if (!mdp)
> +               return -ENOMEM;
> +
> +       mdp->dev = &pdev->dev;
> +
> +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       mdp->base = devm_ioremap_resource(&pdev->dev, r);
> +       if (IS_ERR(mdp->base))
> +               return PTR_ERR(mdp->base);
> +
> +       mdp->clk_main = devm_clk_get(&pdev->dev, "main");
> +       if (IS_ERR(mdp->clk_main))
> +               return PTR_ERR(mdp->clk_main);
> +
> +       mdp->clk_mm = devm_clk_get(&pdev->dev, "mm");
> +       if (IS_ERR(mdp->clk_mm))
> +               return PTR_ERR(mdp->clk_mm);
> +
> +       ret = clk_prepare_enable(mdp->clk_main);

Delay turning on the PWM clock until it is actually needed (pwm_enable)...
Just be careful to ensure that the "main" clock is enabled when
writing registers during mtk_disp_pwm_config.
By the way, is the pwm in a power domain that must also be enabled
when enabling the pwm?

> +       if (ret < 0)
> +               return ret;
> +
> +       ret = clk_prepare_enable(mdp->clk_mm);
> +       if (ret < 0)
> +               goto disable_clk_main;
> +
> +       platform_set_drvdata(pdev, mdp);

Set this only after pwmchip_add() succeeds.

> +
> +       mdp->chip.dev = &pdev->dev;
> +       mdp->chip.ops = &mtk_disp_pwm_ops;
> +       mdp->chip.base = -1;
> +       mdp->chip.npwm = 1;
> +
> +       ret = pwmchip_add(&mdp->chip);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> +               goto disable_clk_mm;
> +       }
> +
> +       return 0;
> +
> +disable_clk_mm:
> +       clk_disable_unprepare(mdp->clk_mm);
> +disable_clk_main:
> +       clk_disable_unprepare(mdp->clk_main);
> +       return ret;
> +}
> +
> +static int mtk_disp_pwm_remove(struct platform_device *pdev)
> +{
> +       struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev);
> +       int ret = pwmchip_remove(&mdp->chip);
> +
> +       clk_disable_unprepare(mdp->clk_main);
> +       clk_disable_unprepare(mdp->clk_mm);

Nit: it is more traditional to disable clocks in the opposite order to
which they are enabled, so:

       clk_disable_unprepare(mdp->clk_mm);
       clk_disable_unprepare(mdp->clk_main);

> +
> +       return ret;
> +}
> +
> +static const struct of_device_id mtk_disp_pwm_of_match[] = {
> +       { .compatible = "mediatek,mt8173-disp-pwm" },
> +       { .compatible = "mediatek,mt6595-disp-pwm" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, mtk_disp_pwm_of_match);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mtk_disp_pwm_suspend(struct device *dev)
> +{
> +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> +
> +       clk_disable_unprepare(mdp->clk_main);
> +       clk_disable_unprepare(mdp->clk_mm);
> +
> +       return 0;
> +}
> +
> +static int mtk_disp_pwm_resume(struct device *dev)
> +{
> +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> +       int ret;
> +
> +       ret = clk_prepare_enable(mdp->clk_main);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = clk_prepare_enable(mdp->clk_mm);
> +       if (ret < 0) {
> +               clk_disable_unprepare(mdp->clk_main);
> +               return ret;
> +       }
> +

Don't you also have to restore the PWM rate and frequency?

Is it possible to save power at runtime by leaving mdp->clk_mm enabled
(to generate the PWM signal), but disable mdp->clk_main (clock
required to access PWM registers)?


> +       return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(mtk_disp_pwm_pm_ops, mtk_disp_pwm_suspend,
> +                        mtk_disp_pwm_resume);
> +
> +static struct platform_driver mtk_disp_pwm_driver = {
> +       .driver = {
> +               .name = "mediatek-disp-pwm",
> +               .pm = &mtk_disp_pwm_pm_ops,
> +               .of_match_table = mtk_disp_pwm_of_match,
> +       },
> +       .probe = mtk_disp_pwm_probe,
> +       .remove = mtk_disp_pwm_remove,
> +};
> +module_platform_driver(mtk_disp_pwm_driver);
> +
> +MODULE_AUTHOR("YH Huang <yh.huang@mediatek.com>");
> +MODULE_DESCRIPTION("MediaTek SoC display PWM driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.8.1.1.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[YH Huang]

On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:
> On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang@mediatek.com> wrote:
> > Add display PWM driver support to modify backlight for MT8173 and MT6595.
> > The PWM has one channel to control the brightness of the display.
> > When the (high_width / period) is closer to 1, the screen is brighter;
> > otherwise, it is darker.
> >
> > Signed-off-by: YH Huang <yh.huang@mediatek.com>
> > ---
> >  drivers/pwm/Kconfig        |  10 ++
> >  drivers/pwm/Makefile       |   1 +
> >  drivers/pwm/pwm-mtk-disp.c | 256 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 267 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-mtk-disp.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index b1541f4..f5b03a4 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -211,6 +211,16 @@ config PWM_LPSS_PLATFORM
> >           To compile this driver as a module, choose M here: the module
> >           will be called pwm-lpss-platform.
> >
> > +config PWM_MTK_DISP
> > +       tristate "MediaTek display PWM driver"
> > +       depends on ARCH_MEDIATEK || COMPILE_TEST
> > +       help
> > +         Generic PWM framework driver for MediaTek disp-pwm device.
> > +         The PWM is used to control the backlight brightness for display.
> > +
> > +         To compile this driver as a module, choose M here: the module
> > +         will be called pwm-mtk-disp.
> > +
> >  config PWM_MXS
> >         tristate "Freescale MXS PWM support"
> >         depends on ARCH_MXS && OF
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index ec50eb5..99c9e75 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_LPC32XX)     += pwm-lpc32xx.o
> >  obj-$(CONFIG_PWM_LPSS)         += pwm-lpss.o
> >  obj-$(CONFIG_PWM_LPSS_PCI)     += pwm-lpss-pci.o
> >  obj-$(CONFIG_PWM_LPSS_PLATFORM)        += pwm-lpss-platform.o
> > +obj-$(CONFIG_PWM_MTK_DISP)     += pwm-mtk-disp.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-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> > new file mode 100644
> > index 0000000..1f17cee
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-mtk-disp.c
> > @@ -0,0 +1,256 @@
> > +/*
> > + * MediaTek display pulse-width-modulation controller driver.
> > + * Copyright (c) 2015 MediaTek Inc.
> > + * Author: YH Huang <yh.huang@mediatek.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.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/pwm.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#define DISP_PWM_EN            0
> 
> The "DISP_PWM_*" are register offsets, so use a hex value, like this:
> 
> #define DISP_PWM_EN                      0x00
> 
> Use BIT() for register *fields*, that is, the individual bits of a register.
> 

Got it!

> > +#define PWM_ENABLE_MASK                BIT(0)
> > +
> > +#define DISP_PWM_COMMIT                BIT(3)
> 
> #define DISP_PWM_COMMIT            0x08
> 
> > +#define PWM_COMMIT_MASK                BIT(0)
> > +
> > +#define DISP_PWM_CON_0         BIT(4)
> 
> #define DISP_PWM_COMMIT            0x10
> 
> > +#define PWM_CLKDIV_SHIFT       16
> > +#define PWM_CLKDIV_MAX         0x3ff
> > +#define PWM_CLKDIV_MASK                (PWM_CLKDIV_MAX << PWM_CLKDIV_SHIFT)
> > +
> > +#define DISP_PWM_CON_1         0x14
> > +#define PWM_PERIOD_MASK                0xfff
> > +/* Shift log2(PWM_PERIOD_MASK + 1) as divisor */
> > +#define PWM_PERIOD_BIT_SHIFT   12
> > +
> > +#define PWM_HIGH_WIDTH_SHIFT   16
> > +#define PWM_HIGH_WIDTH_MASK    (0x1fff << PWM_HIGH_WIDTH_SHIFT)
> > +
> > +struct mtk_disp_pwm {
> > +       struct pwm_chip chip;
> > +       struct device *dev;
> 
> I don't think "dev" is actually used.  And, if needed, it can be
> extracted from "chip".
> 

I will drop it.

> > +       struct clk *clk_main;
> > +       struct clk *clk_mm;
> > +       void __iomem *base;
> > +};
> > +
> > +static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)
> > +{
> > +       return container_of(chip, struct mtk_disp_pwm, chip);
> > +}
> > +
> > +static void mtk_disp_pwm_update_bits(void __iomem *address, u32 mask, u32 value)
> 
> Take "struct mtk_disp_pwm *mdp" as a param and extract mdp->base,
> rather than pass the raw iomem address.
> 

I will revise it like this.

static void mtk_disp_pwm_update_bits(struct mtk_disp_pwm *mdp,
			       u32 reg, u32 mask, u32 value)
{
	void __iomem *address;
	u32 val;

	address = mdp->base + reg;
	val = readl(address);
	val &= ~mask;
	val |= value;
	writel(val, address);
}

> > +{
> > +       u32 val;
> > +
> > +       val = readl(address);
> > +       val &= ~mask;
> > +       val |= value;
> > +       writel(val, address);
> > +}
> > +
> > +static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                              int duty_ns, int period_ns)
> > +{
> > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> > +       u64 div, rate;
> > +       u32 clk_div, period, high_width, value;
> > +
> > +       /*
> > +        * Find period, high_width and clk_div to suit duty_ns and period_ns.
> > +        * Calculate proper div value to keep period value in the bound.
> > +        *
> > +        * period_ns = 10^9 * (clk_div + 1) * (period + 1) / PWM_CLK_RATE
> > +        * duty_ns = 10^9 * (clk_div + 1) * high_width / PWM_CLK_RATE
> > +        *
> > +        * period = (PWM_CLK_RATE * period_ns) / (10^9 * (clk_div + 1)) - 1
> > +        * high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
> > +        */
> > +       rate = clk_get_rate(mdp->clk_main);
> > +       clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
> > +                         PWM_PERIOD_BIT_SHIFT;
> > +       if (clk_div > PWM_CLKDIV_MAX)
> > +               return -EINVAL;
> > +
> > +       div = NSEC_PER_SEC * (clk_div + 1);
> > +       period = div64_u64(rate * period_ns, div);
> > +       if (period > 0)
> > +               period--;
> > +
> > +       high_width = div64_u64(rate * duty_ns, div);
> > +
> > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_0,
> > +                                PWM_CLKDIV_MASK, clk_div << PWM_CLKDIV_SHIFT);
> > +
> > +       value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
> > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_1,
> > +                                PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
> > +
> > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
> > +                                PWM_COMMIT_MASK, 1);
> > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
> > +                                PWM_COMMIT_MASK, 0);
> > +
> > +       return 0;
> > +}
> > +
> > +static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> > +
> > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
> > +                                PWM_ENABLE_MASK, 1);
> > +
> > +       return 0;
> > +}
> > +
> > +static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> > +
> > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
> > +                                PWM_ENABLE_MASK, 0);
> > +}
> > +
> > +static const struct pwm_ops mtk_disp_pwm_ops = {
> > +       .config = mtk_disp_pwm_config,
> > +       .enable = mtk_disp_pwm_enable,
> > +       .disable = mtk_disp_pwm_disable,
> > +       .owner = THIS_MODULE,
> > +};
> > +
> > +static int mtk_disp_pwm_probe(struct platform_device *pdev)
> > +{
> > +       struct mtk_disp_pwm *mdp;
> > +       struct resource *r;
> > +       int ret;
> > +
> > +       mdp = devm_kzalloc(&pdev->dev, sizeof(*mdp), GFP_KERNEL);
> > +       if (!mdp)
> > +               return -ENOMEM;
> > +
> > +       mdp->dev = &pdev->dev;
> > +
> > +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       mdp->base = devm_ioremap_resource(&pdev->dev, r);
> > +       if (IS_ERR(mdp->base))
> > +               return PTR_ERR(mdp->base);
> > +
> > +       mdp->clk_main = devm_clk_get(&pdev->dev, "main");
> > +       if (IS_ERR(mdp->clk_main))
> > +               return PTR_ERR(mdp->clk_main);
> > +
> > +       mdp->clk_mm = devm_clk_get(&pdev->dev, "mm");
> > +       if (IS_ERR(mdp->clk_mm))
> > +               return PTR_ERR(mdp->clk_mm);
> > +
> > +       ret = clk_prepare_enable(mdp->clk_main);
> 
> Delay turning on the PWM clock until it is actually needed (pwm_enable)...
> Just be careful to ensure that the "main" clock is enabled when
> writing registers during mtk_disp_pwm_config.
> By the way, is the pwm in a power domain that must also be enabled
> when enabling the pwm?

It is in the display power domain which must also be enabled.

> 
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret = clk_prepare_enable(mdp->clk_mm);
> > +       if (ret < 0)
> > +               goto disable_clk_main;
> > +
> > +       platform_set_drvdata(pdev, mdp);
> 
> Set this only after pwmchip_add() succeeds.

OK.

> 
> > +
> > +       mdp->chip.dev = &pdev->dev;
> > +       mdp->chip.ops = &mtk_disp_pwm_ops;
> > +       mdp->chip.base = -1;
> > +       mdp->chip.npwm = 1;
> > +
> > +       ret = pwmchip_add(&mdp->chip);
> > +       if (ret < 0) {
> > +               dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> > +               goto disable_clk_mm;
> > +       }
> > +
> > +       return 0;
> > +
> > +disable_clk_mm:
> > +       clk_disable_unprepare(mdp->clk_mm);
> > +disable_clk_main:
> > +       clk_disable_unprepare(mdp->clk_main);
> > +       return ret;
> > +}
> > +
> > +static int mtk_disp_pwm_remove(struct platform_device *pdev)
> > +{
> > +       struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev);
> > +       int ret = pwmchip_remove(&mdp->chip);
> > +
> > +       clk_disable_unprepare(mdp->clk_main);
> > +       clk_disable_unprepare(mdp->clk_mm);
> 
> Nit: it is more traditional to disable clocks in the opposite order to
> which they are enabled, so:
> 
>        clk_disable_unprepare(mdp->clk_mm);
>        clk_disable_unprepare(mdp->clk_main);
> 

OK.

> > +
> > +       return ret;
> > +}
> > +
> > +static const struct of_device_id mtk_disp_pwm_of_match[] = {
> > +       { .compatible = "mediatek,mt8173-disp-pwm" },
> > +       { .compatible = "mediatek,mt6595-disp-pwm" },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_disp_pwm_of_match);
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int mtk_disp_pwm_suspend(struct device *dev)
> > +{
> > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> > +
> > +       clk_disable_unprepare(mdp->clk_main);
> > +       clk_disable_unprepare(mdp->clk_mm);
> > +
> > +       return 0;
> > +}
> > +
> > +static int mtk_disp_pwm_resume(struct device *dev)
> > +{
> > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> > +       int ret;
> > +
> > +       ret = clk_prepare_enable(mdp->clk_main);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret = clk_prepare_enable(mdp->clk_mm);
> > +       if (ret < 0) {
> > +               clk_disable_unprepare(mdp->clk_main);
> > +               return ret;
> > +       }
> > +
> 
> Don't you also have to restore the PWM rate and frequency?
> 
> Is it possible to save power at runtime by leaving mdp->clk_mm enabled
> (to generate the PWM signal), but disable mdp->clk_main (clock
> required to access PWM registers)?

The pwm-backlight driver will restore the data.

After I try to disable anyone of the two clocks at runtime, the
backlight doesn't work well(no immediate update or losing backlight).
So we need to keep both clock enabled.

Regards,
YH Huang

Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[YH Huang]

On Wed, 2015-07-15 at 23:59 +0800, YH Huang wrote:
> On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:
> > On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang@mediatek.com> wrote:
> > > Add display PWM driver support to modify backlight for MT8173 and MT6595.
> > > The PWM has one channel to control the brightness of the display.
> > > When the (high_width / period) is closer to 1, the screen is brighter;
> > > otherwise, it is darker.
> > >
> > > Signed-off-by: YH Huang <yh.huang@mediatek.com>
> > > ---
> > >  drivers/pwm/Kconfig        |  10 ++
> > >  drivers/pwm/Makefile       |   1 +
> > >  drivers/pwm/pwm-mtk-disp.c | 256 +++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 267 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-mtk-disp.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index b1541f4..f5b03a4 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -211,6 +211,16 @@ config PWM_LPSS_PLATFORM
> > >           To compile this driver as a module, choose M here: the module
> > >           will be called pwm-lpss-platform.
> > >
> > > +config PWM_MTK_DISP
> > > +       tristate "MediaTek display PWM driver"
> > > +       depends on ARCH_MEDIATEK || COMPILE_TEST
> > > +       help
> > > +         Generic PWM framework driver for MediaTek disp-pwm device.
> > > +         The PWM is used to control the backlight brightness for display.
> > > +
> > > +         To compile this driver as a module, choose M here: the module
> > > +         will be called pwm-mtk-disp.
> > > +
> > >  config PWM_MXS
> > >         tristate "Freescale MXS PWM support"
> > >         depends on ARCH_MXS && OF
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > index ec50eb5..99c9e75 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_LPC32XX)     += pwm-lpc32xx.o
> > >  obj-$(CONFIG_PWM_LPSS)         += pwm-lpss.o
> > >  obj-$(CONFIG_PWM_LPSS_PCI)     += pwm-lpss-pci.o
> > >  obj-$(CONFIG_PWM_LPSS_PLATFORM)        += pwm-lpss-platform.o
> > > +obj-$(CONFIG_PWM_MTK_DISP)     += pwm-mtk-disp.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-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> > > new file mode 100644
> > > index 0000000..1f17cee
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-mtk-disp.c
> > > @@ -0,0 +1,256 @@
> > > +/*
> > > + * MediaTek display pulse-width-modulation controller driver.
> > > + * Copyright (c) 2015 MediaTek Inc.
> > > + * Author: YH Huang <yh.huang@mediatek.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.
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/err.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +
> > > +#define DISP_PWM_EN            0
> > 
> > The "DISP_PWM_*" are register offsets, so use a hex value, like this:
> > 
> > #define DISP_PWM_EN                      0x00
> > 
> > Use BIT() for register *fields*, that is, the individual bits of a register.
> > 
> 
> Got it!
> 
> > > +#define PWM_ENABLE_MASK                BIT(0)
> > > +
> > > +#define DISP_PWM_COMMIT                BIT(3)
> > 
> > #define DISP_PWM_COMMIT            0x08
> > 
> > > +#define PWM_COMMIT_MASK                BIT(0)
> > > +
> > > +#define DISP_PWM_CON_0         BIT(4)
> > 
> > #define DISP_PWM_COMMIT            0x10
> > 
> > > +#define PWM_CLKDIV_SHIFT       16
> > > +#define PWM_CLKDIV_MAX         0x3ff
> > > +#define PWM_CLKDIV_MASK                (PWM_CLKDIV_MAX << PWM_CLKDIV_SHIFT)
> > > +
> > > +#define DISP_PWM_CON_1         0x14
> > > +#define PWM_PERIOD_MASK                0xfff
> > > +/* Shift log2(PWM_PERIOD_MASK + 1) as divisor */
> > > +#define PWM_PERIOD_BIT_SHIFT   12
> > > +
> > > +#define PWM_HIGH_WIDTH_SHIFT   16
> > > +#define PWM_HIGH_WIDTH_MASK    (0x1fff << PWM_HIGH_WIDTH_SHIFT)
> > > +
> > > +struct mtk_disp_pwm {
> > > +       struct pwm_chip chip;
> > > +       struct device *dev;
> > 
> > I don't think "dev" is actually used.  And, if needed, it can be
> > extracted from "chip".
> > 
> 
> I will drop it.
> 
> > > +       struct clk *clk_main;
> > > +       struct clk *clk_mm;
> > > +       void __iomem *base;
> > > +};
> > > +
> > > +static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)
> > > +{
> > > +       return container_of(chip, struct mtk_disp_pwm, chip);
> > > +}
> > > +
> > > +static void mtk_disp_pwm_update_bits(void __iomem *address, u32 mask, u32 value)
> > 
> > Take "struct mtk_disp_pwm *mdp" as a param and extract mdp->base,
> > rather than pass the raw iomem address.
> > 
> 
> I will revise it like this.
> 
> static void mtk_disp_pwm_update_bits(struct mtk_disp_pwm *mdp,
> 			       u32 reg, u32 mask, u32 value)
> {
> 	void __iomem *address;
> 	u32 val;
> 
> 	address = mdp->base + reg;
> 	val = readl(address);
> 	val &= ~mask;
> 	val |= value;
> 	writel(val, address);
> }
> 
> > > +{
> > > +       u32 val;
> > > +
> > > +       val = readl(address);
> > > +       val &= ~mask;
> > > +       val |= value;
> > > +       writel(val, address);
> > > +}
> > > +
> > > +static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +                              int duty_ns, int period_ns)
> > > +{
> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> > > +       u64 div, rate;
> > > +       u32 clk_div, period, high_width, value;
> > > +
> > > +       /*
> > > +        * Find period, high_width and clk_div to suit duty_ns and period_ns.
> > > +        * Calculate proper div value to keep period value in the bound.
> > > +        *
> > > +        * period_ns = 10^9 * (clk_div + 1) * (period + 1) / PWM_CLK_RATE
> > > +        * duty_ns = 10^9 * (clk_div + 1) * high_width / PWM_CLK_RATE
> > > +        *
> > > +        * period = (PWM_CLK_RATE * period_ns) / (10^9 * (clk_div + 1)) - 1
> > > +        * high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
> > > +        */
> > > +       rate = clk_get_rate(mdp->clk_main);
> > > +       clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
> > > +                         PWM_PERIOD_BIT_SHIFT;
> > > +       if (clk_div > PWM_CLKDIV_MAX)
> > > +               return -EINVAL;
> > > +
> > > +       div = NSEC_PER_SEC * (clk_div + 1);
> > > +       period = div64_u64(rate * period_ns, div);
> > > +       if (period > 0)
> > > +               period--;
> > > +
> > > +       high_width = div64_u64(rate * duty_ns, div);
> > > +
> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_0,
> > > +                                PWM_CLKDIV_MASK, clk_div << PWM_CLKDIV_SHIFT);
> > > +
> > > +       value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_1,
> > > +                                PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
> > > +
> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
> > > +                                PWM_COMMIT_MASK, 1);
> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
> > > +                                PWM_COMMIT_MASK, 0);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > +{
> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> > > +
> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
> > > +                                PWM_ENABLE_MASK, 1);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > +{
> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> > > +
> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
> > > +                                PWM_ENABLE_MASK, 0);
> > > +}
> > > +
> > > +static const struct pwm_ops mtk_disp_pwm_ops = {
> > > +       .config = mtk_disp_pwm_config,
> > > +       .enable = mtk_disp_pwm_enable,
> > > +       .disable = mtk_disp_pwm_disable,
> > > +       .owner = THIS_MODULE,
> > > +};
> > > +
> > > +static int mtk_disp_pwm_probe(struct platform_device *pdev)
> > > +{
> > > +       struct mtk_disp_pwm *mdp;
> > > +       struct resource *r;
> > > +       int ret;
> > > +
> > > +       mdp = devm_kzalloc(&pdev->dev, sizeof(*mdp), GFP_KERNEL);
> > > +       if (!mdp)
> > > +               return -ENOMEM;
> > > +
> > > +       mdp->dev = &pdev->dev;
> > > +
> > > +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +       mdp->base = devm_ioremap_resource(&pdev->dev, r);
> > > +       if (IS_ERR(mdp->base))
> > > +               return PTR_ERR(mdp->base);
> > > +
> > > +       mdp->clk_main = devm_clk_get(&pdev->dev, "main");
> > > +       if (IS_ERR(mdp->clk_main))
> > > +               return PTR_ERR(mdp->clk_main);
> > > +
> > > +       mdp->clk_mm = devm_clk_get(&pdev->dev, "mm");
> > > +       if (IS_ERR(mdp->clk_mm))
> > > +               return PTR_ERR(mdp->clk_mm);
> > > +
> > > +       ret = clk_prepare_enable(mdp->clk_main);
> > 
> > Delay turning on the PWM clock until it is actually needed (pwm_enable)...
> > Just be careful to ensure that the "main" clock is enabled when
> > writing registers during mtk_disp_pwm_config.

Since pwm-backlight driver configures PWM before enable PWM, I enable
clocks at the probe function.
Turning on the PWM clock in pwm_enable will lose the first configuration
of PWM. It won't affect a lot since the system configures PWM many times
at boot time.
So should I delay or keep the original way?

> > By the way, is the pwm in a power domain that must also be enabled
> > when enabling the pwm?
> 
> It is in the display power domain which must also be enabled.
> 
> > 
> > > +       if (ret < 0)
> > > +               return ret;
> > > +
> > > +       ret = clk_prepare_enable(mdp->clk_mm);
> > > +       if (ret < 0)
> > > +               goto disable_clk_main;
> > > +
> > > +       platform_set_drvdata(pdev, mdp);
> > 
> > Set this only after pwmchip_add() succeeds.
> 
> OK.
> 
> > 
> > > +
> > > +       mdp->chip.dev = &pdev->dev;
> > > +       mdp->chip.ops = &mtk_disp_pwm_ops;
> > > +       mdp->chip.base = -1;
> > > +       mdp->chip.npwm = 1;
> > > +
> > > +       ret = pwmchip_add(&mdp->chip);
> > > +       if (ret < 0) {
> > > +               dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> > > +               goto disable_clk_mm;
> > > +       }
> > > +
> > > +       return 0;
> > > +
> > > +disable_clk_mm:
> > > +       clk_disable_unprepare(mdp->clk_mm);
> > > +disable_clk_main:
> > > +       clk_disable_unprepare(mdp->clk_main);
> > > +       return ret;
> > > +}
> > > +
> > > +static int mtk_disp_pwm_remove(struct platform_device *pdev)
> > > +{
> > > +       struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev);
> > > +       int ret = pwmchip_remove(&mdp->chip);
> > > +
> > > +       clk_disable_unprepare(mdp->clk_main);
> > > +       clk_disable_unprepare(mdp->clk_mm);
> > 
> > Nit: it is more traditional to disable clocks in the opposite order to
> > which they are enabled, so:
> > 
> >        clk_disable_unprepare(mdp->clk_mm);
> >        clk_disable_unprepare(mdp->clk_main);
> > 
> 
> OK.
> 
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static const struct of_device_id mtk_disp_pwm_of_match[] = {
> > > +       { .compatible = "mediatek,mt8173-disp-pwm" },
> > > +       { .compatible = "mediatek,mt6595-disp-pwm" },
> > > +       { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mtk_disp_pwm_of_match);
> > > +
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int mtk_disp_pwm_suspend(struct device *dev)
> > > +{
> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> > > +
> > > +       clk_disable_unprepare(mdp->clk_main);
> > > +       clk_disable_unprepare(mdp->clk_mm);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int mtk_disp_pwm_resume(struct device *dev)
> > > +{
> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> > > +       int ret;
> > > +
> > > +       ret = clk_prepare_enable(mdp->clk_main);
> > > +       if (ret < 0)
> > > +               return ret;
> > > +
> > > +       ret = clk_prepare_enable(mdp->clk_mm);
> > > +       if (ret < 0) {
> > > +               clk_disable_unprepare(mdp->clk_main);
> > > +               return ret;
> > > +       }
> > > +
> > 
> > Don't you also have to restore the PWM rate and frequency?
> > 
> > Is it possible to save power at runtime by leaving mdp->clk_mm enabled
> > (to generate the PWM signal), but disable mdp->clk_main (clock
> > required to access PWM registers)?
> 
> The pwm-backlight driver will restore the data.
> 
> After I try to disable anyone of the two clocks at runtime, the
> backlight doesn't work well(no immediate update or losing backlight).
> So we need to keep both clock enabled.
> 
> Regards,
> YH Huang
> 

Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[Daniel Kurtz]

On Thu, Jul 16, 2015 at 1:38 PM, YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> On Wed, 2015-07-15 at 23:59 +0800, YH Huang wrote:
>> On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:
>> > On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>> > > Add display PWM driver support to modify backlight for MT8173 and MT6595.
>> > > The PWM has one channel to control the brightness of the display.
>> > > When the (high_width / period) is closer to 1, the screen is brighter;
>> > > otherwise, it is darker.
>> > >
>> > > Signed-off-by: YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>> > > ---
>> > >  drivers/pwm/Kconfig        |  10 ++
>> > >  drivers/pwm/Makefile       |   1 +
>> > >  drivers/pwm/pwm-mtk-disp.c | 256 +++++++++++++++++++++++++++++++++++++++++++++
>> > >  3 files changed, 267 insertions(+)
>> > >  create mode 100644 drivers/pwm/pwm-mtk-disp.c
>> > >
>> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> > > index b1541f4..f5b03a4 100644
>> > > --- a/drivers/pwm/Kconfig
>> > > +++ b/drivers/pwm/Kconfig
>> > > @@ -211,6 +211,16 @@ config PWM_LPSS_PLATFORM
>> > >           To compile this driver as a module, choose M here: the module
>> > >           will be called pwm-lpss-platform.
>> > >
>> > > +config PWM_MTK_DISP
>> > > +       tristate "MediaTek display PWM driver"
>> > > +       depends on ARCH_MEDIATEK || COMPILE_TEST
>> > > +       help
>> > > +         Generic PWM framework driver for MediaTek disp-pwm device.
>> > > +         The PWM is used to control the backlight brightness for display.
>> > > +
>> > > +         To compile this driver as a module, choose M here: the module
>> > > +         will be called pwm-mtk-disp.
>> > > +
>> > >  config PWM_MXS
>> > >         tristate "Freescale MXS PWM support"
>> > >         depends on ARCH_MXS && OF
>> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> > > index ec50eb5..99c9e75 100644
>> > > --- a/drivers/pwm/Makefile
>> > > +++ b/drivers/pwm/Makefile
>> > > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_LPC32XX)     += pwm-lpc32xx.o
>> > >  obj-$(CONFIG_PWM_LPSS)         += pwm-lpss.o
>> > >  obj-$(CONFIG_PWM_LPSS_PCI)     += pwm-lpss-pci.o
>> > >  obj-$(CONFIG_PWM_LPSS_PLATFORM)        += pwm-lpss-platform.o
>> > > +obj-$(CONFIG_PWM_MTK_DISP)     += pwm-mtk-disp.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-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
>> > > new file mode 100644
>> > > index 0000000..1f17cee
>> > > --- /dev/null
>> > > +++ b/drivers/pwm/pwm-mtk-disp.c
>> > > @@ -0,0 +1,256 @@
>> > > +/*
>> > > + * MediaTek display pulse-width-modulation controller driver.
>> > > + * Copyright (c) 2015 MediaTek Inc.
>> > > + * Author: YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>> > > + *
>> > > + * 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.
>> > > + */
>> > > +
>> > > +#include <linux/clk.h>
>> > > +#include <linux/err.h>
>> > > +#include <linux/io.h>
>> > > +#include <linux/module.h>
>> > > +#include <linux/of.h>
>> > > +#include <linux/pwm.h>
>> > > +#include <linux/platform_device.h>
>> > > +#include <linux/slab.h>
>> > > +
>> > > +#define DISP_PWM_EN            0
>> >
>> > The "DISP_PWM_*" are register offsets, so use a hex value, like this:
>> >
>> > #define DISP_PWM_EN                      0x00
>> >
>> > Use BIT() for register *fields*, that is, the individual bits of a register.
>> >
>>
>> Got it!
>>
>> > > +#define PWM_ENABLE_MASK                BIT(0)
>> > > +
>> > > +#define DISP_PWM_COMMIT                BIT(3)
>> >
>> > #define DISP_PWM_COMMIT            0x08
>> >
>> > > +#define PWM_COMMIT_MASK                BIT(0)
>> > > +
>> > > +#define DISP_PWM_CON_0         BIT(4)
>> >
>> > #define DISP_PWM_COMMIT            0x10
>> >
>> > > +#define PWM_CLKDIV_SHIFT       16
>> > > +#define PWM_CLKDIV_MAX         0x3ff
>> > > +#define PWM_CLKDIV_MASK                (PWM_CLKDIV_MAX << PWM_CLKDIV_SHIFT)
>> > > +
>> > > +#define DISP_PWM_CON_1         0x14
>> > > +#define PWM_PERIOD_MASK                0xfff
>> > > +/* Shift log2(PWM_PERIOD_MASK + 1) as divisor */
>> > > +#define PWM_PERIOD_BIT_SHIFT   12
>> > > +
>> > > +#define PWM_HIGH_WIDTH_SHIFT   16
>> > > +#define PWM_HIGH_WIDTH_MASK    (0x1fff << PWM_HIGH_WIDTH_SHIFT)
>> > > +
>> > > +struct mtk_disp_pwm {
>> > > +       struct pwm_chip chip;
>> > > +       struct device *dev;
>> >
>> > I don't think "dev" is actually used.  And, if needed, it can be
>> > extracted from "chip".
>> >
>>
>> I will drop it.
>>
>> > > +       struct clk *clk_main;
>> > > +       struct clk *clk_mm;
>> > > +       void __iomem *base;
>> > > +};
>> > > +
>> > > +static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)
>> > > +{
>> > > +       return container_of(chip, struct mtk_disp_pwm, chip);
>> > > +}
>> > > +
>> > > +static void mtk_disp_pwm_update_bits(void __iomem *address, u32 mask, u32 value)
>> >
>> > Take "struct mtk_disp_pwm *mdp" as a param and extract mdp->base,
>> > rather than pass the raw iomem address.
>> >
>>
>> I will revise it like this.
>>
>> static void mtk_disp_pwm_update_bits(struct mtk_disp_pwm *mdp,
>>                              u32 reg, u32 mask, u32 value)
>> {
>>       void __iomem *address;
>>       u32 val;
>>
>>       address = mdp->base + reg;
>>       val = readl(address);
>>       val &= ~mask;
>>       val |= value;
>>       writel(val, address);
>> }
>>
>> > > +{
>> > > +       u32 val;
>> > > +
>> > > +       val = readl(address);
>> > > +       val &= ~mask;
>> > > +       val |= value;
>> > > +       writel(val, address);
>> > > +}
>> > > +
>> > > +static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> > > +                              int duty_ns, int period_ns)
>> > > +{
>> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
>> > > +       u64 div, rate;
>> > > +       u32 clk_div, period, high_width, value;
>> > > +
>> > > +       /*
>> > > +        * Find period, high_width and clk_div to suit duty_ns and period_ns.
>> > > +        * Calculate proper div value to keep period value in the bound.
>> > > +        *
>> > > +        * period_ns = 10^9 * (clk_div + 1) * (period + 1) / PWM_CLK_RATE
>> > > +        * duty_ns = 10^9 * (clk_div + 1) * high_width / PWM_CLK_RATE
>> > > +        *
>> > > +        * period = (PWM_CLK_RATE * period_ns) / (10^9 * (clk_div + 1)) - 1
>> > > +        * high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
>> > > +        */
>> > > +       rate = clk_get_rate(mdp->clk_main);
>> > > +       clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
>> > > +                         PWM_PERIOD_BIT_SHIFT;
>> > > +       if (clk_div > PWM_CLKDIV_MAX)
>> > > +               return -EINVAL;
>> > > +
>> > > +       div = NSEC_PER_SEC * (clk_div + 1);
>> > > +       period = div64_u64(rate * period_ns, div);
>> > > +       if (period > 0)
>> > > +               period--;
>> > > +
>> > > +       high_width = div64_u64(rate * duty_ns, div);
>> > > +
>> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_0,
>> > > +                                PWM_CLKDIV_MASK, clk_div << PWM_CLKDIV_SHIFT);
>> > > +
>> > > +       value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
>> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_1,
>> > > +                                PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
>> > > +
>> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
>> > > +                                PWM_COMMIT_MASK, 1);
>> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
>> > > +                                PWM_COMMIT_MASK, 0);
>> > > +
>> > > +       return 0;
>> > > +}
>> > > +
>> > > +static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> > > +{
>> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
>> > > +
>> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
>> > > +                                PWM_ENABLE_MASK, 1);
>> > > +
>> > > +       return 0;
>> > > +}
>> > > +
>> > > +static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> > > +{
>> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
>> > > +
>> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
>> > > +                                PWM_ENABLE_MASK, 0);
>> > > +}
>> > > +
>> > > +static const struct pwm_ops mtk_disp_pwm_ops = {
>> > > +       .config = mtk_disp_pwm_config,
>> > > +       .enable = mtk_disp_pwm_enable,
>> > > +       .disable = mtk_disp_pwm_disable,
>> > > +       .owner = THIS_MODULE,
>> > > +};
>> > > +
>> > > +static int mtk_disp_pwm_probe(struct platform_device *pdev)
>> > > +{
>> > > +       struct mtk_disp_pwm *mdp;
>> > > +       struct resource *r;
>> > > +       int ret;
>> > > +
>> > > +       mdp = devm_kzalloc(&pdev->dev, sizeof(*mdp), GFP_KERNEL);
>> > > +       if (!mdp)
>> > > +               return -ENOMEM;
>> > > +
>> > > +       mdp->dev = &pdev->dev;
>> > > +
>> > > +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > > +       mdp->base = devm_ioremap_resource(&pdev->dev, r);
>> > > +       if (IS_ERR(mdp->base))
>> > > +               return PTR_ERR(mdp->base);
>> > > +
>> > > +       mdp->clk_main = devm_clk_get(&pdev->dev, "main");
>> > > +       if (IS_ERR(mdp->clk_main))
>> > > +               return PTR_ERR(mdp->clk_main);
>> > > +
>> > > +       mdp->clk_mm = devm_clk_get(&pdev->dev, "mm");
>> > > +       if (IS_ERR(mdp->clk_mm))
>> > > +               return PTR_ERR(mdp->clk_mm);
>> > > +
>> > > +       ret = clk_prepare_enable(mdp->clk_main);
>> >
>> > Delay turning on the PWM clock until it is actually needed (pwm_enable)...
>> > Just be careful to ensure that the "main" clock is enabled when
>> > writing registers during mtk_disp_pwm_config.
>
> Since pwm-backlight driver configures PWM before enable PWM, I enable
> clocks at the probe function.
> Turning on the PWM clock in pwm_enable will lose the first configuration
> of PWM. It won't affect a lot since the system configures PWM many times
> at boot time.
> So should I delay or keep the original way?

Why does turning on the PWM clock in pwm_enable lose the first configuration?
Would this be solved by turning enabling/disabling clocks during
mtk_disp_pwm_config()?

>
>> > By the way, is the pwm in a power domain that must also be enabled
>> > when enabling the pwm?
>>
>> It is in the display power domain which must also be enabled.
>>
>> >
>> > > +       if (ret < 0)
>> > > +               return ret;
>> > > +
>> > > +       ret = clk_prepare_enable(mdp->clk_mm);
>> > > +       if (ret < 0)
>> > > +               goto disable_clk_main;
>> > > +
>> > > +       platform_set_drvdata(pdev, mdp);
>> >
>> > Set this only after pwmchip_add() succeeds.
>>
>> OK.
>>
>> >
>> > > +
>> > > +       mdp->chip.dev = &pdev->dev;
>> > > +       mdp->chip.ops = &mtk_disp_pwm_ops;
>> > > +       mdp->chip.base = -1;
>> > > +       mdp->chip.npwm = 1;
>> > > +
>> > > +       ret = pwmchip_add(&mdp->chip);
>> > > +       if (ret < 0) {
>> > > +               dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
>> > > +               goto disable_clk_mm;
>> > > +       }
>> > > +
>> > > +       return 0;
>> > > +
>> > > +disable_clk_mm:
>> > > +       clk_disable_unprepare(mdp->clk_mm);
>> > > +disable_clk_main:
>> > > +       clk_disable_unprepare(mdp->clk_main);
>> > > +       return ret;
>> > > +}
>> > > +
>> > > +static int mtk_disp_pwm_remove(struct platform_device *pdev)
>> > > +{
>> > > +       struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev);
>> > > +       int ret = pwmchip_remove(&mdp->chip);
>> > > +
>> > > +       clk_disable_unprepare(mdp->clk_main);
>> > > +       clk_disable_unprepare(mdp->clk_mm);
>> >
>> > Nit: it is more traditional to disable clocks in the opposite order to
>> > which they are enabled, so:
>> >
>> >        clk_disable_unprepare(mdp->clk_mm);
>> >        clk_disable_unprepare(mdp->clk_main);
>> >
>>
>> OK.
>>
>> > > +
>> > > +       return ret;
>> > > +}
>> > > +
>> > > +static const struct of_device_id mtk_disp_pwm_of_match[] = {
>> > > +       { .compatible = "mediatek,mt8173-disp-pwm" },
>> > > +       { .compatible = "mediatek,mt6595-disp-pwm" },
>> > > +       { }
>> > > +};
>> > > +MODULE_DEVICE_TABLE(of, mtk_disp_pwm_of_match);
>> > > +
>> > > +#ifdef CONFIG_PM_SLEEP
>> > > +static int mtk_disp_pwm_suspend(struct device *dev)
>> > > +{
>> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
>> > > +
>> > > +       clk_disable_unprepare(mdp->clk_main);
>> > > +       clk_disable_unprepare(mdp->clk_mm);
>> > > +
>> > > +       return 0;
>> > > +}
>> > > +
>> > > +static int mtk_disp_pwm_resume(struct device *dev)
>> > > +{
>> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
>> > > +       int ret;
>> > > +
>> > > +       ret = clk_prepare_enable(mdp->clk_main);
>> > > +       if (ret < 0)
>> > > +               return ret;
>> > > +
>> > > +       ret = clk_prepare_enable(mdp->clk_mm);
>> > > +       if (ret < 0) {
>> > > +               clk_disable_unprepare(mdp->clk_main);
>> > > +               return ret;
>> > > +       }
>> > > +
>> >
>> > Don't you also have to restore the PWM rate and frequency?
>> >
>> > Is it possible to save power at runtime by leaving mdp->clk_mm enabled
>> > (to generate the PWM signal), but disable mdp->clk_main (clock
>> > required to access PWM registers)?
>>
>> The pwm-backlight driver will restore the data.
>>
>> After I try to disable anyone of the two clocks at runtime, the
>> backlight doesn't work well(no immediate update or losing backlight).
>> So we need to keep both clock enabled.
>>
>> Regards,
>> YH Huang
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[YH Huang]

On Thu, 2015-07-16 at 14:54 +0800, Daniel Kurtz wrote:
> On Thu, Jul 16, 2015 at 1:38 PM, YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> > On Wed, 2015-07-15 at 23:59 +0800, YH Huang wrote:
> >> On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:
> >> > On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >> > > Add display PWM driver support to modify backlight for MT8173 and MT6595.
> >> > > The PWM has one channel to control the brightness of the display.
> >> > > When the (high_width / period) is closer to 1, the screen is brighter;
> >> > > otherwise, it is darker.
> >> > >
> >> > > Signed-off-by: YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> >> > > ---
> >> > >  drivers/pwm/Kconfig        |  10 ++
> >> > >  drivers/pwm/Makefile       |   1 +
> >> > >  drivers/pwm/pwm-mtk-disp.c | 256 +++++++++++++++++++++++++++++++++++++++++++++
> >> > >  3 files changed, 267 insertions(+)
> >> > >  create mode 100644 drivers/pwm/pwm-mtk-disp.c
> >> > >
> >> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> > > index b1541f4..f5b03a4 100644
> >> > > --- a/drivers/pwm/Kconfig
> >> > > +++ b/drivers/pwm/Kconfig
> >> > > @@ -211,6 +211,16 @@ config PWM_LPSS_PLATFORM
> >> > >           To compile this driver as a module, choose M here: the module
> >> > >           will be called pwm-lpss-platform.
> >> > >
> >> > > +config PWM_MTK_DISP
> >> > > +       tristate "MediaTek display PWM driver"
> >> > > +       depends on ARCH_MEDIATEK || COMPILE_TEST
> >> > > +       help
> >> > > +         Generic PWM framework driver for MediaTek disp-pwm device.
> >> > > +         The PWM is used to control the backlight brightness for display.
> >> > > +
> >> > > +         To compile this driver as a module, choose M here: the module
> >> > > +         will be called pwm-mtk-disp.
> >> > > +
> >> > >  config PWM_MXS
> >> > >         tristate "Freescale MXS PWM support"
> >> > >         depends on ARCH_MXS && OF
> >> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> >> > > index ec50eb5..99c9e75 100644
> >> > > --- a/drivers/pwm/Makefile
> >> > > +++ b/drivers/pwm/Makefile
> >> > > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_LPC32XX)     += pwm-lpc32xx.o
> >> > >  obj-$(CONFIG_PWM_LPSS)         += pwm-lpss.o
> >> > >  obj-$(CONFIG_PWM_LPSS_PCI)     += pwm-lpss-pci.o
> >> > >  obj-$(CONFIG_PWM_LPSS_PLATFORM)        += pwm-lpss-platform.o
> >> > > +obj-$(CONFIG_PWM_MTK_DISP)     += pwm-mtk-disp.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-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> >> > > new file mode 100644
> >> > > index 0000000..1f17cee
> >> > > --- /dev/null
> >> > > +++ b/drivers/pwm/pwm-mtk-disp.c
> >> > > @@ -0,0 +1,256 @@
> >> > > +/*
> >> > > + * MediaTek display pulse-width-modulation controller driver.
> >> > > + * Copyright (c) 2015 MediaTek Inc.
> >> > > + * Author: YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> >> > > + *
> >> > > + * 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.
> >> > > + */
> >> > > +
> >> > > +#include <linux/clk.h>
> >> > > +#include <linux/err.h>
> >> > > +#include <linux/io.h>
> >> > > +#include <linux/module.h>
> >> > > +#include <linux/of.h>
> >> > > +#include <linux/pwm.h>
> >> > > +#include <linux/platform_device.h>
> >> > > +#include <linux/slab.h>
> >> > > +
> >> > > +#define DISP_PWM_EN            0
> >> >
> >> > The "DISP_PWM_*" are register offsets, so use a hex value, like this:
> >> >
> >> > #define DISP_PWM_EN                      0x00
> >> >
> >> > Use BIT() for register *fields*, that is, the individual bits of a register.
> >> >
> >>
> >> Got it!
> >>
> >> > > +#define PWM_ENABLE_MASK                BIT(0)
> >> > > +
> >> > > +#define DISP_PWM_COMMIT                BIT(3)
> >> >
> >> > #define DISP_PWM_COMMIT            0x08
> >> >
> >> > > +#define PWM_COMMIT_MASK                BIT(0)
> >> > > +
> >> > > +#define DISP_PWM_CON_0         BIT(4)
> >> >
> >> > #define DISP_PWM_COMMIT            0x10
> >> >
> >> > > +#define PWM_CLKDIV_SHIFT       16
> >> > > +#define PWM_CLKDIV_MAX         0x3ff
> >> > > +#define PWM_CLKDIV_MASK                (PWM_CLKDIV_MAX << PWM_CLKDIV_SHIFT)
> >> > > +
> >> > > +#define DISP_PWM_CON_1         0x14
> >> > > +#define PWM_PERIOD_MASK                0xfff
> >> > > +/* Shift log2(PWM_PERIOD_MASK + 1) as divisor */
> >> > > +#define PWM_PERIOD_BIT_SHIFT   12
> >> > > +
> >> > > +#define PWM_HIGH_WIDTH_SHIFT   16
> >> > > +#define PWM_HIGH_WIDTH_MASK    (0x1fff << PWM_HIGH_WIDTH_SHIFT)
> >> > > +
> >> > > +struct mtk_disp_pwm {
> >> > > +       struct pwm_chip chip;
> >> > > +       struct device *dev;
> >> >
> >> > I don't think "dev" is actually used.  And, if needed, it can be
> >> > extracted from "chip".
> >> >
> >>
> >> I will drop it.
> >>
> >> > > +       struct clk *clk_main;
> >> > > +       struct clk *clk_mm;
> >> > > +       void __iomem *base;
> >> > > +};
> >> > > +
> >> > > +static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)
> >> > > +{
> >> > > +       return container_of(chip, struct mtk_disp_pwm, chip);
> >> > > +}
> >> > > +
> >> > > +static void mtk_disp_pwm_update_bits(void __iomem *address, u32 mask, u32 value)
> >> >
> >> > Take "struct mtk_disp_pwm *mdp" as a param and extract mdp->base,
> >> > rather than pass the raw iomem address.
> >> >
> >>
> >> I will revise it like this.
> >>
> >> static void mtk_disp_pwm_update_bits(struct mtk_disp_pwm *mdp,
> >>                              u32 reg, u32 mask, u32 value)
> >> {
> >>       void __iomem *address;
> >>       u32 val;
> >>
> >>       address = mdp->base + reg;
> >>       val = readl(address);
> >>       val &= ~mask;
> >>       val |= value;
> >>       writel(val, address);
> >> }
> >>
> >> > > +{
> >> > > +       u32 val;
> >> > > +
> >> > > +       val = readl(address);
> >> > > +       val &= ~mask;
> >> > > +       val |= value;
> >> > > +       writel(val, address);
> >> > > +}
> >> > > +
> >> > > +static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >> > > +                              int duty_ns, int period_ns)
> >> > > +{
> >> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> >> > > +       u64 div, rate;
> >> > > +       u32 clk_div, period, high_width, value;
> >> > > +
> >> > > +       /*
> >> > > +        * Find period, high_width and clk_div to suit duty_ns and period_ns.
> >> > > +        * Calculate proper div value to keep period value in the bound.
> >> > > +        *
> >> > > +        * period_ns = 10^9 * (clk_div + 1) * (period + 1) / PWM_CLK_RATE
> >> > > +        * duty_ns = 10^9 * (clk_div + 1) * high_width / PWM_CLK_RATE
> >> > > +        *
> >> > > +        * period = (PWM_CLK_RATE * period_ns) / (10^9 * (clk_div + 1)) - 1
> >> > > +        * high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
> >> > > +        */
> >> > > +       rate = clk_get_rate(mdp->clk_main);
> >> > > +       clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
> >> > > +                         PWM_PERIOD_BIT_SHIFT;
> >> > > +       if (clk_div > PWM_CLKDIV_MAX)
> >> > > +               return -EINVAL;
> >> > > +
> >> > > +       div = NSEC_PER_SEC * (clk_div + 1);
> >> > > +       period = div64_u64(rate * period_ns, div);
> >> > > +       if (period > 0)
> >> > > +               period--;
> >> > > +
> >> > > +       high_width = div64_u64(rate * duty_ns, div);
> >> > > +
> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_0,
> >> > > +                                PWM_CLKDIV_MASK, clk_div << PWM_CLKDIV_SHIFT);
> >> > > +
> >> > > +       value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_1,
> >> > > +                                PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
> >> > > +
> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
> >> > > +                                PWM_COMMIT_MASK, 1);
> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
> >> > > +                                PWM_COMMIT_MASK, 0);
> >> > > +
> >> > > +       return 0;
> >> > > +}
> >> > > +
> >> > > +static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >> > > +{
> >> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> >> > > +
> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
> >> > > +                                PWM_ENABLE_MASK, 1);
> >> > > +
> >> > > +       return 0;
> >> > > +}
> >> > > +
> >> > > +static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >> > > +{
> >> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> >> > > +
> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
> >> > > +                                PWM_ENABLE_MASK, 0);
> >> > > +}
> >> > > +
> >> > > +static const struct pwm_ops mtk_disp_pwm_ops = {
> >> > > +       .config = mtk_disp_pwm_config,
> >> > > +       .enable = mtk_disp_pwm_enable,
> >> > > +       .disable = mtk_disp_pwm_disable,
> >> > > +       .owner = THIS_MODULE,
> >> > > +};
> >> > > +
> >> > > +static int mtk_disp_pwm_probe(struct platform_device *pdev)
> >> > > +{
> >> > > +       struct mtk_disp_pwm *mdp;
> >> > > +       struct resource *r;
> >> > > +       int ret;
> >> > > +
> >> > > +       mdp = devm_kzalloc(&pdev->dev, sizeof(*mdp), GFP_KERNEL);
> >> > > +       if (!mdp)
> >> > > +               return -ENOMEM;
> >> > > +
> >> > > +       mdp->dev = &pdev->dev;
> >> > > +
> >> > > +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> > > +       mdp->base = devm_ioremap_resource(&pdev->dev, r);
> >> > > +       if (IS_ERR(mdp->base))
> >> > > +               return PTR_ERR(mdp->base);
> >> > > +
> >> > > +       mdp->clk_main = devm_clk_get(&pdev->dev, "main");
> >> > > +       if (IS_ERR(mdp->clk_main))
> >> > > +               return PTR_ERR(mdp->clk_main);
> >> > > +
> >> > > +       mdp->clk_mm = devm_clk_get(&pdev->dev, "mm");
> >> > > +       if (IS_ERR(mdp->clk_mm))
> >> > > +               return PTR_ERR(mdp->clk_mm);
> >> > > +
> >> > > +       ret = clk_prepare_enable(mdp->clk_main);
> >> >
> >> > Delay turning on the PWM clock until it is actually needed (pwm_enable)...
> >> > Just be careful to ensure that the "main" clock is enabled when
> >> > writing registers during mtk_disp_pwm_config.
> >
> > Since pwm-backlight driver configures PWM before enable PWM, I enable
> > clocks at the probe function.
> > Turning on the PWM clock in pwm_enable will lose the first configuration
> > of PWM. It won't affect a lot since the system configures PWM many times
> > at boot time.
> > So should I delay or keep the original way?
> 
> Why does turning on the PWM clock in pwm_enable lose the first configuration?
> Would this be solved by turning enabling/disabling clocks during
> mtk_disp_pwm_config()?
> 

If I don't enable clocks at probe function, I enable clocks at
mtk_disp_pwm_enable() and disable clocks at mtk_disp_pwm_disable().
And this causes missing first configuration.

Enabling clocks in mtk_disp_pwm_config() will fix this.
The backlight doesn't work well(no immediate update or losing backlight)
if I disable anyone of the two clocks in mtk_disp_pwm_config().

Regards,
YH Huang

> >
> >> > By the way, is the pwm in a power domain that must also be enabled
> >> > when enabling the pwm?
> >>
> >> It is in the display power domain which must also be enabled.
> >>
> >> >
> >> > > +       if (ret < 0)
> >> > > +               return ret;
> >> > > +
> >> > > +       ret = clk_prepare_enable(mdp->clk_mm);
> >> > > +       if (ret < 0)
> >> > > +               goto disable_clk_main;
> >> > > +
> >> > > +       platform_set_drvdata(pdev, mdp);
> >> >
> >> > Set this only after pwmchip_add() succeeds.
> >>
> >> OK.
> >>
> >> >
> >> > > +
> >> > > +       mdp->chip.dev = &pdev->dev;
> >> > > +       mdp->chip.ops = &mtk_disp_pwm_ops;
> >> > > +       mdp->chip.base = -1;
> >> > > +       mdp->chip.npwm = 1;
> >> > > +
> >> > > +       ret = pwmchip_add(&mdp->chip);
> >> > > +       if (ret < 0) {
> >> > > +               dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> >> > > +               goto disable_clk_mm;
> >> > > +       }
> >> > > +
> >> > > +       return 0;
> >> > > +
> >> > > +disable_clk_mm:
> >> > > +       clk_disable_unprepare(mdp->clk_mm);
> >> > > +disable_clk_main:
> >> > > +       clk_disable_unprepare(mdp->clk_main);
> >> > > +       return ret;
> >> > > +}
> >> > > +
> >> > > +static int mtk_disp_pwm_remove(struct platform_device *pdev)
> >> > > +{
> >> > > +       struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev);
> >> > > +       int ret = pwmchip_remove(&mdp->chip);
> >> > > +
> >> > > +       clk_disable_unprepare(mdp->clk_main);
> >> > > +       clk_disable_unprepare(mdp->clk_mm);
> >> >
> >> > Nit: it is more traditional to disable clocks in the opposite order to
> >> > which they are enabled, so:
> >> >
> >> >        clk_disable_unprepare(mdp->clk_mm);
> >> >        clk_disable_unprepare(mdp->clk_main);
> >> >
> >>
> >> OK.
> >>
> >> > > +
> >> > > +       return ret;
> >> > > +}
> >> > > +
> >> > > +static const struct of_device_id mtk_disp_pwm_of_match[] = {
> >> > > +       { .compatible = "mediatek,mt8173-disp-pwm" },
> >> > > +       { .compatible = "mediatek,mt6595-disp-pwm" },
> >> > > +       { }
> >> > > +};
> >> > > +MODULE_DEVICE_TABLE(of, mtk_disp_pwm_of_match);
> >> > > +
> >> > > +#ifdef CONFIG_PM_SLEEP
> >> > > +static int mtk_disp_pwm_suspend(struct device *dev)
> >> > > +{
> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> >> > > +
> >> > > +       clk_disable_unprepare(mdp->clk_main);
> >> > > +       clk_disable_unprepare(mdp->clk_mm);
> >> > > +
> >> > > +       return 0;
> >> > > +}
> >> > > +
> >> > > +static int mtk_disp_pwm_resume(struct device *dev)
> >> > > +{
> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> >> > > +       int ret;
> >> > > +
> >> > > +       ret = clk_prepare_enable(mdp->clk_main);
> >> > > +       if (ret < 0)
> >> > > +               return ret;
> >> > > +
> >> > > +       ret = clk_prepare_enable(mdp->clk_mm);
> >> > > +       if (ret < 0) {
> >> > > +               clk_disable_unprepare(mdp->clk_main);
> >> > > +               return ret;
> >> > > +       }
> >> > > +
> >> >
> >> > Don't you also have to restore the PWM rate and frequency?
> >> >
> >> > Is it possible to save power at runtime by leaving mdp->clk_mm enabled
> >> > (to generate the PWM signal), but disable mdp->clk_main (clock
> >> > required to access PWM registers)?
> >>
> >> The pwm-backlight driver will restore the data.
> >>
> >> After I try to disable anyone of the two clocks at runtime, the
> >> backlight doesn't work well(no immediate update or losing backlight).
> >> So we need to keep both clock enabled.
> >>
> >> Regards,
> >> YH Huang
> >>
> >
> >


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[Daniel Kurtz]

On Thu, Jul 16, 2015 at 3:17 PM, YH Huang <yh.huang@mediatek.com> wrote:
> On Thu, 2015-07-16 at 14:54 +0800, Daniel Kurtz wrote:
>> On Thu, Jul 16, 2015 at 1:38 PM, YH Huang <yh.huang@mediatek.com> wrote:
>> > On Wed, 2015-07-15 at 23:59 +0800, YH Huang wrote:
>> >> On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:
>> >> > On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang@mediatek.com> wrote:
>> >> > > Add display PWM driver support to modify backlight for MT8173 and MT6595.
>> >> > > The PWM has one channel to control the brightness of the display.
>> >> > > When the (high_width / period) is closer to 1, the screen is brighter;
>> >> > > otherwise, it is darker.
>> >> > >
>> >> > > Signed-off-by: YH Huang <yh.huang@mediatek.com>
>> >> > > ---
>> >> > >  drivers/pwm/Kconfig        |  10 ++
>> >> > >  drivers/pwm/Makefile       |   1 +
>> >> > >  drivers/pwm/pwm-mtk-disp.c | 256 +++++++++++++++++++++++++++++++++++++++++++++
>> >> > >  3 files changed, 267 insertions(+)
>> >> > >  create mode 100644 drivers/pwm/pwm-mtk-disp.c
>> >> > >
>> >> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> >> > > index b1541f4..f5b03a4 100644
>> >> > > --- a/drivers/pwm/Kconfig
>> >> > > +++ b/drivers/pwm/Kconfig
>> >> > > @@ -211,6 +211,16 @@ config PWM_LPSS_PLATFORM
>> >> > >           To compile this driver as a module, choose M here: the module
>> >> > >           will be called pwm-lpss-platform.
>> >> > >
>> >> > > +config PWM_MTK_DISP
>> >> > > +       tristate "MediaTek display PWM driver"
>> >> > > +       depends on ARCH_MEDIATEK || COMPILE_TEST
>> >> > > +       help
>> >> > > +         Generic PWM framework driver for MediaTek disp-pwm device.
>> >> > > +         The PWM is used to control the backlight brightness for display.
>> >> > > +
>> >> > > +         To compile this driver as a module, choose M here: the module
>> >> > > +         will be called pwm-mtk-disp.
>> >> > > +
>> >> > >  config PWM_MXS
>> >> > >         tristate "Freescale MXS PWM support"
>> >> > >         depends on ARCH_MXS && OF
>> >> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> >> > > index ec50eb5..99c9e75 100644
>> >> > > --- a/drivers/pwm/Makefile
>> >> > > +++ b/drivers/pwm/Makefile
>> >> > > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_LPC32XX)     += pwm-lpc32xx.o
>> >> > >  obj-$(CONFIG_PWM_LPSS)         += pwm-lpss.o
>> >> > >  obj-$(CONFIG_PWM_LPSS_PCI)     += pwm-lpss-pci.o
>> >> > >  obj-$(CONFIG_PWM_LPSS_PLATFORM)        += pwm-lpss-platform.o
>> >> > > +obj-$(CONFIG_PWM_MTK_DISP)     += pwm-mtk-disp.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-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
>> >> > > new file mode 100644
>> >> > > index 0000000..1f17cee
>> >> > > --- /dev/null
>> >> > > +++ b/drivers/pwm/pwm-mtk-disp.c
>> >> > > @@ -0,0 +1,256 @@
>> >> > > +/*
>> >> > > + * MediaTek display pulse-width-modulation controller driver.
>> >> > > + * Copyright (c) 2015 MediaTek Inc.
>> >> > > + * Author: YH Huang <yh.huang@mediatek.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.
>> >> > > + */
>> >> > > +
>> >> > > +#include <linux/clk.h>
>> >> > > +#include <linux/err.h>
>> >> > > +#include <linux/io.h>
>> >> > > +#include <linux/module.h>
>> >> > > +#include <linux/of.h>
>> >> > > +#include <linux/pwm.h>
>> >> > > +#include <linux/platform_device.h>
>> >> > > +#include <linux/slab.h>
>> >> > > +
>> >> > > +#define DISP_PWM_EN            0
>> >> >
>> >> > The "DISP_PWM_*" are register offsets, so use a hex value, like this:
>> >> >
>> >> > #define DISP_PWM_EN                      0x00
>> >> >
>> >> > Use BIT() for register *fields*, that is, the individual bits of a register.
>> >> >
>> >>
>> >> Got it!
>> >>
>> >> > > +#define PWM_ENABLE_MASK                BIT(0)
>> >> > > +
>> >> > > +#define DISP_PWM_COMMIT                BIT(3)
>> >> >
>> >> > #define DISP_PWM_COMMIT            0x08
>> >> >
>> >> > > +#define PWM_COMMIT_MASK                BIT(0)
>> >> > > +
>> >> > > +#define DISP_PWM_CON_0         BIT(4)
>> >> >
>> >> > #define DISP_PWM_COMMIT            0x10
>> >> >
>> >> > > +#define PWM_CLKDIV_SHIFT       16
>> >> > > +#define PWM_CLKDIV_MAX         0x3ff
>> >> > > +#define PWM_CLKDIV_MASK                (PWM_CLKDIV_MAX << PWM_CLKDIV_SHIFT)
>> >> > > +
>> >> > > +#define DISP_PWM_CON_1         0x14
>> >> > > +#define PWM_PERIOD_MASK                0xfff
>> >> > > +/* Shift log2(PWM_PERIOD_MASK + 1) as divisor */
>> >> > > +#define PWM_PERIOD_BIT_SHIFT   12
>> >> > > +
>> >> > > +#define PWM_HIGH_WIDTH_SHIFT   16
>> >> > > +#define PWM_HIGH_WIDTH_MASK    (0x1fff << PWM_HIGH_WIDTH_SHIFT)
>> >> > > +
>> >> > > +struct mtk_disp_pwm {
>> >> > > +       struct pwm_chip chip;
>> >> > > +       struct device *dev;
>> >> >
>> >> > I don't think "dev" is actually used.  And, if needed, it can be
>> >> > extracted from "chip".
>> >> >
>> >>
>> >> I will drop it.
>> >>
>> >> > > +       struct clk *clk_main;
>> >> > > +       struct clk *clk_mm;
>> >> > > +       void __iomem *base;
>> >> > > +};
>> >> > > +
>> >> > > +static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)
>> >> > > +{
>> >> > > +       return container_of(chip, struct mtk_disp_pwm, chip);
>> >> > > +}
>> >> > > +
>> >> > > +static void mtk_disp_pwm_update_bits(void __iomem *address, u32 mask, u32 value)
>> >> >
>> >> > Take "struct mtk_disp_pwm *mdp" as a param and extract mdp->base,
>> >> > rather than pass the raw iomem address.
>> >> >
>> >>
>> >> I will revise it like this.
>> >>
>> >> static void mtk_disp_pwm_update_bits(struct mtk_disp_pwm *mdp,
>> >>                              u32 reg, u32 mask, u32 value)
>> >> {
>> >>       void __iomem *address;
>> >>       u32 val;
>> >>
>> >>       address = mdp->base + reg;
>> >>       val = readl(address);
>> >>       val &= ~mask;
>> >>       val |= value;
>> >>       writel(val, address);
>> >> }
>> >>
>> >> > > +{
>> >> > > +       u32 val;
>> >> > > +
>> >> > > +       val = readl(address);
>> >> > > +       val &= ~mask;
>> >> > > +       val |= value;
>> >> > > +       writel(val, address);
>> >> > > +}
>> >> > > +
>> >> > > +static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> >> > > +                              int duty_ns, int period_ns)
>> >> > > +{
>> >> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
>> >> > > +       u64 div, rate;
>> >> > > +       u32 clk_div, period, high_width, value;
>> >> > > +
>> >> > > +       /*
>> >> > > +        * Find period, high_width and clk_div to suit duty_ns and period_ns.
>> >> > > +        * Calculate proper div value to keep period value in the bound.
>> >> > > +        *
>> >> > > +        * period_ns = 10^9 * (clk_div + 1) * (period + 1) / PWM_CLK_RATE
>> >> > > +        * duty_ns = 10^9 * (clk_div + 1) * high_width / PWM_CLK_RATE
>> >> > > +        *
>> >> > > +        * period = (PWM_CLK_RATE * period_ns) / (10^9 * (clk_div + 1)) - 1
>> >> > > +        * high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
>> >> > > +        */
>> >> > > +       rate = clk_get_rate(mdp->clk_main);
>> >> > > +       clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
>> >> > > +                         PWM_PERIOD_BIT_SHIFT;
>> >> > > +       if (clk_div > PWM_CLKDIV_MAX)
>> >> > > +               return -EINVAL;
>> >> > > +
>> >> > > +       div = NSEC_PER_SEC * (clk_div + 1);
>> >> > > +       period = div64_u64(rate * period_ns, div);
>> >> > > +       if (period > 0)
>> >> > > +               period--;
>> >> > > +
>> >> > > +       high_width = div64_u64(rate * duty_ns, div);
>> >> > > +
>> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_0,
>> >> > > +                                PWM_CLKDIV_MASK, clk_div << PWM_CLKDIV_SHIFT);
>> >> > > +
>> >> > > +       value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
>> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_CON_1,
>> >> > > +                                PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
>> >> > > +
>> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
>> >> > > +                                PWM_COMMIT_MASK, 1);
>> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_COMMIT,
>> >> > > +                                PWM_COMMIT_MASK, 0);
>> >> > > +
>> >> > > +       return 0;
>> >> > > +}
>> >> > > +
>> >> > > +static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> >> > > +{
>> >> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
>> >> > > +
>> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
>> >> > > +                                PWM_ENABLE_MASK, 1);
>> >> > > +
>> >> > > +       return 0;
>> >> > > +}
>> >> > > +
>> >> > > +static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> >> > > +{
>> >> > > +       struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
>> >> > > +
>> >> > > +       mtk_disp_pwm_update_bits(mdp->base + DISP_PWM_EN,
>> >> > > +                                PWM_ENABLE_MASK, 0);
>> >> > > +}
>> >> > > +
>> >> > > +static const struct pwm_ops mtk_disp_pwm_ops = {
>> >> > > +       .config = mtk_disp_pwm_config,
>> >> > > +       .enable = mtk_disp_pwm_enable,
>> >> > > +       .disable = mtk_disp_pwm_disable,
>> >> > > +       .owner = THIS_MODULE,
>> >> > > +};
>> >> > > +
>> >> > > +static int mtk_disp_pwm_probe(struct platform_device *pdev)
>> >> > > +{
>> >> > > +       struct mtk_disp_pwm *mdp;
>> >> > > +       struct resource *r;
>> >> > > +       int ret;
>> >> > > +
>> >> > > +       mdp = devm_kzalloc(&pdev->dev, sizeof(*mdp), GFP_KERNEL);
>> >> > > +       if (!mdp)
>> >> > > +               return -ENOMEM;
>> >> > > +
>> >> > > +       mdp->dev = &pdev->dev;
>> >> > > +
>> >> > > +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> > > +       mdp->base = devm_ioremap_resource(&pdev->dev, r);
>> >> > > +       if (IS_ERR(mdp->base))
>> >> > > +               return PTR_ERR(mdp->base);
>> >> > > +
>> >> > > +       mdp->clk_main = devm_clk_get(&pdev->dev, "main");
>> >> > > +       if (IS_ERR(mdp->clk_main))
>> >> > > +               return PTR_ERR(mdp->clk_main);
>> >> > > +
>> >> > > +       mdp->clk_mm = devm_clk_get(&pdev->dev, "mm");
>> >> > > +       if (IS_ERR(mdp->clk_mm))
>> >> > > +               return PTR_ERR(mdp->clk_mm);
>> >> > > +
>> >> > > +       ret = clk_prepare_enable(mdp->clk_main);
>> >> >
>> >> > Delay turning on the PWM clock until it is actually needed (pwm_enable)...
>> >> > Just be careful to ensure that the "main" clock is enabled when
>> >> > writing registers during mtk_disp_pwm_config.
>> >
>> > Since pwm-backlight driver configures PWM before enable PWM, I enable
>> > clocks at the probe function.
>> > Turning on the PWM clock in pwm_enable will lose the first configuration
>> > of PWM. It won't affect a lot since the system configures PWM many times
>> > at boot time.
>> > So should I delay or keep the original way?
>>
>> Why does turning on the PWM clock in pwm_enable lose the first configuration?
>> Would this be solved by turning enabling/disabling clocks during
>> mtk_disp_pwm_config()?
>>
>
> If I don't enable clocks at probe function, I enable clocks at
> mtk_disp_pwm_enable() and disable clocks at mtk_disp_pwm_disable().
> And this causes missing first configuration.
>
> Enabling clocks in mtk_disp_pwm_config() will fix this.
> The backlight doesn't work well(no immediate update or losing backlight)
> if I disable anyone of the two clocks in mtk_disp_pwm_config().
>
> Regards,
> YH Huang
>
>> >
>> >> > By the way, is the pwm in a power domain that must also be enabled
>> >> > when enabling the pwm?
>> >>
>> >> It is in the display power domain which must also be enabled.
>> >>
>> >> >
>> >> > > +       if (ret < 0)
>> >> > > +               return ret;
>> >> > > +
>> >> > > +       ret = clk_prepare_enable(mdp->clk_mm);
>> >> > > +       if (ret < 0)
>> >> > > +               goto disable_clk_main;
>> >> > > +
>> >> > > +       platform_set_drvdata(pdev, mdp);
>> >> >
>> >> > Set this only after pwmchip_add() succeeds.
>> >>
>> >> OK.
>> >>
>> >> >
>> >> > > +
>> >> > > +       mdp->chip.dev = &pdev->dev;
>> >> > > +       mdp->chip.ops = &mtk_disp_pwm_ops;
>> >> > > +       mdp->chip.base = -1;
>> >> > > +       mdp->chip.npwm = 1;
>> >> > > +
>> >> > > +       ret = pwmchip_add(&mdp->chip);
>> >> > > +       if (ret < 0) {
>> >> > > +               dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
>> >> > > +               goto disable_clk_mm;
>> >> > > +       }
>> >> > > +
>> >> > > +       return 0;
>> >> > > +
>> >> > > +disable_clk_mm:
>> >> > > +       clk_disable_unprepare(mdp->clk_mm);
>> >> > > +disable_clk_main:
>> >> > > +       clk_disable_unprepare(mdp->clk_main);
>> >> > > +       return ret;
>> >> > > +}
>> >> > > +
>> >> > > +static int mtk_disp_pwm_remove(struct platform_device *pdev)
>> >> > > +{
>> >> > > +       struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev);
>> >> > > +       int ret = pwmchip_remove(&mdp->chip);
>> >> > > +
>> >> > > +       clk_disable_unprepare(mdp->clk_main);
>> >> > > +       clk_disable_unprepare(mdp->clk_mm);
>> >> >
>> >> > Nit: it is more traditional to disable clocks in the opposite order to
>> >> > which they are enabled, so:
>> >> >
>> >> >        clk_disable_unprepare(mdp->clk_mm);
>> >> >        clk_disable_unprepare(mdp->clk_main);
>> >> >
>> >>
>> >> OK.
>> >>
>> >> > > +
>> >> > > +       return ret;
>> >> > > +}
>> >> > > +
>> >> > > +static const struct of_device_id mtk_disp_pwm_of_match[] = {
>> >> > > +       { .compatible = "mediatek,mt8173-disp-pwm" },
>> >> > > +       { .compatible = "mediatek,mt6595-disp-pwm" },
>> >> > > +       { }
>> >> > > +};
>> >> > > +MODULE_DEVICE_TABLE(of, mtk_disp_pwm_of_match);
>> >> > > +
>> >> > > +#ifdef CONFIG_PM_SLEEP
>> >> > > +static int mtk_disp_pwm_suspend(struct device *dev)
>> >> > > +{
>> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
>> >> > > +
>> >> > > +       clk_disable_unprepare(mdp->clk_main);
>> >> > > +       clk_disable_unprepare(mdp->clk_mm);
>> >> > > +
>> >> > > +       return 0;
>> >> > > +}
>> >> > > +
>> >> > > +static int mtk_disp_pwm_resume(struct device *dev)
>> >> > > +{
>> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
>> >> > > +       int ret;
>> >> > > +
>> >> > > +       ret = clk_prepare_enable(mdp->clk_main);
>> >> > > +       if (ret < 0)
>> >> > > +               return ret;
>> >> > > +
>> >> > > +       ret = clk_prepare_enable(mdp->clk_mm);
>> >> > > +       if (ret < 0) {
>> >> > > +               clk_disable_unprepare(mdp->clk_main);
>> >> > > +               return ret;
>> >> > > +       }
>> >> > > +
>> >> >
>> >> > Don't you also have to restore the PWM rate and frequency?
>> >> >
>> >> > Is it possible to save power at runtime by leaving mdp->clk_mm enabled
>> >> > (to generate the PWM signal), but disable mdp->clk_main (clock
>> >> > required to access PWM registers)?
>> >>
>> >> The pwm-backlight driver will restore the data.
>> >>
>> >> After I try to disable anyone of the two clocks at runtime, the
>> >> backlight doesn't work well(no immediate update or losing backlight).
>> >> So we need to keep both clock enabled.

Do you mean you see backlight glitch because the clocks / backlight
were *already on* during the first config (Perhaps left on by the
bootloader)?
I don't know how to solve that problem.
Maybe Thierry does.

In any case, this is a minor issue; we really shouldn't hold up
landing the driver to optimize when the clocks are enabled/disabled
:-). I'm happy enough with what you have in this patch.

-Dan

>> >>
>> >> Regards,
>> >> YH Huang
>> >>
>> >
>> >
>
>

Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[YH Huang]

On Thu, 2015-07-16 at 23:21 +0800, Daniel Kurtz wrote:
> On Thu, Jul 16, 2015 at 3:17 PM, YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> > On Thu, 2015-07-16 at 14:54 +0800, Daniel Kurtz wrote:
> >> On Thu, Jul 16, 2015 at 1:38 PM, YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >> > On Wed, 2015-07-15 at 23:59 +0800, YH Huang wrote:
> >> >> On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:
> >> >> > On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >> >> > > +#ifdef CONFIG_PM_SLEEP
> >> >> > > +static int mtk_disp_pwm_suspend(struct device *dev)
> >> >> > > +{
> >> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> >> >> > > +
> >> >> > > +       clk_disable_unprepare(mdp->clk_main);
> >> >> > > +       clk_disable_unprepare(mdp->clk_mm);
> >> >> > > +
> >> >> > > +       return 0;
> >> >> > > +}
> >> >> > > +
> >> >> > > +static int mtk_disp_pwm_resume(struct device *dev)
> >> >> > > +{
> >> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> >> >> > > +       int ret;
> >> >> > > +
> >> >> > > +       ret = clk_prepare_enable(mdp->clk_main);
> >> >> > > +       if (ret < 0)
> >> >> > > +               return ret;
> >> >> > > +
> >> >> > > +       ret = clk_prepare_enable(mdp->clk_mm);
> >> >> > > +       if (ret < 0) {
> >> >> > > +               clk_disable_unprepare(mdp->clk_main);
> >> >> > > +               return ret;
> >> >> > > +       }
> >> >> > > +
> >> >> >
> >> >> > Don't you also have to restore the PWM rate and frequency?
> >> >> >
> >> >> > Is it possible to save power at runtime by leaving mdp->clk_mm enabled
> >> >> > (to generate the PWM signal), but disable mdp->clk_main (clock
> >> >> > required to access PWM registers)?
> >> >>
> >> >> The pwm-backlight driver will restore the data.
> >> >>
> >> >> After I try to disable anyone of the two clocks at runtime, the
> >> >> backlight doesn't work well(no immediate update or losing backlight).
> >> >> So we need to keep both clock enabled.
> 
> Do you mean you see backlight glitch because the clocks / backlight
> were *already on* during the first config (Perhaps left on by the
> bootloader)?
> I don't know how to solve that problem.
> Maybe Thierry does.
> 
> In any case, this is a minor issue; we really shouldn't hold up
> landing the driver to optimize when the clocks are enabled/disabled
> :-). I'm happy enough with what you have in this patch.

Sorry for my terrible expression. Let me try again.
1. We want to disable unnecessary clock at runtime. 
But, I get backlight glitch when I disable clk_main or clk_mm in
mtk_disp_pwm_config(). So both clocks are necessary and we don't disable
them at runtime. 

2. Because pwm-backlight driver calls mtk_disp_pwm_config() before
mtk_disp_pwm_enable(), we will lose the first config if clocks are
enabled in mtk_disp_pwm_enable(). I prefer to enable clocks in probe
function. Samsung did the same way in their pwm driver.

Thanks for your kindly suggestions. I will update the patch soon.

Regards,
YH Huang


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[Daniel Kurtz]

On Fri, Jul 17, 2015 at 12:44 AM, YH Huang <yh.huang@mediatek.com> wrote:
> On Thu, 2015-07-16 at 23:21 +0800, Daniel Kurtz wrote:
>> On Thu, Jul 16, 2015 at 3:17 PM, YH Huang <yh.huang@mediatek.com> wrote:
>> > On Thu, 2015-07-16 at 14:54 +0800, Daniel Kurtz wrote:
>> >> On Thu, Jul 16, 2015 at 1:38 PM, YH Huang <yh.huang@mediatek.com> wrote:
>> >> > On Wed, 2015-07-15 at 23:59 +0800, YH Huang wrote:
>> >> >> On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:
>> >> >> > On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang@mediatek.com> wrote:
>> >> >> > > +#ifdef CONFIG_PM_SLEEP
>> >> >> > > +static int mtk_disp_pwm_suspend(struct device *dev)
>> >> >> > > +{
>> >> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
>> >> >> > > +
>> >> >> > > +       clk_disable_unprepare(mdp->clk_main);
>> >> >> > > +       clk_disable_unprepare(mdp->clk_mm);
>> >> >> > > +
>> >> >> > > +       return 0;
>> >> >> > > +}
>> >> >> > > +
>> >> >> > > +static int mtk_disp_pwm_resume(struct device *dev)
>> >> >> > > +{
>> >> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
>> >> >> > > +       int ret;
>> >> >> > > +
>> >> >> > > +       ret = clk_prepare_enable(mdp->clk_main);
>> >> >> > > +       if (ret < 0)
>> >> >> > > +               return ret;
>> >> >> > > +
>> >> >> > > +       ret = clk_prepare_enable(mdp->clk_mm);
>> >> >> > > +       if (ret < 0) {
>> >> >> > > +               clk_disable_unprepare(mdp->clk_main);
>> >> >> > > +               return ret;
>> >> >> > > +       }
>> >> >> > > +
>> >> >> >
>> >> >> > Don't you also have to restore the PWM rate and frequency?
>> >> >> >
>> >> >> > Is it possible to save power at runtime by leaving mdp->clk_mm enabled
>> >> >> > (to generate the PWM signal), but disable mdp->clk_main (clock
>> >> >> > required to access PWM registers)?
>> >> >>
>> >> >> The pwm-backlight driver will restore the data.
>> >> >>
>> >> >> After I try to disable anyone of the two clocks at runtime, the
>> >> >> backlight doesn't work well(no immediate update or losing backlight).
>> >> >> So we need to keep both clock enabled.
>>
>> Do you mean you see backlight glitch because the clocks / backlight
>> were *already on* during the first config (Perhaps left on by the
>> bootloader)?
>> I don't know how to solve that problem.
>> Maybe Thierry does.
>>
>> In any case, this is a minor issue; we really shouldn't hold up
>> landing the driver to optimize when the clocks are enabled/disabled
>> :-). I'm happy enough with what you have in this patch.
>
> Sorry for my terrible expression. Let me try again.
> 1. We want to disable unnecessary clock at runtime.
> But, I get backlight glitch when I disable clk_main or clk_mm in
> mtk_disp_pwm_config(). So both clocks are necessary and we don't disable
> them at runtime.
>
> 2. Because pwm-backlight driver calls mtk_disp_pwm_config() before
> mtk_disp_pwm_enable(), we will lose the first config if clocks are
> enabled in mtk_disp_pwm_enable(). I prefer to enable clocks in probe
> function. Samsung did the same way in their pwm driver.

I don't understand why you will "lose the first config if clocks are
enabled in mtk_disp_pwm_enable().  I don't believe registers will lose
their values just because you turn enable/disable clocks.

Perhaps I wasn't clear with what I was proposing which is something like this:

mtk_disp_pwm_config()
{
  clk_enable();
  /* write registers */
  clk_disable();
}

mtk_disp_enable()
{
  clk_enable();
  /* write enable bit */
}

mtk_disp_disable()
{
  /* clear enable bit */
  clk_disable();
}


In this way, if mtk_disp_pwm_config() is called when the pwm is
disabled, we will temporarily enable the clocks long enough to update
the register values.  These values should take effect the next time
the PWM is enabled.  We then disable the clocks and wait for the PWM
to be enabled.

If mtk_disp_pwm_config() is called when the pwm is already enabled, we
will increment the enable count on the clocks, but then we decrement
it again immediately.

Thanks,
-Dan



>
> Thanks for your kindly suggestions. I will update the patch soon.
>
> Regards,
> YH Huang
>
>

Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[YH Huang]

On Fri, 2015-07-17 at 01:18 +0800, Daniel Kurtz wrote:
> On Fri, Jul 17, 2015 at 12:44 AM, YH Huang <yh.huang@mediatek.com> wrote:
> > On Thu, 2015-07-16 at 23:21 +0800, Daniel Kurtz wrote:
> >> On Thu, Jul 16, 2015 at 3:17 PM, YH Huang <yh.huang@mediatek.com> wrote:
> >> > On Thu, 2015-07-16 at 14:54 +0800, Daniel Kurtz wrote:
> >> >> On Thu, Jul 16, 2015 at 1:38 PM, YH Huang <yh.huang@mediatek.com> wrote:
> >> >> > On Wed, 2015-07-15 at 23:59 +0800, YH Huang wrote:
> >> >> >> On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:
> >> >> >> > On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang@mediatek.com> wrote:
> >> >> >> > > +#ifdef CONFIG_PM_SLEEP
> >> >> >> > > +static int mtk_disp_pwm_suspend(struct device *dev)
> >> >> >> > > +{
> >> >> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> >> >> >> > > +
> >> >> >> > > +       clk_disable_unprepare(mdp->clk_main);
> >> >> >> > > +       clk_disable_unprepare(mdp->clk_mm);
> >> >> >> > > +
> >> >> >> > > +       return 0;
> >> >> >> > > +}
> >> >> >> > > +
> >> >> >> > > +static int mtk_disp_pwm_resume(struct device *dev)
> >> >> >> > > +{
> >> >> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> >> >> >> > > +       int ret;
> >> >> >> > > +
> >> >> >> > > +       ret = clk_prepare_enable(mdp->clk_main);
> >> >> >> > > +       if (ret < 0)
> >> >> >> > > +               return ret;
> >> >> >> > > +
> >> >> >> > > +       ret = clk_prepare_enable(mdp->clk_mm);
> >> >> >> > > +       if (ret < 0) {
> >> >> >> > > +               clk_disable_unprepare(mdp->clk_main);
> >> >> >> > > +               return ret;
> >> >> >> > > +       }
> >> >> >> > > +
> >> >> >> >
> >> >> >> > Don't you also have to restore the PWM rate and frequency?
> >> >> >> >
> >> >> >> > Is it possible to save power at runtime by leaving mdp->clk_mm enabled
> >> >> >> > (to generate the PWM signal), but disable mdp->clk_main (clock
> >> >> >> > required to access PWM registers)?
> >> >> >>
> >> >> >> The pwm-backlight driver will restore the data.
> >> >> >>
> >> >> >> After I try to disable anyone of the two clocks at runtime, the
> >> >> >> backlight doesn't work well(no immediate update or losing backlight).
> >> >> >> So we need to keep both clock enabled.
> >>
> >> Do you mean you see backlight glitch because the clocks / backlight
> >> were *already on* during the first config (Perhaps left on by the
> >> bootloader)?
> >> I don't know how to solve that problem.
> >> Maybe Thierry does.
> >>
> >> In any case, this is a minor issue; we really shouldn't hold up
> >> landing the driver to optimize when the clocks are enabled/disabled
> >> :-). I'm happy enough with what you have in this patch.
> >
> > Sorry for my terrible expression. Let me try again.
> > 1. We want to disable unnecessary clock at runtime.
> > But, I get backlight glitch when I disable clk_main or clk_mm in
> > mtk_disp_pwm_config(). So both clocks are necessary and we don't disable
> > them at runtime.
> >
> > 2. Because pwm-backlight driver calls mtk_disp_pwm_config() before
> > mtk_disp_pwm_enable(), we will lose the first config if clocks are
> > enabled in mtk_disp_pwm_enable(). I prefer to enable clocks in probe
> > function. Samsung did the same way in their pwm driver.
> 
> I don't understand why you will "lose the first config if clocks are
> enabled in mtk_disp_pwm_enable().  I don't believe registers will lose
> their values just because you turn enable/disable clocks.
> 
> Perhaps I wasn't clear with what I was proposing which is something like this:
> 
> mtk_disp_pwm_config()
> {
>   clk_enable();
>   /* write registers */
>   clk_disable();
> }
> 
> mtk_disp_enable()
> {
>   clk_enable();
>   /* write enable bit */
> }
> 
> mtk_disp_disable()
> {
>   /* clear enable bit */
>   clk_disable();
> }
> 
> 
> In this way, if mtk_disp_pwm_config() is called when the pwm is
> disabled, we will temporarily enable the clocks long enough to update
> the register values.  These values should take effect the next time
> the PWM is enabled.  We then disable the clocks and wait for the PWM
> to be enabled.
> 
> If mtk_disp_pwm_config() is called when the pwm is already enabled, we
> will increment the enable count on the clocks, but then we decrement
> it again immediately.

I think there is something wrong about mdp->clk_mm(generate the PWM
signal) and mdp->clk_main(access PWM registers).
If we want to write the register, we should enable both clocks before.
I try it in this way.

mtk_disp_pwm_config()
{
   clk_enable(mdp->clk_main);
   clk_enable(mdp->clk_mm);
   /* write registers */
   clk_disable(mdp->clk_mm);
   clk_disable(mdp->clk_main);
}
with
A:
mtk_disp_enable()
{
   clk_enable(mdp->clk_main);
   clk_enable(mdp->clk_mm);
   /* write enable bit */
   clk_disable(mdp->clk_mm);
}
or
B:
mtk_disp_enable()
{
   clk_enable(mdp->clk_main);
   clk_enable(mdp->clk_mm);
   /* write enable bit */
   clk_disable(mdp->clk_main);
}

I both get backlight glitch with "A" or "B".
So I think we should keep clocks enabled.

Regards,
YH Huang

Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[Daniel Kurtz]

On Fri, Jul 17, 2015 at 2:35 PM, YH Huang <yh.huang@mediatek.com> wrote:
>
> On Fri, 2015-07-17 at 01:18 +0800, Daniel Kurtz wrote:
> > On Fri, Jul 17, 2015 at 12:44 AM, YH Huang <yh.huang@mediatek.com> wrote:
> > > On Thu, 2015-07-16 at 23:21 +0800, Daniel Kurtz wrote:
> > >> On Thu, Jul 16, 2015 at 3:17 PM, YH Huang <yh.huang@mediatek.com> wrote:
> > >> > On Thu, 2015-07-16 at 14:54 +0800, Daniel Kurtz wrote:
> > >> >> On Thu, Jul 16, 2015 at 1:38 PM, YH Huang <yh.huang@mediatek.com> wrote:
> > >> >> > On Wed, 2015-07-15 at 23:59 +0800, YH Huang wrote:
> > >> >> >> On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:
> > >> >> >> > On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang@mediatek.com> wrote:
> > >> >> >> > > +#ifdef CONFIG_PM_SLEEP
> > >> >> >> > > +static int mtk_disp_pwm_suspend(struct device *dev)
> > >> >> >> > > +{
> > >> >> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> > >> >> >> > > +
> > >> >> >> > > +       clk_disable_unprepare(mdp->clk_main);
> > >> >> >> > > +       clk_disable_unprepare(mdp->clk_mm);
> > >> >> >> > > +
> > >> >> >> > > +       return 0;
> > >> >> >> > > +}
> > >> >> >> > > +
> > >> >> >> > > +static int mtk_disp_pwm_resume(struct device *dev)
> > >> >> >> > > +{
> > >> >> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> > >> >> >> > > +       int ret;
> > >> >> >> > > +
> > >> >> >> > > +       ret = clk_prepare_enable(mdp->clk_main);
> > >> >> >> > > +       if (ret < 0)
> > >> >> >> > > +               return ret;
> > >> >> >> > > +
> > >> >> >> > > +       ret = clk_prepare_enable(mdp->clk_mm);
> > >> >> >> > > +       if (ret < 0) {
> > >> >> >> > > +               clk_disable_unprepare(mdp->clk_main);
> > >> >> >> > > +               return ret;
> > >> >> >> > > +       }
> > >> >> >> > > +
> > >> >> >> >
> > >> >> >> > Don't you also have to restore the PWM rate and frequency?
> > >> >> >> >
> > >> >> >> > Is it possible to save power at runtime by leaving mdp->clk_mm enabled
> > >> >> >> > (to generate the PWM signal), but disable mdp->clk_main (clock
> > >> >> >> > required to access PWM registers)?
> > >> >> >>
> > >> >> >> The pwm-backlight driver will restore the data.
> > >> >> >>
> > >> >> >> After I try to disable anyone of the two clocks at runtime, the
> > >> >> >> backlight doesn't work well(no immediate update or losing backlight).
> > >> >> >> So we need to keep both clock enabled.
> > >>
> > >> Do you mean you see backlight glitch because the clocks / backlight
> > >> were *already on* during the first config (Perhaps left on by the
> > >> bootloader)?
> > >> I don't know how to solve that problem.
> > >> Maybe Thierry does.
> > >>
> > >> In any case, this is a minor issue; we really shouldn't hold up
> > >> landing the driver to optimize when the clocks are enabled/disabled
> > >> :-). I'm happy enough with what you have in this patch.
> > >
> > > Sorry for my terrible expression. Let me try again.
> > > 1. We want to disable unnecessary clock at runtime.
> > > But, I get backlight glitch when I disable clk_main or clk_mm in
> > > mtk_disp_pwm_config(). So both clocks are necessary and we don't disable
> > > them at runtime.
> > >
> > > 2. Because pwm-backlight driver calls mtk_disp_pwm_config() before
> > > mtk_disp_pwm_enable(), we will lose the first config if clocks are
> > > enabled in mtk_disp_pwm_enable(). I prefer to enable clocks in probe
> > > function. Samsung did the same way in their pwm driver.
> >
> > I don't understand why you will "lose the first config if clocks are
> > enabled in mtk_disp_pwm_enable().  I don't believe registers will lose
> > their values just because you turn enable/disable clocks.
> >
> > Perhaps I wasn't clear with what I was proposing which is something like this:
> >
> > mtk_disp_pwm_config()
> > {
> >   clk_enable();
> >   /* write registers */
> >   clk_disable();
> > }
> >
> > mtk_disp_enable()
> > {
> >   clk_enable();
> >   /* write enable bit */
> > }
> >
> > mtk_disp_disable()
> > {
> >   /* clear enable bit */
> >   clk_disable();
> > }
> >
> >
> > In this way, if mtk_disp_pwm_config() is called when the pwm is
> > disabled, we will temporarily enable the clocks long enough to update
> > the register values.  These values should take effect the next time
> > the PWM is enabled.  We then disable the clocks and wait for the PWM
> > to be enabled.
> >
> > If mtk_disp_pwm_config() is called when the pwm is already enabled, we
> > will increment the enable count on the clocks, but then we decrement
> > it again immediately.
>
> I think there is something wrong about mdp->clk_mm(generate the PWM
> signal) and mdp->clk_main(access PWM registers).
> If we want to write the register, we should enable both clocks before.
> I try it in this way.
>
> mtk_disp_pwm_config()
> {
>    clk_enable(mdp->clk_main);
>    clk_enable(mdp->clk_mm);
>    /* write registers */
>    clk_disable(mdp->clk_mm);
>    clk_disable(mdp->clk_main);
> }
> with
> A:
> mtk_disp_enable()
> {
>    clk_enable(mdp->clk_main);
>    clk_enable(mdp->clk_mm);
>    /* write enable bit */
>    clk_disable(mdp->clk_mm);
> }
> or
> B:
> mtk_disp_enable()
> {
>    clk_enable(mdp->clk_main);
>    clk_enable(mdp->clk_mm);
>    /* write enable bit */
>    clk_disable(mdp->clk_main);
> }
>
> I both get backlight glitch with "A" or "B".
> So I think we should keep clocks enabled.


Ok, but what about this:

mtk_disp_pwm_config()
{
   clk_enable(mdp->clk_main);
   /* write registers */
   clk_disable(mdp->clk_main);
}

mtk_disp_enable()
{
   clk_enable(mdp->clk_main);
   clk_enable(mdp->clk_mm);
   /* write enable bit */
}

>
>
> Regards,
> YH Huang
>

Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

[YH Huang]

On Fri, 2015-07-17 at 14:59 +0800, Daniel Kurtz wrote:
> On Fri, Jul 17, 2015 at 2:35 PM, YH Huang <yh.huang@mediatek.com> wrote:
> >
> > On Fri, 2015-07-17 at 01:18 +0800, Daniel Kurtz wrote:
> > > On Fri, Jul 17, 2015 at 12:44 AM, YH Huang <yh.huang@mediatek.com> wrote:
> > > > On Thu, 2015-07-16 at 23:21 +0800, Daniel Kurtz wrote:
> > > >> On Thu, Jul 16, 2015 at 3:17 PM, YH Huang <yh.huang@mediatek.com> wrote:
> > > >> > On Thu, 2015-07-16 at 14:54 +0800, Daniel Kurtz wrote:
> > > >> >> On Thu, Jul 16, 2015 at 1:38 PM, YH Huang <yh.huang@mediatek.com> wrote:
> > > >> >> > On Wed, 2015-07-15 at 23:59 +0800, YH Huang wrote:
> > > >> >> >> On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:
> > > >> >> >> > On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang@mediatek.com> wrote:
> > > >> >> >> > > +#ifdef CONFIG_PM_SLEEP
> > > >> >> >> > > +static int mtk_disp_pwm_suspend(struct device *dev)
> > > >> >> >> > > +{
> > > >> >> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> > > >> >> >> > > +
> > > >> >> >> > > +       clk_disable_unprepare(mdp->clk_main);
> > > >> >> >> > > +       clk_disable_unprepare(mdp->clk_mm);
> > > >> >> >> > > +
> > > >> >> >> > > +       return 0;
> > > >> >> >> > > +}
> > > >> >> >> > > +
> > > >> >> >> > > +static int mtk_disp_pwm_resume(struct device *dev)
> > > >> >> >> > > +{
> > > >> >> >> > > +       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
> > > >> >> >> > > +       int ret;
> > > >> >> >> > > +
> > > >> >> >> > > +       ret = clk_prepare_enable(mdp->clk_main);
> > > >> >> >> > > +       if (ret < 0)
> > > >> >> >> > > +               return ret;
> > > >> >> >> > > +
> > > >> >> >> > > +       ret = clk_prepare_enable(mdp->clk_mm);
> > > >> >> >> > > +       if (ret < 0) {
> > > >> >> >> > > +               clk_disable_unprepare(mdp->clk_main);
> > > >> >> >> > > +               return ret;
> > > >> >> >> > > +       }
> > > >> >> >> > > +
> > > >> >> >> >
> > > >> >> >> > Don't you also have to restore the PWM rate and frequency?
> > > >> >> >> >
> > > >> >> >> > Is it possible to save power at runtime by leaving mdp->clk_mm enabled
> > > >> >> >> > (to generate the PWM signal), but disable mdp->clk_main (clock
> > > >> >> >> > required to access PWM registers)?
> > > >> >> >>
> > > >> >> >> The pwm-backlight driver will restore the data.
> > > >> >> >>
> > > >> >> >> After I try to disable anyone of the two clocks at runtime, the
> > > >> >> >> backlight doesn't work well(no immediate update or losing backlight).
> > > >> >> >> So we need to keep both clock enabled.
> > > >>
> > > >> Do you mean you see backlight glitch because the clocks / backlight
> > > >> were *already on* during the first config (Perhaps left on by the
> > > >> bootloader)?
> > > >> I don't know how to solve that problem.
> > > >> Maybe Thierry does.
> > > >>
> > > >> In any case, this is a minor issue; we really shouldn't hold up
> > > >> landing the driver to optimize when the clocks are enabled/disabled
> > > >> :-). I'm happy enough with what you have in this patch.
> > > >
> > > > Sorry for my terrible expression. Let me try again.
> > > > 1. We want to disable unnecessary clock at runtime.
> > > > But, I get backlight glitch when I disable clk_main or clk_mm in
> > > > mtk_disp_pwm_config(). So both clocks are necessary and we don't disable
> > > > them at runtime.
> > > >
> > > > 2. Because pwm-backlight driver calls mtk_disp_pwm_config() before
> > > > mtk_disp_pwm_enable(), we will lose the first config if clocks are
> > > > enabled in mtk_disp_pwm_enable(). I prefer to enable clocks in probe
> > > > function. Samsung did the same way in their pwm driver.
> > >
> > > I don't understand why you will "lose the first config if clocks are
> > > enabled in mtk_disp_pwm_enable().  I don't believe registers will lose
> > > their values just because you turn enable/disable clocks.
> > >
> > > Perhaps I wasn't clear with what I was proposing which is something like this:
> > >
> > > mtk_disp_pwm_config()
> > > {
> > >   clk_enable();
> > >   /* write registers */
> > >   clk_disable();
> > > }
> > >
> > > mtk_disp_enable()
> > > {
> > >   clk_enable();
> > >   /* write enable bit */
> > > }
> > >
> > > mtk_disp_disable()
> > > {
> > >   /* clear enable bit */
> > >   clk_disable();
> > > }
> > >
> > >
> > > In this way, if mtk_disp_pwm_config() is called when the pwm is
> > > disabled, we will temporarily enable the clocks long enough to update
> > > the register values.  These values should take effect the next time
> > > the PWM is enabled.  We then disable the clocks and wait for the PWM
> > > to be enabled.
> > >
> > > If mtk_disp_pwm_config() is called when the pwm is already enabled, we
> > > will increment the enable count on the clocks, but then we decrement
> > > it again immediately.
> >
> > I think there is something wrong about mdp->clk_mm(generate the PWM
> > signal) and mdp->clk_main(access PWM registers).
> > If we want to write the register, we should enable both clocks before.
> > I try it in this way.
> >
> > mtk_disp_pwm_config()
> > {
> >    clk_enable(mdp->clk_main);
> >    clk_enable(mdp->clk_mm);
> >    /* write registers */
> >    clk_disable(mdp->clk_mm);
> >    clk_disable(mdp->clk_main);
> > }
> > with
> > A:
> > mtk_disp_enable()
> > {
> >    clk_enable(mdp->clk_main);
> >    clk_enable(mdp->clk_mm);
> >    /* write enable bit */
> >    clk_disable(mdp->clk_mm);
> > }
> > or
> > B:
> > mtk_disp_enable()
> > {
> >    clk_enable(mdp->clk_main);
> >    clk_enable(mdp->clk_mm);
> >    /* write enable bit */
> >    clk_disable(mdp->clk_main);
> > }
> >
> > I both get backlight glitch with "A" or "B".
> > So I think we should keep clocks enabled.
> 
> 
> Ok, but what about this:
> 
> mtk_disp_pwm_config()
> {
>    clk_enable(mdp->clk_main);
>    /* write registers */
>    clk_disable(mdp->clk_main);
> }
> 
> mtk_disp_enable()
> {
>    clk_enable(mdp->clk_main);
>    clk_enable(mdp->clk_mm);
>    /* write enable bit */
> }

The pwm-backlight driver update status with this order:
pwm_config(pb->pwm, duty_cycle, pb->period);
pwm_backlight_power_on(pb, brightness);

So we call mtk_disp_pwm_config() first and then mtk_disp_enable().

At the first time we call mtk_disp_pwm_config() from the situation that
PWM is disable, we can't write registers because mdp->clk_mm isn't
enabled yet.

Revising code like this can get the right behavior.
mtk_disp_pwm_config()
{
    clk_enable(mdp->clk_main);
    clk_enable(mdp->clk_mm);
    /* write registers */
    clk_disable(mdp->clk_mm);
    clk_disable(mdp->clk_main);
}

Regards,
YH Huang

[PATCH v5 3/3] arm64: dts: mt8173: add MT8173 display PWM driver support node

[YH Huang]

Add display PWM node in mt8173-evb.dts and mt8173.dtsi.

Signed-off-by: YH Huang <yh.huang@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 15 +++++++++++++++
 arch/arm64/boot/dts/mediatek/mt8173.dtsi    | 22 ++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
index f433c21..b7c1d9f 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
@@ -34,6 +34,21 @@
 	chosen { };
 };
 
+&pio {
+	disp_pwm0_pins: disp_pwm0_pins {
+		pins1 {
+			pinmux = <MT8173_PIN_87_DISP_PWM0__FUNC_DISP_PWM0>;
+			bias-pull-up;
+		};
+	};
+};
+
+&pwm0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&disp_pwm0_pins>;
+	status = "okay";
+};
+
 &pwrap {
 	pmic: mt6397 {
 		compatible = "mediatek,mt6397";
diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 0696f8f..44cab19 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -393,6 +393,28 @@
 			#size-cells = <0>;
 			status = "disabled";
 		};
+
+		pwm0: pwm@1401e000 {
+			compatible = "mediatek,mt8173-disp-pwm",
+				     "mediatek,mt6595-disp-pwm";
+			reg = <0 0x1401e000 0 0x1000>;
+			#pwm-cells = <2>;
+			clocks = <&mmsys CLK_MM_DISP_PWM026M>,
+				 <&mmsys CLK_MM_DISP_PWM0MM>;
+			clock-names = "main", "mm";
+			status = "disabled";
+		};
+
+		pwm1: pwm@1401f000 {
+			compatible = "mediatek,mt8173-disp-pwm",
+				     "mediatek,mt6595-disp-pwm";
+			reg = <0 0x1401f000 0 0x1000>;
+			#pwm-cells = <2>;
+			clocks = <&mmsys CLK_MM_DISP_PWM126M>,
+				 <&mmsys CLK_MM_DISP_PWM1MM>;
+			clock-names = "main", "mm";
+			status = "disabled";
+		};
 	};
 };
 
-- 
1.8.1.1.dirty