RE: [RFC 1/7] PWM: add pwm framework support

[Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> This patch adds framework support for PWM (pulse width modulation) devices.
...

> diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt

> +Identifying PWMs
> +----------------
> +
> +PWMs are identified by unique ids throughout the system. A platform
> +should call pwmchip_reserve() during init time to reserve the id range
> +for internal PWMs so that users have a fixed id to refer to specific
> +PWMs.

pwmchip_reserve() doesn't seem to exist in this patch.

> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c

> +/**
> + * pwmchip_add() - register a new pwm
> + * @chip: the pwm
> + *
> + * register a new pwm. pwm->pwm_id must be initialized. if pwm_id < 0 then
> + * a dynamically assigned id will be used, otherwise the id specified,

I don't see any code that assigns pwm_id if it's negative.

> + */
> +int pwmchip_add(struct pwm_chip *chip)
> +{
> +	struct pwm_device *pwm;
> +	int ret = 0;
> +
> +	pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->chip = chip;
> +
> +	mutex_lock(&pwm_lock);
> +
> +	if (chip->pwm_id >= 0 && _find_pwm(chip->pwm_id)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	list_add_tail(&pwm->node, &pwm_list);
> +out:
> +	mutex_unlock(&pwm_lock);
> +
> +	if (ret)
> +		kfree(pwm);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pwmchip_add);

There have been discussions before re: GPIOs and interrupts that creating
a global namespace for them was a mistake. This new PWM API does the same
thing. Is there an issue here?

I suppose that coming at it purely from a device tree perspective, the
numbering scheme doesn't matter, since it can be dynamically assigned
in the same way that global GPIO and IRQ IDs are with device tree. Still,
there's plenty of non-device tree stuff around, so this is probably worth
thinking about.

Is 0 a valid ID? Given previous GPIO and IRQ discussions, we should
disallow 0 explicitly. The rationale is that such an invalid ID can be
used to indicate "no PWM", and 0 is a good value for that, since it's
what uninitialized global data gets set to.

-- 
nvpublic