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

[Paul Cercueil <paul@crapouillou.net>]

On Mon, Mar 4, 2019 at 1:30 PM, Thierry Reding 
<thierry.reding@gmail.com> 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@crapouillou.net 
>> <mailto:paul@crapouillou.net>>
>>  Tested-by: Mathieu Malaterre <malat@debian.org 
>> <mailto:malat@debian.org>>
>>  Tested-by: Artur Rojek <contact@artur-rojek.eu 
>> <mailto:contact@artur-rojek.eu>>
>>  ---
>> 
>>  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