From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751456AbdFFM7x (ORCPT ); Tue, 6 Jun 2017 08:59:53 -0400 Received: from mail-qt0-f195.google.com ([209.85.216.195]:36680 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751305AbdFFM7u (ORCPT ); Tue, 6 Jun 2017 08:59:50 -0400 MIME-Version: 1.0 In-Reply-To: <1496750118-5570-2-git-send-email-rajmohan.mani@intel.com> References: <1496750118-5570-1-git-send-email-rajmohan.mani@intel.com> <1496750118-5570-2-git-send-email-rajmohan.mani@intel.com> From: Andy Shevchenko Date: Tue, 6 Jun 2017 15:59:49 +0300 Message-ID: Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470 To: Rajmohan Mani Cc: "linux-kernel@vger.kernel.org" , "linux-gpio@vger.kernel.org" , "linux-acpi@vger.kernel.org" , Lee Jones , Linus Walleij , Alexandre Courbot , "Rafael J. Wysocki" , Len Brown Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 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