YH Huang (2):
dt-bindings: pwm: add MediaTek display PWM bindings
pwm: add MediaTek display PWM driver support
.../devicetree/bindings/pwm/pwm-mtk-disp.txt | 25 +++
drivers/pwm/Kconfig | 10 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-mtk-disp.c | 228 +++++++++++++++++++++
4 files changed, 264 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
Document the device-tree binding of MediatTek display PWM.
Signed-off-by: YH Huang <[email protected]>
---
.../devicetree/bindings/pwm/pwm-mtk-disp.txt | 25 ++++++++++++++++++++++
1 file changed, 25 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..f55bf92
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
@@ -0,0 +1,25 @@
+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
+
+Example:
+ pwm0: pwm@1401e000 {
+ compatible = "mediatek,mt8173-disp-pwm",
+ "mediatek,mt6595-disp-pwm";
+ reg = <0 0x1401e000 0 0x1000>;
+ #pwm-cells = <2>;
+ clocks = <&mmsys MM_DISP_PWM026M>,
+ <&mmsys MM_DISP_PWM0MM>;
+ clock-names = "main",
+ "mm";
+ };
--
1.8.1.1.dirty
Add display PWM driver support to modify backlight for MT8173.
Signed-off-by: YH Huang <[email protected]>
---
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-mtk-disp.c | 228 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 239 insertions(+)
create mode 100644 drivers/pwm/pwm-mtk-disp.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b1541f4..90e3c079 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 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/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..d4e4cb6
--- /dev/null
+++ b/drivers/pwm/pwm-mtk-disp.c
@@ -0,0 +1,228 @@
+/*
+ * MediaTek display pulse-width-modulation controller driver.
+ * Copyright (c) 2015 MediaTek Inc.
+ * Author: YH Huang <[email protected]>
+ *
+ * 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 0x0
+#define PWM_ENABLE_MASK 0x1
+
+#define DISP_PWM_COMMIT 0x08
+#define PWM_COMMIT_MASK 0x1
+
+#define DISP_PWM_CON_0 0x10
+#define PWM_CLKDIV_SHIFT 16
+#define PWM_CLKDIV_MASK (0x3ff << PWM_CLKDIV_SHIFT)
+#define PWM_CLKDIV_MAX 0x000003ff
+
+#define DISP_PWM_CON_1 0x14
+#define PWM_PERIOD_MASK 0xfff
+#define PWM_PERIOD_MAX 0x00000fff
+/* Shift log2(PWM_PERIOD_MAX + 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 *mmio_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;
+ 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 + 1) / 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)) - 1
+ */
+ mdp = to_mtk_disp_pwm(chip);
+ 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 = clk_div + 1;
+ period = div64_u64(rate * period_ns, NSEC_PER_SEC * div);
+ if (period > 0)
+ period--;
+
+ high_width = div64_u64(rate * duty_ns, NSEC_PER_SEC * div);
+ if (high_width > 0)
+ high_width--;
+
+ mtk_disp_pwm_update_bits(mdp->mmio_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->mmio_base + DISP_PWM_CON_1,
+ PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
+
+ mtk_disp_pwm_update_bits(mdp->mmio_base + DISP_PWM_COMMIT,
+ PWM_COMMIT_MASK, 1);
+ mtk_disp_pwm_update_bits(mdp->mmio_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;
+
+ mdp = to_mtk_disp_pwm(chip);
+ mtk_disp_pwm_update_bits(mdp->mmio_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;
+
+ mdp = to_mtk_disp_pwm(chip);
+ mtk_disp_pwm_update_bits(mdp->mmio_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->mmio_base = devm_ioremap_resource(&pdev->dev, r);
+ if (IS_ERR(mdp->mmio_base))
+ return PTR_ERR(mdp->mmio_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) {
+ clk_disable_unprepare(mdp->clk_main);
+ return ret;
+ }
+
+ 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);
+ clk_disable_unprepare(mdp->clk_main);
+ clk_disable_unprepare(mdp->clk_mm);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int mtk_disp_pwm_remove(struct platform_device *pdev)
+{
+ struct mtk_disp_pwm *mdp;
+ int ret;
+
+ mdp = platform_get_drvdata(pdev);
+ 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);
+
+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 <[email protected]>");
+MODULE_DESCRIPTION("MediaTek SoC display PWM driver");
+MODULE_LICENSE("GPL v2");
--
1.8.1.1.dirty
On Thu, 2015-05-21 at 21:29 +0800, YH Huang wrote:
> YH Huang (2):
> dt-bindings: pwm: add MediaTek display PWM bindings
> pwm: add MediaTek display PWM driver support
>
> .../devicetree/bindings/pwm/pwm-mtk-disp.txt | 25 +++
> drivers/pwm/Kconfig | 10 +
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-mtk-disp.c | 228 +++++++++++++++++++++
> 4 files changed, 264 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> create mode 100644 drivers/pwm/pwm-mtk-disp.c
Hi YH,
It would be easier for reviewer if you have a summary here on what you
have changed compare to last version.
Also, please add patch series summary even for v2, it remind reviewer
what this series is about.
Joe.C
On Mon, 2015-05-25 at 10:14 +0800, Yingjoe Chen wrote:
> On Thu, 2015-05-21 at 21:29 +0800, YH Huang wrote:
This patch series add the use of display PWM driver and documentation
for Mediatek SoCs. The driver is used to support the backlight of
the panel. This is based on v4.1-rc1.
> > YH Huang (2):
> > dt-bindings: pwm: add MediaTek display PWM bindings
> > pwm: add MediaTek display PWM driver support
> >
> > .../devicetree/bindings/pwm/pwm-mtk-disp.txt | 25 +++
> > drivers/pwm/Kconfig | 10 +
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-mtk-disp.c | 228 +++++++++++++++++++++
> > 4 files changed, 264 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> > create mode 100644 drivers/pwm/pwm-mtk-disp.c
>
> Hi YH,
>
> It would be easier for reviewer if you have a summary here on what you
> have changed compare to last version.
> Also, please add patch series summary even for v2, it remind reviewer
> what this series is about.
>
> Joe.C
>
The patch v2 is refined with everybody's suggestions.
It is much readable and consistent.
If anyone has any suggestions, please just let me know.
Thank you.
Regards,
YH Huang
Hi YH,
2015-06-05 8:15 GMT+02:00 YH Huang <[email protected]>:
> On Mon, 2015-05-25 at 10:14 +0800, Yingjoe Chen wrote:
>> On Thu, 2015-05-21 at 21:29 +0800, YH Huang wrote:
>
> This patch series add the use of display PWM driver and documentation
> for Mediatek SoCs. The driver is used to support the backlight of
> the panel. This is based on v4.1-rc1.
>
>> > YH Huang (2):
>> > dt-bindings: pwm: add MediaTek display PWM bindings
>> > pwm: add MediaTek display PWM driver support
>> >
>> > .../devicetree/bindings/pwm/pwm-mtk-disp.txt | 25 +++
>> > drivers/pwm/Kconfig | 10 +
>> > drivers/pwm/Makefile | 1 +
>> > drivers/pwm/pwm-mtk-disp.c | 228 +++++++++++++++++++++
>> > 4 files changed, 264 insertions(+)
>> > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
>> > create mode 100644 drivers/pwm/pwm-mtk-disp.c
>>
>> Hi YH,
>>
>> It would be easier for reviewer if you have a summary here on what you
>> have changed compare to last version.
>> Also, please add patch series summary even for v2, it remind reviewer
>> what this series is about.
>>
>> Joe.C
>>
>
> The patch v2 is refined with everybody's suggestions.
> It is much readable and consistent.
I suppose that Yingjoe wanted to emphasis that for the reviewer it is
much easier if you describe the changes you made from v1 to v2.
You can find an example (random pick from the mailing list) here:
http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000572.html
Thanks,
Matthias
--
motzblog.wordpress.com
On Fri, 2015-06-05 at 10:39 +0200, Matthias Brugger wrote:
> Hi YH,
>
> 2015-06-05 8:15 GMT+02:00 YH Huang <[email protected]>:
> > On Mon, 2015-05-25 at 10:14 +0800, Yingjoe Chen wrote:
> >> On Thu, 2015-05-21 at 21:29 +0800, YH Huang wrote:
> >
> > This patch series add the use of display PWM driver and documentation
> > for Mediatek SoCs. The driver is used to support the backlight of
> > the panel. This is based on v4.1-rc1.
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.
> >
> >> > YH Huang (2):
> >> > dt-bindings: pwm: add MediaTek display PWM bindings
> >> > pwm: add MediaTek display PWM driver support
> >> >
> >> > .../devicetree/bindings/pwm/pwm-mtk-disp.txt | 25 +++
> >> > drivers/pwm/Kconfig | 10 +
> >> > drivers/pwm/Makefile | 1 +
> >> > drivers/pwm/pwm-mtk-disp.c | 228 +++++++++++++++++++++
> >> > 4 files changed, 264 insertions(+)
> >> > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> >> > create mode 100644 drivers/pwm/pwm-mtk-disp.c
> >>
> >> Hi YH,
> >>
> >> It would be easier for reviewer if you have a summary here on what you
> >> have changed compare to last version.
> >> Also, please add patch series summary even for v2, it remind reviewer
> >> what this series is about.
> >>
> >> Joe.C
> >>
> >
> > The patch v2 is refined with everybody's suggestions.
> > It is much readable and consistent.
>
> I suppose that Yingjoe wanted to emphasis that for the reviewer it is
> much easier if you describe the changes you made from v1 to v2.
> You can find an example (random pick from the mailing list) here:
> http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000572.html
>
> Thanks,
> Matthias
>
I have added the change lists.
Thank you.
Regards,
YH Huang
On Thu, May 21, 2015 at 09:29:24PM +0800, YH Huang wrote:
> Add display PWM driver support to modify backlight for MT8173.
>
> Signed-off-by: YH Huang <[email protected]>
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-mtk-disp.c | 228 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 239 insertions(+)
> create mode 100644 drivers/pwm/pwm-mtk-disp.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f4..90e3c079 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 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/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..d4e4cb6
> --- /dev/null
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -0,0 +1,228 @@
> +/*
> + * MediaTek display pulse-width-modulation controller driver.
> + * Copyright (c) 2015 MediaTek Inc.
> + * Author: YH Huang <[email protected]>
> + *
> + * 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 0x0
> +#define PWM_ENABLE_MASK 0x1
> +
> +#define DISP_PWM_COMMIT 0x08
> +#define PWM_COMMIT_MASK 0x1
> +
> +#define DISP_PWM_CON_0 0x10
> +#define PWM_CLKDIV_SHIFT 16
> +#define PWM_CLKDIV_MASK (0x3ff << PWM_CLKDIV_SHIFT)
> +#define PWM_CLKDIV_MAX 0x000003ff
I think you should make this:
#define PWM_CLKDIV_MAX 0x3ff
#define PWM_CLKDIV_MASK (PWM_CLKDIV_MAX << PWM_CLKDIV_SHIFT)
Just to show that these belong together.
> +
> +#define DISP_PWM_CON_1 0x14
> +#define PWM_PERIOD_MASK 0xfff
> +#define PWM_PERIOD_MAX 0x00000fff
Same here. PWM_PERIOD_MAX isn't actually used anywhere, so perhaps just
drop it altogether. But see also below...
> +/* Shift log2(PWM_PERIOD_MAX + 1) as divisor */
> +#define PWM_PERIOD_BIT_SHIFT 12
I wasn't very clear about this in my earlier review, so let me try to
explain why I think this is confusing. You use this as a divisor, but
you encode it as a shift. It's also PWM_PERIOD_MAX + 1, so I think it
would make more sense to drop this, keep PWM_PERIOD_MAX as above and
then replace the
>> PWM_PERIOD_BIT_SHIFT
below by
/ (PWM_PERIOD_MAX + 1)
> +#define PWM_HIGH_WIDTH_SHIFT 16
> +#define PWM_HIGH_WIDTH_MASK (0x1fff << PWM_HIGH_WIDTH_SHIFT)
Why is the mask wider than for the period? That would imply that the
duty cycle can be longer than a period, which doesn't make any sense.
Can you clarify?
> +struct mtk_disp_pwm {
> + struct pwm_chip chip;
> + struct device *dev;
> + struct clk *clk_main;
> + struct clk *clk_mm;
> + void __iomem *mmio_base;
I think "base" will do just fine.
> +};
> +
> +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;
> + 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
Nit: should have a space between '+' and '1'.
> + * duty_ns = 10^9 * (clk_div + 1) * (high_width + 1) / 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)) - 1
> + */
> + mdp = to_mtk_disp_pwm(chip);
Please put this on the same line as the variable declaration:
struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> + 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 = clk_div + 1;
Perhaps make this:
div = NSEC_PER_SEC * (clk_div + 1);
to avoid the two multiplication below.
> + period = div64_u64(rate * period_ns, NSEC_PER_SEC * div);
So this would become:
period = div64_u64(rate * period_ns, div);
> + if (period > 0)
> + period--;
> +
> + high_width = div64_u64(rate * duty_ns, NSEC_PER_SEC * div);
And this:
high_width = div64_u64(rate * duty_ns, div);
> + if (high_width > 0)
> + high_width--;
> +
> + mtk_disp_pwm_update_bits(mdp->mmio_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->mmio_base + DISP_PWM_CON_1,
> + PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
> +
> + mtk_disp_pwm_update_bits(mdp->mmio_base + DISP_PWM_COMMIT,
> + PWM_COMMIT_MASK, 1);
> + mtk_disp_pwm_update_bits(mdp->mmio_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;
> +
> + mdp = to_mtk_disp_pwm(chip);
The above three lines should be collapsed.
> + mtk_disp_pwm_update_bits(mdp->mmio_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;
> +
> + mdp = to_mtk_disp_pwm(chip);
Same here.
> + mtk_disp_pwm_update_bits(mdp->mmio_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->mmio_base = devm_ioremap_resource(&pdev->dev, r);
> + if (IS_ERR(mdp->mmio_base))
> + return PTR_ERR(mdp->mmio_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) {
> + clk_disable_unprepare(mdp->clk_main);
> + return ret;
> + }
> +
> + 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);
> + clk_disable_unprepare(mdp->clk_main);
> + clk_disable_unprepare(mdp->clk_mm);
> + return ret;
> + }
> +
> + return 0;
> +}
It's customary to collect the error cleanup code in an unwinding section
at the bottom of the function, like so:
ret = clk_prepare_enable(mdp->clk_mm);
if (ret < 0)
goto disable_clk_main;
...
ret = pwmchip_add(&mdp->chip);
if (ret < 0) {
dev_err(&pdev->dev, ...);
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;
This makes sure that you undo things in the proper order and eliminates
the need to duplicate cleanup code in all failure paths.
> +
> +static int mtk_disp_pwm_remove(struct platform_device *pdev)
> +{
> + struct mtk_disp_pwm *mdp;
> + int ret;
> +
> + mdp = platform_get_drvdata(pdev);
Should be on the same line as the variable declaration.
> + 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);
> +
> +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 <[email protected]>");
> +MODULE_DESCRIPTION("MediaTek SoC display PWM driver");
> +MODULE_LICENSE("GPL v2");
Thierry
On Thu, May 21, 2015 at 09:29:23PM +0800, YH Huang wrote:
> Document the device-tree binding of MediatTek display PWM.
s/MediatTek/MediaTek/
>
> Signed-off-by: YH Huang <[email protected]>
This could use a more verbose commit message. You could mention what the
PWM is typically used for (display I presume). Perhaps mention how many
channels it exposes and so on.
> ---
> .../devicetree/bindings/pwm/pwm-mtk-disp.txt | 25 ++++++++++++++++++++++
> 1 file changed, 25 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..f55bf92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> @@ -0,0 +1,25 @@
> +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
You can use the full line width of 78/80 characters, no need to wrap
this prematurely.
> + - 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
> +
> +Example:
> + pwm0: pwm@1401e000 {
> + compatible = "mediatek,mt8173-disp-pwm",
> + "mediatek,mt6595-disp-pwm";
> + reg = <0 0x1401e000 0 0x1000>;
> + #pwm-cells = <2>;
> + clocks = <&mmsys MM_DISP_PWM026M>,
> + <&mmsys MM_DISP_PWM0MM>;
> + clock-names = "main",
> + "mm";
No need to waste a line,
clock-names = "main", "mm";
fits on a single line just fine.
Thierry
On Thu, May 21, 2015 at 09:29:24PM +0800, YH Huang wrote:
> Add display PWM driver support to modify backlight for MT8173.
The driver also claims compatibility with MT6595, so you should mention
that here as well. Also please be more verbose in the commit message.
You should mention things like how many channels the PWM has or typical
use-cases.
Thierry
On Fri, 2015-06-12 at 12:23 +0200, Thierry Reding wrote:
> On Thu, May 21, 2015 at 09:29:23PM +0800, YH Huang wrote:
> > Document the device-tree binding of MediatTek display PWM.
>
> s/MediatTek/MediaTek/
>
> >
> > Signed-off-by: YH Huang <[email protected]>
>
> This could use a more verbose commit message. You could mention what the
> PWM is typically used for (display I presume). Perhaps mention how many
> channels it exposes and so on.
I will add more information in the commit message.
>
> > ---
> > .../devicetree/bindings/pwm/pwm-mtk-disp.txt | 25 ++++++++++++++++++++++
> > 1 file changed, 25 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..f55bf92
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> > @@ -0,0 +1,25 @@
> > +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
>
> You can use the full line width of 78/80 characters, no need to wrap
> this prematurely.
OK.
>
> > + - 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
> > +
> > +Example:
> > + pwm0: pwm@1401e000 {
> > + compatible = "mediatek,mt8173-disp-pwm",
> > + "mediatek,mt6595-disp-pwm";
> > + reg = <0 0x1401e000 0 0x1000>;
> > + #pwm-cells = <2>;
> > + clocks = <&mmsys MM_DISP_PWM026M>,
> > + <&mmsys MM_DISP_PWM0MM>;
> > + clock-names = "main",
> > + "mm";
>
> No need to waste a line,
>
> clock-names = "main", "mm";
>
> fits on a single line just fine.
OK.
>
> Thierry
Thank for your suggestion.
Regards,
YH Huang
On Fri, 2015-06-12 at 12:25 +0200, Thierry Reding wrote:
> On Thu, May 21, 2015 at 09:29:24PM +0800, YH Huang wrote:
> > Add display PWM driver support to modify backlight for MT8173.
>
> The driver also claims compatibility with MT6595, so you should mention
> that here as well. Also please be more verbose in the commit message.
> You should mention things like how many channels the PWM has or typical
> use-cases.
>
> Thierry
I will do it.
Thank for your suggestion.
Regards,
YH Huang
On Fri, 2015-06-12 at 12:20 +0200, Thierry Reding wrote:
> On Thu, May 21, 2015 at 09:29:24PM +0800, YH Huang wrote:
> > Add display PWM driver support to modify backlight for MT8173.
> >
> > Signed-off-by: YH Huang <[email protected]>
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-mtk-disp.c | 228 +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 239 insertions(+)
> > create mode 100644 drivers/pwm/pwm-mtk-disp.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index b1541f4..90e3c079 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 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/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..d4e4cb6
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-mtk-disp.c
> > @@ -0,0 +1,228 @@
> > +/*
> > + * MediaTek display pulse-width-modulation controller driver.
> > + * Copyright (c) 2015 MediaTek Inc.
> > + * Author: YH Huang <[email protected]>
> > + *
> > + * 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 0x0
> > +#define PWM_ENABLE_MASK 0x1
> > +
> > +#define DISP_PWM_COMMIT 0x08
> > +#define PWM_COMMIT_MASK 0x1
> > +
> > +#define DISP_PWM_CON_0 0x10
> > +#define PWM_CLKDIV_SHIFT 16
> > +#define PWM_CLKDIV_MASK (0x3ff << PWM_CLKDIV_SHIFT)
> > +#define PWM_CLKDIV_MAX 0x000003ff
>
> I think you should make this:
>
> #define PWM_CLKDIV_MAX 0x3ff
> #define PWM_CLKDIV_MASK (PWM_CLKDIV_MAX << PWM_CLKDIV_SHIFT)
>
> Just to show that these belong together.
>
It is much clear.
> > +
> > +#define DISP_PWM_CON_1 0x14
> > +#define PWM_PERIOD_MASK 0xfff
> > +#define PWM_PERIOD_MAX 0x00000fff
>
> Same here. PWM_PERIOD_MAX isn't actually used anywhere, so perhaps just
> drop it altogether. But see also below...
>
> > +/* Shift log2(PWM_PERIOD_MAX + 1) as divisor */
> > +#define PWM_PERIOD_BIT_SHIFT 12
>
> I wasn't very clear about this in my earlier review, so let me try to
> explain why I think this is confusing. You use this as a divisor, but
> you encode it as a shift. It's also PWM_PERIOD_MAX + 1, so I think it
> would make more sense to drop this, keep PWM_PERIOD_MAX as above and
> then replace the
>
> >> PWM_PERIOD_BIT_SHIFT
>
> below by
>
> / (PWM_PERIOD_MAX + 1)
>
Maybe I can change in this way:
Remove this: #define PWM_PERIOD_MAX 0x00000fff
Using ">> PWM_PERIOD_BIT_SHIFT" is faster than "/ (PWM_PERIOD_MAX + 1)"
Is this right?
> > +#define PWM_HIGH_WIDTH_SHIFT 16
> > +#define PWM_HIGH_WIDTH_MASK (0x1fff << PWM_HIGH_WIDTH_SHIFT)
>
> Why is the mask wider than for the period? That would imply that the
> duty cycle can be longer than a period, which doesn't make any sense.
> Can you clarify?
>
After discussing with the hardware designer, the duty cycle is
calculated by "high_width / (period + 1)". If period is the "magic
number 0xfff", high_width needs 13 bits to show the situation that duty
cycle is 100%. I should fix the formula for high_width below.
> > +struct mtk_disp_pwm {
> > + struct pwm_chip chip;
> > + struct device *dev;
> > + struct clk *clk_main;
> > + struct clk *clk_mm;
> > + void __iomem *mmio_base;
>
> I think "base" will do just fine.
>
OK.
> > +};
> > +
> > +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;
> > + 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
>
> Nit: should have a space between '+' and '1'.
>
OK.
> > + * duty_ns = 10^9 * (clk_div + 1) * (high_width + 1) / PWM_CLK_RATE
Here should be
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)) - 1
And here
high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
> > + */
> > + mdp = to_mtk_disp_pwm(chip);
>
> Please put this on the same line as the variable declaration:
>
> struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
>
OK.
> > + 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 = clk_div + 1;
>
> Perhaps make this:
>
> div = NSEC_PER_SEC * (clk_div + 1);
>
> to avoid the two multiplication below.
>
You are right.
> > + period = div64_u64(rate * period_ns, NSEC_PER_SEC * div);
>
> So this would become:
>
> period = div64_u64(rate * period_ns, div);
>
Got it.
> > + if (period > 0)
> > + period--;
> > +
> > + high_width = div64_u64(rate * duty_ns, NSEC_PER_SEC * div);
>
> And this:
>
> high_width = div64_u64(rate * duty_ns, div);
>
OK.
> > + if (high_width > 0)
> > + high_width--;
I should remove this two lines above for the new formula.
> > +
> > + mtk_disp_pwm_update_bits(mdp->mmio_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->mmio_base + DISP_PWM_CON_1,
> > + PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
> > +
> > + mtk_disp_pwm_update_bits(mdp->mmio_base + DISP_PWM_COMMIT,
> > + PWM_COMMIT_MASK, 1);
> > + mtk_disp_pwm_update_bits(mdp->mmio_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;
> > +
> > + mdp = to_mtk_disp_pwm(chip);
>
> The above three lines should be collapsed.
>
OK.
> > + mtk_disp_pwm_update_bits(mdp->mmio_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;
> > +
> > + mdp = to_mtk_disp_pwm(chip);
>
> Same here.
>
OK.
> > + mtk_disp_pwm_update_bits(mdp->mmio_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->mmio_base = devm_ioremap_resource(&pdev->dev, r);
> > + if (IS_ERR(mdp->mmio_base))
> > + return PTR_ERR(mdp->mmio_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) {
> > + clk_disable_unprepare(mdp->clk_main);
> > + return ret;
> > + }
> > +
> > + 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);
> > + clk_disable_unprepare(mdp->clk_main);
> > + clk_disable_unprepare(mdp->clk_mm);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> It's customary to collect the error cleanup code in an unwinding section
> at the bottom of the function, like so:
>
> ret = clk_prepare_enable(mdp->clk_mm);
> if (ret < 0)
> goto disable_clk_main;
>
> ...
>
> ret = pwmchip_add(&mdp->chip);
> if (ret < 0) {
> dev_err(&pdev->dev, ...);
> 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;
>
> This makes sure that you undo things in the proper order and eliminates
> the need to duplicate cleanup code in all failure paths.
>
I will rewrite this part.
> > +
> > +static int mtk_disp_pwm_remove(struct platform_device *pdev)
> > +{
> > + struct mtk_disp_pwm *mdp;
> > + int ret;
> > +
> > + mdp = platform_get_drvdata(pdev);
>
> Should be on the same line as the variable declaration.
>
OK.
> > + 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);
> > +
> > +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 <[email protected]>");
> > +MODULE_DESCRIPTION("MediaTek SoC display PWM driver");
> > +MODULE_LICENSE("GPL v2");
>
> Thierry
Thank for your suggestion.
Regards,
YH Huang
On Thu, 2015-06-18 at 18:19 +0800, YH Huang wrote:
> On Fri, 2015-06-12 at 12:20 +0200, Thierry Reding wrote:
> > > +/* Shift log2(PWM_PERIOD_MAX + 1) as divisor */
> > > +#define PWM_PERIOD_BIT_SHIFT 12
> >
> > I wasn't very clear about this in my earlier review, so let me try to
> > explain why I think this is confusing. You use this as a divisor, but
> > you encode it as a shift. It's also PWM_PERIOD_MAX + 1, so I think it
> > would make more sense to drop this, keep PWM_PERIOD_MAX as above and
> > then replace the
> >
> > >> PWM_PERIOD_BIT_SHIFT
> >
> > below by
> >
> > / (PWM_PERIOD_MAX + 1)
> >
>
> Maybe I can change in this way:
> Remove this: #define PWM_PERIOD_MAX 0x00000fff
> Using ">> PWM_PERIOD_BIT_SHIFT" is faster than "/ (PWM_PERIOD_MAX + 1)"
> Is this right?
The place which use this shift is:
clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
PWM_PERIOD_BIT_SHIFT;
div_u64 return u64. If we change >> to /, and somehow compiler didn't
optimize that div into shift, it will cause build error.
Joe.C
On Thu, Jun 18, 2015 at 06:19:44PM +0800, YH Huang wrote:
> On Fri, 2015-06-12 at 12:20 +0200, Thierry Reding wrote:
> > On Thu, May 21, 2015 at 09:29:24PM +0800, YH Huang wrote:
[...]
> > > +#define DISP_PWM_CON_1 0x14
> > > +#define PWM_PERIOD_MASK 0xfff
> > > +#define PWM_PERIOD_MAX 0x00000fff
> >
> > Same here. PWM_PERIOD_MAX isn't actually used anywhere, so perhaps just
> > drop it altogether. But see also below...
> >
> > > +/* Shift log2(PWM_PERIOD_MAX + 1) as divisor */
> > > +#define PWM_PERIOD_BIT_SHIFT 12
> >
> > I wasn't very clear about this in my earlier review, so let me try to
> > explain why I think this is confusing. You use this as a divisor, but
> > you encode it as a shift. It's also PWM_PERIOD_MAX + 1, so I think it
> > would make more sense to drop this, keep PWM_PERIOD_MAX as above and
> > then replace the
> >
> > >> PWM_PERIOD_BIT_SHIFT
> >
> > below by
> >
> > / (PWM_PERIOD_MAX + 1)
> >
>
> Maybe I can change in this way:
> Remove this: #define PWM_PERIOD_MAX 0x00000fff
> Using ">> PWM_PERIOD_BIT_SHIFT" is faster than "/ (PWM_PERIOD_MAX + 1)"
> Is this right?
The compiler should be able to optimize the division to a shift, so both
will likely result in the same code. But I don't mind much either way as
long as we don't define two symbols for essentially the same value.
Thierry
On Thu, Jun 18, 2015 at 09:58:59PM +0800, Yingjoe Chen wrote:
> On Thu, 2015-06-18 at 18:19 +0800, YH Huang wrote:
> > On Fri, 2015-06-12 at 12:20 +0200, Thierry Reding wrote:
> > > > +/* Shift log2(PWM_PERIOD_MAX + 1) as divisor */
> > > > +#define PWM_PERIOD_BIT_SHIFT 12
> > >
> > > I wasn't very clear about this in my earlier review, so let me try to
> > > explain why I think this is confusing. You use this as a divisor, but
> > > you encode it as a shift. It's also PWM_PERIOD_MAX + 1, so I think it
> > > would make more sense to drop this, keep PWM_PERIOD_MAX as above and
> > > then replace the
> > >
> > > >> PWM_PERIOD_BIT_SHIFT
> > >
> > > below by
> > >
> > > / (PWM_PERIOD_MAX + 1)
> > >
> >
> > Maybe I can change in this way:
> > Remove this: #define PWM_PERIOD_MAX 0x00000fff
> > Using ">> PWM_PERIOD_BIT_SHIFT" is faster than "/ (PWM_PERIOD_MAX + 1)"
> > Is this right?
>
>
> The place which use this shift is:
>
> clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
> PWM_PERIOD_BIT_SHIFT;
>
> div_u64 return u64. If we change >> to /, and somehow compiler didn't
> optimize that div into shift, it will cause build error.
Good point. I think every compiler should be able to optimize this, but
the shift isn't any worse than a divide and if we can proactively avoid
portability issues, let's go with the shift.
Thierry