Message-ID: <20200618163814.GI954398@dell>
Date:   Thu, 18 Jun 2020 17:38:14 +0100
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: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly
 select reg tables

On Thu, 18 Jun 2020, Adam Thomson wrote:

> On 18 June 2020 12:15, Lee Jones 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 adds raw I2C read access functionality to 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.
> > > > >
> > > > > Signed-off-by: Adam Thomson
> > <Adam.Thomson.Opensource@...semi.com>
> > > > > ---
> > > > >  drivers/mfd/da9063-core.c            |  31 ------
> > > > >  drivers/mfd/da9063-i2c.c             | 184
> > +++++++++++++++++++++++++++++++-
> > > > ---
> > > > >  include/linux/mfd/da9063/registers.h |  15 ++-
> > > > >  3 files changed, 177 insertions(+), 53 deletions(-)

[...]

> > > > Rather than open coding this, does it make sense to register a small
> > > > (temporary?) Device ID Regmap to read from?
> > >
> > > The original patch submission did exactly that but you indicated you weren't
> > > keen due to overheads, hence the implementation above. Actually what we
> > have
> > > here is a bit smaller than the regmap approach and I really I'd rather not
> > > have to respin again just to revert to something that was dismissed in the first
> > > place over 6 months ago.
> > 
> > Actually the conversation went like:
> > 
> > Lee:
> >   IIUC, you have a dependency issue whereby the device type is required
> >   before you can select the correct Regmap configuration.  Is that
> >   correct?
> > 
> >   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?
> > 
> > Adam:
> >   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.
> > 
> > Lee:
> >   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.
> > 
> >   Ah, they are I2C transactions?  Not the nice readl()s I had in mind.
> > 
> > Adam:
> >   I can knock something together though just to see what it looks like.
> > 
> > Lee:
> >   Well, I'd appreciated that, thanks.
> > 
> > So now we can see them side-by-side we can take them on their own
> > merits.  When I initially requested raw reads, I had readl()s in mind,
> > rather than the extensive code required to read the required registers
> > via I2C.
> 
> To be fair those changes were in V2 of the patch set, which is why I was a quite
> surprised by your suggestion today as you hadn't made this comment against that
> version, given the previous discussion.

It would be very difficult to remember complete revision history for
every patch received.  Especially with the lack of a provided
patch-level changelog.  So I have to take each submission on its own
merits.  This is particularly true for patch-sets ranging over 6
months or more.

The Regmap vs raw accesses decision could easily have gone either way,
so it's not surprising the other was considered during the review of
each submission.  Both times were phrased as an "I wonder if" enquiry,
rather than a demand.

> > However, it looks like there is very little difference between them,
> > thus I do not see a benefit to reverting it back.  The current version
> > seems fine.
> > 
> > I'll conduct a full review shortly, when I have a little more spare
> > time (looking at my current TODO list, this will probably be Monday
> > now).  Although everything does look fine at first glance.
> 
> Thanks. That would be appreciated.

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

Powered by blists - more mailing lists