Re: [PATCH 4/8] pwm: Export pwm_get_state_hw

Uwe Kleine-König
2024-09-04T10:08:07Z

[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]

On Thu, Aug 15, 2024 at 12:11:58PM +0000, Guillaume Stols wrote:
> This function can be used in some other drivers, for instance when we
> want to retrieve the real frequency vs the one that was asked.

I'd write:

For some drivers (here: the upcoming ad7606 adc driver) it's important
to know the actually configured PWM state. This is in general different
from the state returned by pwm_get_state() (i.e. the last applied state)
because most hardware doesn't have nano second granularity. So make
pwm_get_state_hw() a public function.

> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 21fca27bb8a3..82e05ed88310 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -651,7 +651,7 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
> }
> EXPORT_SYMBOL_GPL(pwm_apply_atomic);
>
> -static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
> +int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
> {
> struct pwm_chip *chip = pwm->chip;
> const struct pwm_ops *ops = chip->ops;
> @@ -685,6 +685,7 @@ static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(pwm_get_state_hw);

Now that this is a public function, a kernel doc for it would be nice.

> /**
> * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index fd100c27f109..d48ea3051e28 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -369,6 +369,7 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
> int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
> int pwm_adjust_config(struct pwm_device *pwm);
>
> +int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state);

Nitpick: pwm_get_state_hw() is defined in core.c before
pwm_adjust_config(). Please keep this order in the header.

> /**
> * pwm_config() - change a PWM device configuration
> * @pwm: PWM device

Your patch was PGP signed, but I failed to find your key in the kernel
key repo and on https://keys.openpgp.org. To make your signature
actually useful, you might want to fix that.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]