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

2017-07-20 Thread Lee Jones 
On Wed, 19 Jul 2017, Mani, Rajmohan wrote:

> Hi Lee,
> 
> > > On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> > >
> > > > Hi Andy,
> > > >
> > > > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Jun 6, 2017 at 2:55 PM, 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.
> > > > > >
> > > > > > I dunno why you decide to send this out now, see my comments
> > below.
> > > > > >
> > > > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > > > +   unsigned int version;
> > > > > > > +   int ret;
> > > > > >
> > > > > > > +   /* FIXME: configure these dynamically */
> > > > > >
> > > > > > So, what prevents you to fix this?
> > > > >
> > > > > Nothing I suppose. They're however not needed right now and can be
> > > > > implemented later on if they're ever needed.
> > > > >
> > > >
> > > > Ack
> > >
> > > What does this mean?  Is the plan to fix it or not?  I don't want
> > > FIXMEs in the code that a) can be fixed right away or b) might never be
> > fixed.
> > >
> > 
> > I meant that this can be implemented later on, if there's a need.
> > I will look into this and see how this can be fixed.
> > 
> 
> Thanks for your patience.
> This is fixed with v4 of this series.

Great, thanks.

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

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

2017-07-20 Thread Lee Jones 
On Wed, 19 Jul 2017, Mani, Rajmohan wrote:

> Hi Lee,
> 
> > > On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> > >
> > > > Hi Andy,
> > > >
> > > > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Jun 6, 2017 at 2:55 PM, 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.
> > > > > >
> > > > > > I dunno why you decide to send this out now, see my comments
> > below.
> > > > > >
> > > > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > > > +   unsigned int version;
> > > > > > > +   int ret;
> > > > > >
> > > > > > > +   /* FIXME: configure these dynamically */
> > > > > >
> > > > > > So, what prevents you to fix this?
> > > > >
> > > > > Nothing I suppose. They're however not needed right now and can be
> > > > > implemented later on if they're ever needed.
> > > > >
> > > >
> > > > Ack
> > >
> > > What does this mean?  Is the plan to fix it or not?  I don't want
> > > FIXMEs in the code that a) can be fixed right away or b) might never be
> > fixed.
> > >
> > 
> > I meant that this can be implemented later on, if there's a need.
> > I will look into this and see how this can be fixed.
> > 
> 
> Thanks for your patience.
> This is fixed with v4 of this series.

Great, thanks.

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

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

2017-07-19 Thread Mani, Rajmohan 
Hi Lee,

> > On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> >
> > > Hi Andy,
> > >
> > > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Jun 6, 2017 at 2:55 PM, 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.
> > > > >
> > > > > I dunno why you decide to send this out now, see my comments
> below.
> > > > >
> > > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > > +   unsigned int version;
> > > > > > +   int ret;
> > > > >
> > > > > > +   /* FIXME: configure these dynamically */
> > > > >
> > > > > So, what prevents you to fix this?
> > > >
> > > > Nothing I suppose. They're however not needed right now and can be
> > > > implemented later on if they're ever needed.
> > > >
> > >
> > > Ack
> >
> > What does this mean?  Is the plan to fix it or not?  I don't want
> > FIXMEs in the code that a) can be fixed right away or b) might never be
> fixed.
> >
> 
> I meant that this can be implemented later on, if there's a need.
> I will look into this and see how this can be fixed.
> 

Thanks for your patience.
This is fixed with v4 of this series.

Raj

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

2017-07-19 Thread Mani, Rajmohan 
Hi Lee,

> > On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> >
> > > Hi Andy,
> > >
> > > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Jun 6, 2017 at 2:55 PM, 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.
> > > > >
> > > > > I dunno why you decide to send this out now, see my comments
> below.
> > > > >
> > > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > > +   unsigned int version;
> > > > > > +   int ret;
> > > > >
> > > > > > +   /* FIXME: configure these dynamically */
> > > > >
> > > > > So, what prevents you to fix this?
> > > >
> > > > Nothing I suppose. They're however not needed right now and can be
> > > > implemented later on if they're ever needed.
> > > >
> > >
> > > Ack
> >
> > What does this mean?  Is the plan to fix it or not?  I don't want
> > FIXMEs in the code that a) can be fixed right away or b) might never be
> fixed.
> >
> 
> I meant that this can be implemented later on, if there's a need.
> I will look into this and see how this can be fixed.
> 

Thanks for your patience.
This is fixed with v4 of this series.

Raj

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

2017-06-12 Thread Mani, Rajmohan 
Hi Lee,

> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> 
> > Hi Andy,
> >
> > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > <rajmohan.m...@intel.com> 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.
> > > >
> > > > I dunno why you decide to send this out now, see my comments below.
> > > >
> > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > +   unsigned int version;
> > > > > +   int ret;
> > > >
> > > > > +   /* FIXME: configure these dynamically */
> > > >
> > > > So, what prevents you to fix this?
> > >
> > > Nothing I suppose. They're however not needed right now and can be
> > > implemented later on if they're ever needed.
> > >
> >
> > Ack
> 
> What does this mean?  Is the plan to fix it or not?  I don't want FIXMEs in 
> the
> code that a) can be fixed right away or b) might never be fixed.
> 

I meant that this can be implemented later on, if there's a need.
I will look into this and see how this can be fixed.

Thanks
Raj

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

2017-06-12 Thread Mani, Rajmohan 
Hi Lee,

> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> 
> > Hi Andy,
> >
> > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Jun 6, 2017 at 2:55 PM, 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.
> > > >
> > > > I dunno why you decide to send this out now, see my comments below.
> > > >
> > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > +   unsigned int version;
> > > > > +   int ret;
> > > >
> > > > > +   /* FIXME: configure these dynamically */
> > > >
> > > > So, what prevents you to fix this?
> > >
> > > Nothing I suppose. They're however not needed right now and can be
> > > implemented later on if they're ever needed.
> > >
> >
> > Ack
> 
> What does this mean?  Is the plan to fix it or not?  I don't want FIXMEs in 
> the
> code that a) can be fixed right away or b) might never be fixed.
> 

I meant that this can be implemented later on, if there's a need.
I will look into this and see how this can be fixed.

Thanks
Raj

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

2017-06-12 Thread Lee Jones 
On Fri, 09 Jun 2017, Mani, Rajmohan wrote:

> Hi Andy,
> 
> > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > On Tue, Jun 6, 2017 at 2:55 PM, 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.
> > >
> > > I dunno why you decide to send this out now, see my comments below.
> > >
> > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > +   unsigned int version;
> > > > +   int ret;
> > >
> > > > +   /* FIXME: configure these dynamically */
> > >
> > > So, what prevents you to fix this?
> > 
> > Nothing I suppose. They're however not needed right now and can be
> > implemented later on if they're ever needed.
> > 
> 
> Ack

What does this mean?  Is the plan to fix it or not?  I don't want
FIXMEs in the code that a) can be fixed right away or b) might never
be fixed.

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

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

2017-06-12 Thread Lee Jones 
On Fri, 09 Jun 2017, Mani, Rajmohan wrote:

> Hi Andy,
> 
> > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > On Tue, Jun 6, 2017 at 2:55 PM, 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.
> > >
> > > I dunno why you decide to send this out now, see my comments below.
> > >
> > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > +   unsigned int version;
> > > > +   int ret;
> > >
> > > > +   /* FIXME: configure these dynamically */
> > >
> > > So, what prevents you to fix this?
> > 
> > Nothing I suppose. They're however not needed right now and can be
> > implemented later on if they're ever needed.
> > 
> 
> Ack

What does this mean?  Is the plan to fix it or not?  I don't want
FIXMEs in the code that a) can be fixed right away or b) might never
be fixed.

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

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

2017-06-09 Thread Mani, Rajmohan 
Hi Andy,

> On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 6, 2017 at 2:55 PM, 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.
> >
> > I dunno why you decide to send this out now, see my comments below.
> >
> > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > +   unsigned int version;
> > > +   int ret;
> >
> > > +   /* FIXME: configure these dynamically */
> >
> > So, what prevents you to fix this?
> 
> Nothing I suppose. They're however not needed right now and can be
> implemented later on if they're ever needed.
> 

Ack

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

2017-06-09 Thread Mani, Rajmohan 
Hi Andy,

> On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 6, 2017 at 2:55 PM, 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.
> >
> > I dunno why you decide to send this out now, see my comments below.
> >
> > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > +   unsigned int version;
> > > +   int ret;
> >
> > > +   /* FIXME: configure these dynamically */
> >
> > So, what prevents you to fix this?
> 
> Nothing I suppose. They're however not needed right now and can be
> implemented later on if they're ever needed.
> 

Ack

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

2017-06-09 Thread Mani, Rajmohan 
Hi Andy,

Thanks for the reviews and patience.

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Tuesday, June 06, 2017 6:00 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones <lee.jo...@linaro.org>; Linus Walleij
> <linus.wall...@linaro.org>; Alexandre Courbot <gnu...@gmail.com>; Rafael J.
> Wysocki <r...@rjwysocki.net>; Len Brown <l...@kernel.org>
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.m...@intel.com>
> 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.
> 
> I dunno why you decide to send this out now, see my comments below.
> 

We decided to go with the submission of these drivers for upstream review 
sooner rather than later.

> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> 
> > +   /* FIXME: configure these dynamically */
> 
> So, what prevents you to fix this?
> 

I will respond on top of Sakari's response.

> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as
> > + output */
> 
> > +}
> 
> > +static int tps68470_probe(struct i2c_client *client) {
> > +   struct tps68470 *tps;
> > +   int ret;
> > +
> > +   tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
> > +   if (!tps)
> > +   return -ENOMEM;
> > +
> > +   mutex_init(>lock);
> > +   i2c_set_clientdata(client, tps);
> > +   tps->dev = >dev;
> > +
> > +   tps->regmap = devm_regmap_init_i2c(client,
> _regmap_config);
> > +   if (IS_ERR(tps->regmap)) {
> > +   dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
> > +   return PTR_ERR(tps->regmap);
> > +   }
> > +
> 
> > +   ret = mfd_add_devices(tps->dev, -1, tps68470s,
> > + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> 
> devm_?
> 

Ack

> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   ret = tps68470_chip_init(tps);
> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
> > +   goto fail;
> > +   }
> > +
> > +   return 0;
> 
> > +fail:
> > +   mutex_lock(>lock);
> 
> I'm not sure you need this mutex to be held here.
> Otherwise your code has a bug with locking.
> 

Repeating the response to Heikki here

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> > +
> > +   return ret;
> 
> Taking above into consideration I suggest to clarify your locking scheme.
> 

Same as above.

> > +}
> > +
> > +static int tps68470_remove(struct i2c_client *client) {
> > +   struct tps68470 *tps = i2c_get_clientdata(client);
> > +
> 
> > +   mutex_lock(>lock);
> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> 
> Ditto.
> 

Same as above

> > +   return 0;
> > +}
> 
> > +/**
> > + * struct tps68470 - tps68470 sub-driver chip access routines
> > + *
> 
> kbuild bot will be unhappy. You need to file a description per field.
> 

Ack
It looks like this structure will go away, once I implement the feedback from 
Heikki.

> > + * Device data may be used to access the TPS68470 chip */
> > +
> > +struct tps68470 {
> > +   struct device *dev;
> > +   struct regmap *regmap;
> 
> > +   /*
> > +* Used to synchronize access to tps68470_ operations
> > +* and addition and removal of mfd devices
> > +*/
> 
> Move it to kernel-doc above.
> 

Same as above

> > +   struct mutex lock;
> > +};
> 
> --
> With Best Regards,
> Andy Shevchenko

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

2017-06-09 Thread Mani, Rajmohan 
Hi Andy,

Thanks for the reviews and patience.

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Tuesday, June 06, 2017 6:00 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Tue, Jun 6, 2017 at 2:55 PM, 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.
> 
> I dunno why you decide to send this out now, see my comments below.
> 

We decided to go with the submission of these drivers for upstream review 
sooner rather than later.

> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> 
> > +   /* FIXME: configure these dynamically */
> 
> So, what prevents you to fix this?
> 

I will respond on top of Sakari's response.

> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as
> > + output */
> 
> > +}
> 
> > +static int tps68470_probe(struct i2c_client *client) {
> > +   struct tps68470 *tps;
> > +   int ret;
> > +
> > +   tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
> > +   if (!tps)
> > +   return -ENOMEM;
> > +
> > +   mutex_init(>lock);
> > +   i2c_set_clientdata(client, tps);
> > +   tps->dev = >dev;
> > +
> > +   tps->regmap = devm_regmap_init_i2c(client,
> _regmap_config);
> > +   if (IS_ERR(tps->regmap)) {
> > +   dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
> > +   return PTR_ERR(tps->regmap);
> > +   }
> > +
> 
> > +   ret = mfd_add_devices(tps->dev, -1, tps68470s,
> > + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> 
> devm_?
> 

Ack

> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   ret = tps68470_chip_init(tps);
> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
> > +   goto fail;
> > +   }
> > +
> > +   return 0;
> 
> > +fail:
> > +   mutex_lock(>lock);
> 
> I'm not sure you need this mutex to be held here.
> Otherwise your code has a bug with locking.
> 

Repeating the response to Heikki here

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> > +
> > +   return ret;
> 
> Taking above into consideration I suggest to clarify your locking scheme.
> 

Same as above.

> > +}
> > +
> > +static int tps68470_remove(struct i2c_client *client) {
> > +   struct tps68470 *tps = i2c_get_clientdata(client);
> > +
> 
> > +   mutex_lock(>lock);
> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> 
> Ditto.
> 

Same as above

> > +   return 0;
> > +}
> 
> > +/**
> > + * struct tps68470 - tps68470 sub-driver chip access routines
> > + *
> 
> kbuild bot will be unhappy. You need to file a description per field.
> 

Ack
It looks like this structure will go away, once I implement the feedback from 
Heikki.

> > + * Device data may be used to access the TPS68470 chip */
> > +
> > +struct tps68470 {
> > +   struct device *dev;
> > +   struct regmap *regmap;
> 
> > +   /*
> > +* Used to synchronize access to tps68470_ operations
> > +* and addition and removal of mfd devices
> > +*/
> 
> Move it to kernel-doc above.
> 

Same as above

> > +   struct mutex lock;
> > +};
> 
> --
> With Best Regards,
> Andy Shevchenko

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

