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

[Mani, Rajmohan]

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.m...@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.
> > >
> >
> > 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,
> );
> > > > > > +   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 = >dev;
> > > > > > +   struct regmap *regmap;
> > > > > > +   int ret;
> > > > > > +
> > > > > > +   regmap = devm_regmap_init_i2c(client,
> _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

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

[Mani, Rajmohan]

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 
> > > > > > ---
> > > > > >  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,
> );
> > > > > > +   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 = >dev;
> > > > > > +   struct regmap *regmap;
> > > > > > +   int ret;
> > > > > > +
> > > > > > +   regmap = devm_regmap_init_i2c(client,
> _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

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

[Mani, Rajmohan]

Hi Andy,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, Jul 28, 2017 at 3:30 AM, Mani, Rajmohan
> <rajmohan.m...@intel.com> wrote:
> >> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones <lee.jo...@linaro.org>
> wrote:
> >> > On Tue, 25 Jul 2017, Andy Shevchenko wrote:
> >> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones <lee.jo...@linaro.org>
> wrote:
> >>
> >> >> I briefly checked few ->read() and ->write() implementations and
> >> >> didn't find any evidence of positive numbers that can be returned.
> >> >> Documentation (kernel doc) doesn't shed a light on that. So, to me
> >> >> it sounds unspecified.
> >> >>
> >> >> So, for now (until documentation will be fixed) I would rely on if
> >> >> (ret < 0)
> >> >
> >> > It's not unspecified.  The regmap methods call into
> >> > regcache_write(), where the kerneldoc is clear.
> >>
> >
> > Since, we are interested in the regmap for the I2C bus here, I looked
> > into the implementation of
> >  __devm_regmap_init()
> > __regmap_init()
> > regcache_init()
> > for I2C bus.
> >
> > At the end of __devm_regmap_init() call from devm_regmap_init_i2c()
> inside tps68470_probe(), I see that both cache_bypass and defer_caching
> flags of i2c regmap struct are set. So, it looks regcache_write/read calls do
> not come into play here.
> >
> > So, regmap_write()
> > _regmap_write()
> > map->reg_write (drivers/base/regmap/regmap.c:1665) 
> > translates
> to
> > regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128)
> >
> > These checks in regmap_i2c_write() ensure all return values from
> i2c_master_send() other than the requested number of bytes to write, are
> converted into negative values.
> >
> > if (ret == count)
> > return 0;
> > else if (ret < 0)
> > return ret;
> > else
> > return -EIO;
> >
> > Similar argument goes for regmap_read() as well.
> > With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to 
> > be a
> better choice. Please see if I missed anything here.
> 
> It prooves exactly the Lee's point.
> 
> So, perhaps the best approach is to move to if (ret)  return ret;
> 
> ...if it will be a problem in the future, fix it accordingly.
> 

Ack.
We have spent enough time on this already.

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

[Mani, Rajmohan]

Hi Andy,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, Jul 28, 2017 at 3:30 AM, Mani, Rajmohan
>  wrote:
> >> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones 
> wrote:
> >> > On Tue, 25 Jul 2017, Andy Shevchenko wrote:
> >> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones 
> wrote:
> >>
> >> >> I briefly checked few ->read() and ->write() implementations and
> >> >> didn't find any evidence of positive numbers that can be returned.
> >> >> Documentation (kernel doc) doesn't shed a light on that. So, to me
> >> >> it sounds unspecified.
> >> >>
> >> >> So, for now (until documentation will be fixed) I would rely on if
> >> >> (ret < 0)
> >> >
> >> > It's not unspecified.  The regmap methods call into
> >> > regcache_write(), where the kerneldoc is clear.
> >>
> >
> > Since, we are interested in the regmap for the I2C bus here, I looked
> > into the implementation of
> >  __devm_regmap_init()
> > __regmap_init()
> > regcache_init()
> > for I2C bus.
> >
> > At the end of __devm_regmap_init() call from devm_regmap_init_i2c()
> inside tps68470_probe(), I see that both cache_bypass and defer_caching
> flags of i2c regmap struct are set. So, it looks regcache_write/read calls do
> not come into play here.
> >
> > So, regmap_write()
> > _regmap_write()
> > map->reg_write (drivers/base/regmap/regmap.c:1665) 
> > translates
> to
> > regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128)
> >
> > These checks in regmap_i2c_write() ensure all return values from
> i2c_master_send() other than the requested number of bytes to write, are
> converted into negative values.
> >
> > if (ret == count)
> > return 0;
> > else if (ret < 0)
> > return ret;
> > else
> > return -EIO;
> >
> > Similar argument goes for regmap_read() as well.
> > With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to 
> > be a
> better choice. Please see if I missed anything here.
> 
> It prooves exactly the Lee's point.
> 
> So, perhaps the best approach is to move to if (ret)  return ret;
> 
> ...if it will be a problem in the future, fix it accordingly.
> 

