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

Samuel Ortiz
2009-10-02T13:14:04Z

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