Message-ID: <YFhhGpiHDELxIo9V@orome.fritz.box>
Date:   Mon, 22 Mar 2021 10:19:22 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Clemens Gruber <clemens.gruber@...ruber.com>
Cc:     Sven Van Asbroeck <thesven73@...il.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-pwm@...r.kernel.org
Subject: Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> Hi Sven,
> 
> On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote:
> > Hi Clemens,
> > 
> > On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
> > <clemens.gruber@...ruber.com> wrote:
> > >
> > > Ok, so you suggest we extend our get_state logic to deal with cases
> > > like the following:
> > 
> > Kind of. We can't control how other actors (bootloaders etc) program the
> > chip. As far as I know, there are many, many different register settings that
> > result in the same physical chip outputs. So if .probe() wants to preserve the
> > existing chip settings, .get_state() has to be able to deal with every possible
> > setting. Even invalid ones.
> 
> Is the driver really responsible for bootloaders that program the chip
> with invalid values?
> The chip comes out of PoR with sane default values. If the bootloader of
> a user messes them up, isn't that a bootloader problem instead of a
> Linux kernel driver problem?

It is ultimately a problem of the bootloader and where possible the
bootloader should be fixed. However, fixing bootloaders sometimes isn't
possible, or impractical, so the kernel has to be able to deal with
hardware that's been badly programmed by the bootloader. Within reason,
of course. Sometimes this can't be done in any other way than forcing a
hard reset of the chip, but it should always be a last resort.

> > In addition, .apply() cannot make any assumptions as to which bits are
> > already set/cleared on the chip. Including preserved, invalid settings.
> > 
> > This might get quite complex.
> > 
> > However if we reset the chip in .probe() to a known state (a normalized state,
> > in the mathematical sense), then both .get_state() and .apply() become
> > much simpler. because they only need to deal with known, normalized states.
> 
> Yes, I agree. This would however make it impossible to do a flicker-free
> transition from bootloader to kernel, but that's not really a usecase I
> have so I can live without it.
> 
> Another point in favor of resetting is that the driver already does it.
> Removing the reset of the OFF register may break some boards who rely on
> that behaviour.
> My version only extended the reset to include the ON register.
> 
> > 
> > In short, it's a tradeoff between code complexity, and user friendliness/
> > features.
> > 
> > Sven
> 
> Thierry, Uwe, what's your take on this?
> 
> Thierry: Would you accept it if we continue to reset the registers in
> .probe?

Yes, I think it's fine to continue to reset the registers since that's
basically what the driver already does. It'd be great if you could
follow up with a patch that removes the reset and leaves the hardware in
whatever state the bootloader has set up. Then we can take that patch
for a ride and see if there are any complains about it breaking. If
there are we can always try to fix them, but as a last resort we can
also revert, which then may be something we have to live with. But I
think we should at least try to make this consistent with how other
drivers do this so that people don't stumble over this particular
driver's behaviour.

Thierry

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

Powered by blists - more mailing lists