Re: [PATCH v1 001/101] pwm: Provide devm_pwmchip_alloc() function

[Thierry Reding <thierry.reding@gmail.com>]

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

On Tue, Aug 08, 2023 at 07:17:51PM +0200, 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().
> 
> 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                            | 30 ++++++++++++++++---
>  include/linux/pwm.h                           |  5 ++++
>  4 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
> index 8be086b3f829..73a9ee074737 100644
> --- a/Documentation/driver-api/driver-model/devres.rst
> +++ b/Documentation/driver-api/driver-model/devres.rst
> @@ -414,6 +414,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 3fdc95f7a1d1..a3824bd58e4c 100644
> --- a/Documentation/driver-api/pwm.rst
> +++ b/Documentation/driver-api/pwm.rst
> @@ -134,11 +134,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 8aa3feec12a9..cfcddf62ab01 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -196,6 +196,31 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
>  	return true;
>  }
>  
> +void *pwmchip_priv(struct pwm_chip *chip)
> +{
> +	return &chip[1];
> +}
> +EXPORT_SYMBOL_GPL(pwmchip_priv);
> +
> +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv)
> +{
> +	struct pwm_chip *chip;
> +	size_t alloc_size;
> +	unsigned int i;
> +
> +	alloc_size = sizeof(*chip) + sizeof_priv;
> +
> +	chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL);

Are you sure this works the way you want it to? If you allocate using
device-managed functions, this memory will be released when the driver
is unbound from the device, so we're basically back to square one,
aren't we?

> +	if (!chip)
> +		return ERR_PTR(-ENOMEM);
> +
> +	chip->dev = parent;
> +	chip->npwm = npwm;
> +
> +	return chip;
> +}
> +EXPORT_SYMBOL_GPL(devm_pwmchip_alloc);
> +
>  /**
>   * __pwmchip_add() - register a new PWM chip
>   * @chip: the PWM chip to add
> @@ -208,8 +233,6 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
>   */
>  int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>  {
> -	struct pwm_device *pwm;
> -	unsigned int i;

Am I missing something? You seem to be using this variable in the for
loop below, so how can you remove it?

Thierry

>  	int ret;
>  
>  	if (!chip || !chip->dev || !chip->ops || !chip->npwm)
> @@ -234,9 +257,8 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>  	}
>  
>  	chip->id = ret;
> -
>  	for (i = 0; i < chip->npwm; i++) {
> -		pwm = &chip->pwms[i];
> +		struct pwm_device *pwm = &chip->pwms[i];
>  
>  		pwm->chip = chip;
>  		pwm->hwpwm = i;
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 6f139784d6f5..3c0da17e193c 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -5,6 +5,7 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/of.h>
> +#include <linux/compiler_attributes.h>
>  
>  struct pwm_chip;
>  
> @@ -380,6 +381,10 @@ static inline void pwm_disable(struct pwm_device *pwm)
>  int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
>  		unsigned long timeout);
>  
> +void *pwmchip_priv(struct pwm_chip *chip) __attribute_const__;
> +
> +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);
> -- 
> 2.40.1
> 

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