| Message ID | 1386042188-12246-1-git-send-email-balbi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
The code looks mostly fine, but the implementation of the commit logs seems lazy. Please submit a v3 using coherent sentences with full explanations and correct punctuation.
> The code looks mostly fine, but the implementation of the commit logs > seems lazy. Please submit a v3 using coherent sentences with full > explanations and correct punctuation. Also use the full length of the provided buffer, which I believe is 72 chars, but happy to be correct on that one. It's certainly not 40 chars however.
Hi,
For the whole series:
Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>
A.
On Tue, Dec 03, 2013 at 09:51:36AM +0000, Lee Jones wrote: > The code looks mostly fine, but the implementation of the commit logs > seems lazy. Please submit a v3 using coherent sentences with full > explanations and correct punctuation. example ?
> > The code looks mostly fine, but the implementation of the commit logs > > seems lazy. Please submit a v3 using coherent sentences with full > > explanations and correct punctuation. > > example ? All of your commit messages. > that macro just helps removing some extra ^- Sentences start with an uppercase character. > line of code and hides ffs() calls. > > while at that, also fix a variable shadowing ^- Sentences start with an uppercase character. > bug where 'int irq' was being redeclared inside > inner loop while it was also argument to interrupt > handler. < --------------- 50 chars ----------------- > Please use the full 72 char (or there abouts) width of the buffer.
Hi, On Mon, Dec 09, 2013 at 09:37:48AM +0000, Lee Jones wrote: > > > The code looks mostly fine, but the implementation of the commit logs > > > seems lazy. Please submit a v3 using coherent sentences with full > > > explanations and correct punctuation. > > > > example ? > > All of your commit messages. > > > that macro just helps removing some extra > > ^- Sentences start with an uppercase character. > > > line of code and hides ffs() calls. > > > > while at that, also fix a variable shadowing > > ^- Sentences start with an uppercase character. > > > bug where 'int irq' was being redeclared inside > > inner loop while it was also argument to interrupt > > handler. > > < --------------- 50 chars ----------------- > > > Please use the full 72 char (or there abouts) width of the buffer. I don't see any mention of punctuation problems, however. Also, you're not complaining about the content at all, which tells me those sentences aren't as incoherent as you claimed before. But fair enough, I'll fix those up and add Aaro's Tested-by
> > > > The code looks mostly fine, but the implementation of the commit logs > > > > seems lazy. Please submit a v3 using coherent sentences with full > > > > explanations and correct punctuation. > > > > > > example ? > > > > All of your commit messages. > > > > > that macro just helps removing some extra > > > > ^- Sentences start with an uppercase character. > > > > > line of code and hides ffs() calls. > > > > > > while at that, also fix a variable shadowing > > > > ^- Sentences start with an uppercase character. > > > > > bug where 'int irq' was being redeclared inside > > > inner loop while it was also argument to interrupt > > > handler. > > > > < --------------- 50 chars ----------------- > > > > > Please use the full 72 char (or there abouts) width of the buffer. > > I don't see any mention of punctuation problems, however. Also, you're > not complaining about the content at all, which tells me those sentences > aren't as incoherent as you claimed before. I didn't read them in any detail. I traversed through the patches and saw that the formatting looked obscure on all of them. As I have come to expect more of your submissions, I provided a generic reply detailing how I expected the commit logs to be. I wasn't insinuated that you failed to meet all of the criteria, but they definitely fell short of the mark. > But fair enough, I'll fix those up and add Aaro's Tested-by Thank you.
Hi, On Tue, Dec 10, 2013 at 08:50:07AM +0000, Lee Jones wrote: > > > > > The code looks mostly fine, but the implementation of the commit logs > > > > > seems lazy. Please submit a v3 using coherent sentences with full > > > > > explanations and correct punctuation. > > > > > > > > example ? > > > > > > All of your commit messages. > > > > > > > that macro just helps removing some extra > > > > > > ^- Sentences start with an uppercase character. > > > > > > > line of code and hides ffs() calls. > > > > > > > > while at that, also fix a variable shadowing > > > > > > ^- Sentences start with an uppercase character. > > > > > > > bug where 'int irq' was being redeclared inside > > > > inner loop while it was also argument to interrupt > > > > handler. > > > > > > < --------------- 50 chars ----------------- > > > > > > > Please use the full 72 char (or there abouts) width of the buffer. > > > > I don't see any mention of punctuation problems, however. Also, you're > > not complaining about the content at all, which tells me those sentences > > aren't as incoherent as you claimed before. > > I didn't read them in any detail. I traversed through the patches and so you gave review comments without actually reviewing ? how rude... > saw that the formatting looked obscure on all of them. As I have come > to expect more of your submissions, I provided a generic reply > detailing how I expected the commit logs to be. I wasn't insinuated > that you failed to meet all of the criteria, but they definitely fell > short of the mark. in what way ?
On Mon, Dec 02, 2013 at 09:42:54PM -0600, Felipe Balbi wrote: > we could build that as a driver. What is "that". How can it not be a driver? Do you mean modular? In that case there is no problem, the only thing that doesn't work is unloading the module. Best regards Uwe > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > drivers/mfd/menelaus.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/menelaus.c b/drivers/mfd/menelaus.c > index ad25bfa..975ff9e 100644 > --- a/drivers/mfd/menelaus.c > +++ b/drivers/mfd/menelaus.c > @@ -1262,7 +1262,7 @@ fail: > return err; > } > > -static int __exit menelaus_remove(struct i2c_client *client) > +static int menelaus_remove(struct i2c_client *client) > { > struct menelaus_chip *menelaus = i2c_get_clientdata(client); > > @@ -1283,7 +1283,7 @@ static struct i2c_driver menelaus_i2c_driver = { > .name = DRIVER_NAME, > }, > .probe = menelaus_probe, > - .remove = __exit_p(menelaus_remove), > + .remove = menelaus_remove, > .id_table = menelaus_id, > }; >
Are you planning on re-submitting this patch-set anytime soon? I thought Tony said it was important?
diff --git a/drivers/mfd/menelaus.c b/drivers/mfd/menelaus.c index ad25bfa..975ff9e 100644 --- a/drivers/mfd/menelaus.c +++ b/drivers/mfd/menelaus.c @@ -1262,7 +1262,7 @@ fail: return err; } -static int __exit menelaus_remove(struct i2c_client *client) +static int menelaus_remove(struct i2c_client *client) { struct menelaus_chip *menelaus = i2c_get_clientdata(client); @@ -1283,7 +1283,7 @@ static struct i2c_driver menelaus_i2c_driver = { .name = DRIVER_NAME, }, .probe = menelaus_probe, - .remove = __exit_p(menelaus_remove), + .remove = menelaus_remove, .id_table = menelaus_id, };
we could build that as a driver. Signed-off-by: Felipe Balbi <balbi@ti.com> --- drivers/mfd/menelaus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)