[1/2] pwm: Change how pwm_chip IDs are determined

Traditionally each PWM device had a unique ID stored in the "pwm" member
of struct pwm_device. However this number was hardly used and dropped in
the previous commit. Further the unique ID of a PWM chip matched the ID
of the first contained PWM device.

With the PWM device ID gone PWM chips can get their IDs better and
simpler using an idr.

This is expected to change the numbering of PWM chips, but nothing
should rely on the numbering anyhow.

Other than that the side effects are:

 - The PWM chip IDs are smaller and in most cases consecutive.
 - The ordering in /sys/kernel/debug/pwm is ordered by ascending PWM
   chip ID.

Also use "id" as name for the PWM chip ID which better matches the new
semantic of that number.

For the plan to introduce support for pwmchip character devices this
also simplifies getting the struct pwm_chip matching a given PWM chip ID
(or character device).

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c  | 67 +++++++++++++++++----------------------------
 drivers/pwm/sysfs.c |  2 +-
 include/linux/pwm.h |  3 +-
 3 files changed, 27 insertions(+), 45 deletions(-)

On Tue, Aug 08, 2023 at 06:52:49PM +0200, Uwe Kleine-König wrote:
> Traditionally each PWM device had a unique ID stored in the "pwm" member
> of struct pwm_device. However this number was hardly used and dropped in
> the previous commit. Further the unique ID of a PWM chip matched the ID
> of the first contained PWM device.

This is slightly confusing. The PWM chip "base" was never supposed to be
an ID and its usage in sysfs was more of an accident than actual
purpose. The primary purpose of it was to serve as the base number of
pins in the global number space, but with that number space gone, there
is no use for the base number anymore.

So I think changing the subject to something like this would be better:

	pwm: Replace PWM chip unique base by unique ID

> With the PWM device ID gone PWM chips can get their IDs better and
> simpler using an idr.
> 
> This is expected to change the numbering of PWM chips, but nothing
> should rely on the numbering anyhow.
> 
> Other than that the side effects are:
> 
>  - The PWM chip IDs are smaller and in most cases consecutive.
>  - The ordering in /sys/kernel/debug/pwm is ordered by ascending PWM
>    chip ID.
> 
> Also use "id" as name for the PWM chip ID which better matches the new
> semantic of that number.
> 
> For the plan to introduce support for pwmchip character devices this
> also simplifies getting the struct pwm_chip matching a given PWM chip ID
> (or character device).

Again, it would be interesting to see how this would work in practice.
As things are this is pure speculation, so I'd leave out this comment.

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c  | 67 +++++++++++++++++----------------------------
>  drivers/pwm/sysfs.c |  2 +-
>  include/linux/pwm.h |  3 +-
>  3 files changed, 27 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index dc66e3405bf5..9b1eb37e2e73 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/module.h>
> +#include <linux/idr.h>
>  #include <linux/of.h>
>  #include <linux/pwm.h>
>  #include <linux/list.h>
> @@ -23,52 +24,25 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/pwm.h>
>  
> -#define MAX_PWMS 1024
> -
>  static DEFINE_MUTEX(pwm_lookup_lock);
>  static LIST_HEAD(pwm_lookup_list);
>  
> -/* protects access to pwm_chips and allocated_pwms */
> +/* protects access to pwmchip_idr */
>  static DEFINE_MUTEX(pwm_lock);
>  
> -static LIST_HEAD(pwm_chips);
> -static DECLARE_BITMAP(allocated_pwms, MAX_PWMS);
> -
> -/* Called with pwm_lock held */
> -static int alloc_pwms(unsigned int count)
> -{
> -	unsigned int start;
> -
> -	start = bitmap_find_next_zero_area(allocated_pwms, MAX_PWMS, 0,
> -					   count, 0);
> -
> -	if (start + count > MAX_PWMS)
> -		return -ENOSPC;
> -
> -	bitmap_set(allocated_pwms, start, count);
> -
> -	return start;
> -}
> -
> -/* Called with pwm_lock held */
> -static void free_pwms(struct pwm_chip *chip)
> -{
> -	bitmap_clear(allocated_pwms, chip->base, chip->npwm);
> -
> -	kfree(chip->pwms);
> -	chip->pwms = NULL;
> -}
> +DEFINE_IDR(pwmchip_idr);

static? I would probably have kept the same pwm_chips as well. We
already know that this is an IDR given the type, so no need to repeat
that.

