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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
