[PATCH 02/10] support 88pm8606 in 860x driver

[PATCH 02/10] support 88pm8606 in 860x driver

[Haojian Zhuang]

[PATCH 02/10] support 88pm8606 in 860x driver

[Samuel Ortiz]

Hi Haojian,

On Fri, Nov 13, 2009 at 03:54:52AM -0500, Haojian Zhuang wrote:
> From 52a43fa137c45e62ced134b240c387b600f4ac0e Mon Sep 17 00:00:00 2001
> From: Haojian Zhuang <haojian.zhuang@marvell.com>
> Date: Fri, 6 Nov 2009 09:22:52 -0500
> Subject: [PATCH] mfd: support 88pm8606 in 860x driver
> 
> 88PM8606 and 88PM8607 are always used together. Although they're two discrete
> chips, the logic function is tightly linked. For example, USB charger driver
> is based on these two devices.
> 
> Now share one driver to these two devices. Create a logical chip structure
> that contains both 8606 device and 8607 device.
I think it's a really good idea to have one driver for those 2 devices.
However, I dont see much point in having the mixed_88pm860x common structure.
That forces you in having a static structure there for no real benefit. In
fact, by having it you're restricting yourself to the current HW
configuration, i.e. at most 1 8806 and 1 8807.

You could just have a struct pm860x_chip with buck3_double field (should be a
bool, by the way), a mutex, and be fine with it.

Besides this, the patch looks good to me.

Cheers,
Samuel.

> All I2C operations are accessed by 860x-i2c driver. In order to support both
> I2C client address, the read/write API is changed in below.
> 
> reg_read(chip, descriptor, offset)
> reg_write(chip, descriptor, offset, data)
> 
> At here, descriptor is DESC_8606 or DESC_8607 that means operation on which
> real device.
> 
> The benefit is that client drivers only need one kind of read/write API. I2C
> and MFD driver can be shared in both 8606 and 8607.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
> ---
>  drivers/mfd/88pm860x-core.c  |   72 ++++++++++++++++++++-------
>  drivers/mfd/88pm860x-i2c.c   |  112 ++++++++++++++++++++++++++++--------------
>  include/linux/mfd/88pm8607.h |   45 +++++++++++------
>  3 files changed, 158 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/mfd/88pm860x-core.c b/drivers/mfd/88pm860x-core.c
> index d1464e5..7485eeb 100644
> --- a/drivers/mfd/88pm860x-core.c
> +++ b/drivers/mfd/88pm860x-core.c
> @@ -1,5 +1,5 @@
>  /*
> - * Base driver for Marvell 88PM8607
> + * Base driver for Marvell 88PM860x
>   *
>   * Copyright (C) 2009 Marvell International Ltd.
>   * 	Haojian Zhuang <haojian.zhuang@marvell.com>
> @@ -67,13 +67,25 @@ static struct mfd_cell pm8607_devs[] = {
>  	PM8607_REG_DEVS(ldo14, LDO14),
>  };
> 
> -int pm860x_device_init(struct pm8607_chip *chip,
> -		       struct pm8607_platform_data *pdata)
> +static struct mixed_88pm860x mixed_chip;
> +
> +static int __devinit __88pm8606_init(struct pm860x_chip *chip, void *pdata)
>  {
> -	int i, count;
> -	int ret;
> +	chip->parent = &mixed_chip;
> +	mixed_chip.pm8606 = chip;
> +
> +	return 0;
> +}
> 
> -	ret = pm8607_reg_read(chip, PM8607_CHIP_ID);
> +static int __devinit __88pm8607_init(struct pm860x_chip *chip,
> +				     struct pm8607_plat_data *pdata)
> +{
> +	int ret = 0;
> +
> +	chip->parent = &mixed_chip;
> +	mixed_chip.pm8607 = chip;
> +
> +	ret = pm860x_reg_read(chip->parent, DESC_8607, PM8607_CHIP_ID);
>  	if (ret < 0) {
>  		dev_err(chip->dev, "Failed to read CHIP ID: %d\n", ret);
>  		goto out;
> @@ -88,47 +100,69 @@ int pm860x_device_init(struct pm8607_chip *chip,
>  	}
>  	chip->chip_id = ret;
> 
> -	ret = pm8607_reg_read(chip, PM8607_BUCK3);
> +	ret = pm860x_reg_read(chip->parent, DESC_8607, PM8607_BUCK3);
>  	if (ret < 0) {
>  		dev_err(chip->dev, "Failed to read BUCK3 register: %d\n", ret);
>  		goto out;
>  	}
>  	if (ret & PM8607_BUCK3_DOUBLE)
> -		chip->buck3_double = 1;
> +		chip->parent->buck3_double = 1;
> 
> -	ret = pm8607_reg_read(chip, PM8607_MISC1);
> +	ret = pm860x_reg_read(chip->parent, DESC_8607, PM8607_MISC1);
>  	if (ret < 0) {
>  		dev_err(chip->dev, "Failed to read MISC1 register: %d\n", ret);
>  		goto out;
>  	}
> -	if (pdata->i2c_port == PI2C_PORT)
> +	if (pdata && (pdata->i2c_port == PI2C_PORT))
>  		ret |= PM8607_MISC1_PI2C;
>  	else
>  		ret &= ~PM8607_MISC1_PI2C;
> -	ret = pm8607_reg_write(chip, PM8607_MISC1, ret);
> +	ret = pm860x_reg_write(chip->parent, DESC_8607, PM8607_MISC1, ret);
>  	if (ret < 0) {
>  		dev_err(chip->dev, "Failed to write MISC1 register: %d\n", ret);
>  		goto out;
>  	}
> +out:
> +	return ret;
> +}
> +
> +int __devinit pm860x_device_init(struct pm860x_chip *chip, void *pdata)
> +{
> +	int i, count;
> +	int ret = -EINVAL;
> +
> +	/* Mixed chip isn't initialized yet. */
> +	if ((mixed_chip.flags & MIXED_FLAG_INITIALIZED) == 0)
> +		mutex_init(&mixed_chip.io_lock);
> 
> -	count = ARRAY_SIZE(pm8607_devs);
> -	for (i = 0; i < count; i++) {
> -		ret = mfd_add_devices(chip->dev, i, &pm8607_devs[i],
> -				      1, NULL, 0);
> -		if (ret != 0) {
> -			dev_err(chip->dev, "Failed to add subdevs\n");
> +	if (!strcmp(chip->id.name, "88PM8607")) {
> +		ret = __88pm8607_init(chip, (struct pm8607_plat_data *)pdata);
> +		if (ret < 0)
>  			goto out;
> +
> +		count = ARRAY_SIZE(pm8607_devs);
> +		for (i = 0; i < count; i++) {
> +			ret = mfd_add_devices(chip->dev, i, &pm8607_devs[i],
> +					      1, NULL, 0);
> +			if (ret != 0) {
> +				dev_err(chip->dev, "Failed to add subdevs\n");
> +				goto out;
> +			}
>  		}
> +	} else if (!strcmp(chip->id.name, "88PM8606")) {
> +		ret = __88pm8606_init(chip, pdata);
> +		if (ret < 0)
> +			goto out;
>  	}
>  out:
>  	return ret;
>  }
> 
> -void pm8607_device_exit(struct pm8607_chip *chip)
> +void __devexit pm8607_device_exit(struct pm860x_chip *chip)
>  {
>  	mfd_remove_devices(chip->dev);
>  }
> 
> -MODULE_DESCRIPTION("PMIC Driver for Marvell 88PM8607");
> +MODULE_DESCRIPTION("PMIC Driver for Marvell 88PM860x");
>  MODULE_AUTHOR("Haojian Zhuang <haojian.zhuang@marvell.com>");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/88pm860x-i2c.c b/drivers/mfd/88pm860x-i2c.c
> index dda23cb..5738cf1 100644
> --- a/drivers/mfd/88pm860x-i2c.c
> +++ b/drivers/mfd/88pm860x-i2c.c
> @@ -1,5 +1,5 @@
>  /*
> - * I2C driver for Marvell 88PM8607
> + * I2C driver for Marvell 88PM860x
>   *
>   * Copyright (C) 2009 Marvell International Ltd.
>   * 	Haojian Zhuang <haojian.zhuang@marvell.com>
> @@ -14,8 +14,8 @@
>  #include <linux/i2c.h>
>  #include <linux/mfd/88pm8607.h>
> 
> -static inline int pm8607_read_device(struct pm8607_chip *chip,
> -				     int reg, int bytes, void *dest)
> +static inline int __88pm860x_read(struct pm860x_chip *chip,
> +				  int reg, int bytes, void *dest)
>  {
>  	struct i2c_client *i2c = chip->client;
>  	unsigned char data;
> @@ -32,8 +32,8 @@ static inline int pm8607_read_device(struct pm8607_chip *chip,
>  	return 0;
>  }
> 
> -static inline int pm8607_write_device(struct pm8607_chip *chip,
> -				      int reg, int bytes, void *src)
> +static inline int __88pm860x_write(struct pm860x_chip *chip,
> +				   int reg, int bytes, void *src)
>  {
>  	struct i2c_client *i2c = chip->client;
>  	unsigned char buf[bytes + 1];
> @@ -48,82 +48,123 @@ static inline int pm8607_write_device(struct
> pm8607_chip *chip,
>  	return 0;
>  }
> 
> -int pm8607_reg_read(struct pm8607_chip *chip, int reg)
> +/* Get the chip that is specified by descriptor */
> +#define MATCH_860X_CHIP(p, c, id)			\
> +do {							\
> +	if (p == NULL)					\
> +		goto out;				\
> +	if ((id == DESC_8606) && (p->pm8606))		\
> +		c = p->pm8606;				\
> +	else if ((id == DESC_8607) && (p->pm8607))	\
> +		c = p->pm8607;				\
> +	else						\
> +		goto out;				\
> +} while (0)
> +
> +int pm860x_reg_read(struct mixed_88pm860x *parent, int desc, int reg)
>  {
> +	struct pm860x_chip *chip = NULL;
>  	unsigned char data;
>  	int ret;
> 
> -	mutex_lock(&chip->io_lock);
> +	MATCH_860X_CHIP(parent, chip, desc);
> +
> +	mutex_lock(&parent->io_lock);
>  	ret = chip->read(chip, reg, 1, &data);
> -	mutex_unlock(&chip->io_lock);
> +	mutex_unlock(&parent->io_lock);
> +	dev_dbg(chip->dev, "read [%d]:0x%x\n", reg, ret);
> 
>  	if (ret < 0)
>  		return ret;
>  	else
>  		return (int)data;
> +out:
> +	return -EINVAL;
>  }
> -EXPORT_SYMBOL(pm8607_reg_read);
> +EXPORT_SYMBOL(pm860x_reg_read);
> 
> -int pm8607_reg_write(struct pm8607_chip *chip, int reg,
> +int pm860x_reg_write(struct mixed_88pm860x *parent, int desc, int reg,
>  		     unsigned char data)
>  {
> +	struct pm860x_chip *chip = NULL;
>  	int ret;
> 
> -	mutex_lock(&chip->io_lock);
> +	MATCH_860X_CHIP(parent, chip, desc);
> +
> +	mutex_lock(&parent->io_lock);
>  	ret = chip->write(chip, reg, 1, &data);
> -	mutex_unlock(&chip->io_lock);
> +	mutex_unlock(&parent->io_lock);
> +	dev_dbg(chip->dev, "write [%d]:0x%x\n", reg, data);
> 
>  	return ret;
> +out:
> +	return -EINVAL;
>  }
> -EXPORT_SYMBOL(pm8607_reg_write);
> +EXPORT_SYMBOL(pm860x_reg_write);
> 
> -int pm8607_bulk_read(struct pm8607_chip *chip, int reg,
> +int pm860x_bulk_read(struct mixed_88pm860x *parent, int desc, int reg,
>  		     int count, unsigned char *buf)
>  {
> +	struct pm860x_chip *chip = NULL;
>  	int ret;
> 
> -	mutex_lock(&chip->io_lock);
> +	MATCH_860X_CHIP(parent, chip, desc);
> +
> +	mutex_lock(&parent->io_lock);
>  	ret = chip->read(chip, reg, count, buf);
> -	mutex_unlock(&chip->io_lock);
> +	mutex_unlock(&parent->io_lock);
> 
>  	return ret;
> +out:
> +	return -EINVAL;
>  }
> -EXPORT_SYMBOL(pm8607_bulk_read);
> +EXPORT_SYMBOL(pm860x_bulk_read);
> 
> -int pm8607_bulk_write(struct pm8607_chip *chip, int reg,
> +int pm860x_bulk_write(struct mixed_88pm860x *parent, int desc, int reg,
>  		      int count, unsigned char *buf)
>  {
> +	struct pm860x_chip *chip = NULL;
>  	int ret;
> 
> -	mutex_lock(&chip->io_lock);
> +	MATCH_860X_CHIP(parent, chip, desc);
> +
> +	mutex_lock(&parent->io_lock);
>  	ret = chip->write(chip, reg, count, buf);
> -	mutex_unlock(&chip->io_lock);
> +	mutex_unlock(&parent->io_lock);
> 
>  	return ret;
> +out:
> +	return -EINVAL;
>  }
> -EXPORT_SYMBOL(pm8607_bulk_write);
> +EXPORT_SYMBOL(pm860x_bulk_write);
> 
> -int pm8607_set_bits(struct pm8607_chip *chip, int reg,
> +int pm860x_set_bits(struct mixed_88pm860x *parent, int desc, int reg,
>  		    unsigned char mask, unsigned char data)
>  {
> +	struct pm860x_chip *chip = NULL;
>  	unsigned char value;
>  	int ret;
> 
> -	mutex_lock(&chip->io_lock);
> +	MATCH_860X_CHIP(parent, chip, desc);
> +
> +	mutex_lock(&parent->io_lock);
>  	ret = chip->read(chip, reg, 1, &value);
>  	if (ret < 0)
> -		goto out;
> +		goto out_rd;
>  	value &= ~mask;
>  	value |= data;
>  	ret = chip->write(chip, reg, 1, &value);
> -out:
> -	mutex_unlock(&chip->io_lock);
> +out_rd:
> +	mutex_unlock(&parent->io_lock);
> +	dev_dbg(chip->dev, "set bits [%d]:0x%x, ret:%d\n", reg, data, ret);
> +
>  	return ret;
> +out:
> +	return -EINVAL;
>  }
> -EXPORT_SYMBOL(pm8607_set_bits);
> -
> 
>  static const struct i2c_device_id pm860x_id_table[] = {
> +	{ "88PM8606", 0 },
>  	{ "88PM8607", 0 },
>  	{}
>  };
> @@ -132,31 +173,28 @@ MODULE_DEVICE_TABLE(i2c, pm860x_id_table);
>  static int __devinit pm860x_probe(struct i2c_client *client,
>  				  const struct i2c_device_id *id)
>  {
> -	struct pm8607_platform_data *pdata = client->dev.platform_data;
> -	struct pm8607_chip *chip;
> +	struct pm860x_chip *chip;
> +	struct pm860x_plat_data *pdata;
>  	int ret;
> 
> -	chip = kzalloc(sizeof(struct pm8607_chip), GFP_KERNEL);
> +	chip = kzalloc(sizeof(struct pm860x_chip), GFP_KERNEL);
>  	if (chip == NULL)
>  		return -ENOMEM;
> 
>  	chip->client = client;
>  	chip->dev = &client->dev;
> -	chip->read = pm8607_read_device;
> -	chip->write = pm8607_write_device;
> +	chip->read = __88pm860x_read;
> +	chip->write = __88pm860x_write;
>  	memcpy(&chip->id, id, sizeof(struct i2c_device_id));
>  	i2c_set_clientdata(client, chip);
> 
> -	mutex_init(&chip->io_lock);
>  	dev_set_drvdata(chip->dev, chip);
> -
> +	pdata = (struct pm860x_plat_data *)client->dev.platform_data;
>  	ret = pm860x_device_init(chip, pdata);
>  	if (ret < 0)
>  		goto out;
> 
> -
>  	return 0;
> -
>  out:
>  	i2c_set_clientdata(client, NULL);
>  	kfree(chip);
> diff --git a/include/linux/mfd/88pm8607.h b/include/linux/mfd/88pm8607.h
> index 6e4dcdc..bacc1f9 100644
> --- a/include/linux/mfd/88pm8607.h
> +++ b/include/linux/mfd/88pm8607.h
> @@ -13,6 +13,13 @@
>  #define __LINUX_MFD_88PM8607_H
> 
>  enum {
> +	DESC_INVAL = 0,
> +	DESC_8606,
> +	DESC_8607,
> +	DESC_MAX,
> +};
> +
> +enum {
>  	PM8607_ID_BUCK1 = 0,
>  	PM8607_ID_BUCK2,
>  	PM8607_ID_BUCK3,
> @@ -180,19 +187,28 @@ enum {
>  	PM8607_CHIP_B0 = 0x48,
>  };
> 
> +struct pm860x_chip;
> 
> -struct pm8607_chip {
> +#define MIXED_FLAG_INITIALIZED		(1 << 0)
> +
> +struct mixed_88pm860x {
> +	struct pm860x_chip	*pm8606;
> +	struct pm860x_chip	*pm8607;
> +	struct mutex		io_lock;
> +	int			buck3_double;	/* DVC ramp slope double */
> +	int			flags;
> +};
> +
> +struct pm860x_chip {
>  	struct device		*dev;
>  	struct mutex		io_lock;
>  	struct i2c_client	*client;
>  	struct i2c_device_id	id;
> -
> -	int (*read)(struct pm8607_chip *chip, int reg, int bytes, void *dest);
> -	int (*write)(struct pm8607_chip *chip, int reg, int bytes, void *src);
> -
> -	int			buck3_double;	/* DVC ramp slope double */
> +	struct mixed_88pm860x	*parent;
>  	unsigned char		chip_id;
> 
> +	int (*read)(struct pm860x_chip *chip, int reg, int bytes, void *dest);
> +	int (*write)(struct pm860x_chip *chip, int reg, int bytes, void *src);
>  };
> 
>  #define PM8607_MAX_REGULATOR	15	/* 3 Bucks, 12 LDOs */
> @@ -202,22 +218,21 @@ enum {
>  	PI2C_PORT,
>  };
> 
> -struct pm8607_platform_data {
> +struct pm8607_plat_data {
>  	int	i2c_port;	/* Controlled by GI2C or PI2C */
>  	struct regulator_init_data *regulator[PM8607_MAX_REGULATOR];
>  };
> 
> -extern int pm8607_reg_read(struct pm8607_chip *, int);
> -extern int pm8607_reg_write(struct pm8607_chip *, int, unsigned char);
> -extern int pm8607_bulk_read(struct pm8607_chip *, int, int,
> +extern int pm860x_reg_read(struct mixed_88pm860x *, int, int);
> +extern int pm860x_reg_write(struct mixed_88pm860x *, int, int, unsigned char);
> +extern int pm860x_bulk_read(struct mixed_88pm860x *, int, int, int,
>  			    unsigned char *);
> -extern int pm8607_bulk_write(struct pm8607_chip *, int, int,
> +extern int pm860x_bulk_write(struct mixed_88pm860x *, int, int, int,
>  			     unsigned char *);
> -extern int pm8607_set_bits(struct pm8607_chip *, int, unsigned char,
> +extern int pm860x_set_bits(struct mixed_88pm860x *, int, int, unsigned char,
>  			   unsigned char);
> 
> -extern int pm860x_device_init(struct pm8607_chip *chip,
> -			      struct pm8607_platform_data *pdata);
> -extern void pm860x_device_exit(struct pm8607_chip *chip);
> +extern int pm860x_device_init(struct pm860x_chip *chip, void *pdata);
> +extern void pm860x_device_exit(struct pm860x_chip *chip);
> 
>  #endif /* __LINUX_MFD_88PM860X_H */
> -- 
> 1.5.6.5

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 02/10] support 88pm8606 in 860x driver

[Haojian Zhuang]

On Fri, Nov 20, 2009 at 11:02 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> Hi Haojian,
>
> On Fri, Nov 13, 2009 at 03:54:52AM -0500, Haojian Zhuang wrote:
>> From 52a43fa137c45e62ced134b240c387b600f4ac0e Mon Sep 17 00:00:00 2001
>> From: Haojian Zhuang <haojian.zhuang@marvell.com>
>> Date: Fri, 6 Nov 2009 09:22:52 -0500
>> Subject: [PATCH] mfd: support 88pm8606 in 860x driver
>>
>> 88PM8606 and 88PM8607 are always used together. Although they're two discrete
>> chips, the logic function is tightly linked. For example, USB charger driver
>> is based on these two devices.
>>
>> Now share one driver to these two devices. Create a logical chip structure
>> that contains both 8606 device and 8607 device.
> I think it's a really good idea to have one driver for those 2 devices.
> However, I dont see much point in having the mixed_88pm860x common structure.
> That forces you in having a static structure there for no real benefit. In
> fact, by having it you're restricting yourself to the current HW
> configuration, i.e. at most 1 8806 and 1 8807.
Yes, I just want to restrict at most 1 8806 and 1 8807 in system. Or
we should have 1 8806 or 1 8807 in system.
Although I defined mixed_88pm860x common structure, I didn't use it
directly. I used pm860x_reg_read() to make use of this structure. In
all I2C operations, there's a descriptor indicator. This indicator is
used call 8807 operation in 8806 client. Since USB charger is the
device of 8806, it also need to access 8807 register. Otherwise, I
have to assert two i2c clients in 8806 device driver.
The only benefit of mixed_88pm860x is linking 8806 and 8807 together.
So 8806 device could directly fetch registers of 8807 device.
>
> You could just have a struct pm860x_chip with buck3_double field (should be a
> bool, by the way), a mutex, and be fine with it.
Yes, only one mutex is enough. In the 0002 patch, I defined mutex in
8806, 8807 and mixed_88pm860x. It's over-designed. I removed mutex in
8806 and 8807 in the followed patches. So there's only one mutex in
mixed_88pm860x structure.
>
> Besides this, the patch looks good to me.
>
> Cheers,
> Samuel.
>
>> All I2C operations are accessed by 860x-i2c driver. In order to support both
>> I2C client address, the read/write API is changed in below.
>>
>> reg_read(chip, descriptor, offset)
>> reg_write(chip, descriptor, offset, data)
>>
>> At here, descriptor is DESC_8606 or DESC_8607 that means operation on which
>> real device.
>>
>> The benefit is that client drivers only need one kind of read/write API. I2C
>> and MFD driver can be shared in both 8606 and 8607.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
>> ---

[PATCH 02/10] support 88pm8606 in 860x driver

[Samuel Ortiz]

Hi Haojian,

On Sun, Nov 22, 2009 at 08:25:48AM +0800, Haojian Zhuang wrote:
> On Fri, Nov 20, 2009 at 11:02 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> > Hi Haojian,
> >
> > On Fri, Nov 13, 2009 at 03:54:52AM -0500, Haojian Zhuang wrote:
> >> From 52a43fa137c45e62ced134b240c387b600f4ac0e Mon Sep 17 00:00:00 2001
> >> From: Haojian Zhuang <haojian.zhuang@marvell.com>
> >> Date: Fri, 6 Nov 2009 09:22:52 -0500
> >> Subject: [PATCH] mfd: support 88pm8606 in 860x driver
> >>
> >> 88PM8606 and 88PM8607 are always used together. Although they're two discrete
> >> chips, the logic function is tightly linked. For example, USB charger driver
> >> is based on these two devices.
> >>
> >> Now share one driver to these two devices. Create a logical chip structure
> >> that contains both 8606 device and 8607 device.
> > I think it's a really good idea to have one driver for those 2 devices.
> > However, I dont see much point in having the mixed_88pm860x common structure.
> > That forces you in having a static structure there for no real benefit. In
> > fact, by having it you're restricting yourself to the current HW
> > configuration, i.e. at most 1 8806 and 1 8807.
> Yes, I just want to restrict at most 1 8806 and 1 8807 in system. Or
> we should have 1 8806 or 1 8807 in system.
> Although I defined mixed_88pm860x common structure, I didn't use it
> directly. I used pm860x_reg_read() to make use of this structure. In
> all I2C operations, there's a descriptor indicator. This indicator is
> used call 8807 operation in 8806 client. Since USB charger is the
> device of 8806, it also need to access 8807 register. Otherwise, I
> have to assert two i2c clients in 8806 device driver.
Ok, I dont have the HW specs for those 2 devices, but it seems to me that
wanting to link those 2 is a platform design, not a driver one. In other
words, I dont see why it wouldnt be possible to have a platform with e.g only
one 8806 or only one 8807. Is that correct ? If that's so, then you need to
re-design this driver and forget about your static mixed structure.
If your platform is designed so that you need to access one of the potentially
available 8807 from one of the potentially available 8806, then you need to
define an API to which you'd pass some platform data that would basically tell
which of the 8807 you want to access.

On the other hand if it is impossible from a HW point of view to have a
platform design where you'd have either a single 8806 or a single 8807, then 2
things:
- Your driver design would be ok.
- Those chip designs are really bad as it should be one and only one single
(and in particular only use one single i2c address space and id)

Cheers,
Samuel.


> The only benefit of mixed_88pm860x is linking 8806 and 8807 together.
> So 8806 device could directly fetch registers of 8807 device.
> >
> > You could just have a struct pm860x_chip with buck3_double field (should be a
> > bool, by the way), a mutex, and be fine with it.
> Yes, only one mutex is enough. In the 0002 patch, I defined mutex in
> 8806, 8807 and mixed_88pm860x. It's over-designed. I removed mutex in
> 8806 and 8807 in the followed patches. So there's only one mutex in
> mixed_88pm860x structure.
> >
> > Besides this, the patch looks good to me.
> >
> > Cheers,
> > Samuel.
> >
> >> All I2C operations are accessed by 860x-i2c driver. In order to support both
> >> I2C client address, the read/write API is changed in below.
> >>
> >> reg_read(chip, descriptor, offset)
> >> reg_write(chip, descriptor, offset, data)
> >>
> >> At here, descriptor is DESC_8606 or DESC_8607 that means operation on which
> >> real device.
> >>
> >> The benefit is that client drivers only need one kind of read/write API. I2C
> >> and MFD driver can be shared in both 8606 and 8607.
> >>
> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
> >> ---

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 02/10] support 88pm8606 in 860x driver

[Haojian Zhuang]

On Fri, Nov 27, 2009 at 9:58 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> Hi Haojian,
>
> On Sun, Nov 22, 2009 at 08:25:48AM +0800, Haojian Zhuang wrote:
>> On Fri, Nov 20, 2009 at 11:02 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
>> > Hi Haojian,
>> >
>> > On Fri, Nov 13, 2009 at 03:54:52AM -0500, Haojian Zhuang wrote:
>> >> From 52a43fa137c45e62ced134b240c387b600f4ac0e Mon Sep 17 00:00:00 2001
>> >> From: Haojian Zhuang <haojian.zhuang@marvell.com>
>> >> Date: Fri, 6 Nov 2009 09:22:52 -0500
>> >> Subject: [PATCH] mfd: support 88pm8606 in 860x driver
>> >>
>> >> 88PM8606 and 88PM8607 are always used together. Although they're two discrete
>> >> chips, the logic function is tightly linked. For example, USB charger driver
>> >> is based on these two devices.
>> >>
>> >> Now share one driver to these two devices. Create a logical chip structure
>> >> that contains both 8606 device and 8607 device.
>> > I think it's a really good idea to have one driver for those 2 devices.
>> > However, I dont see much point in having the mixed_88pm860x common structure.
>> > That forces you in having a static structure there for no real benefit. In
>> > fact, by having it you're restricting yourself to the current HW
>> > configuration, i.e. at most 1 8806 and 1 8807.
>> Yes, I just want to restrict at most 1 8806 and 1 8807 in system. Or
>> we should have 1 8806 or 1 8807 in system.
>> Although I defined mixed_88pm860x common structure, I didn't use it
>> directly. I used pm860x_reg_read() to make use of this structure. In
>> all I2C operations, there's a descriptor indicator. This indicator is
>> used call 8807 operation in 8806 client. Since USB charger is the
>> device of 8806, it also need to access 8807 register. Otherwise, I
>> have to assert two i2c clients in 8806 device driver.
> Ok, I dont have the HW specs for those 2 devices, but it seems to me that
> wanting to link those 2 is a platform design, not a driver one. In other
[Haojian]: The original purpose of designing 8806 and 8807 is for
phone. 8807 carries buck converter, LDOs, charging unit, RTC, audio
codec, touch, vibrator and GPIOs. 8806 carries boost converter (supply
WLED), RGB LED, charger, VBUS switcher and external vibrator. 8807
processes low level voltage and 8806 processes high level voltage.
They're different chips, but they work together.

> words, I dont see why it wouldnt be possible to have a platform with e.g only
> one 8806 or only one 8807. Is that correct ? If that's so, then you need to
[Haojian]: If customer likes, they can only enable 8807 or 8806. And
my driver supports this functionality. If we just want to enable 8807,
we shouldn't enable backlight, LED, charger, and so on. If we just
want to enable 8806, we shouldn't enable touch, RTC, LDO, audio codec,
and so on. Driver will check the platform data. If platform data isn't
specified, related driver won't be mounted. That mixed structure is
just a glue logic for linking these two drivers. If we want to have
the functionality of USB charger, we have to make use of both 8807 and
8806. Because 8807 and 8806 has different I2C address. I have to let
usb charger as the sub-device of 8806, and it needs to access 8807
registers since some charger control logic is placed in 8807. That's
the main reason of using mixed structure for glue logic.
> re-design this driver and forget about your static mixed structure.
> If your platform is designed so that you need to access one of the potentially
> available 8807 from one of the potentially available 8806, then you need to
> define an API to which you'd pass some platform data that would basically tell
> which of the 8807 you want to access.
>
> On the other hand if it is impossible from a HW point of view to have a
> platform design where you'd have either a single 8806 or a single 8807, then 2
> things:
> - Your driver design would be ok.
[Haojian]: Great Thanks :)
> - Those chip designs are really bad as it should be one and only one single
> (and in particular only use one single i2c address space and id)
[Haojian]: I agree this opinion that the same functionality logic
should be placed into one chip and using single i2c address. If same
functionality logic can be in one chip, the driver could be simpler
without glue logic.
>
> Cheers,
> Samuel.
>
>
Hi Samuel,

I tried to add some comments of the chip's background. Up to now, I
can't find a better idea to handle this driver without mixed structure
since usb charger linkes them tightly. If there's a better prototype
to handle this, I'll adopt it.

Thanks
Haojian

[PATCH 02/10] support 88pm8606 in 860x driver

[Samuel Ortiz]

Hi Haojian,

On Sun, Nov 29, 2009 at 08:43:45PM -0500, Haojian Zhuang wrote:
> > On the other hand if it is impossible from a HW point of view to have a
> > platform design where you'd have either a single 8806 or a single 8807, then 2
> > things:
> > - Your driver design would be ok.
> [Haojian]: Great Thanks :)
> > - Those chip designs are really bad as it should be one and only one single
> > (and in particular only use one single i2c address space and id)
> [Haojian]: I agree this opinion that the same functionality logic
> should be placed into one chip and using single i2c address. If same
> functionality logic can be in one chip, the driver could be simpler
> without glue logic.
> >
> > Cheers,
> > Samuel.
> >
> >
> Hi Samuel,
> 
> I tried to add some comments of the chip's background. Up to now, I
> can't find a better idea to handle this driver without mixed structure
> since usb charger linkes them tightly. If there's a better prototype
> to handle this, I'll adopt it.
Thanks for the explanations, I appreciate.
Only the board definition can describe what's the relationship between your
8806 and 8807, so here is what I propose: Add an i2c address to your 880x
platform data, and have only a 8806 definition on your board specific file.
Then from the 880x driver, you check for the platform data i2c address. If
it's a valid one, that means we are expecting a 8807 to be present on the
board and thus we can instantiate this i2c device by calling i2c_new_device().
That will give you a link to your 8807 i2c client from your 8806. The
advantage of that solution is to keep the 880x driver board and design
neutral while avoiding any static definitions.
Waiting for your feedback.

Cheers,
Samuel.
 
> Thanks
> Haojian

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 02/10] support 88pm8606 in 860x driver

[Haojian Zhuang]

On Mon, Nov 30, 2009 at 5:19 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> Hi Haojian,
>
> On Sun, Nov 29, 2009 at 08:43:45PM -0500, Haojian Zhuang wrote:
>> > On the other hand if it is impossible from a HW point of view to have a
>> > platform design where you'd have either a single 8806 or a single 8807, then 2
>> > things:
>> > - Your driver design would be ok.
>> [Haojian]: Great Thanks :)
>> > - Those chip designs are really bad as it should be one and only one single
>> > (and in particular only use one single i2c address space and id)
>> [Haojian]: I agree this opinion that the same functionality logic
>> should be placed into one chip and using single i2c address. If same
>> functionality logic can be in one chip, the driver could be simpler
>> without glue logic.
>> >
>> > Cheers,
>> > Samuel.
>> >
>> >
>> Hi Samuel,
>>
>> I tried to add some comments of the chip's background. Up to now, I
>> can't find a better idea to handle this driver without mixed structure
>> since usb charger linkes them tightly. If there's a better prototype
>> to handle this, I'll adopt it.
> Thanks for the explanations, I appreciate.
> Only the board definition can describe what's the relationship between your
> 8806 and 8807, so here is what I propose: Add an i2c address to your 880x
> platform data, and have only a 8806 definition on your board specific file.
> Then from the 880x driver, you check for the platform data i2c address. If
> it's a valid one, that means we are expecting a 8807 to be present on the
> board and thus we can instantiate this i2c device by calling i2c_new_device().
> That will give you a link to your 8807 i2c client from your 8806. The
> advantage of that solution is to keep the 880x driver board and design
> neutral while avoiding any static definitions.
> Waiting for your feedback.
>
Hi Samuel,

Excuse me to response late. If I implement i2c_new_device(), I should
have to create adapter, algorithm and so on. It means that I have to
implement i2c-pxa.c duplicately in mfd driver. Is it right?

And you want to us i2c_new_device(). Does it mean that there's only
8607 device and address in platform driver and mfd driver
distinguishing 8606 from platform data?

Thanks
Haojian

[PATCH 02/10] support 88pm8606 in 860x driver

[Samuel Ortiz]

Hi Haojian

On Mon, Dec 07, 2009 at 02:34:58AM -0500, Haojian Zhuang wrote:
> On Mon, Nov 30, 2009 at 5:19 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> > Hi Haojian,
> >
> > On Sun, Nov 29, 2009 at 08:43:45PM -0500, Haojian Zhuang wrote:
> >> > On the other hand if it is impossible from a HW point of view to have a
> >> > platform design where you'd have either a single 8806 or a single 8807, then 2
> >> > things:
> >> > - Your driver design would be ok.
> >> [Haojian]: Great Thanks :)
> >> > - Those chip designs are really bad as it should be one and only one single
> >> > (and in particular only use one single i2c address space and id)
> >> [Haojian]: I agree this opinion that the same functionality logic
> >> should be placed into one chip and using single i2c address. If same
> >> functionality logic can be in one chip, the driver could be simpler
> >> without glue logic.
> >> >
> >> > Cheers,
> >> > Samuel.
> >> >
> >> >
> >> Hi Samuel,
> >>
> >> I tried to add some comments of the chip's background. Up to now, I
> >> can't find a better idea to handle this driver without mixed structure
> >> since usb charger linkes them tightly. If there's a better prototype
> >> to handle this, I'll adopt it.
> > Thanks for the explanations, I appreciate.
> > Only the board definition can describe what's the relationship between your
> > 8806 and 8807, so here is what I propose: Add an i2c address to your 880x
> > platform data, and have only a 8806 definition on your board specific file.
> > Then from the 880x driver, you check for the platform data i2c address. If
> > it's a valid one, that means we are expecting a 8807 to be present on the
> > board and thus we can instantiate this i2c device by calling i2c_new_device().
> > That will give you a link to your 8807 i2c client from your 8806. The
> > advantage of that solution is to keep the 880x driver board and design
> > neutral while avoiding any static definitions.
> > Waiting for your feedback.
> >
> Hi Samuel,
> 
> Excuse me to response late. If I implement i2c_new_device(), I should
> have to create adapter, algorithm and so on. It means that I have to
> implement i2c-pxa.c duplicately in mfd driver. Is it right?
I dont think so, no. i2c_new_device should be called at i2c probe time. Your
8806 is probed, and you then check the corresponding platform data. If there
is a valid additional 8807 i2c address, it means that your platform comes with
a 8807. You then need to call i2c_new_device(), on the same adapter as your
8806 is sitting in, i.e. i2c_client->adapter. You would build the 8807
board_info structure from your platform data i2c address.
Jean does that make sense from an i2c point of view ? Please let me know if
you need more background on this issue.


> And you want to us i2c_new_device(). Does it mean that there's only
> 8607 device and address in platform driver and mfd driver
> distinguishing 8606 from platform data?
That's correct, yes. As I said, the only place where you can correctly
describe the relationship between your 8806 and 8807 is your platform data.

Cheers,
Samuel.

> Thanks
> Haojian

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 02/10] support 88pm8606 in 860x driver

[Jean Delvare]

On Mon, 7 Dec 2009 12:39:28 +0100, Samuel Ortiz wrote:
> Hi Haojian
> 
> On Mon, Dec 07, 2009 at 02:34:58AM -0500, Haojian Zhuang wrote:
> > On Mon, Nov 30, 2009 at 5:19 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> > > On Sun, Nov 29, 2009 at 08:43:45PM -0500, Haojian Zhuang wrote:
> > >> I tried to add some comments of the chip's background. Up to now, I
> > >> can't find a better idea to handle this driver without mixed structure
> > >> since usb charger linkes them tightly. If there's a better prototype
> > >> to handle this, I'll adopt it.
> > > Thanks for the explanations, I appreciate.
> > > Only the board definition can describe what's the relationship between your
> > > 8806 and 8807, so here is what I propose: Add an i2c address to your 880x
> > > platform data, and have only a 8806 definition on your board specific file.
> > > Then from the 880x driver, you check for the platform data i2c address. If
> > > it's a valid one, that means we are expecting a 8807 to be present on the
> > > board and thus we can instantiate this i2c device by calling i2c_new_device().
> > > That will give you a link to your 8807 i2c client from your 8806. The
> > > advantage of that solution is to keep the 880x driver board and design
> > > neutral while avoiding any static definitions.
> > > Waiting for your feedback.
> > >
> > Hi Samuel,
> > 
> > Excuse me to response late. If I implement i2c_new_device(), I should
> > have to create adapter, algorithm and so on. It means that I have to
> > implement i2c-pxa.c duplicately in mfd driver. Is it right?
> I dont think so, no. i2c_new_device should be called at i2c probe time. Your
> 8806 is probed, and you then check the corresponding platform data. If there
> is a valid additional 8807 i2c address, it means that your platform comes with
> a 8807. You then need to call i2c_new_device(), on the same adapter as your
> 8806 is sitting in, i.e. i2c_client->adapter. You would build the 8807
> board_info structure from your platform data i2c address.
> Jean does that make sense from an i2c point of view ? Please let me know if
> you need more background on this issue.

Some more background would be welcome, yes. In particular on how the
8806 and 8807 relate to each other, and what kind of chips they are.
And what problem you are trying to solve in the first place ;)

In any case, you never get to duplicate the i2c-pxa driver code
anywhere. If you have several I2C bus segments controlled by PXA-like
hardware, then you instantiate that many i2c_adapters, all driven by
the same driver (i2c-pxa.)

Calling i2c_new_device() from an i2c client's probe() routine is rather
unusual. I can't really think of a good reason to do this. If the first
client was declared as part of platform init data, I'd expect the
second to be declared there as well.

There's one which is somewhat similar though: chips which reply to more
than one I2C address. The extra addresses are reserved using
i2c_new_dummy(). This gives you an i2c_client handle for easy register
access. This might be suitable for the problem at hand, but I can't
tell for sure without knowing the details.

> > And you want to us i2c_new_device(). Does it mean that there's only
> > 8607 device and address in platform driver and mfd driver
> > distinguishing 8606 from platform data?
> That's correct, yes. As I said, the only place where you can correctly
> describe the relationship between your 8806 and 8807 is your platform data.

-- 
Jean Delvare

[PATCH 02/10] support 88pm8606 in 860x driver

[Haojian Zhuang]

>
> Some more background would be welcome, yes. In particular on how the
> 8806 and 8807 relate to each other, and what kind of chips they are.
> And what problem you are trying to solve in the first place ;)

These two chips can be used together for power management. 8807 is
focus on regulator, RTC, touch, vibrator. 8806 is focus on backlight,
led, charger, and so on. When charger is in use, it also need to
access registers in 8807. In original implementation, one static mixed
structure is defined to link these two chips together. One i2c driver
was serviced for both 8806 and 8807 by i2c id table. Now Samuel is
help me to remove the static mixed structure.

User can use them together or use only one.

>
> In any case, you never get to duplicate the i2c-pxa driver code
> anywhere. If you have several I2C bus segments controlled by PXA-like
> hardware, then you instantiate that many i2c_adapters, all driven by
> the same driver (i2c-pxa.)
>
> Calling i2c_new_device() from an i2c client's probe() routine is rather
> unusual. I can't really think of a good reason to do this. If the first
> client was declared as part of platform init data, I'd expect the
> second to be declared there as well.
>
> There's one which is somewhat similar though: chips which reply to more
> than one I2C address. The extra addresses are reserved using
> i2c_new_dummy(). This gives you an i2c_client handle for easy register
> access. This might be suitable for the problem at hand, but I can't
> tell for sure without knowing the details.
>

Does it mean that i2c_new_dummy()/i2c_new_device() is doing the same
thing like declaring of i2c chip in platform init data?

[PATCH 02/10] support 88pm8606 in 860x driver

[Jean Delvare]

On Mon, 7 Dec 2009 07:47:44 -0500, Haojian Zhuang wrote:
> >
> > Some more background would be welcome, yes. In particular on how the
> > 8806 and 8807 relate to each other, and what kind of chips they are.
> > And what problem you are trying to solve in the first place ;)
> 
> These two chips can be used together for power management. 8807 is
> focus on regulator, RTC, touch, vibrator. 8806 is focus on backlight,
> led, charger, and so on. When charger is in use, it also need to
> access registers in 8807. In original implementation, one static mixed
> structure is defined to link these two chips together. One i2c driver
> was serviced for both 8806 and 8807 by i2c id table. Now Samuel is
> help me to remove the static mixed structure.

I agree that a static structure is not a good idea and it's better to
avoid it if possible.

> User can use them together or use only one.

By "user" you mean hardware designer? Or end-user?

> > In any case, you never get to duplicate the i2c-pxa driver code
> > anywhere. If you have several I2C bus segments controlled by PXA-like
> > hardware, then you instantiate that many i2c_adapters, all driven by
> > the same driver (i2c-pxa.)
> >
> > Calling i2c_new_device() from an i2c client's probe() routine is rather
> > unusual. I can't really think of a good reason to do this. If the first
> > client was declared as part of platform init data, I'd expect the
> > second to be declared there as well.
> >
> > There's one which is somewhat similar though: chips which reply to more
> > than one I2C address. The extra addresses are reserved using
> > i2c_new_dummy(). This gives you an i2c_client handle for easy register
> > access. This might be suitable for the problem at hand, but I can't
> > tell for sure without knowing the details.
> >
> 
> Does it mean that i2c_new_dummy()/i2c_new_device() is doing the same
> thing like declaring of i2c chip in platform init data?

i2c_new_device() is indeed very similar to declaring the i2c chip in
platform init data. The key difference is that you need an i2c_adapter
handle for i2c_new_device(), while the adapter is referenced by its
number in the platform init data.

i2c_new_dummy() is a little different, it doesn't create a real device,
no probe/remove method will be called. It only reserves a given I2C
address (on the specified adapter) for your own use.

Now that I understand better what problem you are trying to solve, here
are a few hints.

As suggested by Samuel, you can have a single device declared in the
platform init data. Attach custom data to this device, where you
declare if the other chip is present and at which address it lives.
Then in the probe() function of the driver, check the data and call
i2c_new_dummy() on the address in question if present.

There are constraints and requirements to this approach: both chips
must be connected to the same I2C adapter, the same driver must handle
both devices, and your driver must be ready to deal with the main device
being either a 8807 or a 8806, and the secondary chip being present or
not. Shouldn't be overly difficult though.

Calling i2c_new_device() instead of i2c_new_dummy() may lead to cleaner
driver code, however I am far from certain than i2c-core will let you
do it. It might deadlock. I just don't know.

An alternative approach is to declare both devices as part of the
platform init data, and let them be registered separately. Then have a
dedicated registration/removal calls at the end of the probe() method /
beginning of the remove() method. The registration function would track
which devices are up and operational, and when it spots two devices
which would work together, it adds the respective information to each
device so that it can reach its sibling. The removal function would
clean up the links when either sibling is removed.

This also has limitations, as you need a way to decide which devices go
by pair. But assuming that both devices are always on the same I2C
adapter, it shouldn't be too difficult. It is probably cleaner than the
other options, as it is symmetrical, but it might be overkill depending
on the cases you really must handle in practice.

-- 
Jean Delvare

[PATCH 02/10] support 88pm8606 in 860x driver

[Samuel Ortiz]

Hi Jean,

On Mon, Dec 07, 2009 at 02:38:32PM +0100, Jean Delvare wrote:
> On Mon, 7 Dec 2009 07:47:44 -0500, Haojian Zhuang wrote:
> > >
> > > Some more background would be welcome, yes. In particular on how the
> > > 8806 and 8807 relate to each other, and what kind of chips they are.
> > > And what problem you are trying to solve in the first place ;)
> > 
> > These two chips can be used together for power management. 8807 is
> > focus on regulator, RTC, touch, vibrator. 8806 is focus on backlight,
> > led, charger, and so on. When charger is in use, it also need to
> > access registers in 8807. In original implementation, one static mixed
> > structure is defined to link these two chips together. One i2c driver
> > was serviced for both 8806 and 8807 by i2c id table. Now Samuel is
> > help me to remove the static mixed structure.
> 
> I agree that a static structure is not a good idea and it's better to
> avoid it if possible.
> 
> > User can use them together or use only one.
> 
> By "user" you mean hardware designer? Or end-user?
I think he means hardware designer.


> > > In any case, you never get to duplicate the i2c-pxa driver code
> > > anywhere. If you have several I2C bus segments controlled by PXA-like
> > > hardware, then you instantiate that many i2c_adapters, all driven by
> > > the same driver (i2c-pxa.)
> > >
> > > Calling i2c_new_device() from an i2c client's probe() routine is rather
> > > unusual. I can't really think of a good reason to do this. If the first
> > > client was declared as part of platform init data, I'd expect the
> > > second to be declared there as well.
> > >
> > > There's one which is somewhat similar though: chips which reply to more
> > > than one I2C address. The extra addresses are reserved using
> > > i2c_new_dummy(). This gives you an i2c_client handle for easy register
> > > access. This might be suitable for the problem at hand, but I can't
> > > tell for sure without knowing the details.
> > >
> > 
> > Does it mean that i2c_new_dummy()/i2c_new_device() is doing the same
> > thing like declaring of i2c chip in platform init data?
> 
> i2c_new_device() is indeed very similar to declaring the i2c chip in
> platform init data. The key difference is that you need an i2c_adapter
> handle for i2c_new_device(), while the adapter is referenced by its
> number in the platform init data.
> 
> i2c_new_dummy() is a little different, it doesn't create a real device,
> no probe/remove method will be called. It only reserves a given I2C
> address (on the specified adapter) for your own use.
> 
> Now that I understand better what problem you are trying to solve, here
> are a few hints.
> 
> As suggested by Samuel, you can have a single device declared in the
> platform init data. Attach custom data to this device, where you
> declare if the other chip is present and at which address it lives.
> Then in the probe() function of the driver, check the data and call
> i2c_new_dummy() on the address in question if present.
> 
> There are constraints and requirements to this approach: both chips
> must be connected to the same I2C adapter, the same driver must handle
> both devices, and your driver must be ready to deal with the main device
> being either a 8807 or a 8806, and the secondary chip being present or
> not. Shouldn't be overly difficult though.
Yes, the driver fits all those requirements, so it shouldnt be too difficult.

