Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[David Cohen]

Hi Heikki,

Follow my comments below.

On Tue, Dec 01, 2015 at 03:32:37PM +0200, Heikki Krogerus wrote:
> Several Intel PCHs and SOCs have an internal mux that is
> used to share one USB port between USB Device Controller and
> xHCI. The mux is normally handled by System FW/BIOS, but not
> always. For those platforms where the FW does not take care
> of the mux, this driver is needed.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
>  drivers/extcon/Kconfig               |   5 ++
>  drivers/extcon/Makefile              |   1 +
>  drivers/extcon/extcon-intel-usb.c    | 112 +++++++++++++++++++++++++++++++++++
>  include/linux/extcon/intel_usb_mux.h |  31 ++++++++++
>  4 files changed, 149 insertions(+)
>  create mode 100644 drivers/extcon/extcon-intel-usb.c
>  create mode 100644 include/linux/extcon/intel_usb_mux.h
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 0cebbf6..0a7ccb1 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -118,3 +118,8 @@ config EXTCON_USB_GPIO
>  	  Used typically if GPIO is used for USB ID pin detection.
>  
>  endif
> +
> +config EXTCON_INTEL_USB
> +	bool
> +	depends on X86 && USB
> +	select EXTCON
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index ba787d0..e6e031a 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
>  obj-$(CONFIG_EXTCON_USB_GPIO)	+= extcon-usb-gpio.o
> +obj-$(CONFIG_EXTCON_INTEL_USB)	+= extcon-intel-usb.o
> diff --git a/drivers/extcon/extcon-intel-usb.c b/drivers/extcon/extcon-intel-usb.c
> new file mode 100644
> index 0000000..5534781
> --- /dev/null
> +++ b/drivers/extcon/extcon-intel-usb.c
> @@ -0,0 +1,112 @@
> +/**
> + * extcon-intel-usb.c - Driver for Intel USB mux
> + *
> + * Copyright (C) 2015 Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/extcon.h>
> +
> +#include <linux/extcon/intel_usb_mux.h>
> +
> +#define INTEL_MUX_CFG0		0x00
> +#define INTEL_MUX_CFG1		0x04
> +
> +#define CFG0_SW_DRD_MODE_MASK	0x3
> +#define CFG0_SW_DRD_DYN		0
> +#define CFG0_SW_DRD_STATIC_HOST	1
> +#define CFG0_SW_DRD_STATIC_DEV	2
> +#define CFG0_SW_SYNC_SS_AND_HS	BIT(2)
> +#define CFG0_SW_SWITCH_EN	BIT(16)
> +#define CFG0_SW_IDPIN		BIT(20)
> +#define CFG0_SW_IDPIN_EN	BIT(21)
> +#define CFG0_SW_VBUS_VALID	BIT(24)
> +
> +#define CFG1_MODE		BIT(29)
> +
> +struct intel_usb_mux {
> +	struct notifier_block nb;
> +	struct extcon_dev edev;
> +	void __iomem *regs;
> +	u32 cfg0_ctx;
> +};
> +
> +static const int intel_mux_cable[] = {
> +	EXTCON_USB_HOST,
> +	EXTCON_NONE,
> +};
> +
> +static int intel_usb_mux_notifier(struct notifier_block *nb,
> +				  unsigned long old, void *ptr)
> +{
> +	struct intel_usb_mux *mux = container_of(nb, struct intel_usb_mux, nb);
> +	u32 val;
> +
> +	if (mux->edev.state)
> +		val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
> +	else
> +		val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
> +		      CFG0_SW_DRD_STATIC_DEV;
> +
> +	writel(val, mux->regs);
> +	return NOTIFY_OK;
> +}
> +
> +struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
> +					     struct resource *r)
> +{
> +	struct intel_usb_mux *mux;
> +	int ret;
> +
> +	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> +	if (!mux)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mux->regs = ioremap_nocache(r->start, resource_size(r));
> +	if (!mux->regs) {
> +		kfree(mux);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	mux->cfg0_ctx = readl(mux->regs + INTEL_MUX_CFG0);
> +
> +	mux->edev.dev.parent = dev;
> +	mux->edev.supported_cable = intel_mux_cable;
> +
> +	ret = extcon_dev_register(&mux->edev);
> +	if (ret)
> +		goto err;
> +
> +	mux->edev.name = "intel_usb_mux";
> +	mux->edev.state = !!(readl(mux->regs + INTEL_MUX_CFG1) & CFG1_MODE);
> +
> +	/* An external source needs to tell us what to do */
> +	mux->nb.notifier_call = intel_usb_mux_notifier;
> +	ret = extcon_register_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> +	if (ret) {
> +		dev_err(&mux->edev.dev, "failed to register notifier\n");
> +		extcon_dev_unregister(&mux->edev);
> +		goto err;
> +	}
> +	return mux;
> +err:
> +	iounmap(mux->regs);
> +	kfree(mux);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> +
> +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> +{

There are still 2 pending comments for this unregister function:

1) How about a protection against unbalanced unregistering? In case an
user mistakenly unregisters twice or unregisters without a previous
registering.

2) When unregistering, you need to clear the bit CFG0_SW_IDPIN_EN to
set mux to automatic mode again. Or maybe you could save CFG0's initial
value and set it again here. Anyway, when unregistering the mux driver
you need to make sure the mux goes back to its original configuration.

Br, David

> +	extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> +	extcon_dev_unregister(&mux->edev);
> +	writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> +	iounmap(mux->regs);
> +	kfree(mux);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
> diff --git a/include/linux/extcon/intel_usb_mux.h b/include/linux/extcon/intel_usb_mux.h
> new file mode 100644
> index 0000000..f18ce52
> --- /dev/null
> +++ b/include/linux/extcon/intel_usb_mux.h
> @@ -0,0 +1,31 @@
> +/*
> + * Driver for Intel USB mux
> + *
> + * Copyright (C) 2015 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _INTEL_USB_MUX_H
> +#define _INTEL_USB_MUX_H
> +
> +#include <linux/errno.h>
> +
> +struct intel_usb_mux;
> +
> +#ifdef CONFIG_EXTCON_INTEL_USB
> +struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
> +					     struct resource *r);
> +void intel_usb_mux_unregister(struct intel_usb_mux *mux);
> +#else
> +static inline struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
> +							   struct resource *r)
> +{
> +	return ERR_PTR(-ENOTSUPP);
> +}
> +static inline void intel_usb_mux_unregister(struct intel_usb_mux *mux) { }
> +#endif /* CONFIG_EXTCON_INTEL_USB */
> +
> +#endif /* _INTEL_USB_MUX_H */
> -- 
> 2.6.2
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/