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.
> +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(&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_?
> + 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.
> + mfd_remove_devices(tps->dev);
> + mutex_unlock(&tps->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(&tps->lock);
> + mfd_remove_devices(tps->dev);
> + mutex_unlock(&tps->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
--
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
