Re: [PATCH v4 1/3] Staging: IndustryPack bus for the Linux Kernel

[Samuel Iglesias GonsÃlvez]

On 2012-05-10 16:37, Greg Kroah-Hartman wrote:
(snip)
> > > > +static void ipack_device_release(struct device *dev)
> > > > +{
> > > > +}
> > >
> > > Weee.  As per the in-kernel documentation, I get to publically 
> > mock
> > you
> > > for doing something as foolish as thinking you are smarter than 
> > the
> > > kernel by just creating an empty function for the release 
> > callback.
> > >
> > > Did you think this really is the solution for when the kernel is
> > > complaining to you about the fact that you need a callback 
> > function
> > > here?  Surely I didn't just put that logic in the core for no good
> > > reason now, right?
> > >
> > > Please fix this up NOW.
> >
> > OK, I will fix it. However reading my code, I don't see the need to
> > kfree anything here, like in other drivers, for example.
>
> Then your code is designed wrong.  You must free the memory here.  
> The
> problem is that your "core" is not doing the allocation, but are 
> relying
> on the driver to do it instead.  Don't do that, the driver should not
> have to do any of this at all.  Look at other busses for examples.

OK.

> > Is it OK to have a pr_info notifying the release of the device or 
> > should
> > I think again about it?
>
> You should never have a pr_info() call anywhere, what would a user do
> with such a message?  That seems pretty pointless, right?
>
> Also, please always use dev_*() calls instead of pr_*() calls, as you
> should always have access to a struct device in your code.

OK

> > > > +++ b/drivers/staging/ipack/ipack.h
> > > > @@ -0,0 +1,183 @@
> > > > +/*
> > > > + * Industry-pack bus.
> > > > + *
> > > > + * (C) 2011 Samuel Iglesias Gonsalvez <siglesia@xxxxxxx>, CERN
> > > > + * (C) 2012 Samuel Iglesias Gonsalvez <siglesias@xxxxxxxxxx>,
> > Igalia
> > > > + *
> > > > + * This program is free software; you can redistribute it 
> > and/or
> > modify it
> > > > + * under the terms of the GNU General Public License as 
> > published
> > by the Free
> > > > + * Software Foundation; either version 2 of the License, or (at
> > your option)
> > > > + * any later version.
> > >
> > > Again, "any later version", are you sure?  Be very sure about this
> > > please.
> > >
> > > > +struct ipack_device {
> > > > +   char board_name[IPACK_BOARD_NAME_SIZE];
> > >
> > > Why not use dev->name?
> >
> > May I be wrong, do you refer rename it to "name"?
>
> rename what?  Why do you need a board name for a device?  Shouldn't 
> that
> just be the "name" for the device?  And as such, just use the field 
> you
> already have.

In struct device there is the field "init_name". There is a "name" 
field in the corresponding struct kobject inside of dev. This is the 
reason of my misunderstanding.

I will change it.

> > > > +   char bus_name[IPACK_BOARD_NAME_SIZE];
>
> And, why do you need a bus name?  Shouldn't that be implied based on
> what bus the device is attached to?

This is the name of the bus device. The problem here is that the 
ipoctal mezzanine needs to save the IRQ vector in his memory space in a 
different address depending of the carrier board it is plugged to.

It is described in IP-OCTAL's datasheet. So this bus_name variable 
gives the way to do it.

Best regards,

Sam
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/