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: <20170120063907.GA4894@ulmo.ba.sec>
Date:   Fri, 20 Jan 2017 07:39:07 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Clemens Gruber <clemens.gruber@...ruber.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        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 Thu, Jan 19, 2017 at 05:52:10PM +0100, Clemens Gruber wrote:
> On Thu, Jan 19, 2017 at 06:10:08PM +0200, Andy Shevchenko wrote:
> > Combining with your proposal I would see the best approach is to set
> > pca->period_ns accordingly to current prescaler value if you want to.
> 
> Yes, I agree.
> 
> > In your patch I see no benefit, since it's quite unlikely user will want
> > to have 5ms period among all possibilities.
> 
> It is the hardware default, but you are right, the user probably does
> not care about that.
> 
> > So, summarize, I prefer (in order of preference from high to low):
> > - enforce default prescaler value based on default period (it's just one
> > line to write a register)
> > - calculate default period based on actual prescaler value
> 
> Let's go with this one. I'll spin up a v2 where I calculate the
> period_ns value from the current prescaler value in the probe function.

This effectively ends up duplicating much of what the atomic API is
supposed to be doing.

So generally before the atomic API there is no way for the PWM driver
(and consequently the PWM users) to know what the current setting is.
That implies that we either can't care about the settings that were
programmed by some bootloader or that we force the PWM to a setting
on ->probe().

The result is inconsistent behaviour across drivers, and that's just
fine for many cases. But for some cases it is really not something that
we can work with.

So perhaps another possibility would be for this driver to implement the
atomic API. This should give you the necessary infrastructure to do
exactly what you want.

> Thierry: I think you could merge v1 of patch 1/2 from my series
> independently.

Okay, will do.

> I'll send v2 of patch 2/2 with aforementioned changes in the next
> days.

Like I said above, I think atomic API conversion wouldn't be very
difficult for this driver and it has the added advantage of giving you
the proper infrastructure to do this rather than having to duplicate
it in the driver.

That would be my preference, but I'm willing to take v2 of 2/2 as well
if it ends up being really nice and compact. =)

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)