> An alternative approach is to declare both devices as part of the
> platform init data, and let them be registered separately. Then have a
> dedicated registration/removal calls at the end of the probe() method /
> beginning of the remove() method. The registration function would track
> which devices are up and operational, and when it spots two devices
> which would work together, it adds the respective information to each
> device so that it can reach its sibling. The removal function would
> clean up the links when either sibling is removed.
I thought about that solution as well, but couldnt see how we could cleanly
tell how 2 devices relate to each other. Now, as you're saying below, we could
assume that both devices are on the same i2c adapter, that sounds like a
fair requirement.
Personally, I'd prefer to use the i2c_new_dummy() method, but it's Haojian's
call to decide which one he prefers.

 
> This also has limitations, as you need a way to decide which devices go
> by pair. But assuming that both devices are always on the same I2C
> adapter, it shouldn't be too difficult. It is probably cleaner than the
> other options, as it is symmetrical, but it might be overkill depending
> on the cases you really must handle in practice.
Jean, thanks a lot spending your time on this long and detailed explanation, I
appreciate.

Cheers,
Samuel.


> -- 
> Jean Delvare

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 02/10] support 88pm8606 in 860x driver

[Haojian Zhuang]

On Mon, Dec 7, 2009 at 8:59 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> Hi Jean,
>
> On Mon, Dec 07, 2009 at 02:38:32PM +0100, Jean Delvare wrote:
>> On Mon, 7 Dec 2009 07:47:44 -0500, Haojian Zhuang wrote:
>> > >
>> > > Some more background would be welcome, yes. In particular on how the
>> > > 8806 and 8807 relate to each other, and what kind of chips they are.
>> > > And what problem you are trying to solve in the first place ;)
>> >
>> > These two chips can be used together for power management. 8807 is
>> > focus on regulator, RTC, touch, vibrator. 8806 is focus on backlight,
>> > led, charger, and so on. When charger is in use, it also need to
>> > access registers in 8807. In original implementation, one static mixed
>> > structure is defined to link these two chips together. One i2c driver
>> > was serviced for both 8806 and 8807 by i2c id table. Now Samuel is
>> > help me to remove the static mixed structure.
>>
>> I agree that a static structure is not a good idea and it's better to
>> avoid it if possible.
>>
>> > User can use them together or use only one.
>>
>> By "user" you mean hardware designer? Or end-user?
> I think he means hardware designer.
>
>
>> > > In any case, you never get to duplicate the i2c-pxa driver code
>> > > anywhere. If you have several I2C bus segments controlled by PXA-like
>> > > hardware, then you instantiate that many i2c_adapters, all driven by
>> > > the same driver (i2c-pxa.)
>> > >
>> > > Calling i2c_new_device() from an i2c client's probe() routine is rather
>> > > unusual. I can't really think of a good reason to do this. If the first
>> > > client was declared as part of platform init data, I'd expect the
>> > > second to be declared there as well.
>> > >
>> > > There's one which is somewhat similar though: chips which reply to more
>> > > than one I2C address. The extra addresses are reserved using
>> > > i2c_new_dummy(). This gives you an i2c_client handle for easy register
>> > > access. This might be suitable for the problem at hand, but I can't
>> > > tell for sure without knowing the details.
>> > >
>> >
>> > Does it mean that i2c_new_dummy()/i2c_new_device() is doing the same
>> > thing like declaring of i2c chip in platform init data?
>>
>> i2c_new_device() is indeed very similar to declaring the i2c chip in
>> platform init data. The key difference is that you need an i2c_adapter
>> handle for i2c_new_device(), while the adapter is referenced by its
>> number in the platform init data.
>>
>> i2c_new_dummy() is a little different, it doesn't create a real device,
>> no probe/remove method will be called. It only reserves a given I2C
>> address (on the specified adapter) for your own use.
>>
>> Now that I understand better what problem you are trying to solve, here
>> are a few hints.
>>
>> As suggested by Samuel, you can have a single device declared in the
>> platform init data. Attach custom data to this device, where you
>> declare if the other chip is present and at which address it lives.
>> Then in the probe() function of the driver, check the data and call
>> i2c_new_dummy() on the address in question if present.
>>
>> There are constraints and requirements to this approach: both chips
>> must be connected to the same I2C adapter, the same driver must handle
>> both devices, and your driver must be ready to deal with the main device
>> being either a 8807 or a 8806, and the secondary chip being present or
>> not. Shouldn't be overly difficult though.
> Yes, the driver fits all those requirements, so it shouldnt be too difficult.
>
>> An alternative approach is to declare both devices as part of the
>> platform init data, and let them be registered separately. Then have a
>> dedicated registration/removal calls at the end of the probe() method /
>> beginning of the remove() method. The registration function would track
>> which devices are up and operational, and when it spots two devices
>> which would work together, it adds the respective information to each
>> device so that it can reach its sibling. The removal function would
>> clean up the links when either sibling is removed.
> I thought about that solution as well, but couldnt see how we could cleanly
> tell how 2 devices relate to each other. Now, as you're saying below, we could
> assume that both devices are on the same i2c adapter, that sounds like a
> fair requirement.
> Personally, I'd prefer to use the i2c_new_dummy() method, but it's Haojian's
> call to decide which one he prefers.
>
>
>> This also has limitations, as you need a way to decide which devices go
>> by pair. But assuming that both devices are always on the same I2C
>> adapter, it shouldn't be too difficult. It is probably cleaner than the
>> other options, as it is symmetrical, but it might be overkill depending
>> on the cases you really must handle in practice.
> Jean, thanks a lot spending your time on this long and detailed explanation, I
> appreciate.
>

Hi Jean & Samuel,

Thanks for your great help. Now I update these patches with
i2c_new_device(). I'll paste it in a new thread.

Best Regards
Haojian