Messages in this thread | | | Date | Thu, 5 Sep 2013 21:55:03 -0700 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH RESEND v3 1/7] Intel MIC Host Driver for X100 family. |
| |
Very minor nits, you can change this in a future add-on patch:
> +static DEVICE_ATTR(family, S_IRUGO, mic_show_family, NULL);
This should use DEVICE_ATTR_RO(), so that we don't have to audit the permissions of your DEVICE_ATTR() files.
> +static DEVICE_ATTR(stepping, S_IRUGO, mic_show_stepping, NULL);
Same here.
> +static struct attribute *mic_default_attrs[] = { > + &dev_attr_family.attr, > + &dev_attr_stepping.attr, > + > + NULL > +}; > + > +static struct attribute_group mic_attr_group = { > + .attrs = mic_default_attrs, > +}; > + > +static const struct attribute_group *__mic_attr_group[] = { > + &mic_attr_group, > + NULL > +};
These last two structures can be replaced with: ATTRIBUTE_GROUPS(mic_default);
> +void mic_sysfs_init(struct mic_device *mdev) > +{ > + mdev->attr_group = __mic_attr_group; > +}
This is "odd", why not just export the data structure and reference it in the other code? The pci core does this, and so do other busses.
Anyway, it's not a big deal, just a bit strange to me.
thanks,
greg k-h
| |