[U-Boot] [PATCH V2 4/6] power: pmic: pfuze100 support driver model

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello Peng,

On 08/04/2015 07:32 AM, 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.
> 4. Rename PMIC_NUM_OF_REGS macro to PFUZE100_NUM_OF_REGS.
> 5. Rename file name from pmic_pfuze100.c to pfuze100.c to follow new
>     new convention of file naming.
>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Cc: Przemyslaw Marczak <p.marczak@samsung.com>
> Cc: Simon Glass <sjg@chromium.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes v2:
>   Addressed Przemyslaw's comments:
>    Rename PMIC_NUM_OF_REGS to PFUZE100_NUM_OF_REGS
>    Sort variables' order
>    Define PFUZE100_REGULATOR_DRIVER for "pfuze100_regulator" in header file.
>
>   drivers/power/pmic/Makefile        |   2 +-
>   drivers/power/pmic/Kconfig         |   7 +++
>   drivers/power/pmic/pfuze100.c      | 121 +++++++++++++++++++++++++++++++++++++
>   drivers/power/pmic/pmic_pfuze100.c |  32 ----------
>   include/power/pfuze100_pmic.h      |   7 ++-
>   5 files changed, 135 insertions(+), 34 deletions(-)
>   create mode 100644 drivers/power/pmic/pfuze100.c
>   delete mode 100644 drivers/power/pmic/pmic_pfuze100.c
>
> diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile
> index 8c1ce3d..6eb357e 100644
> --- a/drivers/power/pmic/Makefile
> +++ b/drivers/power/pmic/Makefile
> @@ -13,7 +13,7 @@ obj-$(CONFIG_POWER_MAX8998) += pmic_max8998.o
>   obj-$(CONFIG_POWER_MAX8997) += pmic_max8997.o
>   obj-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o
>   obj-$(CONFIG_POWER_MAX77686) += pmic_max77686.o
> -obj-$(CONFIG_POWER_PFUZE100) += pmic_pfuze100.o

You add DM_PMIC_PFUZE100 to Kconfig, but here you check 
CONFIG_POWER_PFUZE100.

