Power(full) framework based on Driver Model

Hi Przemyslaw,

Quoted 81 lines · Przemyslaw Marczak

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Changes v2, V3:
- update documentation with the framework api changes
- remove doc file name 'dm' prefix

---
  doc/driver-model/pmic-framework.txt | 350 ++++++++++++++++++++++++++++++++++++
  1 file changed, 350 insertions(+)

create mode 100644 doc/driver-model/pmic-framework.txt

doc/driver-model/pmic-framework.txt new file +65 -0

new file mode 100644
index 0000000..72651dc
@@ -0,0 +1,350 @@
+#
+# (C) Copyright 2014-2015 Samsung Electronics
+# Przemyslaw Marczak <[email protected]>
+#
+# SPDX-License-Identifier:      GPL-2.0+
+#
+
+PMIC framework based on Driver Model
+====================================
+TOC:
+1. Introduction
+2. How does it work
+3. Pmic driver api
+4. Pmic driver
+5. Pmic command
+6. Regulator driver api
+7. Regulator driver
+8. Regulator command
+
+1. Introduction
+===============
+This is an introduction to driver-model multi uclass PMIC devices support.
+At present it is based on two uclass types:
+
+- UCLASS_PMIC      - basic uclass type for PMIC I/O, which provides common
+                     read/write interface.
+- UCLASS_REGULATOR - additional uclass type for specific PMIC features, which
+                     are various voltage regulators.
+
+New files:
+UCLASS_PMIC:
+- drivers/power/pmic-uclass.c
+- include/power/pmic.h
+UCLASS_REGULATOR:
+- drivers/power/regulator-uclass.c
+- include/power/regulator.h
+
+Commands:
+- lib/cmd_pmic.c
+- lib/cmd_regulator.c
+
+2. How doees it work
+====================
+The Power Management Integrated Circuits (PMIC) are used in embedded systems
+to provide stable, precise and specific voltage power source with over-voltage
+and thermal protection circuits.
+
+The single PMIC can provide various functionalities with single or multiple
+interfaces, like in the example below.
+
+-- SoC
+ |
+ |            ______________________________________
+ | BUS 0     |       Multi interface PMIC IC        |--> LDO out 1
+ | e.g.I2C0  |                                      |--> LDO out N
+ |-----------|---- PMIC device 0 (READ/WRITE ops)   |
+ | or SPI0   |    |_ REGULATOR device (ldo/... ops) |--> BUCK out 1
+ |           |    |_ CHARGER device (charger ops)   |--> BUCK out M
+ |           |    |_ MUIC device (microUSB con ops) |
+ | BUS 1     |    |_ ...                            |---> BATTERY
+ | e.g.I2C1  |                                      |
+ |-----------|---- PMIC device 1 (READ/WRITE ops)   |---> USB in 1
+ . or SPI1   |    |_ RTC device (rtc ops)           |---> USB in 2
+ .           |______________________________________|---> USB out
+ .

doc/driver-model/pmic-framework.txt line 75

Quoted 10 lines

doc/driver-model/pmic-framework.txt new file +10 -0

+
+Since U-Boot provides driver model features for I2C and SPI bus drivers,
+the PMIC devices should also support this. With the new basic uclass types
+for PMIC I/O and regulator features, PMIC drivers can simply provide common
+features, with multiple interface and instance support.
+
+Basic design assumptions:
+
+- Common I/O api - UCLASS_PMIC
+The main assumption is to use UCLASS_PMIC device to provide I/O interface,

an I/O interface

doc/driver-model/pmic-framework.txt line 77

Quoted 2 lines

doc/driver-model/pmic-framework.txt new file +2 -0

+for devices other uclass types. It is no matter what is the type of device
+physical I/O interface.

devices of other uclass types. It doesn't matter what type of physical I/O interface is used.

Usually PMIC devices are using SPI or I2C interface,

s/are using/use/

doc/driver-model/pmic-framework.txt line 82

Quoted 5 lines

doc/driver-model/pmic-framework.txt new file +5 -0

+but use of any other interface (e.g. when PMIC is not directly connected
+to the SoC) - is now possible. Drivers can use the same read/write api.
+
+- Common regulator api - UCLASS_REGULATOR
+For setting the attributes of verious types of regulators with common api,

various

with a common

doc/driver-model/pmic-framework.txt line 83

Quoted 1 line

doc/driver-model/pmic-framework.txt new file +1 -0

+this uclass can be implemented. This allows to drive the each regulator output

allows driving each regulator's output

doc/driver-model/pmic-framework.txt line 84

Quoted 1 line

doc/driver-model/pmic-framework.txt new file +1 -0

+value, on/off state and custom defined operation modes. It also provides the

custom-defined

or perhaps just 'particular'

doc/driver-model/pmic-framework.txt line 86

Quoted 2 lines

doc/driver-model/pmic-framework.txt new file +2 -0

+user interface for all operations.
+For the very simple implementation, the regulator drivers are not required,

For simple implementations, regulator drivers are not required, so the code can use pmic read/write directly.

Quoted 100 lines

doc/driver-model/pmic-framework.txt new file +100 -0

+so the code could base on pmic read/write only.
+
+When board device-tree file includes pmic subnode and the U_Boot compatible
+driver exists, then the pmic device bind should looks like this:
+
+|_ root - will bind the device for I2C/SPI bus node
+  |_ i2c/spi - should bind a device for pmic node
+    |_ pmic (parent) - should bind child devices for its features
+      |_ regulator (child)
+      |_ charger   (child)
+      |_ other     (child)
+
+Usually PMIC design provides:
+ - single I/O interface (single UCLASS_PMIC driver)
+   Then UCLASS_PMIC device should be a parent of all pmic devices, where each
+   is usually different uclass type, but need to access the same interface
+
+ - multiple I/O interfaces (UCLASS_PMIC driver for each)
+   For each interface the UCLASS_PMIC device should be a parent of only those
+   devices (different uclass types), which needs to access the specified
+   interface.
+
+3. Pmic driver api
+===================
+To use the pmic API, config: CONFIG_DM_PMIC is required.
+The new driver API is very simple and is based on 'struct dm_pmic_ops',
+which define two basic operations: device read and write.
+
+The platform data is introduced as 'struct pmic_platdata', to keep an info
+about the device interface.
+
+The api is described in file: 'include/power/pmic.h'
+Getting the device:
+- by name  -  int pmic_get(char *name, struct udevice **pmic);
+
+Device I/O interface
+Can be used with UCLASS_PMIC devices:
+- Read from the device 'len' bytes at 'reg' into the buffer:
+  int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len);
+
+- Write to the device 'len' bytes at 'reg' from the buffer:
+  int pmic_write(struct udevice *dev, uint reg, uint8_t *buffer, int len);
+
+4. Pmic driver
+============================
+As an example of the pmic driver, please take a look into the MAX77686 driver
+'drivers/power/pmic/max77686.c' and the description in 'include/power/pmic.h'
+
+The pmic driver can be defined by U_BOOT_DRIVER() macro:
+
+U_BOOT_DRIVER(pmic_max77686) = {
+       .name = "max77686 pmic",
+       .id = UCLASS_PMIC,
+       .of_match = max77686_ids,     - Allows to bind by compatible
+       .bind = max77686_bind,        - Function called at device bind
+       .ops = &max77686_ops,         - Pmic api function calls
+       .platdata_auto_alloc_size = sizeof(struct pmic_platdata),
+};
+
+To bind the pmic device, field '.of_match' is required with proper compatible.
+
+Driver ops:
+.reg_count = MAX77686_NUM_OF_REGS - number of pmic registers
+.read = max77686_read() - allows to use pmic_read()
+.write = max77686_write() - allows to use pmic_write()
+
+Driver bind:
+- max77686_bind(): called on bind and calls pmic_child_node_scan() to bind the
+  childs which are int "voltage-regulators" subnode. The bind is done using
+  "voltage-regulators" property name.
+
+5. Pmic command
+===============
+To use the pmic command, config: CONFIG_DM_PMIC_CMD is required.
+The new pmic command allows to:
+- list pmic devices
+- choose the current device (like the mmc command)
+- read or write the pmic register
+- dump all pmic registers
+
+This command can use only UCLASS_PMIC devices, since this uclass is designed
+for pmic I/O operations only.
+
+Command options (pmic [option]):
+- list                     - list available PMICs
+- dev <id>                 - set id to current pmic device
+- pmic dump                - dump registers
+- pmic read <reg>          - read register
+- pmic write <reg> <value> - write register
+
+Example of usage:
+# pmic list           - chose one dev Id, e.g. 3
+# pmic dev 0          - set dev seq 0 as current device
+# pmic dump           - dump the registers of the current pmic dev
+# pmic read 0x0       - read the register at address 0x0
+# pmic write 0x0 0x1  - write 0x1 to the register at addres 0x0
+
+6. Regulator driver API
+===================================
+To use the regulator API, config: CONFIG_DM_REGULATOR is required.

doc/driver-model/pmic-framework.txt line 196

Quoted 10 lines

doc/driver-model/pmic-framework.txt new file +10 -0

+The api is described in file: 'include/power/regulator.h'
+The api is based on structure types:
+- 'dm_regulator_info' - dev->uc_priv (auto-allocated)
+- 'dm_regulator_mode' - included in regualtor info
+
+Regulator info keeps the constraints taken from the device-tree and also device
+modes set by the driver (single or array).
+
+The fixed-regulator common driver keeps provides private structure type, to keep
+its gpio attributes. The structure type 'fixed_regulator_priv' is keept in field

kept

Quoted 137 lines

doc/driver-model/pmic-framework.txt new file +137 -0

+'dev->priv' and is allocated by the driver.
+
+6.1 Regulator device-tree node:
+The regulator node should looks like in the example:
+ldo1 {
+        regulator-compatible = "LDO1"; (not used here, but required for bind)
+        regulator-name = "VDD_MMC_1.8V"; (mandatory) *
+        regulator-min-microvolt = <1000000>; (mandatory) *
+        regulator-max-microvolt = <1000000>; (mandatory) *
+        regulator-min-microamp = <1000>; (optional) *
+        regulator-max-microamp = <1000>; (optional) *
+        regulator-always-on; (optional) *
+        regulator-boot-on; (optional) *
+};
+
+For the fixed-voltage regulator, voltage min and max must be equal:
+VDD_MMC: regulator at 0 {
+        compatible = "regulator-fixed";
+        regulator-name = "VDD_MMC"; (mandatory) *
+        regulator-min-microvolt = <2800000>; (mandatory) *
+        regulator-max-microvolt = <2800000>; (mandatory) *
+        regulator-always-on; (optional)
+        regulator-boot-on; (optional)
+        gpio = <&gpc 1 GPIO_ACTIVE_HIGH>; (optional)
+        gpio-open-drain; (optional)
+        enable-active-high; (optional)
+        startup-delay-us
+};
+
+The attributes signed with '*' can be taken by the common function which is
+regulator_ofdata_to_platdata(). The rest attributes for fixed-regulator, are
+taken by the driver.
+
+6.2 Getting the device:
+- by name - int regulator_get(char *name, struct udevice **regulator);
+
+6.3 Device I/O interface
+According to the framework assumptions, where uclass pmic device is a parent
+of pmic devices other uclass types, the regulator devices should use uclass
+pmic interface, with the parent as the device, like this:
+- pmic_read(regulator->parent, some_reg, &some_buff, count);
+- pmic_write(regulator->parent, some_reg, &some_buff, count);
+
+6.4 Device regulator operations
+The regulator function calls are based on few data types:
+- enum regulator_type {...} - standard types: LDO, BUCK, DVS, FIXED
+- struct dm_regulator_info {...} - output name and value limits
+- struct dm_regulator_mode {...} - output operation mode value and name
+- struct dm_regulator_ops {...} - regulator driver function calls
+
+The first argument is always device. And the device uclass id must be always:
+- 'UCLASS_REGULATOR'
+
+Function details are described in regulator header. The basic features are:
+- regulator_info()            - get the regulator info structure
+- regulator_mode()            - get the regulator mode info structure
+- regulator_get/set_value()   - get/set the regulator output voltage
+- regulator_get/set_current() - get/set the regulator output current
+- regulator_get/set_enable()  - get/set the regulator output enable state
+- regulator_get/set_mode()    - get/set the regulator output operation mode
+
+7. Regulator driver
+======================================
+As an example of the regulator driver, please take a look into the MAX77686
+regulator driver (drivers/power/regulator/max77686.c). It implements two drivers
+for the buck and ldo regulators. The buck driver structure:
+
+U_BOOT_DRIVER(max77686_buck) = {
+        .name = "max77686 buck",
+        .id = UCLASS_REGULATOR,
+        .ops = &max77686_buck_ops,
+        .of_match = max77686_buck_ids,
+        .ofdata_to_platdata = max77686_buck_ofdata_to_platdata,
+};
+
+The device-tree buck compatibles:
+static const struct udevice_id max77686_buck_ids[] = {
+        { .compatible = "BUCK1", .data = 1 },
+        { .compatible = "BUCK2", .data = 2 },
+        ...
+        { }, (need always put null at the end)
+};
+
+For each compatible, the data is set as a number of buck. This allows to get
+the device number without parsing the compatible.
+
+The buck driver '.ofdata_to_platdata' call implementation:
+static int max77686_buck_ofdata_to_platdata(struct udevice *dev)
+{
+        struct dm_regulator_info *info = dev->uclass_priv;
+        int ret;
+
+       /* Get the regulation constraints by the common function */
+        ret = regulator_ofdata_to_platdata(dev);
+        if (ret)
+                return ret;
+
+        info->type = REGULATOR_TYPE_BUCK;
+        /**
+         * The device mode array is filled by internal driver function with
+         * the proper name for each mode id.
+         */
+        info->mode_count = max77686_buck_modes(dev2num(dev), &info->mode);
+
+        return 0;
+}
+
+And the call to regulator_ofdata_to_platdata() is responsible for filling the
+regulator output constraints, which are keepts in device-tree regulator nodes,
+e.g. 'arch/arm/dts/exynos4412-odroid.dts'
+
+For the I/O, this driver uses pmic_read/write calls, with the parent device
+as the first argument, e.g.: pmic_read(dev->parent, adr, &val, 1);
+
+8. Regulator command
+====================
+To use the pmic command, config: CONFIG_DM_REGULATOR_CMD is required.
+
+This command is based on driver model regulator api.
+User interface features:
+- list                   - list UCLASS regulator devices
+- regulator dev [id]     - show or [set] operating regulator device
+- regulator [info]       - print constraints info
+- regulator [status]     - print operating status
+- regulator [value] [-f] - print/[set] voltage value [uV] (force)
+- regulator [current]    - print/[set] current value [uA]
+- regulator [mode_id]    - print/[set] operating mode id
+- regulator [enable]     - enable the regulator output
+- regulator [disable]    - disable the regulator output
+
+If pmic device driver provides support to this another pmic uclass, then this
+command provides useful user interface. It was designed to allow safe I/O access
+to the pmic device, without the pmic documentation. If driver provide regulator
+output - value and mode info - then user can operate on it.
+
+Example of usage:
+regulator list              - look after regulator 'seq' number

