[U-Boot] [PATCH 0/6] SMDK5420: Add S2MPS11 pmic support to SMDK5420

[U-Boot] [PATCH 0/6] SMDK5420: Add S2MPS11 pmic support to SMDK5420

[Leela Krishna Amudala]

This patchset adds support for S2MPS11 pmic on SMDK5420

This patchset has dependency on Rajeshwari's base patchset:
[V4] EXYNOS5420: Add SMDK5420 board support
http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/170582

Also, for testing we need Naveen's i2c patchset as well:
i2c: improve s3c24x0 with High-speed and new SYS_I2C framework support
http://www.mail-archive.com/u-boot at lists.denx.de/msg122679.html


Leela Krishna Amudala (6):
  exynos: Use common pmic_reg_update() definition
  power: Explicitly select pmic device's bus
  FDT: Exynos5420: Add compatible srings for PMIC
  SMDK5420: S2MPS11: Adds the register settings for S2MPS11
  exynos: Add a common DT based PMIC driver initialization
  config: SMDK5420: Enable S2MPS11 pmic

 board/samsung/common/board.c     |   40 ++++++-----
 drivers/power/pmic/Makefile      |    1 +
 drivers/power/pmic/pmic_common.c |   97 +++++++++++++++++++++++++
 drivers/power/power_core.c       |   14 ++++
 drivers/power/power_i2c.c        |   81 +++++++++++++++++++--
 include/configs/smdk5420.h       |    4 ++
 include/fdtdec.h                 |    1 +
 include/power/pmic.h             |   35 +++++++++
 include/power/s2mps11_pmic.h     |  144 ++++++++++++++++++++++++++++++++++++++
 lib/fdtdec.c                     |    1 +
 10 files changed, 394 insertions(+), 24 deletions(-)
 create mode 100644 drivers/power/pmic/pmic_common.c
 create mode 100644 include/power/s2mps11_pmic.h

-- 
1.7.10.4

[U-Boot] [PATCH 1/6] exynos: Use common pmic_reg_update() definition

[Leela Krishna Amudala]

This function is used by different Exynos platforms, put it in the
common file.

Signed-off-by: Vadim Bendebury <vbendeb@chromium.org>
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
Reviewed-by: Doug Anderson <dianders@google.com>
---
 board/samsung/common/board.c |   19 -------------------
 drivers/power/power_i2c.c    |   19 +++++++++++++++++++
 include/power/pmic.h         |    1 +
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/board/samsung/common/board.c b/board/samsung/common/board.c
index ce85ddb..87ca9de 100644
--- a/board/samsung/common/board.c
+++ b/board/samsung/common/board.c
@@ -152,25 +152,6 @@ static int board_init_cros_ec_devices(const void *blob)
 
 #if defined(CONFIG_POWER)
 #ifdef CONFIG_POWER_MAX77686
-static int pmic_reg_update(struct pmic *p, int reg, uint regval)
-{
-	u32 val;
-	int ret = 0;
-
-	ret = pmic_reg_read(p, reg, &val);
-	if (ret) {
-		debug("%s: PMIC %d register read failed\n", __func__, reg);
-		return -1;
-	}
-	val |= regval;
-	ret = pmic_reg_write(p, reg, val);
-	if (ret) {
-		debug("%s: PMIC %d register write failed\n", __func__, reg);
-		return -1;
-	}
-	return 0;
-}
-
 static int max77686_init(void)
 {
 	struct pmic *p;
diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
index ac76870..47c606f 100644
--- a/drivers/power/power_i2c.c
+++ b/drivers/power/power_i2c.c
@@ -96,6 +96,25 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
 	return 0;
 }
 
