Re: [PATCH try #4] Input/Joystick Driver: add support AD7142 joystick driver

[Robin Getz <rgetz@blackfin.uclinux.org>]

On Wed 17 Oct 2007 10:07, Jean Delvare pondered:
> Hi Bryan,
> 
> On Wed, 17 Oct 2007 15:12:00 +0800, Bryan Wu wrote:
> > Subject: [PATCH try #4] Input/Joystick Driver: add support AD7142
> joystick driver
> > +#define AD7142_I2C_ADDR		0x2C
> 
> Not worth a define IMHO, as you use it only once and the context is
> completely clear.

> > +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.
> 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/