diff mbox series

[v3,01/14] mux: core: Add mux_control_get_optional() API

Message ID 20170922183803.10701-1-hdegoede@redhat.com
State New
Headers show
Series [v3,01/14] mux: core: Add mux_control_get_optional() API | expand

Commit Message

Hans de Goede Sept. 22, 2017, 6:37 p.m. UTC 
From: Stephen Boyd <stephen.boyd@linaro.org>


Sometimes drivers only use muxes under certain scenarios. For
example, the chipidea usb controller may be connected to a usb
switch on some platforms, and connected directly to a usb port on
others. The driver won't know one way or the other though, so add
a mux_control_get_optional() API that allows the driver to
differentiate errors getting the mux from there not being a mux
for the driver to use at all.

Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>

Signed-off-by: Hans de Goede <hdegoede@redhat.com>

---
 Documentation/driver-model/devres.txt |   1 +
 drivers/mux/core.c                    | 104 +++++++++++++++++++++++++++-------
 include/linux/mux/consumer.h          |   4 ++
 3 files changed, 89 insertions(+), 20 deletions(-)

-- 
2.14.1

Comments

Heikki Krogerus Sept. 26, 2017, 7:45 a.m. UTC | #1 
Hi Hans,

Sorry about the late response.

On Fri, Sep 22, 2017 at 08:37:54PM +0200, Hans de Goede wrote:
> Add MUX_USB_* and MUX_TYPEC_* state constant defines, which can be used by

> USB device/host, resp. Type-C polarity/role/altmode mux drivers and

> consumers to ensure that they agree on the meaning of the

> mux_control_select() state argument.

> 

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

> ---

> Changes in v2:

> -Start numbering of defines at 0 not 1

> -Use a new usb.h header, rather then adding these to consumer.h

> -Add separate MUX_USB_* and MUX_TYPEC_* defines

> 

> Changes in v3:

> -Simplify MUX_TYPEC_* states, drop having separate USB HOST / DEVICE states

> ---

>  include/linux/mux/usb.h | 31 +++++++++++++++++++++++++++++++

>  1 file changed, 31 insertions(+)

>  create mode 100644 include/linux/mux/usb.h

> 

> diff --git a/include/linux/mux/usb.h b/include/linux/mux/usb.h

> new file mode 100644

> index 000000000000..2fec06846e14

> --- /dev/null

> +++ b/include/linux/mux/usb.h

> @@ -0,0 +1,31 @@

> +/*

> + * mux/usb.h - definitions for USB multiplexers

> + *

> + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>

> + *

> + * 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 _LINUX_MUX_USB_H

> +#define _LINUX_MUX_USB_H

> +

> +/* Mux state values for USB device/host role muxes */

> +#define MUX_USB_DEVICE		(0) /* USB device mode */

> +#define MUX_USB_HOST		(1) /* USB host mode */

> +#define MUX_USB_STATES		(2)

> +

> +/*

> + * Mux state values for Type-C polarity/role/altmode muxes.

> + *

> + * MUX_TYPEC_POLARITY_INV may be or-ed together with any other mux-state as

> + * inverted-polarity (Type-C plugged in upside down) can happen with any

> + * other mux-state.

> + */

> +#define MUX_TYPEC_POLARITY_INV		BIT(0)   /* Polarity inverted bit */

> +#define MUX_TYPEC_USB			(0 << 1) /* USB only mode */


Predefined values for the usb role are probable OK (maybe), but..

> +#define MUX_TYPEC_USB_AND_DP		(1 << 1) /* USB host + 2 lanes DP */

> +#define MUX_TYPEC_DP			(2 << 1) /* 4 lanes Display Port */

> +#define MUX_TYPEC_STATES		(3 << 1)


..for alternate modes, no way. We don't need to try to keep a list of
all the possible states in one place. The pin configurations need to be
defined per alternate mode (per SVID), and we should not mix the
whole mux subsystem into that.

You are also only considering muxing. We need to deliver the
negotiated pin configurations to other components as well. For
example, in case of DisplayPort, the DP controller will need to know
at least the lane count, but also most likely the plug orientation.

And also, I think this is clear to everybody, but just in case it
isn't: Let's not mix TCPM or TCPC into the alternate mode specific pin
configuration handling at all. The alternate mode specific drivers can
take care of that. I think I need to prepare RFC out of my "alternate
mode bus" idea to give you guys an idea how that should work. Give me
a day or two.

But in any case, drop all alternate mode stuff from this series.
Let's move one step at the time.


Thanks,

-- 
heikki
Chanwoo Choi Oct. 18, 2017, 2:33 a.m. UTC | #2 
Hi Hans,

On 2017년 09월 23일 03:37, Hans de Goede wrote:
> Cherry Trail SoCs have a built-in USB-role mux for switching between

> the host and device controllers, rather then using an external mux

> controller by a GPIO.

> 

> There is a driver using the mux-subsys to control this mux, this

> commit adds support to the intel-int3496 driver to get a mux_controller

> handle for the mux and set the mux through the mux-subsys rather then

> through a GPIO.

> 

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

> ---

> Changes in v2:

