Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Mark A. Greer]

Hi Jean,

On Sat, Apr 01, 2006 at 07:20:40PM +0200, Jean Delvare wrote:

<snip>

> > I either need to have a bunch of returns in that routine (frowned
> > upon) or I have to get rid of the dev_err msgs which I think are
> > useful.  If you have a better way, let me know.
> 
> No immediate idea and I don't quite have the time to work on it. If
> anyone is unhappy with it, that person gets to suggest a better way ;)

Works for me!

> > Other than that, I think I have addressed all of your concerns.
> > Sorry for all of the iterations.
> 
> No worry about the iterations, good patches go that path, methinks. And
> I take my part of responsability for the slow processing.
> 
> I'm fine with almost everything now, except for a couple things below,
> which I'll change myself if you're OK with my suggestions, before
> pushing the patch upwards:
> 
> >  m41t00_probe(struct i2c_adapter *adap, int addr, int kind)
> >  {
> >  	struct i2c_client *client;
> > -	int rc;
> > +	int rc = -EINVAL;
> >  
> > -	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
> > -	if (!client)
> > -		return -ENOMEM;
> > +	if (!i2c_check_functionality(adap, I2C_FUNC_I2C
> > +			| I2C_FUNC_SMBUS_BYTE_DATA))
> > +		goto early_err;
> 
> Good check, but bad return value, given the way the i2c subsystem works
> for now. There is no error at this point, your i2c driver is simply not
> compatible with one of the i2c busses which exist on the system. You
> must return 0 (no error.)
> 
> > +
> > +	if (!(client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
> > +		rc = -ENOMEM;
> > +		goto early_err;
> > +	}
> 
> And this change doesn't really win anything compared to the original
> code. So I'd suggest:
> 
> @@ -170,23 +286,72 @@
>  	struct i2c_client *client;
>  	int rc;
>  
> +	if (!i2c_check_functionality(adap, I2C_FUNC_I2C
> +			| I2C_FUNC_SMBUS_BYTE_DATA))
> +		return 0;
> +
>  	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
>  	if (!client)
>  		return -ENOMEM;
> 
> This minimizes the changes while doing the right thing.

I'm happy with that.

> > --- linux-2.6.16-mm1-cleanup/include/linux/m41t00.h	1969-12-31 17:00:00.000000000 -0700
> > +++ linux-2.6.16-mm1-newsupp/include/linux/m41t00.h	2006-03-28 14:36:56.000000000 -0700
> > @@ -0,0 +1,50 @@
> > +/*
> > + * Definitions for the ST M41T00 family of i2c rtc chips.
> > + *
> > + * Author: Mark A. Greer <mgreer@xxxxxxxxxx>
> > + *
> > + * 2005 (c) MontaVista Software, Inc. This file is licensed under
> 
> Still no 2006? You added it to the driver file so I guess you want to
> do the same here.

Oops, missed that one.  My bad.

> > +/* SQW output disabled, this is default value by power on*/
> > +#define SQW_FREQ_DISABLE	(0)
> > +
> > +#define SQW_FREQ_32KHZ	(1<<4)		/* 32.768 KHz */
> > +#define SQW_FREQ_8KHZ	(2<<4)		/* 8.192 KHz */
> > +#define SQW_FREQ_4KHZ	(3<<4)		/* 4.096 KHz */
> > +#define SQW_FREQ_2KHZ	(4<<4)		/* 2.048 KHz */
> > +#define SQW_FREQ_1KHZ	(5<<4)		/* 1.024 KHz */
> > +#define SQW_FREQ_512HZ	(6<<4)		/* 512 Hz */
> > +#define SQW_FREQ_256HZ	(7<<4)		/* 256 Hz */
> > +#define SQW_FREQ_128HZ	(8<<4)		/* 128 Hz */
> > +#define SQW_FREQ_64HZ	(9<<4)		/* 64 Hz */
> > +#define SQW_FREQ_32HZ	(10<<4)		/* 32 Hz */
> > +#define SQW_FREQ_16HZ	(11<<4)		/* 16 Hz */
> > +#define SQW_FREQ_8HZ	(12<<4)		/* 8 Hz */
> > +#define SQW_FREQ_4HZ	(13<<4)		/* 4 Hz */
> > +#define SQW_FREQ_2HZ	(14<<4)		/* 2 Hz */
> > +#define SQW_FREQ_1HZ	(15<<4)		/* 1 Hz */
> 
> It just hit me that, given that these defines are in a header file,
> they should have a M41T00_ prefix. Don't you think?

I agree. 

> If so, in order not
> to make the symbol names too long, we can probably remove the FREQ
> part, which doesn't really add anything. So I'd go with:
> 
> +/* SQW output disabled, this is default value by power on */
> +#define M41T00_SQW_DISABLE     (0)
> +
> +#define M41T00_SQW_32KHZ       (1<<4)          /* 32.768 KHz */
> +#define M41T00_SQW_8KHZ                (2<<4)          /* 8.192 KHz */
> +#define M41T00_SQW_4KHZ                (3<<4)          /* 4.096 KHz */
> +#define M41T00_SQW_2KHZ                (4<<4)          /* 2.048 KHz */
> +#define M41T00_SQW_1KHZ                (5<<4)          /* 1.024 KHz */
> +#define M41T00_SQW_512HZ       (6<<4)          /* 512 Hz */
> +#define M41T00_SQW_256HZ       (7<<4)          /* 256 Hz */
> +#define M41T00_SQW_128HZ       (8<<4)          /* 128 Hz */
> +#define M41T00_SQW_64HZ                (9<<4)          /* 64 Hz */
> +#define M41T00_SQW_32HZ                (10<<4)         /* 32 Hz */
> +#define M41T00_SQW_16HZ                (11<<4)         /* 16 Hz */
> +#define M41T00_SQW_8HZ         (12<<4)         /* 8 Hz */
> +#define M41T00_SQW_4HZ         (13<<4)         /* 4 Hz */
> +#define M41T00_SQW_2HZ         (14<<4)         /* 2 Hz */
> +#define M41T00_SQW_1HZ         (15<<4)         /* 1 Hz */
> 
> That's it. If you're OK with my changes, just tell so and we go with my
> version. If anything isn't OK, no problem, but then you get to resend a
> full patch with everything the way you want.

I'm happy with all your changes.  Thanks again for you time, Jean.

Mark
-
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/