>  static struct pwm_chip *pwmchip_find_by_name(const char *name)
>  {
>  	struct pwm_chip *chip;
> +	unsigned long id, tmp;
>  
>  	if (!name)
>  		return NULL;
>  
>  	mutex_lock(&pwm_lock);
>  
> -	list_for_each_entry(chip, &pwm_chips, list) {
> +	idr_for_each_entry_ul(&pwmchip_idr, chip, tmp, id) {
>  		const char *chip_name = dev_name(chip->dev);
>  
>  		if (chip_name && strcmp(chip_name, name) == 0) {
> @@ -278,14 +252,14 @@ int pwmchip_add(struct pwm_chip *chip)
>  
>  	mutex_lock(&pwm_lock);
>  
> -	ret = alloc_pwms(chip->npwm);
> +	ret = idr_alloc(&pwmchip_idr, chip, 0, 0, GFP_KERNEL);
>  	if (ret < 0) {
>  		mutex_unlock(&pwm_lock);
>  		kfree(chip->pwms);
>  		return ret;
>  	}
>  
> -	chip->base = ret;
> +	chip->id = ret;
>  
>  	for (i = 0; i < chip->npwm; i++) {
>  		pwm = &chip->pwms[i];
> @@ -295,8 +269,6 @@ int pwmchip_add(struct pwm_chip *chip)
>  		pwm->hwpwm = i;
>  	}
>  
> -	list_add(&chip->list, &pwm_chips);
> -
>  	mutex_unlock(&pwm_lock);
>  
>  	if (IS_ENABLED(CONFIG_OF))
> @@ -323,11 +295,11 @@ void pwmchip_remove(struct pwm_chip *chip)
>  
>  	mutex_lock(&pwm_lock);
>  
> -	list_del_init(&chip->list);
> -
> -	free_pwms(chip);
> +	idr_remove(&pwmchip_idr, chip->id);
>  
>  	mutex_unlock(&pwm_lock);
> +
> +	kfree(chip->pwms);
>  }
>  EXPORT_SYMBOL_GPL(pwmchip_remove);
>  
> @@ -623,10 +595,11 @@ EXPORT_SYMBOL_GPL(pwm_adjust_config);
>  static struct pwm_chip *fwnode_to_pwmchip(struct fwnode_handle *fwnode)
>  {
>  	struct pwm_chip *chip;
> +	unsigned long id, tmp;
>  
>  	mutex_lock(&pwm_lock);
>  
> -	list_for_each_entry(chip, &pwm_chips, list)
> +	idr_for_each_entry_ul(&pwmchip_idr, chip, tmp, id)
>  		if (chip->dev && device_match_fwnode(chip->dev, fwnode)) {
>  			mutex_unlock(&pwm_lock);
>  			return chip;
> @@ -1085,17 +1058,27 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>  
>  static void *pwm_seq_start(struct seq_file *s, loff_t *pos)
>  {
> +	unsigned long id = *pos;
> +	void *ret;
> +
>  	mutex_lock(&pwm_lock);
>  	s->private = "";
>  
> -	return seq_list_start(&pwm_chips, *pos);
> +	ret = idr_get_next_ul(&pwmchip_idr, &id);
> +	*pos = id;
> +	return ret;
>  }
>  
>  static void *pwm_seq_next(struct seq_file *s, void *v, loff_t *pos)
>  {
> +	unsigned long id = *pos + 1;
> +	void *ret;
> +
>  	s->private = "\n";
>  
> -	return seq_list_next(v, &pwm_chips, pos);
> +	ret = idr_get_next_ul(&pwmchip_idr, &id);
> +	*pos = id;
> +	return ret;
>  }
>  
>  static void pwm_seq_stop(struct seq_file *s, void *v)
> @@ -1105,7 +1088,7 @@ static void pwm_seq_stop(struct seq_file *s, void *v)
>  
>  static int pwm_seq_show(struct seq_file *s, void *v)
>  {
> -	struct pwm_chip *chip = list_entry(v, struct pwm_chip, list);
> +	struct pwm_chip *chip = v;
>  
>  	seq_printf(s, "%s%s/%s, %d PWM device%s\n", (char *)s->private,
>  		   chip->dev->bus ? chip->dev->bus->name : "no-bus",
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 8d1254761e4d..4edb994fa2e1 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -510,7 +510,7 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
>  	 * the kernel it's just not exported.
>  	 */
>  	parent = device_create(&pwm_class, chip->dev, MKDEV(0, 0), chip,
> -			       "pwmchip%d", chip->base);
> +			       "pwmchip%d", chip->id);
>  	if (IS_ERR(parent)) {
>  		dev_warn(chip->dev,
>  			 "device_create failed for pwm_chip sysfs export\n");
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index d2f9f690a9c1..09da803bbd46 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -295,7 +295,7 @@ struct pwm_ops {
>  struct pwm_chip {
>  	struct device *dev;
>  	const struct pwm_ops *ops;
> -	int base;
> +	int id;

Why not make this unsigned long while we're at it? Looks like that's the
native type for IDRs and we already make sure that whatever is stored in
here isn't going to be negative.

All of the above are minor changes, so I can fix them up as I apply the
patch, if you don't have any objections.

Thierry

Hello Thierry,

On Fri, Oct 06, 2023 at 01:03:50PM +0200, Thierry Reding wrote:
> On Tue, Aug 08, 2023 at 06:52:49PM +0200, Uwe Kleine-König wrote:
> > Traditionally each PWM device had a unique ID stored in the "pwm" member
> > of struct pwm_device. However this number was hardly used and dropped in
> > the previous commit. Further the unique ID of a PWM chip matched the ID
> > of the first contained PWM device.
> 
> This is slightly confusing. The PWM chip "base" was never supposed to be
> an ID and its usage in sysfs was more of an accident than actual
> purpose. The primary purpose of it was to serve as the base number of
> pins in the global number space, but with that number space gone, there
> is no use for the base number anymore.
> 
> So I think changing the subject to something like this would be better:
> 
> 	pwm: Replace PWM chip unique base by unique ID

Agreed.

> > With the PWM device ID gone PWM chips can get their IDs better and
> > simpler using an idr.
> > 
> > This is expected to change the numbering of PWM chips, but nothing
> > should rely on the numbering anyhow.
> > 
> > Other than that the side effects are:
> > 
> >  - The PWM chip IDs are smaller and in most cases consecutive.
> >  - The ordering in /sys/kernel/debug/pwm is ordered by ascending PWM
> >    chip ID.
> > 
> > Also use "id" as name for the PWM chip ID which better matches the new
> > semantic of that number.
> > 
> > For the plan to introduce support for pwmchip character devices this
> > also simplifies getting the struct pwm_chip matching a given PWM chip ID
> > (or character device).
> 
> Again, it would be interesting to see how this would work in practice.
> As things are this is pure speculation, so I'd leave out this comment.

That's just that you can lookup by ID in an idr, while you have to
iterate through a linked list otherwise. If you want drop the last
paragraph?!

> > [...]
> > -static void free_pwms(struct pwm_chip *chip)
> > -{
> > -	bitmap_clear(allocated_pwms, chip->base, chip->npwm);
> > -
> > -	kfree(chip->pwms);
> > -	chip->pwms = NULL;
> > -}
> > +DEFINE_IDR(pwmchip_idr);
> 
> static?

Agreed.

> I would probably have kept the same pwm_chips as well. We already know
> that this is an IDR given the type, so no need to repeat that.

You mean:

	static DEFINE_IDR(pwm_chips);

? I don't care much. I probably copied that from spi. A quick grep shows
_idr being quite typical:

	$ git grep DEFINE_IDR v6.6-rc1 | perl -n -e 'print if s/.*DEFINE_IDR\s*\(.*(_.*)\).*/\1/' | sort | uniq -c      1 _devices
	      4 _id
	     53 _idr
	      2 _index
	      1 _INIT(name
	      1 _list
	      3 _minors
	      1 _protocols
	      1 _stats

But if you prefer pwm_chips and take the patch then, I won't oppose.

> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index d2f9f690a9c1..09da803bbd46 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -295,7 +295,7 @@ struct pwm_ops {
> >  struct pwm_chip {
> >  	struct device *dev;
> >  	const struct pwm_ops *ops;
> > -	int base;
> > +	int id;
> 
> Why not make this unsigned long while we're at it? Looks like that's the
> native type for IDRs and we already make sure that whatever is stored in
> here isn't going to be negative.

Agreed.

> All of the above are minor changes, so I can fix them up as I apply the
> patch, if you don't have any objections.

That's fine for me.

Thanks
Uwe