> -Drop || COMPILE_TEST from Kconfig depends on, as we will now fail to

>  compile on !X86

> -Minor code style tweaks

> ---

>  drivers/extcon/Kconfig                |  3 +-

>  drivers/extcon/extcon-intel-int3496.c | 59 +++++++++++++++++++++++++++++++++++

>  2 files changed, 61 insertions(+), 1 deletion(-)


Acked-by: Chanwoo Choi <cw00.choi@samsung.com>


[snip]

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics
Hans de Goede Oct. 18, 2017, 9:14 a.m. UTC | #3 
Hi,

On 18-10-17 04:33, Chanwoo Choi wrote:
> Hi Hans,

> 

> On 2017년 09월 23일 03:37, Hans de Goede wrote:

>> Cherry Trail SoCs have a built-in USB-role mux for switching between

>> the host and device controllers, rather then using an external mux

>> controller by a GPIO.

>>

>> There is a driver using the mux-subsys to control this mux, this

>> commit adds support to the intel-int3496 driver to get a mux_controller

>> handle for the mux and set the mux through the mux-subsys rather then

>> through a GPIO.

>>

>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

>> ---

>> Changes in v2:

>> -Drop || COMPILE_TEST from Kconfig depends on, as we will now fail to

>>   compile on !X86

>> -Minor code style tweaks

>> ---

>>   drivers/extcon/Kconfig                |  3 +-

>>   drivers/extcon/extcon-intel-int3496.c | 59 +++++++++++++++++++++++++++++++++++

>>   2 files changed, 61 insertions(+), 1 deletion(-)

> 

> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>


Note that there have been some comments on this series, and it
is not sure yet how we are going to end up handling this. So
please do not merge this yet, as we may end up with another
solution.

Regards,

Hans
Chanwoo Choi Oct. 18, 2017, 9:40 a.m. UTC | #4 
On 2017년 10월 18일 18:14, Hans de Goede wrote:
> Hi,

> 

> On 18-10-17 04:33, Chanwoo Choi wrote:

>> Hi Hans,

>>

>> On 2017년 09월 23일 03:37, Hans de Goede wrote:

>>> Cherry Trail SoCs have a built-in USB-role mux for switching between

>>> the host and device controllers, rather then using an external mux

>>> controller by a GPIO.

>>>

>>> There is a driver using the mux-subsys to control this mux, this

>>> commit adds support to the intel-int3496 driver to get a mux_controller

>>> handle for the mux and set the mux through the mux-subsys rather then

>>> through a GPIO.

>>>

>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

>>> ---

>>> Changes in v2:

>>> -Drop || COMPILE_TEST from Kconfig depends on, as we will now fail to

>>>   compile on !X86

>>> -Minor code style tweaks

>>> ---

>>>   drivers/extcon/Kconfig                |  3 +-

>>>   drivers/extcon/extcon-intel-int3496.c | 59 +++++++++++++++++++++++++++++++++++

>>>   2 files changed, 61 insertions(+), 1 deletion(-)

>>

>> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

> 

> Note that there have been some comments on this series, and it

> is not sure yet how we are going to end up handling this. So

> please do not merge this yet, as we may end up with another

> solution.


Sure. I don't merge only this patch. After finishing the review
of this patchset, one maintainer apply all patches and then
send immutable pull request.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics
diff mbox series

Patch

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 69f08c0f23a8..5e41f3ac8a05 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -343,6 +343,7 @@  MUX
   devm_mux_chip_alloc()
   devm_mux_chip_register()
   devm_mux_control_get()
+  devm_mux_control_get_optional()
 
 PER-CPU MEM
   devm_alloc_percpu()
diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 6e5cf9d9cd99..244bceb17877 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -289,6 +289,9 @@  EXPORT_SYMBOL_GPL(devm_mux_chip_register);
  */
 unsigned int mux_control_states(struct mux_control *mux)
 {
+	if (!mux)
+		return 0;
+
 	return mux->states;
 }
 EXPORT_SYMBOL_GPL(mux_control_states);
