Message-ID: <yxolbwr62nfdaeobghokx32fihhh56falxpg4u7kex4is7soam@mfswlo4rbkaq>
Date: Wed, 5 Feb 2025 12:12:04 +0100
From: Uwe Kleine-König <ukleinek@...nel.org>
To: "Rafael V. Volkmer" <rafael.v.volkmer@...il.com>
Cc: linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pwm: tiehrpwm: ensures that state.enabled is
 synchronized in .probe()

Hello Rafael,

first of all: There are several issues regarding code style, checkpatch
reports

	total: 20 errors, 13 warnings, 180 lines checked

Please fix them all for v2 (or mention unfixed ones in the cover text,
ideally with a reasoning). (In case you don't know:

	scripts/checkpatch.pl -g @

; for bonus points add --strict.)

On Tue, Feb 04, 2025 at 03:55:40PM -0300, Rafael V. Volkmer wrote:
> Fixes potential desynchronization of state.enabled in the .probe()
> method by suggesting proper handling of hardware state initialization.
> Adds considerations for implementing .get_hw_state() to check the
> current state of the module by checking physical registers.

I don't understand that. What is .get_hw_state()?

Returning here after reading through the complete patch, I suggest
something like:

	If the hardware is already active during probe, it's not
	asserted that the clock is enabled. So check in
	ehrpwm_pwm_probe() if the enable bit is set and call
	clk_enable() and pm_runtime_get_sync() if yes.

> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@...il.com>
> ---
>  drivers/pwm/pwm-tiehrpwm.c | 151 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 150 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 0125e73b98df..5de213bc3ef5 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -91,6 +91,20 @@
>  #define AQCSFRC_CSFA_FRCHIGH	BIT(1)
>  #define AQCSFRC_CSFA_DISSWFRC	(BIT(1) | BIT(0))
>  
> +#define AQCTLA_CAU_MASK   (BIT(5) | BIT(4))
> +#define AQCTLA_CAU_SHIFT  4
> +#define AQCTLA_CAD_MASK   (BIT(9) | BIT(8))
> +#define AQCTLA_CAD_SHIFT  8

Please use

	#define AQCTLA_CAD	GENMASK(9, 8)

and for the usage then instead of

	cad_action = (aqctla_reg & AQCTLA_CAD_MASK) >> AQCTLA_CAD_SHIFT;

better:

	cad_action = FIELD_GET(AQCTLA_CAD, aqctla_reg);

> +
> +/* The ePWM hardware encodes compare actions with two bits each:
> + *   00 = Do nothing
> + *   01 = Clear
> + *   10 = Set
> + *   11 = Toggle
> + */
> +#define AQ_CLEAR  1
> +#define AQ_SET    2
> +
>  #define NUM_PWM_CHANNEL		2	/* EHRPWM channels */
>  
>  struct ehrpwm_context {
> @@ -353,6 +367,118 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	return 0;
>  }
>  
> +static bool ehrpwm_is_enabled(struct pwm_chip *chip)
> +{
> +	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> +	bool ret;
> +	u16 aqcsfrc_reg;
> +	u8 csfa_bits;
> +	u16 aqctla_reg;
> +
> +	if(chip == NULL) {
> +		return -EINVAL;
> +	}

return -EINVAL is bogus for a function returning bool. The solution is
easy: chip is never NULL here, so please drop. 

> +
> +	aqcsfrc_reg	= readw(pc->mmio_base + AQCSFRC);
> +	csfa_bits	= (u8)(aqcsfrc_reg & AQCSFRC_CSFA_MASK);

Single space before = please.

> +	aqctla_reg	= readw(pc->mmio_base + AQCTLA);
> +
> +	ret = (csfa_bits != 0u)	 ? false :
> +	      (aqctla_reg == 0u) ? false : true;

I didn't understand what these values mean, but this is unreadable. I
prefer:

	if (csfa_bits)
		return false;

	if (aqctla_reg)
		return true;

	return false;

Maybe also add code comments about the semantic to the respective
blocks.

> +
> +	return ret;
> +}
> +
> +static u64 ehrpwm_read_period(struct pwm_chip *chip)
> +{
> +	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> +	u64 ret;
> +	unsigned long tbclk_rate;
> +	u16 tbprd_reg;
> +	u64 period_cycles;
> +	u64 period_ns;
> +
> +	if(chip == NULL) {
> +		return -EINVAL;
> +	}
> +
> +	tbprd_reg = readw(pc->mmio_base + TBPRD);
> +	tbclk_rate = clk_get_rate(pc->tbclk);
> +	period_cycles = tbprd_reg + 1u;
> +	
> +	/* period_ns = (period_cycles * 1e9) / tblck_rate */
> +	period_ns = DIV_ROUND_UP_ULL(period_cycles * NSEC_PER_SEC, tbclk_rate);
> +	
> +	ret = period_ns;
> +	return ret;

return period_ns;

Given that ehrpwm_read_period() and ehrpwm_read_duty_cycle() are called
directly after another, I suggest to rework the code that clk_get_rate()
is only called once. (Maybe even put it into ehrpwm_pwm_chip.)

> +}
> +
> +static u64 ehrpwm_read_duty_cycle(struct pwm_chip *chip)
> +{
> +	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> +	u64 ret;
> +	u16 cmpa_reg;
> +	u64 duty_cycles;
> +	u64 duty_ns;
> +	unsigned long tbclk_rate;
> +
> +	if(chip == NULL) {
> +		return -EINVAL;
> +	}
> +
> +	cmpa_reg = readw(pc->mmio_base + CMPA);
> +	tbclk_rate = clk_get_rate(pc->tbclk);
> +	duty_cycles = cmpa_reg;
> +	duty_ns = DIV_ROUND_UP_ULL(duty_cycles * NSEC_PER_SEC, tbclk_rate);
> +	ret = duty_ns;
> +
> +	return ret;
> +}
> +
> +static enum pwm_polarity ehrpwm_read_polarity(struct pwm_chip *chip)
> +{
> +	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> +	enum pwm_polarity ret;
> +	u16 aqctla_reg;
> +	u8 cau_action;
> +	u8 cad_action;
> +
> +	if(chip == NULL) {
> +		return -EINVAL;
> +	}
> +
> +	aqctla_reg	= readw(pc->mmio_base + AQCTLA);
> +	cau_action = (aqctla_reg & AQCTLA_CAU_MASK) >> AQCTLA_CAU_SHIFT;
> +	cad_action = (aqctla_reg & AQCTLA_CAD_MASK) >> AQCTLA_CAD_SHIFT;
> +	
> +	if (cau_action == AQ_SET && cad_action == AQ_CLEAR) {
> +		ret = PWM_POLARITY_NORMAL;
> +	}
> +	else if (cau_action == AQ_CLEAR && cad_action == AQ_SET) {
> +		ret = PWM_POLARITY_INVERSED;
> +	}

I would expect the compiler to issue a warning here about ret being
uninitialized in some situations.

Not sure checkpatch points that out: } and "else" go on the same line.

> +
> +	return ret;
> +}
> +
> +static int ehrpwm_get_hw_state(struct pwm_chip *chip, struct pwm_device *pwm, 
> +								struct pwm_state *state)

The canonical name for this function is ehrpwm_pwm_get_state. And I
would have expected an assignment to .get_state in ehrpwm_pwm_ops.

> +{
> +	int ret;
> +
> +	if(chip == NULL || pwm == NULL || state == NULL){
> +		return -EINVAL;
> +	}
> +
> +	state->enabled = ehrpwm_is_enabled(chip);
> +	state->period = ehrpwm_read_period(chip);
> +    state->duty_cycle = ehrpwm_read_duty_cycle(chip);
> +    state->polarity = ehrpwm_read_polarity(chip);
> +
> +	return ret;
> +}
> +
>  static void ehrpwm_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> @@ -449,8 +575,10 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
>  	struct ehrpwm_pwm_chip *pc;
> +	struct pwm_state state;
>  	struct pwm_chip *chip;
>  	struct clk *clk;
> +	bool tbclk_enabled;
>  	int ret;
>  
>  	chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
> @@ -501,10 +629,31 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, chip);
>  	pm_runtime_enable(&pdev->dev);
>  
> +	ehrpwm_get_hw_state(chip, &chip->pwms[0], &state);
> +	if(state.enabled == true) {

Given that you only use state.enabled here, maybe shorten that to

	if (ehrpwm_is_enabled(...))

> +		ret = clk_prepare_enable(pc->tbclk);

While it's a good idea to enable the clock if the PWM is already running
at probe time, this must happen before pwmchip_add().

> +		if (ret) {     
> +			dev_err(&pdev->dev, "clk_prepare_enable() failed: %d\n", ret);

Please use dev_err_probe() here.

> +			goto err_pwmchip_remove;
> +		}
> +            
> +		tbclk_enabled = true;
> +		ret = pm_runtime_get_sync(&pdev->dev);
> +		if(ret < 0) {
> +			dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d\n", ret);
> +			clk_disable_unprepare(pc->tbclk);
> + 			goto err_pwmchip_remove;
> +		}
> + 	}
> +
>  	return 0;
>  
> +err_pwmchip_remove:
> +	pwmchip_remove(chip);
> +
>  err_clk_unprepare:
> -	clk_unprepare(pc->tbclk);
> +	if(tbclk_enabled)
> +		clk_unprepare(pc->tbclk);
>  
>  	return ret;
>  }

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists