Re: [PATCH v2 1/4] drivers: Add a new framework for multiplexer devices

Hi Jean-Jacques,

On Tue, 5 Nov 2019 at 04:50, Jean-Jacques Hiblot jjhiblot@ti.com wrote:

Add a new subsystem that handles multiplexer controllers. The API is the same as in Linux.

Signed-off-by: Jean-Jacques Hiblot jjhiblot@ti.com

---

Changes in v2:

• Fixed warning in mux_of_xlate_default()
• Improved documentation
• Fixed SPL build

drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/mux/Kconfig | 7 + drivers/mux/Makefile | 6 + drivers/mux/mux-uclass.c | 270 ++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/dt-bindings/mux/mux.h | 17 +++ include/mux-internal.h | 103 +++++++++++++ include/mux.h | 113 ++++++++++++++ 9 files changed, 520 insertions(+) create mode 100644 drivers/mux/Kconfig create mode 100644 drivers/mux/Makefile create mode 100644 drivers/mux/mux-uclass.c create mode 100644 include/dt-bindings/mux/mux.h create mode 100644 include/mux-internal.h create mode 100644 include/mux.h

diff --git a/drivers/Kconfig b/drivers/Kconfig index 9d99ce0226..450aa76e82 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -60,6 +60,8 @@ source "drivers/mmc/Kconfig"

source "drivers/mtd/Kconfig"

+source "drivers/mux/Kconfig"

source "drivers/net/Kconfig"

source "drivers/nvme/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index 0befeddfcb..9d64742580 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -12,6 +12,7 @@ obj-$(CONFIG_$(SPL_TPL_)INPUT) += input/ obj-$(CONFIG_$(SPL_TPL_)LED) += led/ obj-$(CONFIG_$(SPL_TPL_)MMC_SUPPORT) += mmc/ obj-$(CONFIG_$(SPL_TPL_)NAND_SUPPORT) += mtd/nand/raw/ +obj-$(CONFIG_$(SPL_)MULTIPLEXER) += mux/ obj-$(CONFIG_$(SPL_TPL_)PCH_SUPPORT) += pch/ obj-$(CONFIG_$(SPL_TPL_)PCI) += pci/ obj-$(CONFIG_$(SPL_TPL_)PHY) += phy/ diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig new file mode 100644 index 0000000000..ad0199c058 --- /dev/null +++ b/drivers/mux/Kconfig @@ -0,0 +1,7 @@ +menu "Multiplexer drivers"

+config MULTIPLEXER

•   bool "Multiplexer Support"
  

•   depends on DM
  

Please add help here.

+endmenu diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile new file mode 100644 index 0000000000..351e4363d3 --- /dev/null +++ b/drivers/mux/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0+ +# +# (C) Copyright 2019 +# Jean-Jacques Hiblot jjhiblot@ti.com

+obj-$(CONFIG_$(SPL_)MULTIPLEXER) += mux-uclass.o diff --git a/drivers/mux/mux-uclass.c b/drivers/mux/mux-uclass.c new file mode 100644 index 0000000000..6aaf4dc964 --- /dev/null +++ b/drivers/mux/mux-uclass.c @@ -0,0 +1,270 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Multiplexer subsystem
• • Based on the linux multiplexer framework
• • Copyright (C) 2017 Axentia Technologies AB
• • Author: Peter Rosin peda@axentia.se
• • Copyright (C) 2017-2018 Texas Instruments Incorporated - http://www.ti.com/
• • Jean-Jacques Hiblot jjhiblot@ti.com
• */

+#include <common.h> +#include <dm.h> +#include <mux-internal.h> +#include <dm/device-internal.h> +#include <dt-bindings/mux/mux.h>

+/*

• • The idle-as-is "state" is not an actual state that may be selected, it
• • only implies that the state should not be changed. So, use that state
• • as indication that the cached state of the multiplexer is unknown.
• */

+#define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS

+static inline const struct mux_control_ops *mux_dev_ops(struct udevice *dev) +{

•   return (const struct mux_control_ops *)dev->driver->ops;
  

+}

+static int mux_control_set(struct mux_control *mux, int state)

Please add comments to these static functions.

+{

•   int ret = mux_dev_ops(mux->dev)->set(mux, state);
  

•   mux->cached_state = ret < 0 ? MUX_CACHE_UNKNOWN : state;
  

•   return ret;
  

+}

+unsigned int mux_control_states(struct mux_control *mux) +{

•   return mux->states;
  

+}

+static int __mux_control_select(struct mux_control *mux, int state) +{

•   int ret;
  

•   if (WARN_ON(state < 0 || state >= mux->states))
  

•           return -EINVAL;
  

•   if (mux->cached_state == state)
  

•           return 0;
  

•   ret = mux_control_set(mux, state);
  

•   if (ret >= 0)
  

•           return 0;
  

•   /* The mux update failed, try to revert if appropriate... */
  

•   if (mux->idle_state != MUX_IDLE_AS_IS)
  

•           mux_control_set(mux, mux->idle_state);
  

•   return ret;
  

+}

+int mux_control_select(struct mux_control *mux, unsigned int state) +{

•   int ret;
  

•   if (mux->in_use)
  

•           return -EBUSY;
  

•   ret = __mux_control_select(mux, state);
  

•   if (ret < 0)
  

•           return ret;
  

•   mux->in_use = true;
  

•   return 0;
  

+}

+int mux_control_deselect(struct mux_control *mux) +{

•   int ret = 0;
  

•   if (mux->idle_state != MUX_IDLE_AS_IS &&
  

•       mux->idle_state != mux->cached_state)
  

•           ret = mux_control_set(mux, mux->idle_state);
  

•   mux->in_use = false;
  

•   return ret;
  

+}

+static int mux_of_xlate_default(struct mux_chip *mux_chip,

•                           struct ofnode_phandle_args *args,
  

•                           struct mux_control **muxp)
  

+{

•   struct mux_control *mux;
  

•   int id;
  

•   debug("%s(muxp=%p)\n", __func__, muxp);
  

Could use log_debug() perhaps?

•   if (args->args_count > 1) {
  

•           debug("Invaild args_count: %d\n", args->args_count);
  

•           return -EINVAL;
  

•   }
  

•   if (args->args_count)
  

•           id = args->args[0];
  

•   else
  

•           id = 0;
  

•   if (id >= mux_chip->controllers) {
  

•           dev_err(dev, "bad mux controller %u specified in %s\n",
  

•                   id, ofnode_get_name(args->node));
  

•           return -ERANGE;
  

•   }
  

•   mux = &mux_chip->mux[id];
  

•   mux->id = id;
  

•   *muxp = mux;
  

•   return 0;
  

+}

+static int mux_get_by_indexed_prop(struct udevice *dev, const char *prop_name,

•                              int index, struct mux_control **mux)
  

Again please comment these static functions.

+{

•   int ret;
  

•   struct ofnode_phandle_args args;
  

•   struct udevice *dev_mux;
  

•   const struct mux_control_ops *ops;
  

•   struct mux_chip *mux_chip;
  

•   debug("%s(dev=%p, index=%d, mux=%p)\n", __func__, dev, index, mux);
  

•   ret = dev_read_phandle_with_args(dev, prop_name, "#mux-control-cells",
  

•                                    0, index, &args);
  

•   if (ret) {
  

•           debug("%s: fdtdec_parse_phandle_with_args failed: err=%d\n",
  

•                 __func__, ret);
  

•           return ret;
  

•   }
  

•   ret = uclass_get_device_by_ofnode(UCLASS_MUX, args.node, &dev_mux);
  

•   if (ret) {
  

•           debug("%s: uclass_get_device_by_ofnode failed: err=%d\n",
  

•                 __func__, ret);
  

•           return ret;
  

•   }
  

•   mux_chip = dev_get_uclass_priv(dev_mux);
  

•   ops = mux_dev_ops(dev_mux);
  

•   if (ops->of_xlate)
  

•           ret = ops->of_xlate(mux_chip, &args, mux);
  

•   else
  

•           ret = mux_of_xlate_default(mux_chip, &args, mux);
  

•   if (ret) {
  

•           debug("of_xlate() failed: %d\n", ret);
  

•           return ret;
  

•   }
  

•   (*mux)->dev = dev_mux;
  

•   return 0;
  

+}

