linux-kernel - RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

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] [day] [month] [year] [list]

Message-ID: <6F87890CF0F5204F892DEA1EF0D77A59725DFDBA@FMSMSX114.amr.corp.intel.com>
Date:   Fri, 28 Jul 2017 23:49:05 +0000
From:   "Mani, Rajmohan" <rajmohan.mani@...el.com>
To:     Lee Jones <lee.jones@...aro.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Alexandre Courbot <gnurou@...il.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        "sakari.ailus@...ux.intel.com" <sakari.ailus@...ux.intel.com>,
        "Andy Shevchenko" <andy.shevchenko@...il.com>
Subject: RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

Hi Lee,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Tue, 25 Jul 2017, Mani, Rajmohan wrote:
> 
> > Hi Lee,
> >
> > > Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> > >
> > > On Fri, 21 Jul 2017, Mani, Rajmohan wrote:
> > > > > On Wed, 19 Jul 2017, Rajmohan Mani wrote:
> > > > >
> > > > > > The TPS68470 device is an advanced power management unit that
> > > > > > powers
> > > > > a
> > > > > > Compact Camera Module (CCM), generates clocks for image
> > > > > > sensors, drives a dual LED for Flash and incorporates two LED
> > > > > > drivers for general purpose indicators.
> > > > > >
> > > > > > This patch adds support for TPS68470 mfd device.
> > > > > >
> > > > > > Signed-off-by: Rajmohan Mani <rajmohan.mani@...el.com>
> > > > > > ---
> > > > > >  drivers/mfd/Kconfig          |  18 +++++++
> > > > > >  drivers/mfd/Makefile         |   1 +
> > > > > >  drivers/mfd/tps68470.c       | 110
> > > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/linux/mfd/tps68470.h |  97
> > > > > > ++++++++++++++++++++++++++++++++++++++
> > > > > >  4 files changed, 226 insertions(+)  create mode 100644
> > > > > > drivers/mfd/tps68470.c  create mode 100644
> > > > > > include/linux/mfd/tps68470.h
> > >
> > > [...]
> > >
> > > > > > +static const struct regmap_config tps68470_regmap_config = {
> > > > > > +	.reg_bits = 8,
> > > > > > +	.val_bits = 8,
> > > > > > +	.max_register = TPS68470_REG_MAX, };
> > > > > > +
> > > > > > +static int tps68470_chip_init(struct device *dev, struct
> > > > > > +regmap
> > > > > > +*regmap) {
> > > > > > +	unsigned int version;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	/* Force software reset */
> > > > > > +	ret = regmap_write(regmap, TPS68470_REG_RESET,
> > > > > TPS68470_REG_RESET_MASK);
> > > > > > +	if (ret < 0)
> > > > >
> > > > > Will 'if (!ret)' do?
> > > > >
> > > >
> > > > We intend to check error conditions and bail out. So, if (ret < 0)
> > > > works for
> > > this.
> > >
> > > Yes, 'if (!ret)' does that too.
> > >
> >
> > regmap_write() and regmap_read() functions return 0 on success. Hence
> we can not use 'if (!ret)' here, since we check for error conditions.
> 
> Sorry, this was a typo.
> 
> It should be 'if (ret)'.
> 
> What I'm trying to get at is the " < 0" is not required.
> 

Ack

> > > > > > +		return ret;
> > > > > > +
> > > > > > +	ret = regmap_read(regmap, TPS68470_REG_REVID,
> &version);
> > > > > > +	if (ret < 0) {
> > > > >
> > > > > Same
> > > > >
> > > >
> > > > We intend to check error conditions and bail out. So, if (ret < 0)
> > > > works for
> > > this.
> > >
> > > As above.
> > >
> >
> > Same as above
> >
> > > > > > +		dev_err(dev, "Failed to read revision register: %d\n",
> ret);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int tps68470_probe(struct i2c_client *client) {
> > > > > > +	struct device *dev = &client->dev;
> > > > > > +	struct regmap *regmap;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	regmap = devm_regmap_init_i2c(client,
> &tps68470_regmap_config);
> > > > > > +	if (IS_ERR(regmap)) {
> > > > > > +		dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> > > > > > +			PTR_ERR(regmap));
> > > > > > +		return PTR_ERR(regmap);
> > > > > > +	}
> > > > > > +
> > > > > > +	i2c_set_clientdata(client, regmap);
> > > > > > +
> > > > > > +	ret = tps68470_chip_init(dev, regmap);
> > > > > > +	if (ret < 0) {
> > > > >
> > > > > Same
> > > > >
> > > >
> > > > We intend to check error conditions and bail out. So, if (ret < 0)
> > > > works for
> > > this.
> > >
> > > As above.
> > >
> >
> > Same as above
> 
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog