| 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
| ||
|
Message-ID: <TY2PR01MB369297B5977B36C9286C06CAD8C70@TY2PR01MB3692.jpnprd01.prod.outlook.com>
Date: Mon, 14 Dec 2020 08:22:26 +0000
From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To: "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>,
"lgirdwood@...il.com" <lgirdwood@...il.com>,
"marek.vasut+renesas@...il.com" <marek.vasut+renesas@...il.com>,
"broonie@...nel.org" <broonie@...nel.org>,
"bgolaszewski@...libre.com" <bgolaszewski@...libre.com>,
"lee.jones@...aro.org" <lee.jones@...aro.org>,
"linus.walleij@...aro.org" <linus.walleij@...aro.org>
CC: linux-power <linux-power@...rohmeurope.com>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
Khiem Nguyen <khiem.nguyen.xt@...esas.com>,
"linux-renesas-soc@...r.kernel.org"
<linux-renesas-soc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 04/10] regulator: bd9571mwv: Add BD9574MWF support
Hello Matti-san,
> From: Vaittinen, Matti, Sent: Monday, December 14, 2020 4:13 PM
>
> Hello Shimoda-san,
>
> On Mon, 2020-12-14 at 04:57 +0000, Yoshihiro Shimoda wrote:
> > Hello Matti-san,
> >
> > > From: Vaittinen, Matti, Sent: Friday, December 11, 2020 9:34 PM
> > >
> > > Hello again Shimada-san,
> > >
> > > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > > > Add support for BD9574MWF which is silimar chip with BD9571MWV.
> > > > Note that BD9574MWF doesn't support AVS and VID.
> > >
> > > I'd like to understand what is VID?
> >
> > It seems reading some voltages from registers.
> > For example, BD9571 has "VD18_VID" register which
> > is prohibit to write. But, BD9574 doesn't have this
> > register. Also, the driver names "vid_ops",
> > so I described "VID" here. Perhaps, we should revise
> > the description to clear. (Please look "Updated description" in this
> > email.)
>
> Thank you for detailed explanation. So as far as I understood - VID is
> a register which displays the current output voltage, right?
Yes.
> If I am
> not mistaken, this is different from register where (initial) voltage
> is set?
Yes. I checked on my environment (H3 Salvator-XS).
> > > > Signed-off-by: Yoshihiro Shimoda <
> > > > yoshihiro.shimoda.uh@...esas.com>
> > > > ---
> > > > drivers/regulator/bd9571mwv-regulator.c | 10 ++++++++--
> > > > 1 file changed, 8 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/regulator/bd9571mwv-regulator.c
> > > > b/drivers/regulator/bd9571mwv-regulator.c
> > > > index 02120b0..041339b 100644
> > > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > > @@ -1,6 +1,6 @@
> > > > // SPDX-License-Identifier: GPL-2.0
> > > > /*
> > > > - * ROHM BD9571MWV-M regulator driver
> > > > + * ROHM BD9571MWV-M and BD9574MWF-M regulator driver
> > > > *
> > > > * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@...il.com
> > > > >
> > > > *
> > > > @@ -9,6 +9,7 @@
> > > > * NOTE: VD09 is missing
> > > > */
> > > >
> > > > +#include <linux/mfd/rohm-generic.h>
> > > > #include <linux/module.h>
> > > > #include <linux/of.h>
> > > > #include <linux/platform_device.h>
> > > > @@ -277,6 +278,7 @@ static int bd9571mwv_regulator_probe(struct
> > > > platform_device *pdev)
> > > > struct regulator_dev *rdev;
> > > > unsigned int val;
> > > > int i;
> > > > + enum rohm_chip_type chip = platform_get_device_id(pdev)-
> > > > > driver_data;
> > > >
> > > > bdreg = devm_kzalloc(&pdev->dev, sizeof(*bdreg), GFP_KERNEL);
> > > > if (!bdreg)
> > > > @@ -292,6 +294,9 @@ static int bd9571mwv_regulator_probe(struct
> > > > platform_device *pdev)
> > > > config.regmap = bdreg->regmap;
> > > >
> > > > for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> > > > + /* BD9574MWF supports DVFS only */
> > > > + if (chip == ROHM_CHIP_TYPE_BD9574 && regulators[i].id
> > > > != DVFS)
> > > > + continue;
> > >
> > > Does this mean that reading VD09 voltage is not supported by
> > > driver?
> >
> > Yes. Also, reading VD{18,25,33} voltage are not supported.
>
> I think that would be excellent comment here. Maybe something like: "We
> don't support voltage rails VD{09,18,25,33} by this driver on BD9574."
Thank you for the suggestion! I'll use this comment.
> > > (I
> > > assumed VD09 initial voltage can be set from eeprom(?) and read by
> > > driver - I may be wrong though). Perhaps it is worth mentioning in
> > > the
> > > commit message as a driver restriction?
> >
> > Yes, these voltage can be set from eeprom and read by driver.
> > So, I updated the description like below.
> >
> > -- Updated description --
> > Add support for BD9574MWF which is similar chip with BD9571MWV.
> > Note that since BD9574MWF doesn't have avs_ops and vid_ops
> > related registers, this driver avoids to use these registers
> > if BD9574MWF is used.
> > ------------------------
>
> Thank you :) What I was after is that I would like to leave a note
> about 'what could be improved' or about what is the 'software
> limitation' here so that if anyone later needs the other voltage rails
> he would have a hint about what could be done.
>
> Do you think mentioning that "the VD09 voltage could be read from PMIC
> but that is not supported by this commit" in commit message would be
> Ok?
I think OK because VD09 could be read from "BD9574MWF_VD09_VINIT"
register, but that is not supported but this commit.
> > > And just asking out of the curiosity - are the other voltage rails
> > > listed in data-sheet (VD18, DDR0, VD33, VD09 and LDO1,...,LDO4)
> > > set-up
> > > from DT as fixed-regulators? Any reason why they are not set-up
> > > here?
> >
> > TBH, I don't know why. Perhaps, the driver cannot read DDR0, LDO[1-4]
> > values?
>
> I also think that all voltages can't be read. I was just thinking that
> it might make sense to always create the fixed regulators from PMIC
> driver - because if PMIC is used - then these voltage rails do exist.
> (This was just a question so that I could learn - not so much of a
> review comment.)
>
> If you re-spin the series for other fixups - then I would appreciate
> some comment about omitting the rest of the voltage outputs.
>
> Other than that - for what it is worth:
>
> Reviewed-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Thank you very much for your review!
Best regards,
Yoshihiro Shimoda
Powered by blists - more mailing lists