lkml.org 
[lkml]   [2017]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
Date
Hi Andy,

Thanks for the reviews and patience.

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Tuesday, June 06, 2017 6:00 AM
> To: Mani, Rajmohan <rajmohan.mani@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux-
> acpi@vger.kernel.org; Lee Jones <lee.jones@linaro.org>; Linus Walleij
> <linus.walleij@linaro.org>; Alexandre Courbot <gnurou@gmail.com>; Rafael J.
> Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@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.mani@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(&client->dev, sizeof(*tps), GFP_KERNEL);
> > + if (!tps)
> > + return -ENOMEM;
> > +
> > + mutex_init(&tps->lock);
> > + i2c_set_clientdata(client, tps);
> > + tps->dev = &client->dev;
> > +
> > + tps->regmap = devm_regmap_init_i2c(client,
> &tps68470_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(&tps->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(&tps->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(&tps->lock);
> > + mfd_remove_devices(tps->dev);
> > + mutex_unlock(&tps->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
\
 
 \ /
  Last update: 2017-06-12 01:53    [W:0.177 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site