From: YH Huang <yh.huang@mediatek.com>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
Mark Rutland <mark.rutland@arm.com>,
Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
<linux-pwm@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<srv_heupstream@mediatek.com>,
<linux-mediatek@lists.infradead.org>,
Sascha Hauer <kernel@pengutronix.de>, <yh.huang@mediatek.com>,
<yingjoe.chen@mediatek.com>
Subject: Re: [PATCH v2 2/2] pwm: add MediaTek display PWM driver support
Date: Thu, 18 Jun 2015 18:19:44 +0800 [thread overview]
Message-ID: <1434622784.18278.39.camel@mtksdaap41> (raw)
In-Reply-To: <20150612102046.GF19400@ulmo.nvidia.com>
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 <yh.huang@mediatek.com>
> > ---
> > 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 <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 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 <yh.huang@mediatek.com>");
> > +MODULE_DESCRIPTION("MediaTek SoC display PWM driver");
> > +MODULE_LICENSE("GPL v2");
>
> Thierry
Thank for your suggestion.
Regards,
YH Huang
WARNING: multiple messages have this Message-ID (diff)
From: YH Huang <yh.huang@mediatek.com>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
Mark Rutland <mark.rutland@arm.com>,
Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
srv_heupstream@mediatek.com, linux-mediatek@lists.infradead.org,
Sascha Hauer <kernel@pengutronix.de>,
yh.huang@mediatek.com, yingjoe.chen@mediatek.com
Subject: Re: [PATCH v2 2/2] pwm: add MediaTek display PWM driver support
Date: Thu, 18 Jun 2015 18:19:44 +0800 [thread overview]
Message-ID: <1434622784.18278.39.camel@mtksdaap41> (raw)
In-Reply-To: <20150612102046.GF19400@ulmo.nvidia.com>
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 <yh.huang@mediatek.com>
> > ---
> > 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 <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 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 <yh.huang@mediatek.com>");
> > +MODULE_DESCRIPTION("MediaTek SoC display PWM driver");
> > +MODULE_LICENSE("GPL v2");
>
> Thierry
Thank for your suggestion.
Regards,
YH Huang
WARNING: multiple messages have this Message-ID (diff)
From: yh.huang@mediatek.com (YH Huang)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 2/2] pwm: add MediaTek display PWM driver support
Date: Thu, 18 Jun 2015 18:19:44 +0800 [thread overview]
Message-ID: <1434622784.18278.39.camel@mtksdaap41> (raw)
In-Reply-To: <20150612102046.GF19400@ulmo.nvidia.com>
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 <yh.huang@mediatek.com>
> > ---
> > 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 <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 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 <yh.huang@mediatek.com>");
> > +MODULE_DESCRIPTION("MediaTek SoC display PWM driver");
> > +MODULE_LICENSE("GPL v2");
>
> Thierry
Thank for your suggestion.
Regards,
YH Huang
next prev parent reply other threads:[~2015-06-18 10:23 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-21 13:29 [PATCH v2 0/2] Add MediaTek display PWM driver YH Huang
2015-05-21 13:29 ` YH Huang
2015-05-21 13:29 ` YH Huang
2015-05-21 13:29 ` [PATCH v2 1/2] dt-bindings: pwm: add MediaTek display PWM bindings YH Huang
2015-05-21 13:29 ` YH Huang
2015-05-21 13:29 ` YH Huang
2015-06-12 10:23 ` Thierry Reding
2015-06-12 10:23 ` Thierry Reding
2015-06-18 9:00 ` YH Huang
2015-06-18 9:00 ` YH Huang
2015-06-18 9:00 ` YH Huang
2015-05-21 13:29 ` [PATCH v2 2/2] pwm: add MediaTek display PWM driver support YH Huang
2015-05-21 13:29 ` YH Huang
2015-05-21 13:29 ` YH Huang
2015-06-12 10:20 ` Thierry Reding
2015-06-12 10:20 ` Thierry Reding
2015-06-18 10:19 ` YH Huang [this message]
2015-06-18 10:19 ` YH Huang
2015-06-18 10:19 ` YH Huang
2015-06-18 13:58 ` Yingjoe Chen
2015-06-18 13:58 ` Yingjoe Chen
2015-06-18 13:58 ` Yingjoe Chen
2015-06-29 9:27 ` Thierry Reding
2015-06-29 9:27 ` Thierry Reding
2015-06-29 9:21 ` Thierry Reding
2015-06-29 9:21 ` Thierry Reding
2015-06-12 10:25 ` Thierry Reding
2015-06-12 10:25 ` Thierry Reding
2015-06-18 9:02 ` YH Huang
2015-06-18 9:02 ` YH Huang
2015-06-18 9:02 ` YH Huang
2015-05-25 2:14 ` [PATCH v2 0/2] Add MediaTek display PWM driver Yingjoe Chen
2015-05-25 2:14 ` Yingjoe Chen
2015-05-25 2:14 ` Yingjoe Chen
2015-06-05 6:15 ` YH Huang
2015-06-05 6:15 ` YH Huang
2015-06-05 8:39 ` Matthias Brugger
2015-06-05 8:39 ` Matthias Brugger
2015-06-05 10:45 ` YH Huang
2015-06-05 10:45 ` YH Huang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1434622784.18278.39.camel@mtksdaap41 \
--to=yh.huang@mediatek.com \
--cc=devicetree@vger.kernel.org \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-pwm@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=matthias.bgg@gmail.com \
--cc=pawel.moll@arm.com \
--cc=robh+dt@kernel.org \
--cc=srv_heupstream@mediatek.com \
--cc=thierry.reding@gmail.com \
--cc=yingjoe.chen@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.