Re: [PATCH 1/2] pwm: Add different PWM output types support

[Uwe Kleine-König]

On Mon, Sep 23, 2019 at 10:43:43PM -0700, Guru Das Srinagesh wrote:
> On Mon, Sep 16, 2019 at 04:01:46PM +0200, Thierry Reding wrote:
> > On Fri, Sep 13, 2019 at 03:57:43PM -0700, Guru Das Srinagesh wrote:
> > > From: Fenglin Wu <fenglinw@xxxxxxxxxxxxxx>
> > > 
> > > Normally, PWM channel has fixed output until software request to change
> > > its settings. There are some PWM devices which their outputs could be
> > > changed autonomously according to a predefined pattern programmed in
> > > hardware. Add pwm_output_type enum type to identify these two different
> > > PWM types and add relevant helper functions to set and get PWM output
> > > types and pattern.
> > > 
> > > Change-Id: Ia1f914a45ab4f4dd7be037a395eeb89d0e65a80e
> > > Signed-off-by: Fenglin Wu <fenglinw@xxxxxxxxxxxxxx>
> > > Signed-off-by: Guru Das Srinagesh <gurus@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/pwm/core.c  | 26 ++++++++++++++++++++
> > >  drivers/pwm/sysfs.c | 50 ++++++++++++++++++++++++++++++++++++++
> > >  include/linux/pwm.h | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 146 insertions(+)
> > 
> > This doesn't seem right to me. Are you describing a PWM pin that's
> > actually driven in GPIO mode? We usually configure that using pinctrl.
> > 
> > Thierry
> 
> Sorry, let me clarify.
> 
> Some Qualcomm PMICs have a PWM block called the Light Pulse Generator (LPG).
> This block allows for the generation of a HW-controlled PWM "pattern", i.e. a
> sequential altering of duty cycle, in addition to the normal PWM "fixed" duty
> cycle operation, which is what the framework does currently. This pattern is
> user-configurable in the form of a look-up table in the devicetree. The LPG's
> registers have to be configured with the data in the look up table in order to
> start the generation of the pattern. An example of a pattern is the "breath"
> pattern, which simply ramps up the duty cycle and then ramps it down.

I'll try to describe it in my words to check if I got it right: So the
mode you want to add needs a sequence of PWM states and the hardware is
expected to apply them in turn, each for a configurable count of
periods. If I understand this right, this is expected to be cyclic?

> This "pattern" mode is what has been defined as PWM_OUTPUT_MODULATED in this
> patch. I see that the use of the term "modulated" is misleading - a more
> accurate term would be PWM_OUTPUT_PATTERN perhaps.

Not sure "pattern" is better. 

The PWM on the newer imx SoCs (using the imx27 driver) has a FIFO with
length 4 that allows to program changing settings. Only the duty cycle
can be modified and as repeat count only 1, 2, 4 and 8 are available. I
assume the FIFO can be fed by the dma engine.

Note I only know this feature from reading the reference manual and
never used it.

> This patch merely adds framework support to differentiate between the "fixed"
> and "pattern" modes of operation. Actions such as configuring the LPG with the
> devicetree pattern and setting it up for generating the pattern are performed
> in the driver only if the output type is read as "pattern" and not otherwise.

Up to now I'm not convinced that this extension is a good one that can
be supported by several PWM implementations. I'd say we should collect
first some details about different implementations and what these could
implement to get a picture what kind of API is sensible.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |