Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1692266 > unrolled thread

[PATCH v4 0/3] TPS68470 PMIC drivers

Started byRajmohan Mani <rajmohan.mani@intel.com>
First post2017-07-20 01:30 +0200
Last post2017-07-21 14:30 +0200
Articles 15 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/3] TPS68470 PMIC drivers Rajmohan Mani <rajmohan.mani@intel.com> - 2017-07-20 01:30 +0200
    [PATCH v4 1/3] mfd: Add new mfd device TPS68470 Rajmohan Mani <rajmohan.mani@intel.com> - 2017-07-20 01:30 +0200
      Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 Lee Jones <lee.jones@linaro.org> - 2017-07-20 11:10 +0200
        RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-07-21 03:00 +0200
          Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 Lee Jones <lee.jones@linaro.org> - 2017-07-25 11:20 +0200
            Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-25 12:30 +0200
              Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 Lee Jones <lee.jones@linaro.org> - 2017-07-26 10:30 +0200
                Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-26 12:30 +0200
                  RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-07-28 02:40 +0200
                    Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-28 11:20 +0200
                      RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-07-29 01:50 +0200
            RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-07-25 19:10 +0200
              Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 Lee Jones <lee.jones@linaro.org> - 2017-07-26 10:30 +0200
                RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-07-29 01:50 +0200
        RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470 "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-07-21 14:30 +0200

#1692266 — [PATCH v4 0/3] TPS68470 PMIC drivers

FromRajmohan Mani <rajmohan.mani@intel.com>
Date2017-07-20 01:30 +0200
Subject[PATCH v4 0/3] TPS68470 PMIC drivers
Message-ID<u56zL-7bx-5@gated-at.bofh.it>
This is the patch series for TPS68470 PMIC that works as a camera PMIC.

The patch series provide the following 3 drivers, to help configure the voltage regulators, clocks and GPIOs provided by the TPS68470 PMIC, to be able to use the camera sensors connected to this PMIC.

TPS68470 MFD driver:
This is the multi function driver that initializes the TPS68470 PMIC and supports the GPIO and Op Region functions.

TPS68470 GPIO driver:
This is the PMIC GPIO driver that will be used by the OS GPIO layer, when the BIOS / firmware triggered GPIO access is done.

TPS68470 Op Region driver:
This is the driver that will be invoked, when the BIOS / firmware configures the voltage / clock for the sensors / vcm devices connected to the PMIC.

---

Update on 2 GPIO chips implementation over 1:
	- Attempted to implement 2 GPIO chips, but ran into couple of
	  issues in the kernel, so we couldn't get it to work.
	- It was decided to postpone this change, since it is not
	  critical
	  
