Re: [RFC 2/9] mfd: Add driver for Multifunction USB Device

[Lee Jones <lee.jones@linaro.org>]

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@tronnes.org>
> ---
>  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@tronnes.org>");
> +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