doc/driver-model/pmic-framework.txt line 343

Quoted 10 lines

doc/driver-model/pmic-framework.txt new file +10 -0

+regulator dev 'seq'         - set current device
+regulator status            - show device status
+regulator info              - list device available regulation constraints
+regulator value             - prints device voltage value
+regulator value 1000000     - set device voltage value to 1000000 uV
+regulator value 1200000 -f  - if value exceeds limits - set force
+regulator mode              - print device operation mode id
+regulator mode 5            - set devices operation mode to '5' (mode id)
+
+The -f option (forcibly) or mode - only if descriptor is available

I think something like this would be clearer:

The -f option (forcibly) and 'mode' sub-command are only available if the regulator descriptor is available

doc/driver-model/pmic-framework.txt line 350

Quoted 7 lines

doc/driver-model/pmic-framework.txt new file +7 -0

+
+Note:
+The regulator descriptor, 'min' and 'max' limits prevents setting unsafe value.
+But sometimes it is useful to change the regulator value for some test - so the
+force option (-f) is available. This option is not available for change the mode
+since this depends on a pmic device design, but the required voltage value can
+change, e.g. if some hardware uses pins header.

changed.

What does "e..g. if some hardware uses pins header" mean? Can you reword that to be clearer?

Quoted 3 lines

--
1.9.1

Regards,
Simon

In the power_init_board function call, regulator driver init is called, so before compile, make sure that any power framework is defined.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 board/samsung/common/board.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

board/samsung/common/board.c +2 -3

index 2e17da8..c4cbb63 100644
@@ -21,9 +21,9 @@
 #include <asm/arch/pinmux.h>
 #include <asm/arch/power.h>
 #include <asm/arch/system.h>
-#include <power/pmic.h>
 #include <asm/arch/sromc.h>
 #include <lcd.h>
+#include <i2c.h>
 #include <samsung/misc.h>
 
 DECLARE_GLOBAL_DATA_PTR;

@@ -168,7 +168,7 @@ int board_early_init_f(void)
 }
 #endif
 
-#if defined(CONFIG_POWER)
+#if defined(CONFIG_POWER) || defined(CONFIG_DM_PMIC)
 int power_init_board(void)
 {
        set_ps_hold_ctrl();
--

1.9.1

This commit change the old pmic framework calls with the new ones.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Changes v2:
- remove board_init_i2c() call
- update regulator calls
- update headers
- samsung/misc.c: include required header

Changes v3:
- adjust regulator calls to new api

---
 board/samsung/common/misc.c   |   1 +
 board/samsung/odroid/odroid.c | 113 +++++++++++++++++++++++++++++++++---------
 2 files changed, 91 insertions(+), 23 deletions(-)

board/samsung/common/misc.c +1 -0

index 1a77c82..f0d69d4 100644
@@ -16,6 +16,7 @@
 #include <asm/arch/cpu.h>
 #include <asm/gpio.h>
 #include <linux/input.h>
+#include <dm.h>
 #include <power/pmic.h>
 #include <mmc.h>
 

board/samsung/odroid/odroid.c +11 -3

index ae41c29..aa3b0ff 100644
@@ -12,7 +12,9 @@
 #include <asm/arch/gpio.h>
 #include <asm/gpio.h>
 #include <asm/arch/cpu.h>
+#include <dm.h>
 #include <power/pmic.h>
+#include <power/regulator.h>
 #include <power/max77686_pmic.h>
 #include <errno.h>
 #include <mmc.h>

@@ -405,15 +407,62 @@ static void board_gpio_init(void)
 
 static int pmic_init_max77686(void)
 {
-       struct pmic *p = pmic_get("MAX77686_PMIC");
+       struct udevice *dev;
+       int ret;
 
-       if (pmic_probe(p))
-               return -ENODEV;
+       ret = regulator_get("VDDQ_EMMC_1.8V", &dev);
+       if (ret) {
+               error("Regulator get error: %d", ret);
+               return ret;
+       }
+
+       ret = regulator_set_value(dev, 1800000);

board/samsung/odroid/odroid.c +69 -21

+
+       ret = regulator_get("TFLASH_2.8V", &dev);
+       if (ret) {
+               error("Regulator get error: %d", ret);
+               return ret;
+       }
+
+       ret = regulator_set_value(dev, 2800000);
+       if (ret) {
+               error("Regulator %s value setting error: %d", dev->name, ret);
+               return ret;
+       }
+
+       ret = regulator_set_enable(dev, true);
+       if (ret) {
+               error("Regulator %s enable error: %d", dev->name, ret);
+               return ret;
+       }
+
+       ret = regulator_get("VDDQ_EMMC_2.8V", &dev);
+       if (ret) {
+               error("Regulator get error: %d", ret);
+               return ret;
+       }
 
-       /* Set LDO Voltage */
-       max77686_set_ldo_voltage(p, 20, 1800000);       /* LDO20 eMMC */
-       max77686_set_ldo_voltage(p, 21, 2800000);       /* LDO21 SD */
-       max77686_set_ldo_voltage(p, 22, 2800000);       /* LDO22 eMMC */
+       ret = regulator_set_value(dev, 2800000);
+       if (ret) {
+               error("Regulator %s value setting error: %d", dev->name, ret);
+               return ret;
+       }
+
+       ret = regulator_set_enable(dev, true);
+       if (ret) {
+               error("Regulator %s enable error: %d", dev->name, ret);
+               return ret;
+       }
 
        return 0;
 }

@@ -434,7 +483,6 @@ int exynos_init(void)
 
 int exynos_power_init(void)
 {
-       pmic_init(0);
        pmic_init_max77686();
 
        return 0;

@@ -443,19 +491,20 @@ int exynos_power_init(void)
 #ifdef CONFIG_USB_GADGET
 static int s5pc210_phy_control(int on)
 {
-       struct pmic *p_pmic;
-
-       p_pmic = pmic_get("MAX77686_PMIC");
-       if (!p_pmic)
-               return -ENODEV;
+       struct udevice *dev;
+       int ret;
 
-       if (pmic_probe(p_pmic))
-               return -1;
+       ret = regulator_get("VDD_UOTG_3.0V", &dev);
+       if (ret) {
+               error("Regulator get error: %d", ret);
+               return ret;
+       }
 
        if (on)
-               return max77686_set_ldo_mode(p_pmic, 12, OPMODE_ON);
+               return regulator_set_mode(dev, OPMODE_ON);
        else
-               return max77686_set_ldo_mode(p_pmic, 12, OPMODE_LPM);
+               return regulator_set_mode(dev, OPMODE_LPM);
+
 }
 
 struct s3c_plat_otg_data s5pc210_otg_data = {

@@ -472,7 +521,8 @@ struct s3c_plat_otg_data s5pc210_otg_data = {
 int board_usb_init(int index, enum usb_init_type init)
 {
 #ifdef CONFIG_CMD_USB
-       struct pmic *p_pmic;
+       struct udevice *dev;
+       int ret;
 
        /* Set Ref freq 0 => 24MHz, 1 => 26MHz*/
        /* Odroid Us have it at 24MHz, Odroid Xs at 26MHz */

@@ -490,14 +540,31 @@ int board_usb_init(int index, enum usb_init_type init)
        /* Power off and on BUCK8 for LAN9730 */
        debug("LAN9730 - Turning power buck 8 OFF and ON.\n");
 
-       p_pmic = pmic_get("MAX77686_PMIC");
-       if (p_pmic && !pmic_probe(p_pmic)) {
-               max77686_set_buck_voltage(p_pmic, 8, 750000);
-               max77686_set_buck_voltage(p_pmic, 8, 3300000);
+       ret = regulator_get("VCC_P3V3_2.85V", &dev);
+       if (ret) {
+               error("Regulator get error: %d", ret);
+               return ret;
        }
 
-#endif
+       ret = regulator_set_enable(dev, true);
+       if (ret) {
+               error("Regulator %s enable setting error: %d", dev->name, ret);
+               return ret;
+       }
+
+       ret = regulator_set_value(dev, 750000);
+       if (ret) {
+               error("Regulator %s value setting error: %d", dev->name, ret);
+               return ret;
+       }
 
+       ret = regulator_set_value(dev, 3300000);
+       if (ret) {
+               error("Regulator %s value setting error: %d", dev->name, ret);
+               return ret;
+       }
+
+#endif
        debug("USB_udc_probe\n");
        return s3c_udc_probe(&s5pc210_otg_data);
 }
--

1.9.1

Hi Przemyslaw,

Quoted 65 lines · Przemyslaw Marczak

This commit change the old pmic framework calls with the new ones.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Changes v2:
- remove board_init_i2c() call
- update regulator calls
- update headers
- samsung/misc.c: include required header

Changes v3:
- adjust regulator calls to new api

---
  board/samsung/common/misc.c   |   1 +
  board/samsung/odroid/odroid.c | 113 +++++++++++++++++++++++++++++++++---------
  2 files changed, 91 insertions(+), 23 deletions(-)

board/samsung/common/misc.c +1 -0

index 1a77c82..f0d69d4 100644
@@ -16,6 +16,7 @@
  #include <asm/arch/cpu.h>
  #include <asm/gpio.h>
  #include <linux/input.h>
+#include <dm.h>
  #include <power/pmic.h>
  #include <mmc.h>
 

board/samsung/odroid/odroid.c +16 -3

index ae41c29..aa3b0ff 100644
@@ -12,7 +12,9 @@
  #include <asm/arch/gpio.h>
  #include <asm/gpio.h>
  #include <asm/arch/cpu.h>
+#include <dm.h>
  #include <power/pmic.h>
+#include <power/regulator.h>
  #include <power/max77686_pmic.h>
  #include <errno.h>
  #include <mmc.h>

@@ -405,15 +407,62 @@ static void board_gpio_init(void)
 
  static int pmic_init_max77686(void)
  {
-       struct pmic *p = pmic_get("MAX77686_PMIC");
+       struct udevice *dev;
+       int ret;
 
-       if (pmic_probe(p))
-               return -ENODEV;
+       ret = regulator_get("VDDQ_EMMC_1.8V", &dev);
+       if (ret) {
+               error("Regulator get error: %d", ret);
+               return ret;
+       }
+
+       ret = regulator_set_value(dev, 1800000);
+       if (ret) {
+               error("Regulator %s value setting error: %d", dev->name, ret);
+               return ret;
+       }
+

board/samsung/odroid/odroid.c static void board_gpio_init(void) · line 429

Quoted 5 lines

board/samsung/odroid/odroid.c +5 -0

+       ret = regulator_set_enable(dev, true);
+       if (ret) {
+               error("Regulator %s enable error: %d", dev->name, ret);
+               return ret;
+       }

How about adding a function that finds a regulator, sets its voltage and enables it? Then you can avoid duplicating the same code 3 times.

Quoted 118 lines

board/samsung/odroid/odroid.c +62 -20

+
+       ret = regulator_get("TFLASH_2.8V", &dev);
+       if (ret) {
+               error("Regulator get error: %d", ret);
+               return ret;
+       }
+
+       ret = regulator_set_value(dev, 2800000);
+       if (ret) {
+               error("Regulator %s value setting error: %d", dev->name, ret);
+               return ret;
+       }
+
+       ret = regulator_set_enable(dev, true);
+       if (ret) {
+               error("Regulator %s enable error: %d", dev->name, ret);
+               return ret;
+       }
+
+       ret = regulator_get("VDDQ_EMMC_2.8V", &dev);
+       if (ret) {
+               error("Regulator get error: %d", ret);
+               return ret;
+       }
 
-       /* Set LDO Voltage */
-       max77686_set_ldo_voltage(p, 20, 1800000);       /* LDO20 eMMC */
-       max77686_set_ldo_voltage(p, 21, 2800000);       /* LDO21 SD */
-       max77686_set_ldo_voltage(p, 22, 2800000);       /* LDO22 eMMC */
+       ret = regulator_set_value(dev, 2800000);
+       if (ret) {
+               error("Regulator %s value setting error: %d", dev->name, ret);
+               return ret;
+       }
+
+       ret = regulator_set_enable(dev, true);
+       if (ret) {
+               error("Regulator %s enable error: %d", dev->name, ret);
+               return ret;
+       }
 
         return 0;
  }

@@ -434,7 +483,6 @@ int exynos_init(void)
 
  int exynos_power_init(void)
  {
-       pmic_init(0);
         pmic_init_max77686();
 
         return 0;

@@ -443,19 +491,20 @@ int exynos_power_init(void)
  #ifdef CONFIG_USB_GADGET
  static int s5pc210_phy_control(int on)
  {
-       struct pmic *p_pmic;
-
-       p_pmic = pmic_get("MAX77686_PMIC");
-       if (!p_pmic)
-               return -ENODEV;
+       struct udevice *dev;
+       int ret;
 
-       if (pmic_probe(p_pmic))
-               return -1;
+       ret = regulator_get("VDD_UOTG_3.0V", &dev);
+       if (ret) {
+               error("Regulator get error: %d", ret);
+               return ret;
+       }
 
         if (on)
-               return max77686_set_ldo_mode(p_pmic, 12, OPMODE_ON);
+               return regulator_set_mode(dev, OPMODE_ON);
         else
-               return max77686_set_ldo_mode(p_pmic, 12, OPMODE_LPM);
+               return regulator_set_mode(dev, OPMODE_LPM);
+
  }
 
  struct s3c_plat_otg_data s5pc210_otg_data = {

@@ -472,7 +521,8 @@ struct s3c_plat_otg_data s5pc210_otg_data = {
  int board_usb_init(int index, enum usb_init_type init)
  {
  #ifdef CONFIG_CMD_USB
-       struct pmic *p_pmic;
+       struct udevice *dev;
+       int ret;
 
         /* Set Ref freq 0 => 24MHz, 1 => 26MHz*/
         /* Odroid Us have it at 24MHz, Odroid Xs at 26MHz */

@@ -490,14 +540,31 @@ int board_usb_init(int index, enum usb_init_type init)
         /* Power off and on BUCK8 for LAN9730 */
         debug("LAN9730 - Turning power buck 8 OFF and ON.\n");
 
-       p_pmic = pmic_get("MAX77686_PMIC");
-       if (p_pmic && !pmic_probe(p_pmic)) {
-               max77686_set_buck_voltage(p_pmic, 8, 750000);
-               max77686_set_buck_voltage(p_pmic, 8, 3300000);
+       ret = regulator_get("VCC_P3V3_2.85V", &dev);
+       if (ret) {
+               error("Regulator get error: %d", ret);
+               return ret;
         }
 
-#endif
+       ret = regulator_set_enable(dev, true);
+       if (ret) {
+               error("Regulator %s enable setting error: %d", dev->name, ret);
+               return ret;
+       }
+
+       ret = regulator_set_value(dev, 750000);
+       if (ret) {
+               error("Regulator %s value setting error: %d", dev->name, ret);
+               return ret;
+       }
 

Quoted 11 lines

board/samsung/odroid/odroid.c +7 -1

+       ret = regulator_set_value(dev, 3300000);
+       if (ret) {
+               error("Regulator %s value setting error: %d", dev->name, ret);
+               return ret;
+       }
+
+#endif
         debug("USB_udc_probe\n");
         return s3c_udc_probe(&s5pc210_otg_data);
  }
--

Quoted 2 lines

1.9.1

Regards,
Simon

Adding regulators subnode to fdt max77686 node, allows properly init regulators by the max77686 regulator driver. This enables the complete functionality of the regulator command.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Changes V2:
- odroid: dts: remove pmic alias

Changes V3:
- none

---
 arch/arm/dts/exynos4412-odroid.dts | 247 +++++++++++++++++++++++++++++++++++++
 1 file changed, 247 insertions(+)

This change enables the configs required to init and setup max77686 regulator driver, using the new driver model pmic and regulator API.

This commits enables:
- CONFIG_ERRNO_STR
- CONFIG_DM_PMIC
- CONFIG_DM_PMIC_CMD
- CONFIG_DM_PMIC_MAX77686
- CONFIG_DM_REGULATOR
- CONFIG_DM_REGULATOR_CMD
- CONFIG_DM_REGULATOR_MAX77686

And removes the unused:
- CONFIG_DM_I2C_COMPAT
- CONFIG_POWER
- CONFIG_POWER_I2C
- CONFIG_POWER_MAX77686

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Changes V2:
- config: enable dm i2c; cleanup
- remove CONFIG_DM_I2C_COMPAT
- enable regulator command

Changes V3:
- move options to defconfig

---
 configs/odroid_defconfig | 8 +++++++-
 include/configs/odroid.h | 5 -----
 2 files changed, 7 insertions(+), 6 deletions(-)

configs/odroid_defconfig +7 -1

index d32b5b5..1e29abe 100644
@@ -4,5 +4,11 @@ CONFIG_TARGET_ODROID=y
 CONFIG_OF_CONTROL=y
 CONFIG_DEFAULT_DEVICE_TREE="exynos4412-odroid"
 CONFIG_DM_I2C=y
-CONFIG_DM_I2C_COMPAT=y
 # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
+CONFIG_ERRNO_STR=y
+CONFIG_DM_PMIC=y
+CONFIG_DM_PMIC_CMD=y
+CONFIG_DM_PMIC_MAX77686=y
+CONFIG_DM_REGULATOR=y
+CONFIG_DM_REGULATOR_CMD=y
+CONFIG_DM_REGULATOR_MAX77686=y

include/configs/odroid.h +0 -6

index 5ee0abe..3874baa 100644
@@ -182,11 +182,6 @@
 #define CONFIG_SYS_I2C_S3C24X0_SPEED   100000
 #define CONFIG_SYS_I2C_S3C24X0_SLAVE   0
 
-/* POWER */
-#define CONFIG_POWER
-#define CONFIG_POWER_I2C
-#define CONFIG_POWER_MAX77686
-
 /* GPT */
 #define CONFIG_RANDOM_UUID
 
--

1.9.1

Quoted 18 lines · Przemyslaw Marczak

This change enables the configs required to init and setup max77686 regulator driver, using the new driver model pmic and regulator API.

This commits enables:
- CONFIG_ERRNO_STR
- CONFIG_DM_PMIC
- CONFIG_DM_PMIC_CMD
- CONFIG_DM_PMIC_MAX77686
- CONFIG_DM_REGULATOR
- CONFIG_DM_REGULATOR_CMD
- CONFIG_DM_REGULATOR_MAX77686

And removes the unused:
- CONFIG_DM_I2C_COMPAT
- CONFIG_POWER
- CONFIG_POWER_I2C
- CONFIG_POWER_MAX77686

Quoted 1 line

Signed-off-by Przemyslaw Marczak <[email protected]>

Acked-by Simon Glass <[email protected]>

(but pelase rename the commands so that they are CONFIG_CMD_DM_PCI and CONFIG_CMD_DM_REGULATOR)

Quoted 46 lines

---
Changes V2:
- config: enable dm i2c; cleanup
- remove CONFIG_DM_I2C_COMPAT
- enable regulator command

Changes V3:
- move options to defconfig

---
  configs/odroid_defconfig | 8 +++++++-
  include/configs/odroid.h | 5 -----
  2 files changed, 7 insertions(+), 6 deletions(-)

configs/odroid_defconfig +7 -1

index d32b5b5..1e29abe 100644
@@ -4,5 +4,11 @@ CONFIG_TARGET_ODROID=y
  CONFIG_OF_CONTROL=y
  CONFIG_DEFAULT_DEVICE_TREE="exynos4412-odroid"
  CONFIG_DM_I2C=y
-CONFIG_DM_I2C_COMPAT=y
  # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
+CONFIG_ERRNO_STR=y
+CONFIG_DM_PMIC=y
+CONFIG_DM_PMIC_CMD=y
+CONFIG_DM_PMIC_MAX77686=y
+CONFIG_DM_REGULATOR=y
+CONFIG_DM_REGULATOR_CMD=y
+CONFIG_DM_REGULATOR_MAX77686=y

include/configs/odroid.h +0 -5

index 5ee0abe..3874baa 100644
@@ -182,11 +182,6 @@
  #define CONFIG_SYS_I2C_S3C24X0_SPEED   100000
  #define CONFIG_SYS_I2C_S3C24X0_SLAVE   0
 
-/* POWER */
-#define CONFIG_POWER
-#define CONFIG_POWER_I2C
-#define CONFIG_POWER_MAX77686
-
  /* GPT */
  #define CONFIG_RANDOM_UUID
 

Quoted 3 lines

--
1.9.1

Hello,
Here is the third RFC version of the new PMIC framework.Big thanks to Simon Glass, your comments were really helpful, and I think, that this version is much more better to discuss, than the previous. The changes made in this version are described below each commit. Sorry that I didn't reply to each patch, I agreed with most and just started the work.

Best regards

Przemyslaw Marczak (17):
exynos5: fix build break by adding CONFIG_POWER
fdt_ro.c: add new function: fdt_node_check_prop_compatible()
dm: core: lists.c: add new function lists_bind_fdt_by_prop()
lib: Kconfig: add entry for errno_str() function
dm: pmic: add implementation of driver model pmic uclass
dm: regulator: add implementation of driver model regulator uclass
dm: pmic: add pmic command
dm: regulator: add regulator command
pmic: max77686 set the same compatible as in the kernel
dm: pmic: add max77686 pmic driver
dm: regulator: add max77686 regulator driver
dm: regulator: add fixed voltage regulator driver
doc: driver-model: pmic and regulator uclass documentation
dm: board:samsung: power_init_board: add requirement of CONFIG_DM_PMIC
odroid: board: add support to dm pmic api
odroid: dts: add 'voltage-regulators' description to max77686 node
odroid: config: enable dm pmic, dm regulator and max77686 driver

  Makefile                             |   1 +
  arch/arm/dts/exynos4412-odroid.dts   | 249 +++++++++-
  arch/arm/dts/exynos4412-trats2.dts   |   2 +-
  arch/arm/dts/exynos5250-smdk5250.dts |   2 +-
  arch/arm/dts/exynos5250-snow.dts     |   2 +-
  board/samsung/common/board.c         |   4 +-
  board/samsung/common/misc.c          |   1 +
  board/samsung/odroid/odroid.c        | 113 ++++-
  common/Kconfig                       |  36 ++
  common/Makefile                      |   4 +
  common/cmd_pmic.c                    | 210 +++++++++
  common/cmd_regulator.c               | 385 +++++++++++++++
  configs/odroid_defconfig             |   8 +-
  doc/driver-model/pmic-framework.txt  | 350 ++++++++++++++
  drivers/core/lists.c                 |  28 +-
  drivers/power/Kconfig                | 124 ++++-
  drivers/power/Makefile               |   3 +-
  drivers/power/pmic-uclass.c          | 130 ++++++
  drivers/power/pmic/Makefile          |   1 +
  drivers/power/pmic/max77686.c        |  76 +++
  drivers/power/pmic/pmic_max77686.c   |   2 +-
  drivers/power/regulator-uclass.c     | 219 +++++++++
  drivers/power/regulator/Makefile     |   9 +
  drivers/power/regulator/fixed.c      | 124 +++++
  drivers/power/regulator/max77686.c   | 876 +++++++++++++++++++++++++++++++++++
  include/configs/exynos5-common.h     |   4 +
  include/configs/odroid.h             |   5 -
  include/dm/lists.h                   |  18 +
  include/dm/uclass-id.h               |   4 +
  include/libfdt.h                     |  27 ++
  include/power/max77686_pmic.h        |  26 +-
  include/power/pmic.h                 | 210 +++++++++
  include/power/regulator.h            | 259 +++++++++++
  lib/Kconfig                          |   8 +
  lib/fdtdec.c                         |   2 +-
  lib/libfdt/fdt_ro.c                  |  14 +-
  36 files changed, 3481 insertions(+), 55 deletions(-)

create mode 100644 common/cmd_pmic.c
create mode 100644 common/cmd_regulator.c create mode 100644 doc/driver-model/pmic-framework.txt create mode 100644 drivers/power/pmic-uclass.c create mode 100644 drivers/power/pmic/max77686.c create mode 100644 drivers/power/regulator-uclass.c create mode 100644 drivers/power/regulator/Makefile create mode 100644 drivers/power/regulator/fixed.c create mode 100644 drivers/power/regulator/max77686.c create mode 100644 include/power/regulator.h

Hello,
Here is the third RFC version of the new PMIC framework.Big thanks to Simon Glass, your comments were really helpful, and I think, that this version is much more better to discuss, than the previous. The changes made in this version are described below each commit. Sorry that I didn't reply to each patch, I agreed with most and just started the work.

Best regards

Przemyslaw Marczak (17):
exynos5: fix build break by adding CONFIG_POWER
fdt_ro.c: add new function: fdt_node_check_prop_compatible()
dm: core: lists.c: add new function lists_bind_fdt_by_prop()
lib: Kconfig: add entry for errno_str() function
dm: pmic: add implementation of driver model pmic uclass
dm: regulator: add implementation of driver model regulator uclass
dm: pmic: add pmic command
dm: regulator: add regulator command
pmic: max77686 set the same compatible as in the kernel
dm: pmic: add max77686 pmic driver
dm: regulator: add max77686 regulator driver
dm: regulator: add fixed voltage regulator driver
doc: driver-model: pmic and regulator uclass documentation
dm: board:samsung: power_init_board: add requirement of CONFIG_DM_PMIC
odroid: board: add support to dm pmic api
odroid: dts: add 'voltage-regulators' description to max77686 node
odroid: config: enable dm pmic, dm regulator and max77686 driver

 Makefile                             |   1 +
 arch/arm/dts/exynos4412-odroid.dts   | 249 +++++++++-
 arch/arm/dts/exynos4412-trats2.dts   |   2 +-
 arch/arm/dts/exynos5250-smdk5250.dts |   2 +-
 arch/arm/dts/exynos5250-snow.dts     |   2 +-
 board/samsung/common/board.c         |   4 +-
 board/samsung/common/misc.c          |   1 +
 board/samsung/odroid/odroid.c        | 113 ++++-
 common/Kconfig                       |  36 ++
 common/Makefile                      |   4 +
 common/cmd_pmic.c                    | 210 +++++++++
 common/cmd_regulator.c               | 385 +++++++++++++++
 configs/odroid_defconfig             |   8 +-
 doc/driver-model/pmic-framework.txt  | 350 ++++++++++++++
 drivers/core/lists.c                 |  28 +-
 drivers/power/Kconfig                | 124 ++++-
 drivers/power/Makefile               |   3 +-
 drivers/power/pmic-uclass.c          | 130 ++++++
 drivers/power/pmic/Makefile          |   1 +
 drivers/power/pmic/max77686.c        |  76 +++
 drivers/power/pmic/pmic_max77686.c   |   2 +-
 drivers/power/regulator-uclass.c     | 219 +++++++++
 drivers/power/regulator/Makefile     |   9 +
 drivers/power/regulator/fixed.c      | 124 +++++
 drivers/power/regulator/max77686.c   | 876 +++++++++++++++++++++++++++++++++++
 include/configs/exynos5-common.h     |   4 +
 include/configs/odroid.h             |   5 -
 include/dm/lists.h                   |  18 +
 include/dm/uclass-id.h               |   4 +
 include/libfdt.h                     |  27 ++
 include/power/max77686_pmic.h        |  26 +-
 include/power/pmic.h                 | 210 +++++++++
 include/power/regulator.h            | 259 +++++++++++
 lib/Kconfig                          |   8 +
 lib/fdtdec.c                         |   2 +-
 lib/libfdt/fdt_ro.c                  |  14 +-
 36 files changed, 3481 insertions(+), 55 deletions(-)
 create mode 100644 common/cmd_pmic.c
 create mode 100644 common/cmd_regulator.c
 create mode 100644 doc/driver-model/pmic-framework.txt
 create mode 100644 drivers/power/pmic-uclass.c
 create mode 100644 drivers/power/pmic/max77686.c
 create mode 100644 drivers/power/regulator-uclass.c
 create mode 100644 drivers/power/regulator/Makefile
 create mode 100644 drivers/power/regulator/fixed.c
 create mode 100644 drivers/power/regulator/max77686.c
 create mode 100644 include/power/regulator.h

--
1.9.1

Hi Przemyslaw,

Quoted 6 lines · Przemyslaw Marczak

Hello,
Here is the third RFC version of the new PMIC framework.Big thanks to Simon Glass, your comments were really helpful, and I think, that this version is much more better to discuss, than the previous. The changes made in this version are described below each commit. Sorry that I didn't reply to each patch, I agreed with most and just started the work.

This is looking really good. Here are a few overall comments.

1. There is one oddity that I'd like to address before merging.

I don't think the fdt_node_check_prop_compatible() is a good idea, nor necessary. I don't think we should consider the regulator-compatible property to be a compatible string. It has values like LDO8, LDO9 and these don't look like compatible strings, which are normally unique and point to a driver. Here they point to a particular device.

A similar problem is faced in pinctrl and if you look at gpio_exynos_bind() you will see that it works through the sub-nodes, creating devices as needed.

I don't think using udevice_id is right here either.

Here is my suggestion:

a. Create a new structure like this:

struct pmic_child_info {
const char *prefix; // "LDO" or "BUCK"
const char *driver_name; // "max77686_ldo" or "max77686_buck"
};

b. Pass a list of these to pmic_child_node_scan(). In your case there will be three entries, one for LDO, one for BUCK, plus a NULL termination entry,

c. It can work through the subnodes looking for the given prefixes. It then calls device_bind_driver() on each. Then it changes the returned device's of_data to hold the correct value (obtained with strtol() on the part of the name that follows the prefix - e.g. 17 for "LDO17"). This will be easier if you rebase on u-boot-dm/usb-working, where the data is just a long, not a device tree pointer.

d. Now you have the same effect as before, but you can drop the tables like max77686_ldo_ids[] and avoid misappropriating driver model's device lookup.

2. Should we put the regulator stuff in drivers/regulator, as with Linux?

3. Can you please bring in the regulator and pmic device tree binding files, plus max77686?

4. We really do need tests! I suspect that you could create a sandbox I2C pmic that has a few registers and regulators. See i2c_eeprom_emul.c for a basic example. Then you can write some tests that find the pmi,c find the regulator, read and write a few registers, and read and write a few regulators. That would be enough test coverage to get us started. I know this is different from previous U-Boot policy, but tests are a big win during development and also for years to come (as people can change the framework and have some confidence that they did not break anything).

It can be a follow-on patch separate from your series but I'm really not keen on bringing in a major new driver model framework with no tests. If you are struggling for time, let me know and I can try to help by writing a sandbox I2C PMIC for example.

Regards,
Simon

Quoted 70 lines

Best regards

Przemyslaw Marczak (17):
exynos5: fix build break by adding CONFIG_POWER
fdt_ro.c: add new function: fdt_node_check_prop_compatible()
dm: core: lists.c: add new function lists_bind_fdt_by_prop()
lib: Kconfig: add entry for errno_str() function
dm: pmic: add implementation of driver model pmic uclass
dm: regulator: add implementation of driver model regulator uclass
dm: pmic: add pmic command
dm: regulator: add regulator command
pmic: max77686 set the same compatible as in the kernel
dm: pmic: add max77686 pmic driver
dm: regulator: add max77686 regulator driver
dm: regulator: add fixed voltage regulator driver
doc: driver-model: pmic and regulator uclass documentation
dm: board:samsung: power_init_board: add requirement of CONFIG_DM_PMIC
odroid: board: add support to dm pmic api
odroid: dts: add 'voltage-regulators' description to max77686 node
odroid: config: enable dm pmic, dm regulator and max77686 driver

  Makefile                             |   1 +
  arch/arm/dts/exynos4412-odroid.dts   | 249 +++++++++-
  arch/arm/dts/exynos4412-trats2.dts   |   2 +-
  arch/arm/dts/exynos5250-smdk5250.dts |   2 +-
  arch/arm/dts/exynos5250-snow.dts     |   2 +-
  board/samsung/common/board.c         |   4 +-
  board/samsung/common/misc.c          |   1 +
  board/samsung/odroid/odroid.c        | 113 ++++-
  common/Kconfig                       |  36 ++
  common/Makefile                      |   4 +
  common/cmd_pmic.c                    | 210 +++++++++
  common/cmd_regulator.c               | 385 +++++++++++++++
  configs/odroid_defconfig             |   8 +-
  doc/driver-model/pmic-framework.txt  | 350 ++++++++++++++
  drivers/core/lists.c                 |  28 +-
  drivers/power/Kconfig                | 124 ++++-
  drivers/power/Makefile               |   3 +-
  drivers/power/pmic-uclass.c          | 130 ++++++
  drivers/power/pmic/Makefile          |   1 +
  drivers/power/pmic/max77686.c        |  76 +++
  drivers/power/pmic/pmic_max77686.c   |   2 +-
  drivers/power/regulator-uclass.c     | 219 +++++++++
  drivers/power/regulator/Makefile     |   9 +
  drivers/power/regulator/fixed.c      | 124 +++++
  drivers/power/regulator/max77686.c   | 876 +++++++++++++++++++++++++++++++++++
  include/configs/exynos5-common.h     |   4 +
  include/configs/odroid.h             |   5 -
  include/dm/lists.h                   |  18 +
  include/dm/uclass-id.h               |   4 +
  include/libfdt.h                     |  27 ++
  include/power/max77686_pmic.h        |  26 +-
  include/power/pmic.h                 | 210 +++++++++
  include/power/regulator.h            | 259 +++++++++++
  lib/Kconfig                          |   8 +
  lib/fdtdec.c                         |   2 +-
  lib/libfdt/fdt_ro.c                  |  14 +-
  36 files changed, 3481 insertions(+), 55 deletions(-)

create mode 100644 common/cmd_pmic.c
create mode 100644 common/cmd_regulator.c create mode 100644 doc/driver-model/pmic-framework.txt create mode 100644 drivers/power/pmic-uclass.c create mode 100644 drivers/power/pmic/max77686.c create mode 100644 drivers/power/regulator-uclass.c create mode 100644 drivers/power/regulator/Makefile create mode 100644 drivers/power/regulator/fixed.c create mode 100644 drivers/power/regulator/max77686.c create mode 100644 include/power/regulator.h

Quoted 3 lines

--
1.9.1

Hello Simon,

Quoted 16 lines · Simon Glass

Hi Przemyslaw,

Quoted 7 lines · Przemyslaw Marczak

Hello,
Here is the third RFC version of the new PMIC framework.Big thanks to Simon Glass, your comments were really helpful, and I think, that this version is much more better to discuss, than the previous. The changes made in this version are described below each commit. Sorry that I didn't reply to each patch, I agreed with most and just started the work.

This is looking really good. Here are a few overall comments.

Quoted 8 lines

1. There is one oddity that I'd like to address before merging.

I don't think the fdt_node_check_prop_compatible() is a good idea, nor necessary. I don't think we should consider the regulator-compatible property to be a compatible string. It has values like LDO8, LDO9 and these don't look like compatible strings, which are normally unique and point to a driver. Here they point to a particular device.

Right, those compatibles don't point to a driver. This is a specific case of use from the kernel.

Quoted 6 lines

A similar problem is faced in pinctrl and if you look at gpio_exynos_bind() you will see that it works through the sub-nodes, creating devices as needed.

I don't think using udevice_id is right here either.

Yes, I know how it's done. But we haven't the compatibles for each GPIO as for the regulators, and each GPIO driver, bind the childs by its own implementation.

I tried to reuse the existing code. It was nice, but I missed one thing...if there are more, than one driver with the same e.g. "LDO1" compatible, then the first one will bind - and it could be the wrong one...

But, it could be tune-up, to get the right drivers list by arg.

Quoted 4 lines

Here is my suggestion:

a. Create a new structure like this:

Quoted 9 lines

struct pmic_child_info {
const char *prefix; // "LDO" or "BUCK"
const char *driver_name; // "max77686_ldo" or "max77686_buck"
};

b. Pass a list of these to pmic_child_node_scan(). In your case there will be three entries, one for LDO, one for BUCK, plus a NULL termination entry,

Ok, this could be good.

Quoted 6 lines

c. It can work through the subnodes looking for the given prefixes. It then calls device_bind_driver() on each. Then it changes the returned device's of_data to hold the correct value (obtained with strtol() on the part of the name that follows the prefix - e.g. 17 for "LDO17"). This will be easier if you rebase on u-boot-dm/usb-working, where the data is just a long, not a device tree pointer.

Yes, it's easy. I made something like this in the first version of this patchset, to parse the nodes and fill the max77686 regulator descriptors in function get_desc_for_compat(), from this patch:
http://lists.denx.de/pipermail/u-boot/2014-October/191024.html

Quoted 4 lines

d. Now you have the same effect as before, but you can drop the tables like max77686_ldo_ids[] and avoid misappropriating driver model's device lookup.

Yes, this is an advantage.

I wonder about the usage of "struct udevice_id", which should contain the data, defined only by the driver, right?

In the method you mentioned, we bind the pmic childs and then we modify the "dev->of_id->data" by putting the regulator number from matched prefix in it.

This is ok, but I think, that the second part of this idea should be done by the driver. I think, that the external function shouldn't modify this driver data on bind.

This could be solved by leave this job for the device driver. For example on max77686, we could leave the dev->of_id as null and use dev->priv for the node prefix id.

So, it's not a big problem.

Here, I would like mention the problem with the regulator_get() by name, for which we want use the "regulator-name" constraint.

For this version, it requires probing of all regulator devices, since it uses the dev->uclass_priv, which I think, is a good place to provide the framework-specific structure type for regulator constraints and mode descriptors.

What about moving it back to dev->platdata? Then:
- the structure type: "dm_regulator_info", will move to "dm_regulator_platdata"

- only its .name field could be assigned at regulator bind stage from the "regulator-name" constraint

- the bind could fail, when the constraint name not found

- the rest of constraints will assign in .ofdata_to_platdata() function call as it is at the present version

Then, we also don't need probe each regulator device, when using regulator_get() function.

Quoted 3 lines

2. Should we put the regulator stuff in drivers/regulator, as with Linux?

I will do this in the next version.

Quoted 3 lines

3. Can you please bring in the regulator and pmic device tree binding files, plus max77686?

Right, I forgot about this.

Quoted 16 lines

4. We really do need tests! I suspect that you could create a sandbox I2C pmic that has a few registers and regulators. See i2c_eeprom_emul.c for a basic example. Then you can write some tests that find the pmi,c find the regulator, read and write a few registers, and read and write a few regulators. That would be enough test coverage to get us started. I know this is different from previous U-Boot policy, but tests are a big win during development and also for years to come (as people can change the framework and have some confidence that they did not break anything).

It can be a follow-on patch separate from your series but I'm really not keen on bringing in a major new driver model framework with no tests. If you are struggling for time, let me know and I can try to help by writing a sandbox I2C PMIC for example.

Quoted 3 lines

Regards,
Simon

Ok, I will add sandbox drivers for this all, but first let's prepare the final patchset version of this framework, and then I will send a separate patchset for sandbox before merge of this part.

Quoted 73 lines

Quoted 73 lines

Best regards

Przemyslaw Marczak (17):
exynos5: fix build break by adding CONFIG_POWER
fdt_ro.c: add new function: fdt_node_check_prop_compatible()
dm: core: lists.c: add new function lists_bind_fdt_by_prop()
lib: Kconfig: add entry for errno_str() function
dm: pmic: add implementation of driver model pmic uclass
dm: regulator: add implementation of driver model regulator uclass
dm: pmic: add pmic command
dm: regulator: add regulator command
pmic: max77686 set the same compatible as in the kernel
dm: pmic: add max77686 pmic driver
dm: regulator: add max77686 regulator driver
dm: regulator: add fixed voltage regulator driver
doc: driver-model: pmic and regulator uclass documentation
dm: board:samsung: power_init_board: add requirement of CONFIG_DM_PMIC
odroid: board: add support to dm pmic api
odroid: dts: add 'voltage-regulators' description to max77686 node
odroid: config: enable dm pmic, dm regulator and max77686 driver

  Makefile                             |   1 +
  arch/arm/dts/exynos4412-odroid.dts   | 249 +++++++++-
  arch/arm/dts/exynos4412-trats2.dts   |   2 +-
  arch/arm/dts/exynos5250-smdk5250.dts |   2 +-
  arch/arm/dts/exynos5250-snow.dts     |   2 +-
  board/samsung/common/board.c         |   4 +-
  board/samsung/common/misc.c          |   1 +
  board/samsung/odroid/odroid.c        | 113 ++++-
  common/Kconfig                       |  36 ++
  common/Makefile                      |   4 +
  common/cmd_pmic.c                    | 210 +++++++++
  common/cmd_regulator.c               | 385 +++++++++++++++
  configs/odroid_defconfig             |   8 +-
  doc/driver-model/pmic-framework.txt  | 350 ++++++++++++++
  drivers/core/lists.c                 |  28 +-
  drivers/power/Kconfig                | 124 ++++-
  drivers/power/Makefile               |   3 +-
  drivers/power/pmic-uclass.c          | 130 ++++++
  drivers/power/pmic/Makefile          |   1 +
  drivers/power/pmic/max77686.c        |  76 +++
  drivers/power/pmic/pmic_max77686.c   |   2 +-
  drivers/power/regulator-uclass.c     | 219 +++++++++
  drivers/power/regulator/Makefile     |   9 +
  drivers/power/regulator/fixed.c      | 124 +++++
  drivers/power/regulator/max77686.c   | 876
+++++++++++++++++++++++++++++++++++
  include/configs/exynos5-common.h     |   4 +
  include/configs/odroid.h             |   5 -
  include/dm/lists.h                   |  18 +
  include/dm/uclass-id.h               |   4 +
  include/libfdt.h                     |  27 ++
  include/power/max77686_pmic.h        |  26 +-
  include/power/pmic.h                 | 210 +++++++++
  include/power/regulator.h            | 259 +++++++++++
  lib/Kconfig                          |   8 +
  lib/fdtdec.c                         |   2 +-
  lib/libfdt/fdt_ro.c                  |  14 +-
  36 files changed, 3481 insertions(+), 55 deletions(-)

create mode 100644 common/cmd_pmic.c
create mode 100644 common/cmd_regulator.c create mode 100644 doc/driver-model/pmic-framework.txt create mode 100644 drivers/power/pmic-uclass.c create mode 100644 drivers/power/pmic/max77686.c create mode 100644 drivers/power/regulator-uclass.c create mode 100644 drivers/power/regulator/Makefile create mode 100644 drivers/power/regulator/fixed.c create mode 100644 drivers/power/regulator/max77686.c create mode 100644 include/power/regulator.h

Quoted 4 lines

Quoted 3 lines

--
1.9.1

Thank you again for the review!
I will send the next version probably on 7-8-th April.

Move the configs listed below from exynos5-dt-common.h to exynos5-common.h:
- CONFIG_POWER
- CONFIG_POWER_I2C
fixes build break for Arndale and Smdk5250 boards.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 include/configs/exynos5-common.h | 4 ++++
 1 file changed, 4 insertions(+)

Quoted 6 lines · Przemyslaw Marczak

Move the configs listed below from exynos5-dt-common.h to exynos5-common.h:
- CONFIG_POWER
- CONFIG_POWER_I2C
fixes build break for Arndale and Smdk5250 boards.

Quoted 4 lines

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 include/configs/exynos5-common.h | 4 ++++
 1 file changed, 4 insertions(+)

Acked-by Simon Glass <[email protected]>

This config name was never used, because the present pmic command was precompiled for the CONFIG_POWER.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 include/configs/exynos4-common.h | 1 -
 1 file changed, 1 deletion(-)

Quoted 7 lines · Przemyslaw Marczak

This config name was never used, because the present pmic command was precompiled for the CONFIG_POWER.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 include/configs/exynos4-common.h | 1 -
 1 file changed, 1 deletion(-)

Acked-by Simon Glass <[email protected]>

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 lib/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

Quoted 4 lines · Przemyslaw Marczak

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 lib/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

(please make sure to always add a commit message)

Acked-by Simon Glass <[email protected]>

This commit introduces the PMIC uclass implementation. It allows providing the basic I/O interface for PMIC devices. For the multi-function PMIC devices, this can be used as I/O parent device, for each IC's interface. Then, each PMIC particular function can be provided by the child device's operations, and the child devices will use its parent for read/write by the common API.

Core files:
- 'include/power/pmic.h'
- 'drivers/power/pmic/pmic-uclass.c'

The old pmic framework is still kept and is independent.

For more detailed informations, please look into the header file.

Changes:
- new uclass-id: UCLASS_PMIC
- new config: CONFIG_DM_PMIC

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Changes V2:
- pmic uclass: adjust uclass code to the mainline changes
- pmic uclass: remove pmic_i2c and pmic_spi
- pmic uclass: modify pmic_platdata
- pmic uclass: add pmic_if_* functions
- pmic uclass: remove pmic_init_dm()
- pmic uclass: cleanup
- pmic.h: define pmic ops structure (read/write operations)
- pmic.h: add comments to functions

Changes V3:
- pmic-uclass.c and pmic.h:
-- remove pmic_if_* functions
-- add new function pmic_child_node_scan()
- add Kconfig entry

Changes V4:
- move drivers/power/pmic-uclass.c to drivers/power/pmic/pmic-uclass.c
- move DM_PMIC Kconfig entry: drivers/power/Kconfig to drivers/power/pmic/Kconfig
- drivers/power/Kconfig: Add menu "Power" and include pmic Kconfig
- Kconfig: provide only the general information about the PMIC
- pmic-uclass.c: add pmic_bind_childs()
- pmic-uclass.c: add debug
- pmic-uclass.c: cleanup includes
- pmic-uclass.c: remove pmic_get_uclass_ops() and use of dev_get_driver_ops()
- pmic-uclass.c: use of uclass_get_device_by_name()
- pmic-uclass.c: add str_get_num() - for get number from string
- include/power/pmic.h - start comments rewording
- power/pmic.h: comments update

---
 drivers/power/Kconfig            |   6 ++
 drivers/power/pmic/Kconfig       |  11 +++
 drivers/power/pmic/Makefile      |   1 +
 drivers/power/pmic/pmic-uclass.c | 158 ++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h           |   3 +
 include/power/pmic.h             | 189 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 368 insertions(+)
 create mode 100644 drivers/power/pmic/Kconfig
 create mode 100644 drivers/power/pmic/pmic-uclass.c

drivers/power/Kconfig +6 -0

index f8f0239..d03626e 100644
@@ -1,3 +1,7 @@
+menu "Power"
+
+source "drivers/power/pmic/Kconfig"
+
 config AXP221_POWER
        boolean "axp221 / axp223 pmic support"
        depends on MACH_SUN6I || MACH_SUN8I

@@ -73,3 +77,5 @@ config AXP221_ELDO3_VOLT
        disable eldo3. On some A31(s) tablets it might be used to supply
        1.2V for the SSD2828 chip (converter of parallel LCD interface
        into MIPI DSI).
+
+endmenu

drivers/power/pmic/Kconfig new file +11 -0

new file mode 100644
index 0000000..d06d632
@@ -0,0 +1,11 @@
+config DM_PMIC
+       bool "Enable Driver Model for PMIC drivers (UCLASS_PMIC)"
+       depends on DM
+       ---help---
+       This config enables the driver-model PMIC support.
+       UCLASS_PMIC - designed to provide an I/O interface for PMIC devices.
+       For the multi-function PMIC devices, this can be used as parent I/O
+       device for each IC's interface. Then, each children uses its parent
+       for read/write. For detailed description, please refer to the files:
+       - 'drivers/power/pmic/pmic-uclass.c'
+       - 'include/power/pmic.h'

drivers/power/pmic/Makefile +1 -0

index 985cfdb..594f620 100644
@@ -5,6 +5,7 @@
 # SPDX-License-Identifier:     GPL-2.0+
 #
 
+obj-$(CONFIG_DM_PMIC) += pmic-uclass.o
 obj-$(CONFIG_POWER_LTC3676) += pmic_ltc3676.o
 obj-$(CONFIG_POWER_MAX8998) += pmic_max8998.o
 obj-$(CONFIG_POWER_MAX8997) += pmic_max8997.o

drivers/power/pmic/pmic-uclass.c new file +102 -0

new file mode 100644
index 0000000..d82d3da
@@ -0,0 +1,158 @@
+/*
+ * Copyright (C) 2014-2015 Samsung Electronics
+ * Przemyslaw Marczak <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <fdtdec.h>
+#include <errno.h>
+#include <dm.h>
+#include <dm/lists.h>
+#include <dm/device-internal.h>
+#include <dm/uclass-internal.h>
+#include <power/pmic.h>
+#include <linux/ctype.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static ulong str_get_num(const char *ptr, const char *maxptr)
+{
+       if (!ptr || !maxptr)
+               return 0;
+
+       while (!isdigit(*ptr) && ptr++ < maxptr);
+
+       return simple_strtoul(ptr, NULL, 0);
+}
+
+int pmic_bind_childs(struct udevice *pmic, int offset,
+                    const struct pmic_child_info *child_info)
+{
+       const struct pmic_child_info *info;
+       const void *blob = gd->fdt_blob;
+       struct driver *drv;
+       struct udevice *child;
+       const char *node_name;
+       int node_name_len;
+       int bind_count = 0;
+       int node;
+       int prefix_len;
+       int ret;
+
+       debug("%s for '%s' at node offset: %d\n", __func__, pmic->name,
+             pmic->of_offset);
+
+       for (node = fdt_first_subnode(blob, offset);
+            node > 0;
+            node = fdt_next_subnode(blob, node)) {
+               node_name = fdt_get_name(blob, node, &node_name_len);
+
+               debug("* Found child node: '%s' at offset:%d\n", node_name,
+                                                                node);
+
+               child = NULL;
+               info = child_info;
+               while (info->prefix) {
+                       prefix_len = strlen(info->prefix);
+                       if (strncasecmp(info->prefix, node_name, prefix_len) ||
+                           !info->driver) {
+                               info++;
+                               continue;
+                       }
+
+                       debug("  - compatible prefix: '%s'\n", info->prefix);
+
+                       drv = lists_driver_lookup_name(info->driver);
+                       if (!drv) {
+                               debug("  - driver: '%s' not found!\n",
+                                     info->driver);
+                               continue;
+                       }
+
+                       debug("  - found child driver: '%s'\n", drv->name);
+
+                       ret = device_bind(pmic, drv, node_name, NULL,
+                                         node, &child);
+                       if (ret) {
+                               debug("  - child binding error: %d\n", ret);
+                               continue;
+                       }
+
+                       debug("  - bound child device: '%s'\n", child->name);
+
+                       child->driver_data = str_get_num(node_name +
+                                                        prefix_len,
+                                                        node_name +
+                                                        node_name_len);
+
+                       debug("  - set 'child->driver_data': %lu\n",
+                             child->driver_data);
+                       break;
+               }
+
+               if (child)
+                       bind_count++;
+               else
+                       debug("  - compatible prefix not found\n");
+       }
+
+       debug("Bound: %d childs for PMIC: '%s'\n", bind_count, pmic->name);
+       return bind_count;

drivers/power/pmic/pmic-uclass.c new file +46 -0

+       if (!ops)
+               return -ENOSYS;
+
+       return ops->reg_count;
+}
+
+int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len)
+{
+       const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);
+       int ret;
+
+       if (!buffer)
+               return -EFAULT;
+
+       if (!ops || !ops->read)
+               return -ENOSYS;
+
+       ret = ops->read(dev, reg, buffer, len);
+       if (ret)
+               return ret;
+
+       return 0;
+}
+
+int pmic_write(struct udevice *dev, uint reg, const uint8_t *buffer, int len)
+{
+       const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);
+       int ret;
+
+       if (!buffer)
+               return -EFAULT;
+
+       if (!ops || !ops->write)
+               return -ENOSYS;
+
+       ret = ops->write(dev, reg, buffer, len);
+       if (ret)
+               return ret;
+
+       return 0;
+}
+
+UCLASS_DRIVER(pmic) = {
+       .id             = UCLASS_PMIC,
+       .name           = "pmic",
+};

include/dm/uclass-id.h +3 -0

index fddfd35..23b3eb9 100644
@@ -46,6 +46,9 @@ enum uclass_id {
        UCLASS_USB_DEV_GENERIC, /* USB generic device */
        UCLASS_MASS_STORAGE,    /* Mass storage device */
 
+       /* Power Management */
+       UCLASS_PMIC,            /* PMIC I/O device */
+
        UCLASS_COUNT,
        UCLASS_INVALID = -1,
 };

include/power/pmic.h +3 -0

index afbc5aa..f7ae781 100644
@@ -1,4 +1,7 @@
 /*
+ *  Copyright (C) 2014-2015 Samsung Electronics
+ *  Przemyslaw Marczak <[email protected]>
+ *
  *  Copyright (C) 2011-2012 Samsung Electronics
  *  Lukasz Majewski <[email protected]>
  *

include/power/pmic.h +2 -0

 enum { PMIC_I2C, PMIC_SPI, PMIC_NONE};
+
+#ifdef CONFIG_POWER
 enum { I2C_PMIC, I2C_NUM, };
 enum { PMIC_READ, PMIC_WRITE, };
 enum { PMIC_SENSOR_BYTE_ORDER_LITTLE, PMIC_SENSOR_BYTE_ORDER_BIG, };

@@ -77,7 +83,189 @@ struct pmic {
        struct pmic *parent;

include/power/pmic.h +9 -0

+ *
+ * The driver model tree could look like this:
+ *
+ *_ root device
+ * |_ BUS 0 device (e.g. I2C0)                 - UCLASS_I2C/SPI/...
+ * | |_ PMIC device (READ/WRITE ops)           - UCLASS_PMIC
+ * |   |_ REGULATOR device (ldo/buck/... ops)  - UCLASS_REGULATOR
+ * |   |_ CHARGER device (charger ops)         - UCLASS_CHARGER (in the future)
+ * |   |_ MUIC device (microUSB connector ops) - UCLASS_MUIC    (in the future)

include/power/pmic.h +9 -0

+ * of proper child devices. Each child usually implements a different function,
+ * controlled by the same interface.
+ *
+ * The binding should be done automatically. If device tree nodes/subnodes are
+ * proper defined, then:
+ *
+ * |_ the ROOT driver will bind the device for I2C/SPI node:
+ *   |_ the I2C/SPI driver should bind a device for pmic node:
+ *     |_ the PMIC driver should bind devices for its childs:

include/power/pmic.h +10 -0

+ * the 'pmic_bind_childs()' - please refer to the description of this function
+ * in this header file. This function, should be called in the driver's '.bind'
+ * method.
+ *
+ * For the example driver, please refer the MAX77686 driver:
+ * - 'drivers/power/pmic/max77686.c'
+ */
+
+/**
+ * struct dm_pmic_ops - PMIC device I/O interface

include/power/pmic.h +50 -0

+ * for reduce a number of lines with the same code for read/write or get/set.
+ *
+ * @PMIC_OP_GET - get operation
+ * @PMIC_OP_SET - set operation
+*/
+enum pmic_op_type {
+       PMIC_OP_GET,
+       PMIC_OP_SET,
+};
+
+/**
+ * struct pmic_child_info - basic device's child info for bind child nodes with
+ * the driver by the node name prefix and driver name. This is a helper struct
+ * for function: pmic_bind_childs().
+ *
+ * @prefix - child node name prefix (or its name if is unique or single)
+ * @driver - driver name for the sub-node with prefix
+ */
+struct pmic_child_info {
+       const char *prefix;
+       const char *driver;
+};
+
+/* drivers/power/pmic-uclass.c */
+
+/**
+ * pmic_bind_childs() - bind drivers for given parent pmic, using child info
+ * found in 'child_info' array.
+ *
+ * @pmic       - pmic device - the parent of found child's
+ * @child_info - N-childs info array
+ * @return a positive number of childs, or 0 if no child found (error)
+ *
+ * Note: For N-childs the child_info array should have N+1 entries and the last
+ * entry prefix should be NULL - the same as for drivers compatible.
+ *
+ * For example, a single prefix info (N=1):
+ * static const struct pmic_child_info bind_info[] = {
+ *     { .prefix = "ldo", .driver = "ldo_driver" },
+ *     { },
+ * };
+ *
+ * This function is useful for regulator sub-nodes:
+ * my_regulator at 0xa {
+ *     reg = <0xa>;
+ *     (pmic - bind automatically by compatible)
+ *     compatible = "my_pmic";
+ *     ...
+ *     (pmic's childs - bind by pmic_bind_childs())
+ *     (nodes prefix: "ldo", driver: "my_regulator_ldo")

include/power/pmic.h +38 -1

+
+/**
+ * pmic_get: get the pmic device using its name
+ *
+ * @name - device name
+ * @devp - returned pointer to the pmic device
+ * @return 0 on success or negative value of errno.
+ *
+ * The returned devp device can be used with pmic_read/write calls
+ */
+int pmic_get(const char *name, struct udevice **devp);
+
+/**
+ * pmic_reg_count: get the pmic register count
+ *
+ * The required pmic device can be obtained by 'pmic_get()'
+ *
+ * @dev - pointer to the UCLASS_PMIC device
+ * @return register count value on success or negative value of errno.
+ */
+int pmic_reg_count(struct udevice *dev);
+
+/**
+ * pmic_read/write: read/write to the UCLASS_PMIC device
+ *
+ * The required pmic device can be obtained by 'pmic_get()'
+ *
+ * @pmic   - pointer to the UCLASS_PMIC device
+ * @reg    - device register offset
+ * @buffer - pointer to read/write buffer
+ * @len    - byte count for read/write
+ * @return 0 on success or negative value of errno.
+ */
+int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len);
+int pmic_write(struct udevice *dev, uint reg, const uint8_t *buffer, int len);
+#endif /* CONFIG_DM_PMIC */
 
+#ifdef CONFIG_POWER
 int pmic_init(unsigned char bus);
 int power_init_board(void);
 int pmic_dialog_init(unsigned char bus);

@@ -88,6 +276,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);
+#endif
 
 #define pmic_i2c_addr (p->hw.i2c.addr)
 #define pmic_i2c_tx_num (p->hw.i2c.tx_num)
--

1.9.1

Hi Przemyslaw,

Quoted 50 lines · Przemyslaw Marczak

This commit introduces the PMIC uclass implementation. It allows providing the basic I/O interface for PMIC devices. For the multi-function PMIC devices, this can be used as I/O parent device, for each IC's interface. Then, each PMIC particular function can be provided by the child device's operations, and the child devices will use its parent for read/write by the common API.

Core files:
- 'include/power/pmic.h'
- 'drivers/power/pmic/pmic-uclass.c'

The old pmic framework is still kept and is independent.

For more detailed informations, please look into the header file.

Changes:
- new uclass-id: UCLASS_PMIC
- new config: CONFIG_DM_PMIC

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Changes V2:
- pmic uclass: adjust uclass code to the mainline changes
- pmic uclass: remove pmic_i2c and pmic_spi
- pmic uclass: modify pmic_platdata
- pmic uclass: add pmic_if_* functions
- pmic uclass: remove pmic_init_dm()
- pmic uclass: cleanup
- pmic.h: define pmic ops structure (read/write operations)
- pmic.h: add comments to functions

Changes V3:
- pmic-uclass.c and pmic.h:
-- remove pmic_if_* functions
-- add new function pmic_child_node_scan()
- add Kconfig entry

Changes V4:
- move drivers/power/pmic-uclass.c to drivers/power/pmic/pmic-uclass.c
- move DM_PMIC Kconfig entry: drivers/power/Kconfig to drivers/power/pmic/Kconfig
- drivers/power/Kconfig: Add menu "Power" and include pmic Kconfig
- Kconfig: provide only the general information about the PMIC
- pmic-uclass.c: add pmic_bind_childs()
- pmic-uclass.c: add debug
- pmic-uclass.c: cleanup includes
- pmic-uclass.c: remove pmic_get_uclass_ops() and use of dev_get_driver_ops()
- pmic-uclass.c: use of uclass_get_device_by_name()
- pmic-uclass.c: add str_get_num() - for get number from string
- include/power/pmic.h - start comments rewording
- power/pmic.h: comments update

Quoted 10 lines

---
 drivers/power/Kconfig            |   6 ++
 drivers/power/pmic/Kconfig       |  11 +++
 drivers/power/pmic/Makefile      |   1 +
 drivers/power/pmic/pmic-uclass.c | 158 ++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h           |   3 +
 include/power/pmic.h             | 189 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 368 insertions(+)
 create mode 100644 drivers/power/pmic/Kconfig
 create mode 100644 drivers/power/pmic/pmic-uclass.c

Acked-by Simon Glass <[email protected]>

I have a few nits below - perhaps they can be targeted in a follow-up patch or two? I'd like to merge this soon and it is not worth holding up the series for nits.

Hi Przemyslaw,

Quoted 58 lines · Przemyslaw Marczak

This commit introduces the PMIC uclass implementation. It allows providing the basic I/O interface for PMIC devices. For the multi-function PMIC devices, this can be used as I/O parent device, for each IC's interface. Then, each PMIC particular function can be provided by the child device's operations, and the child devices will use its parent for read/write by the common API.

Core files:
- 'include/power/pmic.h'
- 'drivers/power/pmic/pmic-uclass.c'

The old pmic framework is still kept and is independent.

For more detailed informations, please look into the header file.

Changes:
- new uclass-id: UCLASS_PMIC
- new config: CONFIG_DM_PMIC

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Changes V2:
- pmic uclass: adjust uclass code to the mainline changes
- pmic uclass: remove pmic_i2c and pmic_spi
- pmic uclass: modify pmic_platdata
- pmic uclass: add pmic_if_* functions
- pmic uclass: remove pmic_init_dm()
- pmic uclass: cleanup
- pmic.h: define pmic ops structure (read/write operations)
- pmic.h: add comments to functions

Changes V3:
- pmic-uclass.c and pmic.h:
-- remove pmic_if_* functions
-- add new function pmic_child_node_scan()
- add Kconfig entry

Changes V4:
- move drivers/power/pmic-uclass.c to drivers/power/pmic/pmic-uclass.c
- move DM_PMIC Kconfig entry: drivers/power/Kconfig to drivers/power/pmic/Kconfig
- drivers/power/Kconfig: Add menu "Power" and include pmic Kconfig
- Kconfig: provide only the general information about the PMIC
- pmic-uclass.c: add pmic_bind_childs()
- pmic-uclass.c: add debug
- pmic-uclass.c: cleanup includes
- pmic-uclass.c: remove pmic_get_uclass_ops() and use of dev_get_driver_ops()
- pmic-uclass.c: use of uclass_get_device_by_name()
- pmic-uclass.c: add str_get_num() - for get number from string
- include/power/pmic.h - start comments rewording
- power/pmic.h: comments update

---
  drivers/power/Kconfig            |   6 ++
  drivers/power/pmic/Kconfig       |  11 +++
  drivers/power/pmic/Makefile      |   1 +
  drivers/power/pmic/pmic-uclass.c | 158 ++++++++++++++++++++++++++++++++
  include/dm/uclass-id.h           |   3 +
  include/power/pmic.h             | 189 +++++++++++++++++++++++++++++++++++++++
  6 files changed, 368 insertions(+)

Quoted 2 lines

create mode 100644 drivers/power/pmic/Kconfig create mode 100644 drivers/power/pmic/pmic-uclass.c

Acked-by Simon Glass <[email protected]>

I have a few nits below - perhaps they can be targeted in a follow-up patch or two? I'd like to merge this soon and it is not worth holding up the series for nits.

Quoted 156 lines

drivers/power/Kconfig +6 -0

index f8f0239..d03626e 100644
@@ -1,3 +1,7 @@
+menu "Power"
+
+source "drivers/power/pmic/Kconfig"
+
  config AXP221_POWER
         boolean "axp221 / axp223 pmic support"
         depends on MACH_SUN6I || MACH_SUN8I

@@ -73,3 +77,5 @@ config AXP221_ELDO3_VOLT
         disable eldo3. On some A31(s) tablets it might be used to supply
         1.2V for the SSD2828 chip (converter of parallel LCD interface
         into MIPI DSI).
+
+endmenu

drivers/power/pmic/Kconfig new file +11 -0

new file mode 100644
index 0000000..d06d632
@@ -0,0 +1,11 @@
+config DM_PMIC
+       bool "Enable Driver Model for PMIC drivers (UCLASS_PMIC)"
+       depends on DM
+       ---help---
+       This config enables the driver-model PMIC support.
+       UCLASS_PMIC - designed to provide an I/O interface for PMIC devices.
+       For the multi-function PMIC devices, this can be used as parent I/O
+       device for each IC's interface. Then, each children uses its parent
+       for read/write. For detailed description, please refer to the files:
+       - 'drivers/power/pmic/pmic-uclass.c'
+       - 'include/power/pmic.h'

drivers/power/pmic/Makefile +1 -0

index 985cfdb..594f620 100644
@@ -5,6 +5,7 @@
  # SPDX-License-Identifier:     GPL-2.0+
  #
 
+obj-$(CONFIG_DM_PMIC) += pmic-uclass.o
  obj-$(CONFIG_POWER_LTC3676) += pmic_ltc3676.o
  obj-$(CONFIG_POWER_MAX8998) += pmic_max8998.o
  obj-$(CONFIG_POWER_MAX8997) += pmic_max8997.o

drivers/power/pmic/pmic-uclass.c new file +102 -0

new file mode 100644
index 0000000..d82d3da
@@ -0,0 +1,158 @@
+/*
+ * Copyright (C) 2014-2015 Samsung Electronics
+ * Przemyslaw Marczak <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <fdtdec.h>
+#include <errno.h>
+#include <dm.h>
+#include <dm/lists.h>
+#include <dm/device-internal.h>
+#include <dm/uclass-internal.h>
+#include <power/pmic.h>
+#include <linux/ctype.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static ulong str_get_num(const char *ptr, const char *maxptr)
+{
+       if (!ptr || !maxptr)
+               return 0;
+
+       while (!isdigit(*ptr) && ptr++ < maxptr);
+
+       return simple_strtoul(ptr, NULL, 0);
+}
+
+int pmic_bind_childs(struct udevice *pmic, int offset,
+                    const struct pmic_child_info *child_info)
+{
+       const struct pmic_child_info *info;
+       const void *blob = gd->fdt_blob;
+       struct driver *drv;
+       struct udevice *child;
+       const char *node_name;
+       int node_name_len;
+       int bind_count = 0;
+       int node;
+       int prefix_len;
+       int ret;
+
+       debug("%s for '%s' at node offset: %d\n", __func__, pmic->name,
+             pmic->of_offset);
+
+       for (node = fdt_first_subnode(blob, offset);
+            node > 0;
+            node = fdt_next_subnode(blob, node)) {
+               node_name = fdt_get_name(blob, node, &node_name_len);
+
+               debug("* Found child node: '%s' at offset:%d\n", node_name,
+                                                                node);
+
+               child = NULL;
+               info = child_info;
+               while (info->prefix) {
+                       prefix_len = strlen(info->prefix);
+                       if (strncasecmp(info->prefix, node_name, prefix_len) ||
+                           !info->driver) {
+                               info++;
+                               continue;
+                       }
+
+                       debug("  - compatible prefix: '%s'\n", info->prefix);
+
+                       drv = lists_driver_lookup_name(info->driver);
+                       if (!drv) {
+                               debug("  - driver: '%s' not found!\n",
+                                     info->driver);
+                               continue;
+                       }
+
+                       debug("  - found child driver: '%s'\n", drv->name);
+
+                       ret = device_bind(pmic, drv, node_name, NULL,
+                                         node, &child);
+                       if (ret) {
+                               debug("  - child binding error: %d\n", ret);
+                               continue;
+                       }
+
+                       debug("  - bound child device: '%s'\n", child->name);
+
+                       child->driver_data = str_get_num(node_name +
+                                                        prefix_len,
+                                                        node_name +
+                                                        node_name_len);
+
+                       debug("  - set 'child->driver_data': %lu\n",
+                             child->driver_data);
+                       break;
+               }
+
+               if (child)
+                       bind_count++;
+               else
+                       debug("  - compatible prefix not found\n");
+       }
+
+       debug("Bound: %d childs for PMIC: '%s'\n", bind_count, pmic->name);
+       return bind_count;

drivers/power/pmic/pmic-uclass.c line 112

Quoted 10 lines

drivers/power/pmic/pmic-uclass.c new file +10 -0

+}
+
+int pmic_get(const char *name, struct udevice **devp)
+{
+       return uclass_get_device_by_name(UCLASS_PMIC, name, devp);
+}
+
+int pmic_reg_count(struct udevice *dev)
+{
+       const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);

blank line here

Quoted 73 lines

drivers/power/pmic/pmic-uclass.c new file +46 -0

+       if (!ops)
+               return -ENOSYS;
+
+       return ops->reg_count;
+}
+
+int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len)
+{
+       const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);
+       int ret;
+
+       if (!buffer)
+               return -EFAULT;
+
+       if (!ops || !ops->read)
+               return -ENOSYS;
+
+       ret = ops->read(dev, reg, buffer, len);
+       if (ret)
+               return ret;
+
+       return 0;
+}
+
+int pmic_write(struct udevice *dev, uint reg, const uint8_t *buffer, int len)
+{
+       const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);
+       int ret;
+
+       if (!buffer)
+               return -EFAULT;
+
+       if (!ops || !ops->write)
+               return -ENOSYS;
+
+       ret = ops->write(dev, reg, buffer, len);
+       if (ret)
+               return ret;
+
+       return 0;
+}
+
+UCLASS_DRIVER(pmic) = {
+       .id             = UCLASS_PMIC,
+       .name           = "pmic",
+};

include/dm/uclass-id.h +3 -0

index fddfd35..23b3eb9 100644
@@ -46,6 +46,9 @@ enum uclass_id {
         UCLASS_USB_DEV_GENERIC, /* USB generic device */
         UCLASS_MASS_STORAGE,    /* Mass storage device */
 
+       /* Power Management */
+       UCLASS_PMIC,            /* PMIC I/O device */
+
         UCLASS_COUNT,
         UCLASS_INVALID = -1,
  };

include/power/pmic.h +3 -0

index afbc5aa..f7ae781 100644
@@ -1,4 +1,7 @@
  /*
+ *  Copyright (C) 2014-2015 Samsung Electronics
+ *  Przemyslaw Marczak <[email protected]>
+ *
   *  Copyright (C) 2011-2012 Samsung Electronics
   *  Lukasz Majewski <[email protected]>
   *

include/power/pmic.h line 17

Quoted 6 lines

include/power/pmic.h +1 -0

@@ -9,10 +12,13 @@
  #define __CORE_PMIC_H_
 
  #include <linux/list.h>
+#include <spi.h>
  #include <i2c.h>
  #include <power/power_chrg.h>

At some point can you update the ordering of this lot? I think it should be:

i2c.g
spi.h
linux/
power/

Quoted 9 lines

include/power/pmic.h +2 -0

  enum { PMIC_I2C, PMIC_SPI, PMIC_NONE};
+
+#ifdef CONFIG_POWER
  enum { I2C_PMIC, I2C_NUM, };
  enum { PMIC_READ, PMIC_WRITE, };
  enum { PMIC_SENSOR_BYTE_ORDER_LITTLE, PMIC_SENSOR_BYTE_ORDER_BIG, };

@@ -77,7 +83,189 @@ struct pmic {
         struct pmic *parent;

include/power/pmic.h struct pmic { · line 93

Quoted 10 lines

include/power/pmic.h +8 -0

         struct list_head list;
  };
+#endif /* CONFIG_POWER */
+
+#ifdef CONFIG_DM_PMIC
+/**
+ * U-Boot PMIC Framework
+ * =====================
+ *
+ * UCLASS_PMIC - The is designed to provide an I/O interface for PMIC devices.

This is designed

include/power/pmic.h struct pmic { · line 96

Quoted 3 lines

include/power/pmic.h +3 -0

+ *
+ * For the multi-function PMIC devices, this can be used as parent I/O device
+ * for each IC's interface. Then, each children uses its parent for read/write.

each child

Quoted 9 lines

include/power/pmic.h +9 -0

+ *
+ * The driver model tree could look like this:
+ *
+ *_ root device
+ * |_ BUS 0 device (e.g. I2C0)                 - UCLASS_I2C/SPI/...
+ * | |_ PMIC device (READ/WRITE ops)           - UCLASS_PMIC
+ * |   |_ REGULATOR device (ldo/buck/... ops)  - UCLASS_REGULATOR
+ * |   |_ CHARGER device (charger ops)         - UCLASS_CHARGER (in the future)
+ * |   |_ MUIC device (microUSB connector ops) - UCLASS_MUIC    (in the future)

include/power/pmic.h struct pmic { · line 115

Quoted 10 lines

include/power/pmic.h +10 -0

+ * |   |_ ...
+ * |
+ * |_ BUS 1 device (e.g. I2C1)                 - UCLASS_I2C/SPI/...
+ *   |_ PMIC device (READ/WRITE ops)           - UCLASS_PMIC
+ *     |_ RTC device (rtc ops)                 - UCLASS_RTC     (in the future)
+ *
+ * We can find two PMIC cases in boards design:
+ * - single I/O interface
+ * - multiple I/O interfaces
+ * We bind single PMIC device for each interface, to provide an I/O as a parent,

a single

Quoted 9 lines

include/power/pmic.h +9 -0

+ * of proper child devices. Each child usually implements a different function,
+ * controlled by the same interface.
+ *
+ * The binding should be done automatically. If device tree nodes/subnodes are
+ * proper defined, then:
+ *
+ * |_ the ROOT driver will bind the device for I2C/SPI node:
+ *   |_ the I2C/SPI driver should bind a device for pmic node:
+ *     |_ the PMIC driver should bind devices for its childs:

include/power/pmic.h struct pmic { · line 134

Quoted 10 lines

include/power/pmic.h +10 -0

+ *       |_ regulator (child)
+ *       |_ charger   (child)
+ *       |_ other     (child)
+ *
+ * The same for other device nodes, for multi-interface PMIC.
+ *
+ * Note:
+ * Each PMIC interface driver should use a different compatible string.
+ *
+ * If each pmic child device driver need access the PMIC-specific registers,

If a pmic child device driver needs

include/power/pmic.h struct pmic { · line 144

Quoted 10 lines

include/power/pmic.h +10 -0

+ * it need know only the register address and the access can be done through
+ * the parent pmic driver. Like in the example:
+ *
+ *_ root driver
+ * |_ dev: bus I2C0                                         - UCLASS_I2C
+ * | |_ dev: my_pmic (read/write)              (is parent)  - UCLASS_PMIC
+ * |   |_ dev: my_regulator (set value/etc..)  (is child)   - UCLASS_REGULATOR
+ *
+ * To ensure such device relationship, the pmic device driver should also bind
+ * all its child devices, like in the example below. It should be done by call

by calling pmic_bind_childs()

(which you should rename to pmic_bind_children() I think)

Quoted 15 lines

include/power/pmic.h +15 -0

+ * the 'pmic_bind_childs()' - please refer to the description of this function
+ * in this header file. This function, should be called in the driver's '.bind'
+ * method.
+ *
+ * For the example driver, please refer the MAX77686 driver:
+ * - 'drivers/power/pmic/max77686.c'
+ */
+
+/**
+ * struct dm_pmic_ops - PMIC device I/O interface
+ *
+ * Should be implemented by UCLASS_PMIC device drivers. The standard
+ * device operations provides the I/O interface for it's childs.
+ *
+ * @reg_count: devices register count

include/power/pmic.h struct pmic { · line 164

Quoted 5 lines

include/power/pmic.h +5 -0

+ * @read:      read 'len' bytes at "reg" and store it into the 'buffer'
+ * @write:     write 'len' bytes from the 'buffer' to the register at 'reg' address
+ */
+struct dm_pmic_ops {
+       int reg_count;

This should not be in ops as it is not an operation. Perhaps add a function for it, or put it in the uclass platform data?

include/power/pmic.h struct pmic { · line 170

Quoted 6 lines

include/power/pmic.h +6 -0

+       int (*read)(struct udevice *dev, uint reg, uint8_t *buffer, int len);
+       int (*write)(struct udevice *dev, uint reg, const uint8_t *buffer,
+                    int len);
+};
+
+/* enum pmic_op_type - used for various pmic devices operation calls,

/**
* enum pmic_op_type -

Quoted 50 lines

include/power/pmic.h +50 -0

+ * for reduce a number of lines with the same code for read/write or get/set.
+ *
+ * @PMIC_OP_GET - get operation
+ * @PMIC_OP_SET - set operation
+*/
+enum pmic_op_type {
+       PMIC_OP_GET,
+       PMIC_OP_SET,
+};
+
+/**
+ * struct pmic_child_info - basic device's child info for bind child nodes with
+ * the driver by the node name prefix and driver name. This is a helper struct
+ * for function: pmic_bind_childs().
+ *
+ * @prefix - child node name prefix (or its name if is unique or single)
+ * @driver - driver name for the sub-node with prefix
+ */
+struct pmic_child_info {
+       const char *prefix;
+       const char *driver;
+};
+
+/* drivers/power/pmic-uclass.c */
+
+/**
+ * pmic_bind_childs() - bind drivers for given parent pmic, using child info
+ * found in 'child_info' array.
+ *
+ * @pmic       - pmic device - the parent of found child's
+ * @child_info - N-childs info array
+ * @return a positive number of childs, or 0 if no child found (error)
+ *
+ * Note: For N-childs the child_info array should have N+1 entries and the last
+ * entry prefix should be NULL - the same as for drivers compatible.
+ *
+ * For example, a single prefix info (N=1):
+ * static const struct pmic_child_info bind_info[] = {
+ *     { .prefix = "ldo", .driver = "ldo_driver" },
+ *     { },
+ * };
+ *
+ * This function is useful for regulator sub-nodes:
+ * my_regulator at 0xa {
+ *     reg = <0xa>;
+ *     (pmic - bind automatically by compatible)
+ *     compatible = "my_pmic";
+ *     ...
+ *     (pmic's childs - bind by pmic_bind_childs())
+ *     (nodes prefix: "ldo", driver: "my_regulator_ldo")

include/power/pmic.h struct pmic { · line 230

Quoted 10 lines

include/power/pmic.h +10 -0

+ *     ldo1 { ... };
+ *     ldo2 { ... };
+ *
+ *     (nodes prefix: "buck", driver: "my_regulator_buck")
+ *     buck1 { ... };
+ *     buck2 { ... };
+ * };
+ */
+int pmic_bind_childs(struct udevice *pmic, int offset,
+                    const struct pmic_child_info *child_info);

pmic_bind_children

Quoted 47 lines

include/power/pmic.h +38 -0

+
+/**
+ * pmic_get: get the pmic device using its name
+ *
+ * @name - device name
+ * @devp - returned pointer to the pmic device
+ * @return 0 on success or negative value of errno.
+ *
+ * The returned devp device can be used with pmic_read/write calls
+ */
+int pmic_get(const char *name, struct udevice **devp);
+
+/**
+ * pmic_reg_count: get the pmic register count
+ *
+ * The required pmic device can be obtained by 'pmic_get()'
+ *
+ * @dev - pointer to the UCLASS_PMIC device
+ * @return register count value on success or negative value of errno.
+ */
+int pmic_reg_count(struct udevice *dev);
+
+/**
+ * pmic_read/write: read/write to the UCLASS_PMIC device
+ *
+ * The required pmic device can be obtained by 'pmic_get()'
+ *
+ * @pmic   - pointer to the UCLASS_PMIC device
+ * @reg    - device register offset
+ * @buffer - pointer to read/write buffer
+ * @len    - byte count for read/write
+ * @return 0 on success or negative value of errno.
+ */
+int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len);
+int pmic_write(struct udevice *dev, uint reg, const uint8_t *buffer, int len);
+#endif /* CONFIG_DM_PMIC */
 
+#ifdef CONFIG_POWER
  int pmic_init(unsigned char bus);
  int power_init_board(void);
  int pmic_dialog_init(unsigned char bus);

@@ -88,6 +276,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);
+#endif
 

Quoted 5 lines

include/power/pmic.h +0 -1

  #define pmic_i2c_addr (p->hw.i2c.addr)
  #define pmic_i2c_tx_num (p->hw.i2c.tx_num)
--

1.9.1

Regards,
Simon

Hello Simon,

Quoted 71 lines · Simon Glass

Hi Przemyslaw,

Quoted 62 lines · Przemyslaw Marczak

This commit introduces the PMIC uclass implementation. It allows providing the basic I/O interface for PMIC devices. For the multi-function PMIC devices, this can be used as I/O parent device, for each IC's interface. Then, each PMIC particular function can be provided by the child device's operations, and the child devices will use its parent for read/write by the common API.

Core files:
- 'include/power/pmic.h'
- 'drivers/power/pmic/pmic-uclass.c'

The old pmic framework is still kept and is independent.

For more detailed informations, please look into the header file.

Changes:
- new uclass-id: UCLASS_PMIC
- new config: CONFIG_DM_PMIC

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Changes V2:
- pmic uclass: adjust uclass code to the mainline changes
- pmic uclass: remove pmic_i2c and pmic_spi
- pmic uclass: modify pmic_platdata
- pmic uclass: add pmic_if_* functions
- pmic uclass: remove pmic_init_dm()
- pmic uclass: cleanup
- pmic.h: define pmic ops structure (read/write operations)
- pmic.h: add comments to functions

Changes V3:
- pmic-uclass.c and pmic.h:
-- remove pmic_if_* functions
-- add new function pmic_child_node_scan()
- add Kconfig entry

Changes V4:
- move drivers/power/pmic-uclass.c to drivers/power/pmic/pmic-uclass.c
- move DM_PMIC Kconfig entry: drivers/power/Kconfig to drivers/power/pmic/Kconfig
- drivers/power/Kconfig: Add menu "Power" and include pmic Kconfig
- Kconfig: provide only the general information about the PMIC
- pmic-uclass.c: add pmic_bind_childs()
- pmic-uclass.c: add debug
- pmic-uclass.c: cleanup includes
- pmic-uclass.c: remove pmic_get_uclass_ops() and use of dev_get_driver_ops()
- pmic-uclass.c: use of uclass_get_device_by_name()
- pmic-uclass.c: add str_get_num() - for get number from string
- include/power/pmic.h - start comments rewording
- power/pmic.h: comments update

---
  drivers/power/Kconfig            |   6 ++
  drivers/power/pmic/Kconfig       |  11 +++
  drivers/power/pmic/Makefile      |   1 +
  drivers/power/pmic/pmic-uclass.c | 158 ++++++++++++++++++++++++++++++++
  include/dm/uclass-id.h           |   3 +
  include/power/pmic.h             | 189
+++++++++++++++++++++++++++++++++++++++
  6 files changed, 368 insertions(+)

Quoted 2 lines

create mode 100644 drivers/power/pmic/Kconfig create mode 100644 drivers/power/pmic/pmic-uclass.c

Quoted 6 lines

Acked-by Simon Glass <[email protected]>

I have a few nits below - perhaps they can be targeted in a follow-up patch or two? I'd like to merge this soon and it is not worth holding up the series for nits.

That's good information. I will fix it all and resend ASAP.

Quoted 398 lines

Quoted 168 lines

drivers/power/Kconfig +6 -0

index f8f0239..d03626e 100644
@@ -1,3 +1,7 @@
+menu "Power"
+
+source "drivers/power/pmic/Kconfig"
+
  config AXP221_POWER
         boolean "axp221 / axp223 pmic support"
         depends on MACH_SUN6I || MACH_SUN8I

@@ -73,3 +77,5 @@ config AXP221_ELDO3_VOLT
         disable eldo3. On some A31(s) tablets it might be used to supply
         1.2V for the SSD2828 chip (converter of parallel LCD interface
         into MIPI DSI).
+
+endmenu

drivers/power/pmic/Kconfig new file +6 -0

new file mode 100644
index 0000000..d06d632
@@ -0,0 +1,11 @@
+config DM_PMIC
+       bool "Enable Driver Model for PMIC drivers (UCLASS_PMIC)"
+       depends on DM
+       ---help---
+       This config enables the driver-model PMIC support.
+       UCLASS_PMIC - designed to provide an I/O interface for PMIC

devices.
+ For the multi-function PMIC devices, this can be used as parent
I/O
+ device for each IC's interface. Then, each children uses its
parent
+ for read/write. For detailed description, please refer to the
files:

drivers/power/pmic/Kconfig new file +2 -0

+       - 'drivers/power/pmic/pmic-uclass.c'
+       - 'include/power/pmic.h'

drivers/power/pmic/Makefile +1 -0

index 985cfdb..594f620 100644
@@ -5,6 +5,7 @@
  # SPDX-License-Identifier:     GPL-2.0+
  #
 
+obj-$(CONFIG_DM_PMIC) += pmic-uclass.o
  obj-$(CONFIG_POWER_LTC3676) += pmic_ltc3676.o
  obj-$(CONFIG_POWER_MAX8998) += pmic_max8998.o
  obj-$(CONFIG_POWER_MAX8997) += pmic_max8997.o

drivers/power/pmic/pmic-uclass.c new file +52 -0

new file mode 100644
index 0000000..d82d3da
@@ -0,0 +1,158 @@
+/*
+ * Copyright (C) 2014-2015 Samsung Electronics
+ * Przemyslaw Marczak <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <fdtdec.h>
+#include <errno.h>
+#include <dm.h>
+#include <dm/lists.h>
+#include <dm/device-internal.h>
+#include <dm/uclass-internal.h>
+#include <power/pmic.h>
+#include <linux/ctype.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static ulong str_get_num(const char *ptr, const char *maxptr)
+{
+       if (!ptr || !maxptr)
+               return 0;
+
+       while (!isdigit(*ptr) && ptr++ < maxptr);
+
+       return simple_strtoul(ptr, NULL, 0);
+}
+
+int pmic_bind_childs(struct udevice *pmic, int offset,
+                    const struct pmic_child_info *child_info)
+{
+       const struct pmic_child_info *info;
+       const void *blob = gd->fdt_blob;
+       struct driver *drv;
+       struct udevice *child;
+       const char *node_name;
+       int node_name_len;
+       int bind_count = 0;
+       int node;
+       int prefix_len;
+       int ret;
+
+       debug("%s for '%s' at node offset: %d\n", __func__, pmic->name,
+             pmic->of_offset);
+
+       for (node = fdt_first_subnode(blob, offset);
+            node > 0;
+            node = fdt_next_subnode(blob, node)) {
+               node_name = fdt_get_name(blob, node, &node_name_len);
+
+               debug("* Found child node: '%s' at offset:%d\n",

node_name,

drivers/power/pmic/pmic-uclass.c new file +7 -0

+                                                                node);
+
+               child = NULL;
+               info = child_info;
+               while (info->prefix) {
+                       prefix_len = strlen(info->prefix);
+                       if (strncasecmp(info->prefix, node_name,

prefix_len) ||

drivers/power/pmic/pmic-uclass.c new file +6 -0

+                           !info->driver) {
+                               info++;
+                               continue;
+                       }
+
+                       debug("  - compatible prefix: '%s'\n",

info->prefix);

drivers/power/pmic/pmic-uclass.c new file +9 -0

+
+                       drv = lists_driver_lookup_name(info->driver);
+                       if (!drv) {
+                               debug("  - driver: '%s' not found!\n",
+                                     info->driver);
+                               continue;
+                       }
+
+                       debug("  - found child driver: '%s'\n",

drv->name);

drivers/power/pmic/pmic-uclass.c new file +5 -0

+
+                       ret = device_bind(pmic, drv, node_name, NULL,
+                                         node, &child);
+                       if (ret) {
+                               debug("  - child binding error: %d\n",

ret);

drivers/power/pmic/pmic-uclass.c new file +4 -0

+                               continue;
+                       }
+
+                       debug("  - bound child device: '%s'\n",

child->name);

drivers/power/pmic/pmic-uclass.c new file +18 -0

+
+                       child->driver_data = str_get_num(node_name +
+                                                        prefix_len,
+                                                        node_name +
+                                                        node_name_len);
+
+                       debug("  - set 'child->driver_data': %lu\n",
+                             child->driver_data);
+                       break;
+               }
+
+               if (child)
+                       bind_count++;
+               else
+                       debug("  - compatible prefix not found\n");
+       }
+
+       debug("Bound: %d childs for PMIC: '%s'\n", bind_count,

pmic->name);

drivers/power/pmic/pmic-uclass.c new file +1 -0

+       return bind_count;

drivers/power/pmic/pmic-uclass.c line 112

Quoted 10 lines

drivers/power/pmic/pmic-uclass.c new file +10 -0

+}
+
+int pmic_get(const char *name, struct udevice **devp)
+{
+       return uclass_get_device_by_name(UCLASS_PMIC, name, devp);
+}
+
+int pmic_reg_count(struct udevice *dev)
+{
+       const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);

blank line here

Quoted 74 lines

drivers/power/pmic/pmic-uclass.c new file +25 -0

+       if (!ops)
+               return -ENOSYS;
+
+       return ops->reg_count;
+}
+
+int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len)
+{
+       const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);
+       int ret;
+
+       if (!buffer)
+               return -EFAULT;
+
+       if (!ops || !ops->read)
+               return -ENOSYS;
+
+       ret = ops->read(dev, reg, buffer, len);
+       if (ret)
+               return ret;
+
+       return 0;
+}
+
+int pmic_write(struct udevice *dev, uint reg, const uint8_t *buffer, int

len)

drivers/power/pmic/pmic-uclass.c new file +21 -0

+{
+       const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);
+       int ret;
+
+       if (!buffer)
+               return -EFAULT;
+
+       if (!ops || !ops->write)
+               return -ENOSYS;
+
+       ret = ops->write(dev, reg, buffer, len);
+       if (ret)
+               return ret;
+
+       return 0;
+}
+
+UCLASS_DRIVER(pmic) = {
+       .id             = UCLASS_PMIC,
+       .name           = "pmic",
+};

include/dm/uclass-id.h +3 -0

index fddfd35..23b3eb9 100644
@@ -46,6 +46,9 @@ enum uclass_id {
         UCLASS_USB_DEV_GENERIC, /* USB generic device */
         UCLASS_MASS_STORAGE,    /* Mass storage device */
 
+       /* Power Management */
+       UCLASS_PMIC,            /* PMIC I/O device */
+
         UCLASS_COUNT,
         UCLASS_INVALID = -1,
  };

include/power/pmic.h +3 -0

index afbc5aa..f7ae781 100644
@@ -1,4 +1,7 @@
  /*
+ *  Copyright (C) 2014-2015 Samsung Electronics
+ *  Przemyslaw Marczak <[email protected]>
+ *
   *  Copyright (C) 2011-2012 Samsung Electronics
   *  Lukasz Majewski <[email protected]>
   *

include/power/pmic.h line 17

Quoted 6 lines

include/power/pmic.h +1 -0

@@ -9,10 +12,13 @@
  #define __CORE_PMIC_H_
 
  #include <linux/list.h>
+#include <spi.h>
  #include <i2c.h>
  #include <power/power_chrg.h>

At some point can you update the ordering of this lot? I think it should be:

i2c.g
spi.h
linux/
power/

Quoted 19 lines

include/power/pmic.h +10 -0

  enum { PMIC_I2C, PMIC_SPI, PMIC_NONE};
+
+#ifdef CONFIG_POWER
  enum { I2C_PMIC, I2C_NUM, };
  enum { PMIC_READ, PMIC_WRITE, };
  enum { PMIC_SENSOR_BYTE_ORDER_LITTLE, PMIC_SENSOR_BYTE_ORDER_BIG, };

@@ -77,7 +83,189 @@ struct pmic {
         struct pmic *parent;
         struct list_head list;
  };
+#endif /* CONFIG_POWER */
+
+#ifdef CONFIG_DM_PMIC
+/**
+ * U-Boot PMIC Framework
+ * =====================
+ *
+ * UCLASS_PMIC - The is designed to provide an I/O interface for PMIC

Quoted 1 line

devices.

This is designed

Quoted 5 lines

+ *
+ * For the multi-function PMIC devices, this can be used as parent I/O device
+ * for each IC's interface. Then, each children uses its parent for read/write.

each child

Quoted 14 lines

+ *
+ * The driver model tree could look like this:
+ *
+ *_ root device

+ * |_ BUS 0 device (e.g. I2C0)                 - UCLASS_I2C/SPI/...
+ * | |_ PMIC device (READ/WRITE ops)           - UCLASS_PMIC
+ * |   |_ REGULATOR device (ldo/buck/... ops)  - UCLASS_REGULATOR
+ * |   |_ CHARGER device (charger ops)         - UCLASS_CHARGER (in the
future)
+ * |   |_ MUIC device (microUSB connector ops) - UCLASS_MUIC    (in the
future)
+ * |   |_ ...
+ * |
+ * |_ BUS 1 device (e.g. I2C1)                 - UCLASS_I2C/SPI/...

Quoted 9 lines

+ * |_ PMIC device (READ/WRITE ops) - UCLASS_PMIC
+ * |_ RTC device (rtc ops) - UCLASS_RTC (in the
future)
+ *
+ * We can find two PMIC cases in boards design:
+ * - single I/O interface
+ * - multiple I/O interfaces
+ * We bind single PMIC device for each interface, to provide an I/O as a parent,

a single

Quoted 14 lines

+ * of proper child devices. Each child usually implements a different function,
+ * controlled by the same interface.
+ *
+ * The binding should be done automatically. If device tree nodes/subnodes are
+ * proper defined, then:
+ *
+ * |_ the ROOT driver will bind the device for I2C/SPI node:
+ * |_ the I2C/SPI driver should bind a device for pmic node:
+ * |_ the PMIC driver should bind devices for its childs:

+ *       |_ regulator (child)
+ *       |_ charger   (child)
+ *       |_ other     (child)

Quoted 8 lines

+ *
+ * The same for other device nodes, for multi-interface PMIC.
+ *
+ * Note:
+ * Each PMIC interface driver should use a different compatible string.
+ *
+ * If each pmic child device driver need access the PMIC-specific registers,

If a pmic child device driver needs

Quoted 7 lines

+ * it need know only the register address and the access can be done through
+ * the parent pmic driver. Like in the example:
+ *
+ *_ root driver

+ * |_ dev: bus I2C0                                         - UCLASS_I2C
+ * | |_ dev: my_pmic (read/write)              (is parent)  -

Quoted 8 lines

UCLASS_PMIC
+ * | |_ dev: my_regulator (set value/etc..) (is child) -
UCLASS_REGULATOR
+ *
+ * To ensure such device relationship, the pmic device driver should also bind
+ * all its child devices, like in the example below. It should be done by call

by calling pmic_bind_childs()

(which you should rename to pmic_bind_children() I think)

Quoted 19 lines

+ * the 'pmic_bind_childs()' - please refer to the description of this function
+ * in this header file. This function, should be called in the driver's '.bind'
+ * method.
+ *
+ * For the example driver, please refer the MAX77686 driver:
+ * - 'drivers/power/pmic/max77686.c'
+ */

include/power/pmic.h +10 -0

+
+/**
+ * struct dm_pmic_ops - PMIC device I/O interface
+ *
+ * Should be implemented by UCLASS_PMIC device drivers. The standard
+ * device operations provides the I/O interface for it's childs.
+ *
+ * @reg_count: devices register count
+ * @read:      read 'len' bytes at "reg" and store it into the 'buffer'
+ * @write:     write 'len' bytes from the 'buffer' to the register at

Quoted 9 lines

include/power/pmic.h struct pmic { · line 164

Quoted 4 lines

'reg' address
+ */

include/power/pmic.h +2 -0

+struct dm_pmic_ops {
+       int reg_count;

This should not be in ops as it is not an operation. Perhaps add a function for it, or put it in the uclass platform data?

Adding this to uclass platform data will add another problem - choosing the right place for setting this value, because it's usually not given in dts, however the device node "reg" property could also provide the address length as it is for the memory.
But probably you will agree, that this is a job for I2C/SPI subsystem, and moreover has no sense, since usually the dts files don't provide the "reg" property as "<reg length>" for i2c/spi devices.

So maybe putting here an 'ops' function is better idea. int (*get_reg_count)(struct udevice *dev);

This is used only for the PMIC command, so if return -ENOSYS, then just "dump" command will no available for the device.

Quoted 132 lines

include/power/pmic.h struct pmic { · line 170

Quoted 10 lines

+ int (*read)(struct udevice *dev, uint reg, uint8_t *buffer, int
len);
+ int (*write)(struct udevice *dev, uint reg, const uint8_t
*buffer,

include/power/pmic.h +4 -0

+                    int len);
+};
+
+/* enum pmic_op_type - used for various pmic devices operation calls,

/**
* enum pmic_op_type -

Quoted 63 lines

+ * for reduce a number of lines with the same code for read/write or get/set.
+ *
+ * @PMIC_OP_GET - get operation
+ * @PMIC_OP_SET - set operation

include/power/pmic.h +8 -0

+*/
+enum pmic_op_type {
+       PMIC_OP_GET,
+       PMIC_OP_SET,
+};
+
+/**
+ * struct pmic_child_info - basic device's child info for bind child

nodes with
+ * the driver by the node name prefix and driver name. This is a helper struct
+ * for function: pmic_bind_childs().
+ *
+ * @prefix - child node name prefix (or its name if is unique or single)
+ * @driver - driver name for the sub-node with prefix
+ */

include/power/pmic.h +9 -0

+struct pmic_child_info {
+       const char *prefix;
+       const char *driver;
+};
+
+/* drivers/power/pmic-uclass.c */
+
+/**
+ * pmic_bind_childs() - bind drivers for given parent pmic, using child

info
+ * found in 'child_info' array.
+ *
+ * @pmic - pmic device - the parent of found child's
+ * @child_info - N-childs info array
+ * @return a positive number of childs, or 0 if no child found (error)
+ *
+ * Note: For N-childs the child_info array should have N+1 entries and the last
+ * entry prefix should be NULL - the same as for drivers compatible.
+ *
+ * For example, a single prefix info (N=1):
+ * static const struct pmic_child_info bind_info[] = {
+ * { .prefix = "ldo", .driver = "ldo_driver" },
+ * { },
+ * };
+ *
+ * This function is useful for regulator sub-nodes:
+ * my_regulator at 0xa {

+ *     reg = <0xa>;
+ *     (pmic - bind automatically by compatible)
+ *     compatible = "my_pmic";
+ *     ...
+ *     (pmic's childs - bind by pmic_bind_childs())
+ *     (nodes prefix: "ldo", driver: "my_regulator_ldo")
+ *     ldo1 { ... };
+ *     ldo2 { ... };
+ *
+ *     (nodes prefix: "buck", driver: "my_regulator_buck")
+ *     buck1 { ... };
+ *     buck2 { ... };

+ * };
+ */

include/power/pmic.h struct pmic { · line 230

Quoted 2 lines

include/power/pmic.h +2 -0

+int pmic_bind_childs(struct udevice *pmic, int offset,
+                    const struct pmic_child_info *child_info);

pmic_bind_children

Quoted 48 lines

include/power/pmic.h +35 -0

+
+/**
+ * pmic_get: get the pmic device using its name
+ *
+ * @name - device name
+ * @devp - returned pointer to the pmic device
+ * @return 0 on success or negative value of errno.
+ *
+ * The returned devp device can be used with pmic_read/write calls
+ */
+int pmic_get(const char *name, struct udevice **devp);
+
+/**
+ * pmic_reg_count: get the pmic register count
+ *
+ * The required pmic device can be obtained by 'pmic_get()'
+ *
+ * @dev - pointer to the UCLASS_PMIC device
+ * @return register count value on success or negative value of errno.
+ */
+int pmic_reg_count(struct udevice *dev);
+
+/**
+ * pmic_read/write: read/write to the UCLASS_PMIC device
+ *
+ * The required pmic device can be obtained by 'pmic_get()'
+ *
+ * @pmic   - pointer to the UCLASS_PMIC device
+ * @reg    - device register offset
+ * @buffer - pointer to read/write buffer
+ * @len    - byte count for read/write
+ * @return 0 on success or negative value of errno.
+ */
+int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len);
+int pmic_write(struct udevice *dev, uint reg, const uint8_t *buffer, int

len);

include/power/pmic.h +3 -0

+#endif /* CONFIG_DM_PMIC */
 
+#ifdef CONFIG_POWER
  int pmic_init(unsigned char bus);
  int power_init_board(void);
  int pmic_dialog_init(unsigned char bus);

@@ -88,6 +276,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);
+#endif
 

Quoted 4 lines

Quoted 4 lines

include/power/pmic.h +0 -1

  #define pmic_i2c_addr (p->hw.i2c.addr)
  #define pmic_i2c_tx_num (p->hw.i2c.tx_num)
--

1.9.1