linux-kernel - RE: [RESEND PATCH 1/2] mfd: da9063: Fix revision handling to correctly select reg tables

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] [day] [month] [year] [list]

Message-ID: <AM6PR10MB2263116205FAAE470E1F23C380ED0@AM6PR10MB2263.EURPRD10.PROD.OUTLOOK.COM>
Date:   Tue, 25 Feb 2020 10:08:03 +0000
From:   Adam Thomson <Adam.Thomson.Opensource@...semi.com>
To:     Lee Jones <lee.jones@...aro.org>,
        Adam Thomson <Adam.Thomson.Opensource@...semi.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Support Opensource <Support.Opensource@...semi.com>
Subject: RE: [RESEND PATCH 1/2] mfd: da9063: Fix revision handling to
 correctly select reg tables

On 25 February 2020 09:00, Lee Jones wrote:

> On Mon, 24 Feb 2020, Adam Thomson wrote:
> 
> > On 24 February 2020 11:27, Lee Jones wrote:
> >
> > > On Mon, 24 Feb 2020, Adam Thomson wrote:
> > >
> > > > On 24 February 2020 09:57, Lee Jones wrote:
> > > >
> > > > > On Fri, 24 Jan 2020, Adam Thomson wrote:
> > > > >
> > > > > > The current implementation performs checking in the i2c_probe()
> > > > > > function of the variant_code but does this immediately after the
> > > > > > containing struct has been initialised as all zero. This means the
> > > > > > check for variant code will always default to using the BB tables
> > > > > > and will never select AD. The variant code is subsequently set
> > > > > > by device_init() and later used by the RTC so really it's a little
> > > > > > fortunate this mismatch works.
> > > > > >
> > > > > > This update creates an initial temporary regmap instantiation to
> > > > > > simply read the chip and variant/revision information (common to
> > > > > > all revisions) so that it can subsequently correctly choose the
> > > > > > proper regmap tables for real initialisation.
> > > > >
> > > > > IIUC, you have a dependency issue whereby the device type is required
> > > > > before you can select the correct Regmap configuration.  Is that
> > > > > correct?
> > > >
> > > > Yep, spot on.
> > > >
> > > > > If so, using Regmap for the initial register reads sounds like
> > > > > over-kill.  What's stopping you simply using raw reads before the
> > > > > Regmap is instantiated?
> > > >
> > > > Actually nothing and I did consider this at the start. Nice thing with regmap
> > > > is it's all tidily contained and provides the page swapping mechanism to
> access
> > > > higher page registers like the variant information. Given this is only once at
> > > > probe time it felt like this was a reasonable solution. However if you're not
> > > > keen I can update to use raw access instead.
> > >
> > > It would be nice to compare the 2 solutions side by side.  I can't see
> > > the raw reads of a few device-ID registers being anywhere near 170
> > > lines though.
> >
> > To be fair the regmap specific additions for the temporary register access, are
> > maybe 50 - 60 lines. The rest is to do with handling the result which you'll
> > need anyway to select the correct register map. I reckon to provide raw read
> and
> > write access we're going to probably be similar or more as we'll need to write
> > the page register then read from the relevant ID registers. Using this an
> > example for the lines count:
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/mfd/tps6507x.c#L37
> 
> Ah, they are I2C transactions?  Not the nice readl()s I had in mind.

Yes, I2C register access so needs more work.

> 
> > I can knock something together though just to see what it looks like.
> 
> Well, I'd appreciated that, thanks.

No problem, I'll try and get something back soon.