Ack.
We have spent enough time on this already.

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

[Andy Shevchenko]

On Fri, Jul 28, 2017 at 3:30 AM, Mani, Rajmohan  wrote:
>> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones  wrote:
>> > On Tue, 25 Jul 2017, Andy Shevchenko wrote:
>> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones  wrote:
>>
>> >> I briefly checked few ->read() and ->write() implementations and
>> >> didn't find any evidence of positive numbers that can be returned.
>> >> Documentation (kernel doc) doesn't shed a light on that. So, to me it
>> >> sounds unspecified.
>> >>
>> >> So, for now (until documentation will be fixed) I would rely on if
>> >> (ret < 0)
>> >
>> > It's not unspecified.  The regmap methods call into regcache_write(),
>> > where the kerneldoc is clear.
>>
>
> Since, we are interested in the regmap for the I2C bus here, I looked into 
> the implementation of
>  __devm_regmap_init()
> __regmap_init()
> regcache_init()
> for I2C bus.
>
> At the end of __devm_regmap_init() call from devm_regmap_init_i2c() inside 
> tps68470_probe(), I see that both cache_bypass and defer_caching flags of i2c 
> regmap struct are set. So, it looks regcache_write/read calls do not come 
> into play here.
>
> So, regmap_write()
> _regmap_write()
> map->reg_write (drivers/base/regmap/regmap.c:1665) translates 
> to
> regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128)
>
> These checks in regmap_i2c_write() ensure all return values from 
> i2c_master_send() other than the requested number of bytes to write, are 
> converted into negative values.
>
> if (ret == count)
> return 0;
> else if (ret < 0)
> return ret;
> else
> return -EIO;
>
> Similar argument goes for regmap_read() as well.
> With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to be 
> a better choice. Please see if I missed anything here.

It prooves exactly the Lee's point.

So, perhaps the best approach is to move to
if (ret)
 return ret;

...if it will be a problem in the future, fix it accordingly.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jul 28, 2017 at 3:30 AM, Mani, Rajmohan  wrote:
>> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones  wrote:
>> > On Tue, 25 Jul 2017, Andy Shevchenko wrote:
>> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones  wrote:
>>
>> >> I briefly checked few ->read() and ->write() implementations and
>> >> didn't find any evidence of positive numbers that can be returned.
>> >> Documentation (kernel doc) doesn't shed a light on that. So, to me it
>> >> sounds unspecified.
>> >>
>> >> So, for now (until documentation will be fixed) I would rely on if
>> >> (ret < 0)
>> >
>> > It's not unspecified.  The regmap methods call into regcache_write(),
>> > where the kerneldoc is clear.
>>
>
> Since, we are interested in the regmap for the I2C bus here, I looked into 
> the implementation of
>  __devm_regmap_init()
> __regmap_init()
> regcache_init()
> for I2C bus.
>
> At the end of __devm_regmap_init() call from devm_regmap_init_i2c() inside 
> tps68470_probe(), I see that both cache_bypass and defer_caching flags of i2c 
> regmap struct are set. So, it looks regcache_write/read calls do not come 
> into play here.
>
> So, regmap_write()
> _regmap_write()
> map->reg_write (drivers/base/regmap/regmap.c:1665) translates 
> to
> regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128)
>
> These checks in regmap_i2c_write() ensure all return values from 
> i2c_master_send() other than the requested number of bytes to write, are 
> converted into negative values.
>
> if (ret == count)
> return 0;
> else if (ret < 0)
> return ret;
> else
> return -EIO;
>
> Similar argument goes for regmap_read() as well.
> With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to be 
> a better choice. Please see if I missed anything here.

It prooves exactly the Lee's point.

So, perhaps the best approach is to move to
if (ret)
 return ret;

...if it will be a problem in the future, fix it accordingly.

-- 
With Best Regards,
Andy Shevchenko

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

[Mani, Rajmohan]

Hi Lee, Andy,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones <lee.jo...@linaro.org> wrote:
> > On Tue, 25 Jul 2017, Andy Shevchenko wrote:
> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones <lee.jo...@linaro.org> wrote:
> 
> >> I briefly checked few ->read() and ->write() implementations and
> >> didn't find any evidence of positive numbers that can be returned.
> >> Documentation (kernel doc) doesn't shed a light on that. So, to me it
> >> sounds unspecified.
> >>
> >> So, for now (until documentation will be fixed) I would rely on if
> >> (ret < 0)
> >
> > It's not unspecified.  The regmap methods call into regcache_write(),
> > where the kerneldoc is clear.
> 

Since, we are interested in the regmap for the I2C bus here, I looked into the 
implementation of
 __devm_regmap_init()
__regmap_init()
regcache_init()
for I2C bus.

At the end of __devm_regmap_init() call from devm_regmap_init_i2c() inside 
tps68470_probe(), I see that both cache_bypass and defer_caching flags of i2c 
regmap struct are set. So, it looks regcache_write/read calls do not come into 
play here.

So, regmap_write()
_regmap_write()
map->reg_write (drivers/base/regmap/regmap.c:1665) translates to
regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128)

These checks in regmap_i2c_write() ensure all return values from 
i2c_master_send() other than the requested number of bytes to write, are 
converted into negative values.

if (ret == count)
return 0;
else if (ret < 0)
return ret;
else
return -EIO;

Similar argument goes for regmap_read() as well.
With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to be a 
better choice. Please see if I missed anything here.

> drivers/base/regmap/regcache.c:266
> 
> " * Return a negative value on failure, 0 on success."
> 
> I can hardly find this any cleaner than "unspecified".
> 
> >  Perhaps we can also change the regmap kerneldoc headers too to match,
> > which might lessen the disparity.
> 
> I'm not familiar with the guts of regmap API, so, I would stick with if (ret 
> < 0)
> for now until documentation specifies positive return values.
> 

Ack

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

[Mani, Rajmohan]

Hi Lee, Andy,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones  wrote:
> > On Tue, 25 Jul 2017, Andy Shevchenko wrote:
> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones  wrote:
> 
> >> I briefly checked few ->read() and ->write() implementations and
> >> didn't find any evidence of positive numbers that can be returned.
> >> Documentation (kernel doc) doesn't shed a light on that. So, to me it
> >> sounds unspecified.
> >>
> >> So, for now (until documentation will be fixed) I would rely on if
> >> (ret < 0)
> >
> > It's not unspecified.  The regmap methods call into regcache_write(),
> > where the kerneldoc is clear.
> 

Since, we are interested in the regmap for the I2C bus here, I looked into the 
implementation of
 __devm_regmap_init()
__regmap_init()
regcache_init()
for I2C bus.

At the end of __devm_regmap_init() call from devm_regmap_init_i2c() inside 
tps68470_probe(), I see that both cache_bypass and defer_caching flags of i2c 
regmap struct are set. So, it looks regcache_write/read calls do not come into 
play here.

So, regmap_write()
_regmap_write()
map->reg_write (drivers/base/regmap/regmap.c:1665) translates to
regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128)

These checks in regmap_i2c_write() ensure all return values from 
i2c_master_send() other than the requested number of bytes to write, are 
converted into negative values.

if (ret == count)
return 0;
else if (ret < 0)
return ret;
else
return -EIO;

Similar argument goes for regmap_read() as well.
With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to be a 
better choice. Please see if I missed anything here.

