Re: [RFC 3/7] of: Add PWM support.

[Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>]

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

* Stephen Warren wrote:
> Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> > This patch adds helpers to support device-tree bindings for the generic
> > PWM API.
> 
> > diff --git a/drivers/of/pwm.c b/drivers/of/pwm.c
> 
> > +/**
> > + * of_get_named_pwm() - get a PWM number and period to use with the PWM API
> > + * @np:		device node to get the PWM from
> > + * @propname:	property name containing PWM specifier(s)
> > + * @index:	index of the PWM
> > + * @period_ns:	a pointer to the PWM period (in ns) to fill in
> > + *
> > + * Returns PWM number to use with the Linux generic PWM API or a negative
> > + * error code on failure. If @period_ns is not NULL the function fills in
> > + * the value parsed from the period-ns property.
> > + */
> > +int of_get_named_pwm(struct device_node *np, const char *propname,
> > +		     int index, unsigned int *period_ns)
> > +{
> ...
> > +	if ((cells > 1) && period_ns)
> > +		*period_ns = be32_to_cpu(spec[1]);
> 
> What if the client needs to know period_ns, yet the DT doesn't provide
> it?
> 
> I think a better approach would be to use an "of_xlate" function like
> GPIO and IRQ do. This way, the PWM device's own bindings get to define
> what the extra cells mean, and the definition of of_xlate can be such
> that it must return a period_ns value in all cases; in some cases, the
> driver may return a hard-coded value if the HW doesn't support the
> feature, whereas in most cases the value would be parsed from the
> extra cells.
> 
> Without seeing the complete binding documentation and an example, it's
> difficult to think about whether it's a good idea to include period_ns
> in the PWM specifier or not. An alternative might be a property in the
> PWM node itself.

I like the "of_xlate" alternative better. Adding a property to the PWM node
would hard-code the period regardless of the user. That doesn't really
reflect the PWM API.

I will play around with it a bit and make sure to include PWM binding
documentation in the next version.

> Actually, even better would be for struct pwm_chip to contain a struct
> device * instead. That'd allow the PWM core to use that for e.g. dev_err
> calls, and it includes the of_node for use in the match function above.

I like that idea. I'll address that in the next version.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]