On Tuesday, August 19, 2008 11:39 am Alex Chiang wrote:
> * Rolf Eike Beer <eike-kernel@xxxxxxxxx>:
> > Alex Chiang wrote:
> > > +static inline const char *slot_name(struct slot *slot)
> > > +{
> > > + return hotplug_slot_name(slot->hotplug_slot);
> > > +}
> > > +
> > > /*
> > > * struct acpiphp_bridge - PCI bridge information
> > > *
> >
> > I don't see a point in this function. Why not call hotplug_slot_name()
> > directly?
>
> You're correct that we don't exactly need it in acpiphp. However,
> it is a useful helper function for some of the other drivers, and
> I thought it would be better to keep consistency if possible.
>
> Also, it helps later on, when trying to stay below the 80 column
> limit. :)
If they're all identical, maybe we should pull this up into a common function
(with a short name :). That can be a subsequent cleanup though.
> > > @@ -84,7 +87,6 @@ static struct hotplug_slot_ops acpi_hotplug_slot_ops
> > > = { .get_adapter_status = get_adapter_status,
> > > };
> > >
> > > -
> > > /**
> > > * acpiphp_register_attention - set attention LED callback
> > > * @info: must be completely filled with LED callbacks
> >
> > Fuzz.
>
> Yes, it's fuzz, but my practice has been to clean up* source files
> during the course of making actual, functional changes. Better
> than sending a mostly-useless whitespace patchbomb, IMO.
>
> * Note that "clean up" here means "reasonable cleanup" that
> doesn't detract from reading the rest of the patch.
Yeah the main reason for separating whitespace changes from real ones is to
avoid making the patch hard to read in case a problem crops up. Killing an
extra newline doesn't make things harder to read, imo, so it's fine with me
if you keep it (though if you *do* end up doing a cleanup patch later you
could save it for that).
> > > @@ -92,7 +94,7 @@ static struct hotplug_slot_ops acpi_hotplug_slot_ops
> > > = { * Description: This is used to register a hardware specific ACPI *
> > > driver that manipulates the attention LED. All the fields in * info
> > > must be set.
> > > - */
> > > + **/
> > > int acpiphp_register_attention(struct acpiphp_attention_info *info)
> > > {
> > > int retval = -EINVAL;
> >
> > Fuzz. Make the patch look bigger than it actually is. But that's just a
> > note, Jesse will have to judge if this is acceptable.
>
> See above. Fixing incorrect kerneldoc doesn't seem so bad, IMO.
kerneldoc is ok with either */ or **/ as a closing comment, so you can drop
these bits.
> > > acpiphp_slot->slot = slot;
> > > - snprintf(slot->name, sizeof(slot->name), "%u", slot->acpi_slot->sun);
> > > + memset(name, 0, SLOT_NAME_SIZE);
> > > + snprintf(name, SLOT_NAME_SIZE, "%u", slot->acpi_slot->sun);
> >
> > The memset() is not needed at all. And the sizeof is IMHO a good idea
> > anyway as it allows to get rid of the define.
>
> Hm, don't need a memset? I won't have garbage on the stack?
> </n00b>
Yeah, but you'll overwrite it with snprintf anyway, so the memset is
redundant.
> On the other hand, keeping the #define is important, because
> again, that's the established convention of the PCI hotplug
> drivers.
>
> Thanks for the review.
Going from the define to sizeof() everywhere could be another cleanup, but I
don't have strong feelings about that.
Jesse
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
