linux-kernel - Re: [PATCH 2/2] pwm: pca9685: fix prescaler initialization

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1484829279.2133.236.camel@linux.intel.com>
Date:   Thu, 19 Jan 2017 14:34:39 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Clemens Gruber <clemens.gruber@...ruber.com>,
        Thierry Reding <thierry.reding@...il.com>
Cc:     linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Florian Vaussard <florian.vaussard@...g-vd.ch>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>
Subject: Re: [PATCH 2/2] pwm: pca9685: fix prescaler initialization

On Wed, 2017-01-18 at 15:25 +0100, Clemens Gruber wrote:
> On Wed, Jan 18, 2017 at 04:01:58PM +0200, Andy Shevchenko wrote:
> > On Wed, 2017-01-18 at 14:53 +0100, Clemens Gruber wrote:
> > > Yes, but the period could be different, maybe modified in the
> > > bootloader
> > > or at a previous boot without hardware reset in between. (We do
> > > not
> > > send
> > > a SWRST to the chip, so the period register could be different)
> > 
> > It's fragile to rely on some external settings, right? Wouldn't be
> > better to leave device in a known state after ->probe()?
> 
> Yes, that's what this patch tries to solve by verifying that the
> external setting (the prescale register) is set to its hardware
> default
> value of 0x1E (corresponding to a period of 1/200 Hz).
> If it is not 0x1E, the driver will reconfigure the prescaler according
> to the desired period at the time of the next configuration.

Yes, and my question is what is possible go wrong if you just enforce
prescaler to be 1/200Hz?

> > > Until now, we assumed it is always 1/200 Hz and skipped the
> > > lengthy
> > > prescale configuration (put chip into sleep mode, set prescaler,
> > > go
> > > out
> > > of sleep mode, udelay for 0.5ms until the oscillator is back up)
> > > if
> > > the
> > > user wants a period of 1/200 Hz.
> > > 
> > > With this patch, we check if it is in fact set to the hardware
> > > default.
> > > If not, we set pca->period_ns to 0 which leads to changing the
> > > prescaler
> > > in the next call to pca9685_pwm_config.
> > 
> > And this contradicts, for my opinion, to the logic you referred in
> > the
> > first paragraph. If you would like to use preset values, you need to
> > read and recalculate period correctly.
> 
> I don't want to use preset values. I wanted to be sure that we do not
> skip the prescaler configuration if the prescale register was
> modified.
> We should only skip it, if it is already at 0x1E.

Same question here.

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy