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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

Thanks for the reviews and patience.

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@xxxxxxxxx]
> Sent: Tuesday, June 06, 2017 6:00 AM
> To: Mani, Rajmohan <rajmohan.mani@xxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; linux-
> acpi@xxxxxxxxxxxxxxx; Lee Jones <lee.jones@xxxxxxxxxx>; Linus Walleij
> <linus.walleij@xxxxxxxxxx>; Alexandre Courbot <gnurou@xxxxxxxxx>; Rafael J.
> Wysocki <rjw@xxxxxxxxxxxxx>; Len Brown <lenb@xxxxxxxxxx>
> 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@xxxxxxxxx>
> 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
��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux