Re: [PATCH v2] mfd: ADP5520 Multifunction LCD Backlight and Keypad Input Device Driver

[Samuel Ortiz <sameo@linux.intel.com>]

Hi Michael,

On Fri, Oct 02, 2009 at 10:38:16AM +0100, Hennerich, Michael wrote:
> >> +static int __devinit adp5520_add_subdevs(struct adp5520_chip *chip,
> >> +					struct adp5520_platform_data
> *pdata)
> >> +{
> >> +	struct adp5520_subdev_info *subdev;
> >> +	struct platform_device *pdev;
> >> +	int i, ret = 0;
> >> +
> >> +	for (i = 0; i < pdata->num_subdevs; i++) {
> >> +		subdev = &pdata->subdevs[i];
> >> +
> >> +		pdev = platform_device_alloc(subdev->name, subdev->id);
> >> +
> >> +		pdev->dev.parent = chip->dev;
> >> +		pdev->dev.platform_data = subdev->platform_data;
> >> +
> >> +		ret = platform_device_add(pdev);
> >> +		if (ret)
> >> +			goto failed;
> >> +	}
> >> +	return 0;
> >Here I would expect the MFD core driver to know about all the potential
> >subdevices and add them in that routine, and not take the subdevices
> list from
> >the platform definition.
> >I realize da903x has the same issue btw.
> 
> The ADP5520 is an I2C device and gets registered via struct
> i2c_board_info.
> How about having multiple ADP5520 in a system with different I2C salve
> addresses?
> Each ADP5520 having different Keypad, Backlight and GPIO configurations
> passed in platform_data?
> How will they map? The MFD core is struct resource centric, which is not
> going to help here.
With 2 of those devices, your board file would look like that:

static struct i2c_board_info __initdata board_i2c_board_info[] = {
	{	/* APD5520 #1 */
		I2C_BOARD_INFO("pmic-adp5520", SLAVE_ADDR_1),
		.platform_data	= &apd5520_platform_1,
	},
	{	/* APD5520 #2 */
		I2C_BOARD_INFO("pmic-adp5520", SLAVE_ADDR_2),
		.platform_data	= &apd5520_platform_2,
	},
};

and your platform data would be an aggregation of all subdevices platform data
structures:

struct adp5520_platform_data {
	struct adp5520_leds_platfrom_data *leds;
	struct adp5520_keys_platfrom_data *keyp;
	struct adp5520_gpio_platfrom_data *gpio;
}

Then, your mfd/adp5520.c will have that piece of code:

static struct platform_device adp5520_gpio_device = {
	.name	= "adp5520-gpio",
	.id	= -1,
}

static struct platform_device *adp5520_subdevs[] = {
	&adp5520_gpio_device,
	&adp5520_leds_device,
	&adp5520_keyp_device,
}

Finally, your i2c probe routine would assign platform_data pointers to te
right devices:

static int __devinit adp5520_probe(struct i2c_client *client,
					const struct i2c_device_id *id)
{
	struct adp5520_platform_data *pdata = client->dev.platform_data; 
[...]
	for (i = 0; i < ARRAY_SIZE(adp5520_subdevs); i++) {
		adp5520_subdevs[i]->dev.parent = &client->dev;
		adp5520_assign_pdata(&adp5520_subdevs[i], pdata);
	}

	platform_add_devices(adp5520_subdevs,
			ARRAY_SIZE(adp5520_subdevs)); 
}

Where adp5520_assign_pdata() is a routine setting the platform_device's
platform_data pointer according e.g. to the platform_device name field.

Would that make sense to you ?

Cheers,
Samuel.

> I could be doing something like this:
>                            
> Index: drivers/mfd/adp5520.c
> 
> ===================================================================
> 
> --- drivers/mfd/adp5520.c       (revision 7535)
> 
> +++ drivers/mfd/adp5520.c       (working copy)
> 
> @@ -25,6 +25,7 @@
> 
>  #include <linux/irq.h>
> 
>  #include <linux/workqueue.h>
> 
>  #include <linux/i2c.h>
> 
> +#include <linux/mfd/core.h>
> 
>  
> 
>  #include <linux/mfd/adp5520.h>
> 
>  
> 
> @@ -235,6 +236,21 @@
> 
>         return ret;
> 
>  }
> 
>  
> 
> +static struct mfd_cell __devinitdata adp5520_cells[] = {
> 
> +       {
> 
> +               .name = "adp5520-backlight",
> 
> +       },
> 
> +       {
> 
> +               .name = "adp5520-led",
> 
> +       },
> 
> +       {
> +               .name = "adp5520-gpio",
> +       },
> +       {
> +               .name = "adp5520-keys",
> +       },
> +};
> +
>  static int __devinit adp5520_probe(struct i2c_client *client,
>                                         const struct i2c_device_id *id)
>  {
> @@ -284,11 +300,20 @@
>                 goto out_free_irq;
>         }
> 
> +#if 0
>         ret = adp5520_add_subdevs(chip, pdata);
> 
>         if (!ret)
>                 return ret;
> +#endif
> 
> +       ret = mfd_add_devices(&chip->dev, id->driver_data,
> +                       adp5520_cells, ARRAY_SIZE(adp5520_cells),
> +                       NULL, client->irq);
> +
> +       if (!ret)
> +               return ret;
> +
>  out_free_irq:
>         if (chip->irq)
>                 free_irq(chip->irq, chip);
> @@ -337,7 +362,8 @@
>  #endif
> 
>  static const struct i2c_device_id adp5520_id[] = {
> -       { "pmic-adp5520", 0 },
> +       { "adp5520", ID_ADP5520 },
> +       { "adp5501", ID_ADP5501 },
>         { }
>  };
>  MODULE_DEVICE_TABLE(i2c, adp5520_id); 
> 
> 
> However this would just work for exactly one ADP5520 in a system.
> A way out could be to append the I2C salve address to the cell .name?
> 
> Comments appreciated.
> 
> >
> >Also, please note that you could use the mfd-core API for adding
> devices, but
> >that's just optional.
> >
> >Cheers,
> >Samuel.
> >
> >--
> >Intel Open Source Technology Centre
> >http://oss.intel.com/
> 
> Best regards,
> Michael

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