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

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]

Date:   Thu, 10 Dec 2020 07:33:26 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     "yoshihiro.shimoda.uh@...esas.com" <yoshihiro.shimoda.uh@...esas.com>,
        "geert@...ux-m68k.org" <geert@...ux-m68k.org>
CC:     "marek.vasut+renesas@...il.com" <marek.vasut+renesas@...il.com>,
        "khiem.nguyen.xt@...esas.com" <khiem.nguyen.xt@...esas.com>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

On Thu, 2020-12-10 at 04:44 +0000, Yoshihiro Shimoda wrote:
> Hi Geert-san,
> 
> Thank you for your review!
> 
> > 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.

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
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
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 :] )

Best Regards
    Matti Vaittinen