Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Vaittinen, Matti]

On Thu, 2020-12-10 at 09:19 +0100, Geert Uytterhoeven wrote:
> Hi Matti, Shimoda-san,
> 
> On Thu, Dec 10, 2020 at 8:33 AM Vaittinen, Matti
> <Matti.Vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> > On Thu, 2020-12-10 at 04:44 +0000, Yoshihiro Shimoda wrote:
> > > > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020
> > > > 10:30
> > > > PM
> > > <snip>
> > > > > --- a/drivers/mfd/bd9571mwv.c
> > > > > +++ b/drivers/mfd/bd9571mwv.c
> > > > > 
> > > > > @@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct
> > > > > i2c_client
> > > > > *client,
> > > > >         product_code = (unsigned int)ret;
> > > > >         if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
> > > > >                 bd->data = &bd9571mwv_data;
> > > > > +       else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
> > > > > +               bd->data = &bd9574mwf_data;
> > > > > 
> > > > >         if (!bd->data) {
> > > > >                 dev_err(bd->dev, "No found supported device
> > > > > %d\n",
> > > > 
> > > > While BD9571MWV and BD9574MWF can be distinguished at runtime,
> > > > I think it would still be a good idea to document a
> > > > "rohm,bd9574mwf"
> > > > compatible value in the DT bindings, and let the driver match
> > > > on
> > > > that.
> > > 
> > > In this driver point of view, we can use such the DT bindings,
> > > however, in the board point of view, it's difficult to describe
> > > which chip is installed on r8a77990-ebisu.dts. So, I'd like to
> > > keep this runtime detection.
> 
> To clarify: I meant to document and add the compatible value, but
> treat both compatible values the same in the driver, and still do
> runtime
> probing.
> 
> > First of all - I don't want to insist changes here so my comment
> > can be
> > ignored. I would definitely like seeing the support for BD9574 in-
> > tree!
> > 
> > But as a 'nit':
> > I don't know what are the difficulties you are referring to so it's
> > hard for me to comment this. Without better understanding of board
> > dts
> > files - I think BD9574MWF should really have own compatible as I
> > understood it is different from the BD9571MWV. Relying on product
> > code
> > probing sounds like something that may easily break when/if new
> > variants are produced. ( I've seen new HW variants using the same
> > ID information being produced in previous companies I've worked.
> > Sure
> 
> Yes, this happens from time to time, unfortunately.
> 
> > ROHM wouldn't do this but still... :] ). And producing boards where
> > DTS
> > does not allow describing the correct components sounds like asking
> > for
> > a nose-bleed to me... If probing of IC type fails AND there is
> > devices
> > with wrong PMIC information burned in DT - then fixing it can be a
> > nightmare. So I would really try make DTS files such that they can
> > be
> 
> The issue we're facing is that older Ebisu-4D boards have BD9571,
> while
> newer boards have BD9574.  The schematics say "BD9574MWF-M (tentative
> ver:BD9571TL1_E3)", so it looks like both parts are pin-compatible
> (ignoring missing pins for AVS, LDO1, LDO2, and LDO6 on BD9574).
> Hence arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts has a device
> node
> compatible with "rohm,bd9571mwv".  If we have runtime probing, we can
> keep on using that for both variants.

Thank you for the explanation :) This is a nice learning experience for
me!

> > changed to match the actual board. (Perhaps introduce the
> > compatible
> > for BD9574MWF - make this driver to match both of the PMICs - leave
> > the
> > runtime probing here for now - and in parallel work with the DTS
> > files
> > so that eventually the probing can be removed(?) That was my 10
> > cents
> > on this topic :] )
> 
> Exactly. Thanks!

I am more than happy with this solution :)

--Matti