Re: [PATCH v6 9/9] misc: mux-adg792a: add mux controller driver for ADG792A/G

[Peter Rosin]

On 2017-01-01 12:24, Jonathan Cameron wrote:
> On 30/11/16 08:17, Peter Rosin wrote:
>> Analog Devices ADG792A/G is a triple 4:1 mux.
>>
>> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
> Looks pretty good. Some minor suggestions inline.
> 
> This convinced me of two things:
> 1. Need a separate subsystem directory for muxes - having them under misc
> is going to lead to long term mess.
> 2. Devm alloc and registration functions will make the drivers all simpler.

Ok, I'm making the move to drivers/mux/* for v7 and adding more devm_*
functions.

> Also, browsing through ADIs list of muxes and switches it's clear that
> one classic case will be where an i2c octal or similar switch is used with
> outputs wired together in weird combinations to act as a mux.  Going to
> be 'fun' describing that.
> 
> There are also potentially cross point switches to be described ;)
> (I had to look up what one of those was ;)
> 
> Crosspoints aren't implausible as front ends for ADCs as you might
> want to be able rapidly sample any 2 of say 16 channels coming from
> for example a max14661.  We'd have to figure out how to add buffered
> capture support with sensible restrictions to the iio-mux driver
> to do that - realistically I think we would just not allow buffered
> capture with the mux having to switch.  Without hardware support
> (i.e. an ADC with external mux control) it would be too slow.
> 
> Always good to bury some idle thoughts deep in the review of a random
> driver ;) I'll never be able to remember where they were let alone
> anyone else.

But that's switches, and this is muxes. Switches are way more flexible,
so it's only natural that they are on a completely different level when
it comes to trying a generic description of them... Intentionally not
going there :-)

