Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE

Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE

[Timur Tabi]

I currently have this in my code:

static const struct i2c_device_id cs4270_id[] = {
	{"cs4270", 0},
	{}
};
MODULE_DEVICE_TABLE(i2c, cs4270_id);

static struct i2c_driver cs4270_i2c_driver = {
	.driver = {
		.name = "cs4270",
		.owner = THIS_MODULE,
	},
	.id_table = cs4270_id,
	.probe = cs4270_i2c_probe,
	.remove = cs4270_i2c_remove,
};

ret = i2c_add_driver(&cs4270_i2c_driver);

I would like to use the i2c_device_id.driver_data variable to pass private data
to my cs4270_i2c_probe() function.  So it will look like this:

	socdev = kmalloc(...);
	c24270_id.driver_data = socdev;
	i2c_add_driver(&cs4270_i2c_driver);

And then in my cs4270_i2c_probe(), I would do this:

static int cs4270_i2c_probe(struct i2c_client *i2c_client,
	const struct i2c_device_id *id)
{
	struct X *socdev = (struct X *) id->driver_data.

The problem I'm having is the MODULE_DEVICE_TABLE.  What I really should be
doing is this:

	socdev = kmalloc(...);
	struct i2c_driver *cs4270_i2c_driver = kmalloc(...);
	struct i2c_device_id *cs4270_id = kmalloc(...);
	cs4270_i2c_driver->id_table = cs4270_id;
	c24270_id->driver_data = socdev;
	i2c_add_driver(cs4270_i2c_driver);

But if I do this, then I can't use MODULE_DEVICE_TABLE.  So I have two questions:

1) What happens if I don't use MODULE_DEVICE_TABLE to identify my I2C ID table?

2) Is there a way to mimic the behavior of MODULE_DEVICE_TABLE on a
dynamically-created ID table?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE

[Jon Smirl]

On Fri, Jan 23, 2009 at 4:03 PM, Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> I currently have this in my code:

I'm not sure what you are trying to achieve. The id table has one
entry per chip. It is static data, all instances of the chip should
have the same ID entry.

Look at drivers/of_i2c.c and how it passed in dynamic data.

Also, I just added of_find_i2c_device_by_node() so you can follow
references in the device tree.
i2s driver loads and uses the codec-handle to locate the i2c entry.

                i2s@2200 { /* PSC2 in i2s mode */
                        compatible =
"fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
                        cell-index = <1>;
                        reg = <0x2200 0x100>;
                        interrupts = <0x2 0x2 0x0>;
                        interrupt-parent = <&mpc5200_pic>;
                        codec-handle = <&tas0>;
                };

               i2c@3d00 {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        compatible =
"fsl,mpc5200b-i2c","fsl,mpc5200-i2c","fsl-i2c";
                        cell-index = <0>;
                        reg = <0x3d00 0x40>;
                        interrupts = <0x2 0xf 0x0>;
                        interrupt-parent = <&mpc5200_pic>;
                        fsl5200-clocking;

                        tas0:codec@1b {
                                compatible = "ti,tas5504";
                                reg = <0x1b>;
                        };
                        clock0:clock@68 {
                                compatible = "maxim,max9485";
                                reg = <0x68>;
                        };
                };


>
> static const struct i2c_device_id cs4270_id[] = {
>        {"cs4270", 0},
>        {}
> };
> MODULE_DEVICE_TABLE(i2c, cs4270_id);
>
> static struct i2c_driver cs4270_i2c_driver = {
>        .driver = {
>                .name = "cs4270",
>                .owner = THIS_MODULE,
>        },
>        .id_table = cs4270_id,
>        .probe = cs4270_i2c_probe,
>        .remove = cs4270_i2c_remove,
> };
>
> ret = i2c_add_driver(&cs4270_i2c_driver);
>
> I would like to use the i2c_device_id.driver_data variable to pass private data
> to my cs4270_i2c_probe() function.  So it will look like this:
>
>        socdev = kmalloc(...);
>        c24270_id.driver_data = socdev;
>        i2c_add_driver(&cs4270_i2c_driver);
>
> And then in my cs4270_i2c_probe(), I would do this:
>
> static int cs4270_i2c_probe(struct i2c_client *i2c_client,
>        const struct i2c_device_id *id)
> {
>        struct X *socdev = (struct X *) id->driver_data.
>
> The problem I'm having is the MODULE_DEVICE_TABLE.  What I really should be
> doing is this:
>
>        socdev = kmalloc(...);
>        struct i2c_driver *cs4270_i2c_driver = kmalloc(...);
>        struct i2c_device_id *cs4270_id = kmalloc(...);
>        cs4270_i2c_driver->id_table = cs4270_id;
>        c24270_id->driver_data = socdev;
>        i2c_add_driver(cs4270_i2c_driver);
>
> But if I do this, then I can't use MODULE_DEVICE_TABLE.  So I have two questions:
>
> 1) What happens if I don't use MODULE_DEVICE_TABLE to identify my I2C ID table?
>
> 2) Is there a way to mimic the behavior of MODULE_DEVICE_TABLE on a
> dynamically-created ID table?
>
> --
> Timur Tabi
> Linux kernel developer at Freescale
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Re: Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE

[Mark Brown]

On Fri, Jan 23, 2009 at 03:03:19PM -0600, Timur Tabi wrote:

> The problem I'm having is the MODULE_DEVICE_TABLE.  What I really should be
> doing is this:

> 	socdev = kmalloc(...);
> 	struct i2c_driver *cs4270_i2c_driver = kmalloc(...);
> 	struct i2c_device_id *cs4270_id = kmalloc(...);
> 	cs4270_i2c_driver->id_table = cs4270_id;
> 	c24270_id->driver_data = socdev;
> 	i2c_add_driver(cs4270_i2c_driver);

