From: Thierry Reding <thierry.reding@gmail.com> To: Zhi Mao <zhi.mao@mediatek.com> Cc: john@phrozen.org, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Matthias Brugger <matthias.bgg@gmail.com>, linux-pwm@vger.kernel.org, srv_heupstream@mediatek.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, yingjoe.chen@mediatek.com, yt.shen@mediatek.com, sean.wang@mediatek.com, zhenbao.liu@mediatek.com Subject: Re: [PATCH v3 6/6] pwm: mediatek: add MT2712/MT7622 support Date: Mon, 21 Aug 2017 10:05:11 +0200 [thread overview] Message-ID: <20170821080511.GP18996@ulmo> (raw) In-Reply-To: <1498802721-32455-7-git-send-email-zhi.mao@mediatek.com> [-- Attachment #1: Type: text/plain, Size: 4447 bytes --] On Fri, Jun 30, 2017 at 02:05:21PM +0800, Zhi Mao wrote: > 1. support multiple chip(MT2712, MT7622, MT7623) > 2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8 > the register offset address of pwm8 for MT2712 is not fixed 0x40 > and they are not the same as pwm0~6. > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > --- > drivers/pwm/pwm-mediatek.c | 55 +++++++++++++++++++++++++++++++++++--------- > 1 file changed, 44 insertions(+), 11 deletions(-) > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > index 1d78ab1..2c9ce24 100644 > --- a/drivers/pwm/pwm-mediatek.c > +++ b/drivers/pwm/pwm-mediatek.c > @@ -16,6 +16,7 @@ > #include <linux/module.h> > #include <linux/clk.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/pwm.h> > #include <linux/slab.h> > @@ -40,11 +41,19 @@ enum { > MTK_CLK_PWM3, > MTK_CLK_PWM4, > MTK_CLK_PWM5, > + MTK_CLK_PWM6, > + MTK_CLK_PWM7, > + MTK_CLK_PWM8, > MTK_CLK_MAX, > }; > > -static const char * const mtk_pwm_clk_name[] = { > - "main", "top", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5" > +static const char * const mtk_pwm_clk_name[MTK_CLK_MAX] = { > + "main", "top", "pwm1", "pwm2", "pwm3", "pwm4", > + "pwm5", "pwm6", "pwm7", "pwm8" You're wrapping these lines at arbitrary boundaries. Make sure to use all of the 80 columns at your disposal. > +}; > + > +struct mtk_com_pwm_data { What does the _com stand for in the above? > + unsigned int pwm_nums; > }; Maybe name this num_pwms for consistency with other drivers? > > /** > @@ -57,6 +66,11 @@ struct mtk_pwm_chip { > struct pwm_chip chip; > void __iomem *regs; > struct clk *clks[MTK_CLK_MAX]; > + const struct mtk_com_pwm_data *data; > +}; > + > +static const unsigned long mtk_pwm_com_reg[] = { There's another of these _com that I don't understand what it means. Also since these are all fairly small offsets, these can simply be unsigned int. > + 0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220 > }; > > static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip) > @@ -103,14 +117,14 @@ static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm) > static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num, > unsigned int offset) > { > - return readl(chip->regs + 0x10 + (num * 0x40) + offset); > + return readl(chip->regs + mtk_pwm_com_reg[num] + offset); > } > > static inline void mtk_pwm_writel(struct mtk_pwm_chip *chip, > unsigned int num, unsigned int offset, > u32 value) > { > - writel(value, chip->regs + 0x10 + (num * 0x40) + offset); > + writel(value, chip->regs + mtk_pwm_com_reg[num] + offset); > } > > static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > @@ -194,23 +208,28 @@ static int mtk_pwm_probe(struct platform_device *pdev) > if (!pc) > return -ENOMEM; > > + pc->data = of_device_get_match_data(&pdev->dev); > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > pc->regs = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(pc->regs)) > return PTR_ERR(pc->regs); > > - for (i = 0; i < MTK_CLK_MAX; i++) { > + for (i = 0; i < pc->data->pwm_nums + 2; i++) { > pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]); > - if (IS_ERR(pc->clks[i])) > + if (IS_ERR(pc->clks[i])) { > + dev_err(&pdev->dev, "[PWM] clock: %s fail: %ld\n", > + mtk_pwm_clk_name[i], PTR_ERR(pc->clks[i])); Why include the "[PWM] " prefix in the above message? > return PTR_ERR(pc->clks[i]); > + } > } > > - platform_set_drvdata(pdev, pc); > - > pc->chip.dev = &pdev->dev; > pc->chip.ops = &mtk_pwm_ops; > pc->chip.base = -1; > - pc->chip.npwm = 5; > + pc->chip.npwm = pc->data->pwm_nums; > + > + platform_set_drvdata(pdev, pc); No need to move the location of the platform_set_drvdata() call. It's needless churn. > static const struct of_device_id mtk_pwm_of_match[] = { > - { .compatible = "mediatek,mt7623-pwm" }, > - { } > + {.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data}, > + {.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data}, > + {.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data}, > + {}, Spaces after { and before }, please. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: thierry.reding@gmail.com (Thierry Reding) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 6/6] pwm: mediatek: add MT2712/MT7622 support Date: Mon, 21 Aug 2017 10:05:11 +0200 [thread overview] Message-ID: <20170821080511.GP18996@ulmo> (raw) In-Reply-To: <1498802721-32455-7-git-send-email-zhi.mao@mediatek.com> On Fri, Jun 30, 2017 at 02:05:21PM +0800, Zhi Mao wrote: > 1. support multiple chip(MT2712, MT7622, MT7623) > 2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8 > the register offset address of pwm8 for MT2712 is not fixed 0x40 > and they are not the same as pwm0~6. > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com> > --- > drivers/pwm/pwm-mediatek.c | 55 +++++++++++++++++++++++++++++++++++--------- > 1 file changed, 44 insertions(+), 11 deletions(-) > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > index 1d78ab1..2c9ce24 100644 > --- a/drivers/pwm/pwm-mediatek.c > +++ b/drivers/pwm/pwm-mediatek.c > @@ -16,6 +16,7 @@ > #include <linux/module.h> > #include <linux/clk.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/pwm.h> > #include <linux/slab.h> > @@ -40,11 +41,19 @@ enum { > MTK_CLK_PWM3, > MTK_CLK_PWM4, > MTK_CLK_PWM5, > + MTK_CLK_PWM6, > + MTK_CLK_PWM7, > + MTK_CLK_PWM8, > MTK_CLK_MAX, > }; > > -static const char * const mtk_pwm_clk_name[] = { > - "main", "top", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5" > +static const char * const mtk_pwm_clk_name[MTK_CLK_MAX] = { > + "main", "top", "pwm1", "pwm2", "pwm3", "pwm4", > + "pwm5", "pwm6", "pwm7", "pwm8" You're wrapping these lines at arbitrary boundaries. Make sure to use all of the 80 columns at your disposal. > +}; > + > +struct mtk_com_pwm_data { What does the _com stand for in the above? > + unsigned int pwm_nums; > }; Maybe name this num_pwms for consistency with other drivers? > > /** > @@ -57,6 +66,11 @@ struct mtk_pwm_chip { > struct pwm_chip chip; > void __iomem *regs; > struct clk *clks[MTK_CLK_MAX]; > + const struct mtk_com_pwm_data *data; > +}; > + > +static const unsigned long mtk_pwm_com_reg[] = { There's another of these _com that I don't understand what it means. Also since these are all fairly small offsets, these can simply be unsigned int. > + 0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220 > }; > > static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip) > @@ -103,14 +117,14 @@ static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm) > static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num, > unsigned int offset) > { > - return readl(chip->regs + 0x10 + (num * 0x40) + offset); > + return readl(chip->regs + mtk_pwm_com_reg[num] + offset); > } > > static inline void mtk_pwm_writel(struct mtk_pwm_chip *chip, > unsigned int num, unsigned int offset, > u32 value) > { > - writel(value, chip->regs + 0x10 + (num * 0x40) + offset); > + writel(value, chip->regs + mtk_pwm_com_reg[num] + offset); > } > > static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > @@ -194,23 +208,28 @@ static int mtk_pwm_probe(struct platform_device *pdev) > if (!pc) > return -ENOMEM; > > + pc->data = of_device_get_match_data(&pdev->dev); > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > pc->regs = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(pc->regs)) > return PTR_ERR(pc->regs); > > - for (i = 0; i < MTK_CLK_MAX; i++) { > + for (i = 0; i < pc->data->pwm_nums + 2; i++) { > pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]); > - if (IS_ERR(pc->clks[i])) > + if (IS_ERR(pc->clks[i])) { > + dev_err(&pdev->dev, "[PWM] clock: %s fail: %ld\n", > + mtk_pwm_clk_name[i], PTR_ERR(pc->clks[i])); Why include the "[PWM] " prefix in the above message? > return PTR_ERR(pc->clks[i]); > + } > } > > - platform_set_drvdata(pdev, pc); > - > pc->chip.dev = &pdev->dev; > pc->chip.ops = &mtk_pwm_ops; > pc->chip.base = -1; > - pc->chip.npwm = 5; > + pc->chip.npwm = pc->data->pwm_nums; > + > + platform_set_drvdata(pdev, pc); No need to move the location of the platform_set_drvdata() call. It's needless churn. > static const struct of_device_id mtk_pwm_of_match[] = { > - { .compatible = "mediatek,mt7623-pwm" }, > - { } > + {.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data}, > + {.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data}, > + {.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data}, > + {}, Spaces after { and before }, please. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170821/72c1f9de/attachment.sig>
next prev parent reply other threads:[~2017-08-21 8:05 UTC|newest] Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-06-30 6:05 [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-06-30 6:05 ` [PATCH v3 1/6] pwm: kconfig: modify mediatek information Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-08-21 7:31 ` Thierry Reding 2017-08-21 7:31 ` Thierry Reding 2017-06-30 6:05 ` [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-07-05 11:09 ` Matthias Brugger 2017-07-05 11:09 ` Matthias Brugger 2017-07-06 6:16 ` Zhi Mao 2017-07-06 6:16 ` Zhi Mao 2017-07-06 6:16 ` Zhi Mao 2017-07-06 6:43 ` Zhi Mao 2017-07-06 6:43 ` Zhi Mao 2017-07-06 6:43 ` Zhi Mao 2017-07-18 16:34 ` Matthias Brugger 2017-07-18 16:34 ` Matthias Brugger 2017-08-21 7:35 ` Thierry Reding 2017-08-21 7:35 ` Thierry Reding 2017-08-21 7:35 ` Thierry Reding 2017-06-30 6:05 ` [PATCH v3 3/6] pwm: mediatek: fix clock control issue Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-08-21 7:47 ` Thierry Reding 2017-08-21 7:47 ` Thierry Reding 2017-08-21 7:47 ` Thierry Reding 2017-06-30 6:05 ` [PATCH v3 4/6] pwm: bindings: add MT2712/MT7622 information Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-08-21 7:51 ` Thierry Reding 2017-08-21 7:51 ` Thierry Reding 2017-06-30 6:05 ` [PATCH v3 5/6] pwm: mediatek: add PWM_CLK_DIV_MAX Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-08-21 7:58 ` Thierry Reding 2017-08-21 7:58 ` Thierry Reding 2017-06-30 6:05 ` [PATCH v3 6/6] pwm: mediatek: add MT2712/MT7622 support Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-06-30 6:05 ` Zhi Mao 2017-08-21 8:05 ` Thierry Reding [this message] 2017-08-21 8:05 ` Thierry Reding 2017-08-21 9:05 ` Zhi Mao 2017-08-21 9:05 ` Zhi Mao 2017-08-21 9:05 ` Zhi Mao 2017-07-17 3:16 ` [PATCH v3 0/6] mediatek: pwm driver " Zhi Mao 2017-07-17 3:16 ` Zhi Mao 2017-07-17 3:16 ` Zhi Mao 2017-08-02 7:19 ` Zhi Mao 2017-08-02 7:19 ` Zhi Mao 2017-08-02 7:19 ` Zhi Mao 2017-08-02 8:42 ` John Crispin 2017-08-02 8:42 ` John Crispin 2017-08-02 8:42 ` John Crispin 2017-08-03 9:41 ` Zhi Mao 2017-08-03 9:41 ` Zhi Mao 2017-08-03 9:41 ` Zhi Mao
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=20170821080511.GP18996@ulmo \ --to=thierry.reding@gmail.com \ --cc=devicetree@vger.kernel.org \ --cc=john@phrozen.org \ --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=robh+dt@kernel.org \ --cc=sean.wang@mediatek.com \ --cc=srv_heupstream@mediatek.com \ --cc=yingjoe.chen@mediatek.com \ --cc=yt.shen@mediatek.com \ --cc=zhenbao.liu@mediatek.com \ --cc=zhi.mao@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: linkBe 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.