On Wed, Mar 14, 2012 at 01:19:44AM +0000, Love, Robert W wrote:
> On 03/12/2012 10:00 PM, Greg KH wrote:
> > On Mon, Mar 12, 2012 at 04:09:31PM -0700, Robert Love wrote:
> >> +static void fcoe_ctlr_attrs_release(struct device *dev)
> >> +{
> >> + struct fcoe_ctlr_attrs *ctlr = dev_to_ctlr(dev);
> >> +
> >> + put_device(ctlr->dev.parent);
> >> + ctlr->dev.parent = NULL;
> > You should never have to put a reference count on your parent, nor worry
> > about setting this value to NULL. Just assign the parent when you
> > register the device, no need to increment it.
>
> Cool, I'll make these changes.
>
> >> +#define fcoe_ctlr_id(x) \
> >> + ((x)->id)
> >> +#define fcoe_ctlr_work_q_name(x) \
> >> + ((x)->work_q_name)
> > <snip>
> >
> > Ick, what are all of these for, please don't do that.
> >
>
> These are only interesting when you look at the macros used to create
> the show/store handlers for the attributes.
<snip>
Ah, yes, sorry, that's worthwhile.
> My feeling is that when you looked at the code you just saw unnecessary
> accessors routines as their usage is not so obvious. I do not intend to
> be using these accessors anywhere else other than the withing the
> show/store building routines.
Ok, and they are in the .c file, not .h, right?
> Given my explanation, do you still dislike these?
Nope, they are fine.
> I could move them to the fcoe_sysfs.c so they're not in a header and
> therefore would look less like accessors that developers should use...
Yes, that will be good.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