+int mux_get_by_index(struct udevice *dev, int index, struct mux_control **mux) +{

•   return mux_get_by_indexed_prop(dev, "mux-controls", index, mux);
  

+}

+int mux_control_get(struct udevice *dev, const char *name,

•               struct mux_control **mux)
  

+{

•   int index;
  

•   debug("%s(dev=%p, name=%s, mux=%p)\n", __func__, dev, name, mux);
  

•   index = dev_read_stringlist_search(dev, "mux-control-names", name);
  

•   if (index < 0) {
  

•           debug("fdt_stringlist_search() failed: %d\n", index);
  

•           return index;
  

•   }
  

•   return mux_get_by_index(dev, index, mux);
  

+}

+void mux_control_put(struct mux_control *mux) +{

•   mux_control_deselect(mux);
  

+}

+static void devm_mux_control_release(struct udevice *dev, void *res) +{

•   mux_control_put(*(struct mux_control **)res);
  

+}

+struct mux_control *devm_mux_control_get(struct udevice *dev, const char *id)

This should return an integer error with mux_control as a parameter passed in by the caller, not allocated here. The madness below and in the caller to convert everything to pointers makes no sense to me.

+{

•   int rc;
  

•   struct mux_control **mux;
  

•   mux = devres_alloc(devm_mux_control_release,
  

•                      sizeof(struct mux_control *), __GFP_ZERO);
  

•   if (unlikely(!mux))
  

•           return ERR_PTR(-ENOMEM);
  

Then you don't need to alloc this since it is passed in.

•   rc = mux_control_get(dev, id, mux);
  

•   if (rc)
  

•           return ERR_PTR(rc);
  

•   devres_add(dev, mux);
  

•   return *mux;
  

+}

+int mux_alloc_controllers(struct udevice *dev, unsigned int controllers)

This function needs comments in its header file.

+{

•   int i;
  

•   struct mux_chip *mux_chip = dev_get_uclass_priv(dev);
  

•   mux_chip->mux = devm_kmalloc(dev,
  

•                                sizeof(struct mux_control) * controllers,
  

•                                __GFP_ZERO);
  

•   if (!mux_chip->mux)
  

•           return -ENOMEM;
  

•   mux_chip->controllers = controllers;
  

•   for (i = 0; i < mux_chip->controllers; ++i) {
  

•           struct mux_control *mux = &mux_chip->mux[i];
  

•           mux->dev = dev;
  

•           mux->cached_state = MUX_CACHE_UNKNOWN;
  

•           mux->idle_state = MUX_IDLE_AS_IS;
  

•           mux->in_use = false;
  

•           mux->id = i;
  

•   }
  

•   return 0;
  

+}

+int mux_uclass_post_probe(struct udevice *dev)

static

+{

•   int i, ret;
  

•   struct mux_chip *mux_chip = dev_get_uclass_priv(dev);
  

Can you add a comment here as to what this is doing?

•   for (i = 0; i < mux_chip->controllers; ++i) {
  

•           struct mux_control *mux = &mux_chip->mux[i];
  

•           if (mux->idle_state == mux->cached_state)
  

•                   continue;
  

•           ret = mux_control_set(mux, mux->idle_state);
  

•           if (ret < 0) {
  

•                   dev_err(&mux_chip->dev, "unable to set idle state\n");
  

•                   return ret;
  

•           }
  

•   }
  

•   return 0;
  

+}