@@ -338,6 +341,9 @@  int mux_control_select(struct mux_control *mux, unsigned int state)
 {
 	int ret;
 
+	if (!mux)
+		return 0;
+
 	ret = down_killable(&mux->lock);
 	if (ret < 0)
 		return ret;
@@ -370,6 +376,9 @@  int mux_control_try_select(struct mux_control *mux, unsigned int state)
 {
 	int ret;
 
+	if (!mux)
+		return 0;
+
 	if (down_trylock(&mux->lock))
 		return -EBUSY;
 
@@ -398,6 +407,9 @@  int mux_control_deselect(struct mux_control *mux)
 {
 	int ret = 0;
 
+	if (!mux)
+		return 0;
+
 	if (mux->idle_state != MUX_IDLE_AS_IS &&
 	    mux->idle_state != mux->cached_state)
 		ret = mux_control_set(mux, mux->idle_state);
@@ -423,14 +435,8 @@  static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
 	return dev ? to_mux_chip(dev) : NULL;
 }
 
-/**
- * mux_control_get() - Get the mux-control for a device.
- * @dev: The device that needs a mux-control.
- * @mux_name: The name identifying the mux-control.
- *
- * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
- */
-struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+static struct mux_control *
+__mux_control_get(struct device *dev, const char *mux_name, bool optional)
 {
 	struct device_node *np = dev->of_node;
 	struct of_phandle_args args;
@@ -442,16 +448,22 @@  struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 	if (mux_name) {
 		index = of_property_match_string(np, "mux-control-names",
 						 mux_name);
+		if ((index == -EINVAL || index == -ENODATA) && optional)
+			return NULL;
 		if (index < 0) {
 			dev_err(dev, "mux controller '%s' not found\n",
 				mux_name);
 			return ERR_PTR(index);
 		}
+		/* OF does point to a mux, so it's no longer optional */
+		optional = false;
 	}
 
 	ret = of_parse_phandle_with_args(np,
 					 "mux-controls", "#mux-control-cells",
 					 index, &args);
+	if (ret == -ENOENT && optional)
+		return NULL;
 	if (ret) {
 		dev_err(dev, "%pOF: failed to get mux-control %s(%i)\n",
 			np, mux_name ?: "", index);
@@ -484,8 +496,35 @@  struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 
 	return &mux_chip->mux[controller];
 }
+
+/**
+ * mux_control_get() - Get the mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+{
+	return __mux_control_get(dev, mux_name, false);
+}
 EXPORT_SYMBOL_GPL(mux_control_get);
 
+/**
+ * mux_control_get_optional() - Get the optional mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: NULL if no mux with the provided name is found, a pointer to
+ * the named mux-control or an ERR_PTR with a negative errno.
+ */
+struct mux_control *
+mux_control_get_optional(struct device *dev, const char *mux_name)
+{
+	return __mux_control_get(dev, mux_name, true);
+}
+EXPORT_SYMBOL_GPL(mux_control_get_optional);
+
 /**
  * mux_control_put() - Put away the mux-control for good.
  * @mux: The mux-control to put away.
@@ -494,6 +533,9 @@  EXPORT_SYMBOL_GPL(mux_control_get);
  */
 void mux_control_put(struct mux_control *mux)
 {
+	if (!mux)
+		return;
+
 	put_device(&mux->chip->dev);
 }
 EXPORT_SYMBOL_GPL(mux_control_put);
@@ -505,16 +547,8 @@  static void devm_mux_control_release(struct device *dev, void *res)
 	mux_control_put(mux);
 }
 
-/**
- * devm_mux_control_get() - Get the mux-control for a device, with resource
- *			    management.
- * @dev: The device that needs a mux-control.
- * @mux_name: The name identifying the mux-control.
- *
- * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
- */
-struct mux_control *devm_mux_control_get(struct device *dev,
-					 const char *mux_name)
+static struct mux_control *
+__devm_mux_control_get(struct device *dev, const char *mux_name, bool optional)
 {
 	struct mux_control **ptr, *mux;
 
@@ -522,8 +556,8 @@  struct mux_control *devm_mux_control_get(struct device *dev,
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	mux = mux_control_get(dev, mux_name);
-	if (IS_ERR(mux)) {
+	mux = __mux_control_get(dev, mux_name, optional);
+	if (IS_ERR_OR_NULL(mux)) {
 		devres_free(ptr);
 		return mux;
 	}
@@ -533,8 +567,38 @@  struct mux_control *devm_mux_control_get(struct device *dev,
 
 	return mux;
 }
+
+/**
+ * devm_mux_control_get() - Get the mux-control for a device, with resource
+ *			    management.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *
+devm_mux_control_get(struct device *dev, const char *mux_name)
+{
+	return __devm_mux_control_get(dev, mux_name, false);
+}
 EXPORT_SYMBOL_GPL(devm_mux_control_get);
 
+/**
+ * devm_mux_control_get_optional() - Get the optional mux-control for a device,
+ *				     with resource management.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: NULL if no mux with the provided name is found, a pointer to
+ * the named mux-control or an ERR_PTR with a negative errno.
+ */
+struct mux_control *
+devm_mux_control_get_optional(struct device *dev, const char *mux_name)
+{
+	return __devm_mux_control_get(dev, mux_name, true);
+}
+EXPORT_SYMBOL_GPL(devm_mux_control_get_optional);
+
 /*
  * Using subsys_initcall instead of module_init here to try to ensure - for
  * the non-modular case - that the subsystem is initialized when mux consumers
diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
index ea96d4c82be7..fc98547bf494 100644
--- a/include/linux/mux/consumer.h
+++ b/include/linux/mux/consumer.h
@@ -26,9 +26,13 @@  int __must_check mux_control_try_select(struct mux_control *mux,
 int mux_control_deselect(struct mux_control *mux);
 
 struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
+struct mux_control *mux_control_get_optional(struct device *dev,
+					     const char *mux_name);
 void mux_control_put(struct mux_control *mux);
 
 struct mux_control *devm_mux_control_get(struct device *dev,
 					 const char *mux_name);
+struct mux_control *devm_mux_control_get_optional(struct device *dev,
+						  const char *mux_name);
 
 #endif /* _LINUX_MUX_CONSUMER_H */