Re: [PATCH v2] pwm: add pca9685 driver

[Thierry Reding]

On Tue, May 28, 2013 at 09:38:01AM +0200, Steffen Trumtrar wrote:
> On Mon, May 27, 2013 at 09:13:34PM +0200, Thierry Reding wrote:
> > On Mon, May 27, 2013 at 11:28:27AM +0200, Steffen Trumtrar wrote:
[...]
> > > +Optional properties:
> > > +  - invrt:  <0> output logic state not inverted
> > > +	    <1> output logic state inverted
> > 
> > Why not make this "invert"?
> > 
> 
> I used the naming of NXPs datasheet, but I can change that.

I didn't say it explicitly, but "invert" should also be a boolean
property.

> > > +  - outdrv: <0> configure outputs with open-drain structure
> > > +	    <1> configure outputs with totem pole structure
> > 
> > And this could be a boolean property called "totem-pole"? I think that's
> > much more intuitive to use.
> > 
> 
> This is also a register field from NXP. The datasheet references the invrt and
> outdrv bit in different locations to describe some use case and the corresponding
> setting of this two bits. That is why I thought it might be easier to use.
> Get use case from datasheet -> put corresponding value in DT -> done.
> On the other hand, the DT might be easier to understand with a boolean property.

That was exactly my reasoning. I think it's good and meaningful to
mirror the naming of the datasheet in the driver source code, but the DT
description can be much more intuitive by using common language instead
of some abbreviation taken from the datasheet.

> I would than go for "open-drain" though, as totem-pole is the reset default.

Sounds good.

> > > +	if (duty_ns < 1)
> > > +		return 0;
> > 
> > Doesn't duty_ns < 1 mean the signal is off? Why are you ignoring this
> > case?
> > 
> 
> I just tested with led-pwm which calls config and then disable.
> So, I don't need to do any setup in this case. I don't know, if that is
> PWM framework or driver specific. Maybe I do have to do something in this
> case? I will have a look.

Generally it can't be assumed that every user will call pwm_disable()
after configuring the duty-cycle to be 0. Both of the in-kernel users do
that, but it's not a strict requirement by the API. Calling
pwm_disable() typically results in the PWM signal being switched off.
pwm_config() with a zero duty-cycle isn't necessarily the same thing,
even if there is often no way to distinguish between both cases on a
physical level.

> > > +	/* the PWM subsystem does not support a pre-delay */
> > 
> > What do you mean by that? How is it relevant to this code. Maybe there's
> > a case to be made to add functionality that you miss.
> > 
> 
> Well, you are right. The PCA supports setting two delays: start of duty cycle and end of
> duty cycle. The PWM subsystem AFAIK only supports setting the length of the duty cycle.
> Which results in me setting the start of duty cycle to 0.
> If other PWM chips also support this and there actually is a use case for this,
> than that might be a functionality missing from the PWM subsystem.

So the PCA doesn't define the length of the duty-cycle, but rather an
offset where it starts as well as an offset where it ends? That's very
different from other chips we currently support. I don't think any of
them can do this.

On a related note, there's a bit of a gray area in the PWM subsystem
when it comes to what the PWM signal is supposed to look like. This
first came up when a chip was added that could invert the pin output.
None of our use-cases currently care about the exact form of the signal
and are only interested in the power delivered by the signal (LED,
backlight).

As you say, there's currently no use-case that requires this
functionality, so little sense it adding support now.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature