[U-Boot] [PATCH 3/5] power: pmic: pfuze100 support driver model

[Peng Fan]

Hello Przemyslaw,
On Mon, Aug 03, 2015 at 05:01:12PM +0200, Przemyslaw Marczak wrote:
>Hello Peng,
>
>I have few comments.
>
>On 07/28/2015 04:48 PM, Peng Fan wrote:
>>1. Support driver model for pfuze100.
>>2. Introduce a new Kconfig entry DM_PMIC_PFUZE100 for pfuze100
>>3. This driver intends to support PF100, PF200 and PF3000, so add
>>    the device id into the udevice_id array.
>>
>>Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
>>Cc: Przemyslaw Marczak <p.marczak at samsung.com>
>>Cc: Simon Glass <sjg at chromium.org>
>>---
>>  drivers/power/pmic/Kconfig         |  7 +++
>>  drivers/power/pmic/pmic_pfuze100.c | 89 ++++++++++++++++++++++++++++++++++++++
>>  include/power/pfuze100_pmic.h      |  3 ++
>>  3 files changed, 99 insertions(+)
>>
>>diff --git a/drivers/power/pmic/Kconfig b/drivers/power/pmic/Kconfig
>>index 164f421..0df91be 100644
>>--- a/drivers/power/pmic/Kconfig
>>+++ b/drivers/power/pmic/Kconfig
>>@@ -10,6 +10,13 @@ config DM_PMIC
>>  	- 'drivers/power/pmic/pmic-uclass.c'
>>  	- 'include/power/pmic.h'
>>
>>+config DM_PMIC_PFUZE100
>>+	bool "Enable Driver Model for PMIC PFUZE100"
>>+	depends on DM_PMIC
>>+	---help---
>>+	This config enables implementation of driver-model pmic uclass features
>>+	for PMIC PFUZE100. The driver implements read/write operations.
>>+
>>  config DM_PMIC_MAX77686
>>  	bool "Enable Driver Model for PMIC MAX77686"
>>  	depends on DM_PMIC
>>diff --git a/drivers/power/pmic/pmic_pfuze100.c b/drivers/power/pmic/pmic_pfuze100.c
>>index 22a04c0..be9d05c 100644
>>--- a/drivers/power/pmic/pmic_pfuze100.c
>>+++ b/drivers/power/pmic/pmic_pfuze100.c
>
>Please keep the new convention of file naming and use just
>pfuze100.c. Then, later we will remove the old files.

Ok. Will rename the file to pfuze100.c in V2.

>
>>@@ -2,15 +2,22 @@
>>   * Copyright (C) 2014 Gateworks Corporation
>>   * Tim Harvey <tharvey at gateworks.com>
>>   *
>>+ * Copyright (C) 2015 Freescale Semiconductor, Inc
>>+ * Peng Fan <Peng.Fan at freescale.com>
>>+ *
>>   * SPDX-License-Identifier:      GPL-2.0+
>>   */
>>
>>  #include <common.h>
>>+#include <fdtdec.h>
>>  #include <errno.h>
>>+#include <dm.h>
>>  #include <i2c.h>
>>  #include <power/pmic.h>
>>+#include <power/regulator.h>
>>  #include <power/pfuze100_pmic.h>
>>
>>+#ifndef CONFIG_DM_PMIC
>>  int power_pfuze100_init(unsigned char bus)
>>  {
>>  	static const char name[] = "PFUZE100";
>>@@ -30,3 +37,85 @@ int power_pfuze100_init(unsigned char bus)
>>
>>  	return 0;
>>  }
>>+#else
>>+DECLARE_GLOBAL_DATA_PTR;
>>+
>>+static const struct pmic_child_info pmic_children_info[] = {
>>+	/* sw[x], swbst */
>
>The driver name is used at two places, so could please define it in
>the header?

Ok. Will fix in V2.

>
>>+	{ .prefix = "s", .driver = "pfuze100_regulator" },
>>+	/* vgen[x], vsnvs, vcc, v33, vcc_sd */
>>+	{ .prefix = "v", .driver = "pfuze100_regulator" },
>>+	{ },
>>+};
>>+
>>+static int pfuze100_reg_count(struct udevice *dev)
>>+{
>
>This enum name is too general, so please add proper prefix.

Will fix this in V2.
>
>>+	return PMIC_NUM_OF_REGS;
>>+}
>>+
>>+static int pfuze100_write(struct udevice *dev, uint reg, const uint8_t *buff,
>>+			  int len)
>>+{
>>+	if (dm_i2c_write(dev, reg, buff, len)) {
>>+		error("write error to device: %p register: %#x!", dev, reg);
>>+		return -EIO;
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+static int pfuze100_read(struct udevice *dev, uint reg, uint8_t *buff, int len)
>>+{
>>+	if (dm_i2c_read(dev, reg, buff, len)) {
>>+		error("read error from device: %p register: %#x!", dev, reg);
>>+		return -EIO;
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+static int pfuze100_bind(struct udevice *dev)
>>+{
>
>Please sort the variables below.

Ok.

[...]

Regards,
Peng.
--