Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966217AbdGUA5s (ORCPT ); Thu, 20 Jul 2017 20:57:48 -0400 Received: from mga09.intel.com ([134.134.136.24]:12054 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965629AbdGUA5o (ORCPT ); Thu, 20 Jul 2017 20:57:44 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,386,1496127600"; d="scan'208";a="129971286" From: "Mani, Rajmohan" To: Lee Jones CC: "linux-kernel@vger.kernel.org" , "linux-gpio@vger.kernel.org" , "linux-acpi@vger.kernel.org" , Linus Walleij , Alexandre Courbot , "Rafael J. Wysocki" , Len Brown , "sakari.ailus@linux.intel.com" , "Andy Shevchenko" Subject: RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 Thread-Topic: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 Thread-Index: AQHTAOa9zEd+focor0e+zW969r8dYKJc4YQAgACTgJA= Date: Fri, 21 Jul 2017 00:57:42 +0000 Message-ID: <6F87890CF0F5204F892DEA1EF0D77A59725DD60A@FMSMSX114.amr.corp.intel.com> References: <1500506426-3047-1-git-send-email-rajmohan.mani@intel.com> <1500506426-3047-2-git-send-email-rajmohan.mani@intel.com> <20170720090300.ayx5pz5l4yntefpt@dell> In-Reply-To: <20170720090300.ayx5pz5l4yntefpt@dell> 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.106] 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 nfs id v6L0vu0b001829 Content-Length: 11762 Lines: 395 Hi Lee, Thanks for the reviews. > -----Original Message----- > From: Lee Jones [mailto:lee.jones@linaro.org] > Sent: Thursday, July 20, 2017 2:03 AM > To: Mani, Rajmohan > Cc: linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux- > acpi@vger.kernel.org; Linus Walleij ; Alexandre > Courbot ; Rafael J. Wysocki ; Len > Brown ; sakari.ailus@linux.intel.com; Andy Shevchenko > > Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 > > On Wed, 19 Jul 2017, 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. > > > > Signed-off-by: Rajmohan Mani > > --- > > drivers/mfd/Kconfig | 18 +++++++ > > drivers/mfd/Makefile | 1 + > > drivers/mfd/tps68470.c | 110 > +++++++++++++++++++++++++++++++++++++++++++ > > include/linux/mfd/tps68470.h | 97 > > ++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 226 insertions(+) > > create mode 100644 drivers/mfd/tps68470.c create mode 100644 > > include/linux/mfd/tps68470.h > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index > > 3eb5c93..960be43 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -1311,6 +1311,24 @@ config MFD_TPS65217 > > This driver can also be built as a module. If so, the module > > will be called tps65217. > > > > +config MFD_TPS68470 > > + bool "TI TPS68470 Power Management / LED chips" > > + depends on ACPI && I2C=y > > + select MFD_CORE > > + select REGMAP_I2C > > + select I2C_DESIGNWARE_PLATFORM > > + help > > + If you say yes here you get support for the TPS68470 series of > > + Power Management / LED chips. > > + > > + These include voltage regulators, led and other features > > LED(s) > Ack > > + that are often used in portable devices. > > + > > + This option is a bool as it provides an ACPI operation > > + region, which must be available before any of the devices > > + using this, are probed. This option also configures the > > Remove the ','. > Ack > > + designware-i2c driver to be builtin, for the same reason. > > built-in > Ack > > + > > config MFD_TI_LP873X > > tristate "TI LP873X Power Management IC" > > depends on I2C > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index > > c16bf1e..6dd2b94 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -82,6 +82,7 @@ obj-$(CONFIG_MFD_TPS65910) += tps65910.o > > obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o > > obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o > > obj-$(CONFIG_MFD_TPS65912_SPI) += tps65912-spi.o > > +obj-$(CONFIG_MFD_TPS68470) += tps68470.o > > obj-$(CONFIG_MFD_TPS80031) += tps80031.o > > obj-$(CONFIG_MENELAUS) += menelaus.o > > > > diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c new file > > mode 100644 index 0000000..a692af7 > > --- /dev/null > > +++ b/drivers/mfd/tps68470.c > > @@ -0,0 +1,110 @@ > > +/* > > + * TPS68470 chip family multi-function driver > > Does it really cover a family? If so 'TPS68470' seems very specific. > > "Multi-Function Driver" or even better "Core" or "Parent" driver. > No. This is just for TPS68470. I picked "Parent" driver > > + * Copyright (C) 2017 Intel Corporation > > '\n' here. > Ack > > + * Authors: > > + * Rajmohan Mani > > + * Tianshu Qiu > > + * Jian Xu Zheng > > + * Yuning Pu > > Tab these out: > > * Authors: > * Rajmohan Mani > * Tianshu Qiu > * Jian Xu Zheng > * Yuning Pu > Ack > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation version 2. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied > > + warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > Can you use the short version? > Ack I will update the other patches of this series too. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +static const struct mfd_cell tps68470s[] = { > > + { > > + .name = "tps68470-gpio", > > + }, > > + { > > + .name = "tps68470_pmic_opregion", > > + }, > > +}; > > Make these one line each: > > { .name = "tps68470-gpio" } > > ... and drop the ',' > Ack > > +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 device *dev, struct regmap > > +*regmap) { > > + unsigned int version; > > + int ret; > > + > > + /* Force software reset */ > > + ret = regmap_write(regmap, TPS68470_REG_RESET, > TPS68470_REG_RESET_MASK); > > + if (ret < 0) > > Will 'if (!ret)' do? > We intend to check error conditions and bail out. So, if (ret < 0) works for this. > > + return ret; > > + > > + ret = regmap_read(regmap, TPS68470_REG_REVID, &version); > > + if (ret < 0) { > > Same > We intend to check error conditions and bail out. So, if (ret < 0) works for this. > > + dev_err(dev, "Failed to read revision register: %d\n", ret); > > + return ret; > > + } > > + > > + dev_info(dev, "TPS68470 REVID: 0x%x\n", version); > > + > > + 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)); > > + return PTR_ERR(regmap); > > + } > > + > > + i2c_set_clientdata(client, regmap); > > + > > + ret = tps68470_chip_init(dev, regmap); > > + if (ret < 0) { > > Same > We intend to check error conditions and bail out. So, if (ret < 0) works for this. > > + dev_err(dev, "TPS68470 Init Error %d\n", ret); > > + return ret; > > + } > > + > > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, > tps68470s, > > + ARRAY_SIZE(tps68470s), NULL, 0, NULL); > > + if (ret < 0) { > > + dev_err(dev, "devm_mfd_add_devices failed: %d\n", ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static const struct acpi_device_id tps68470_acpi_ids[] = { > > + {"INT3472"}, > > + {}, > > +}; > > + > > Remove this line. > Ack > > +MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids); > > + > > +static struct i2c_driver tps68470_driver = { > > + .driver = { > > + .name = "tps68470", > > + .acpi_match_table = tps68470_acpi_ids, > > + }, > > + .probe_new = tps68470_probe, > > +}; > > +builtin_i2c_driver(tps68470_driver); > > diff --git a/include/linux/mfd/tps68470.h > > b/include/linux/mfd/tps68470.h new file mode 100644 index > > 0000000..44f9d9f > > --- /dev/null > > +++ b/include/linux/mfd/tps68470.h > > @@ -0,0 +1,97 @@ > > +/* > > + * Copyright (c) 2017 Intel Corporation > > + * > > + * Functions to access TPS68470 power management chip. > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation version 2. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied > > +warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > Short version? > Ack > > + */ > > + > > +#ifndef __LINUX_MFD_TPS68470_H > > +#define __LINUX_MFD_TPS68470_H > > + > > +/* Register addresses */ > > +#define TPS68470_REG_POSTDIV2 0x06 > > +#define TPS68470_REG_BOOSTDIV 0x07 > > +#define TPS68470_REG_BUCKDIV 0x08 > > +#define TPS68470_REG_PLLSWR 0x09 > > +#define TPS68470_REG_XTALDIV 0x0A > > +#define TPS68470_REG_PLLDIV 0x0B > > +#define TPS68470_REG_POSTDIV 0x0C > > +#define TPS68470_REG_PLLCTL 0x0D > > +#define TPS68470_REG_PLLCTL2 0x0E > > +#define TPS68470_REG_CLKCFG1 0x0F > > +#define TPS68470_REG_CLKCFG2 0x10 > > +#define TPS68470_REG_GPCTL0A 0x14 > > +#define TPS68470_REG_GPCTL0B 0x15 > > +#define TPS68470_REG_GPCTL1A 0x16 > > +#define TPS68470_REG_GPCTL1B 0x17 > > +#define TPS68470_REG_GPCTL2A 0x18 > > +#define TPS68470_REG_GPCTL2B 0x19 > > +#define TPS68470_REG_GPCTL3A 0x1A > > +#define TPS68470_REG_GPCTL3B 0x1B > > +#define TPS68470_REG_GPCTL4A 0x1C > > +#define TPS68470_REG_GPCTL4B 0x1D > > +#define TPS68470_REG_GPCTL5A 0x1E > > +#define TPS68470_REG_GPCTL5B 0x1F > > +#define TPS68470_REG_GPCTL6A 0x20 > > +#define TPS68470_REG_GPCTL6B 0x21 > > +#define TPS68470_REG_SGPO 0x22 > > +#define TPS68470_REG_GPDI 0x26 > > +#define TPS68470_REG_GPDO 0x27 > > +#define TPS68470_REG_VCMVAL 0x3C > > +#define TPS68470_REG_VAUX1VAL 0x3D > > +#define TPS68470_REG_VAUX2VAL 0x3E > > +#define TPS68470_REG_VIOVAL 0x3F > > +#define TPS68470_REG_VSIOVAL 0x40 > > +#define TPS68470_REG_VAVAL 0x41 > > +#define TPS68470_REG_VDVAL 0x42 > > +#define TPS68470_REG_S_I2C_CTL 0x43 > > +#define TPS68470_REG_VCMCTL 0x44 > > +#define TPS68470_REG_VAUX1CTL 0x45 > > +#define TPS68470_REG_VAUX2CTL 0x46 > > +#define TPS68470_REG_VACTL 0x47 > > +#define TPS68470_REG_VDCTL 0x48 > > +#define TPS68470_REG_RESET 0x50 > > +#define TPS68470_REG_REVID 0xFF > > + > > +#define TPS68470_REG_MAX TPS68470_REG_REVID > > + > > +/* Register field definitions */ > > + > > +#define TPS68470_REG_RESET_MASK GENMASK(7, 0) > > +#define TPS68470_VAVAL_AVOLT_MASK GENMASK(6, 0) > > + > > +#define TPS68470_VDVAL_DVOLT_MASK GENMASK(5, 0) > > +#define TPS68470_VCMVAL_VCVOLT_MASK GENMASK(6, 0) > > +#define TPS68470_VIOVAL_IOVOLT_MASK GENMASK(6, 0) > > +#define TPS68470_VSIOVAL_IOVOLT_MASK GENMASK(6, 0) > > +#define TPS68470_VAUX1VAL_AUX1VOLT_MASK GENMASK(6, 0) > > +#define TPS68470_VAUX2VAL_AUX2VOLT_MASK GENMASK(6, 0) > > + > > +#define TPS68470_VACTL_EN_MASK GENMASK(0, 0) > > +#define TPS68470_VDCTL_EN_MASK GENMASK(0, 0) > > +#define TPS68470_VCMCTL_EN_MASK GENMASK(0, 0) > > +#define TPS68470_S_I2C_CTL_EN_MASK GENMASK(1, 0) > > +#define TPS68470_VAUX1CTL_EN_MASK GENMASK(0, 0) > > +#define TPS68470_VAUX2CTL_EN_MASK GENMASK(0, 0) > > +#define TPS68470_PLL_EN_MASK GENMASK(0, 0) > > + > > +#define TPS68470_CLKCFG1_MODE_A_MASK GENMASK(1, 0) > > +#define TPS68470_CLKCFG1_MODE_B_MASK GENMASK(3, 2) > > + > > +#define TPS68470_GPIO_CTL_REG_A(x) (TPS68470_REG_GPCTL0A + > (x) * 2) > > +#define TPS68470_GPIO_CTL_REG_B(x) (TPS68470_REG_GPCTL0B + > (x) * 2) > > +#define TPS68470_GPIO_MODE_MASK GENMASK(1, 0) > > +#define TPS68470_GPIO_MODE_IN 0 > > +#define TPS68470_GPIO_MODE_IN_PULLUP 1 > > +#define TPS68470_GPIO_MODE_OUT_CMOS 2 > > +#define TPS68470_GPIO_MODE_OUT_ODRAIN 3 > > + > > +#endif /* __LINUX_MFD_TPS68470_H */ > > -- > Lee Jones > Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source > software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog