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

[PATCH v6 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 v6:
1. Enable clocks in the pwm_enable function
2. Remove suspend/resume code since pwm-backlight driver has done the same things
3. Revise some code to make it easier to read

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       |  42 ++++
 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                         | 232 +++++++++++++++++++++
 6 files changed, 322 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 v6 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@mediatek.com>
---
 .../devicetree/bindings/pwm/pwm-mtk-disp.txt       | 42 ++++++++++++++++++++++
 1 file changed, 42 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..f8f59ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
@@ -0,0 +1,42 @@
+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>;
+	};
+
+	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>;
+	};
-- 
1.8.1.1.dirty

[PATCH v6 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 | 232 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 243 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..69682a0
--- /dev/null
+++ b/drivers/pwm/pwm-mtk-disp.c
@@ -0,0 +1,232 @@
+/*
+ * 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		0x00
+#define PWM_ENABLE_MASK		BIT(0)
+
+#define DISP_PWM_COMMIT		0x08
+#define PWM_COMMIT_MASK		BIT(0)
+
+#define DISP_PWM_CON_0		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 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(struct mtk_disp_pwm *mdp, u32 offset,
+				     u32 mask, u32 value)
+{
+	void __iomem *address;
+	u32 val;
+
+	address = mdp->base + offset;
+	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);
+	value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
+
+	clk_enable(mdp->clk_main);
+	clk_enable(mdp->clk_mm);
+
+	mtk_disp_pwm_update_bits(mdp, DISP_PWM_CON_0,
+				 PWM_CLKDIV_MASK, clk_div << PWM_CLKDIV_SHIFT);
+	mtk_disp_pwm_update_bits(mdp, DISP_PWM_CON_1,
+				 PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
+	mtk_disp_pwm_update_bits(mdp, DISP_PWM_COMMIT,
+				 PWM_COMMIT_MASK, 1);
+	mtk_disp_pwm_update_bits(mdp, DISP_PWM_COMMIT,
+				 PWM_COMMIT_MASK, 0);
+
+	clk_disable(mdp->clk_mm);
+	clk_disable(mdp->clk_main);
+
+	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);
+
+	clk_enable(mdp->clk_main);
+	clk_enable(mdp->clk_mm);
+
+	mtk_disp_pwm_update_bits(mdp, 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, DISP_PWM_EN,
+				 PWM_ENABLE_MASK, 0);
+
+	clk_disable(mdp->clk_mm);
+	clk_disable(mdp->clk_main);
+}
+
+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;
+
+	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(mdp->clk_main);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_prepare(mdp->clk_mm);
+	if (ret < 0)
+		goto disable_clk_main;
+
+	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;
+	}
+
+	platform_set_drvdata(pdev, mdp);
+
+	return 0;
+
+disable_clk_mm:
+	clk_unprepare(mdp->clk_mm);
+disable_clk_main:
+	clk_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_unprepare(mdp->clk_mm);
+	clk_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);
+
+static struct platform_driver mtk_disp_pwm_driver = {
+	.driver = {
+		.name = "mediatek-disp-pwm",
+		.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

[PATCH v6 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

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

[Daniel Kurtz]

On Mon, Jul 20, 2015 at 4:17 PM, YH Huang <yh.huang@mediatek.com> wrote:
> Add display PWM node in mt8173-evb.dts and mt8173.dtsi.
>
> Signed-off-by: YH Huang <yh.huang@mediatek.com>

For the series:
Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>

> ---
>  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
>
> --
> 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 v6 1/3] dt-bindings: pwm: add MediaTek display PWM bindings

[Matthias Brugger]

On Monday, July 20, 2015 04:17:15 PM YH Huang 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       | 42
> ++++++++++++++++++++++ 1 file changed, 42 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..f8f59ba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> @@ -0,0 +1,42 @@
> +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.

I had another look on the mt6589 datasheet and for me it doesn't look like as 
if this drivers is compatible to mt6589.

DISP_PWM_CON_0 offset 0x10 maps to interrupt enable register and 
DISP_PWM_CON_1 offset 0x14 maps to interrupt status register.

This looks wrong to me, as you use both registers to write clock divider and 
clock period.

Regarding that this is v6 of the patch set, I would propose that you just drop 
the compatible string for mt6589 or you implement the register offset on basis 
of the compatible string so that mt6589 can you the driver as well.

Best regards,
Matthias

Re: [PATCH v6 0/3] Add MediaTek display PWM driver

[Matthias Brugger]

On Monday, July 20, 2015 04:17:14 PM YH Huang wrote:
> 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 v6:
> 1. Enable clocks in the pwm_enable function
> 2. Remove suspend/resume code since pwm-backlight driver has done the same
> things 3. Revise some code to make it easier to read
> 

Can you please provide the whole change log for all versions the next time.
It's easier this way to understand the evolution of the series.

Thanks,
Matthias

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

[Daniel Kurtz]

On Fri, Jul 24, 2015 at 4:40 PM, Matthias Brugger
<matthias.bgg@gmail.com> wrote:
> On Monday, July 20, 2015 04:17:15 PM YH Huang 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       | 42
>> ++++++++++++++++++++++ 1 file changed, 42 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..f8f59ba
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
>> @@ -0,0 +1,42 @@
>> +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.
>
> I had another look on the mt6589 datasheet and for me it doesn't look like as
> if this drivers is compatible to mt6589.

Matthias - the compatible is "mt6595", not mt6589 :-).
Which datasheet did you check?

-Dan


>
> DISP_PWM_CON_0 offset 0x10 maps to interrupt enable register and
> DISP_PWM_CON_1 offset 0x14 maps to interrupt status register.
>
> This looks wrong to me, as you use both registers to write clock divider and
> clock period.
>
> Regarding that this is v6 of the patch set, I would propose that you just drop
> the compatible string for mt6589 or you implement the register offset on basis
> of the compatible string so that mt6589 can you the driver as well.
>
> Best regards,
> Matthias
> --
> 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 v6 0/3] Add MediaTek display PWM driver

[YH Huang]

On Fri, 2015-07-24 at 10:42 +0200, Matthias Brugger wrote:
> On Monday, July 20, 2015 04:17:14 PM YH Huang wrote:
> > 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 v6:
> > 1. Enable clocks in the pwm_enable function
> > 2. Remove suspend/resume code since pwm-backlight driver has done the same
> > things 3. Revise some code to make it easier to read
> > 
> 
> Can you please provide the whole change log for all versions the next time.
> It's easier this way to understand the evolution of the series.

Change in v6:
1. Enable clocks in the pwm_enable function.
2. Remove suspend/resume code since pwm-backlight driver has done the
same things.
3. Revise some code to make it easier to read.

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

Change in v4:
1. Codebase is on v4.2-rc1.
2. Add the PWM node in dtsi.
3. Change the dependency in Kconfig.
4. Rewrite some code for readability.

Change in v3:
1. Add suspend/resume function.
2. Fix the formula for high_width calculation.
3. Rewrite some code to make it easier to read.
4. Add more information in the commit message.

Change in v2:
1. Rewrite descriptions for driver in kconfig to make it much clear.
2. Rename the driver from "pwm-mediatek-disp" to "pwm-mtk-disp".
3. Disable clocks in the error path.
4. Change the shift values in decimal instead of hex.
5. Rename and add some variables or function name for consistency and
readability.
6. Drop unnecessary parentheses, spaces, variables and add newlines to
make the code easiler to read.

Thanks for your suggestion.

Regards,
YH Huang

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

[Matthias Brugger]

On Friday, July 24, 2015 05:00:13 PM Daniel Kurtz wrote:
> On Fri, Jul 24, 2015 at 4:40 PM, Matthias Brugger
> 
> <matthias.bgg@gmail.com> wrote:
> > On Monday, July 20, 2015 04:17:15 PM YH Huang 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       | 42
> >> 
> >> ++++++++++++++++++++++ 1 file changed, 42 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..f8f59ba
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> >> @@ -0,0 +1,42 @@
> >> +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.
> > 
> > I had another look on the mt6589 datasheet and for me it doesn't look like
> > as if this drivers is compatible to mt6589.
> 
> Matthias - the compatible is "mt6595", not mt6589 :-).
> Which datasheet did you check?
> 

In further versions the driver stated it support mt6589, so I didn' realize 
that it is mt6595 now. Logically I checked mt6589 datasheet... :(

Sorry for the noise!

Re: [PATCH v6 0/3] Add MediaTek display PWM driver

[YH Huang]

On Fri, 2015-07-24 at 17:10 +0800, YH Huang wrote:
> On Fri, 2015-07-24 at 10:42 +0200, Matthias Brugger wrote:
> > On Monday, July 20, 2015 04:17:14 PM YH Huang wrote:
> > > 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 v6:
> > > 1. Enable clocks in the pwm_enable function
> > > 2. Remove suspend/resume code since pwm-backlight driver has done the same
> > > things 3. Revise some code to make it easier to read
> > > 
> > 
> > Can you please provide the whole change log for all versions the next time.
> > It's easier this way to understand the evolution of the series.
> 
> Change in v6:
> 1. Enable clocks in the pwm_enable function.
> 2. Remove suspend/resume code since pwm-backlight driver has done the
> same things.
> 3. Revise some code to make it easier to read.
> 
> Change in v5:
> 1. Configure PWM output via pinctrl.
> 2. Fix the parameter name in dtsi.
> 
> Change in v4:
> 1. Codebase is on v4.2-rc1.
> 2. Add the PWM node in dtsi.
> 3. Change the dependency in Kconfig.
> 4. Rewrite some code for readability.
> 
> Change in v3:
> 1. Add suspend/resume function.
> 2. Fix the formula for high_width calculation.
> 3. Rewrite some code to make it easier to read.
> 4. Add more information in the commit message.
> 
> Change in v2:
> 1. Rewrite descriptions for driver in kconfig to make it much clear.
> 2. Rename the driver from "pwm-mediatek-disp" to "pwm-mtk-disp".
> 3. Disable clocks in the error path.
> 4. Change the shift values in decimal instead of hex.
> 5. Rename and add some variables or function name for consistency and
> readability.
> 6. Drop unnecessary parentheses, spaces, variables and add newlines to
> make the code easiler to read.

Hi all,

If you have any suggestions, please let me know.
Perhaps we can apply the patch?

Regards,
YH Huang

Re: [PATCH v6 0/3] Add MediaTek display PWM driver

[YH Huang]

On Fri, 2015-07-24 at 17:10 +0800, YH Huang wrote:
> On Fri, 2015-07-24 at 10:42 +0200, Matthias Brugger wrote:
> > On Monday, July 20, 2015 04:17:14 PM YH Huang wrote:
> > > 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 v6:
> > > 1. Enable clocks in the pwm_enable function
> > > 2. Remove suspend/resume code since pwm-backlight driver has done the same
> > > things 3. Revise some code to make it easier to read
> > > 
> > 
> > Can you please provide the whole change log for all versions the next time.
> > It's easier this way to understand the evolution of the series.
> 
> Change in v6:
> 1. Enable clocks in the pwm_enable function.
> 2. Remove suspend/resume code since pwm-backlight driver has done the
> same things.
> 3. Revise some code to make it easier to read.
> 
> Change in v5:
> 1. Configure PWM output via pinctrl.
> 2. Fix the parameter name in dtsi.
> 
> Change in v4:
> 1. Codebase is on v4.2-rc1.
> 2. Add the PWM node in dtsi.
> 3. Change the dependency in Kconfig.
> 4. Rewrite some code for readability.
> 
> Change in v3:
> 1. Add suspend/resume function.
> 2. Fix the formula for high_width calculation.
> 3. Rewrite some code to make it easier to read.
> 4. Add more information in the commit message.
> 
> Change in v2:
> 1. Rewrite descriptions for driver in kconfig to make it much clear.
> 2. Rename the driver from "pwm-mediatek-disp" to "pwm-mtk-disp".
> 3. Disable clocks in the error path.
> 4. Change the shift values in decimal instead of hex.
> 5. Rename and add some variables or function name for consistency and
> readability.
> 6. Drop unnecessary parentheses, spaces, variables and add newlines to
> make the code easiler to read.
> 
> Thanks for your suggestion.

Hi Thierry,

Do you have any suggestions?
Maybe we could apply the patch?

Regards,
YH Huang

Re: [PATCH v6 0/3] Add MediaTek display PWM driver

[Daniel Kurtz]

Thierry,

I think these patches are ready.
Will you please consider picking up in your pwm tree?

-Dan


On Mon, Aug 3, 2015 at 2:21 PM, YH Huang <yh.huang@mediatek.com> wrote:
> On Fri, 2015-07-24 at 17:10 +0800, YH Huang wrote:
>> On Fri, 2015-07-24 at 10:42 +0200, Matthias Brugger wrote:
>> > On Monday, July 20, 2015 04:17:14 PM YH Huang wrote:
>> > > 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 v6:
>> > > 1. Enable clocks in the pwm_enable function
>> > > 2. Remove suspend/resume code since pwm-backlight driver has done the same
>> > > things 3. Revise some code to make it easier to read
>> > >
>> >
>> > Can you please provide the whole change log for all versions the next time.
>> > It's easier this way to understand the evolution of the series.
>>
>> Change in v6:
>> 1. Enable clocks in the pwm_enable function.
>> 2. Remove suspend/resume code since pwm-backlight driver has done the
>> same things.
>> 3. Revise some code to make it easier to read.
>>
>> Change in v5:
>> 1. Configure PWM output via pinctrl.
>> 2. Fix the parameter name in dtsi.
>>
>> Change in v4:
>> 1. Codebase is on v4.2-rc1.
>> 2. Add the PWM node in dtsi.
>> 3. Change the dependency in Kconfig.
>> 4. Rewrite some code for readability.
>>
>> Change in v3:
>> 1. Add suspend/resume function.
>> 2. Fix the formula for high_width calculation.
>> 3. Rewrite some code to make it easier to read.
>> 4. Add more information in the commit message.
>>
>> Change in v2:
>> 1. Rewrite descriptions for driver in kconfig to make it much clear.
>> 2. Rename the driver from "pwm-mediatek-disp" to "pwm-mtk-disp".
>> 3. Disable clocks in the error path.
>> 4. Change the shift values in decimal instead of hex.
>> 5. Rename and add some variables or function name for consistency and
>> readability.
>> 6. Drop unnecessary parentheses, spaces, variables and add newlines to
>> make the code easiler to read.
>>
>> Thanks for your suggestion.
>
> Hi Thierry,
>
> Do you have any suggestions?
> Maybe we could apply the patch?
>
> Regards,
> YH Huang
>
> --
> 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 v6 2/3] pwm: add MediaTek display PWM driver support

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 04:17:16PM +0800, YH Huang 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 | 232 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 243 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

This is going to break randconfig builds. From a quick look your driver
will fail to build at least on architectures that don't set HAVE_IOMEM.
I know that's quite exotic, but I've had to deal with fallout from
things like this in the past. If you want || COMPILE_TEST you should at
least add a "depends on HAVE_IOMEM".

linux/clk.h seems to have stubs for all of the functions that you use,
so those shouldn't be a problem.

> +	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/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> new file mode 100644
> index 0000000..69682a0
> --- /dev/null
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -0,0 +1,232 @@
> +/*
> + * 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>

The above two aren't correctly sorted.

> +#include <linux/slab.h>
> +
> +#define DISP_PWM_EN		0x00
> +#define PWM_ENABLE_MASK		BIT(0)
> +
> +#define DISP_PWM_COMMIT		0x08
> +#define PWM_COMMIT_MASK		BIT(0)
> +
> +#define DISP_PWM_CON_0		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

There are still two names for the same value here. I thought we had
agreed on this becoming something like the below?

	#define PWM_PERIOD_BIT_SHIFT 12
	#define PWM_PERIOD_MASK ((1 << PWM_PERIOD_BIT_SHIFT) - 1)

Although PWM_PERIOD_BIT_WIDTH would probably be a better name. Or
PWM_PERIOD_BITS or similar.

> +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);

I'd prefer this to be separate lines:

	int ret;

	ret = pwmchip_remove(&mdp->chip);

Thierry

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

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

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 04:17:15PM +0800, YH Huang wrote:
> Document the device-tree binding of MediatTek display PWM.

I already mentioned this a while back: s/MediatTek/MediaTek/.

Thierry

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

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

[YH Huang]

On Mon, 2015-08-17 at 15:23 +0200, Thierry Reding wrote:
> On Mon, Jul 20, 2015 at 04:17:15PM +0800, YH Huang wrote:
> > Document the device-tree binding of MediatTek display PWM.
> 
> I already mentioned this a while back: s/MediatTek/MediaTek/.
> 
Sorry, I will correct it to "MediaTek".

Regards,
YH Huang

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

[YH Huang]

On Mon, 2015-08-17 at 15:23 +0200, Thierry Reding wrote:
> On Mon, Jul 20, 2015 at 04:17:16PM +0800, YH Huang 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 | 232 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 243 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
> 
> This is going to break randconfig builds. From a quick look your driver
> will fail to build at least on architectures that don't set HAVE_IOMEM.
> I know that's quite exotic, but I've had to deal with fallout from
> things like this in the past. If you want || COMPILE_TEST you should at
> least add a "depends on HAVE_IOMEM".
> 
> linux/clk.h seems to have stubs for all of the functions that you use,
> so those shouldn't be a problem.
> 

I will add it and this will be like:

depends on ARCH_MEDIATEK || COMPILE_TEST
depends on HAS_IOMEM

> > +	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/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> > new file mode 100644
> > index 0000000..69682a0
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-mtk-disp.c
> > @@ -0,0 +1,232 @@
> > +/*
> > + * 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>
> 
> The above two aren't correctly sorted.
> 

Arrange like this:

#include <linux/platform_device.h>
#include <linux/pwm.h>

> > +#include <linux/slab.h>
> > +
> > +#define DISP_PWM_EN		0x00
> > +#define PWM_ENABLE_MASK		BIT(0)
> > +
> > +#define DISP_PWM_COMMIT		0x08
> > +#define PWM_COMMIT_MASK		BIT(0)
> > +
> > +#define DISP_PWM_CON_0		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
> 
> There are still two names for the same value here. I thought we had
> agreed on this becoming something like the below?
> 
> 	#define PWM_PERIOD_BIT_SHIFT 12
> 	#define PWM_PERIOD_MASK ((1 << PWM_PERIOD_BIT_SHIFT) - 1)
> 
> Although PWM_PERIOD_BIT_WIDTH would probably be a better name. Or
> PWM_PERIOD_BITS or similar.
> 

I will revise to 

#define PWM_PERIOD_BIT_WIDTH 12
#define PWM_PERIOD_MASK ((1 << PWM_PERIOD_BIT_WIDTH) - 1)

> > +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);
> 
> I'd prefer this to be separate lines:
> 
> 	int ret;
> 
> 	ret = pwmchip_remove(&mdp->chip);

OK.

Thanks for your suggestions.

Regards,
YH Huang