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

[Steve Twiss]

Hi Marek and Geert,

On 06 June 2018 00:02 Marek Vasut wrote,

> Subject: Re: [PATCH v3 06/10] mfd: da9063: Add custom regmap for DA9063L
> 
> On 06/05/2018 10:17 PM, Steve Twiss wrote:
> > 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".
> 
> So the AD was broken since forever and noone noticed ? :)

Not quite. 
The AD support is working, but the driver doesn't work like everybody
expects because it uses the BB chip model. But it does work because the chip
model for BB is valid for AD; in this case BB represents a superset of AD
registers (and any mismatches are never accessed or mean anything in AD).

> Do you have an AD hardware and can you fix it ?

Part of my work is to support the community and I think this is fixable.

But all of this shouldn't affect your DA9063L submission should it?

Regards,
Steve