regulator_register() API

regulator_register() API

[Rodolfo Giometti]

Hello,

I'm trying to add the regulator support to my driver for max8821
(white led charge pump with mono class D audio amp and dual LDO).

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.

Since max8821 has both leds and LDOs I compared the functions
regulator_register() and led_classdev_register() and IMHO the latter
is more versatile/easy-to-use than the former. :)

In a multifunctional device connected with an I2C bus (as the max8821
is) it's quite complex defining a regulator device since I must
provide a new device for each regulators and redefing platform_data
for each registration.

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");
        }

and writing the support for my multifunctional device will be
easier. :)

What about if I add a new function regulator_classdev_register() which
in turn calls regulator_register() doing in a similar way than
led_classdev_register() does?

This will keep backward compatibility with old drivers and may offer a
more versatile way to define a regulator expecially for
multifunctional devices.

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: regulator_register() API

[Mark Brown]

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.

> 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.  

Re: regulator_register() API

[Rodolfo Giometti]

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@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: regulator_register() API

[Mark Brown]

On Tue, Jun 09, 2009 at 05:52:35PM +0200, Rodolfo Giometti wrote:

> 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!

The regulator driver does not need to allocate a struct device.  The
struct device that is passed in is for the chip as a whole and would
normally be something like the struct device for the I2C client.  As
I say it's there mostly for the benefit of printk().

The regulator API returns a pointer to the class device that is
allocated and ensures that that class device is parented by the device
that was passed in.

> 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;

The code in, for example, the lp3971 driver looks pretty much the same
as this except instead of copying data into the struct it passes data
into the registration API.