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

[Jean Delvare <khali@linux-fr.org>]

Hi Robin,

On Wed, 17 Oct 2007 15:14:40 -0400, Robin Getz wrote:
> 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)?

Not necessarily. Given that detecting this chip safely seems to be
difficult (if not impossible), I'd rather only list the addresses which
are actually known to be used.

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

If the device does have an identification register, then yes you should
test here. However, I seem to understand that the AD7142 uses 2-byte
register addressing, while most I2C devices use 1-byte register
addressing (I2C is tricky in that respect). This means that "reading
from a register" for the AD7142 corresponds to "writing to a register"
for other chips. So even just reading from the device ID register in
the probe function isn't safe.

So to be on the safe side, the ad7142 driver should not probe _any_
address by default. Users should use the "probe" command line parameter
to explicitly ask for a given address to be probed.

> 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?

No, there is no standard at all. Each driver does its best to identify
the chip depending of the specific register map. When the devices don't
have ID registers, we test for unused bits or registers cycling over
arbitrary boundaries.

> 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.

Agreed, don't probe on reset values.

> > 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.

It the current state it's not acceptable. Too dangerous.

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

That's the way to go, really.

-- 
Jean Delvare