Hi Thierry, Thank you for the detailed review. I was intending to avoid adding a new compatible string, and to limit the changes. But your comments make sense, and I will split the commit and use the compatible string to identify the IP differences.
Thanks, Shenwei -----Original Message----- From: Thierry Reding [mailto:thierry.red...@gmail.com] Sent: Wednesday, June 6, 2018 3:34 AM To: Shenwei Wang <shenwei.w...@nxp.com> Cc: linux-...@vger.kernel.org; dl-linux-imx <linux-...@nxp.com>; linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/1] pwm: fsl-ftm: Support the new version of FTM block on i.MX8x On Thu, May 24, 2018 at 01:08:48PM -0500, shenwei.w...@nxp.com wrote: > On the new i.MX8x SoC family, the following changes were made on the > FTM > block: > > 1. Need to enable the IPG clock before accessing any FTM registers. > Because the IPG clock is not an option for FTM counter clock source, > it can't be used as the ftm_sys clock. > > 2. An additional PWM enable bit was added for each PWM channel in > register FTM_SC[16:23]. It supports 8 channels. Bit16 is for channel > 0, and bit23 for channel 7. Generally if you need to itemize changes in your commit message it is a good indication that you should be splitting up the patch into multiple logical changes. In this case, one possibility would be to turn this into three patches: 1. Introduce the concept on an "interface" clock which is by default the same as the ftm_sys clock. 2. Add support for enable bits, based on some per-compatible data structure (see for example pwm-tegra.c for an example of how to do this). 3. Enable support for the new SoC family by adding an instance of the per-compatible structure for that compatible string. > As the IP version information can not be obtained in any of the FTM > registers, a property of "fsl,has-pwmen-bits" is added in the ftm pwm > device node. If it has the property, the driver set the PWM enable bit > when a PWM channel is requested. I don't see a corresponding device tree bindings update for this change. Also, I don't think doing this via a separate property is the right way, you can just derive this from the new compatible string which you need to add anyway. So to the above patches, add: 0. Add DT bindings for new SoC family with a mention that they need to provide the new IPG clock. > > Signed-off-by: Shenwei Wang <shenwei.w...@nxp.com> > --- > drivers/pwm/pwm-fsl-ftm.c | 35 +++++++++++++++++++++++++++++------ > 1 file changed, 29 insertions(+), 6 deletions(-) > > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c > index 557b4ea..0426458f 100644 > --- a/drivers/pwm/pwm-fsl-ftm.c > +++ b/drivers/pwm/pwm-fsl-ftm.c > @@ -86,7 +86,9 @@ struct fsl_pwm_chip { > struct regmap *regmap; > > int period_ns; > + bool has_pwmen; > > + struct clk *ipg_clk; > struct clk *clk[FSL_PWM_CLK_MAX]; > }; > > @@ -97,16 +99,31 @@ static inline struct fsl_pwm_chip > *to_fsl_chip(struct pwm_chip *chip) > > static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device > *pwm) { > + int ret; > struct fsl_pwm_chip *fpc = to_fsl_chip(chip); > > - return clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]); > + ret = clk_prepare_enable(fpc->ipg_clk); This is confusing because it looks as if you're breaking existing drivers, until you realize that... > + > + if ((!ret) && (fpc->has_pwmen)) { > + mutex_lock(&fpc->lock); > + regmap_update_bits(fpc->regmap, FTM_SC, > + BIT(pwm->hwpwm + 16), BIT(pwm->hwpwm + 16)); > + mutex_unlock(&fpc->lock); > + } > + > + return ret; > } > > static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device > *pwm) { > struct fsl_pwm_chip *fpc = to_fsl_chip(chip); > > - clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]); > + if (fpc->has_pwmen) { > + mutex_lock(&fpc->lock); > + regmap_update_bits(fpc->regmap, FTM_SC, BIT(pwm->hwpwm + 16), > 0); > + mutex_unlock(&fpc->lock); > + } > + clk_disable_unprepare(fpc->ipg_clk); > } > > static int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc, @@ > -363,7 +380,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc) { > int ret; > > - ret = clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]); > + ret = clk_prepare_enable(fpc->ipg_clk); > if (ret) > return ret; > > @@ -371,7 +388,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc) > regmap_write(fpc->regmap, FTM_OUTINIT, 0x00); > regmap_write(fpc->regmap, FTM_OUTMASK, 0xFF); > > - clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]); > + clk_disable_unprepare(fpc->ipg_clk); > > return 0; > } > @@ -428,6 +445,10 @@ static int fsl_pwm_probe(struct platform_device *pdev) > return PTR_ERR(fpc->clk[FSL_PWM_CLK_SYS]); > } > > + fpc->ipg_clk = devm_clk_get(&pdev->dev, "ipg"); > + if (IS_ERR(fpc->ipg_clk)) > + fpc->ipg_clk = fpc->clk[FSL_PWM_CLK_SYS]; ... fpc->ipg_clk is actually the same as fpc->clk[FSL_PWM_CLK_SYS] for older chips. I think this could be made more obvious by splitting this patch into several smaller ones as suggested above. > + > fpc->clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix"); > if (IS_ERR(fpc->clk[FSL_PWM_CLK_FIX])) > return PTR_ERR(fpc->clk[FSL_PWM_CLK_FIX]); > @@ -446,6 +467,8 @@ static int fsl_pwm_probe(struct platform_device *pdev) > fpc->chip.of_pwm_n_cells = 3; > fpc->chip.base = -1; > fpc->chip.npwm = 8; > + fpc->has_pwmen = of_property_read_bool(pdev->dev.of_node, > + "fsl,ftm-has-pwmen-bits"); As I said earlier, I think this should be derived from the compatible string. That is, you could have a data structure such as: struct fsl_pwm_soc { bool has_enable_bits; }; static const struct fsl_pwm_soc vf610_ftm_pwm = { .has_enable_bits = false, }; /* up to here would be part of patch 2 */ /* and this is part of patch 3, along with an entry in the OF * match table */ static const struct fsl_pwm_soc imx8x_ftm_pwm = { .has_enable_bits = true, }; Thierry