Changes in v4:
	- MFD driver:
	- Removed board specific code and FIXME comment
	- Moved i2c.h include from tps68470.h to tps68470.c
	- Moved the TPS68470 REVID read code after PMIC reset
	- Fixed typo in debug error message (on failure of
	  devm_mfd_add_devices() )
	- Enhanced dependency on I2C by changing it to I2C=y
	  (to fix build errors if I2C is built as module
	   e.g tps68470.c:71: undefined reference to `__devm_regmap_init_i2c'
	       tps68470.c:117: undefined reference to `i2c_register_driver')
	- Removed most of the unused header file definitions
	- Moved devm_mfd_add_devices() after PMIC resett
	- Used probe_new() and removed i2c_device_id table
	  
	  The following patch from Andy is needed for the driver to be
	  probed.
	  http://marc.info/?l=linux-acpi&m=150030081523885&w=2	
	  
	- GPIO driver:
	- Added newline at the end of Kconfig description
	- Updated commit message about the descriptive
	  names for the GPIOs and the typical usage model
	  of the GPIO driver

	- Opregion driver:
	- Added dependency on MFD_TPS68470
	- Converted 2 liner into one line code

Changes in v3:
	- MFD driver:
	- Removed GPIO lookup table
	- Reverted to probe() for consistency
	- Addressed other comments from Andy

	- GPIO driver:
	- Removed the code that initializes the default values
	  of GPIOs to zeros
	- Used gpiochip_get_data() to access data inside the gpio_chip
 
Changes in v2:
	- MFD driver:
	- Removed tps68470_* wrappers around regmap_* calls
	- Removed "struct tps68470"
	- used devm_mfd_add_devices and removed mutex in mfd driver
	- Added reasoning about the need of having mfd driver
	  as bool/builtin

	- Opregion driver:
	- renamed opregion driver file / internal symbol names
	  with tps68470_pmic*
	- Made opregion driver tables as const
	- Removed unused *handler_context in common handler
	- Replaced "int" with "unsigned int"
	- Changed to WARN macro to dev_warn()
	- Destroyed mutex on error
	- Added reasoning about the need of having Opregion driver
	  as bool/builtin
	
	- GPIO driver:
	- Implemented get_direction() in the GPIO driver
	- Setup gpio_chip.names
	- Moved the GPIO lookup table code inside mfd driver
	- Added reasoning about the need of having GPIO driver
	  as bool/builtin

---

Rajmohan Mani (3):
  mfd: Add new mfd device TPS68470
  gpio: Add support for TPS68470 GPIOs
  ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

 drivers/acpi/Kconfig              |  16 ++
 drivers/acpi/Makefile             |   2 +
 drivers/acpi/pmic/tps68470_pmic.c | 455 ++++++++++++++++++++++++++++++++++++++
 drivers/gpio/Kconfig              |  15 ++
 drivers/gpio/Makefile             |   1 +
 drivers/gpio/gpio-tps68470.c      | 174 +++++++++++++++
 drivers/mfd/Kconfig               |  18 ++
 drivers/mfd/Makefile              |   1 +
 drivers/mfd/tps68470.c            | 110 +++++++++
 include/linux/mfd/tps68470.h      |  97 ++++++++
 10 files changed, 889 insertions(+)
 create mode 100644 drivers/acpi/pmic/tps68470_pmic.c
 create mode 100644 drivers/gpio/gpio-tps68470.c
 create mode 100644 drivers/mfd/tps68470.c
 create mode 100644 include/linux/mfd/tps68470.h

-- 
1.9.1

[toc] | [next] | [standalone]

#1692267 — [PATCH v4 1/3] mfd: Add new mfd device TPS68470

FromRajmohan Mani <rajmohan.mani@intel.com>
Date2017-07-20 01:30 +0200
Subject[PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u56zL-7bx-13@gated-at.bofh.it>
In reply to#1692266
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
+	  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 */
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1692645 — Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

FromLee Jones <lee.jones@linaro.org>
Date2017-07-20 11:10 +0200
SubjectRe: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u5fD5-5dz-35@gated-at.bofh.it>
In reply to#1692267
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 */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1693316 — RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-07-21 03:00 +0200
SubjectRE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u5usq-6hA-3@gated-at.bofh.it>
In reply to#1692645
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

[toc] | [prev] | [next] | [standalone]

#1695529 — Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

FromLee Jones <lee.jones@linaro.org>
Date2017-07-25 11:20 +0200
SubjectRe: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u74av-VK-25@gated-at.bofh.it>
In reply to#1693316
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.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1695588 — Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-07-25 12:30 +0200
SubjectRe: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u75gf-1yO-61@gated-at.bofh.it>
In reply to#1695529
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)


-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1696868 — Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

FromLee Jones <lee.jones@linaro.org>
Date2017-07-26 10:30 +0200
SubjectRe: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u7pRE-6lO-3@gated-at.bofh.it>
In reply to#1695588
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.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1696960 — Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-07-26 12:30 +0200
SubjectRe: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u7rJL-7vZ-5@gated-at.bofh.it>
In reply to#1696868
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.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1698387 — RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-07-28 02:40 +0200
SubjectRE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u81tU-4QH-7@gated-at.bofh.it>
In reply to#1696960
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

[toc] | [prev] | [next] | [standalone]

#1698610 — Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-07-28 11:20 +0200
SubjectRe: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u89B7-1Pt-1@gated-at.bofh.it>
In reply to#1698387
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.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1699166 — RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-07-29 01:50 +0200
SubjectRE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u8nb3-2d9-9@gated-at.bofh.it>
In reply to#1698610
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.

[toc] | [prev] | [next] | [standalone]

#1695951 — RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-07-25 19:10 +0200
SubjectRE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u7bvj-5B8-3@gated-at.bofh.it>
In reply to#1695529
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

[toc] | [prev] | [next] | [standalone]

#1696869 — Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

FromLee Jones <lee.jones@linaro.org>
Date2017-07-26 10:30 +0200
SubjectRe: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u7pRE-6lO-5@gated-at.bofh.it>
In reply to#1695951
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

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1699167 — RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-07-29 01:50 +0200
SubjectRE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u8nb3-2d9-11@gated-at.bofh.it>
In reply to#1696869
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

[toc] | [prev] | [next] | [standalone]

#1693661 — RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-07-21 14:30 +0200
SubjectRE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
Message-ID<u5Fe9-4LB-1@gated-at.bofh.it>
In reply to#1692645
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web