RE: [PATCH V2] drivers/i2c/busses/i2c-at91.c: fix brokeness

RE: [PATCH V2] drivers/i2c/busses/i2c-at91.c: fix brokeness

[Voss, Nikolaus]

Hi,

> IMHO, you should split this patch into three or more smaller patches.
> You're doing lots of different things in one commit and it'll be a pain to
> bisect should this cause any issues to anyone.

I didn't split the patch because it is virtually a complete rewrite.
Due to the severe limitations of the old driver, I think it should
replace the old driver.

> > +/* -*- linux-c -*-
> 
> you shouldn't add this editor hooks to linux source files.

Ok.


> > -static int at91_i2c_resume(struct platform_device *pdev)
> > +static int at91_i2c_resume(struct device *dev)
> >  {
> > -	return clk_enable(twi_clk);
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct at91_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> 
> dev_get_drvdata(dev) will do the same of two above lines. You don't need to
> fetch the platform_device...

Ok.

> > -MODULE_AUTHOR("Rick Bronson");
> > +MODULE_AUTHOR("Nikolaus Voss");
> 
> wow... are you sure ? Rick will always be the original author, no ?

Yes, of course, but I don't want to declare him responsible for my
own mistakes. The new driver has almost nothing to do with the old.

Thanks for your *very* fast reply and review.

Niko

RE: [PATCH V2] drivers/i2c/busses/i2c-at91.c: fix brokeness

[Voss, Nikolaus]

> > > IMHO, you should split this patch into three or more smaller patches.
> > > You're doing lots of different things in one commit and it'll be a
> > > pain to bisect should this cause any issues to anyone.
> >
> > I didn't split the patch because it is virtually a complete rewrite.
> > Due to the severe limitations of the old driver, I think it should
> > replace the old driver.
> 
> The final decision is up to Ben and/or Jean but I think we should always have
> incremental patches, not sure if we should allow big patches for the reasons
> above.

Splitting the patch implies the possibility to test each incremental
change independently, a possibility I don't have with my current setup as
the old driver didn't work at all for me (for example, my client needs
repeated start). I developed and tested the driver in an all or nothing-at-all
approach. Splitting the patch would be a purely academic exercise for me,
without any extra value beyond readability (which is admittedly bad now).
>From that point of view, I should maybe submit the patch as a new independent
driver (although it is a logical replacement for the old one)?

Niko

Re: [PATCH V2] drivers/i2c/busses/i2c-at91.c: fix brokeness

[Jean Delvare]

On Mon, 7 Nov 2011 12:06:52 +0100, Voss, Nikolaus wrote:
> > > > IMHO, you should split this patch into three or more smaller patches.
> > > > You're doing lots of different things in one commit and it'll be a
> > > > pain to bisect should this cause any issues to anyone.
> > >
> > > I didn't split the patch because it is virtually a complete rewrite.
> > > Due to the severe limitations of the old driver, I think it should
> > > replace the old driver.
> > 
> > The final decision is up to Ben and/or Jean but I think we should always have
> > incremental patches, not sure if we should allow big patches for the reasons
> > above.

The final call is obviously to Ben, not me, as this driver falls under
his jurisdiction. But for what it's worth, I consider the small-steps
rule void when it comes to fixing a plain broken driver by almost fully
rewriting it. The reviewer should really review the resulting code
rather than the patch. If it makes everybody happier, then killing the
old code completely first is certainly an option.

-- 
Jean Delvare

Re: [PATCH V2] drivers/i2c/busses/i2c-at91.c: fix brokeness

[Nicolas Ferre]

On 11/07/2011 01:04 PM, Jean Delvare :
> On Mon, 7 Nov 2011 12:06:52 +0100, Voss, Nikolaus wrote:
>>>>> IMHO, you should split this patch into three or more smaller patches.
>>>>> You're doing lots of different things in one commit and it'll be a
>>>>> pain to bisect should this cause any issues to anyone.
>>>>
>>>> I didn't split the patch because it is virtually a complete rewrite.
>>>> Due to the severe limitations of the old driver, I think it should
>>>> replace the old driver.
>>>
>>> The final decision is up to Ben and/or Jean but I think we should always have
>>> incremental patches, not sure if we should allow big patches for the reasons
>>> above.
> 
> The final call is obviously to Ben, not me, as this driver falls under
> his jurisdiction. But for what it's worth, I consider the small-steps
> rule void when it comes to fixing a plain broken driver by almost fully
> rewriting it. The reviewer should really review the resulting code
> rather than the patch. If it makes everybody happier, then killing the
> old code completely first is certainly an option.

I agree with this.

-- 
Nicolas Ferre

Re: [PATCH V2] drivers/i2c/busses/i2c-at91.c: fix brokeness

[Hubert Feurstein]

Hi,

sorry, but your patch still does not apply. You fixed the tab issue,
but it seems that there are still some bad whitespaces left which
corrupt your patch.

Regards
Hubert