Hello Greg,
On Sat, Jun 16, 2018 at 09:58:07AM +0200, Greg KH wrote:
> On Fri, Jun 15, 2018 at 10:54:45PM +0200, Jacek Anaszewski wrote:
> > On 06/14/2018 11:16 PM, Uwe Kleine-König wrote:
> > > --- a/drivers/usb/core/ledtrig-usbport.c
> > > +++ b/drivers/usb/core/ledtrig-usbport.c
> > > @@ -113,11 +113,17 @@ static ssize_t usbport_trig_port_store(struct device *dev,
> > > static struct attribute *ports_attrs[] = {
> > > NULL,
> > > };
> > > +
>
> Ok, that looks nice...
>
> > > static const struct attribute_group ports_group = {
> > > .name = "ports",
> > > .attrs = ports_attrs,
> > > };
> > > +static const struct attribute_group *ports_groups[] = {
> > > + &ports_group,
> > > + NULL
> > > +};
>
> No extra line there?
In the patch as I sent, I kept the separating empty line, see below.
> And what about the ATTRIBUTE_GROUPS() macro?
I tried that first, but this doesn't provide .name and later in the file
ports_group.name is used in usbport_trig_add_port(), so I thought I
cannot simply drop it.
> > > /***************************************
> > > * Adding & removing ports
> > > ***************************************/
> > > @@ -301,59 +307,44 @@ static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
> > > static int usbport_trig_activate(struct led_classdev *led_cdev)
> > > {
> > > struct usbport_trig_data *usbport_data;
> > > - int err;
>
> Leave a blank line here please.
The original patch has:
@@ -301,59 +307,44 @@ static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
static int usbport_trig_activate(struct led_classdev *led_cdev)
{
struct usbport_trig_data *usbport_data;
- int err;
usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
...
I got it back like this (via kernel@xxxxxxxxxxxxxx), Jacek's reply that
adds you to Cc: was the first one that doesn't show this empty line. So
I think that was messed up by Jacek's MUA.
> > > usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
> > > if (!usbport_data)
> > > - return 0;
> > > + return -ENOMEM;
>
> This is a separate bug fix.
No, not really. The ledtrig core just learned about .activate returning
an int; it was void before. With the patch that changed the prototype
(patch 2 in this series) the return 0 was added. So this is not a bug
fix but making use of the new feature that we can return an error code
here now.
> Anyway, all minor things, feel free to fix them up later on:
>
> Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Thanks.
BTW, checkpatch points out that the lines of the commit log are too
long. Should I resend just for that (given that Greg's ack means the
rest can stay as is)? Or can you fix that up when committing?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
