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

From: Robin Getz
Date: Wed Oct 17 2007 - 15:13:56 EST


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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/