> drivers/base/regmap/regcache.c:266
> 
> " * Return a negative value on failure, 0 on success."
> 
> I can hardly find this any cleaner than "unspecified".
> 
> >  Perhaps we can also change the regmap kerneldoc headers too to match,
> > which might lessen the disparity.
> 
> I'm not familiar with the guts of regmap API, so, I would stick with if (ret 
> < 0)
> for now until documentation specifies positive return values.
> 

Ack

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

[Andy Shevchenko]

On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones  wrote:
> On Tue, 25 Jul 2017, Andy Shevchenko wrote:
>> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones  wrote:

>> I briefly checked few ->read() and ->write() implementations and
>> didn't find any evidence of positive numbers that can be returned.
>> Documentation (kernel doc) doesn't shed a light on that. So, to me it
>> sounds unspecified.
>>
>> So, for now (until documentation will be fixed) I would rely on
>> if (ret < 0)
>
> It's not unspecified.  The regmap methods call into regcache_write(),
> where the kerneldoc is clear.

drivers/base/regmap/regcache.c:266

" * Return a negative value on failure, 0 on success."

I can hardly find this any cleaner than "unspecified".

>  Perhaps we can also change the regmap
> kerneldoc headers too to match, which might lessen the disparity.

I'm not familiar with the guts of regmap API, so, I would stick with
if (ret < 0) for now until documentation specifies positive return values.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones  wrote:
> On Tue, 25 Jul 2017, Andy Shevchenko wrote:
>> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones  wrote:

>> I briefly checked few ->read() and ->write() implementations and
>> didn't find any evidence of positive numbers that can be returned.
>> Documentation (kernel doc) doesn't shed a light on that. So, to me it
>> sounds unspecified.
>>
>> So, for now (until documentation will be fixed) I would rely on
>> if (ret < 0)
>
> It's not unspecified.  The regmap methods call into regcache_write(),
> where the kerneldoc is clear.

drivers/base/regmap/regcache.c:266

" * Return a negative value on failure, 0 on success."

I can hardly find this any cleaner than "unspecified".

>  Perhaps we can also change the regmap
> kerneldoc headers too to match, which might lessen the disparity.

I'm not familiar with the guts of regmap API, so, I would stick with
if (ret < 0) for now until documentation specifies positive return values.

-- 
With Best Regards,
Andy Shevchenko

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

[Lee Jones]

On Tue, 25 Jul 2017, Andy Shevchenko wrote:

> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones  wrote:
> > On Fri, 21 Jul 2017, Mani, Rajmohan wrote:
> >> > On Wed, 19 Jul 2017, Rajmohan Mani wrote:
> 
> >> > > + /* 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.
> 
> Did you mean
> 
> if (ret)
>  return ret;
> 
> ?

Yes, I just noticed!

Apologies for the confusion.

> I briefly checked few ->read() and ->write() implementations and
> didn't find any evidence of positive numbers that can be returned.
> Documentation (kernel doc) doesn't shed a light on that. So, to me it
> sounds unspecified.
> 
> So, for now (until documentation will be fixed) I would rely on
> if (ret < 0)

It's not unspecified.  The regmap methods call into regcache_write(),
where the kerneldoc is clear.  Perhaps we can also change the regmap
kerneldoc headers too to match, which might lessen the disparity.

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

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

[Lee Jones]

On Tue, 25 Jul 2017, Andy Shevchenko wrote:

> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones  wrote:
> > On Fri, 21 Jul 2017, Mani, Rajmohan wrote:
> >> > On Wed, 19 Jul 2017, Rajmohan Mani wrote:
> 
> >> > > + /* 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.
> 
> Did you mean
> 
> if (ret)
>  return ret;
> 
> ?

Yes, I just noticed!

Apologies for the confusion.

> I briefly checked few ->read() and ->write() implementations and
> didn't find any evidence of positive numbers that can be returned.
> Documentation (kernel doc) doesn't shed a light on that. So, to me it
> sounds unspecified.
> 
> So, for now (until documentation will be fixed) I would rely on
> if (ret < 0)

It's not unspecified.  The regmap methods call into regcache_write(),
where the kerneldoc is clear.  Perhaps we can also change the regmap
kerneldoc headers too to match, which might lessen the disparity.

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

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

[Lee Jones]

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

> > > > > + return ret;
> > > > > +
> > > > > + ret = regmap_read(regmap, TPS68470_REG_REVID, );
> > > > > + 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 = >dev;
> > > > > + struct regmap *regmap;
> > > > > + int ret;
> > > > > +
> > > > > + regmap = devm_regmap_init_i2c(client, _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

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

[Lee Jones]

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

> > > > > + return ret;
> > > > > +
> > > > > + ret = regmap_read(regmap, TPS68470_REG_REVID, );
> > > > > + 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 = >dev;
> > > > > + struct regmap *regmap;
> > > > > + int ret;
> > > > > +
> > > > > + regmap = devm_regmap_init_i2c(client, _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

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

[Mani, Rajmohan]

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

regmap_write() and regmap_read() functions return 0 on success. Hence we can 
not use 'if (!ret)' here, since we check for error conditions.

> > > > +   return ret;
> > > > +
> > > > +   ret = regmap_read(regmap, TPS68470_REG_REVID, );
> > > > +   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 = >dev;
> > > > +   struct regmap *regmap;
> > > > +   int ret;
> > > > +
> > > > +   regmap = devm_regmap_init_i2c(client, _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

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

[Mani, Rajmohan]

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

> > > > +   return ret;
> > > > +
> > > > +   ret = regmap_read(regmap, TPS68470_REG_REVID, );
> > > > +   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 = >dev;
> > > > +   struct regmap *regmap;
> > > > +   int ret;
> > > > +
> > > > +   regmap = devm_regmap_init_i2c(client, _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

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

[Andy Shevchenko]

On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones  wrote:
> On Fri, 21 Jul 2017, Mani, Rajmohan wrote:
>> > On Wed, 19 Jul 2017, Rajmohan Mani wrote:

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

Did you mean

if (ret)
 return ret;

?

I briefly checked few ->read() and ->write() implementations and
didn't find any evidence of positive numbers that can be returned.
Documentation (kernel doc) doesn't shed a light on that. So, to me it
sounds unspecified.

So, for now (until documentation will be fixed) I would rely on
if (ret < 0)


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones  wrote:
> On Fri, 21 Jul 2017, Mani, Rajmohan wrote:
>> > On Wed, 19 Jul 2017, Rajmohan Mani wrote:

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

Did you mean

if (ret)
 return ret;

?

I briefly checked few ->read() and ->write() implementations and
didn't find any evidence of positive numbers that can be returned.
Documentation (kernel doc) doesn't shed a light on that. So, to me it
sounds unspecified.

So, for now (until documentation will be fixed) I would rely on
if (ret < 0)


-- 
With Best Regards,
Andy Shevchenko

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

[Lee Jones]

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 
> > > ---
> > >  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, );
> > > + 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 = >dev;
> > > + struct regmap *regmap;
> > > + int ret;
> > > +
> > > + regmap = devm_regmap_init_i2c(client, _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

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

[Lee Jones]

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 
> > > ---
> > >  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, );
> > > + 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 = >dev;
> > > + struct regmap *regmap;
> > > + int ret;
> > > +
> > > + regmap = devm_regmap_init_i2c(client, _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

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

[Mani, Rajmohan]

Hi Lee,

> > > + * This program is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU General Public License as
> > > + * published by the Free Software Foundation version 2.
> > > + *
> > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > > + * kind, whether express or implied; without even the implied
> > > + warranty
> > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> the
> > > + * GNU General Public License for more details.
> >
> > Can you use the short version?
> >
> 
> Ack
> I will update the other patches of this series too.
> 

I just confirmed that we are recommended to use the above long version of 
headers. So, these headers will stay as such.

Thanks
Raj

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

[Mani, Rajmohan]

Hi Lee,

> > > + * This program is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU General Public License as
> > > + * published by the Free Software Foundation version 2.
> > > + *
> > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > > + * kind, whether express or implied; without even the implied
> > > + warranty
> > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> the
> > > + * GNU General Public License for more details.
> >
> > Can you use the short version?
> >
> 
> Ack
> I will update the other patches of this series too.
> 

I just confirmed that we are recommended to use the above long version of 
headers. So, these headers will stay as such.

Thanks
Raj

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

[Mani, Rajmohan]

Hi Lee,

Thanks for the reviews.

> -Original Message-
> From: Lee Jones [mailto:lee.jo...@linaro.org]
> Sent: Thursday, July 20, 2017 2:03 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Linus Walleij <linus.wall...@linaro.org>; Alexandre
> Courbot <gnu...@gmail.com>; Rafael J. Wysocki <r...@rjwysocki.net>; Len
> Brown <l...@kernel.org>; sakari.ai...@linux.intel.com; Andy Shevchenko
> <andy.shevche...@gmail.com>
> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> 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.m...@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
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > 3eb5c93..960be43 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1311,6 +1311,24 @@ config MFD_TPS65217
> >   This driver can also be built as a module.  If so, the module
> >   will be called tps65217.
> >
> > +config MFD_TPS68470
> > +   bool "TI TPS68470 Power Management / LED chips"
> > +   depends on ACPI && I2C=y
> > +   select MFD_CORE
> > +   select REGMAP_I2C
> > +   select I2C_DESIGNWARE_PLATFORM
> > +   help
> > + If you say yes here you get support for the TPS68470 series of
> > + Power Management / LED chips.
> > +
> > + These include voltage regulators, led and other features
> 
> LED(s)
> 

Ack

> > + that are often used in portable devices.
> > +
> > + This option is a bool as it provides an ACPI operation
> > + region, which must be available before any of the devices
> > + using this, are probed. This option also configures the
> 
> Remove the ','.
> 

Ack

> > + designware-i2c driver to be builtin, for the same reason.
> 
> built-in
> 

Ack

> > +
> >  config MFD_TI_LP873X
> > tristate "TI LP873X Power Management IC"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > c16bf1e..6dd2b94 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -82,6 +82,7 @@ obj-$(CONFIG_MFD_TPS65910)+= tps65910.o
> >  obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o
> >  obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o
> >  obj-$(CONFIG_MFD_TPS65912_SPI)  += tps65912-spi.o
> > +obj-$(CONFIG_MFD_TPS68470) += tps68470.o
> >  obj-$(CONFIG_MFD_TPS80031) += tps80031.o
> >  obj-$(CONFIG_MENELAUS) += menelaus.o
> >
> > diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c new file
> > mode 100644 index 000..a692af7
> > --- /dev/null
> > +++ b/drivers/mfd/tps68470.c
> > @@ -0,0 +1,110 @@
> > +/*
> > + * TPS68470 chip family multi-function driver
> 
> Does it really cover a family?  If so 'TPS68470' seems very specific.
> 
> "Multi-Function Driver" or even better "Core" or "Parent" driver.
> 

No. This is just for TPS68470.
I picked "Parent" driver

> > + * Copyright (C) 2017 Intel Corporation
> 
> '\n' here.
> 

Ack

> > + * Authors:
> > + * Rajmohan Mani <rajmohan.m...@intel.com>
> > + * Tianshu Qiu <tian.shu@intel.com>
> > + * Jian Xu Zheng <jian.xu.zh...@intel.com>
> > + * Yuning Pu <yuning...@intel.com>
> 
> Tab these out:
> 
>  * Authors:
>  *Rajmohan Mani <rajmohan.m...@intel.com>
>  *Tianshu Qiu <tian.shu@intel.com>
>  *Jian Xu Zheng <jian.xu.zh...@intel.com>
>  *Yuning Pu <yuning...@intel.com>
> 

Ack

> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * pu

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

[Mani, Rajmohan]

Hi Lee,

Thanks for the reviews.

> -Original Message-
> From: Lee Jones [mailto:lee.jo...@linaro.org]
> Sent: Thursday, July 20, 2017 2:03 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Linus Walleij ; Alexandre
> Courbot ; Rafael J. Wysocki ; Len
> Brown ; sakari.ai...@linux.intel.com; Andy Shevchenko
> 
> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> 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 
> > ---
> >  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
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > 3eb5c93..960be43 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1311,6 +1311,24 @@ config MFD_TPS65217
> >   This driver can also be built as a module.  If so, the module
> >   will be called tps65217.
> >
> > +config MFD_TPS68470
> > +   bool "TI TPS68470 Power Management / LED chips"
> > +   depends on ACPI && I2C=y
> > +   select MFD_CORE
> > +   select REGMAP_I2C
> > +   select I2C_DESIGNWARE_PLATFORM
> > +   help
> > + If you say yes here you get support for the TPS68470 series of
> > + Power Management / LED chips.
> > +
> > + These include voltage regulators, led and other features
> 
> LED(s)
> 

Ack

> > + that are often used in portable devices.
> > +
> > + This option is a bool as it provides an ACPI operation
> > + region, which must be available before any of the devices
> > + using this, are probed. This option also configures the
> 
> Remove the ','.
> 

Ack

> > + designware-i2c driver to be builtin, for the same reason.
> 
> built-in
> 

Ack

> > +
> >  config MFD_TI_LP873X
> > tristate "TI LP873X Power Management IC"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > c16bf1e..6dd2b94 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -82,6 +82,7 @@ obj-$(CONFIG_MFD_TPS65910)+= tps65910.o
> >  obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o
> >  obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o
> >  obj-$(CONFIG_MFD_TPS65912_SPI)  += tps65912-spi.o
> > +obj-$(CONFIG_MFD_TPS68470) += tps68470.o
> >  obj-$(CONFIG_MFD_TPS80031) += tps80031.o
> >  obj-$(CONFIG_MENELAUS) += menelaus.o
> >
> > diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c new file
> > mode 100644 index 000..a692af7
> > --- /dev/null
> > +++ b/drivers/mfd/tps68470.c
> > @@ -0,0 +1,110 @@
> > +/*
> > + * TPS68470 chip family multi-function driver
> 
> Does it really cover a family?  If so 'TPS68470' seems very specific.
> 
> "Multi-Function Driver" or even better "Core" or "Parent" driver.
> 

No. This is just for TPS68470.
I picked "Parent" driver

> > + * Copyright (C) 2017 Intel Corporation
> 
> '\n' here.
> 

Ack

> > + * Authors:
> > + * Rajmohan Mani 
> > + * Tianshu Qiu 
> > + * Jian Xu Zheng 
> > + * Yuning Pu 
> 
> Tab these out:
> 
>  * Authors:
>  *Rajmohan Mani 
>  *Tianshu Qiu 
>  *Jian Xu Zheng 
>  *Yuning Pu 
> 

Ack

> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied
> > + warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> 
> Can you use the short version?
> 

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

[Lee Jones]

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 
> ---
>  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
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3eb5c93..960be43 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1311,6 +1311,24 @@ config MFD_TPS65217
> This driver can also be built as a module.  If so, the module
> will be called tps65217.
>  
> +config MFD_TPS68470
> + bool "TI TPS68470 Power Management / LED chips"
> + depends on ACPI && I2C=y
> + select MFD_CORE
> + select REGMAP_I2C
> + select I2C_DESIGNWARE_PLATFORM
> + help
> +   If you say yes here you get support for the TPS68470 series of
> +   Power Management / LED chips.
> +
> +   These include voltage regulators, led and other features

LED(s)

> +   that are often used in portable devices.
> +
> +   This option is a bool as it provides an ACPI operation
> +   region, which must be available before any of the devices
> +   using this, are probed. This option also configures the

Remove the ','.

> +   designware-i2c driver to be builtin, for the same reason.

built-in

> +
>  config MFD_TI_LP873X
>   tristate "TI LP873X Power Management IC"
>   depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index c16bf1e..6dd2b94 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_MFD_TPS65910)  += tps65910.o
>  obj-$(CONFIG_MFD_TPS65912)   += tps65912-core.o
>  obj-$(CONFIG_MFD_TPS65912_I2C)   += tps65912-i2c.o
>  obj-$(CONFIG_MFD_TPS65912_SPI)  += tps65912-spi.o
> +obj-$(CONFIG_MFD_TPS68470)   += tps68470.o
>  obj-$(CONFIG_MFD_TPS80031)   += tps80031.o
>  obj-$(CONFIG_MENELAUS)   += menelaus.o
>  
> diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c
> new file mode 100644
> index 000..a692af7
> --- /dev/null
> +++ b/drivers/mfd/tps68470.c
> @@ -0,0 +1,110 @@
> +/*
> + * TPS68470 chip family multi-function driver

Does it really cover a family?  If so 'TPS68470' seems very specific.

"Multi-Function Driver" or even better "Core" or "Parent" driver.

> + * Copyright (C) 2017 Intel Corporation

'\n' here.

> + * Authors:
> + * Rajmohan Mani 
> + * Tianshu Qiu 
> + * Jian Xu Zheng 
> + * Yuning Pu 

Tab these out:

 * Authors:
 *  Rajmohan Mani 
 *  Tianshu Qiu 
 *  Jian Xu Zheng 
 *  Yuning Pu 

> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

Can you use the short version?

> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static const struct mfd_cell tps68470s[] = {
> + {
> + .name = "tps68470-gpio",
> + },
> + {
> + .name = "tps68470_pmic_opregion",
> + },
> +};

Make these one line each:

 { .name = "tps68470-gpio" }

... and drop the ','

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

> + return ret;
> +
> + ret = regmap_read(regmap, TPS68470_REG_REVID, );
> + if (ret < 0) {

Same

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

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

[Lee Jones]

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 
> ---
>  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
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3eb5c93..960be43 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1311,6 +1311,24 @@ config MFD_TPS65217
> This driver can also be built as a module.  If so, the module
> will be called tps65217.
>  
> +config MFD_TPS68470
> + bool "TI TPS68470 Power Management / LED chips"
> + depends on ACPI && I2C=y
> + select MFD_CORE
> + select REGMAP_I2C
> + select I2C_DESIGNWARE_PLATFORM
> + help
> +   If you say yes here you get support for the TPS68470 series of
> +   Power Management / LED chips.
> +
> +   These include voltage regulators, led and other features

LED(s)

> +   that are often used in portable devices.
> +
> +   This option is a bool as it provides an ACPI operation
> +   region, which must be available before any of the devices
> +   using this, are probed. This option also configures the

Remove the ','.

> +   designware-i2c driver to be builtin, for the same reason.

built-in

> +
>  config MFD_TI_LP873X
>   tristate "TI LP873X Power Management IC"
>   depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index c16bf1e..6dd2b94 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_MFD_TPS65910)  += tps65910.o
>  obj-$(CONFIG_MFD_TPS65912)   += tps65912-core.o
>  obj-$(CONFIG_MFD_TPS65912_I2C)   += tps65912-i2c.o
>  obj-$(CONFIG_MFD_TPS65912_SPI)  += tps65912-spi.o
> +obj-$(CONFIG_MFD_TPS68470)   += tps68470.o
>  obj-$(CONFIG_MFD_TPS80031)   += tps80031.o
>  obj-$(CONFIG_MENELAUS)   += menelaus.o
>  
> diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c
> new file mode 100644
> index 000..a692af7
> --- /dev/null
> +++ b/drivers/mfd/tps68470.c
> @@ -0,0 +1,110 @@
> +/*
> + * TPS68470 chip family multi-function driver

Does it really cover a family?  If so 'TPS68470' seems very specific.

"Multi-Function Driver" or even better "Core" or "Parent" driver.

> + * Copyright (C) 2017 Intel Corporation

'\n' here.

> + * Authors:
> + * Rajmohan Mani 
> + * Tianshu Qiu 
> + * Jian Xu Zheng 
> + * Yuning Pu 

Tab these out:

 * Authors:
 *  Rajmohan Mani 
 *  Tianshu Qiu 
 *  Jian Xu Zheng 
 *  Yuning Pu 

> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

Can you use the short version?

> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static const struct mfd_cell tps68470s[] = {
> + {
> + .name = "tps68470-gpio",
> + },
> + {
> + .name = "tps68470_pmic_opregion",
> + },
> +};

Make these one line each:

 { .name = "tps68470-gpio" }

... and drop the ','

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

> + return ret;
> +
> + ret = regmap_read(regmap, TPS68470_REG_REVID, );
> + if (ret < 0) {

Same

> + 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 = >dev;
> + struct regmap *regmap;
> + int ret;
> +
> + regmap = devm_regmap_init_i2c(client, _regmap_config);
> + if (IS_ERR(regmap)) {
> +