Hi Heikki, Thanks for the reviews and patience. > -----Original Message----- > From: Heikki Krogerus [mailto:heikki.krogerus@xxxxxxxxxxxxxxx] > Sent: Tuesday, June 06, 2017 5:49 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 > > 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(&tps->lock); > > + ret = regmap_read(tps->regmap, reg, val); > > + mutex_unlock(&tps->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(&tps->lock); > > + ret = regmap_write(tps->regmap, reg, val); > > + mutex_unlock(&tps->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(&tps->lock); > > + ret = regmap_update_bits(tps->regmap, reg, mask, val); > > + mutex_unlock(&tps->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, &version); > > + 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(&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); > > + if (ret < 0) { > > + dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret); > > + return ret; > > + } > > devm_mfd_add_devices()? > Ack > > + 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); > > Why do you need to lock here? > Same as explained above (to address Alan's comments) > > + mfd_remove_devices(tps->dev); > > + mutex_unlock(&tps->lock); > > + > > + return ret; > > +} > > + > > +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); > > + > > + return 0; > > +} > > + > > +static const struct acpi_device_id tps68470_acpi_ids[] = { > > + {"INT3472"}, > > + {}, > > +}; > > + > > +MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids); > > + > > +static struct i2c_driver tps68470_driver = { > > + .driver = { > > + .name = "tps68470", > > + .acpi_match_table = ACPI_PTR(tps68470_acpi_ids), > > + }, > > + .probe_new = tps68470_probe, > > + .remove = tps68470_remove, > > +}; > > <snip> > > > +/** > > + * struct tps68470 - tps68470 sub-driver chip access routines > > + * > > + * 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 > > + */ > > + struct mutex lock; > > Is this lock really necessary at all? Actually, you probable don't even need this > structure at all if you just rely on regmap functions in the drivers. > Ack I am looking into this and will get back with v2. > > Thanks, > > -- > heikki -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
