On Wed 17 Oct 2007 10:07, Jean Delvare pondered:

Hi Bryan,

On Wed, 17 Oct 2007 15:12:00 +0800, Bryan Wu wrote:

quoted

Subject: [PATCH try #4] Input/Joystick Driver: add support AD7142

joystick driver

quoted

+#define AD7142_I2C_ADDR		0x2C

Not worth a define IMHO, as you use it only once and the context is
completely clear.

quoted

+static int ad7142_probe(struct i2c_adapter *adap, int addr, int kind)
+{
+	struct ad7142_data *data;
+	struct i2c_client *client;
+	struct input_dev *input_dev;
+	int rc;
+
+	data = kzalloc(sizeof(struct ad7142_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	client = &data->client;
+
+	strlcpy(client->name, AD7142_DRV_NAME, I2C_NAME_SIZE);
+	client->addr = addr;
+	client->adapter = adap;
+	client->driver = &ad7142_driver;
+

This is unsafe: you're not doing any kind of detection here. 

I assume that in order to do things "right", all possible addresses should be 
probed (for this part, that is 0x2C, 0x2D, 0x2E, 0x2F)?

And it should read a device ID (register 0x17 == 0xE62n (where n==rev number))

How many values/registers do you need to read before you say "this is the 
one"? rather than a temp sensor with a similar value somewhere. Is there a 
table anywhere that maps this out?

It is pretty difficult just to probe on reset values, since that disallows 
warm resets. This part - like most other I2C parts, has no external reset, 
only software reset.

The risk is mitigated by the fact
that your driver is currently built for Blackfin only, but this might
change in the future. 

The hope is that others can use it - it shouldn't be specific to Blackfin.

This is a very good reason to switch to a 
new-style i2c driver.