This is because the ASoC core still hasn't quite got the idea of
instantiating the differnet bits of the device separately.  Really what
you're having to do here is fairly nasty and there's no good reason for
it to be supported - if something is being registered dynamically and
passing instantiation specific data around it really should be the
device and not the driver.  The fact that attempting to do this looks
bad is a win on the part of the I2C core.

For now an idiomatic solution for ASoC drivers is to have a single
static variable that you use to get the socdev through.

Re: Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE

[Timur Tabi]

Mark Brown wrote:

> Really what
> you're having to do here is fairly nasty and there's no good reason for
> it to be supported - if something is being registered dynamically and
> passing instantiation specific data around it really should be the
> device and not the driver.

What we need is a way for the codec driver to do its stuff without needing a 
socdev.

I think I see your point, though.  I'm calling i2c_add_driver, which 
technically could result in my i2c_probe function being called multiple times, 
if there are multiple matches in the device tree.  I'll have to check that out 
on Monday.

 > The fact that attempting to do this looks
> bad is a win on the part of the I2C core.
>
> For now an idiomatic solution for ASoC drivers is to have a single
> static variable that you use to get the socdev through.

Well, that's what I do today.  I was hoping to avoid that, but if I'm right 
about i2c_add_driver, then this trick doesn't really work either.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE

[Mark Brown]

On Fri, Jan 23, 2009 at 08:23:32PM -0600, Timur Tabi wrote:

> I think I see your point, though.  I'm calling i2c_add_driver, which  
> technically could result in my i2c_probe function being called multiple 
> times, if there are multiple matches in the device tree.  I'll have to 
> check that out on Monday.

Yes, it will - any device specific data must be registered when the
device is registered.  You may want to look at how the wm8900 handles
this, it makes use of the DAI registration functions to make this a
little nicer.  It does still have the same problem but is much closer to
the end result and means you can start registering the driver normally
now.

>> For now an idiomatic solution for ASoC drivers is to have a single
>> static variable that you use to get the socdev through.

> Well, that's what I do today.  I was hoping to avoid that, but if I'm 
> right about i2c_add_driver, then this trick doesn't really work either.

This really isn't something that can be worked around well in the
individual drivers.

Re: Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE

[Ben Dooks]

On Fri, Jan 23, 2009 at 03:03:19PM -0600, Timur Tabi wrote:
> I currently have this in my code:
> 
> static const struct i2c_device_id cs4270_id[] = {
> 	{"cs4270", 0},
> 	{}
> };
> MODULE_DEVICE_TABLE(i2c, cs4270_id);
> 
> static struct i2c_driver cs4270_i2c_driver = {
> 	.driver = {
> 		.name = "cs4270",
> 		.owner = THIS_MODULE,
> 	},
> 	.id_table = cs4270_id,
> 	.probe = cs4270_i2c_probe,
> 	.remove = cs4270_i2c_remove,
> };
> 
> ret = i2c_add_driver(&cs4270_i2c_driver);
> 
> I would like to use the i2c_device_id.driver_data variable to pass private data
> to my cs4270_i2c_probe() function.  So it will look like this:
> 
> 	socdev = kmalloc(...);
> 	c24270_id.driver_data = socdev;
> 	i2c_add_driver(&cs4270_i2c_driver);

you seem to have managed to short-circuit part of the device
creation process. You do not need to pass the data via the device
driver, you should pass it when creating the device. 

If you look at i2c_board_info which can be passed into i2c_new_device
or similar functions, there is a platform_data field you can fill out
and this is passed in to your probe routine in the i2c device.

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE

[Timur Tabi]

On Sat, Jan 24, 2009 at 10:50 AM, Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> wrote:
> you seem to have managed to short-circuit part of the device
> creation process. You do not need to pass the data via the device
> driver, you should pass it when creating the device.

Yes, I think my code is broken.

> If you look at i2c_board_info which can be passed into i2c_new_device
> or similar functions, there is a platform_data field you can fill out
> and this is passed in to your probe routine in the i2c device.

I don't think this will work for me.  I'm running this on a PowerPC
system, and we use a device tree to represent the I2C devices on the
various I2C buses.  My driver does not call i2c_new_device.  This is
done for me in of_register_i2c_devices().  As soon as my driver calls
i2c_add_driver(), my I2C probe function will be called, once for each
I2C device defined in the device tree.  So I don't think I can update
the i2c_board_info structure.

>
> --
> Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
>
>  'a smiley only costs 4 bytes'
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE

[Jon Smirl]

On Mon, Jan 26, 2009 at 1:06 PM, Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> On Sat, Jan 24, 2009 at 10:50 AM, Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> wrote:
>> you seem to have managed to short-circuit part of the device
>> creation process. You do not need to pass the data via the device
>> driver, you should pass it when creating the device.
>
> Yes, I think my code is broken.
>
>> If you look at i2c_board_info which can be passed into i2c_new_device
>> or similar functions, there is a platform_data field you can fill out
>> and this is passed in to your probe routine in the i2c device.
>
> I don't think this will work for me.  I'm running this on a PowerPC
> system, and we use a device tree to represent the I2C devices on the
> various I2C buses.  My driver does not call i2c_new_device.  This is
> done for me in of_register_i2c_devices().  As soon as my driver calls
> i2c_add_driver(), my I2C probe function will be called, once for each
> I2C device defined in the device tree.  So I don't think I can update
> the i2c_board_info structure.

Is it possible to use of_find_i2c_device_by_node() during
initialization of your I2S device and poke the OF specific info into
the codec at that point?

>
>>
>> --
>> Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
>>
>>  'a smiley only costs 4 bytes'
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>
>
> --
> Timur Tabi
> Linux kernel developer at Freescale
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org