Message-ID: <20090609155235.GK18591@gundam.enneenne.com>
Date:	Tue, 9 Jun 2009 17:52:35 +0200
From:	Rodolfo Giometti <giometti@...eenne.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	Liam Girdwood <lrg@...mlogic.co.uk>, linux-kernel@...r.kernel.org
Subject: Re: regulator_register() API

On Tue, Jun 09, 2009 at 04:18:34PM +0100, Mark Brown wrote:
> On Tue, Jun 09, 2009 at 03:59:47PM +0200, Rodolfo Giometti wrote:
> 
> > Current regulator_register() implementation forces the caller to
> > allocate a proper device for each regulators and also the line:
> 
> >    struct regulator_init_data *init_data = dev->platform_data;
> 
> > forces the user to define the pointer platform_data as a "struct
> > regulator_init_data" only.
> 
> That's not the case in current mainline - the init data is passed in as
> an argument to regulator_register().  There should be no requirement
> that the struct device be unique, you should be able to use the same
> struct device for all the regulators on the device.  It's mostly just
> used for printk.
> 
> > However if the regulator_register() worked in a similar way to
> > led_classdev_register(), I simply can do something like this:
> 
> >         for (i = 0; i < regulators_num; i++) {
> > 	       /* init regulators structs */
> > 	       ...
> > 
> >                ret = regulator_register(&client->dev, &reg[i].dev);
> >                if (ret < 0)
> > 		       dev_warn(&client->dev, "unable to register\n");
> >         }
> 
> You can do exactly this in current mainline.  You do need to supply init
> data for each regulator to make them useful.  The change came in commit
> 0527100fd11d9710c7e153d791da78824b7b46fa which was merged during the
> 2.6.30 merge window.

Great! However this resolve one issue, the caller still needs to
allocate a device struct by itsown. On the other hand, doing like
led_classdev_register() does will resolve it also!

See first lines of the function:

   int led_classdev_register(struct device *parent, struct led_classdev
   *led_cdev)
   {
           int rc;

           led_cdev->dev = device_create(leds_class, parent, 0, led_cdev,
                                         "%s", led_cdev->name);
           if (IS_ERR(led_cdev->dev))
                   return PTR_ERR(led_cdev->dev);

           /* register the attributes */
           rc = device_create_file(led_cdev->dev, &dev_attr_brightness);
           if (rc)
                   goto err_out;

	   ...

As you can see in this case I simply can do:

        /* Register the led devices */
        for (i = 0; i < 6; i++)
                if (pdata->led[i].name) {
                        data->led[i].dev.name = pdata->led[i].name;
                        data->led[i].dev.brightness = pdata->led[i].brightness;
                        data->led[i].dev.brightness_set = max8821_led_set;
#ifdef CONFIG_LEDS_TRIGGERS
                        data->led[i].dev.default_trigger =
                                                pdata->led[i].trigger;
#endif
                        INIT_WORK(&data->led[i].work, max8821_led_set_work);
                        ret = led_classdev_register(&client->dev,
                                                        &data->led[i].dev);
                        if (ret < 0)
                                dev_warn(&client->dev, "unable to register "
                                                "led%d into the system\n",
                                                i + 1);
                }

No device allocation at all, I simply supply the parent device and the
register function does the rest, also all info regarding the led
device are into "struct led_classdev".

> > This will keep backward compatibility with old drivers and may offer a
> > more versatile way to define a regulator expecially for
> > multifunctional devices.
> 
> Most of the regulators currently supported are part of multi-function
> devices so they're not an unusual case here.  

But I never said that it is an unusual case, I said it's more complex
to manage. :)

By doing a "struct regulator_classdev" and defining a
regulator_classdev_register() the device driver writer will be easier!
Maybe we can just adding the regulator_classdev_register() as a
wrapper function...

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@...eenne.com
Linux Device Driver                          giometti@...ux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists