lkml.org 
[lkml]   [2020]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: (EXT) Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element
    From
    Date
    On Wed, 2020-02-26 at 20:21 +0100, Uwe Kleine-König wrote:
    > On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote:
    > > On Wed, 2020-02-26 at 16:10 +0100, Uwe Kleine-König wrote:
    > > > Hello Matthias,
    > > >
    > > > as you seem to have this hardware on your desk, it would be great
    > > > if
    > > > you
    > > > could answer the following questions:
    > > >
    > > > - Does the hardware complete the currently running period before
    > > > applying a new setting?
    > >
    > > The datasheet claims:
    > >
    > > > Because the loading of the LEDn_ON and LEDn_OFF registers is via
    > > > the
    > > > I 2 C-bus, and
    > > > asynchronous to the internal oscillator, we want to ensure that
    > > > we do
    > > > not see any visual
    > > > artifacts of changing the ON and OFF values. This is achieved by
    > > > updating the changes at
    > > > the end of the LOW cycle.
    > >
    > > My interpretation is that the hardware will complete its period
    > > before
    > > applying the new settings. I might check with a scope tomorrow-ish.
    >
    > I agree given that you can update duty_cycle and period in a single
    > write as you considered below. Maybe it is worth playing with small
    > periods and a slow i2c bus speed (or hijack the bus by simulating a
    > clock stretch).
    >
    > > > - Is this racy somehow (i.e. can it happen that when going from
    > > > duty_cycle/period = 1000/5000 to duty_cycle/period =
    > > > 4000/10000 the
    > > > output is 1000/10000 (or 4000/5000) for one cycle)?
    > >
    > > It currently is racy. It should be possible to fix that either by
    > > updating all 4 registers in a single I2C write, or by using the
    > > "update
    > > on ACK" mode which requires all 4 registers to be updated before
    > > the
    > > new setting is applied (I'm not sure if this mode would require
    > > using a
    > > single I2C write as well though).
    >
    > I can offer a second pair of eyeballs to interpret the datasheet.
    > Will
    > take a look tomorrow.
    >
    > > > - Does the hardware complete the currently running period before
    > > > .enabled = false is configured?
    > >
    > > As my interpretation is that new settings are applied
    > > asynchronously, I
    > > assume that the final running period is completed after .enabled is
    > > set
    > > to false.
    > >
    > > > - How does the output pin behave on a disabled PWM. (Usual
    > > > candidates
    > > > are: freeze where is just happens to be, constant inactive and
    > > > High-Z).
    > >
    > > Constant inactive. This is also the case when the chip is put into
    > > sleep mode. Note that the interpretation of "inactive" depends in
    > > the
    > > invert flag in the MODE2 register.
    >
    > This is optimal.
    >
    > > As it turns out, this driver is broken in yet another way I didn't
    > > find
    > > before: For changing the global prescaler the chip needs to be put
    > > into
    > > sleep mode, but the driver doesn't follow the restart sequence
    > > described in the datasheet when waking it back up. In consequence,
    > > changing the period of one PWM does not only modify the period of
    > > all
    > > PWMs (which is bad enough, but can't be avoided with this
    > > hardware),
    > > but it also leaves all PWMs disabled...
    > >
    > > As this hardware only has a single prescaler for all PWMs, should
    > > changing the period for individual PWMs even be allowed at all?
    > > Maybe
    > > only when all other PWMs are inactive?
    >
    > yes, that is the general approach. Please document this in a
    > Limitiations: paragraph. See drivers/pwm/pwm-imx-tpm.c which has a
    > similar problem.

    This raises the question what to do about the GPIO mode supported by
    the driver: While the period does not affect GPIO usage of PWMs,
    changing the period would put the chip in sleep mode and thus briefly
    disable active GPIOs. I assume that this should preclude changing the
    period when there are any PWMs requsted as GPIOs, but now the order in
    which things are initialized becomes crucial:

    - All references to PWMs of the same PCA9685 must specify the same
    period
    - When requesting a PWM as GPIO, no period can be specified
    => When a PWM referenced as GPIO is requested before the first actual
    PWM, setting the correct period becomes impossible.

    I can't think of a nice solution that doesn't require serious rework -
    maybe we need at least an optional period property in DTS to support
    this case? This could either be implemented as a default period or a
    fixed period.

    A more elaborate solution could be to remove the GPIO code from PCA9685
    and implement a generic GPIO-PWM driver instead that could be
    configured in DTS (again, I have no idea how to support non-DTS
    platforms). Unfortunately, I assume I won't have time to realize such a
    solution myself.

    Matthias


    >
    > > I could imagine setting it in DTS instead (but I'm not sure what to
    > > do
    > > about non-OF users of this driver, for example when configured via
    > > ACPI).
    >
    > I don't like fixing the period in the device tree. This isn't a
    > hardware
    > property and it is less flexible than possible.
    >
    > Best regards
    > Uwe
    >

    \
     
     \ /
      Last update: 2020-02-28 14:30    [W:4.173 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site