Re: [PATCH v4 3/10] pwm: mediatek: allocate the clks array dynamically and fix mt7628 pwm

From: Uwe Kleine-König
Date: Tue Aug 20 2019 - 01:52:01 EST

Next message: Sumit Garg: "Re: [RFC/RFT v4 1/5] tpm: move tpm_buf code to include/linux/"
Previous message: Sumit Garg: "Re: [RFC/RFT v4 0/5] Add generic trusted keys framework/subsystem"
In reply to: Sam Shih: "[PATCH v4 3/10] pwm: mediatek: allocate the clks array dynamically and fix mt7628 pwm"
Next in thread: Sam Shih: "[PATCH v4 4/10] pwm: mediatek: use pwm_mediatek as common prefix"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello,

On Tue, Aug 20, 2019 at 09:40:18AM +0800, Sam Shih wrote:
> From: Ryder Lee <ryder.lee@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Signed-off-by: Sam Shih <sam.shih@xxxxxxxxxxxx>
> ---
> 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/ |

Next message: Sumit Garg: "Re: [RFC/RFT v4 1/5] tpm: move tpm_buf code to include/linux/"
Previous message: Sumit Garg: "Re: [RFC/RFT v4 0/5] Add generic trusted keys framework/subsystem"
In reply to: Sam Shih: "[PATCH v4 3/10] pwm: mediatek: allocate the clks array dynamically and fix mt7628 pwm"
Next in thread: Sam Shih: "[PATCH v4 4/10] pwm: mediatek: use pwm_mediatek as common prefix"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]