Re: [PATCH] drivers: firmware: edd: fixed coding style errors

[Derek Perrin <d.roc16@gmail.com>]

On Sunday, December 22, 2013 02:13:39 PM Joe Perches wrote:
> On Sun, 2013-12-22 at 15:17 -0600, Derek Perrin wrote:
> > Fixed coding style errors. Spaces, tab and parenthesis errors.
> 
> There's a real error in this patch.
> 
> When you do whitespace only changes, please
> verify that the old and new object files produced
> are unchanged.
> 
> One trivial bit too,
> 
> > diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
> 
> []
> 
> > @@ -62,8 +62,8 @@ struct edd_device {
> > 
> >  struct edd_attribute {
> >  
> >  	struct attribute attr;
> > 
> > -	ssize_t(*show) (struct edd_device * edev, char *buf);
> > -	int (*test) (struct edd_device * edev);
> > +	ssize_t(*show) (struct edd_device *edev, char *buf);
> > +	int (*test) (struct edd_device *edev);
> 
> I think most prefer function pointer prototypes like:
> 
> 	size_t (*show)(struct edd_device *edev, char *buf);
> 	int (*test)(struct edd_device *edev);
> 
> > @@ -791,7 +791,7 @@ edd_exit(void)
> > 
> >  	struct edd_device *edev;
> >  	
> >  	for (i = 0; i < edd_num_devices(); i++) {
> > 
> > -		if ((edev = edd_devices[i]))
> > +		if ((edev == edd_devices[i]))
> > 
> >  			edd_device_unregister(edev);
> >  	
> >  	}
> >  	kset_unregister(edd_kset);
> 
> This is wrong.
> It's an assignment in the block and it's important.
> 
> This could be rewritten as:
> 
> 	for (i = 0; i < edd_num_devices(); i++) {
> 		edev = edd_devices[i];
> 		if (edev)
> 			edd_device_unregister(edev);
> 	}
> 
> or just remove the temporary.


Sorry, I'm really new to contributing to the kernel and just learning the 
process. I can make a new patch that fixes the assignment. I'm not seeing any 
difference between what I did and what you said about the function pointer 
prototypes.