Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 2862 bytes --]

Hi Jean,

> OK, I understand how it works now, thanks for pointing me to the
> relevant piece of code. It's easier to modify the code when you
> understand the current logic ;)

:)

> The idea is to assign fixed i2c bus numbers to the relevant i2c buses,
> and declare the i2c devices connected to each bus by bus number and
> device address. The i2c-powermac buses are created in
> arch/powerpc/platforms/powermac/low_i2c.c, so you would have to
> instantiate the i2c devices there too. That would basically mean
> merging part of snd-aoa-fabric-layout into
> arch/powerpc/platforms/powermac/low_i2c.c as I understand it, I don't
> know if this sounds reasonable to you.

That doesn't sound too hot -- the fabric module is quite a lot of code
and data.

> So I think we have two options: switch the powermac systems to fixed
> i2c bus numbers and instantiate the i2c sound devices from
> arch/powerpc/platforms/powermac/*, or find a way to obtain a reference
> to the relevant i2c_adapter.
> 
> I think I have found a way to achieve the latter. This isn't exactly
> how the new model was supposed to work, but it has the advantage to be
> way less intrusive than my original proposal. It may require larger
> changes in the future as the i2c-core cleanups go on, but this should
> do for now.

Heh :)

> My new approach doesn't auto-load anything. Here we go, what you think?
> This time there is no logic change, I'm really only turning legacy code
> into new-style i2c code (basically calling i2c_new_device() instead of
> i2c_attach_client()) and that's about it.
> 
> (Once again this is only build-tested and I would like people with the
> hardware to give it a try.)

Looks reasonable.

>  static int onyx_create(struct i2c_adapter *adapter,
>  		       struct device_node *node,
>  		       int addr)
>  {
> +	struct i2c_board_info info;
> +	struct i2c_client *client;
> +
> +	memset(&info, 0, sizeof(struct i2c_board_info));
> +	strlcpy(info.type, "aoa_codec_onyx", I2C_NAME_SIZE);
> +	if (node) {
> +		info.addr = addr;
> +		info.platform_data = node;
> +		client = i2c_new_device(adapter, &info);
> +	} else {
> +		/* probe both possible addresses for the onyx chip */
> +		unsigned short probe_onyx[] = {
> +			0x46, 0x47, I2C_CLIENT_END
> +		};
> +
> +		client = i2c_new_probed_device(adapter, &info, probe_onyx);
> +	}
> +	if (!client)
> +		return -ENODEV;
> +
> +	list_add_tail(&client->detected, &client->driver->clients);

That list_add looks a little hackish, and wouldn't it be racy?

> +static const struct i2c_device_id onyx_i2c_id[] = {
> +	{ "aoa_codec_onyx", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, onyx_i2c_id);

Should it really export the device table?

(same comments for tas)

It'll be until mid next week that I can test anything.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]