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

Message-ID: <20200225090020.GX3494@dell>
Date:   Tue, 25 Feb 2020 09:00:20 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     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 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.

> I can knock something together though just to see what it looks like.

Well, I'd appreciated that, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog