Re: [RFC V2 1/3] [smi2021] Add gm7113c chip to the saa7115 driver

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em Thu, 25 Apr 2013 22:33:20 +0200
Jon Arne Jørgensen <jonarne@jonarne.no> escreveu:

> On Thu, Apr 25, 2013 at 05:13:28PM -0300, Mauro Carvalho Chehab wrote:
> > Em Thu, 25 Apr 2013 21:10:18 +0200
> > Jon Arne Jørgensen <jonarne@jonarne.no> escreveu:
> > 
> > >  	/* Check whether this chip is part of the saa711x series */
> > > -	if (memcmp(name + 1, "f711", 4)) {
> > > +	if (memcmp(id->name + 1, "gm7113c", 7)) {
> > > +		chip_id = 'c';
> > 
> > There are several issues on the above:
> > 1) "id" may be NULL on autodetect mode;
> > 
> > 2) Why are you adding 1 here?
> >    It should be, instead id->name
> > 
> > 3) memcmp returns 0 if matches. So, the test is wrong.
> >    So, It should be instead:
> > 	if (!memcmp(id->name, "gm7113c", 7)) {
> > 
> > 4) Also, while that works, it seems a little hackish...
> > 
> 
> Oh, this is embarrassing.
> I just tried to change as little as possible in this module to make the
> device work.

No problems. On my experience, quick hacks like that are the
ones that generally have more troubles ;)

> > Something like:
> > 
> > static int saa711x_detect_chip(struct i2c_client *client,
> > 			       struct saa711x_state *state,
> > 			       const struct i2c_device_id *id)
> > {
> > 	int i;
> > 	char chip_id, name[16];
> > 
> > 	/*
> > 	 * Check for gm7113c (a saa7113 clone). Currently, there's no
> > 	 * known way to autodetect it, so boards that use will need to
> > 	 * explicitly fill the id->name field.
> > 	 */
> > 	if (id && !memcmp(id->name, "gm7113c", 7)) {
> > 		state->ident = V4L2_IDENT_GM7113C;
> > 		snprintf(client->name, sizeof(client->name), "%s", id->name);
> > 		return 0;
> > 	}
> > 
> > 	/* Check for Philips/NXP original chips */
> > 	for (i = 0; i < sizeof(name); i++) {
> > 		i2c_smbus_write_byte_data(client, 0, i);
> > 		name[i] = (i2c_smbus_read_byte_data(client, 0) & 0x0f) + '0';
> > 		if (name[i] > '9')
> > 			name[i] += 'a' - '9' - 1;
> > 	}
> > 	name[i] = '\0';
> > 
> > 	if (memcmp(name + 1, "f711", 4))
> > 		return -ENODEV;
> > 
> > 	chip_id = name[5];
> > 
> > 	snprintf(client->name, sizeof(client->name), "saa711%c", chip_id);
> > 
> > 	/*
> > 	 * Put here the code that fills state->ident for Philips/NXP chips
> > 	 */
> > ...
> > 
> > 	return 0;
> 
> Yes this seems to be a much better way to do it.
> I will fix my code.

Thanks!
Mauro