>> ---
>>  drivers/misc/Kconfig       |  12 ++++
>>  drivers/misc/Makefile      |   1 +
>>  drivers/misc/mux-adg792a.c | 154 +++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 167 insertions(+)
>>  create mode 100644 drivers/misc/mux-adg792a.c
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 2ce675e410c5..45567a444bbf 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -780,6 +780,18 @@ menuconfig MULTIPLEXER
>>  
>>  if MULTIPLEXER
>>  
>> +config MUX_ADG792A
>> +	tristate "Analog Devices ADG792A/ADG792G Multiplexers"
>> +	depends on I2C
>> +	help
>> +	  ADG792A and ADG792G Wide Bandwidth Triple 4:1 Multiplexers
>> +
>> +	  The driver supports both operating the three multiplexers in
>> +	  parellel and operating them independently.
> parallel
>> +
>> +	  To compile the driver as a module, choose M here: the module will
>> +	  be called mux-adg792a.
>> +
>>  config MUX_GPIO
>>  	tristate "GPIO-controlled Multiplexer"
>>  	depends on OF && GPIOLIB
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 0befa2bba762..10ab8d34c9e5 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -54,6 +54,7 @@ obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
>>  obj-$(CONFIG_CXL_BASE)		+= cxl/
>>  obj-$(CONFIG_PANEL)             += panel.o
>>  obj-$(CONFIG_MULTIPLEXER)      	+= mux-core.o
>> +obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
>>  obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
>>  
>>  lkdtm-$(CONFIG_LKDTM)		+= lkdtm_core.o
>> diff --git a/drivers/misc/mux-adg792a.c b/drivers/misc/mux-adg792a.c
>> new file mode 100644
>> index 000000000000..7d309a78af65
>> --- /dev/null
>> +++ b/drivers/misc/mux-adg792a.c
>> @@ -0,0 +1,154 @@
>> +/*
>> + * Multiplexer driver for Analog Devices ADG792A/G Triple 4:1 mux
>> + *
>> + * Copyright (C) 2016 Axentia Technologies AB
>> + *
>> + * Author: Peter Rosin <peda@xxxxxxxxxx>
>> + *
>> + * 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/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/mux.h>
>> +
>> +#define ADG792A_LDSW		BIT(0)
>> +#define ADG792A_RESET		BIT(1)
>> +#define ADG792A_DISABLE(mux)	(0x50 | (mux))
>> +#define ADG792A_DISABLE_ALL	(0x5f)
>> +#define ADG792A_MUX(mux, state)	(0xc0 | (((mux) + 1) << 2) | (state))
>> +#define ADG792A_MUX_ALL(state)	(0xc0 | (state))
>> +
>> +#define ADG792A_DISABLE_STATE	(4)
>> +#define ADG792A_KEEP_STATE	(5)
>> +
>> +static int adg792a_set(struct mux_control *mux, int state)
>> +{
>> +	struct i2c_client *i2c = to_i2c_client(mux->chip->dev.parent);
>> +	u8 cmd;
>> +
>> +	if (mux->chip->controllers == 1) {
>> +		/* parallel mux controller operation */
>> +		if (state == ADG792A_DISABLE_STATE)
>> +			cmd = ADG792A_DISABLE_ALL;
>> +		else
>> +			cmd = ADG792A_MUX_ALL(state);
>> +	} else {
>> +		unsigned int controller = mux_control_get_index(mux);
>> +
>> +		if (state == ADG792A_DISABLE_STATE)
>> +			cmd = ADG792A_DISABLE(controller);
>> +		else
>> +			cmd = ADG792A_MUX(controller, state);
>> +	}
>> +
>> +	return i2c_smbus_write_byte_data(i2c, cmd, ADG792A_LDSW);
>> +}
>> +
>> +static const struct mux_control_ops adg792a_ops = {
>> +	.set = adg792a_set,
>> +};
>> +
>> +static int adg792a_probe(struct i2c_client *i2c,
>> +			 const struct i2c_device_id *id)
>> +{
>> +	struct device *dev = &i2c->dev;
>> +	struct mux_chip *mux_chip;
>> +	bool parallel;
>> +	int ret;
>> +	int i;
>> +
>> +	parallel = of_property_read_bool(i2c->dev.of_node, "adi,parallel");
>> +
>> +	mux_chip = mux_chip_alloc(dev, parallel ? 1 : 3, 0);
> This makes me wonder if we can have a more generic binding.
> mux-poles = 3 vs mux-poles = 1?

The adg729 in theory allows to create one double pole mux and one single
pole mux (three variations, depending on which mux is single pole).
However, I did not put all that much effort into this driver. It is
mainly a proof of concept, as mentioned in the cover letter, to "prove"
that the proposed mux bindings are valid and that it is right to
have separate mux nodes in devicetree. I'm not even sure it should
be going upstream as it has seen zero testing. (But hey, it builds, what
can be wrong?)

>> +	if (!mux_chip)
>> +		return -ENOMEM;
>> +
>> +	mux_chip->ops = &adg792a_ops;
>> +	dev_set_drvdata(dev, mux_chip);
>> +
>> +	ret = i2c_smbus_write_byte_data(i2c, ADG792A_DISABLE_ALL,
>> +					ADG792A_RESET | ADG792A_LDSW);
>> +	if (ret < 0)
>> +		goto free_mux_chip;
>> +
>> +	for (i = 0; i < mux_chip->controllers; ++i) {
>> +		struct mux_control *mux = &mux_chip->mux[i];
>> +		u32 idle_state;
>> +
>> +		mux->states = 4;
>> +
>> +		ret = of_property_read_u32_index(i2c->dev.of_node,
>> +						 "adi,idle-state", i,
>> +						 &idle_state);
>> +		if (ret >= 0) {
>> +			if (idle_state > ADG792A_KEEP_STATE) {
>> +				dev_err(dev, "invalid idle-state %u\n",
>> +					idle_state);
>> +				ret = -EINVAL;
>> +				goto free_mux_chip;
>> +			}
>> +			if (idle_state != ADG792A_KEEP_STATE)
>> +				mux->idle_state = idle_state;
>> +		}
>> +	}
>> +
>> +	ret = mux_chip_register(mux_chip);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to register mux-chip\n");
>> +		goto free_mux_chip;
>> +	}
>> +
>> +	if (parallel)
>> +		dev_info(dev, "1 triple 4-way mux-controller registered\n");
> I'd use the relay / switch standard description for this so 
> 'triple pole, quadruple throw mux registered'.
>> +	else
>> +		dev_info(dev, "3 4-way mux-controllers registered\n");
> '3x single pole, quadruple throw muxes registered'.

Ok, fine by me.

>> +
>> +	return 0;
>> +
>> +free_mux_chip:
>> +	mux_chip_free(mux_chip);
>> +	return ret;
>> +}
>> +
>> +static int adg792a_remove(struct i2c_client *i2c)
>> +{
>> +	struct mux_chip *mux_chip = dev_get_drvdata(&i2c->dev);
>> +
> Definitely looking like it's worth managed versions of mux_chip_register and
> mux_chip_alloc given this is another case where they would let us get rid
> of the remove function entirely.
>> +	mux_chip_unregister(mux_chip);
>> +	mux_chip_free(mux_chip);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id adg792a_id[] = {
>> +	{ .name = "adg792a", },
>> +	{ .name = "adg792g", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, adg792a_id);
>> +
>> +static const struct of_device_id adg792a_of_match[] = {
>> +	{ .compatible = "adi,adg792a", },
>> +	{ .compatible = "adi,adg792g", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, adg792a_of_match);
>> +
>> +static struct i2c_driver adg792a_driver = {
>> +	.driver		= {
>> +		.name		= "adg792a",
>> +		.of_match_table = of_match_ptr(adg792a_of_match),
>> +	},
>> +	.probe		= adg792a_probe,
>> +	.remove		= adg792a_remove,
>> +	.id_table	= adg792a_id,
>> +};
>> +module_i2c_driver(adg792a_driver);
>> +
>> +MODULE_DESCRIPTION("Analog Devices ADG792A/G Triple 4:1 mux driver");
>> +MODULE_AUTHOR("Peter Rosin <peda@xxxxxxxxxx");
>> +MODULE_LICENSE("GPL v2");
>>
>