Hi Paul,
> On Jun 15, 2015, at 17:43 , Paul Bolle <pebolle@xxxxxxxxxx> wrote:
>
> Some remarks (that might not touch the subjects you want to get feedback
> on for an RFC).
>
> On Fri, 2015-06-12 at 23:04 +0300, Pantelis Antoniou wrote:
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>
>> +config DEV_OVERLAYMGR
>> + tristate "Device overlay manager"
>> + depends on OF
>> + select OF_OVERLAY
>> + default n
>
> Why bother with "default n"?
>
>> + help
>> + Say Y here to include support for the automagical dev
>> + overlay manager.
>
>> --- /dev/null
>> +++ b/drivers/misc/devovmgr.c
>
>> +#include <linux/ctype.h>
>> +#include <linux/cpu.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_fdt.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/sizes.h>
>> +#include <linux/slab.h>
>> +#include <linux/proc_fs.h>
>> +#include <linux/configfs.h>
>> +#include <linux/types.h>
>> +#include <linux/stat.h>
>> +#include <linux/limits.h>
>> +#include <linux/file.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/firmware.h>
>> +#include <linux/pci.h>
>> +#include <linux/usb.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/firmware.h>
>
> You're including <linux/firmware.h> twice.
>
Right,
>> +/* copy of drivers/pci/pci.h */
>
> Because?
>
Cause it’s not exported out of PCI core.
>> +static inline const struct pci_device_id *
>> +pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
>> +{
>> + if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
>> + (id->device == PCI_ANY_ID || id->device == dev->device) &&
>> + (id->subvendor == PCI_ANY_ID ||
>> + id->subvendor == dev->subsystem_vendor) &&
>> + (id->subdevice == PCI_ANY_ID ||
>> + id->subdevice == dev->subsystem_device) &&
>> + !((id->class ^ dev->class) & id->class_mask))
>> + return id;
>> + return NULL;
>> +}
>
>> +#if IS_ENABLED(CONFIG_USB)
>> +/* in drivers/usb/core/driver.c */
>> +extern int usb_match_device(struct usb_device *dev,
>> + const struct usb_device_id *id);
>
> And that's an internal function of the usb core, isn't it?
>
Yes.
>> +static int __init dovmgr_init(void)
>> +{
>> + int ret;
>> +
>> + config_group_init(&dovmgr_subsys.su_group);
>> +
>> +#if IS_ENABLED(CONFIG_PCI)
>> + ret = dovmgr_pci_init();
>> + if (ret != 0)
>> + goto err_no_pci_init;
>> +#endif
>> +#if IS_ENABLED(CONFIG_USB)
>> + ret = dovmgr_usb_init();
>> + if (ret != 0)
>> + goto err_no_usb_init;
>> +#endif
>> +
>> + ret = configfs_register_subsystem(&dovmgr_subsys);
>> + if (ret != 0) {
>> + pr_err("%s: failed to register subsys\n", __func__);
>> + goto err_no_configfs;
>> + }
>> + pr_info("%s: OK\n", __func__);
>> + return 0;
>> +
>> +err_no_configfs:
>> +#if IS_ENABLED(CONFIG_USB)
>> + dovmgr_usb_cleanup();
>> +err_no_usb_init:
>> +#endif
>> +#if IS_ENABLED(CONFIG_PCI)
>> + dovmgr_pci_cleanup();
>> +err_no_pci_init:
>> +#endif
>> + return ret;
>> +}
>> +late_initcall(dovmgr_init);
>
> Lot's of "#if IS_ENABLED(CONFIG_USB)" and "#if IS_ENABLED(CONFIG_PCI)"
> in the code. The above function is a rather ugly example.
>
> Is there a point to all this if neither PCI nor USB is enabled?
>
> USB can be 'm'. Does this build and work in that case?
>
> There's no MODULE_LICENSE() macro. If this is a module then loading it
> will taint the kernel.
>
> There's also no function that is, well, called by module_exit() to allow
> (easy) unloading (and do the needed cleaning up on unload). Did you
> intend DEV_OVERLAYMGR to be bool instead?
>
Note this is an RFC after all, it’s not intended for inclusion as is. I just
present the technique and how it works. Yes, all those defines are probably
not good, but I’d like some remarks on the implementation first.
> Thanks,
>
>
> Paul Bolle
>
Regards
— Pantelis
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
