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 at mvista.com>
> > + *
> > + * 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
