| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190820055138.hvvwdsdswrmptvej@pengutronix.de>
Date: Tue, 20 Aug 2019 07:51:38 +0200
From: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
To: Sam Shih <sam.shih@...iatek.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Matthias Brugger <matthias.bgg@...il.com>,
Thierry Reding <thierry.reding@...il.com>,
Ryder Lee <ryder.lee@...iatek.com>,
John Crispin <john@...ozen.org>, linux-pwm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v4 3/10] pwm: mediatek: allocate the clks array
dynamically and fix mt7628 pwm
Hello,
On Tue, Aug 20, 2019 at 09:40:18AM +0800, Sam Shih wrote:
> From: Ryder Lee <ryder.lee@...iatek.com>
>
> Instead of using fixed size of arrays, allocate the memory for them
> based on the information we get from the chips.
>
> Also fix mt7628 pwm during configure from userspace. The SoC
> is legacy MIPS and has no complex clock tree. This patch add property
> clock-frequency to the SoC specific data and legacy MIPS SoC need to
> configure it in DT. This property is use for period calculation.
This fix is worth a separate patch.
> Signed-off-by: Ryder Lee <ryder.lee@...iatek.com>
> Signed-off-by: Sam Shih <sam.shih@...iatek.com>
> ---
> Changes since v4:
> - Follow reviewers's comments
> 1. use pc->soc->has_clks to check clocks exist or not.
> 2. Add error message when probe() unable to get clks
> - Fixes bug when SoC is old mips which has no complex clock tree.
> if clocks not exist, use the new property from DT to apply period caculation;
> otherwise, use clk_get_rate to get clock frequency and apply period caculation.
> ---
> drivers/pwm/pwm-mediatek.c | 94 +++++++++++++++++++++++---------------
> 1 file changed, 56 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index f9d67fb66adb..a70b69a975c1 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -35,25 +35,6 @@
>
> #define PWM_CLK_DIV_MAX 7
>
> -enum {
> - MTK_CLK_MAIN = 0,
> - MTK_CLK_TOP,
> - MTK_CLK_PWM1,
> - MTK_CLK_PWM2,
> - 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[MTK_CLK_MAX] = {
> - "main", "top", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5", "pwm6", "pwm7",
> - "pwm8"
> -};
> -
> struct mtk_pwm_platform_data {
> unsigned int fallback_npwms;
> bool pwm45_fixup;
> @@ -64,12 +45,17 @@ struct mtk_pwm_platform_data {
> * struct mtk_pwm_chip - struct representing PWM chip
> * @chip: linux PWM chip representation
> * @regs: base address of PWM chip
> - * @clks: list of clocks
> + * @clk_top: the top clock generator
> + * @clk_main: the clock used by PWM core
> + * @clk_pwms: the clock used by each PWM channel
> */
> struct mtk_pwm_chip {
> struct pwm_chip chip;
> void __iomem *regs;
> - struct clk *clks[MTK_CLK_MAX];
> + struct clk *clk_top;
> + struct clk *clk_main;
> + struct clk **clk_pwms;
> + unsigned int clk_freq;
> const struct mtk_pwm_platform_data *soc;
> };
>
> @@ -90,24 +76,24 @@ static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> if (!pc->soc->has_clks)
> return 0;
>
> - ret = clk_prepare_enable(pc->clks[MTK_CLK_TOP]);
> + ret = clk_prepare_enable(pc->clk_top);
> if (ret < 0)
> return ret;
>
> - ret = clk_prepare_enable(pc->clks[MTK_CLK_MAIN]);
> + ret = clk_prepare_enable(pc->clk_main);
> if (ret < 0)
> goto disable_clk_top;
>
> - ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> + ret = clk_prepare_enable(pc->clk_pwms[pwm->hwpwm]);
> if (ret < 0)
> goto disable_clk_main;
>
> return 0;
>
> disable_clk_main:
> - clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
> + clk_disable_unprepare(pc->clk_main);
> disable_clk_top:
> - clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> + clk_disable_unprepare(pc->clk_top);
>
> return ret;
> }
> @@ -119,9 +105,9 @@ static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> if (!pc->soc->has_clks)
> return;
>
> - clk_disable_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> - clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
> - clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> + clk_disable_unprepare(pc->clk_pwms[pwm->hwpwm]);
> + clk_disable_unprepare(pc->clk_main);
> + clk_disable_unprepare(pc->clk_top);
> }
>
> static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
> @@ -141,19 +127,24 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> int duty_ns, int period_ns)
> {
> struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
> - struct clk *clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm];
> + unsigned int clk_freq;
> u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH,
> reg_thres = PWMTHRES;
> u64 resolution;
> int ret;
>
> + if (pc->soc->has_clks)
> + clk_freq = clk_get_rate(pc->clk_pwms[pwm->hwpwm]);
> + else
> + clk_freq = pc->clk_freq;
> +
> ret = mtk_pwm_clk_enable(chip, pwm);
> if (ret < 0)
> return ret;
>
> /* Using resolution in picosecond gets accuracy higher */
> resolution = (u64)NSEC_PER_SEC * 1000;
> - do_div(resolution, clk_get_rate(clk));
> + do_div(resolution, clk_freq);
>
> cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, resolution);
> while (cnt_period > 8191) {
> @@ -229,7 +220,8 @@ static int mtk_pwm_probe(struct platform_device *pdev)
> struct device_node *np = pdev->dev.of_node;
> struct mtk_pwm_chip *pc;
> struct resource *res;
> - unsigned int i, npwms;
> + unsigned int npwms;
> + unsigned int clk_freq;
> int ret;
>
> pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> @@ -255,13 +247,40 @@ static int mtk_pwm_probe(struct platform_device *pdev)
> }
> }
>
> - for (i = 0; i < npwms + 2 && pc->soc->has_clks; i++) {
> - pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]);
> - if (IS_ERR(pc->clks[i])) {
> - dev_err(&pdev->dev, "clock: %s fail: %ld\n",
> - mtk_pwm_clk_name[i], PTR_ERR(pc->clks[i]));
> - return PTR_ERR(pc->clks[i]);
> + if (pc->soc->has_clks) {
Instead of using pc->soc->has_clks you could try
ret = of_property_read_u32(np, "clock-frequency", &clk_freq);
and depend on that in the above if. This might allow to drop the
.has_clks member.
> + int i;
> +
> + pc->clk_pwms = devm_kcalloc(&pdev->dev, npwms,
> + sizeof(*pc->clk_pwms), GFP_KERNEL);
> + if (!pc->clk_pwms)
> + return -ENOMEM;
> +
> + pc->clk_top = devm_clk_get(&pdev->dev, "top");
> + if (IS_ERR(pc->clk_top))
> + return PTR_ERR(pc->clk_top);
> +
> + pc->clk_main = devm_clk_get(&pdev->dev, "main");
> + if (IS_ERR(pc->clk_main))
> + return PTR_ERR(pc->clk_main);
You missed to add an error message for "top" and "main".
> + for (i = 0; i < npwms; i++) {
> + char name[8];
> +
> + snprintf(name, sizeof(name), "pwm%d", i + 1);
> + pc->clk_pwms[i] = devm_clk_get(&pdev->dev, name);
> + if (IS_ERR(pc->clk_pwms[i])) {
> + dev_err(&pdev->dev, "failed to get %s\n", name);
I'd mention "clock" in the error string and the return code.
> + return PTR_ERR(pc->clk_pwms[i]);
> + }
> + }
> + } else {
> + ret = of_property_read_u32(np, "clock-frequency",
> + &clk_freq);
Please align follow up lines to the opening (.
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to get clk_freq\n");
> + return ret;
> }
> + pc->clk_freq = clk_freq;
> }
>
> platform_set_drvdata(pdev, pc);
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Powered by blists - more mailing lists