+UCLASS_DRIVER(mux) = {

•   .id             = UCLASS_MUX,
  

•   .name           = "mux",
  

•   .post_probe     = mux_uclass_post_probe,
  

•   .per_device_auto_alloc_size = sizeof(struct mux_chip),
  

+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 0c563d898b..28822689a9 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -66,6 +66,7 @@ enum uclass_id { UCLASS_MMC, /* SD / MMC card or chip */ UCLASS_MOD_EXP, /* RSA Mod Exp device */ UCLASS_MTD, /* Memory Technology Device (MTD) device */

•   UCLASS_MUX,             /* Multiplexer device */
    UCLASS_NOP,             /* No-op devices */
    UCLASS_NORTHBRIDGE,     /* Intel Northbridge / SDRAM controller */
    UCLASS_NVME,            /* NVM Express device */
  

diff --git a/include/dt-bindings/mux/mux.h b/include/dt-bindings/mux/mux.h new file mode 100644 index 0000000000..042719218d --- /dev/null +++ b/include/dt-bindings/mux/mux.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • This header provides constants for most Multiplexer bindings.
• • Most Multiplexer bindings specify an idle state. In most cases, the
• • the multiplexer can be left as is when idle, and in some cases it can
• • disconnect the input/output and leave the multiplexer in a high
• • impedance state.
• */

+#ifndef _DT_BINDINGS_MUX_MUX_H +#define _DT_BINDINGS_MUX_MUX_H

+#define MUX_IDLE_AS_IS (-1) +#define MUX_IDLE_DISCONNECT (-2)

+#endif diff --git a/include/mux-internal.h b/include/mux-internal.h new file mode 100644 index 0000000000..c590bd0c74 --- /dev/null +++ b/include/mux-internal.h @@ -0,0 +1,103 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • Based on the linux multiplexer framework
• • Copyright (C) 2017 Axentia Technologies AB
• • Author: Peter Rosin peda@axentia.se
• • Copyright (C) 2017-2018 Texas Instruments Incorporated - http://www.ti.com/
• • Jean-Jacques Hiblot jjhiblot@ti.com
• */

+#ifndef _MUX_UCLASS_H +#define _MUX_UCLASS_H

MUX_INTERNAL

+/* See mux.h for background documentation. */

+#include <mux.h>

Can you drop this? I don't think it is needed.

+struct ofnode_phandle_args;

+/**

• • struct mux_chip - Represents a chip holding mux controllers.
• • @controllers: Number of mux controllers handled by the chip.
• • @mux: Array of mux controllers that are handled.
• • @dev: Device structure.
• • @ops: Mux controller operations.

Doesn't seem to match the struct.

• • This a per-device uclass-private data.
• */

+struct mux_chip {

•   unsigned int controllers;
  

•   struct mux_control *mux;
  

+};

+/**

• • struct mux_control_ops - Mux controller operations for a mux chip.
• • @set: Set the state of the given mux controller.
• */

+struct mux_control_ops {

•   /**
  

•    * set - Apply a state to a multiplexer control
  

•    *
  

•    * @mux:        A multiplexer control
  

•    * @return 0 if OK, or a negative error code.
  

•    */
  

•   int (*set)(struct mux_control *mux, int state);
  

•   /**
  

•    * of_xlate - Translate a client's device-tree (OF) multiplexer
  

•    * specifier.
  

•    *
  

•    * If this function pointer is set to NULL, the multiplexer core will
  

•    * use a default implementation, which assumes #mux-control-cells = <1>
  

•    * and that the DT cell contains a simple integer channel ID.
  

•    *
  

•    * @dev_mux:    The multiplexer device. A single device may handle
  

•    *              several multiplexer controls.
  

•    * @args:       The multiplexer specifier values from device tree.
  

•    * @mux:        (out) A multiplexer control
  

Can you rename @muxp with the 'p' indicating it is a pointer to the value? This is the convention in driver model.

•    * @return 0 if OK, or a negative error code.
  

•    */
  

•   int (*of_xlate)(struct mux_chip *dev_mux,
  

•                   struct ofnode_phandle_args *args,
  

•                   struct mux_control **mux);
  

+};

+/**

• • struct mux_control - Represents a mux controller.
• • @chip: The mux chip that is handling this mux controller.
• • @cached_state: The current mux controller state, or -1 if none.
• • @states: The number of mux controller states.
• • @idle_state: The mux controller state to use when inactive, or one

• •                 of MUX_IDLE_AS_IS and MUX_IDLE_DISCONNECT.
    

Missing comments on a few fields

• • Mux drivers may only change @states and @idle_state, and may only do so
• • between allocation and registration of the mux controller. Specifically,
• • @cached_state is internal to the mux core and should never be written by
• • mux drivers.
• */

+struct mux_control {

•   bool    in_use;
  

•   struct udevice *dev;
  

•   int cached_state;
  

•   unsigned int states;
  

•   int idle_state;
  

•   int id;
  

+};

+/**

• • mux_control_get_index() - Get the index of the given mux controller
• • @mux: The mux-control to get the index for.
• • Return: The index of the mux controller within the mux chip the mux
• • controller is a part of.
• */

+static inline unsigned int mux_control_get_index(struct mux_control *mux) +{

•   return mux->id;
  

+}

+int mux_alloc_controllers(struct udevice *dev, unsigned int controllers);

+#endif diff --git a/include/mux.h b/include/mux.h new file mode 100644 index 0000000000..060f71a47c --- /dev/null +++ b/include/mux.h @@ -0,0 +1,113 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • Based on the linux multiplexer framework

That explains the lack of comments I suppose! But we need to add them tor U-Boot.

• • At its core, a multiplexer (or mux), also known as a data selector, is a
• • device that selects between several analog or digital input signals and
• • forwards it to a single output line. This notion can be extended to work
• • with buses, like a I2C bus multiplexer for example.
• • Copyright (C) 2017 Axentia Technologies AB
• • Author: Peter Rosin peda@axentia.se
• • Copyright (C) 2017-2018 Texas Instruments Incorporated - http://www.ti.com/
• • Jean-Jacques Hiblot jjhiblot@ti.com
• */

+#ifndef _MUX_H_ +#define _MUX_H_

+#include <linux/errno.h> +#include <linux/types.h>

+struct udevice; +struct mux_control;

+#if CONFIG_IS_ENABLED(MULTIPLEXER) +/**

• • mux_control_states() - Query the number of multiplexer states.
• • @mux: The mux-control to query.
• • Return: The number of multiplexer states.
• */

+unsigned int mux_control_states(struct mux_control *mux);

+/**

• • mux_control_select() - Select the given multiplexer state.
• • @mux: The mux-control to request a change of state from.
• • @state: The new requested state.
• • On successfully selecting the mux-control state, it will be locked until
• • there is a call to mux_control_deselect(). If the mux-control is already
• • selected when mux_control_select() is called, the function will indicate
• • -EBUSY
• • Therefore, make sure to call mux_control_deselect() when the operation is
• • complete and the mux-control is free for others to use, but do not call
• • mux_control_deselect() if mux_control_select() fails.
• • Return: 0 when the mux-control state has the requested state or a negative
• • errno on error.
• */

+int __must_check mux_control_select(struct mux_control *mux,

•                               unsigned int state);
  

+#define mux_control_try_select(mux) mux_control_select(mux)

+/**

• • mux_control_deselect() - Deselect the previously selected multiplexer state.
• • @mux: The mux-control to deselect.
• • It is required that a single call is made to mux_control_deselect() for
• • each and every successful call made to either of mux_control_select() or
• • mux_control_try_select().
• • Return: 0 on success and a negative errno on error. An error can only
• • occur if the mux has an idle state. Note that even if an error occurs, the
• • mux-control is unlocked and is thus free for the next access.
• */

+int mux_control_deselect(struct mux_control *mux);

+int mux_get_by_index(struct udevice *dev, int index, struct mux_control **mux); +int mux_control_get(struct udevice *dev, const char *name,

•               struct mux_control **mux);
  

+void mux_control_put(struct mux_control *mux);

+struct mux_control *devm_mux_control_get(struct udevice *dev,

•                                    const char *mux_name);
  

Many functions missing comments here.

+#else +unsigned int mux_control_states(struct mux_control *mux) +{

•   return -ENOSYS;
  

+}

+int __must_check mux_control_select(struct mux_control *mux,

•                               unsigned int state)
  

+{

•   return -ENOSYS;
  

+}

+#define mux_control_try_select(mux) mux_control_select(mux)

+int mux_control_deselect(struct mux_control *mux) +{

•   return -ENOSYS;
  

+}

+struct mux_control *mux_control_get(struct udevice *dev, const char *mux_name) +{

•   return NULL;
  

+}

+void mux_control_put(struct mux_control *mux) +{ +}

+struct mux_control *devm_mux_control_get(struct udevice *dev,

•                                    const char *mux_name)
  

+{

•   return NULL;
  

+} +#endif

+#endif

2.17.1

Regards, Simon