| Message ID | 1500506426-3047-2-git-send-email-rajmohan.mani@intel.com |
|---|---|
| State | New |
| Headers | show |
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 <rajmohan.mani@intel.com> > --- > 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) > + 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 ','. > + designware-i2c driver to be builtin, for the same reason. built-in > + > 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. > + * Copyright (C) 2017 Intel Corporation '\n' here. > + * Authors: > + * Rajmohan Mani <rajmohan.mani@intel.com> > + * Tianshu Qiu <tian.shu.qiu@intel.com> > + * Jian Xu Zheng <jian.xu.zheng@intel.com> > + * Yuning Pu <yuning.pu@intel.com> Tab these out: * Authors: * Rajmohan Mani <rajmohan.mani@intel.com> * Tianshu Qiu <tian.shu.qiu@intel.com> * Jian Xu Zheng <jian.xu.zheng@intel.com> * Yuning Pu <yuning.pu@intel.com> > + * 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? > + */ > + > +#include <linux/acpi.h> > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/tps68470.h> > +#include <linux/regmap.h> > + > +static const struct mfd_cell tps68470s[] = { > + { > + .name = "tps68470-gpio", > + }, > + { > + .name = "tps68470_pmic_opregion", > + }, > +}; Make these one line each: { .name = "tps68470-gpio" } ... and drop the ',' > +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? > + return ret; > + > + ret = regmap_read(regmap, TPS68470_REG_REVID, &version); > + if (ret < 0) { Same > + 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 > + 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. > +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? > + */ > + > +#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 */
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 <rajmohan.mani@intel.com> > Cc: linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux- > acpi@vger.kernel.org; Linus Walleij <linus.walleij@linaro.org>; Alexandre > Courbot <gnurou@gmail.com>; Rafael J. Wysocki <rjw@rjwysocki.net>; Len > Brown <lenb@kernel.org>; sakari.ailus@linux.intel.com; Andy Shevchenko > <andy.shevchenko@gmail.com> > 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 <rajmohan.mani@intel.com> > > --- > > 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 <rajmohan.mani@intel.com> > > + * Tianshu Qiu <tian.shu.qiu@intel.com> > > + * Jian Xu Zheng <jian.xu.zheng@intel.com> > > + * Yuning Pu <yuning.pu@intel.com> > > Tab these out: > > * Authors: > * Rajmohan Mani <rajmohan.mani@intel.com> > * Tianshu Qiu <tian.shu.qiu@intel.com> > * Jian Xu Zheng <jian.xu.zheng@intel.com> > * Yuning Pu <yuning.pu@intel.com> > 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 <linux/acpi.h> > > +#include <linux/delay.h> > > +#include <linux/i2c.h> > > +#include <linux/init.h> > > +#include <linux/mfd/core.h> > > +#include <linux/mfd/tps68470.h> > > +#include <linux/regmap.h> > > + > > +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
Hi Lee, > > > + * 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. > I just confirmed that we are recommended to use the above long version of headers. So, these headers will stay as such. Thanks Raj
On Fri, 21 Jul 2017, Mani, Rajmohan wrote: > > 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 <rajmohan.mani@intel.com> > > > --- > > > 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 [...] > > > +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. Yes, 'if (!ret)' does that too. > > > + 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. As above. > > > + 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. As above. [...]
On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones <lee.jones@linaro.org> wrote: > On Fri, 21 Jul 2017, Mani, Rajmohan wrote: >> > On Wed, 19 Jul 2017, Rajmohan Mani wrote: >> > > + /* 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. > > Yes, 'if (!ret)' does that too. Did you mean if (ret) return ret; ? I briefly checked few ->read() and ->write() implementations and didn't find any evidence of positive numbers that can be returned. Documentation (kernel doc) doesn't shed a light on that. So, to me it sounds unspecified. So, for now (until documentation will be fixed) I would rely on if (ret < 0)
Hi Lee, > Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 > > On Fri, 21 Jul 2017, Mani, Rajmohan wrote: > > > 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 <rajmohan.mani@intel.com> > > > > --- > > > > 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 > > [...] > > > > > +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. > > Yes, 'if (!ret)' does that too. > regmap_write() and regmap_read() functions return 0 on success. Hence we can not use 'if (!ret)' here, since we check for error conditions. > > > > + 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. > > As above. > Same as above > > > > + 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. > > As above. > Same as above
On Tue, 25 Jul 2017, Mani, Rajmohan wrote: > Hi Lee, > > > Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 > > > > On Fri, 21 Jul 2017, Mani, Rajmohan wrote: > > > > 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 <rajmohan.mani@intel.com> > > > > > --- > > > > > 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 > > > > [...] > > > > > > > +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. > > > > Yes, 'if (!ret)' does that too. > > > > regmap_write() and regmap_read() functions return 0 on success. Hence we can not use 'if (!ret)' here, since we check for error conditions. Sorry, this was a typo. It should be 'if (ret)'. What I'm trying to get at is the " < 0" is not required. > > > > > + 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. > > > > As above. > > > > Same as above > > > > > > + 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. > > > > As above. > > > > Same as above
On Tue, 25 Jul 2017, Andy Shevchenko wrote: > On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones <lee.jones@linaro.org> wrote: > > On Fri, 21 Jul 2017, Mani, Rajmohan wrote: > >> > On Wed, 19 Jul 2017, Rajmohan Mani wrote: > > >> > > + /* 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. > > > > Yes, 'if (!ret)' does that too. > > Did you mean > > if (ret) > return ret; > > ? Yes, I just noticed! Apologies for the confusion. > I briefly checked few ->read() and ->write() implementations and > didn't find any evidence of positive numbers that can be returned. > Documentation (kernel doc) doesn't shed a light on that. So, to me it > sounds unspecified. > > So, for now (until documentation will be fixed) I would rely on > if (ret < 0) It's not unspecified. The regmap methods call into regcache_write(), where the kerneldoc is clear. Perhaps we can also change the regmap kerneldoc headers too to match, which might lessen the disparity.
On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones <lee.jones@linaro.org> wrote: > On Tue, 25 Jul 2017, Andy Shevchenko wrote: >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones <lee.jones@linaro.org> wrote: >> I briefly checked few ->read() and ->write() implementations and >> didn't find any evidence of positive numbers that can be returned. >> Documentation (kernel doc) doesn't shed a light on that. So, to me it >> sounds unspecified. >> >> So, for now (until documentation will be fixed) I would rely on >> if (ret < 0) > > It's not unspecified. The regmap methods call into regcache_write(), > where the kerneldoc is clear. drivers/base/regmap/regcache.c:266 " * Return a negative value on failure, 0 on success." I can hardly find this any cleaner than "unspecified". > Perhaps we can also change the regmap > kerneldoc headers too to match, which might lessen the disparity. I'm not familiar with the guts of regmap API, so, I would stick with if (ret < 0) for now until documentation specifies positive return values.
Hi Lee, Andy, > Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 > > On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones <lee.jones@linaro.org> wrote: > > On Tue, 25 Jul 2017, Andy Shevchenko wrote: > >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones <lee.jones@linaro.org> wrote: > > >> I briefly checked few ->read() and ->write() implementations and > >> didn't find any evidence of positive numbers that can be returned. > >> Documentation (kernel doc) doesn't shed a light on that. So, to me it > >> sounds unspecified. > >> > >> So, for now (until documentation will be fixed) I would rely on if > >> (ret < 0) > > > > It's not unspecified. The regmap methods call into regcache_write(), > > where the kerneldoc is clear. > Since, we are interested in the regmap for the I2C bus here, I looked into the implementation of __devm_regmap_init() __regmap_init() regcache_init() for I2C bus. At the end of __devm_regmap_init() call from devm_regmap_init_i2c() inside tps68470_probe(), I see that both cache_bypass and defer_caching flags of i2c regmap struct are set. So, it looks regcache_write/read calls do not come into play here. So, regmap_write() _regmap_write() map->reg_write (drivers/base/regmap/regmap.c:1665) translates to regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128) These checks in regmap_i2c_write() ensure all return values from i2c_master_send() other than the requested number of bytes to write, are converted into negative values. if (ret == count) return 0; else if (ret < 0) return ret; else return -EIO; Similar argument goes for regmap_read() as well. With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to be a better choice. Please see if I missed anything here. > drivers/base/regmap/regcache.c:266 > > " * Return a negative value on failure, 0 on success." > > I can hardly find this any cleaner than "unspecified". > > > Perhaps we can also change the regmap kerneldoc headers too to match, > > which might lessen the disparity. > > I'm not familiar with the guts of regmap API, so, I would stick with if (ret < 0) > for now until documentation specifies positive return values. > Ack
On Fri, Jul 28, 2017 at 3:30 AM, Mani, Rajmohan <rajmohan.mani@intel.com> wrote: >> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones <lee.jones@linaro.org> wrote: >> > On Tue, 25 Jul 2017, Andy Shevchenko wrote: >> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones <lee.jones@linaro.org> wrote: >> >> >> I briefly checked few ->read() and ->write() implementations and >> >> didn't find any evidence of positive numbers that can be returned. >> >> Documentation (kernel doc) doesn't shed a light on that. So, to me it >> >> sounds unspecified. >> >> >> >> So, for now (until documentation will be fixed) I would rely on if >> >> (ret < 0) >> > >> > It's not unspecified. The regmap methods call into regcache_write(), >> > where the kerneldoc is clear. >> > > Since, we are interested in the regmap for the I2C bus here, I looked into the implementation of > __devm_regmap_init() > __regmap_init() > regcache_init() > for I2C bus. > > At the end of __devm_regmap_init() call from devm_regmap_init_i2c() inside tps68470_probe(), I see that both cache_bypass and defer_caching flags of i2c regmap struct are set. So, it looks regcache_write/read calls do not come into play here. > > So, regmap_write() > _regmap_write() > map->reg_write (drivers/base/regmap/regmap.c:1665) translates to > regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128) > > These checks in regmap_i2c_write() ensure all return values from i2c_master_send() other than the requested number of bytes to write, are converted into negative values. > > if (ret == count) > return 0; > else if (ret < 0) > return ret; > else > return -EIO; > > Similar argument goes for regmap_read() as well. > With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to be a better choice. Please see if I missed anything here. It prooves exactly the Lee's point. So, perhaps the best approach is to move to if (ret) return ret; ...if it will be a problem in the future, fix it accordingly.
Hi Andy, > Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 > > On Fri, Jul 28, 2017 at 3:30 AM, Mani, Rajmohan > <rajmohan.mani@intel.com> wrote: > >> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones <lee.jones@linaro.org> > wrote: > >> > On Tue, 25 Jul 2017, Andy Shevchenko wrote: > >> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones <lee.jones@linaro.org> > wrote: > >> > >> >> I briefly checked few ->read() and ->write() implementations and > >> >> didn't find any evidence of positive numbers that can be returned. > >> >> Documentation (kernel doc) doesn't shed a light on that. So, to me > >> >> it sounds unspecified. > >> >> > >> >> So, for now (until documentation will be fixed) I would rely on if > >> >> (ret < 0) > >> > > >> > It's not unspecified. The regmap methods call into > >> > regcache_write(), where the kerneldoc is clear. > >> > > > > Since, we are interested in the regmap for the I2C bus here, I looked > > into the implementation of > > __devm_regmap_init() > > __regmap_init() > > regcache_init() > > for I2C bus. > > > > At the end of __devm_regmap_init() call from devm_regmap_init_i2c() > inside tps68470_probe(), I see that both cache_bypass and defer_caching > flags of i2c regmap struct are set. So, it looks regcache_write/read calls do > not come into play here. > > > > So, regmap_write() > > _regmap_write() > > map->reg_write (drivers/base/regmap/regmap.c:1665) translates > to > > regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128) > > > > These checks in regmap_i2c_write() ensure all return values from > i2c_master_send() other than the requested number of bytes to write, are > converted into negative values. > > > > if (ret == count) > > return 0; > > else if (ret < 0) > > return ret; > > else > > return -EIO; > > > > Similar argument goes for regmap_read() as well. > > With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to be a > better choice. Please see if I missed anything here. > > It prooves exactly the Lee's point. > > So, perhaps the best approach is to move to if (ret) return ret; > > ...if it will be a problem in the future, fix it accordingly. > Ack. We have spent enough time on this already.
Hi Lee, > Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 > > On Tue, 25 Jul 2017, Mani, Rajmohan wrote: > > > Hi Lee, > > > > > Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 > > > > > > On Fri, 21 Jul 2017, Mani, Rajmohan wrote: > > > > > 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 <rajmohan.mani@intel.com> > > > > > > --- > > > > > > 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 > > > > > > [...] > > > > > > > > > +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. > > > > > > Yes, 'if (!ret)' does that too. > > > > > > > regmap_write() and regmap_read() functions return 0 on success. Hence > we can not use 'if (!ret)' here, since we check for error conditions. > > Sorry, this was a typo. > > It should be 'if (ret)'. > > What I'm trying to get at is the " < 0" is not required. > Ack > > > > > > + 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. > > > > > > As above. > > > > > > > Same as above > > > > > > > > + 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. > > > > > > As above. > > > > > > > Same as above > > -- > Lee Jones > Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source > software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
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 + 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 + designware-i2c driver to be builtin, for the same reason. + 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 + * + * Copyright (C) 2017 Intel Corporation + * Authors: + * Rajmohan Mani <rajmohan.mani@intel.com> + * Tianshu Qiu <tian.shu.qiu@intel.com> + * Jian Xu Zheng <jian.xu.zheng@intel.com> + * Yuning Pu <yuning.pu@intel.com> + * + * 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. + */ + +#include <linux/acpi.h> +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/mfd/core.h> +#include <linux/mfd/tps68470.h> +#include <linux/regmap.h> + +static const struct mfd_cell tps68470s[] = { + { + .name = "tps68470-gpio", + }, + { + .name = "tps68470_pmic_opregion", + }, +}; + +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) + return 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); + + 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) { + 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"}, + {}, +}; + +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. + */ + +#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 */
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 <rajmohan.mani@intel.com> --- 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