Re: [PATCH v4] media: rc: pwm-ir-tx: Switch to atomic PWM API

[Uwe Kleine-König]

On Wed, Oct 27, 2021 at 12:34:30PM -0300, Maíra Canal wrote:
> Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
> replace it for the atomic PWM API.
> 
> Signed-off-by: Maíra Canal <maira.canal@xxxxxx>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>

While it's true that he kernel robot told you about a problem in an
earlier revision, adding the tag here is misleading, because in the end
you only see the tag in the commit history, and there is suggests that
the commit fixes something that was reported in the kernel tree before.

For this reason I usually only add a thanks after the tripple dash.

Also note this nitpick: Your S-o-b should always be the last thing.

> ---
> V1 -> V2: Assign variables directly and simplify conditional statement
> V2 -> V3: Fix declaration of undeclared variables
> V3 -> V4: Fix DIV_ROUND_CLOSEST error with u64 variables
> ---
>  drivers/media/rc/pwm-ir-tx.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> index 4bc28d2c9cc9..105a9c24f1e3 100644
> --- a/drivers/media/rc/pwm-ir-tx.c
> +++ b/drivers/media/rc/pwm-ir-tx.c
> @@ -53,22 +53,21 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
>  {
>  	struct pwm_ir *pwm_ir = dev->priv;
>  	struct pwm_device *pwm = pwm_ir->pwm;
> -	int i, duty, period;
> +	struct pwm_state state;
> +	int i;
>  	ktime_t edge;
>  	long delta;
>  
> -	period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
> -	duty = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * period, 100);
> +	pwm_init_state(pwm, &state);
>  
> -	pwm_config(pwm, duty, period);
> +	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
> +	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
>  
>  	edge = ktime_get();
>  
>  	for (i = 0; i < count; i++) {
> -		if (i % 2) // space
> -			pwm_disable(pwm);
> -		else
> -			pwm_enable(pwm);
> +		state.enabled = !(i % 2);
> +		pwm_apply_state(pwm, &state);
>  
>  		edge = ktime_add_us(edge, txbuf[i]);
>  		delta = ktime_us_delta(edge, ktime_get());
> @@ -76,7 +75,8 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
>  			usleep_range(delta, delta + 10);
>  	}
>  
> -	pwm_disable(pwm);
> +	state.enabled = false;
> +	pwm_apply_state(pwm, &state);
>  
>  	return count;
>  }

The conversion is right (I think), note this could be optimized a bit
further: state.period only depends on carrier which rarely changes, so
the calculation could be done in pwm_ir_set_carrier(). Ditto for duty
which only depends on state.period and pwm_ir->duty_cycle. (This is for
a separate commit though.)

Acked-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>

Thanks
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature