lkml.org 
[lkml]   [2009]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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@enneenne.com
Linux Device Driver giometti@linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti


\
 
 \ /
  Last update: 2009-06-09 17:55    [W:0.058 / U:6.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site