Re: [PATCH] pwm-pca9685: Allow any of the 16 PWMs to be used as a GPIO

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

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

On Thu, Oct 20, 2016 at 04:01:17PM +0300, Mika Westerberg wrote:
> On Thu, Oct 20, 2016 at 02:51:44PM +0200, Thierry Reding wrote:
> > On Thu, Oct 20, 2016 at 02:18:44PM +0300, Mika Westerberg wrote:
> > > On Thu, Oct 20, 2016 at 12:45:41PM +0200, Thierry Reding wrote:
> > > > On Wed, Oct 19, 2016 at 09:56:38PM +0300, Mika Westerberg wrote:
> > > > > On Tue, Sep 20, 2016 at 05:40:56PM +0300, Mika Westerberg wrote:
> > > > > > The PCA9685 controller has full on/off bit for each PWM channel. Setting
> > > > > > this bit bypasses the PWM control and the line works just as it would be a
> > > > > > GPIO. Furthermore in Intel Galileo it is actually used as GPIO output for
> > > > > > discreet muxes on the board.
> > > > > > 
> > > > > > This patch adds GPIO output only support for the driver so that we can
> > > > > > control the muxes on Galileo using standard GPIO interfaces available in
> > > > > > the kernel. GPIO and PWM functionality is exclusive so only one can be
> > > > > > active at a time on a single PWM channel.
> > > > > > 
> > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > 
> > > > > Thierry,
> > > > > 
> > > > > Any comments on this?
> > > > 
> > > > It seems to me like maybe pinmux would be a better framework to handle
> > > > this kind of use case. The full on/off bit sounds like it's a mux that
> > > > selects between GPIO and PWM functionality.
> > > > 
> > > > Have you thought about supporting pinmux for this?
> > > 
> > > Adding a full pinmux just for this sounds a bit overkill if you ask me.
> > > 
> > > How about adding that ->gpio_set() callback in pwm_ops and let the PWM
> > > core to export a GPIO chip in that case? That would support also other
> > > PWMs having similar full on/off capability without the need to add a new
> > > pinmux driver for each.
> > 
> > I'm reluctant to add this to the PWM core because it's effectively
> > duplicating something for which a proper subsystem already exists.
> 
> OK, I see.
> 
> > If adding pinmux is considered overkill, maybe doing so needs to be
> > simplified. Surely if this is applicable to more than one PWM controller
> > some helpers could be extracted to make it easier to add.
> > 
> > But if it isn't generally useful I don't think it makes sense to add it
> > to the PWM core either. So I think our choices here are to register via
> > pinmux and in the process add helpers to make that easier (remove the
> > overkill) or to keep this to the driver until we start seeing a pattern
> > emerge.
> 
> I would then go for keeping the code in this single driver for now and
> re-think this later if other drivers start to develop similar.

That's fine with me. I'll take a closer look at this patch and apply if
I don't find anything out of place.

Thierry

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