RE: [PATCH v3 06/10] mfd: da9063: Add custom regmap for DA9063L

[Steve Twiss]

Hi Marek and Geert,

On 04 June 2018 17:25 Marek Vasut wrote,

> Subject: Re: [PATCH v3 06/10] mfd: da9063: Add custom regmap for DA9063L
> 
> On 06/04/2018 09:39 AM, Geert Uytterhoeven wrote:
> > Hi Marek, Steve,
> >
> > On Sat, Jun 2, 2018 at 12:11 PM, Marek Vasut <marek.vasut@xxxxxxxxx> wrote:
> >> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
> >> block, the DA9063L does not have an RTC. Add custom regmap for DA9063L to
> >> prevent access into that register block.

Ok. I've said previously in [v3 07/10], but I'll copy again:
There is now an internal Dialog request to remove the RTC references from the DA9063L datasheet.
Adding that first part to the sentence in the commit log: "While the datasheet for DA9063L
(2v1, 23-Mar-2017) lists the RTC register block" -- it exists in error for the register map table
on page 91, but the datasheet also identifies those registers in Table 102 on page 126 as
"Reserved".

Pointing out the ambiguity in this version of the datasheet seems redundant in the commit log.
Also Dialog do not store a history of Datasheets on their website so once this is updated (although
this update is not in my hands) the datasheet will be replaced. So, it seems this comment could
make the commit message just as misleading as the current datasheet.

How about something simpler?
"The DA9063L does not have an RTC. Add custom regmap for DA9063L to prevent access
into that register block."

> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
> >
> > Thanks for your patch!
> >
> >> --- a/drivers/mfd/da9063-i2c.c
> >> +++ b/drivers/mfd/da9063-i2c.c
> >> @@ -254,6 +341,10 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> >
> > Note that the line above doesn't check da9063->type, but da9063-
> >variant_code...
> >
> >>                 da9063_regmap_config.rd_table = &da9063_ad_readable_table;
> >>                 da9063_regmap_config.wr_table = &da9063_ad_writeable_table;
> >>                 da9063_regmap_config.volatile_table = &da9063_ad_volatile_table;
> >> +       } else if (da9063->type == PMIC_TYPE_DA9063L) {
> >
> > ... so this may be slightly confusing.
> 
> I know.
> 
> >> +               da9063_regmap_config.rd_table = &da9063l_bb_readable_table;
> >> +               da9063_regmap_config.wr_table = &da9063l_bb_writeable_table;
> >> +               da9063_regmap_config.volatile_table = &da9063l_bb_volatile_table;
> >>         } else {
> >>                 da9063_regmap_config.rd_table = &da9063_bb_readable_table;
> >>                 da9063_regmap_config.wr_table = &da9063_bb_writeable_table;
> >
> > However, da9063->variant_code doesn't seem to have been filled in at this
> > point yet (the call to da9063_device_init() doing so is below, at the end
> > of the probe function!), so commit 9cb42e2a8ed06e91 ("mfd: da9063: Add
> > support for AD silicon variant") never actually handled the AD silicon variant
> > correctly? Or am I missing something?

Okay ... No. You're not missing anything. I had noticed that.
The AD chip model is not referenced and by default only the BB chip model is used.

> Ha, that is a good point.

Yeah, it's a good point, but it's not an amusing point.
The device tree only distinguishes a "dlg,da9063", there is no AD type in the DT schema.
There is no datasheet listing AD registers supported by Dialog, only BB.

But, AD registers were added back into the header file in commit 9cb42e2a8ed06e91
and the RTC driver was updated to distinguish between the AD and BB according to
the type of variant detected at run-time during the da9063_device_init() call.

The real problem is that this leads to two competing chip detection methods for the
DA9063. The function da9063_device_init() autodetects the chip variant, but
autodetection cannot define the chip model. It's circular: the chip model cannot be
autodetected because a chip model is needed to access the register used during
autodetection.

Which leads me back to what I said two paragraphs up:
> The device tree only distinguishes a "dlg,da9063", there is no AD type in the DT schema.
> There is no datasheet listing AD registers supported by Dialog, only BB.

This is not how it is done in the DA9062 and DA9061 driver: the variant code is only
used to print the information to the console during start-up and it is the DT that defines
the chip model based upon "dlg,da9062" or "dlg,da9061".

Steve