Re: [PATCH 2/2] [I2C] add PCA9543 chip driver

Hennerich, Michael
2008-05-13T11:54:21Z

>-----Original Message-----
>From: Riku Voipio [mailto:riku.voipio-WgUW+8SLYMv1KXRcyAk9cg@public.gmane.org]
>Sent: Montag, 12. Mai 2008 14:31
>To: Bryan Wu
>Cc: i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org; khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org; Michael Hennerich
>Subject: Re: [i2c] [PATCH 2/2] [I2C] add PCA9543 chip driver
>
>Bryan Wu wrote:
>> From: Michael Hennerich
>> --- /dev/null
>> +++ b/drivers/i2c/chips/pca9543.c
>> @@ -0,0 +1,212 @@
>> +/*
>> + pca9543.c - Part of lm_sensors, Linux kernel modules for
hardware
>> + monitoring
>>
>Not a sensor chip..
>> +/* A few notes about the PCA9543:
>> +
>> +* The PCA9543 is a bi-directional translating switch, controlled
by
>the
>> + I2C bus. The SCL/SDA upstream pair fans out to two downstream
>> + pairs, or channels. Any individual SCx/SDx channels or
combination
>> + of channels can be selected, determined by the contents of the
>> + programmable control register. Two interrupt inputs, INT0 to
INT3,
>> + one for each of the downstream pairs, are provided. One
interrupt
>> + output INT, which acts as an AND of the two interrupt inputs, is
>> + provided.
>> +
>> +*/
>>
>You might want to add a url to the datasheet, rather than
>a copypaste from it. Any description in the driver sould
>> +
>> +/* This is the driver that will be inserted */
>> +static struct i2c_driver pca9543_driver = {
>> + .driver = {
>> + .name = "pca9543",
>> + },
>> + .id = I2C_DRIVERID_PCF8574,
>> + .attach_adapter = pca9543_attach_adapter,
>> + .detach_client = pca9543_detach_client,
>> +};
>>
>Since the chip is undetectable, and used on embedded platform, I think
you
>should consider a "new-style" driver with .probe/remove instead.
>See i2c/writing-clients
>> + struct i2c_client *new_client;
>>
>> + new_client = &data->client;
>> + i2c_set_clientdata(new_client, data);
>> + new_client->addr = address;
>> + new_client->adapter = adapter;
>> + new_client->driver = &pca9543_driver;
>> + new_client->flags = 0;
>>
>>
>s/new_client/client/ please.
>> + /* Now, we would do the remaining detection. But the PCF8574 is
>plainly
>> + impossible to detect! Stupid chip. */
>> +
>>
>copypaste from PCF8574 driver? :) Again, consider if the new style
drivers
>would me more useful for your platform.

Yes copypaste from PCF8574 driver. There is not much value in here, we
either send a new patch including your feedback or drop this.

Best regards,
Michael