diff mbox series

[v4,2/2] spi: Add generic SPI multiplexer

Message ID 20200131023433.12133-3-chris.packham@alliedtelesis.co.nz (mailing list archive)
State Superseded
Headers show
Series SPI bus multiplexing | expand

Commit Message

Chris Packham Jan. 31, 2020, 2:34 a.m. UTC
Add a SPI device driver that sits in-band and provides a SPI controller
which supports chip selects via a mux-control. This enables extra SPI
devices to be connected with limited native chip selects.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v4:
    - incorporate review feedback from Andy

 drivers/spi/Kconfig   |  12 +++
 drivers/spi/Makefile  |   1 +
 drivers/spi/spi-mux.c | 189 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 202 insertions(+)
 create mode 100644 drivers/spi/spi-mux.c

Comments

Andy Shevchenko Feb. 3, 2020, 9:50 a.m. UTC | #1
On Fri, Jan 31, 2020 at 4:34 AM Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:
>
> Add a SPI device driver that sits in-band and provides a SPI controller
> which supports chip selects via a mux-control. This enables extra SPI
> devices to be connected with limited native chip selects.

Thanks for an update, my comments below.

...

>  #
>  # Add new SPI master controllers in alphabetical order above this line
>  #

> +#

Redundant line.

...

> +/*
> + * General Purpose SPI multiplexer
> + */

I think Mark wants to have this in one line with C++ style of comments.

...

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/mux/consumer.h>

Perhaps sorted?

...

> +       /* do the transfer */
> +       ret = spi_async(priv->spi, m);
> +       return ret;

return spi_async(...);

...


> +       priv->mux = devm_mux_control_get(&spi->dev, NULL);

> +       ret = PTR_ERR_OR_ZERO(priv->mux);

This is a bit complicated.

> +       if (ret) {

Why not simple do

  if (IS_ERR(priv->mux)) {
    ret = PTR_ERR(...);
    ...
  }

?

> +               if (ret != -EPROBE_DEFER)
> +                       dev_err(&spi->dev, "failed to get control-mux\n");
> +               goto err_put_ctlr;
> +       }

> +       ctlr->dev.of_node = spi->dev.of_node;

I'm wondering why SPI core can't handle this by default (like GPIO
subsystem does).

> +       ret = devm_spi_register_controller(&spi->dev, ctlr);
> +       if (ret)
> +               goto err_put_ctlr;
> +

> +       return ret;

return 0;

...

> +static const struct of_device_id spi_mux_of_match[] = {
> +       { .compatible = "spi-mux" },
> +       { },

Comma is not needed in terminator line.

> +};
Chris Packham Feb. 3, 2020, 9:11 p.m. UTC | #2
Hi Andy,

On Mon, 2020-02-03 at 11:50 +0200, Andy Shevchenko wrote:

Other comments all accepted and will be addressed in v5.

> 
> ...
> 
> 
> > +       priv->mux = devm_mux_control_get(&spi->dev, NULL);
> > +       ret = PTR_ERR_OR_ZERO(priv->mux);
> 
> This is a bit complicated.
> 
> > +       if (ret) {
> 
> Why not simple do
> 
>   if (IS_ERR(priv->mux)) {
>     ret = PTR_ERR(...);
>     ...
>   }
> 
> ?

I've had other maintainers/reviewers suggest the opposite for patches
I've submitted to other subsystems which is why I went with
PTR_ERR_OR_ZERO. It also works well with the goto err_put_ctlr; which
needs ret to be set. It's not exactly a common pattern in the spi code
so I'd be happy to go the other way if that's the desired convention
for spi.

> 
> > +               if (ret != -EPROBE_DEFER)
> > +                       dev_err(&spi->dev, "failed to get control-mux\n");
> > +               goto err_put_ctlr;
> > +       }
> > +       ctlr->dev.of_node = spi->dev.of_node;
> 
> I'm wondering why SPI core can't handle this by default (like GPIO
> subsystem does).
> 

We do have it for spi devices in of_register_spi_device(). I'm not
aware of any reason spi_register_controller() couldn't do the same for
controllers. It would affect a large number of drivers so should
probably be separate to this series. It's probably reasonably
automatable with coccinelle.
Andy Shevchenko Feb. 4, 2020, 11:07 a.m. UTC | #3
On Mon, Feb 3, 2020 at 11:12 PM Chris Packham
<Chris.Packham@alliedtelesis.co.nz> wrote:
> On Mon, 2020-02-03 at 11:50 +0200, Andy Shevchenko wrote:

