On Wed, Sep 03, 2014 at 11:40:38AM +0300, Dan Carpenter wrote:
> On Tue, Sep 02, 2014 at 11:46:35PM +0530, Sudip Mukherjee wrote:
> > From: Sudip Mukherjee <sudip@xxxxxxxxxxxxxxx>
>
> I really would prefer if you just figured out your email settings so
> this isn't needed. The From: header is mostly used for people
> forwarding patches from other people. We have allowed people to use
> the From header like this if they can't get their corporate email
> configured properly but I try to discorage it. If everyone starts using
> From headers like this then it becomes a pain to deal with.
>
I will configure the corporate mail. I am the server admin , so there should
not be any problem in settings. :)
> >
> > removed unused variables
> > fixed sparse warning of context imbalance in 'do_locked_client_insert'
> > different lock contexts for basic block
> >
> > Signed-off-by: Sudip Mukherjee <sudip@xxxxxxxxxxxxxxx>
> > ---
> >
>
> This patch is much better and more interesting, but I still want some
> more changes.
>
I have already sent v3 of the patch just before your mail , based on
what greg k-h has suggested about the commnent. Please discard that.
> > v1 of the patch of the patch just fixed the sparse warning.
> > On suggestion of Dan Carpenter v2 is the total rewrite of the function.
> > Two of the function arguments (interruptHandle,channelId) are also not used. Wanted to remove them as well ,
> > but then thought maybe the original author have planned for some use of those variables.
>
> In the kernel we don't put code in until we are ready to use it. Don't
> worry about future changes. But on the other hand, don't remove the
> parameters in this patch because that is doing too many changes in one
> patch. It would have to be done in a follow on patch if you decide to
> do it.
>
> > - if (locked) {
> > - spin_unlock_irqrestore((spinlock_t *) lock, flags);
> > - locked = 0;
> > + goto unlock;
> > + visor_signalqueue_empty(queueinfo->chan, whichqueue);
>
> Just remove this function. But mention it in the changelog in case
> there are side effects.
>
> > + /*visor_signal_insert() only return 0 or 1 */
>
> Don't put obvious comments like this. A normal reader will assume that
> this function is boolean based on how it is used.
>
> > + if (visor_signal_insert(queueinfo->chan, whichqueue, pSignal) == 1) {
>
> Don't put the == 1. In terms of English, 1 really is intended as
> "success" and not the number one. Also don't test for == true or
> == false.
>
> if (foo) {
> if (foo == true) {
>
> These two statement *mean* the same thing in terms of English, but the
> first one is simpler and less wordy.
>
> regards,
> dan carpenter
thanks
sudip
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
