Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752487AbdFLJMH (ORCPT ); Mon, 12 Jun 2017 05:12:07 -0400 Received: from mga07.intel.com ([134.134.136.100]:46272 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752453AbdFLJMF (ORCPT ); Mon, 12 Jun 2017 05:12:05 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,333,1493708400"; d="scan'208";a="1181277022" From: "Mani, Rajmohan" To: Andy Shevchenko 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" Subject: RE: [PATCH v2 1/3] mfd: Add new mfd device TPS68470 Thread-Topic: [PATCH v2 1/3] mfd: Add new mfd device TPS68470 Thread-Index: AQHS4npbRSIEEY7JwEGO0xJc3w4tcaIgGocAgADUxjA= Date: Mon, 12 Jun 2017 09:12:03 +0000 Message-ID: <6F87890CF0F5204F892DEA1EF0D77A59725BF43C@FMSMSX114.amr.corp.intel.com> References: <1497161395-36504-1-git-send-email-rajmohan.mani@intel.com> <1497161395-36504-2-git-send-email-rajmohan.mani@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.1.200.108] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v5C9CHF1029012 Content-Length: 5397 Lines: 181 Hi Andy, Thanks for the reviews. > Subject: Re: [PATCH v2 1/3] mfd: Add new mfd device TPS68470 > > On Sun, Jun 11, 2017 at 9:09 AM, 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. > > Thanks! This looks much better, though see my few comments below. > > > +/* > > + * This lookup table for the TPS68470 GPIOs, lists > > + * the 7 GPIOs (that can be configured as input or output > > + * as appropriate) and 3 special purpose GPIOs that are > > + * "output only". Exporting these GPIOs in a system mounted > > + * with the TPS68470, in conjunction with the gpio-tps68470 > > + * driver, allows the platform firmware to configure these > > + * GPIOs appropriately, through the ACPI operation region. > > + * These 7 configurable GPIOs can be connected to power rails, > > + * sensor control (e.g sensor reset), while the 3 GPIOs can > > + * be used for sensor control. > > + */ > > > +struct gpiod_lookup_table gpios_table = { > > + .dev_id = NULL, > > Why dev_id is NULL? > I have removed the GPIO lookup tables in the driver. > > + .table = { > > + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH), > > + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH), > > + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH), > > + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH), > > + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH), > > + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH), > > + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH), > > + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable", > GPIO_ACTIVE_HIGH), > > + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH), > > + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn", > GPIO_ACTIVE_HIGH), > > + {}, > > + }, > > +}; > > I don't remember if I asked already why this table exists at all in the driver. > Shouldn't it be provided by ACPI _DSD? > Ack. I have removed the GPIO lookup tables in the driver. > > +static int tps68470_chip_init(struct device *dev, struct regmap > > +*regmap) { > > + unsigned int version; > > + int ret; > > + > > + ret = regmap_read(regmap, TPS68470_REG_REVID, &version); > > + if (ret < 0) { > > + dev_err(dev, "Failed to read revision register: %d\n", ret); > > + return ret; > > + } > > + > > > + dev_info(dev, "TPS68470 REVID: 0x%x\n", version); > > This will confuse user when probe fails. Should be printed only when we return > 0 for sure. > Ack. > > + ret = regmap_write(regmap, TPS68470_REG_RESET, 0xff); > > + if (ret < 0) > > + return ret; > > + > > > + /* FIXME: configure these dynamically */ > > Please, either fix or remove this comment. > Will keep this comment, until I see how this can be fixed. > > + /* Enable daisy chain */ > > + ret = regmap_update_bits(regmap, 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); > > This might require a comment, though I'm fine with it as long as it close to > previous excerpt. > Ack. Added comment. > > + return 0; > > +} > > > +static int tps68470_probe(struct i2c_client *client) { > > + struct device *dev = &client->dev; > > + struct regmap *regmap; > > + int ret; > > + > > + regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config); > > + if (IS_ERR(regmap)) { > > > + dev_err(dev, "devm_regmap_init_i2c Error %ld\n", > > + PTR_ERR(regmap)); > > 1. Indentation. Ack > 2. Do we really need this message? > Since the driver probe fails, it would be useful to know more info on that. > > + return PTR_ERR(regmap); > > + } > > + > > + i2c_set_clientdata(client, regmap); > > + > > + gpiod_add_lookup_table(&gpios_table); > > + > > > + ret = devm_mfd_add_devices(dev, -1, tps68470s, > > + ARRAY_SIZE(tps68470s), NULL, 0, NULL); > > -1 has a definition for such case, use it instead. > Ack > > + if (ret < 0) { > > + dev_err(dev, "mfd_add_devices failed: %d\n", ret); > > + return ret; > > + } > > > +static const struct i2c_device_id tps68470_id_table[] = { > > + {}, > > +}; > > + > > +MODULE_DEVICE_TABLE(i2c, tps68470_id_table); > > Either choose ->probe() over ->probe_new() or remove above. > Ack. Chose the former. > > +static struct i2c_driver tps68470_driver = { > > + .driver = { > > + .name = "tps68470", > > > + .acpi_match_table = ACPI_PTR(tps68470_acpi_ids), > > ACPI_PTR() is redundant. > Ack > > +#include > > And this is for...? > use by tps68470.c which uses i2c_driver and other such i2c defines.