lkml.org 
[lkml]   [2015]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] RFC: Device overlay manager (PCI/USB + DT)
From
Date
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.

> +/* copy of drivers/pci/pci.h */

Because?

> +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?

> +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?

Thanks,


Paul Bolle



\
 
 \ /
  Last update: 2015-06-15 17:01    [W:0.055 / U:0.848 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site