+int pmic_reg_update(struct pmic *p, int reg, uint regval)
+{
+	u32 val;
+	int ret = 0;
+
+	ret = pmic_reg_read(p, reg, &val);
+	if (ret) {
+		debug("%s: PMIC %d register read failed\n", __func__, reg);
+		return -1;
+	}
+	val |= regval;
+	ret = pmic_reg_write(p, reg, val);
+	if (ret) {
+		debug("%s: PMIC %d register write failed\n", __func__, reg);
+		return -1;
+	}
+	return 0;
+}
+
 int pmic_probe(struct pmic *p)
 {
 	i2c_set_bus_num(p->bus);
diff --git a/include/power/pmic.h b/include/power/pmic.h
index 0e7aa31..d17dbdc 100644
--- a/include/power/pmic.h
+++ b/include/power/pmic.h
@@ -83,6 +83,7 @@ int pmic_probe(struct pmic *p);
 int pmic_reg_read(struct pmic *p, u32 reg, u32 *val);
 int pmic_reg_write(struct pmic *p, u32 reg, u32 val);
 int pmic_set_output(struct pmic *p, u32 reg, int ldo, int on);
+int pmic_reg_update(struct pmic *p, int reg, uint regval);
 
 #define pmic_i2c_addr (p->hw.i2c.addr)
 #define pmic_i2c_tx_num (p->hw.i2c.tx_num)
-- 
1.7.10.4

[U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus

[Leela Krishna Amudala]

The current pmic i2c code assumes the current i2c bus is
the same as the pmic device's bus. There is nothing ensuring
that to be true. Therefore, select the proper bus before performing
a transaction.

Signed-off-by: Aaron Durbin <adurbin@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
Reviewed-by: Doug Anderson <dianders@google.com>
---
 drivers/power/power_i2c.c |   62 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
index 47c606f..c22e01f 100644
--- a/drivers/power/power_i2c.c
+++ b/drivers/power/power_i2c.c
@@ -16,9 +16,45 @@
 #include <i2c.h>
 #include <compiler.h>
 
+static int pmic_select(struct pmic *p)
+{
+	int ret, old_bus;
+
+	old_bus = i2c_get_bus_num();
+	if (old_bus != p->bus) {
+		debug("%s: Select bus %d\n", __func__, p->bus);
+		ret = i2c_set_bus_num(p->bus);
+		if (ret) {
+			debug("%s: Cannot select pmic %s, err %d\n",
+			      __func__, p->name, ret);
+			return -1;
+		}
+	}
+
+	return old_bus;
+}
+
+static int pmic_deselect(int old_bus)
+{
+	int ret;
+
+	if (old_bus != i2c_get_bus_num()) {
+		ret = i2c_set_bus_num(old_bus);
+		debug("%s: Select bus %d\n", __func__, old_bus);
+		if (ret) {
+			debug("%s: Cannot restore i2c bus, err %d\n",
+			      __func__, ret);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
 {
 	unsigned char buf[4] = { 0 };
+	int ret, old_bus;
 
 	if (check_reg(p, reg))
 		return -1;
@@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
 		return -1;
 	}
 
-	if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
+	old_bus = pmic_select(p);
+	if (old_bus < 0)
 		return -1;
 
-	return 0;
+	ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
+	pmic_deselect(old_bus);
+	return ret;
 }
 
 int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
 {
 	unsigned char buf[4] = { 0 };
 	u32 ret_val = 0;
+	int ret, old_bus;
 
 	if (check_reg(p, reg))
 		return -1;
 
-	if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
+	old_bus = pmic_select(p);
+	if (old_bus < 0)
 		return -1;
 
+	ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
+	pmic_deselect(old_bus);
+	if (ret)
+		return ret;
+
 	switch (pmic_i2c_tx_num) {
 	case 3:
 		if (p->sensor_byte_order == PMIC_SENSOR_BYTE_ORDER_BIG)
@@ -117,9 +163,15 @@ int pmic_reg_update(struct pmic *p, int reg, uint regval)
 
 int pmic_probe(struct pmic *p)
 {
-	i2c_set_bus_num(p->bus);
+	int ret, old_bus;
+
+	old_bus = pmic_select(p);
+	if (old_bus < 0)
+		return -1;
 	debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
-	if (i2c_probe(pmic_i2c_addr)) {
+	ret = i2c_probe(pmic_i2c_addr);
+	pmic_deselect(old_bus);
+	if (ret) {
 		printf("Can't find PMIC:%s\n", p->name);
 		return -1;
 	}
-- 
1.7.10.4

[U-Boot] [PATCH 3/6] FDT: Exynos5420: Add compatible srings for PMIC

[Leela Krishna Amudala]

Add required compatible strings for PMIC S2MPS11

Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
---
 include/fdtdec.h |    1 +
 lib/fdtdec.c     |    1 +
 2 files changed, 2 insertions(+)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 6bf83bf..6290078 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -85,6 +85,7 @@ enum fdt_compat_id {
 	COMPAT_INFINEON_SLB9635_TPM,	/* Infineon SLB9635 TPM */
 	COMPAT_INFINEON_SLB9645_TPM,	/* Infineon SLB9645 TPM */
 	COMPAT_SAMSUNG_EXYNOS5_I2C,	/* Exynos5 High Speed I2C Controller */
+	COMPAT_SAMSUNG_S2MPS11_PMIC,	/* S2MPS11 PMIC */
 
 	COMPAT_COUNT,
 };
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index dc35856..0ea1c08 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -58,6 +58,7 @@ static const char * const compat_names[COMPAT_COUNT] = {
 	COMPAT(INFINEON_SLB9635_TPM, "infineon,slb9635-tpm"),
 	COMPAT(INFINEON_SLB9645_TPM, "infineon,slb9645-tpm"),
 	COMPAT(SAMSUNG_EXYNOS5_I2C, "samsung,exynos5-hsi2c"),
+	COMPAT(SAMSUNG_S2MPS11_PMIC, "samsung,s2mps11-pmic"),
 };
 
 const char *fdtdec_get_compatible(enum fdt_compat_id id)
-- 
1.7.10.4

[U-Boot] [PATCH 4/6] SMDK5420: S2MPS11: Adds the register settings for S2MPS11

[Leela Krishna Amudala]

Adds the register settings, addresses and voltages associated with S2MPS11

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
Reviewed-by: Vadim Bendebury <vbendeb@google.com>
---
 include/power/s2mps11_pmic.h |  144 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)
 create mode 100644 include/power/s2mps11_pmic.h

diff --git a/include/power/s2mps11_pmic.h b/include/power/s2mps11_pmic.h
new file mode 100644
index 0000000..e4f21f9
--- /dev/null
+++ b/include/power/s2mps11_pmic.h
@@ -0,0 +1,144 @@
+/*
+ * s2mps11_pmic.h
+ *
+ * Copyright (c) 2012 Samsung Electronics Co., Ltd
+ *              http://www.samsung.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;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ *
+ */
+#ifndef __S2MPS11_H
+#define __S2MPS11_H
+
+/* S2MPS11 registers */
+enum s2mps11_reg {
+	S2MPS11_REG_ID,
+	S2MPS11_REG_INT1,
+	S2MPS11_REG_INT2,
+	S2MPS11_REG_INT3,
+	S2MPS11_REG_INT1M,
+	S2MPS11_REG_INT2M,
+	S2MPS11_REG_INT3M,
+	S2MPS11_REG_ST1,
+	S2MPS11_REG_ST2,
+	S2MPS11_REG_OFFSRC,
+	S2MPS11_REG_PWRONSRC,
+	S2MPS11_REG_RTC_CTRL,
+	S2MPS11_REG_CTRL1,
+	S2MPS11_REG_ETC_TEST,
+	S2MPS11_REG_RSVD3,
+	S2MPS11_REG_BU_CHG,
+	S2MPS11_REG_RAMP,
+	S2MPS11_REG_RAMP_BUCK,
+	S2MPS11_REG_LDO1_8,
+	S2MPS11_REG_LDO9_16,
+	S2MPS11_REG_LDO17_24,
+	S2MPS11_REG_LDO25_32,
+	S2MPS11_REG_LDO33_38,
+	S2MPS11_REG_LDO1_8_1,
+	S2MPS11_REG_LDO9_16_1,
+	S2MPS11_REG_LDO17_24_1,
+	S2MPS11_REG_LDO25_32_1,
+	S2MPS11_REG_LDO33_38_1,
+	S2MPS11_REG_OTP_ADRL,
+	S2MPS11_REG_OTP_ADRH,
+	S2MPS11_REG_OTP_DATA,
+	S2MPS11_REG_MON1SEL,
+	S2MPS11_REG_MON2SEL,
+	S2MPS11_REG_LEE,
+	S2MPS11_REG_RSVD_NO,
+	S2MPS11_REG_UVLO,
+	S2MPS11_REG_LEE_NO,
+	S2MPS11_REG_B1CTRL1,
+	S2MPS11_REG_B1CTRL2,
+	S2MPS11_REG_B2CTRL1,
+	S2MPS11_REG_B2CTRL2,
+	S2MPS11_REG_B3CTRL1,
+	S2MPS11_REG_B3CTRL2,
+	S2MPS11_REG_B4CTRL1,
+	S2MPS11_REG_B4CTRL2,
+	S2MPS11_REG_B5CTRL1,
+	S2MPS11_REG_BUCK5_SW,
+	S2MPS11_REG_B5CTRL2,
+	S2MPS11_REG_B5CTRL3,
+	S2MPS11_REG_B5CTRL4,
+	S2MPS11_REG_B5CTRL5,
+	S2MPS11_REG_B6CTRL1,
+	S2MPS11_REG_B6CTRL2,
+	S2MPS11_REG_B7CTRL1,
+	S2MPS11_REG_B7CTRL2,
+	S2MPS11_REG_B8CTRL1,
+	S2MPS11_REG_B8CTRL2,
+	S2MPS11_REG_B9CTRL1,
+	S2MPS11_REG_B9CTRL2,
+	S2MPS11_REG_B10CTRL1,
+	S2MPS11_REG_B10CTRL2,
+	S2MPS11_REG_L1CTRL,
+	S2MPS11_REG_L2CTRL,
+	S2MPS11_REG_L3CTRL,
+	S2MPS11_REG_L4CTRL,
+	S2MPS11_REG_L5CTRL,
+	S2MPS11_REG_L6CTRL,
+	S2MPS11_REG_L7CTRL,
+	S2MPS11_REG_L8CTRL,
+	S2MPS11_REG_L9CTRL,
+	S2MPS11_REG_L10CTRL,
+	S2MPS11_REG_L11CTRL,
+	S2MPS11_REG_L12CTRL,
+	S2MPS11_REG_L13CTRL,
+	S2MPS11_REG_L14CTRL,
+	S2MPS11_REG_L15CTRL,
+	S2MPS11_REG_L16CTRL,
+	S2MPS11_REG_L17CTRL,
+	S2MPS11_REG_L18CTRL,
+	S2MPS11_REG_L19CTRL,
+	S2MPS11_REG_L20CTRL,
+	S2MPS11_REG_L21CTRL,
+	S2MPS11_REG_L22CTRL,
+	S2MPS11_REG_L23CTRL,
+	S2MPS11_REG_L24CTRL,
+	S2MPS11_REG_L25CTRL,
+	S2MPS11_REG_L26CTRL,
+	S2MPS11_REG_L27CTRL,
+	S2MPS11_REG_L28CTRL,
+	S2MPS11_REG_L29CTRL,
+	S2MPS11_REG_L30CTRL,
+	S2MPS11_REG_L31CTRL,
+	S2MPS11_REG_L32CTRL,
+	S2MPS11_REG_L33CTRL,
+	S2MPS11_REG_L34CTRL,
+	S2MPS11_REG_L35CTRL,
+	S2MPS11_REG_L36CTRL,
+	S2MPS11_REG_L37CTRL,
+	S2MPS11_REG_L38CTRL,
+
+	S2MPS11_NUM_OF_REGS,
+};
+
+/* I2C device address for pmic S2MPS11 */
+#define S2MPS11_I2C_ADDR (0xCC >> 1)
+#define S2MPS11_BUS_NUM	4
+
+/* Value to set voltage as 1V */
+#define S2MPS11_BUCK_CTRL2_1V	0x40
+/* Value to set voltage as 1.2V */
+#define S2MPS11_BUCK_CTRL2_1_2V	0x60
+/* Value to set voltage as 1.2625V */
+#define S2MPS11_BUCK_CTRL2_1_2625V	0x6A
+
+/* Buck register addresses */
+#define S2MPS11_BUCK1_CTRL2	0x26
+#define S2MPS11_BUCK2_CTRL2	0x28
+#define S2MPS11_BUCK3_CTRL2	0x2a
+#define S2MPS11_BUCK4_CTRL2	0x2c
+#define S2MPS11_BUCK6_CTRL2	0x34
+#define S2MPS11_LDO22_CTRL	0x52
+
+#define S2MPS11_DEVICE_NAME "S2MPS11_PMIC"
+
+#define S2MPS11_RTC_CTRL_32KHZ_CP_EN	(1 << 1)
+#define S2MPS11_RTC_CTRL_JIT	(1 << 4)
+#endif /*  __LINUX_MFD_S2MPS11_H */
-- 
1.7.10.4

[U-Boot] [PATCH 5/6] exynos: Add a common DT based PMIC driver initialization

[Leela Krishna Amudala]

Most of i2c PMIC drivers follow the same initialization sequence,
let's generalize it in a common file.

The initialization function finds the PMIC in the device tree, and if
found - registers it in the list of known PMICs and initializes it,
iterating through the table of settings supplied by the caller.

Signed-off-by: Vadim Bendebury <vbendeb@chromium.org>
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
Reviewed-by: Doug Anderson <dianders@google.com>
---
 board/samsung/common/board.c     |   23 ++++++++-
 drivers/power/pmic/Makefile      |    1 +
 drivers/power/pmic/pmic_common.c |   97 ++++++++++++++++++++++++++++++++++++++
 drivers/power/power_core.c       |   14 ++++++
 include/power/pmic.h             |   34 +++++++++++++
 5 files changed, 168 insertions(+), 1 deletion(-)
 create mode 100644 drivers/power/pmic/pmic_common.c

diff --git a/board/samsung/common/board.c b/board/samsung/common/board.c
index 87ca9de..561e270 100644
--- a/board/samsung/common/board.c
+++ b/board/samsung/common/board.c
@@ -19,6 +19,7 @@
 #include <asm/arch/power.h>
 #include <power/pmic.h>
 #include <power/max77686_pmic.h>
+#include <power/s2mps11_pmic.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -150,6 +151,25 @@ static int board_init_cros_ec_devices(const void *blob)
 }
 #endif
 
+#ifdef CONFIG_POWER_S2MPS11
+int board_init_s2mps11(void)
+{
+	const struct pmic_init_ops pmic_ops[] = {
+		{PMIC_REG_WRITE, S2MPS11_BUCK1_CTRL2, S2MPS11_BUCK_CTRL2_1V},
+		{PMIC_REG_WRITE, S2MPS11_BUCK2_CTRL2,
+			S2MPS11_BUCK_CTRL2_1_2625V},
+		{PMIC_REG_WRITE, S2MPS11_BUCK3_CTRL2, S2MPS11_BUCK_CTRL2_1V},
+		{PMIC_REG_WRITE, S2MPS11_BUCK4_CTRL2, S2MPS11_BUCK_CTRL2_1V},
+		{PMIC_REG_WRITE, S2MPS11_BUCK6_CTRL2, S2MPS11_BUCK_CTRL2_1V},
+		{PMIC_REG_UPDATE, S2MPS11_REG_RTC_CTRL,
+			S2MPS11_RTC_CTRL_32KHZ_CP_EN | S2MPS11_RTC_CTRL_JIT},
+		{PMIC_REG_BAIL}
+	};
+
+	return pmic_common_init(COMPAT_SAMSUNG_S2MPS11_PMIC, pmic_ops);
+}
+#endif
+
 #if defined(CONFIG_POWER)
 #ifdef CONFIG_POWER_MAX77686
 static int max77686_init(void)
@@ -255,8 +275,9 @@ int power_init_board(void)
 
 #ifdef CONFIG_POWER_MAX77686
 	ret = max77686_init();
+#elif defined(CONFIG_POWER_S2MPS11)
+	ret = board_init_s2mps11();
 #endif
-
 	return ret;
 }
 #endif
diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile
index f054470..97b945c 100644
--- a/drivers/power/pmic/Makefile
+++ b/drivers/power/pmic/Makefile
@@ -13,6 +13,7 @@ COBJS-$(CONFIG_POWER_MAX8998) += pmic_max8998.o
 COBJS-$(CONFIG_POWER_MAX8997) += pmic_max8997.o
 COBJS-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o
 COBJS-$(CONFIG_POWER_MAX77686) += pmic_max77686.o
+COBJS-$(CONFIG_POWER) += pmic_common.o
 
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
diff --git a/drivers/power/pmic/pmic_common.c b/drivers/power/pmic/pmic_common.c
new file mode 100644
index 0000000..772d1ee
--- /dev/null
+++ b/drivers/power/pmic/pmic_common.c
@@ -0,0 +1,97 @@
+/*
+ * Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+#include <common.h>
+#include <fdtdec.h>
+#include <errno.h>
+#include <power/pmic.h>
+#include <power/s2mps11_pmic.h>
+#include <power/max77686_pmic.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static unsigned pmic_number_of_regs(enum fdt_compat_id pmic_compat)
+{
+	switch (pmic_compat) {
+	case COMPAT_SAMSUNG_S2MPS11_PMIC:
+		return S2MPS11_NUM_OF_REGS;
+	default:
+		break;
+	}
+	return 0;
+}
+
+int pmic_common_init(enum fdt_compat_id pmic_compat,
+		     const struct pmic_init_ops *pmic_ops)
+{
+	const void *blob = gd->fdt_blob;
+	struct pmic *p;
+	int node, parent, ret;
+	unsigned number_of_regs = pmic_number_of_regs(pmic_compat);
+	const char *pmic_name, *comma;
+
+	if (!number_of_regs) {
+		printf("%s: %s - not a supported PMIC\n",
+		       __func__, fdtdec_get_compatible(pmic_compat));
+		return -1;
+	}
+
+	node = fdtdec_next_compatible(blob, 0, pmic_compat);
+	if (node < 0) {
+		debug("PMIC: Error %s. No node for %s in device tree\n",
+		      fdt_strerror(node), fdtdec_get_compatible(pmic_compat));
+		return node;
+	}
+
+	pmic_name = fdtdec_get_compatible(pmic_compat);
+	comma = strchr(pmic_name, ',');
+	if (comma)
+		pmic_name = comma + 1;
+
+	p = pmic_alloc();
+
+	if (!p) {
+		printf("%s: POWER allocation error!\n", __func__);
+		return -ENOMEM;
+	}
+	parent = fdt_parent_offset(blob, node);
+	if (parent < 0) {
+		debug("%s: Cannot find node parent\n", __func__);
+		return -1;
+	}
+
+	p->bus = i2c_get_bus_num_fdt(parent);
+	if (p->bus < 0) {
+		debug("%s: Cannot find I2C bus\n", __func__);
+		return -1;
+	}
+	p->hw.i2c.addr = fdtdec_get_int(blob, node, "reg", 9);
+
+	p->name = pmic_name;
+	p->interface = PMIC_I2C;
+	p->hw.i2c.tx_num = 1;
+	p->number_of_regs = number_of_regs;
+	p->compat_id = pmic_compat;
+
+	ret = 0;
+	while ((pmic_ops->reg_op != PMIC_REG_BAIL) && !ret) {
+		if (pmic_ops->reg_op == PMIC_REG_WRITE)
+			ret = pmic_reg_write(p,
+					     pmic_ops->reg_addr,
+					     pmic_ops->reg_value);
+		else
+			ret = pmic_reg_update(p,
+					     pmic_ops->reg_addr,
+					     pmic_ops->reg_value);
+		pmic_ops++;
+	}
+
+	if (ret)
+		printf("%s: Failed accessing reg 0x%x of %s\n",
+		       __func__, pmic_ops[-1].reg_addr, p->name);
+	else
+		printf("PMIC %s initialized\n", p->name);
+	return ret;
+}
diff --git a/drivers/power/power_core.c b/drivers/power/power_core.c
index d79971b..3e5eefa 100644
--- a/drivers/power/power_core.c
+++ b/drivers/power/power_core.c
@@ -205,6 +205,20 @@ int do_pmic(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	return CMD_RET_SUCCESS;
 }
 
+struct pmic *pmic_get_by_id(enum fdt_compat_id pmic_compat)
+{
+	struct pmic *p;
+
+	list_for_each_entry(p, &pmic_list, list) {
+		if (p->compat_id == pmic_compat) {
+			debug("%s: pmic %s -> 0x%p\n", __func__, p->name, p);
+			return p;
+		}
+	}
+
+	return NULL;
+}
+
 U_BOOT_CMD(
 	pmic,	CONFIG_SYS_MAXARGS, 1, do_pmic,
 	"PMIC",
diff --git a/include/power/pmic.h b/include/power/pmic.h
index d17dbdc..f7d3cf4 100644
--- a/include/power/pmic.h
+++ b/include/power/pmic.h
@@ -12,6 +12,7 @@
 #include <linux/list.h>
 #include <i2c.h>
 #include <power/power_chrg.h>
+#include <fdtdec.h>
 
 enum { PMIC_I2C, PMIC_SPI, PMIC_NONE};
 enum { I2C_PMIC, I2C_NUM, };
@@ -72,6 +73,7 @@ struct pmic {
 
 	struct pmic *parent;
 	struct list_head list;
+	enum fdt_compat_id compat_id;
 };
 
 int pmic_init(unsigned char bus);
@@ -84,6 +86,38 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val);
 int pmic_reg_write(struct pmic *p, u32 reg, u32 val);
 int pmic_set_output(struct pmic *p, u32 reg, int ldo, int on);
 int pmic_reg_update(struct pmic *p, int reg, uint regval);
+/*
+ * Find registered PMIC based on its compatibility ID.
+ *
+ * @param pmic_compat   compatibility ID of the PMIC to search for.
+ * @return pointer to the relevant 'struct pmic' on success or NULL
+ */
+struct pmic *pmic_get_by_id(enum fdt_compat_id pmic_compat);
+
+enum pmic_reg_op { PMIC_REG_BAIL, PMIC_REG_WRITE, PMIC_REG_UPDATE };
+struct pmic_init_ops {
+	enum pmic_reg_op reg_op;
+	u8	reg_addr;
+	u8	reg_value;
+};
+
+/**
+ * Common function used to intialize an i2c based PMIC.
+ *
+ * This function finds the PMIC in the device tree based on its compatibility
+ * ID. If found, the struct pmic is allocated, initialized and registered.
+ *
+ * Then the table of initialization settings is scanned and the PMIC registers
+ * are set as dictated by the table contents,
+ *
+ * @param pmic_compat   compatibility ID f the PMIC to be initialized.
+ * @param pmic_ops      a pointer to the table containing PMIC initialization
+ *			settings. The last entry contains reg_op
+ *			of PMIC_REG_BAIL.
+ * @return zero on success, nonzero on failure
+ */
+int pmic_common_init(enum fdt_compat_id pmic_compat,
+		     const struct pmic_init_ops *pmic_ops);
 
 #define pmic_i2c_addr (p->hw.i2c.addr)
 #define pmic_i2c_tx_num (p->hw.i2c.tx_num)
-- 
1.7.10.4

[U-Boot] [PATCH 6/6] config: SMDK5420: Enable S2MPS1111111111111111111111 pmic

[Leela Krishna Amudala]

configure S2MPS11 pmic on SMDK5420

Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
---
 include/configs/smdk5420.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/configs/smdk5420.h b/include/configs/smdk5420.h
index 447f8e5..46aeec0 100644
--- a/include/configs/smdk5420.h
+++ b/include/configs/smdk5420.h
@@ -53,4 +53,8 @@
 
 #define CONFIG_MAX_I2C_NUM	11
 
+#define CONFIG_POWER
+#define CONFIG_POWER_I2C
+#define CONFIG_POWER_S2MPS11
+
 #endif	/* __CONFIG_5420_H */
-- 
1.7.10.4

[U-Boot] [PATCH 6/6] config: SMDK5420: Enable S2MPS1111111111111111111111 pmic

[Leela Krishna Amudala]

Hello,
I'll change the typo error in the patch header in next version.

/Leela krishna

On Oct 1, 2013 8:11 PM, "Leela Krishna Amudala" <l.krishna@samsung.com>
wrote:
>
> configure S2MPS11 pmic on SMDK5420
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
>  include/configs/smdk5420.h |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/configs/smdk5420.h b/include/configs/smdk5420.h
> index 447f8e5..46aeec0 100644
> --- a/include/configs/smdk5420.h
> +++ b/include/configs/smdk5420.h
> @@ -53,4 +53,8 @@
>
>  #define CONFIG_MAX_I2C_NUM     11
>
> +#define CONFIG_POWER
> +#define CONFIG_POWER_I2C
> +#define CONFIG_POWER_S2MPS11
> +
>  #endif /* __CONFIG_5420_H */
> --
> 1.7.10.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 1/6] exynos: Use common pmic_reg_update() definition

[Lukasz Majewski]

Hi Leela,

> This function is used by different Exynos platforms, put it in the
> common file.
> 
> Signed-off-by: Vadim Bendebury <vbendeb@chromium.org>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> Reviewed-by: Doug Anderson <dianders@google.com>
> ---
>  board/samsung/common/board.c |   19 -------------------
>  drivers/power/power_i2c.c    |   19 +++++++++++++++++++
>  include/power/pmic.h         |    1 +
>  3 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/board/samsung/common/board.c
> b/board/samsung/common/board.c index ce85ddb..87ca9de 100644
> --- a/board/samsung/common/board.c
> +++ b/board/samsung/common/board.c
> @@ -152,25 +152,6 @@ static int board_init_cros_ec_devices(const void
> *blob) 
>  #if defined(CONFIG_POWER)
>  #ifdef CONFIG_POWER_MAX77686
> -static int pmic_reg_update(struct pmic *p, int reg, uint regval)
> -{
> -	u32 val;
> -	int ret = 0;
> -
> -	ret = pmic_reg_read(p, reg, &val);
> -	if (ret) {
> -		debug("%s: PMIC %d register read failed\n",
> __func__, reg);
> -		return -1;
> -	}
> -	val |= regval;
> -	ret = pmic_reg_write(p, reg, val);
> -	if (ret) {
> -		debug("%s: PMIC %d register write failed\n",
> __func__, reg);
> -		return -1;
> -	}
> -	return 0;
> -}
> -
>  static int max77686_init(void)
>  {
>  	struct pmic *p;
> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
> index ac76870..47c606f 100644
> --- a/drivers/power/power_i2c.c
> +++ b/drivers/power/power_i2c.c
> @@ -96,6 +96,25 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32
> *val) return 0;
>  }
>  
> +int pmic_reg_update(struct pmic *p, int reg, uint regval)

It seems like this function shall be added to pmic_core.c, since it uses
the pmic_reg_{read|write}, which are defined for both, spi and i2c.

> +{
> +	u32 val;
> +	int ret = 0;
> +
> +	ret = pmic_reg_read(p, reg, &val);
> +	if (ret) {
> +		debug("%s: PMIC %d register read failed\n",
> __func__, reg);
> +		return -1;
> +	}
> +	val |= regval;
> +	ret = pmic_reg_write(p, reg, val);
> +	if (ret) {
> +		debug("%s: PMIC %d register write failed\n",
> __func__, reg);
> +		return -1;
> +	}
> +	return 0;
> +}
> +
>  int pmic_probe(struct pmic *p)
>  {
>  	i2c_set_bus_num(p->bus);
> diff --git a/include/power/pmic.h b/include/power/pmic.h
> index 0e7aa31..d17dbdc 100644
> --- a/include/power/pmic.h
> +++ b/include/power/pmic.h
> @@ -83,6 +83,7 @@ int pmic_probe(struct pmic *p);
>  int pmic_reg_read(struct pmic *p, u32 reg, u32 *val);
>  int pmic_reg_write(struct pmic *p, u32 reg, u32 val);
>  int pmic_set_output(struct pmic *p, u32 reg, int ldo, int on);
> +int pmic_reg_update(struct pmic *p, int reg, uint regval);
>  
>  #define pmic_i2c_addr (p->hw.i2c.addr)
>  #define pmic_i2c_tx_num (p->hw.i2c.tx_num)



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus

[Lukasz Majewski]

Hi Leela,

> The current pmic i2c code assumes the current i2c bus is
> the same as the pmic device's bus. There is nothing ensuring
> that to be true. Therefore, select the proper bus before performing
> a transaction.
> 
> Signed-off-by: Aaron Durbin <adurbin@chromium.org>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> Reviewed-by: Doug Anderson <dianders@google.com>
> ---
>  drivers/power/power_i2c.c |   62
> +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57
> insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
> index 47c606f..c22e01f 100644
> --- a/drivers/power/power_i2c.c
> +++ b/drivers/power/power_i2c.c
> @@ -16,9 +16,45 @@
>  #include <i2c.h>
>  #include <compiler.h>
>  
> +static int pmic_select(struct pmic *p)
> +{
> +	int ret, old_bus;
> +
> +	old_bus = i2c_get_bus_num();
> +	if (old_bus != p->bus) {
> +		debug("%s: Select bus %d\n", __func__, p->bus);
> +		ret = i2c_set_bus_num(p->bus);
> +		if (ret) {
> +			debug("%s: Cannot select pmic %s, err %d\n",
> +			      __func__, p->name, ret);
> +			return -1;
> +		}
> +	}
> +
> +	return old_bus;
> +}
> +
> +static int pmic_deselect(int old_bus)
> +{
> +	int ret;
> +
> +	if (old_bus != i2c_get_bus_num()) {
> +		ret = i2c_set_bus_num(old_bus);
> +		debug("%s: Select bus %d\n", __func__, old_bus);
> +		if (ret) {
> +			debug("%s: Cannot restore i2c bus, err %d\n",
> +			      __func__, ret);
> +			return -1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>  {
>  	unsigned char buf[4] = { 0 };
> +	int ret, old_bus;
>  
>  	if (check_reg(p, reg))
>  		return -1;
> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32
> val) return -1;
>  	}
>  
> -	if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
> +	old_bus = pmic_select(p);
> +	if (old_bus < 0)
>  		return -1;
>  
> -	return 0;
> +	ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);

I'm wondering if setting the bus before each, single i2c write (when we
usually perform several writes to one device) will not be an overkill
(we search the ll_entry_count linker list each time to find max i2c
adapter names) ?

The i2c_set_bus_num() is now done at pmic_probe(), but this also
introduces overkill for "probing" each device when we want to read from
it.

As a side note - I would appreciate if you would add Stefano Babic and
me on the Cc (as we are listed at e.g. power_core.c).


> +	pmic_deselect(old_bus);
> +	return ret;
>  }
>  
>  int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>  {
>  	unsigned char buf[4] = { 0 };
>  	u32 ret_val = 0;
> +	int ret, old_bus;
>  
>  	if (check_reg(p, reg))
>  		return -1;
>  
> -	if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
> +	old_bus = pmic_select(p);
> +	if (old_bus < 0)
>  		return -1;
>  
> +	ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
> +	pmic_deselect(old_bus);
> +	if (ret)
> +		return ret;
> +
>  	switch (pmic_i2c_tx_num) {
>  	case 3:
>  		if (p->sensor_byte_order ==
> PMIC_SENSOR_BYTE_ORDER_BIG) @@ -117,9 +163,15 @@ int
> pmic_reg_update(struct pmic *p, int reg, uint regval) 
>  int pmic_probe(struct pmic *p)
>  {
> -	i2c_set_bus_num(p->bus);
> +	int ret, old_bus;
> +
> +	old_bus = pmic_select(p);
> +	if (old_bus < 0)
> +		return -1;
>  	debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
> -	if (i2c_probe(pmic_i2c_addr)) {
> +	ret = i2c_probe(pmic_i2c_addr);
> +	pmic_deselect(old_bus);
> +	if (ret) {
>  		printf("Can't find PMIC:%s\n", p->name);
>  		return -1;
>  	}



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 4/6] SMDK5420: S2MPS11: Adds the register settings for S2MPS11

[Lukasz Majewski]

Hi Leela,

> Adds the register settings, addresses and voltages associated with
> S2MPS11
> 
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> Reviewed-by: Vadim Bendebury <vbendeb@google.com>
> ---
>  include/power/s2mps11_pmic.h |  144
> ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 144
> insertions(+) create mode 100644 include/power/s2mps11_pmic.h
> 
> diff --git a/include/power/s2mps11_pmic.h
> b/include/power/s2mps11_pmic.h new file mode 100644
> index 0000000..e4f21f9
> --- /dev/null
> +++ b/include/power/s2mps11_pmic.h
> @@ -0,0 +1,144 @@
> +/*
> + * s2mps11_pmic.h
> + *
> + * Copyright (c) 2012 Samsung Electronics Co., Ltd
> + *              http://www.samsung.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;  either version 2 of the  License, or
> (at your
> + *  option) any later version.

We have switched to SPDX license. Please write similar code to e.g.
power_core.c

> + *
> + */
> +#ifndef __S2MPS11_H
> +#define __S2MPS11_H
> +
> +/* S2MPS11 registers */
> +enum s2mps11_reg {
> +	S2MPS11_REG_ID,
> +	S2MPS11_REG_INT1,
> +	S2MPS11_REG_INT2,
> +	S2MPS11_REG_INT3,
> +	S2MPS11_REG_INT1M,
> +	S2MPS11_REG_INT2M,
> +	S2MPS11_REG_INT3M,
> +	S2MPS11_REG_ST1,
> +	S2MPS11_REG_ST2,
> +	S2MPS11_REG_OFFSRC,
> +	S2MPS11_REG_PWRONSRC,
> +	S2MPS11_REG_RTC_CTRL,
> +	S2MPS11_REG_CTRL1,
> +	S2MPS11_REG_ETC_TEST,
> +	S2MPS11_REG_RSVD3,
> +	S2MPS11_REG_BU_CHG,
> +	S2MPS11_REG_RAMP,
> +	S2MPS11_REG_RAMP_BUCK,
> +	S2MPS11_REG_LDO1_8,
> +	S2MPS11_REG_LDO9_16,
> +	S2MPS11_REG_LDO17_24,
> +	S2MPS11_REG_LDO25_32,
> +	S2MPS11_REG_LDO33_38,
> +	S2MPS11_REG_LDO1_8_1,
> +	S2MPS11_REG_LDO9_16_1,
> +	S2MPS11_REG_LDO17_24_1,
> +	S2MPS11_REG_LDO25_32_1,
> +	S2MPS11_REG_LDO33_38_1,
> +	S2MPS11_REG_OTP_ADRL,
> +	S2MPS11_REG_OTP_ADRH,
> +	S2MPS11_REG_OTP_DATA,
> +	S2MPS11_REG_MON1SEL,
> +	S2MPS11_REG_MON2SEL,
> +	S2MPS11_REG_LEE,
> +	S2MPS11_REG_RSVD_NO,
> +	S2MPS11_REG_UVLO,
> +	S2MPS11_REG_LEE_NO,
> +	S2MPS11_REG_B1CTRL1,
> +	S2MPS11_REG_B1CTRL2,
> +	S2MPS11_REG_B2CTRL1,
> +	S2MPS11_REG_B2CTRL2,
> +	S2MPS11_REG_B3CTRL1,
> +	S2MPS11_REG_B3CTRL2,
> +	S2MPS11_REG_B4CTRL1,
> +	S2MPS11_REG_B4CTRL2,
> +	S2MPS11_REG_B5CTRL1,
> +	S2MPS11_REG_BUCK5_SW,
> +	S2MPS11_REG_B5CTRL2,
> +	S2MPS11_REG_B5CTRL3,
> +	S2MPS11_REG_B5CTRL4,
> +	S2MPS11_REG_B5CTRL5,
> +	S2MPS11_REG_B6CTRL1,
> +	S2MPS11_REG_B6CTRL2,
> +	S2MPS11_REG_B7CTRL1,
> +	S2MPS11_REG_B7CTRL2,
> +	S2MPS11_REG_B8CTRL1,
> +	S2MPS11_REG_B8CTRL2,
> +	S2MPS11_REG_B9CTRL1,
> +	S2MPS11_REG_B9CTRL2,
> +	S2MPS11_REG_B10CTRL1,
> +	S2MPS11_REG_B10CTRL2,
> +	S2MPS11_REG_L1CTRL,
> +	S2MPS11_REG_L2CTRL,
> +	S2MPS11_REG_L3CTRL,
> +	S2MPS11_REG_L4CTRL,
> +	S2MPS11_REG_L5CTRL,
> +	S2MPS11_REG_L6CTRL,
> +	S2MPS11_REG_L7CTRL,
> +	S2MPS11_REG_L8CTRL,
> +	S2MPS11_REG_L9CTRL,
> +	S2MPS11_REG_L10CTRL,
> +	S2MPS11_REG_L11CTRL,
> +	S2MPS11_REG_L12CTRL,
> +	S2MPS11_REG_L13CTRL,
> +	S2MPS11_REG_L14CTRL,
> +	S2MPS11_REG_L15CTRL,
> +	S2MPS11_REG_L16CTRL,
> +	S2MPS11_REG_L17CTRL,
> +	S2MPS11_REG_L18CTRL,
> +	S2MPS11_REG_L19CTRL,
> +	S2MPS11_REG_L20CTRL,
> +	S2MPS11_REG_L21CTRL,
> +	S2MPS11_REG_L22CTRL,
> +	S2MPS11_REG_L23CTRL,
> +	S2MPS11_REG_L24CTRL,
> +	S2MPS11_REG_L25CTRL,
> +	S2MPS11_REG_L26CTRL,
> +	S2MPS11_REG_L27CTRL,
> +	S2MPS11_REG_L28CTRL,
> +	S2MPS11_REG_L29CTRL,
> +	S2MPS11_REG_L30CTRL,
> +	S2MPS11_REG_L31CTRL,
> +	S2MPS11_REG_L32CTRL,
> +	S2MPS11_REG_L33CTRL,
> +	S2MPS11_REG_L34CTRL,
> +	S2MPS11_REG_L35CTRL,
> +	S2MPS11_REG_L36CTRL,
> +	S2MPS11_REG_L37CTRL,
> +	S2MPS11_REG_L38CTRL,
> +
> +	S2MPS11_NUM_OF_REGS,
> +};
> +
> +/* I2C device address for pmic S2MPS11 */
> +#define S2MPS11_I2C_ADDR (0xCC >> 1)
> +#define S2MPS11_BUS_NUM	4
> +
> +/* Value to set voltage as 1V */
> +#define S2MPS11_BUCK_CTRL2_1V	0x40
> +/* Value to set voltage as 1.2V */
> +#define S2MPS11_BUCK_CTRL2_1_2V	0x60
> +/* Value to set voltage as 1.2625V */
> +#define S2MPS11_BUCK_CTRL2_1_2625V	0x6A
> +
> +/* Buck register addresses */
> +#define S2MPS11_BUCK1_CTRL2	0x26
> +#define S2MPS11_BUCK2_CTRL2	0x28
> +#define S2MPS11_BUCK3_CTRL2	0x2a
> +#define S2MPS11_BUCK4_CTRL2	0x2c
> +#define S2MPS11_BUCK6_CTRL2	0x34
> +#define S2MPS11_LDO22_CTRL	0x52
> +
> +#define S2MPS11_DEVICE_NAME "S2MPS11_PMIC"
> +
> +#define S2MPS11_RTC_CTRL_32KHZ_CP_EN	(1 << 1)
> +#define S2MPS11_RTC_CTRL_JIT	(1 << 4)
> +#endif /*  __LINUX_MFD_S2MPS11_H */



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus

[Heiko Schocher]

Hello Lukasz,

Am 02.10.2013 17:11, schrieb Lukasz Majewski:
> Hi Leela,
>
>> The current pmic i2c code assumes the current i2c bus is
>> the same as the pmic device's bus. There is nothing ensuring
>> that to be true. Therefore, select the proper bus before performing
>> a transaction.
>>
>> Signed-off-by: Aaron Durbin<adurbin@chromium.org>
>> Signed-off-by: Simon Glass<sjg@chromium.org>
>> Signed-off-by: Leela Krishna Amudala<l.krishna@samsung.com>
>> Reviewed-by: Doug Anderson<dianders@google.com>
>> ---
>>   drivers/power/power_i2c.c |   62
>> +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57
>> insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
>> index 47c606f..c22e01f 100644
>> --- a/drivers/power/power_i2c.c
>> +++ b/drivers/power/power_i2c.c
>> @@ -16,9 +16,45 @@
>>   #include<i2c.h>
>>   #include<compiler.h>
>>
>> +static int pmic_select(struct pmic *p)
>> +{
>> +	int ret, old_bus;
>> +
>> +	old_bus = i2c_get_bus_num();
>> +	if (old_bus != p->bus) {
>> +		debug("%s: Select bus %d\n", __func__, p->bus);
>> +		ret = i2c_set_bus_num(p->bus);
>> +		if (ret) {
>> +			debug("%s: Cannot select pmic %s, err %d\n",
>> +			      __func__, p->name, ret);
>> +			return -1;
>> +		}
>> +	}
>> +
>> +	return old_bus;
>> +}
>> +
>> +static int pmic_deselect(int old_bus)
>> +{
>> +	int ret;
>> +
>> +	if (old_bus != i2c_get_bus_num()) {
>> +		ret = i2c_set_bus_num(old_bus);
>> +		debug("%s: Select bus %d\n", __func__, old_bus);
>> +		if (ret) {
>> +			debug("%s: Cannot restore i2c bus, err %d\n",
>> +			      __func__, ret);
>> +			return -1;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>>   {
>>   	unsigned char buf[4] = { 0 };
>> +	int ret, old_bus;
>>
>>   	if (check_reg(p, reg))
>>   		return -1;
>> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32
>> val) return -1;
>>   	}
>>
>> -	if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>> +	old_bus = pmic_select(p);
>> +	if (old_bus<  0)
>>   		return -1;
>>
>> -	return 0;
>> +	ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
>
> I'm wondering if setting the bus before each, single i2c write (when we
> usually perform several writes to one device) will not be an overkill
> (we search the ll_entry_count linker list each time to find max i2c
> adapter names) ?

Yes, maybe we could optimze this in drivers/i2c/i2c_core.c. It should be
enough to detect the max adapter once ... but it is not a "search"...
ll_entry_count() calculates the number ...

Looking in i2c_set_bus_num(), I think it can be optimized ...
lets speaking code:

diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c
index d1072e8..170423a 100644
--- a/drivers/i2c/i2c_core.c
+++ b/drivers/i2c/i2c_core.c
@@ -278,20 +278,22 @@ unsigned int i2c_get_bus_num(void)
   */
  int i2c_set_bus_num(unsigned int bus)
  {
-       int max = ll_entry_count(struct i2c_adapter, i2c);
+       int max;
+
+       if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))
+               return 0;

-       if (I2C_ADAPTER(bus) >= max) {
-               printf("Error, wrong i2c adapter %d max %d possible\n",
-                      I2C_ADAPTER(bus), max);
-               return -2;
-       }
  #ifndef CONFIG_SYS_I2C_DIRECT_BUS
         if (bus >= CONFIG_SYS_NUM_I2C_BUSES)
                 return -1;
  #endif

-       if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))
-               return 0;
+       max = ll_entry_count(struct i2c_adapter, i2c);
+       if (I2C_ADAPTER(bus) >= max) {
+               printf("Error, wrong i2c adapter %d max %d possible\n",
+                      I2C_ADAPTER(bus), max);
+               return -2;
+       }

  #ifndef CONFIG_SYS_I2C_DIRECT_BUS
         i2c_mux_disconnet_all();

So, first check, if we are on the correct bus, and return immediately!
What do you think?

Beside of that, pmic_select() does the check, if we are on the correct
bus too, and calls i2c_set_bus_num() only, if not ... so this is here
no problem ... but exactly I want to get rid of this code as it is in
pmic_select() someday, when all i2c drivers converted to the new i2c
framework. i2c_set_bus_num() should go static then in drivers/i2c/i2c_core.c
and i2c_read/write/... become a new "int bus" parameter ... but this
will be a big api change ... but will prevent exactly such code
all over the u-boot code ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus

[Leela Krishna Amudala]

Hello Lukasz,

Thanks for reviewing the patch.

On Wed, Oct 2, 2013 at 8:41 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi Leela,
>
>> The current pmic i2c code assumes the current i2c bus is
>> the same as the pmic device's bus. There is nothing ensuring
>> that to be true. Therefore, select the proper bus before performing
>> a transaction.
>>
>> Signed-off-by: Aaron Durbin <adurbin@chromium.org>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>> Reviewed-by: Doug Anderson <dianders@google.com>
>> ---
>>  drivers/power/power_i2c.c |   62
>> +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57
>> insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
>> index 47c606f..c22e01f 100644
>> --- a/drivers/power/power_i2c.c
>> +++ b/drivers/power/power_i2c.c
>> @@ -16,9 +16,45 @@
>>  #include <i2c.h>
>>  #include <compiler.h>
>>
>> +static int pmic_select(struct pmic *p)
>> +{
>> +     int ret, old_bus;
>> +
>> +     old_bus = i2c_get_bus_num();
>> +     if (old_bus != p->bus) {
>> +             debug("%s: Select bus %d\n", __func__, p->bus);
>> +             ret = i2c_set_bus_num(p->bus);
>> +             if (ret) {
>> +                     debug("%s: Cannot select pmic %s, err %d\n",
>> +                           __func__, p->name, ret);
>> +                     return -1;
>> +             }
>> +     }
>> +
>> +     return old_bus;
>> +}
>> +
>> +static int pmic_deselect(int old_bus)
>> +{
>> +     int ret;
>> +
>> +     if (old_bus != i2c_get_bus_num()) {
>> +             ret = i2c_set_bus_num(old_bus);
>> +             debug("%s: Select bus %d\n", __func__, old_bus);
>> +             if (ret) {
>> +                     debug("%s: Cannot restore i2c bus, err %d\n",
>> +                           __func__, ret);
>> +                     return -1;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>  int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>>  {
>>       unsigned char buf[4] = { 0 };
>> +     int ret, old_bus;
>>
>>       if (check_reg(p, reg))
>>               return -1;
>> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32
>> val) return -1;
>>       }
>>
>> -     if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>> +     old_bus = pmic_select(p);
>> +     if (old_bus < 0)
>>               return -1;
>>
>> -     return 0;
>> +     ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
>
> I'm wondering if setting the bus before each, single i2c write (when we
> usually perform several writes to one device) will not be an overkill
> (we search the ll_entry_count linker list each time to find max i2c
> adapter names) ?
>

Here we are not setting the bus before each i2c write, If you look at the
pmic_select() function code it shows like
static int pmic_select(struct pmic *p)
{
    /......./
    old_bus = i2c_get_bus_num();
    if (old_bus != p->bus) {
    /...../
    }
    return old_bus;
}

Here we are trying to get the bus number and if it is equal to the bus
on which we are going to write we are returning immediately which is minimal.

> The i2c_set_bus_num() is now done at pmic_probe(), but this also
> introduces overkill for "probing" each device when we want to read from
> it.
>
> As a side note - I would appreciate if you would add Stefano Babic and
> me on the Cc (as we are listed at e.g. power_core.c).
>
>

Okay, will do it.

Best Wishes,
Leela Krishna

>> +     pmic_deselect(old_bus);
>> +     return ret;
>>  }
>>
>>  int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>>  {
>>       unsigned char buf[4] = { 0 };
>>       u32 ret_val = 0;
>> +     int ret, old_bus;
>>
>>       if (check_reg(p, reg))
>>               return -1;
>>
>> -     if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>> +     old_bus = pmic_select(p);
>> +     if (old_bus < 0)
>>               return -1;
>>
>> +     ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
>> +     pmic_deselect(old_bus);
>> +     if (ret)
>> +             return ret;
>> +
>>       switch (pmic_i2c_tx_num) {
>>       case 3:
>>               if (p->sensor_byte_order ==
>> PMIC_SENSOR_BYTE_ORDER_BIG) @@ -117,9 +163,15 @@ int
>> pmic_reg_update(struct pmic *p, int reg, uint regval)
>>  int pmic_probe(struct pmic *p)
>>  {
>> -     i2c_set_bus_num(p->bus);
>> +     int ret, old_bus;
>> +
>> +     old_bus = pmic_select(p);
>> +     if (old_bus < 0)
>> +             return -1;
>>       debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
>> -     if (i2c_probe(pmic_i2c_addr)) {
>> +     ret = i2c_probe(pmic_i2c_addr);
>> +     pmic_deselect(old_bus);
>> +     if (ret) {
>>               printf("Can't find PMIC:%s\n", p->name);
>>               return -1;
>>       }
>
>
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus

[Lukasz Majewski]

Hi Leela,

> Hello Lukasz,
> 
> Thanks for reviewing the patch.
> 
> On Wed, Oct 2, 2013 at 8:41 PM, Lukasz Majewski
> <l.majewski@samsung.com> wrote:
> > Hi Leela,
> >
> >> The current pmic i2c code assumes the current i2c bus is
> >> the same as the pmic device's bus. There is nothing ensuring
> >> that to be true. Therefore, select the proper bus before performing
> >> a transaction.
> >>
> >> Signed-off-by: Aaron Durbin <adurbin@chromium.org>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> >> Reviewed-by: Doug Anderson <dianders@google.com>
> >> ---
> >>  drivers/power/power_i2c.c |   62
> >> +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57
> >> insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
> >> index 47c606f..c22e01f 100644
> >> --- a/drivers/power/power_i2c.c
> >> +++ b/drivers/power/power_i2c.c
> >> @@ -16,9 +16,45 @@
> >>  #include <i2c.h>
> >>  #include <compiler.h>
> >>
> >> +static int pmic_select(struct pmic *p)
> >> +{
> >> +     int ret, old_bus;
> >> +
> >> +     old_bus = i2c_get_bus_num();
> >> +     if (old_bus != p->bus) {
> >> +             debug("%s: Select bus %d\n", __func__, p->bus);
> >> +             ret = i2c_set_bus_num(p->bus);
> >> +             if (ret) {
> >> +                     debug("%s: Cannot select pmic %s, err %d\n",
> >> +                           __func__, p->name, ret);
> >> +                     return -1;
> >> +             }
> >> +     }
> >> +
> >> +     return old_bus;
> >> +}
> >> +
> >> +static int pmic_deselect(int old_bus)
> >> +{
> >> +     int ret;
> >> +
> >> +     if (old_bus != i2c_get_bus_num()) {
> >> +             ret = i2c_set_bus_num(old_bus);
> >> +             debug("%s: Select bus %d\n", __func__, old_bus);
> >> +             if (ret) {
> >> +                     debug("%s: Cannot restore i2c bus, err %d\n",
> >> +                           __func__, ret);
> >> +                     return -1;
> >> +             }
> >> +     }
> >> +
> >> +     return 0;
> >> +}
> >> +
> >>  int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
> >>  {
> >>       unsigned char buf[4] = { 0 };
> >> +     int ret, old_bus;
> >>
> >>       if (check_reg(p, reg))
> >>               return -1;
> >> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32
> >> val) return -1;
> >>       }
> >>
> >> -     if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
> >> +     old_bus = pmic_select(p);
> >> +     if (old_bus < 0)
> >>               return -1;
> >>
> >> -     return 0;
> >> +     ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
> >
> > I'm wondering if setting the bus before each, single i2c write
> > (when we usually perform several writes to one device) will not be
> > an overkill (we search the ll_entry_count linker list each time to
> > find max i2c adapter names) ?
> >
> 
> Here we are not setting the bus before each i2c write, If you look at
> the pmic_select() function code it shows like
> static int pmic_select(struct pmic *p)
> {
>     /......./
>     old_bus = i2c_get_bus_num();
>     if (old_bus != p->bus) {
>     /...../
>     }
>     return old_bus;
> }
> 
> Here we are trying to get the bus number and if it is equal to the bus
> on which we are going to write we are returning immediately which is
> minimal.

Thanks for explanation.

> 
> > The i2c_set_bus_num() is now done at pmic_probe(), but this also
> > introduces overkill for "probing" each device when we want to read
> > from it.
> >
> > As a side note - I would appreciate if you would add Stefano Babic
> > and me on the Cc (as we are listed at e.g. power_core.c).
> >
> >
> 
> Okay, will do it.
> 
> Best Wishes,
> Leela Krishna
> 
> >> +     pmic_deselect(old_bus);
> >> +     return ret;
> >>  }
> >>
> >>  int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
> >>  {
> >>       unsigned char buf[4] = { 0 };
> >>       u32 ret_val = 0;
> >> +     int ret, old_bus;
> >>
> >>       if (check_reg(p, reg))
> >>               return -1;
> >>
> >> -     if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
> >> +     old_bus = pmic_select(p);
> >> +     if (old_bus < 0)
> >>               return -1;
> >>
> >> +     ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
> >> +     pmic_deselect(old_bus);
> >> +     if (ret)
> >> +             return ret;
> >> +
> >>       switch (pmic_i2c_tx_num) {
> >>       case 3:
> >>               if (p->sensor_byte_order ==
> >> PMIC_SENSOR_BYTE_ORDER_BIG) @@ -117,9 +163,15 @@ int
> >> pmic_reg_update(struct pmic *p, int reg, uint regval)
> >>  int pmic_probe(struct pmic *p)
> >>  {
> >> -     i2c_set_bus_num(p->bus);
> >> +     int ret, old_bus;
> >> +
> >> +     old_bus = pmic_select(p);
> >> +     if (old_bus < 0)
> >> +             return -1;
> >>       debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
> >> -     if (i2c_probe(pmic_i2c_addr)) {
> >> +     ret = i2c_probe(pmic_i2c_addr);
> >> +     pmic_deselect(old_bus);
> >> +     if (ret) {
> >>               printf("Can't find PMIC:%s\n", p->name);
> >>               return -1;
> >>       }
> >
> >
> >
> > --
> > Best regards,
> >
> > Lukasz Majewski
> >
> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus

[Lukasz Majewski]

Hi Heiko,

Sorry for a late reply.

> Hello Lukasz,
> 
> Am 02.10.2013 17:11, schrieb Lukasz Majewski:
> > Hi Leela,
> >
> >> The current pmic i2c code assumes the current i2c bus is
> >> the same as the pmic device's bus. There is nothing ensuring
> >> that to be true. Therefore, select the proper bus before performing
> >> a transaction.
> >>
> >> Signed-off-by: Aaron Durbin<adurbin@chromium.org>
> >> Signed-off-by: Simon Glass<sjg@chromium.org>
> >> Signed-off-by: Leela Krishna Amudala<l.krishna@samsung.com>
> >> Reviewed-by: Doug Anderson<dianders@google.com>
> >> ---
> >>   drivers/power/power_i2c.c |   62
> >> +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57
> >> insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
> >> index 47c606f..c22e01f 100644
> >> --- a/drivers/power/power_i2c.c
> >> +++ b/drivers/power/power_i2c.c
> >> @@ -16,9 +16,45 @@
> >>   #include<i2c.h>
> >>   #include<compiler.h>
> >>
> >> +static int pmic_select(struct pmic *p)
> >> +{
> >> +	int ret, old_bus;
> >> +
> >> +	old_bus = i2c_get_bus_num();
> >> +	if (old_bus != p->bus) {
> >> +		debug("%s: Select bus %d\n", __func__, p->bus);
> >> +		ret = i2c_set_bus_num(p->bus);
> >> +		if (ret) {
> >> +			debug("%s: Cannot select pmic %s, err
> >> %d\n",
> >> +			      __func__, p->name, ret);
> >> +			return -1;
> >> +		}
> >> +	}
> >> +
> >> +	return old_bus;
> >> +}
> >> +
> >> +static int pmic_deselect(int old_bus)
> >> +{
> >> +	int ret;
> >> +
> >> +	if (old_bus != i2c_get_bus_num()) {
> >> +		ret = i2c_set_bus_num(old_bus);
> >> +		debug("%s: Select bus %d\n", __func__, old_bus);
> >> +		if (ret) {
> >> +			debug("%s: Cannot restore i2c bus, err
> >> %d\n",
> >> +			      __func__, ret);
> >> +			return -1;
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>   int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
> >>   {
> >>   	unsigned char buf[4] = { 0 };
> >> +	int ret, old_bus;
> >>
> >>   	if (check_reg(p, reg))
> >>   		return -1;
> >> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32
> >> val) return -1;
> >>   	}
> >>
> >> -	if (i2c_write(pmic_i2c_addr, reg, 1, buf,
> >> pmic_i2c_tx_num))
> >> +	old_bus = pmic_select(p);
> >> +	if (old_bus<  0)
> >>   		return -1;
> >>
> >> -	return 0;
> >> +	ret = i2c_write(pmic_i2c_addr, reg, 1, buf,
> >> pmic_i2c_tx_num);
> >
> > I'm wondering if setting the bus before each, single i2c write
> > (when we usually perform several writes to one device) will not be
> > an overkill (we search the ll_entry_count linker list each time to
> > find max i2c adapter names) ?
> 
> Yes, maybe we could optimze this in drivers/i2c/i2c_core.c. It should
> be enough to detect the max adapter once ... but it is not a
> "search"... ll_entry_count() calculates the number ...

Yes, you are right. I've overlooked it.

With -Os compiler flag this compiles to a few ASM instructions.
Obviously it is NOT a performance killer :-) (I made unnecessary
fuzzz... sorry).

> 
> Looking in i2c_set_bus_num(), I think it can be optimized ...
> lets speaking code:
> 
> diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c
> index d1072e8..170423a 100644
> --- a/drivers/i2c/i2c_core.c
> +++ b/drivers/i2c/i2c_core.c
> @@ -278,20 +278,22 @@ unsigned int i2c_get_bus_num(void)
>    */
>   int i2c_set_bus_num(unsigned int bus)
>   {
> -       int max = ll_entry_count(struct i2c_adapter, i2c);
> +       int max;
> +
> +       if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))
> +               return 0;

This looks nice.

> 
> -       if (I2C_ADAPTER(bus) >= max) {
> -               printf("Error, wrong i2c adapter %d max %d
> possible\n",
> -                      I2C_ADAPTER(bus), max);
> -               return -2;
> -       }
>   #ifndef CONFIG_SYS_I2C_DIRECT_BUS
>          if (bus >= CONFIG_SYS_NUM_I2C_BUSES)
>                  return -1;
>   #endif
> 
> -       if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))
> -               return 0;
> +       max = ll_entry_count(struct i2c_adapter, i2c);
> +       if (I2C_ADAPTER(bus) >= max) {
> +               printf("Error, wrong i2c adapter %d max %d
> possible\n",
> +                      I2C_ADAPTER(bus), max);
> +               return -2;
> +       }
> 
>   #ifndef CONFIG_SYS_I2C_DIRECT_BUS
>          i2c_mux_disconnet_all();
> 
> So, first check, if we are on the correct bus, and return immediately!
> What do you think?

I think that it is acceptable.

> 
> Beside of that, pmic_select() does the check, if we are on the correct
> bus too, and calls i2c_set_bus_num() only, if not ... so this is here
> no problem ... 

Yes, I see.

> but exactly I want to get rid of this code as it is in
> pmic_select() someday, when all i2c drivers converted to the new i2c
> framework.

My 2 cents. I understand that pmic_select() preserves old i2c bus
number, when PMIC performs transmission. This is probably done to not
break the legacy code (where one driver assumed, that it is alone).

If this is necessary, then I'm OK with this. However I personally think,
that drivers shall call API functions from i2c core (like i2c_bus_num())
only with bus number to switch and do not store and preserve the i2c
value. This is my personal comment.


> i2c_set_bus_num() should go static then in
> drivers/i2c/i2c_core.c and i2c_read/write/... become a new "int bus"
> parameter ... but this will be a big api change ... but will prevent
> exactly such code all over the u-boot code ...
> 
> bye,
> Heiko



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus

[Heiko Schocher]

Hello Lukasz,

Am 03.10.2013 18:15, schrieb Lukasz Majewski:
> Hi Heiko,
>
> Sorry for a late reply.
>
>> Hello Lukasz,
>>
>> Am 02.10.2013 17:11, schrieb Lukasz Majewski:
>>> Hi Leela,
>>>
>>>> The current pmic i2c code assumes the current i2c bus is
>>>> the same as the pmic device's bus. There is nothing ensuring
>>>> that to be true. Therefore, select the proper bus before performing
>>>> a transaction.
>>>>
>>>> Signed-off-by: Aaron Durbin<adurbin@chromium.org>
>>>> Signed-off-by: Simon Glass<sjg@chromium.org>
>>>> Signed-off-by: Leela Krishna Amudala<l.krishna@samsung.com>
>>>> Reviewed-by: Doug Anderson<dianders@google.com>
>>>> ---
>>>>    drivers/power/power_i2c.c |   62
>>>> +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57
>>>> insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
>>>> index 47c606f..c22e01f 100644
>>>> --- a/drivers/power/power_i2c.c
>>>> +++ b/drivers/power/power_i2c.c
[...]
>> Yes, maybe we could optimze this in drivers/i2c/i2c_core.c. It should
>> be enough to detect the max adapter once ... but it is not a
>> "search"... ll_entry_count() calculates the number ...
>
> Yes, you are right. I've overlooked it.
>
> With -Os compiler flag this compiles to a few ASM instructions.
> Obviously it is NOT a performance killer :-) (I made unnecessary
> fuzzz... sorry).

No problem!

>> Looking in i2c_set_bus_num(), I think it can be optimized ...
>> lets speaking code:
>>
>> diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c
>> index d1072e8..170423a 100644
>> --- a/drivers/i2c/i2c_core.c
>> +++ b/drivers/i2c/i2c_core.c
>> @@ -278,20 +278,22 @@ unsigned int i2c_get_bus_num(void)
>>     */
>>    int i2c_set_bus_num(unsigned int bus)
>>    {
>> -       int max = ll_entry_count(struct i2c_adapter, i2c);
>> +       int max;
>> +
>> +       if ((bus == I2C_BUS)&&  (I2C_ADAP->init_done>  0))
>> +               return 0;
>
> This looks nice.

Ok! I post soon a patch for it ...

>> -       if (I2C_ADAPTER(bus)>= max) {
>> -               printf("Error, wrong i2c adapter %d max %d
>> possible\n",
>> -                      I2C_ADAPTER(bus), max);
>> -               return -2;
>> -       }
>>    #ifndef CONFIG_SYS_I2C_DIRECT_BUS
>>           if (bus>= CONFIG_SYS_NUM_I2C_BUSES)
>>                   return -1;
>>    #endif
>>
>> -       if ((bus == I2C_BUS)&&  (I2C_ADAP->init_done>  0))
>> -               return 0;
>> +       max = ll_entry_count(struct i2c_adapter, i2c);
>> +       if (I2C_ADAPTER(bus)>= max) {
>> +               printf("Error, wrong i2c adapter %d max %d
>> possible\n",
>> +                      I2C_ADAPTER(bus), max);
>> +               return -2;
>> +       }
>>
>>    #ifndef CONFIG_SYS_I2C_DIRECT_BUS
>>           i2c_mux_disconnet_all();
>>
>> So, first check, if we are on the correct bus, and return immediately!
>> What do you think?
>
> I think that it is acceptable.

Good.

>> Beside of that, pmic_select() does the check, if we are on the correct
>> bus too, and calls i2c_set_bus_num() only, if not ... so this is here
>> no problem ...
>
> Yes, I see.
>
>> but exactly I want to get rid of this code as it is in
>> pmic_select() someday, when all i2c drivers converted to the new i2c
>> framework.
>
> My 2 cents. I understand that pmic_select() preserves old i2c bus
> number, when PMIC performs transmission. This is probably done to not
> break the legacy code (where one driver assumed, that it is alone).
>
> If this is necessary, then I'm OK with this. However I personally think,
> that drivers shall call API functions from i2c core (like i2c_bus_num())
> only with bus number to switch and do not store and preserve the i2c
> value. This is my personal comment.

Full Ack. I am just thinking, that we can get rid of such constructs,
independent of the new i2c framework switch. We just need to introduce
a "current_i2c_cmd_bus" in common/cmd_i2c.c. This var stores the
current i2c bus where i2c commands are executed ... and all other
subsystems, which use the i2c_api can call i2c_set_bus_num() without
a previous "save old bus" and after the i2c bus usage a "restore i2c
bus" ... I try to look into this, maybe we can do this before all
i2c drivers are ported to the new framework ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus

[Lukasz Majewski]

Hi Heiko,

> Hello Lukasz,
> 
> Am 03.10.2013 18:15, schrieb Lukasz Majewski:
> > Hi Heiko,
> >
> > Sorry for a late reply.
> >
> >> Hello Lukasz,
> >>
> >> Am 02.10.2013 17:11, schrieb Lukasz Majewski:
> >>> Hi Leela,
> >>>
> >>>> The current pmic i2c code assumes the current i2c bus is
> >>>> the same as the pmic device's bus. There is nothing ensuring
> >>>> that to be true. Therefore, select the proper bus before
> >>>> performing a transaction.
> >>>>
> >>>> Signed-off-by: Aaron Durbin<adurbin@chromium.org>
> >>>> Signed-off-by: Simon Glass<sjg@chromium.org>
> >>>> Signed-off-by: Leela Krishna Amudala<l.krishna@samsung.com>
> >>>> Reviewed-by: Doug Anderson<dianders@google.com>
> >>>> ---
> >>>>    drivers/power/power_i2c.c |   62
> >>>> +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57
> >>>> insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/power/power_i2c.c
> >>>> b/drivers/power/power_i2c.c index 47c606f..c22e01f 100644
> >>>> --- a/drivers/power/power_i2c.c
> >>>> +++ b/drivers/power/power_i2c.c
> [...]
> >> Yes, maybe we could optimze this in drivers/i2c/i2c_core.c. It
> >> should be enough to detect the max adapter once ... but it is not a
> >> "search"... ll_entry_count() calculates the number ...
> >
> > Yes, you are right. I've overlooked it.
> >
> > With -Os compiler flag this compiles to a few ASM instructions.
> > Obviously it is NOT a performance killer :-) (I made unnecessary
> > fuzzz... sorry).
> 
> No problem!
> 
> >> Looking in i2c_set_bus_num(), I think it can be optimized ...
> >> lets speaking code:
> >>
> >> diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c
> >> index d1072e8..170423a 100644
> >> --- a/drivers/i2c/i2c_core.c
> >> +++ b/drivers/i2c/i2c_core.c
> >> @@ -278,20 +278,22 @@ unsigned int i2c_get_bus_num(void)
> >>     */
> >>    int i2c_set_bus_num(unsigned int bus)
> >>    {
> >> -       int max = ll_entry_count(struct i2c_adapter, i2c);
> >> +       int max;
> >> +
> >> +       if ((bus == I2C_BUS)&&  (I2C_ADAP->init_done>  0))
> >> +               return 0;
> >
> > This looks nice.
> 
> Ok! I post soon a patch for it ...
> 
> >> -       if (I2C_ADAPTER(bus)>= max) {
> >> -               printf("Error, wrong i2c adapter %d max %d
> >> possible\n",
> >> -                      I2C_ADAPTER(bus), max);
> >> -               return -2;
> >> -       }
> >>    #ifndef CONFIG_SYS_I2C_DIRECT_BUS
> >>           if (bus>= CONFIG_SYS_NUM_I2C_BUSES)
> >>                   return -1;
> >>    #endif
> >>
> >> -       if ((bus == I2C_BUS)&&  (I2C_ADAP->init_done>  0))
> >> -               return 0;
> >> +       max = ll_entry_count(struct i2c_adapter, i2c);
> >> +       if (I2C_ADAPTER(bus)>= max) {
> >> +               printf("Error, wrong i2c adapter %d max %d
> >> possible\n",
> >> +                      I2C_ADAPTER(bus), max);
> >> +               return -2;
> >> +       }
> >>
> >>    #ifndef CONFIG_SYS_I2C_DIRECT_BUS
> >>           i2c_mux_disconnet_all();
> >>
> >> So, first check, if we are on the correct bus, and return
> >> immediately! What do you think?
> >
> > I think that it is acceptable.
> 
> Good.
> 
> >> Beside of that, pmic_select() does the check, if we are on the
> >> correct bus too, and calls i2c_set_bus_num() only, if not ... so
> >> this is here no problem ...
> >
> > Yes, I see.
> >
> >> but exactly I want to get rid of this code as it is in
> >> pmic_select() someday, when all i2c drivers converted to the new
> >> i2c framework.
> >
> > My 2 cents. I understand that pmic_select() preserves old i2c bus
> > number, when PMIC performs transmission. This is probably done to
> > not break the legacy code (where one driver assumed, that it is
> > alone).
> >
> > If this is necessary, then I'm OK with this. However I personally
> > think, that drivers shall call API functions from i2c core (like
> > i2c_bus_num()) only with bus number to switch and do not store and
> > preserve the i2c value. This is my personal comment.
> 
> Full Ack. I am just thinking, that we can get rid of such constructs,
> independent of the new i2c framework switch. We just need to introduce
> a "current_i2c_cmd_bus" in common/cmd_i2c.c. This var stores the
> current i2c bus where i2c commands are executed ... 

I think that "last used bus" variable shall be stored/managed at
i2c_core.c. I can use i2c without cmd_i2c.c compiled (as it is with
pmic and fuel gauge, which use different buses).

> and all other
> subsystems, which use the i2c_api can call i2c_set_bus_num() without
> a previous "save old bus" and after the i2c bus usage a "restore i2c
> bus" ...


> I try to look into this, maybe we can do this before all
> i2c drivers are ported to the new framework ...

Ok, lets wait for a patch.

> 
> bye,
> Heiko



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus

[Heiko Schocher]

Hello Lukasz,

Am 04.10.2013 10:58, schrieb Lukasz Majewski:
> Hi Heiko,
>
>> Hello Lukasz,
>>
>> Am 03.10.2013 18:15, schrieb Lukasz Majewski:
>>> Hi Heiko,
>>>
>>> Sorry for a late reply.
>>>
>>>> Hello Lukasz,
>>>>
>>>> Am 02.10.2013 17:11, schrieb Lukasz Majewski:
>>>>> Hi Leela,
[...]
>>>> but exactly I want to get rid of this code as it is in
>>>> pmic_select() someday, when all i2c drivers converted to the new
>>>> i2c framework.
>>>
>>> My 2 cents. I understand that pmic_select() preserves old i2c bus
>>> number, when PMIC performs transmission. This is probably done to
>>> not break the legacy code (where one driver assumed, that it is
>>> alone).
>>>
>>> If this is necessary, then I'm OK with this. However I personally
>>> think, that drivers shall call API functions from i2c core (like
>>> i2c_bus_num()) only with bus number to switch and do not store and
>>> preserve the i2c value. This is my personal comment.
>>
>> Full Ack. I am just thinking, that we can get rid of such constructs,
>> independent of the new i2c framework switch. We just need to introduce
>> a "current_i2c_cmd_bus" in common/cmd_i2c.c. This var stores the
>> current i2c bus where i2c commands are executed ...
>
> I think that "last used bus" variable shall be stored/managed at
> i2c_core.c. I can use i2c without cmd_i2c.c compiled (as it is with

Thats the case for the new framework! It stores the current bus, and
so i2c_set_bus_num() can decide, if it is necessary to switch to
a new bus ...

Additionally to get rid of such "store oldbus, switch to new, restore
old bus" constructs, the i2c commands needs to store somewhere, which
i2c bus the i2c commands currently use, because you can do a "i2c md ..."
then a "date" (maybe on antoher i2c bus) and then again a "i2c md ..."
I think, thats the real historical reason for doing all over the code
such "save oldbus" code ...

That var (saying i2c_cur_cmd_bus) can easily stored in common/cmd_i2c.c,
as commands are only useable after relocation. So, we must just add a
i2c_set_bus_num(i2c_cur_cmd_bus) call before a command is executed ...
That can be done in common/cmd_i2c.c also ...

So, in the end, before every i2c access we call i2c_set_bus_num()
and have no longer to store an "old bus" ... and if we have reached
this point, we can make i2c_set_bus_num() static, and add to the
i2c api a new first "int bus" parameter .... and can delete all
i2c_set_bus_num() calls ... so every i2c device must know, on which
i2c bus it works ... and as a bonus, we can get rid of all i2c_init()
calls all over around the code, as i2c_set_bus_num() checks, if the
drivers is initialized or not, if not initialize it, look if i2c
muxes are involved on this bus, and so on ...

> pmic and fuel gauge, which use different buses).

Yep, and its enough to call i2c_set_bus_num() before you want to access
a device ... i2c_set_bus_num() do all necessary steps for you.

>> and all other
>> subsystems, which use the i2c_api can call i2c_set_bus_num() without
>> a previous "save old bus" and after the i2c bus usage a "restore i2c
>> bus" ...
>
>
>> I try to look into this, maybe we can do this before all
>> i2c drivers are ported to the new framework ...
>
> Ok, lets wait for a patch.

I have such a patch in my queue, but it works only for drivers, which use
the new framework I think ... so I must think about it, if this works
for old style drivers too ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany