Re: [PATCH 0/2] twl4030-usb patches

[Felipe Balbi <me@felipebalbi.com>]

On Tue, Sep 02, 2008 at 12:37:56PM -0700, David Brownell wrote:
> Same way it does now ...
> 
> What's BROKEN with the current scheme is that the driver
> model tree looks something like
> 
> 	/sys/devices/platform/omap_i2c.X/X-00{N0,N1,N2,N3}
> 	/sys/devices/platform/{twl-gpio,twl-rtc,twl-foo,...}
> 
> That is, these devices are in the wrong place.  Now, I don't
> really care much if they're all parented by the N0 node;
> that's basically just a strange artifact of that chip.  All
> that I suggested was making sure those platform devices
> are properly parented:  by N0/N1/whatever.

Agreed... it sure looks odd.

> Pass it through twl4030 platform data.  It's all board-specific,
> and if you don't pass platform data for e.g. vibrator or LEDs
> then you'd know this board doesn't have that hardware ... so
> don't create those platform devices (even if the driver does
> happen to be configured into this multi-board kernel).

got it.

> static int twl4030_probe(struct i2c_client *c, const struct i2c_device_id *id)
> {
> 	struct twl4030_core_platform_data *pdata = c->platform_data;
> 	struct platform_device *pdev;
> 	int status;
> 
> 	...
> 	if (have_twl_rtc_driver() && pdata->rtc_data) {
> 		pdev = platform_device_alloc("twl4030_rtc", -1);
> 		... nullcheck ...
> 		status = platform_add_data(pdev, pdata->rtc_data,
> 				sizeof(*pdata->rtc_data));
> 		... status check ...
> 		status = platform_device_add_resources(pdev, ... the irq ... );
> 		... status_check ...
> 		pdev->dev.parent = &c->dev;
> 		status = platform_device_add(pdev);
> 		... status check ...
> 	}
> 	... etc
> }
> 
> Or I suppose you could statically allocate the device data; not
> particularly encouraged where pdev->dev.parent is non-null though.

hmm... that looks good. Gotta dig more into the platform_driver stuff.
Thanks for the point.

> > One thing we might say, Jean Delvare won't accept twl4030 the way it is
> > now. Old style i2c drivers will be dropped soon. We have to get twl4030
> > properly done otherwise it'll be a pain later.
> 
> He's also not keen on growing drivers/i2c/chips ...
> So this driver should probably move to drivers/mfd too.

Sure... it should really be there since it really is a multifunction
device. In that case it would be merged through Samuel Ortiz, right ?
Most likely Cc:ing i2c@lm-sensors.org so i2c people could comment on the
code as well.

> (When it starts moving upstream, I suspect some folk will
> ask why it doesn't support the drivers/regulator calls;
> that's still new, I'd not worry much.)

Well, but if we have proper APIs for doing stuff, we should really start
using them, don't you think ?

I'll postpone my twl4030-usb patch and try to work on twl4030-core.c.
It'll be a huge task moving all of that to new style i2c driver.

-- 
balbi