linux-kernel - Re: [PATCH v4 07/10] regulator: mt6358: Add support for MT6358 regulator

Open Source and information security mailing list archives

Message-ID: <1565076527.23984.5.camel@mtksdaap41>
Date:   Tue, 6 Aug 2019 15:28:47 +0800
From:   Hsin-hsiung Wang <hsin-hsiung.wang@...iatek.com>
To:     Mark Brown <broonie@...nel.org>
CC:     Mark Rutland <mark.rutland@....com>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        <srv_heupstream@...iatek.com>, <devicetree@...r.kernel.org>,
        Sean Wang <sean.wang@...iatek.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        <linux-kernel@...r.kernel.org>,
        Richard Fontana <rfontana@...hat.com>,
        "Rob Herring" <robh+dt@...nel.org>,
        <linux-mediatek@...ts.infradead.org>,
        "Allison Randal" <allison@...utok.net>,
        <linux-rtc@...r.kernel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Eddie Huang <eddie.huang@...iatek.com>,
        Lee Jones <lee.jones@...aro.org>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4 07/10] regulator: mt6358: Add support for MT6358
 regulator

Hi Mark,

On Mon, 2019-08-05 at 14:10 +0100, Mark Brown wrote:
> On Mon, Aug 05, 2019 at 01:21:55PM +0800, Hsin-Hsiung Wang wrote:
> 
> > +static const u32 vmch_voltages[] = {
> > +	2900000, 3000000, 3300000,
> > +};
> 
> > +static const u32 vemc_voltages[] = {
> > +	2900000, 3000000, 3300000,
> > +};
> 
> Several of these tables appear to be identical.
> 
I will use the same voltage table in the next patch.

> > +static inline unsigned int mt6358_map_mode(unsigned int mode)
> > +{
> > +	return mode == MT6358_BUCK_MODE_AUTO ?
> > +		REGULATOR_MODE_NORMAL : REGULATOR_MODE_FAST;
> > +}
> 
> There is no need for this to be an inline and please write normal
> conditional statements to improve legibility.  There's other examples in
> the driver.
> 
will fix it in the next patch.

> > +static int mt6358_get_buck_voltage_sel(struct regulator_dev *rdev)
> > +{
> > +	int ret, regval;
> > +	struct mt6358_regulator_info *info = rdev_get_drvdata(rdev);
> > +
> > +	ret = regmap_read(rdev->regmap, info->da_vsel_reg, &regval);
> > +	if (ret != 0) {
> > +		dev_info(&rdev->dev,
> > +			 "Failed to get mt6358 Buck %s vsel reg: %d\n",
> > +			 info->desc.name, ret);
> 
> dev_err() for errors here and throughout the driver.
> 
will fix it in the next patch.

> > +		return ret;
> > +	}
> > +
> > +	ret = (regval >> info->da_vsel_shift) & info->da_vsel_mask;
> > +
> > +	return ret;
> > +}
> 
> This looks like a standard get_voltage_sel_regmap()?
> 
MT6358 has buck voltage status registers to show the actual output
voltage and the registers are different from the voltage setting
registers.
We want to get the actual voltage output, so we use the da_vsel status
registers here.

> > +err_mode:
> > +	if (ret != 0)
> > +		return ret;
> > +
> > +	return 0;
> 
> Or just return ret unconditionally?
will modify it to return ret unconditionally in the next patch.

Thanks a lot.
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

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