I'd really like someone from USB to have a look through this too.
I'll do a quick first pass and provide some general comments though.
On Sun, 16 Feb 2020, Noralf Trønnes wrote:
> A Multifunction USB Device is a device that supports functions like gpio
> and display or any other function that can be represented as a USB regmap.
> Interrupts over USB is also supported if such an endpoint is present.
Do you have a datasheet?
> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> ---
> drivers/mfd/Kconfig | 8 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/mud.c | 580 ++++++++++++++++++++++++++++++++++++++++
As interesting as a "mud driver" sounds. Something more forthcoming
might be better, "multi_usb" as a very quick example, but perhaps
something more imaginative/distinguishing would be better.
> include/linux/mfd/mud.h | 16 ++
> 4 files changed, 605 insertions(+)
> create mode 100644 drivers/mfd/mud.c
> create mode 100644 include/linux/mfd/mud.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 52818dbcfe1f..9950794d907e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1968,6 +1968,14 @@ config MFD_STMFX
> additional drivers must be enabled in order to use the functionality
> of the device.
>
> +config MFD_MUD
> + tristate "Multifunction USB Device core driver"
> + depends on USB
> + select MFD_CORE
> + select REGMAP_USB
> + help
> + Select this to get support for the Multifunction USB Device.
> +
> menu "Multimedia Capabilities Port drivers"
> depends on ARCH_SA1100
>
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 29e6767dd60c..0adfab9afaed 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -255,4 +255,5 @@ obj-$(CONFIG_MFD_ROHM_BD70528) += rohm-bd70528.o
> obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
> obj-$(CONFIG_MFD_STMFX) += stmfx.o
> obj-$(CONFIG_MFD_RPISENSE_CORE) += rpisense-core.o
> +obj-$(CONFIG_MFD_MUD) += mud.o
>
> diff --git a/drivers/mfd/mud.c b/drivers/mfd/mud.c
> new file mode 100644
> index 000000000000..f5f31478656d
> --- /dev/null
> +++ b/drivers/mfd/mud.c
> @@ -0,0 +1,580 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2020 Noralf Trønnes
> + */
> +
> +#include <linux/bitmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/list.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/mud.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/regmap_usb.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/usb.h>
> +#include <linux/workqueue.h>
> +
> +/* Temporary debugging aid */
> +#undef dev_dbg
> +#define dev_dbg dev_info
> +
> +#define mdebug(fmt, ...) \
> +do { \
> + if (1) \
> + pr_debug(fmt, ##__VA_ARGS__); \
> +} while (0)
No thank you.
We already have quite a few debugging facilities in the kernel.
> +struct mud_irq_event {
> + struct list_head node;
> + DECLARE_BITMAP(status, REGMAP_USB_MAX_MAPS);
> +};
> +
> +struct mud_irq {
> + struct irq_domain *domain;
> + unsigned int num_irqs;
> +
> + struct workqueue_struct *workq;
> + struct work_struct work;
> + struct urb *urb;
> +
> + spinlock_t lock; /* Protect the values below */
> + unsigned long *mask;
> + u16 tag;
> + struct list_head eventlist;
> +
> + unsigned int stats_illegal;
> + unsigned int stats_already_seen;
> + unsigned int stats_lost;
> +};
I think this should have a Kerneldoc header.
> +struct mud_device {
s/device/ddata/
> + struct usb_device *usb;
> + struct mud_irq *mirq;
> + struct mfd_cell *cells;
Why does this need to be in here?
> + unsigned int num_cells;
Why do these need to be stored in device data?
> +};
> +
> +static void mud_irq_work(struct work_struct *work)
> +{
> + struct mud_irq *mirq = container_of(work, struct mud_irq, work);
> + struct mud_irq_event *event;
> + unsigned long n, flags;
> + unsigned int irq;
> +
> + mdebug("%s: IN\n", __func__);
All of these prints need to go. They have no place in an upstreamed
driver and the whole thing reads much better without them.
> + while (true) {
> + spin_lock_irqsave(&mirq->lock, flags);
> + event = list_first_entry_or_null(&mirq->eventlist, struct mud_irq_event, node);
> + if (event) {
> + list_del(&event->node);
> + mdebug(" status: %*pb\n", mirq->num_irqs, event->status);
> + bitmap_and(event->status, event->status, mirq->mask, mirq->num_irqs);
> + }
> + spin_unlock_irqrestore(&mirq->lock, flags);
'\n'
> + if (!event)
> + break;
> +
> + for_each_set_bit(n, event->status, mirq->num_irqs) {
> + irq = irq_find_mapping(mirq->domain, n);
> + mdebug(" n=%lu irq=%u\n", n, irq);
> + if (irq)
> + handle_nested_irq(irq);
> + }
> +
> + kfree(event);
> + }
> +
> + mdebug("%s: OUT\n", __func__);
> +}
> +
> +#define BYTES_PER_LONG (BITS_PER_LONG / BITS_PER_BYTE)
> +
> +static void mud_irq_queue(struct urb *urb)
> +{
> + u8 *buf = urb->transfer_buffer + sizeof(u16);
Comment.
> + struct mud_irq *mirq = urb->context;
> + struct device *dev = &urb->dev->dev;
> + struct mud_irq_event *event = NULL;
> + unsigned int i, tag, diff;
> + unsigned long flags;
> +
> + if (urb->actual_length != urb->transfer_buffer_length) {
> + dev_err_once(dev, "Interrupt packet wrong length: %u\n",
> + urb->actual_length);
> + mirq->stats_illegal++;
> + return;
> + }
> +
> + spin_lock_irqsave(&mirq->lock, flags);
> +
> + tag = le16_to_cpup(urb->transfer_buffer);
> + if (tag == mirq->tag) {
> + dev_dbg(dev, "Interrupt tag=%u already seen, ignoring\n", tag);
> + mirq->stats_already_seen++;
> + goto unlock;
> + }
> +
> + if (tag > mirq->tag)
> + diff = tag - mirq->tag;
> + else
> + diff = U16_MAX - mirq->tag + tag + 1;
Comment.
> + if (diff > 1) {
> + dev_err_once(dev, "Interrupts lost: %u\n", diff - 1);
> + mirq->stats_lost += diff - 1;
> + }
> +
> + event = kzalloc(sizeof(*event), GFP_ATOMIC);
> + if (!event) {
> + mirq->stats_lost += 1;
> + goto unlock;
> + }
This hides a potential OOM issue.
Not sure what to do about that. Maybe the USB guys have an idea.
> + list_add_tail(&event->node, &mirq->eventlist);
> +
> + for (i = 0; i < (urb->transfer_buffer_length - sizeof(u16)); i++) {
> + unsigned long *val = &event->status[i / BYTES_PER_LONG];
> + unsigned int mod = i % BYTES_PER_LONG;
> +
> + if (!mod)
> + *val = buf[i];
> + else
> + *val |= ((unsigned long)buf[i]) << (mod * BITS_PER_BYTE);
> + }
Comment. In fact, comments throughout please.
(I'm going to stop saying "comment" from here on).
> + mdebug("%s: tag=%u\n", __func__, tag);
> +
> + mirq->tag = tag;
> +unlock:
> + spin_unlock_irqrestore(&mirq->lock, flags);
> +
> + if (event)
> + queue_work(mirq->workq, &mirq->work);
> +}
> +
> +static void mud_irq_urb_completion(struct urb *urb)
> +{
> + struct device *dev = &urb->dev->dev;
> + int ret;
> +
> + mdebug("%s: actual_length=%u\n", __func__, urb->actual_length);
> +
> + switch (urb->status) {
> + case 0:
> + mud_irq_queue(urb);
> + break;
> + case -EPROTO: /* FIXME: verify: dwc2 reports this on disconnect */
What does this mean? Why can't you fix it now?
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + dev_dbg(dev, "irq urb shutting down with status: %d\n", urb->status);
s/irq/IRQ/ in all comments and prints.
Same with URB?
> + return;
> + default:
> + dev_dbg(dev, "irq urb failure with status: %d\n", urb->status);
> + break;
So it's failed, but you're going to attempt to submit it anyway?
> + }
> +
> + ret = usb_submit_urb(urb, GFP_ATOMIC);
> + if (ret && ret != -ENODEV)
> + dev_err(dev, "irq usb_submit_urb failed with result %d\n", ret);
> +}
> +
> +static void mud_irq_mask(struct irq_data *data)
> +{
> + struct mud_irq *mirq = irq_data_get_irq_chip_data(data);
> + unsigned long flags;
> +
> + mdebug("%s: hwirq=%lu\n", __func__, data->hwirq);
> +
> + spin_lock_irqsave(&mirq->lock, flags);
> + clear_bit(data->hwirq, mirq->mask);
> + spin_unlock_irqrestore(&mirq->lock, flags);
> +}
> +
> +static void mud_irq_unmask(struct irq_data *data)
> +{
> + struct mud_irq *mirq = irq_data_get_irq_chip_data(data);
> + unsigned long flags;
> +
> + mdebug("%s: hwirq=%lu\n", __func__, data->hwirq);
> +
> + spin_lock_irqsave(&mirq->lock, flags);
> + set_bit(data->hwirq, mirq->mask);
> + spin_unlock_irqrestore(&mirq->lock, flags);
> +}
> +
> +static struct irq_chip mud_irq_chip = {
> + .name = "mud-irq",
> + .irq_mask = mud_irq_mask,
> + .irq_unmask = mud_irq_unmask,
> +};
This tabbing seems excessive.
> +static void __maybe_unused
> +mud_irq_domain_debug_show(struct seq_file *m, struct irq_domain *d,
> + struct irq_data *data, int ind)
Best not to over-shorten variable names for no good reason.
'm', 'd' and 'ind' aren't good choices.
> +{
> + struct mud_irq *mirq = d ? d->host_data : irq_data_get_irq_chip_data(data);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mirq->lock, flags);
> +
> + seq_printf(m, "%*sTag: %u\n", ind, "", mirq->tag);
> + seq_printf(m, "%*sIllegal: %u\n", ind, "", mirq->stats_illegal);
> + seq_printf(m, "%*sAlready seen: %u\n", ind, "", mirq->stats_already_seen);
> + seq_printf(m, "%*sLost: %u\n", ind, "", mirq->stats_lost);
> +
> + spin_unlock_irqrestore(&mirq->lock, flags);
> +}
> +
> +static int mud_irq_domain_map(struct irq_domain *d, unsigned int virq,
> + irq_hw_number_t hwirq)
> +{
> + irq_set_chip_data(virq, d->host_data);
> + irq_set_chip_and_handler(virq, &mud_irq_chip, handle_simple_irq);
> + irq_set_nested_thread(virq, true);
> + irq_set_noprobe(virq);
> +
> + return 0;
> +}
> +
> +static void mud_irq_domain_unmap(struct irq_domain *d, unsigned int virq)
> +{
> + irq_set_chip_and_handler(virq, NULL, NULL);
> + irq_set_chip_data(virq, NULL);
> +}
> +
> +static const struct irq_domain_ops mud_irq_ops = {
> + .map = mud_irq_domain_map,
> + .unmap = mud_irq_domain_unmap,
> +#ifdef CONFIG_GENERIC_IRQ_DEBUGFS
> + .debug_show = mud_irq_domain_debug_show,
> +#endif
> +};
> +
> +static int mud_irq_start(struct mud_irq *mirq)
> +{
> + if (!mirq)
> + return 0;
> +
> + return usb_submit_urb(mirq->urb, GFP_KERNEL);
> +}
> +
> +static void mud_irq_stop(struct mud_irq *mirq)
> +{
> + if (!mirq)
> + return;
> +
> + usb_kill_urb(mirq->urb);
> + flush_work(&mirq->work);
> +}
> +
> +static void mud_irq_release(struct mud_irq *mirq)
> +{
> + if (!mirq)
> + return;
> +
> + if (mirq->workq)
> + destroy_workqueue(mirq->workq);
> +
> + if (mirq->domain) {
> + irq_hw_number_t hwirq;
> +
> + for (hwirq = 0; hwirq < mirq->num_irqs; hwirq++)
> + irq_dispose_mapping(irq_find_mapping(mirq->domain, hwirq));
> +
> + irq_domain_remove(mirq->domain);
> + }
> +
> + usb_free_coherent(mirq->urb->dev, mirq->urb->transfer_buffer_length,
> + mirq->urb->transfer_buffer, mirq->urb->transfer_dma);
> + usb_free_urb(mirq->urb);
> + bitmap_free(mirq->mask);
> + kfree(mirq);
> +}
> +
> +static struct mud_irq *mud_irq_create(struct usb_interface *interface,
> + unsigned int num_irqs)
> +{
> + struct usb_device *usb = interface_to_usbdev(interface);
> + struct device *dev = &interface->dev;
> + struct usb_endpoint_descriptor *ep;
> + struct fwnode_handle *fn;
> + struct urb *urb = NULL;
> + struct mud_irq *mirq;
> + void *buf = NULL;
> + size_t buf_size;
> + int ret;
> +
> + mdebug("%s: dev->id=%d\n", __func__, dev->id);
> +
> + ret = usb_find_int_in_endpoint(interface->cur_altsetting, &ep);
> + if (ret == -ENXIO)
> + return NULL;
> + if (ret)
> + return ERR_PTR(ret);
> +
> + mirq = kzalloc(sizeof(*mirq), GFP_KERNEL);
> + if (!mirq)
> + return ERR_PTR(-ENOMEM);
> +
> + mirq->mask = bitmap_zalloc(num_irqs, GFP_KERNEL);
> + if (!mirq->mask) {
> + ret = -ENOMEM;
> + goto release;
> + }
> +
> + spin_lock_init(&mirq->lock);
> + mirq->num_irqs = num_irqs;
> +
> + urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!urb) {
> + ret = -ENOMEM;
> + goto release;
> + }
> +
> + buf_size = usb_endpoint_maxp(ep);
> + if (buf_size != (sizeof(u16) + DIV_ROUND_UP(num_irqs, BITS_PER_BYTE))) {
> + dev_err(dev, "Interrupt endpoint wMaxPacketSize too small: %zu\n", buf_size);
> + ret = -EINVAL;
> + goto release;
> + }
> +
> + buf = usb_alloc_coherent(usb, buf_size, GFP_KERNEL, &urb->transfer_dma);
> + if (!buf) {
> + usb_free_urb(urb);
> + ret = -ENOMEM;
> + goto release;
> + }
> +
> + usb_fill_int_urb(urb, usb,
> + usb_rcvintpipe(usb, usb_endpoint_num(ep)),
> + buf, buf_size, mud_irq_urb_completion,
> + mirq, ep->bInterval);
> + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> +
> + mirq->urb = urb;
> +
> + if (dev->of_node) {
> + fn = of_node_to_fwnode(dev->of_node);
> + } else {
> + fn = irq_domain_alloc_named_fwnode("mud-irq");
> + if (!fn) {
> + ret = -ENOMEM;
> + goto release;
> + }
> + }
> +
> + mirq->domain = irq_domain_create_linear(fn, num_irqs, &mud_irq_ops, mirq);
> + if (!dev->of_node)
> + irq_domain_free_fwnode(fn);
> + if (!mirq->domain) {
> + ret = -ENOMEM;
> + goto release;
> + }
> +
> + INIT_LIST_HEAD(&mirq->eventlist);
> + INIT_WORK(&mirq->work, mud_irq_work);
> +
> + mirq->workq = alloc_workqueue("mud-irq/%s", WQ_HIGHPRI, 0, dev_name(dev));
> + if (!mirq->workq) {
> + ret = -ENOMEM;
> + goto release;
> + }
> +
> + return mirq;
> +
> +release:
> + mud_irq_release(mirq);
> +
> + return ERR_PTR(ret);
> +}
> +
> +static int mud_probe_regmap(struct usb_interface *interface, struct mfd_cell *cell,
> + unsigned int index, struct mud_irq *mirq)
> +{
> + struct mud_cell_pdata *pdata;
> + struct resource *res = NULL;
> + int ret;
> +
> + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + ret = regmap_usb_get_map_descriptor(interface, index, &pdata->desc);
Can you give an example of what a desc might look like?
I'm particularly interested in pdata->desc.name.
> + if (ret)
> + goto error;
This will attempt to free 'res' which is currently NULL.
> + mdebug("%s: name='%s' index=%u\n", __func__, pdata->desc.name, index);
> + mdebug(" bRegisterValueBits=%u\n", pdata->desc.bRegisterValueBits);
> + mdebug(" bCompression=0x%02x\n", pdata->desc.bCompression);
> + mdebug(" bMaxTransferSizeOrder=%u (%ukB)\n",
> + pdata->desc.bMaxTransferSizeOrder,
> + (1 << pdata->desc.bMaxTransferSizeOrder) / 1024);
> +
> + if (mirq) {
> + res = kzalloc(sizeof(*res), GFP_KERNEL);
> + if (!res) {
> + ret = -ENOMEM;
> + goto error;
This will attempt to free 'res' which is currently NULL.
> + }
> +
> + res->flags = IORESOURCE_IRQ;
> + res->start = irq_create_mapping(mirq->domain, index);
> + mdebug(" res->start=%u\n", (unsigned int)res->start);
> + res->end = res->start;
> +
> + cell->resources = res;
> + cell->num_resources = 1;
> + }
> +
> + pdata->interface = interface;
This looks like something that should be stored in ddata.
> + pdata->index = index;
Don't usually like indexes - what is this used for?
> + cell->name = pdata->desc.name;
> + cell->platform_data = pdata;
> + cell->pdata_size = sizeof(*pdata);
> + /*
> + * A Multifunction USB Device can have multiple functions of the same
> + * type. mfd_add_device() in its current form will only match on the
> + * first node in the Device Tree.
> + */
> + cell->of_compatible = cell->name;
> +
> + return 0;
> +
> +error:
> + kfree(res);
I think you should remove this line, as it's never useful here.
> + kfree(pdata);
> +
> + return ret;
> +}
> +
> +static void mud_free(struct mud_device *mud)
> +{
> + unsigned int i;
> +
> + mud_irq_release(mud->mirq);
> +
> + for (i = 0; i < mud->num_cells; i++) {
> + kfree(mud->cells[i].platform_data);
> + kfree(mud->cells[i].resources);
> + }
> +
> + kfree(mud->cells);
> + kfree(mud);
> +}
> +
> +static int mud_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct device *dev = &interface->dev;
> + unsigned int i, num_regmaps;
> + struct mud_device *mud;
> + int ret;
> +
> + mdebug("%s: interface->dev.of_node=%px usb->dev.of_node=%px",
> + __func__, interface->dev.of_node,
> + usb_get_dev(interface_to_usbdev(interface))->dev.of_node);
> +
> + ret = regmap_usb_get_interface_descriptor(interface, &num_regmaps);
> + if (ret)
> + return ret;
> + if (!num_regmaps)
> + return -EINVAL;
> +
> + mdebug("%s: num_regmaps=%u\n", __func__, num_regmaps);
> +
> + mud = kzalloc(sizeof(*mud), GFP_KERNEL);
> + if (!mud)
> + return -ENOMEM;
> +
> + mud->mirq = mud_irq_create(interface, num_regmaps);
> + if (IS_ERR(mud->mirq)) {
> + ret = PTR_ERR(mud->mirq);
> + goto err_free;
> + }
> +
> + mud->num_cells = num_regmaps;
> + mud->cells = kcalloc(num_regmaps, sizeof(*mud->cells), GFP_KERNEL);
> + if (!mud->cells)
> + goto err_free;
> +
> + for (i = 0; i < num_regmaps; i++) {
> + ret = mud_probe_regmap(interface, &mud->cells[i], i, mud->mirq);
> + if (ret) {
> + dev_err(dev, "Failed to probe regmap index %i (error %d)\n", i, ret);
> + goto err_free;
> + }
> + }
> +
> + ret = mud_irq_start(mud->mirq);
> + if (ret) {
> + dev_err(dev, "Failed to start irq (error %d)\n", ret);
> + goto err_free;
> + }
> +
> + ret = mfd_add_hotplug_devices(dev, mud->cells, mud->num_cells);
> + if (ret) {
> + dev_err(dev, "Failed to add mfd devices to core.");
"MFD" is not a thing. It's made up.
"Failed to register child devices" makes more sense.
NIT: I don't see full stops in any of your other messages.
> + goto err_stop;
> + }
> +
> + mud->usb = usb_get_dev(interface_to_usbdev(interface));
> +
> + usb_set_intfdata(interface, mud);
> +
> + if (mud->usb->product)
> + dev_info(dev, "%s\n", mud->usb->product);
> +
> + return 0;
> +
> +err_stop:
> + mud_irq_stop(mud->mirq);
> +err_free:
> + mud_free(mud);
> +
> + return ret;
> +}
> +
> +static void mud_disconnect(struct usb_interface *interface)
> +{
> + struct mud_device *mud = usb_get_intfdata(interface);
> +
> + mfd_remove_devices(&interface->dev);
> + mud_irq_stop(mud->mirq);
> + usb_put_dev(mud->usb);
> + mud_free(mud);
> +
> + dev_dbg(&interface->dev, "disconnected\n");
> +}
> +
> +static const struct usb_device_id mud_table[] = {
> + /*
> + * FIXME:
> + * Apply for a proper pid: https://github.com/openmoko/openmoko-usb-oui
> + *
> + * Or maybe the Linux Foundation will provide one from their vendor id.
> + */
Probably not a good idea to take this into the upstream kernel without
a valid, registered PID. Suggest you do this *first*.
> + { USB_DEVICE_INTERFACE_CLASS(0x1d50, 0x6150, USB_CLASS_VENDOR_SPEC) },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, mud_table);
> +
> +static struct usb_driver mud_driver = {
> + .name = "mud",
> + .probe = mud_probe,
> + .disconnect = mud_disconnect,
> + .id_table = mud_table,
> +};
> +
> +module_usb_driver(mud_driver);
> +
> +MODULE_DESCRIPTION("Generic USB Device mfd core driver");
> +MODULE_AUTHOR("Noralf Trønnes <noralf@xxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/mud.h b/include/linux/mfd/mud.h
> new file mode 100644
> index 000000000000..b2059fa57429
> --- /dev/null
> +++ b/include/linux/mfd/mud.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef __LINUX_MUD_H
> +#define __LINUX_MUD_H
> +
> +#include <linux/regmap_usb.h>
> +
> +struct usb_interface;
> +
> +struct mud_cell_pdata {
> + struct usb_interface *interface;
Shouldn't be here - more of a ddata thing.
> + unsigned int index;
Indexes are generally not a good idea.
> + struct regmap_usb_map_descriptor desc;
Shouldn't be here - more of a ddata thing.
> +};
> +
> +#endif
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
