lkml.org 
[lkml]   [2016]   [Aug]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] usb: core: add support for HCD providers
On Tue, Jul 12, 2016 at 02:35:19PM +0200, Rafał Miłecki wrote:
> When working with Device Tree we may need to reference controllers
> (their nodes) and query for HCDs. This is useful for getting some
> runtime info about host controllers like e.g. assigned bus number.
>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> drivers/usb/core/Makefile | 1 +
> drivers/usb/core/provider.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/usb/provider.h | 39 ++++++++++++++++++++++
> 3 files changed, 119 insertions(+)
> create mode 100644 drivers/usb/core/provider.c
> create mode 100644 include/linux/usb/provider.h
>
> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> index 9780877..20b91d1 100644
> --- a/drivers/usb/core/Makefile
> +++ b/drivers/usb/core/Makefile
> @@ -9,5 +9,6 @@ usbcore-y += port.o of.o
>
> usbcore-$(CONFIG_PCI) += hcd-pci.o
> usbcore-$(CONFIG_ACPI) += usb-acpi.o
> +usbcore-$(CONFIG_OF) += provider.o
>
> obj-$(CONFIG_USB) += usbcore.o
> diff --git a/drivers/usb/core/provider.c b/drivers/usb/core/provider.c
> new file mode 100644
> index 0000000..4b9165a
> --- /dev/null
> +++ b/drivers/usb/core/provider.c
> @@ -0,0 +1,79 @@
> +#include <linux/slab.h>
> +#include <linux/usb/provider.h>
> +
> +static DEFINE_MUTEX(hcd_provider_mutex);
> +static LIST_HEAD(hcd_provider_list);
> +
> +struct hcd_provider {
> + struct device_node *np;
> + struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data);
> + void *data;
> + struct list_head list;
> +};
> +
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> + struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),

Typedef for the function pointer?


> + void *data)
> +{
> + struct hcd_provider *hcd_provider;
> +
> + if (!np)
> + return ERR_PTR(-EINVAL);

How can that be true?

> +
> + hcd_provider = kzalloc(sizeof(*hcd_provider), GFP_KERNEL);
> + if (!hcd_provider)
> + return ERR_PTR(-ENOMEM);
> +
> + hcd_provider->np = np;
> + hcd_provider->of_xlate = of_xlate;
> + hcd_provider->data = data;
> +
> + mutex_lock(&hcd_provider_mutex);
> + list_add_tail(&hcd_provider->list, &hcd_provider_list);
> + mutex_unlock(&hcd_provider_mutex);
> +
> + return hcd_provider;
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_provider_register);
> +
> +void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
> +{
> + if (IS_ERR(hcd_provider))
> + return;
> +
> + mutex_lock(&hcd_provider_mutex);
> + list_del(&hcd_provider->list);
> + mutex_unlock(&hcd_provider_mutex);
> +
> + kfree(hcd_provider);
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_provider_unregister);
> +
> +struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data)
> +{
> + if (args->args_count != 0)
> + return ERR_PTR(-EINVAL);

Huh?

> + return data;

What is this function for? Why even have it?

> +}
> +EXPORT_SYMBOL_GPL(of_hcd_xlate_simple);
> +
> +struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
> +{
> + struct usb_hcd *hcd = ERR_PTR(-ENOENT);
> + struct hcd_provider *provider;
> +
> + if (!args)
> + return ERR_PTR(-EINVAL);

How is args not set?

> +
> + mutex_lock(&hcd_provider_mutex);
> + list_for_each_entry(provider, &hcd_provider_list, list) {
> + if (provider->np == args->np) {
> + hcd = provider->of_xlate(args, provider->data);
> + break;
> + }
> + }
> + mutex_unlock(&hcd_provider_mutex);
> +
> + return hcd;
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_get_from_provider);
> diff --git a/include/linux/usb/provider.h b/include/linux/usb/provider.h
> new file mode 100644
> index 0000000..c66e006
> --- /dev/null
> +++ b/include/linux/usb/provider.h
> @@ -0,0 +1,39 @@
> +#ifndef __USB_CORE_PROVIDER_H
> +#define __USB_CORE_PROVIDER_H
> +
> +#include <linux/of.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +
> +struct hcd_provider;
> +
> +#ifdef CONFIG_OF
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> + struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
> + void *data);
> +void of_hcd_provider_unregister(struct hcd_provider *hcd_provider);
> +struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data);
> +struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args);
> +#else
> +static inline
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> + struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
> + void *data)
> +{
> + return ERR_PTR(-ENOSYS);
> +}
> +static inline void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
> +{
> +}
> +static inline struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args,
> + void *data)
> +{
> + return ERR_PTR(-ENOSYS);
> +}
> +static inline struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
> +{
> + return NULL;
> +}
> +#endif

Why all of the "of" stuff? Why not make it generic for any firmware
type (acpi, OF, etc.)?

And I really don't like this, isn't there other ways to get this
information if you really need it?

thanks,

greg k-h

\
 
 \ /
  Last update: 2016-08-09 16:21    [W:0.040 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site