Re: [PATCH v10 13/27] pwm: jz4740: Use clocks from TCU driver

[Paul Cercueil]

On Mon, Mar 4, 2019 at 1:30 PM, Thierry Reding 
<thierry.reding@xxxxxxxxx> wrote:
> On Sat, Mar 02, 2019 at 08:33:59PM -0300, Paul Cercueil wrote:
> >  The ingenic-timer "TCU" driver provides us with clocks, that can be
> >  (un)gated, reparented or reclocked from devicetree, instead of 
> > having
> >  these settings hardcoded in this driver.
> >
> >  While this driver is devicetree-compatible, it is never (as of now)
> >  probed from devicetree, so this change does not introduce a ABI 
> > problem
> >  with current devicetree files.
> >
> >  Note that the "select REGMAP" has been dropped because it's
> >  already enabled by the "select INGENIC_TIMER".
> >
> >  Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx 
> > <mailto:paul@xxxxxxxxxxxxxxx>>
> >  Tested-by: Mathieu Malaterre <malat@xxxxxxxxxx 
> > <mailto:malat@xxxxxxxxxx>>
> >  Tested-by: Artur Rojek <contact@xxxxxxxxxxxxxx 
> > <mailto:contact@xxxxxxxxxxxxxx>>
> >  ---
> >
> >  Notes:
> >           v9: New patch
> >
> >           v10: Explain in commit message why "select REGMAP" was 
> > dropped
> >
> >   drivers/pwm/Kconfig      |  3 ++-
> >   drivers/pwm/pwm-jz4740.c | 39 
> > ++++++++++++++++++++++++++-------------
> >   2 files changed, 28 insertions(+), 14 deletions(-)
> >
> >  diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >  index ace8ea4b6247..4e201e970509 100644
> >  --- a/drivers/pwm/Kconfig
> >  +++ b/drivers/pwm/Kconfig
> >  @@ -204,7 +204,8 @@ config PWM_IMX
> >   config PWM_JZ4740
> >   	tristate "Ingenic JZ47xx PWM support"
> >   	depends on MACH_INGENIC
> >  -	select REGMAP
> >  +	depends on COMMON_CLK
> >  +	select INGENIC_TIMER
>
> Okay... sounds like this would be okay. I'm assuming you go through 
> that
> extra step of selecting REGMAP in the prior patch and dropping it here
> again so that you keep the series bisectible?

Yes, exactly.

> >   	help
> >   	  Generic PWM framework driver for Ingenic JZ47xx based
> >   	  machines.
> >  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> >  index 8dfac5ffd71c..c6136bd4434b 100644
> >  --- a/drivers/pwm/pwm-jz4740.c
> >  +++ b/drivers/pwm/pwm-jz4740.c
> >  @@ -28,7 +28,7 @@
> >
> >   struct jz4740_pwm_chip {
> >   	struct pwm_chip chip;
> >  -	struct clk *clk;
> >  +	struct clk *clks[NUM_PWM];
> >   	struct regmap *map;
> >   };
> >
> >  @@ -40,6 +40,9 @@ static inline struct jz4740_pwm_chip 
> > *to_jz4740(struct pwm_chip *chip)
> >   static int jz4740_pwm_request(struct pwm_chip *chip, struct 
> > pwm_device *pwm)
> >   {
> >   	struct jz4740_pwm_chip *jz = to_jz4740(chip);
> >  +	struct clk *clk;
> >  +	char clk_name[16];
> >  +	int ret;
> >
> >   	/*
> >   	 * Timers 0 and 1 are used for system tasks, so they are 
> > unavailable
> >  @@ -48,16 +51,29 @@ static int jz4740_pwm_request(struct pwm_chip 
> > *chip, struct pwm_device *pwm)
> >   	if (pwm->hwpwm < 2)
> >   		return -EBUSY;
> >
> >  -	regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm));
> >  +	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
> >
> >  +	clk = clk_get(chip->dev, clk_name);
> >  +	if (IS_ERR(clk))
> >  +		return PTR_ERR(clk);
> >  +
> >  +	ret = clk_prepare_enable(clk);
> >  +	if (ret) {
> >  +		clk_put(clk);
> >  +		return ret;
> >  +	}
> >  +
> >  +	jz->clks[pwm->hwpwm] = clk;
> >   	return 0;
> >   }
>
> Since you're already using ->request(), why not add a per-PWM 
> structure
> that tracks the clock? There are a number of other PWMs that already 
> do
> something similar. You can use pwm_{set,get}_chip_data() to associate
> chip-specific extra data with a PWM.
>
> Thierry

Good idea.

-Paul