[PATCH v2 1/4] drivers: Add a new framework for multiplexer devices
Simon Glass
sjg at chromium.org
Tue Dec 24 16:58:52 CET 2019
Hi Jean-Jacques,
On Tue, 5 Nov 2019 at 04:50, Jean-Jacques Hiblot <jjhiblot at 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 at 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 at 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 at axentia.se>
> + *
> + * Copyright (C) 2017-2018 Texas Instruments Incorporated - http://www.ti.com/
> + * Jean-Jacques Hiblot <jjhiblot at 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 at axentia.se>
> + *
> + * Copyright (C) 2017-2018 Texas Instruments Incorporated - http://www.ti.com/
> + * Jean-Jacques Hiblot <jjhiblot at 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 at axentia.se>
> + *
> + * Copyright (C) 2017-2018 Texas Instruments Incorporated - http://www.ti.com/
> + * Jean-Jacques Hiblot <jjhiblot at 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
More information about the U-Boot
mailing list