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
