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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