...

> > > +       priv->mux = devm_mux_control_get(&spi->dev, NULL);
> > > +       ret = PTR_ERR_OR_ZERO(priv->mux);
> >
> > This is a bit complicated.
> >
> > > +       if (ret) {
> >
> > Why not simple do
> >
> >   if (IS_ERR(priv->mux)) {
> >     ret = PTR_ERR(...);
> >     ...
> >   }
> >
> > ?
>
> I've had other maintainers/reviewers suggest the opposite for patches
> I've submitted to other subsystems which is why I went with
> PTR_ERR_OR_ZERO. It also works well with the goto err_put_ctlr; which
> needs ret to be set. It's not exactly a common pattern in the spi code
> so I'd be happy to go the other way if that's the desired convention
> for spi.

Either way is the same amount of lines. The slight difference, that we
don't check for 0. Can you check if generated code is different in
these cases?
diff mbox series

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 870f7797b56b..90df945490d9 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -880,6 +880,18 @@  config SPI_ZYNQMP_GQSPI
 #
 # Add new SPI master controllers in alphabetical order above this line
 #
+#
+
+comment "SPI Multiplexer support"
+
+config SPI_MUX
+	tristate "SPI multiplexer support"
+	select MULTIPLEXER
+	help
+	  This adds support for SPI multiplexers. Each SPI mux will be
+	  accessible as a SPI controller, the devices behind the mux will appear
+	  to be chip selects on this controller. It is still necessary to
+	  select one or more specific mux-controller drivers.
 
 #
 # There are lots of SPI device types, with sensors and memory
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index bb49c9e6d0a0..116409533727 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -9,6 +9,7 @@  ccflags-$(CONFIG_SPI_DEBUG) := -DDEBUG
 # config declarations into driver model code
 obj-$(CONFIG_SPI_MASTER)		+= spi.o
 obj-$(CONFIG_SPI_MEM)			+= spi-mem.o
+obj-$(CONFIG_SPI_MUX)			+= spi-mux.o
 obj-$(CONFIG_SPI_SPIDEV)		+= spidev.o
 obj-$(CONFIG_SPI_LOOPBACK_TEST)		+= spi-loopback-test.o
 
diff --git a/drivers/spi/spi-mux.c b/drivers/spi/spi-mux.c
new file mode 100644
index 000000000000..86708644fd2f
--- /dev/null
+++ b/drivers/spi/spi-mux.c
@@ -0,0 +1,189 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * General Purpose SPI multiplexer
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/mux/consumer.h>
+
+#define SPI_MUX_NO_CS	((unsigned int)-1)
+
+/**
+ * DOC: Driver description
+ *
+ * This driver supports a MUX on an SPI bus. This can be useful when you need
+ * more chip selects than the hardware peripherals support, or than are
+ * available in a particular board setup.
+ *
+ * The driver will create an additional SPI controller. Devices added under the
+ * mux will be handled as 'chip selects' on this controller.
+ */
+
+/**
+ * struct spi_mux_priv - the basic spi_mux structure
+ * @spi:		pointer to the device struct attached to the parent
+ *			spi controller
+ * @current_cs:		The current chip select set in the mux
+ * @child_msg_complete: The mux replaces the complete callback in the child's
+ *			message to its own callback; this field is used by the
+ *			driver to store the child's callback during a transfer
+ * @child_msg_context:	Used to store the child's context to the callback
+ * @child_msg_dev:	Used to store the spi_device pointer to the child
+ * @mux:		mux_control structure used to provide chip selects for
+ *			downstream spi devices
+ */
+struct spi_mux_priv {
+	struct spi_device	*spi;
+	unsigned int		current_cs;
+
+	void			(*child_msg_complete)(void *context);
+	void			*child_msg_context;
+	struct spi_device	*child_msg_dev;
+	struct mux_control	*mux;
+};
+
+/* should not get called when the parent controller is doing a transfer */
+static int spi_mux_select(struct spi_device *spi)
+{
+	struct spi_mux_priv *priv = spi_controller_get_devdata(spi->controller);
+	int ret;
+
+	if (priv->current_cs == spi->chip_select)
+		return 0;
+
+	dev_dbg(&priv->spi->dev, "setting up the mux for cs %d\n",
+		spi->chip_select);
+
+	/* copy the child device's settings except for the cs */
+	priv->spi->max_speed_hz = spi->max_speed_hz;
+	priv->spi->mode = spi->mode;
+	priv->spi->bits_per_word = spi->bits_per_word;
+
+	ret = mux_control_select(priv->mux, spi->chip_select);
+	if (ret)
+		return ret;
+
+	priv->current_cs = spi->chip_select;
+
+	return 0;
+}
+
+static int spi_mux_setup(struct spi_device *spi)
+{
+	struct spi_mux_priv *priv = spi_controller_get_devdata(spi->controller);
+
+	/*
+	 * can be called multiple times, won't do a valid setup now but we will
+	 * change the settings when we do a transfer (necessary because we
+	 * can't predict from which device it will be anyway)
+	 */
+	return spi_setup(priv->spi);
+}
+
+static void spi_mux_complete_cb(void *context)
+{
+	struct spi_mux_priv *priv = (struct spi_mux_priv *)context;
+	struct spi_controller *ctlr = spi_get_drvdata(priv->spi);
+	struct spi_message *m = ctlr->cur_msg;
+
+	m->complete = priv->child_msg_complete;
+	m->context = priv->child_msg_context;
+	m->spi = priv->child_msg_dev;
+	spi_finalize_current_message(ctlr);
+	mux_control_deselect(priv->mux);
+}
+
+static int spi_mux_transfer_one_message(struct spi_controller *ctlr,
+						struct spi_message *m)
+{
+	struct spi_mux_priv *priv = spi_controller_get_devdata(ctlr);
+	struct spi_device *spi = m->spi;
+	int ret;
+
+	ret = spi_mux_select(spi);
+	if (ret)
+		return ret;
+
+	/*
+	 * Replace the complete callback, context and spi_device with our own
+	 * pointers. Save originals
+	 */
+	priv->child_msg_complete = m->complete;
+	priv->child_msg_context = m->context;
+	priv->child_msg_dev = m->spi;
+
+	m->complete = spi_mux_complete_cb;
+	m->context = priv;
+	m->spi = priv->spi;
+
+	/* do the transfer */
+	ret = spi_async(priv->spi, m);
+	return ret;
+}
+
+static int spi_mux_probe(struct spi_device *spi)
+{
+	struct spi_controller *ctlr;
+	struct spi_mux_priv *priv;
+	int ret;
+
+	ctlr = spi_alloc_master(&spi->dev, sizeof(*priv));
+	if (!ctlr)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, ctlr);
+	priv = spi_controller_get_devdata(ctlr);
+	priv->spi = spi;
+
+	priv->mux = devm_mux_control_get(&spi->dev, NULL);
+	ret = PTR_ERR_OR_ZERO(priv->mux);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&spi->dev, "failed to get control-mux\n");
+		goto err_put_ctlr;
+	}
+
+	priv->current_cs = SPI_MUX_NO_CS;
+
+	/* supported modes are the same as our parent's */
+	ctlr->mode_bits = spi->controller->mode_bits;
+	ctlr->flags = spi->controller->flags;
+	ctlr->transfer_one_message = spi_mux_transfer_one_message;
+	ctlr->setup = spi_mux_setup;
+	ctlr->num_chipselect = mux_control_states(priv->mux);
+	ctlr->bus_num = -1;
+	ctlr->dev.of_node = spi->dev.of_node;
+
+	ret = devm_spi_register_controller(&spi->dev, ctlr);
+	if (ret)
+		goto err_put_ctlr;
+
+	return ret;
+
+err_put_ctlr:
+	spi_controller_put(ctlr);
+
+	return ret;
+}
+
+static const struct of_device_id spi_mux_of_match[] = {
+	{ .compatible = "spi-mux" },
+	{ },
+};
+
+static struct spi_driver spi_mux_driver = {
+	.probe  = spi_mux_probe,
+	.driver = {
+		.name   = "spi-mux",
+		.of_match_table = spi_mux_of_match,
+	},
+};
+
+module_spi_driver(spi_mux_driver);
+
+MODULE_DESCRIPTION("SPI multiplexer");
+MODULE_LICENSE("GPL");