From mboxrd@z Thu Jan  1 00:00:00 1970
From: Przemyslaw Marczak <p.marczak@samsung.com>
Date: Mon, 03 Aug 2015 17:01:12 +0200
Subject: [U-Boot] [PATCH 3/5] power: pmic: pfuze100 support driver model
In-Reply-To: <1438094935-18213-4-git-send-email-Peng.Fan@freescale.com>
References: <1438094935-18213-1-git-send-email-Peng.Fan@freescale.com>
 <1438094935-18213-4-git-send-email-Peng.Fan@freescale.com>
Message-ID: <55BF8238.8000902@samsung.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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@freescale.com>
> Cc: Przemyslaw Marczak <p.marczak@samsung.com>
> Cc: Simon Glass <sjg@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.

> @@ -2,15 +2,22 @@
>    * Copyright (C) 2014 Gateworks Corporation
>    * Tim Harvey <tharvey@gateworks.com>
>    *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc
> + * Peng Fan <Peng.Fan@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?

> +	{ .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.

> +	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.

> +	int regulators_node;
> +	const void *blob = gd->fdt_blob;
> +	int children;
> +

...snip...

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com