> +obj-$(CONFIG_POWER_PFUZE100) += pfuze100.o
>   obj-$(CONFIG_POWER_TPS65090_I2C) += pmic_tps65090.o
>   obj-$(CONFIG_POWER_TPS65090_EC) += pmic_tps65090_ec.o
>   obj-$(CONFIG_POWER_TPS65217) += pmic_tps65217.o
> 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/pfuze100.c b/drivers/power/pmic/pfuze100.c
> new file mode 100644
> index 0000000..8b26806
> --- /dev/null
> +++ b/drivers/power/pmic/pfuze100.c
> @@ -0,0 +1,121 @@
> +/*
> + * 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>
> +

Last time I meant, that the old code like this inside the ifndef, should 
stay in an old file, and the new one for DM in this new file.

Then you also don't need #ifdef, only separated config options in 
Makefile: CONFIG_DM_PMIC_PFUZE100 and CONFIG_POWER_PFUZE100.

> +#ifndef CONFIG_DM_PMIC
> +int power_pfuze100_init(unsigned char bus)
> +{
> +	static const char name[] = "PFUZE100";
> +	struct pmic *p = pmic_alloc();
> +
> +	if (!p) {
> +		printf("%s: POWER allocation error!\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	p->name = name;
> +	p->interface = PMIC_I2C;
> +	p->number_of_regs = PMIC_NUM_OF_REGS;
> +	p->hw.i2c.addr = CONFIG_POWER_PFUZE100_I2C_ADDR;
> +	p->hw.i2c.tx_num = 1;
> +	p->bus = bus;
> +
> +	return 0;
> +}
> +#else
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static const struct pmic_child_info pmic_children_info[] = {
> +	/* sw[x], swbst */
> +	{ .prefix = "s", .driver = PFUZE100_REGULATOR_DRIVER },
> +	/* vgen[x], vsnvs, vcc, v33, vcc_sd */
> +	{ .prefix = "v", .driver = PFUZE100_REGULATOR_DRIVER },
> +	{ },
> +};
> +
> +static int pfuze100_reg_count(struct udevice *dev)
> +{
> +	return PFUZE100_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)
> +{
> +	int children;
> +	int regulators_node;
> +	const void *blob = gd->fdt_blob;
> +
> +	regulators_node = fdt_subnode_offset(blob, dev->of_offset,
> +					     "regulators");
> +	if (regulators_node <= 0) {
> +		debug("%s: %s regulators subnode not found!", __func__,
> +		      dev->name);
> +		return -ENXIO;
> +	}
> +
> +	debug("%s: '%s' - found regulators subnode\n", __func__, dev->name);
> +
> +	children = pmic_bind_children(dev, regulators_node, pmic_children_info);
> +	if (!children)
> +		debug("%s: %s - no child found\n", __func__, dev->name);
> +
> +	/* Always return success for this device */
> +	return 0;
> +}
> +
> +static struct dm_pmic_ops pfuze100_ops = {
> +	.reg_count = pfuze100_reg_count,
> +	.read = pfuze100_read,
> +	.write = pfuze100_write,
> +};
> +
> +static const struct udevice_id pfuze100_ids[] = {

You don't need casting and the commas before parenthesis at the end of line.

{ .compatible = "fsl,pfuze100", .data = PFUZE100 },

> +	{ .compatible = "fsl,pfuze100", .data = (long)PFUZE100, },
> +	{ .compatible = "fsl,pfuze200", .data = (long)PFUZE200, },
> +	{ .compatible = "fsl,pfuze3000", .data = (long)PFUZE3000, },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(pmic_pfuze100) = {
> +	.name = "pfuze100 pmic",
> +	.id = UCLASS_PMIC,
> +	.of_match = pfuze100_ids,
> +	.bind = pfuze100_bind,
> +	.ops = &pfuze100_ops,
> +};
> +#endif
> diff --git a/drivers/power/pmic/pmic_pfuze100.c b/drivers/power/pmic/pmic_pfuze100.c
> deleted file mode 100644
> index 22a04c0..0000000

And this file can stay as it is, just add the new one with new config.

> --- a/drivers/power/pmic/pmic_pfuze100.c
> +++ /dev/null
> @@ -1,32 +0,0 @@
> -/*
> - * Copyright (C) 2014 Gateworks Corporation
> - * Tim Harvey <tharvey@gateworks.com>
> - *
> - * SPDX-License-Identifier:      GPL-2.0+
> - */
> -
> -#include <common.h>
> -#include <errno.h>
> -#include <i2c.h>
> -#include <power/pmic.h>
> -#include <power/pfuze100_pmic.h>
> -
> -int power_pfuze100_init(unsigned char bus)
> -{
> -	static const char name[] = "PFUZE100";
> -	struct pmic *p = pmic_alloc();
> -
> -	if (!p) {
> -		printf("%s: POWER allocation error!\n", __func__);
> -		return -ENOMEM;
> -	}
> -
> -	p->name = name;
> -	p->interface = PMIC_I2C;
> -	p->number_of_regs = PMIC_NUM_OF_REGS;
> -	p->hw.i2c.addr = CONFIG_POWER_PFUZE100_I2C_ADDR;
> -	p->hw.i2c.tx_num = 1;
> -	p->bus = bus;
> -
> -	return 0;
> -}
> diff --git a/include/power/pfuze100_pmic.h b/include/power/pfuze100_pmic.h
> index cb10605..c40a976 100644
> --- a/include/power/pfuze100_pmic.h
> +++ b/include/power/pfuze100_pmic.h
> @@ -8,6 +8,11 @@
>   #ifndef __PFUZE100_PMIC_H_
>   #define __PFUZE100_PMIC_H_
>
> +/* Device ID */
> +enum {PFUZE100 = 0x10, PFUZE200 = 0x11, PFUZE3000 = 0x30};
> +
> +#define PFUZE100_REGULATOR_DRIVER	"pfuze100_regulator"
> +
>   /* PFUZE100 registers */
>   enum {
>   	PFUZE100_DEVICEID	= 0x00,
> @@ -54,7 +59,7 @@ enum {
>   	PFUZE100_VGEN5VOL	= 0x70,
>   	PFUZE100_VGEN6VOL	= 0x71,
>
> -	PMIC_NUM_OF_REGS	= 0x7f,
> +	PFUZE100_NUM_OF_REGS	= 0x7f,
>   };
>
>   /*
>

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