Re: [PATCH v5 040/111] pwm: Provide devm_pwmchip_alloc() function

[Alex Elder <elder@ieee.org>]

On 1/25/24 6:09 AM, Uwe Kleine-König wrote:
> This function allocates a struct pwm_chip and driver data. Compared to
> the status quo the split into pwm_chip and driver data is new, otherwise
> it doesn't change anything relevant (yet).
> 
> The intention is that after all drivers are switched to use this
> allocation function, its possible to add a struct device to struct
> pwm_chip to properly track the latter's lifetime without touching all
> drivers again. Proper lifetime tracking is a necessary precondition to
> introduce character device support for PWMs (that implements atomic
> setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm
> userspace support).
> 
> The new function pwmchip_priv() (obviously?) only works for chips
> allocated with devm_pwmchip_alloc().

I think this looks good.  Two questions:
- Should you explicitly align the private data?  Or do you believe
   the default alignment (currently pointer size aligned) is adequate?
- Is there a non-devres version of the allocation function?

					-Alex

> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   .../driver-api/driver-model/devres.rst        |  1 +
>   Documentation/driver-api/pwm.rst              | 10 ++++----
>   drivers/pwm/core.c                            | 25 +++++++++++++++++++
>   include/linux/pwm.h                           |  2 ++
>   4 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
> index c5f99d834ec5..e4df72c408d2 100644
> --- a/Documentation/driver-api/driver-model/devres.rst
> +++ b/Documentation/driver-api/driver-model/devres.rst
> @@ -420,6 +420,7 @@ POWER
>     devm_reboot_mode_unregister()
>   
>   PWM
> +  devm_pwmchip_alloc()
>     devm_pwmchip_add()
>     devm_pwm_get()
>     devm_fwnode_pwm_get()
> diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
> index 3c28ccc4b611..cee66c7f0335 100644
> --- a/Documentation/driver-api/pwm.rst
> +++ b/Documentation/driver-api/pwm.rst
> @@ -143,11 +143,11 @@ to implement the pwm_*() functions itself. This means that it's impossible
>   to have multiple PWM drivers in the system. For this reason it's mandatory
>   for new drivers to use the generic PWM framework.
>   
> -A new PWM controller/chip can be added using pwmchip_add() and removed
> -again with pwmchip_remove(). pwmchip_add() takes a filled in struct
> -pwm_chip as argument which provides a description of the PWM chip, the
> -number of PWM devices provided by the chip and the chip-specific
> -implementation of the supported PWM operations to the framework.
> +A new PWM controller/chip can be allocated using devm_pwmchip_alloc, then added
> +using pwmchip_add() and removed again with pwmchip_remove(). pwmchip_add()
> +takes a filled in struct pwm_chip as argument which provides a description of
> +the PWM chip, the number of PWM devices provided by the chip and the
> +chip-specific implementation of the supported PWM operations to the framework.
>   
>   When implementing polarity support in a PWM driver, make sure to respect the
>   signal conventions in the PWM framework. By definition, normal polarity
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 1b4c3d0caa82..b821a2b0b172 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -454,6 +454,31 @@ of_pwm_single_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
>   }
>   EXPORT_SYMBOL_GPL(of_pwm_single_xlate);
>   
> +static void *pwmchip_priv(struct pwm_chip *chip)
> +{
> +	return (void *)chip + sizeof(*chip);
> +}
> +
> +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv)
> +{
> +	struct pwm_chip *chip;
> +	size_t alloc_size;
> +
> +	alloc_size = size_add(sizeof(*chip), sizeof_priv);
> +
> +	chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL);
> +	if (!chip)
> +		return ERR_PTR(-ENOMEM);
> +
> +	chip->dev = parent;
> +	chip->npwm = npwm;
> +
> +	pwmchip_set_drvdata(chip, pwmchip_priv(chip));
> +
> +	return chip;
> +}
> +EXPORT_SYMBOL_GPL(devm_pwmchip_alloc);
> +
>   static void of_pwmchip_add(struct pwm_chip *chip)
>   {
>   	if (!chip->dev || !chip->dev->of_node)
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 2c49d2fe2fe7..8bc7504aa7d4 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -403,6 +403,8 @@ static inline bool pwm_might_sleep(struct pwm_device *pwm)
>   int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
>   		unsigned long timeout);
>   
> +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv);
> +
>   int __pwmchip_add(struct pwm_chip *chip, struct module *owner);
>   #define pwmchip_add(chip) __pwmchip_add(chip, THIS_MODULE)
>   void pwmchip_remove(struct pwm_chip *chip);