Re: [PATCH v7 1/6] fieldbus_dev: add Fieldbus Device subsystem.

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jan 22, 2019 at 06:12:31PM +0100, Andreas Färber wrote:
> Am 22.01.19 um 17:56 schrieb Paul Gortmaker:
> > [[PATCH v7 1/6] fieldbus_dev: add Fieldbus Device subsystem.] On 22/01/2019 (Tue 10:21) Sven Van Asbroeck wrote:
> > 
> >> Fieldbus device (client) adapters allow data exchange with a PLC aka.
> >> "Fieldbus Controller" over a fieldbus (Profinet, FLNet, etc.)
> >>
> >> They are typically used when a Linux device wants to expose itself
> >> as an actuator, motor, console light, switch, etc. over the fieldbus.
> >>
> >> This framework is designed to provide a generic interface to Fieldbus
> >> Devices from both the Linux Kernel and the userspace.
> >>
> >> Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com>
> > 
> > [...]
> > 
> >> diff --git a/drivers/fieldbus/Kconfig b/drivers/fieldbus/Kconfig
> >> new file mode 100644
> >> index 000000000000..5c2bef950d04
> >> --- /dev/null
> >> +++ b/drivers/fieldbus/Kconfig
> >> @@ -0,0 +1,19 @@
> >> +menuconfig FIELDBUS_DEV
> >> +	bool "Fieldbus Device Support"
> > 
> > OK, so the core support is NOT tristate, ie not modular,
> 
> Is that intentional though, and if so, why?
> 
> If possible, I would much rather see it changed to tristate and leave
> all the metadata below.
> 
> > so I think...
> > 
> >> diff --git a/drivers/fieldbus/dev_core.c b/drivers/fieldbus/dev_core.c
> >> new file mode 100644
> >> index 000000000000..c816df3201bb
> >> --- /dev/null
> >> +++ b/drivers/fieldbus/dev_core.c
> >> @@ -0,0 +1,348 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Fieldbus Device Driver Core
> >> + *
> >> + */
> >> +
> >> +#include <linux/fieldbus_dev.h>
> >> +#include <linux/mutex.h>
> >> +#include <linux/module.h>
> > 
> > ...you don't need module.h here
> > 
> >> +#include <linux/device.h>
> >> +#include <linux/idr.h>
> >> +#include <linux/fs.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/poll.h>
> >> +
> > 
> > [...]
> > 
> >> +
> >> +static void __exit fieldbus_exit(void)
> >> +{
> >> +	unregister_chrdev_region(fieldbus_devt, MAX_FIELDBUSES);
> >> +	class_unregister(&fieldbus_class);
> >> +}
> >> +
> >> +subsys_initcall(fieldbus_init);
> >> +module_exit(fieldbus_exit);
> > 
> > ...and the module_exit is never called, so fieldbus_exit is dead code
> > and hence both should be removed.
> > 
> >> +
> >> +MODULE_AUTHOR("Sven Van Asbroeck <TheSven73@googlemail.com>");
> >> +MODULE_AUTHOR("Jonathan Stiles <jonathans@arcx.com>");
> >> +MODULE_DESCRIPTION("Fieldbus Device Driver Core");
> >> +MODULE_LICENSE("GPL v2");
> 
> Sven, as this code appears to be all by ArcX people, any chance you can
> make it just GPL (SPDX GPL-2.0-or-later)? Only reason to have it v2-only

SPDX "GPL-2.0+" please.

> would be if this code was derived from v2-only code that can't be
> relicensed anymore.

Are you _sure_ you want v2+?  I ask as I am forced to :)

And I thought when I made the comment about this code being a module,
that would be the change, not this one forcing it not to be :(

thanks,

greg k-h