Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Lee Jones <lee.jones@linaro.org>]

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@intel.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.

> > > +		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.

> > > +		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.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog