lkml.org 
[lkml]   [2020]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element
    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.

    > 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

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

    \
     
     \ /
      Last update: 2020-02-26 20:22    [W:5.066 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site