2017-06-09 Thread Mani, Rajmohan 
Hi Heikki,

Thanks for the reviews and patience.

> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: Tuesday, June 06, 2017 5:49 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones <lee.jo...@linaro.org>; Linus Walleij
> <linus.wall...@linaro.org>; Alexandre Courbot <gnu...@gmail.com>; Rafael J.
> Wysocki <r...@rjwysocki.net>; Len Brown <l...@kernel.org>
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> Hi Rajmohan,
> 
> On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote:
> > +/*
> > + * tps68470_reg_read: Read a single tps68470 register.
> > + *
> > + * @tps: Device to read from.
> > + * @reg: Register to read.
> > + * @val: Contains the value
> > + */
> > +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int *val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_read(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_read);
> > +
> > +/*
> > + * tps68470_reg_write: Write a single tps68470 register.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to write to.
> > + * @val: Value to write.
> > + */
> > +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_write(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_write);
> > +
> > +/*
> > + * tps68470_update_bits: Modify bits w.r.t mask and val.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to read-write to.
> > + * @mask: Mask.
> > + * @val: Value to write.
> > + */
> > +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int mask, unsigned int val) {
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_update_bits(tps->regmap, reg, mask, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_update_bits);
> 
> I'm not sure you need those above wrappers at all, regmap is handling locking 
> in
> any case.
> 

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +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 tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> > +
> > +   ret = tps68470_reg_read(tps, TPS68470_REG_REVID, );
> > +   if (ret < 0) {
> > +   dev_err(tps->dev,
> > +   "Failed to read revision register: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version);
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* FIXME: configure these dynamically */
> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as output */
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps6847

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

2017-06-09 Thread Mani, Rajmohan 
Hi Heikki,

Thanks for the reviews and patience.

> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: Tuesday, June 06, 2017 5:49 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> Hi Rajmohan,
> 
> On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote:
> > +/*
> > + * tps68470_reg_read: Read a single tps68470 register.
> > + *
> > + * @tps: Device to read from.
> > + * @reg: Register to read.
> > + * @val: Contains the value
> > + */
> > +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int *val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_read(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_read);
> > +
> > +/*
> > + * tps68470_reg_write: Write a single tps68470 register.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to write to.
> > + * @val: Value to write.
> > + */
> > +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_write(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_write);
> > +
> > +/*
> > + * tps68470_update_bits: Modify bits w.r.t mask and val.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to read-write to.
> > + * @mask: Mask.
> > + * @val: Value to write.
> > + */
> > +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int mask, unsigned int val) {
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_update_bits(tps->regmap, reg, mask, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_update_bits);
> 
> I'm not sure you need those above wrappers at all, regmap is handling locking 
> in
> any case.
> 

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +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 tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> > +
> > +   ret = tps68470_reg_read(tps, TPS68470_REG_REVID, );
> > +   if (ret < 0) {
> > +   dev_err(tps->dev,
> > +   "Failed to read revision register: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version);
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* FIXME: configure these dynamically */
> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as output */
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL6A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /*
> > +* When SDA and SCL are r

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

2017-06-07 Thread Sakari Ailus 
Hi Andy,

On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 6, 2017 at 2:55 PM, 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.
> 
> I dunno why you decide to send this out now, see my comments below.
> 
> > +static int tps68470_chip_init(struct tps68470 *tps)
> > +{
> > +   unsigned int version;
> > +   int ret;
> 
> > +   /* FIXME: configure these dynamically */
> 
> So, what prevents you to fix this?

Nothing I suppose. They're however not needed right now and can be
implemented later on if they're ever needed.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

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

2017-06-07 Thread Sakari Ailus 
Hi Andy,

On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 6, 2017 at 2:55 PM, 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.
> 
> I dunno why you decide to send this out now, see my comments below.
> 
> > +static int tps68470_chip_init(struct tps68470 *tps)
> > +{
> > +   unsigned int version;
> > +   int ret;
> 
> > +   /* FIXME: configure these dynamically */
> 
> So, what prevents you to fix this?

Nothing I suppose. They're however not needed right now and can be
implemented later on if they're ever needed.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

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

2017-06-07 Thread kbuild test robot 
Hi Rajmohan,

[auto build test ERROR on ljones-mfd/for-mfd-next]
[also build test ERROR on v4.12-rc4 next-20170607]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Rajmohan-Mani/mfd-Add-new-mfd-device-TPS68470/20170607-080306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=alpha 

All error/warnings (new ones prefixed by >>):

>> drivers/mfd/tps68470.c:217:1: warning: data definition has no type or 
>> storage class
MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids);
^~~
>> drivers/mfd/tps68470.c:217:1: error: type defaults to 'int' in declaration 
>> of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
>> drivers/mfd/tps68470.c:217:1: warning: parameter names (without types) in 
>> function declaration
   drivers/mfd/tps68470.c: In function 'tps68470_probe':
   drivers/mfd/tps68470.c:175:3: warning: 'ret' may be used uninitialized in 
this function [-Wmaybe-uninitialized]
  dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
  ^
   cc1: some warnings being treated as errors

vim +217 drivers/mfd/tps68470.c

   211  
   212  static const struct acpi_device_id tps68470_acpi_ids[] = {
   213  {"INT3472"},
   214  {},
   215  };
   216  
 > 217  MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids);
   218  
   219  static struct i2c_driver tps68470_driver = {
   220  .driver = {

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

2017-06-07 Thread kbuild test robot 
Hi Rajmohan,

[auto build test ERROR on ljones-mfd/for-mfd-next]
[also build test ERROR on v4.12-rc4 next-20170607]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Rajmohan-Mani/mfd-Add-new-mfd-device-TPS68470/20170607-080306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=alpha 

All error/warnings (new ones prefixed by >>):

>> drivers/mfd/tps68470.c:217:1: warning: data definition has no type or 
>> storage class
MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids);
^~~
>> drivers/mfd/tps68470.c:217:1: error: type defaults to 'int' in declaration 
>> of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
>> drivers/mfd/tps68470.c:217:1: warning: parameter names (without types) in 
>> function declaration
   drivers/mfd/tps68470.c: In function 'tps68470_probe':
   drivers/mfd/tps68470.c:175:3: warning: 'ret' may be used uninitialized in 
this function [-Wmaybe-uninitialized]
  dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
  ^
   cc1: some warnings being treated as errors

vim +217 drivers/mfd/tps68470.c

   211  
   212  static const struct acpi_device_id tps68470_acpi_ids[] = {
   213  {"INT3472"},
   214  {},
   215  };
   216  
 > 217  MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids);
   218  
   219  static struct i2c_driver tps68470_driver = {
   220  .driver = {

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

2017-06-06 Thread kbuild test robot 
Hi Rajmohan,

[auto build test WARNING on ljones-mfd/for-mfd-next]
[also build test WARNING on v4.12-rc4 next-20170606]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Rajmohan-Mani/mfd-Add-new-mfd-device-TPS68470/20170607-080306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/mfd/tps68470.c: In function 'tps68470_probe':
>> drivers/mfd/tps68470.c:175:3: warning: 'ret' may be used uninitialized in 
>> this function [-Wmaybe-uninitialized]
  dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
  ^

vim +/ret +175 drivers/mfd/tps68470.c

   159  
   160  static int tps68470_probe(struct i2c_client *client)
   161  {
   162  struct tps68470 *tps;
   163  int ret;
   164  
   165  tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
   166  if (!tps)
   167  return -ENOMEM;
   168  
   169  mutex_init(>lock);
   170  i2c_set_clientdata(client, tps);
   171  tps->dev = >dev;
   172  
   173  tps->regmap = devm_regmap_init_i2c(client, 
_regmap_config);
   174  if (IS_ERR(tps->regmap)) {
 > 175  dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", 
 > ret);
   176  return PTR_ERR(tps->regmap);
   177  }
   178  
   179  ret = mfd_add_devices(tps->dev, -1, tps68470s,
   180ARRAY_SIZE(tps68470s), NULL, 0, NULL);
   181  if (ret < 0) {
   182  dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
   183  return ret;

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

2017-06-06 Thread kbuild test robot 
Hi Rajmohan,

[auto build test WARNING on ljones-mfd/for-mfd-next]
[also build test WARNING on v4.12-rc4 next-20170606]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Rajmohan-Mani/mfd-Add-new-mfd-device-TPS68470/20170607-080306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/mfd/tps68470.c: In function 'tps68470_probe':
>> drivers/mfd/tps68470.c:175:3: warning: 'ret' may be used uninitialized in 
>> this function [-Wmaybe-uninitialized]
  dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
  ^

vim +/ret +175 drivers/mfd/tps68470.c

   159  
   160  static int tps68470_probe(struct i2c_client *client)
   161  {
   162  struct tps68470 *tps;
   163  int ret;
   164  
   165  tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
   166  if (!tps)
   167  return -ENOMEM;
   168  
   169  mutex_init(>lock);
   170  i2c_set_clientdata(client, tps);
   171  tps->dev = >dev;
   172  
   173  tps->regmap = devm_regmap_init_i2c(client, 
_regmap_config);
   174  if (IS_ERR(tps->regmap)) {
 > 175  dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", 
 > ret);
   176  return PTR_ERR(tps->regmap);
   177  }
   178  
   179  ret = mfd_add_devices(tps->dev, -1, tps68470s,
   180ARRAY_SIZE(tps68470s), NULL, 0, NULL);
   181  if (ret < 0) {
   182  dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
   183  return ret;

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

2017-06-06 Thread Andy Shevchenko 
On Tue, Jun 6, 2017 at 2:55 PM, 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.

I dunno why you decide to send this out now, see my comments below.

> +static int tps68470_chip_init(struct tps68470 *tps)
> +{
> +   unsigned int version;
> +   int ret;

> +   /* FIXME: configure these dynamically */

So, what prevents you to fix this?

> +   /* Enable Daisy Chain LDO and configure relevant GPIOs as output */

> +}

> +static int tps68470_probe(struct i2c_client *client)
> +{
> +   struct tps68470 *tps;
> +   int ret;
> +
> +   tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
> +   if (!tps)
> +   return -ENOMEM;
> +
> +   mutex_init(>lock);
> +   i2c_set_clientdata(client, tps);
> +   tps->dev = >dev;
> +
> +   tps->regmap = devm_regmap_init_i2c(client, _regmap_config);
> +   if (IS_ERR(tps->regmap)) {
> +   dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
> +   return PTR_ERR(tps->regmap);
> +   }
> +

> +   ret = mfd_add_devices(tps->dev, -1, tps68470s,
> + ARRAY_SIZE(tps68470s), NULL, 0, NULL);

devm_?

> +   if (ret < 0) {
> +   dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> +   return ret;
> +   }
> +
> +   ret = tps68470_chip_init(tps);
> +   if (ret < 0) {
> +   dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
> +   goto fail;
> +   }
> +
> +   return 0;

> +fail:
> +   mutex_lock(>lock);

I'm not sure you need this mutex to be held here.
Otherwise your code has a bug with locking.

> +   mfd_remove_devices(tps->dev);
> +   mutex_unlock(>lock);
> +
> +   return ret;

Taking above into consideration I suggest to clarify your locking scheme.

> +}
> +
> +static int tps68470_remove(struct i2c_client *client)
> +{
> +   struct tps68470 *tps = i2c_get_clientdata(client);
> +

> +   mutex_lock(>lock);
> +   mfd_remove_devices(tps->dev);
> +   mutex_unlock(>lock);

Ditto.

> +   return 0;
> +}

> +/**
> + * struct tps68470 - tps68470 sub-driver chip access routines
> + *

kbuild bot will be unhappy. You need to file a description per field.

> + * Device data may be used to access the TPS68470 chip
> + */
> +
> +struct tps68470 {
> +   struct device *dev;
> +   struct regmap *regmap;

> +   /*
> +* Used to synchronize access to tps68470_ operations
> +* and addition and removal of mfd devices
> +*/

Move it to kernel-doc above.

> +   struct mutex lock;
> +};

-- 
With Best Regards,
Andy Shevchenko

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

2017-06-06 Thread Andy Shevchenko 
On Tue, Jun 6, 2017 at 2:55 PM, 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.

I dunno why you decide to send this out now, see my comments below.

> +static int tps68470_chip_init(struct tps68470 *tps)
> +{
> +   unsigned int version;
> +   int ret;

> +   /* FIXME: configure these dynamically */

So, what prevents you to fix this?

> +   /* Enable Daisy Chain LDO and configure relevant GPIOs as output */

> +}

> +static int tps68470_probe(struct i2c_client *client)
> +{
> +   struct tps68470 *tps;
> +   int ret;
> +
> +   tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
> +   if (!tps)
> +   return -ENOMEM;
> +
> +   mutex_init(>lock);
> +   i2c_set_clientdata(client, tps);
> +   tps->dev = >dev;
> +
> +   tps->regmap = devm_regmap_init_i2c(client, _regmap_config);
> +   if (IS_ERR(tps->regmap)) {
> +   dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
> +   return PTR_ERR(tps->regmap);
> +   }
> +

> +   ret = mfd_add_devices(tps->dev, -1, tps68470s,
> + ARRAY_SIZE(tps68470s), NULL, 0, NULL);

devm_?

> +   if (ret < 0) {
> +   dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> +   return ret;
> +   }
> +
> +   ret = tps68470_chip_init(tps);
> +   if (ret < 0) {
> +   dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
> +   goto fail;
> +   }
> +
> +   return 0;

> +fail:
> +   mutex_lock(>lock);

I'm not sure you need this mutex to be held here.
Otherwise your code has a bug with locking.

> +   mfd_remove_devices(tps->dev);
> +   mutex_unlock(>lock);
> +
> +   return ret;

Taking above into consideration I suggest to clarify your locking scheme.

> +}
> +
> +static int tps68470_remove(struct i2c_client *client)
> +{
> +   struct tps68470 *tps = i2c_get_clientdata(client);
> +

> +   mutex_lock(>lock);
> +   mfd_remove_devices(tps->dev);
> +   mutex_unlock(>lock);

Ditto.

> +   return 0;
> +}

> +/**
> + * struct tps68470 - tps68470 sub-driver chip access routines
> + *

kbuild bot will be unhappy. You need to file a description per field.

> + * Device data may be used to access the TPS68470 chip
> + */
> +
> +struct tps68470 {
> +   struct device *dev;
> +   struct regmap *regmap;

> +   /*
> +* Used to synchronize access to tps68470_ operations
> +* and addition and removal of mfd devices
> +*/

Move it to kernel-doc above.

> +   struct mutex lock;
> +};

-- 
With Best Regards,
Andy Shevchenko

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

2017-06-06 Thread Heikki Krogerus 
Hi Rajmohan,

On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote:
> +/*
> + * tps68470_reg_read: Read a single tps68470 register.
> + *
> + * @tps: Device to read from.
> + * @reg: Register to read.
> + * @val: Contains the value
> + */
> +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg,
> + unsigned int *val)
> +{
> + int ret;
> +
> + mutex_lock(>lock);
> + ret = regmap_read(tps->regmap, reg, val);
> + mutex_unlock(>lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tps68470_reg_read);
> +
> +/*
> + * tps68470_reg_write: Write a single tps68470 register.
> + *
> + * @tps68470: Device to write to.
> + * @reg: Register to write to.
> + * @val: Value to write.
> + */
> +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg,
> + unsigned int val)
> +{
> + int ret;
> +
> + mutex_lock(>lock);
> + ret = regmap_write(tps->regmap, reg, val);
> + mutex_unlock(>lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tps68470_reg_write);
> +
> +/*
> + * tps68470_update_bits: Modify bits w.r.t mask and val.
> + *
> + * @tps68470: Device to write to.
> + * @reg: Register to read-write to.
> + * @mask: Mask.
> + * @val: Value to write.
> + */
> +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + int ret;
> +
> + mutex_lock(>lock);
> + ret = regmap_update_bits(tps->regmap, reg, mask, val);
> + mutex_unlock(>lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tps68470_update_bits);

I'm not sure you need those above wrappers at all, regmap is handling
locking in any case.

> +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 tps68470 *tps)
> +{
> + unsigned int version;
> + int ret;
> +
> + ret = tps68470_reg_read(tps, TPS68470_REG_REVID, );
> + if (ret < 0) {
> + dev_err(tps->dev,
> + "Failed to read revision register: %d\n", ret);
> + return ret;
> + }
> +
> + dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version);
> +
> + ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff);
> + if (ret < 0)
> + return ret;
> +
> + /* FIXME: configure these dynamically */
> + /* Enable Daisy Chain LDO and configure relevant GPIOs as output */
> + ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2);
> + if (ret < 0)
> + return ret;
> +
> + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2);
> + if (ret < 0)
> + return ret;
> +
> + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2);
> + if (ret < 0)
> + return ret;
> +
> + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL6A, 2);
> + if (ret < 0)
> + return ret;
> +
> + /*
> +  * When SDA and SCL are routed to GPIO1 and GPIO2, the mode
> +  * for these GPIOs must be configured using their respective
> +  * GPCTLxA registers as inputs with no pull-ups.
> +  */
> + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL1A, 0);
> + if (ret < 0)
> + return ret;
> +
> + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL2A, 0);
> + if (ret < 0)
> + return ret;
> +
> + /* Enable daisy chain */
> + ret = tps68470_update_bits(tps, TPS68470_REG_S_I2C_CTL, 1, 1);
> + if (ret < 0)
> + return ret;
> +
> + usleep_range(TPS68470_DAISY_CHAIN_DELAY_US,
> + TPS68470_DAISY_CHAIN_DELAY_US + 10);
> + return 0;
> +}
> +
> +static int tps68470_probe(struct i2c_client *client)
> +{
> + struct tps68470 *tps;
> + int ret;
> +
> + tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
> + if (!tps)
> + return -ENOMEM;
> +
> + mutex_init(>lock);
> + i2c_set_clientdata(client, tps);
> + tps->dev = >dev;
> +
> + tps->regmap = devm_regmap_init_i2c(client, _regmap_config);
> + if (IS_ERR(tps->regmap)) {
> + dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
> + return PTR_ERR(tps->regmap);
> + }
> +
> + ret = mfd_add_devices(tps->dev, -1, tps68470s,
> +   ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> + if (ret < 0) {
> + dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> + return ret;
> + }

devm_mfd_add_devices()?

> + ret = tps68470_chip_init(tps);
> + if (ret < 0) {
> + dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
> + goto fail;
> + }
> +
> + return 0;
> +fail:
> + mutex_lock(>lock);

Why do you need to lock here?

> + mfd_remove_devices(tps->dev);
> + mutex_unlock(>lock);
> +
> + return ret;
> +}
> +
> +static int tps68470_remove(struct i2c_client *client)
> +{
> +

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

2017-06-06 Thread Heikki Krogerus 
Hi Rajmohan,

On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote:
> +/*
> + * tps68470_reg_read: Read a single tps68470 register.
> + *
> + * @tps: Device to read from.
> + * @reg: Register to read.
> + * @val: Contains the value
> + */
> +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg,
> + unsigned int *val)
> +{
> + int ret;
> +
> + mutex_lock(>lock);
> + ret = regmap_read(tps->regmap, reg, val);
> + mutex_unlock(>lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tps68470_reg_read);
> +
> +/*
> + * tps68470_reg_write: Write a single tps68470 register.
> + *
> + * @tps68470: Device to write to.
> + * @reg: Register to write to.
> + * @val: Value to write.
> + */
> +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg,
> + unsigned int val)
> +{
> + int ret;
> +
> + mutex_lock(>lock);
> + ret = regmap_write(tps->regmap, reg, val);
> + mutex_unlock(>lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tps68470_reg_write);
> +
> +/*
> + * tps68470_update_bits: Modify bits w.r.t mask and val.
> + *
> + * @tps68470: Device to write to.
> + * @reg: Register to read-write to.
> + * @mask: Mask.
> + * @val: Value to write.
> + */
> +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + int ret;
> +
> + mutex_lock(>lock);
> + ret = regmap_update_bits(tps->regmap, reg, mask, val);
> + mutex_unlock(>lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tps68470_update_bits);

I'm not sure you need those above wrappers at all, regmap is handling
locking in any case.

> +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 tps68470 *tps)
> +{
> + unsigned int version;
> + int ret;
> +
> + ret = tps68470_reg_read(tps, TPS68470_REG_REVID, );
> + if (ret < 0) {
> + dev_err(tps->dev,
> + "Failed to read revision register: %d\n", ret);
> + return ret;
> + }
> +
> + dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version);
> +
> + ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff);
> + if (ret < 0)
> + return ret;
> +
> + /* FIXME: configure these dynamically */
> + /* Enable Daisy Chain LDO and configure relevant GPIOs as output */
> + ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2);
> + if (ret < 0)
> + return ret;
> +
> + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2);
> + if (ret < 0)
> + return ret;
> +
> + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2);
> + if (ret < 0)
> + return ret;
> +
> + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL6A, 2);
> + if (ret < 0)
> + return ret;
> +
> + /*
> +  * When SDA and SCL are routed to GPIO1 and GPIO2, the mode
> +  * for these GPIOs must be configured using their respective
> +  * GPCTLxA registers as inputs with no pull-ups.
> +  */
> + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL1A, 0);
> + if (ret < 0)
> + return ret;
> +
> + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL2A, 0);
> + if (ret < 0)
> + return ret;
> +
> + /* Enable daisy chain */
> + ret = tps68470_update_bits(tps, TPS68470_REG_S_I2C_CTL, 1, 1);
> + if (ret < 0)
> + return ret;
> +
> + usleep_range(TPS68470_DAISY_CHAIN_DELAY_US,
> + TPS68470_DAISY_CHAIN_DELAY_US + 10);
> + return 0;
> +}
> +
> +static int tps68470_probe(struct i2c_client *client)
> +{
> + struct tps68470 *tps;
> + int ret;
> +
> + tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
> + if (!tps)
> + return -ENOMEM;
> +
> + mutex_init(>lock);
> + i2c_set_clientdata(client, tps);
> + tps->dev = >dev;
> +
> + tps->regmap = devm_regmap_init_i2c(client, _regmap_config);
> + if (IS_ERR(tps->regmap)) {
> + dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
> + return PTR_ERR(tps->regmap);
> + }
> +
> + ret = mfd_add_devices(tps->dev, -1, tps68470s,
> +   ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> + if (ret < 0) {
> + dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> + return ret;
> + }

devm_mfd_add_devices()?

> + ret = tps68470_chip_init(tps);
> + if (ret < 0) {
> + dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
> + goto fail;
> + }
> +
> + return 0;
> +fail:
> + mutex_lock(>lock);

Why do you need to lock here?

> + mfd_remove_devices(tps->dev);
> + mutex_unlock(>lock);
> +
> + return ret;
> +}
> +
> +static int tps68470_remove(struct i2c_client *client)
> +{
> +