Re: [PATCH V2 1/2] mux: adgs1408: new driver for Analog Devices ADGS1408/1409 mux …

["Caprioru, Mircea" <Mircea.Caprioru@analog.com>]

On Wed, 2018-07-18 at 14:49 +0200, Peter Rosin wrote:
> On 2018-07-17 15:42, Mircea Caprioru wrote:
> > This patch adds basic support for Analog Device ADGS1408/09 SPI mux
> > controller.
> > 
> > The device is probed and set to a disabled state. It uses the new
> > mux
> > controller framework.
> > 
> > Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
> > ---
> > Changelog V1 -> V2
> > - removed adgs140x wildcard
> > - removed cells verification since only 0 configuration supported
> 
> Ahhh, the two muxes in 1409 cannot be operated independently, as this
> is a
> "differential" mux. That simplifies things quite a bit, and good
> thing you
> caught this early!

Yeah, I understood the mux framework a bit better the second time I
looked at it (after your first review).
> 
> > - added id enum for ADGS1408 and ADGS1409
> > - sorted includes
> > 
> >  MAINTAINERS            |   5 ++
> >  drivers/mux/Kconfig    |  12 +++++
> >  drivers/mux/Makefile   |   2 +
> >  drivers/mux/adgs1408.c | 120
> > +++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 139 insertions(+)
> >  create mode 100644 drivers/mux/adgs1408.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 192d7f73fd01..458d42d6f4f3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -810,6 +810,11 @@ L:	linux-media@vger.kernel.org
> >  S:	Maintained
> >  F:	drivers/media/i2c/ad9389b*
> >  
> > +ANALOG DEVICES INC ADGS1408 DRIVER
> > +M:	Mircea Caprioru <mircea.caprioru@analog.com>
> > +S:	Supported
> > +F:	drivers/mux/adgs1408.c
> > +
> >  ANALOG DEVICES INC ADV7180 DRIVER
> >  M:	Lars-Peter Clausen <lars@metafoo.de>
> >  L:	linux-media@vger.kernel.org
> > diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> > index 6241678e99af..cf825e9f47ef 100644
> > --- a/drivers/mux/Kconfig
> > +++ b/drivers/mux/Kconfig
> > @@ -21,6 +21,18 @@ config MUX_ADG792A
> >  	  To compile the driver as a module, choose M here: the
> > module will
> >  	  be called mux-adg792a.
> >  
> > +config MUX_ADGS1408
> > +	tristate "Analog Devices ADGS1408/ADGS1409 Multiplexers"
> > +	depends on SPI
> > +	help
> > +	  ADGS1408 8:1 multiplexer and ADGS1409 double 4:1
> > multiplexer
> > +	  switches.
> > +
> > +	  The driver supports driving each switch independently.
> 
> Remove this line. It is confusing since 1408 only have one mux and
> the two muxes in the dual 1409 cannot be operated independently.

Ack.
> 
> > +
> > +	  To compile the driver as a module, choose M here: the
> > module will
> > +	  be called mux-adgs1408.
> > +
> >  config MUX_GPIO
> >  	tristate "GPIO-controlled Multiplexer"
> >  	depends on GPIOLIB || COMPILE_TEST
> > diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> > index c3d883955fd5..6e9fa47daf56 100644
> > --- a/drivers/mux/Makefile
> > +++ b/drivers/mux/Makefile
> > @@ -5,10 +5,12 @@
> >  
> >  mux-core-objs			:= core.o
> >  mux-adg792a-objs		:= adg792a.o
> > +mux-adgs1408-objs		:= adgs1408.o
> >  mux-gpio-objs			:= gpio.o
> >  mux-mmio-objs			:= mmio.o
> >  
> >  obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
> >  obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
> > +obj-$(CONFIG_MUX_ADGS1408)	+= mux-adgs1408.o
> >  obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
> >  obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
> > diff --git a/drivers/mux/adgs1408.c b/drivers/mux/adgs1408.c
> > new file mode 100644
> > index 000000000000..a5192c5e484b
> > --- /dev/null
> > +++ b/drivers/mux/adgs1408.c
> > @@ -0,0 +1,120 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> 
> The preferred spelling is GPL-2.0-or-later

Hmmm... 
This seems to be new. There are quite a few places where GPL-2.0+ is
used.
Will use GPL-2.0-or-later here.
> 
> > +/*
> > + * ADG1408 SPI MUX driver
> 
> ADGS1408; you are missing the S.
> 
> And I think you could/should mention ADGS1409 here.

Ack.
> 
> > + *
> > + * Copyright 2018 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/spi/spi.h>
> 
> This #include sorts after linux/property.h.
> 
> > +#include <linux/module.h>
> > +#include <linux/mux/driver.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/property.h>
> > +
> > +#define ADGS1408_SW_DATA       (0x01)
> > +#define ADGS1408_REG_READ(reg) ((reg) | 0x80)
> > +#define ADGS1408_DISABLE       (0x00)
> > +#define ADGS1408_MUX(state)    (((state) << 1) | 1)
> > +
> > +enum {
> > +	ADGS1408,
> > +	ADGS1409
> 
> Add a trailing comma here, to make future changes less intrusive.

Ack.
> 
> > +};
> > +
> > +static int adgs1408_spi_reg_write(struct spi_device *spi,
> > +				  u8 reg_addr, u8 reg_data)
> > +{
> > +	u8 tx_buf[2];
> > +
> > +	tx_buf[0] = reg_addr;
> > +	tx_buf[1] = reg_data;
> > +
> > +	return spi_write_then_read(spi, tx_buf, sizeof(tx_buf),
> > NULL, 0);
> > +}
> > +
> > +static int adgs1408_set(struct mux_control *mux, int state)
> > +{
> > +	struct spi_device *spi = to_spi_device(mux->chip-
> > >dev.parent);
> > +	u8 reg;
> > +
> > +	if (state == MUX_IDLE_DISCONNECT)
> > +		reg = ADGS1408_DISABLE;
> > +	else
> > +		reg = ADGS1408_MUX(state);
> > +
> > +	return adgs1408_spi_reg_write(spi, ADGS1408_SW_DATA, reg);
> > +}
> > +
> > +static const struct mux_control_ops adgs1408_ops = {
> > +	.set = adgs1408_set,
> > +};
> > +
> > +static int adgs1408_probe(struct spi_device *spi)
> > +{
> > +	const struct spi_device_id *id = spi_get_device_id(spi);
> > +	struct device *dev = &spi->dev;
> > +	struct mux_chip *mux_chip;
> > +	struct mux_control *mux;
> > +	int ret, idle_state;
> > +
> > +	mux_chip = devm_mux_chip_alloc(dev, 1, 0);
> > +	if (IS_ERR(mux_chip))
> > +		return PTR_ERR(mux_chip);
> > +
> > +	mux_chip->ops = &adgs1408_ops;
> > +
> > +	ret = adgs1408_spi_reg_write(spi, ADGS1408_SW_DATA,
> > ADGS1408_DISABLE);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = device_property_read_u32_array(dev, "idle-state",
> > +					     &idle_state,
> > +					     mux_chip-
> > >controllers);
> > +	if (ret < 0) {
> > +		idle_state = MUX_IDLE_AS_IS;
> > +	}
> 
> Drop the {} here.

Ack.
> 
> > +
> > +	mux = mux_chip->mux;
> > +
> > +	if (id->driver_data == ADGS1408)
> > +		mux->states = 8;
> > +	else
> > +		mux->states = 4;
> > +
> > +	switch (idle_state) {
> > +	case MUX_IDLE_DISCONNECT:
> > +	case MUX_IDLE_AS_IS:
> > +	case 0 ... 7:
> > +		/* adgs1409 supports only 4 states */
> > +		if (id->driver_data == ADGS1409 && idle_state > 3)
> > +			return -EINVAL;
> > +		mux->idle_state = idle_state;
> > +		break;
> 
> If you reverse the test above, or if you rewrite it like below, you
> can fall
> through and get the error message (w/o duplication), like this (since
> the
> MUX_IDLE_... defines are both negative, but you already depend on
> that):
> 
> 		/* adgs1409 supports only 4 states */
> 		if (idle_state < mux->states) {
> 			mux->idle_state = idle_state;
> 			break;
> 		}
> 		/* fall through */

Makes sense.
> 
> > +	default:
> > +		dev_err(dev, "invalid idle-state %d\n",
> > idle_state);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return devm_mux_chip_register(dev, mux_chip);
> > +}
> > +
> > +static const struct spi_device_id adgs1408_id[] = {
> > +	{ "adi,adgs1408", ADGS1408 },
> > +	{ "adi,adgs1409", ADGS1409 },
> 
> This looks weird. Other spi_device_id lookup-tables do not have dt-
> style
> ids like this; they have the equivalent of "adgs1408" and "adgs1409".

Will revert to the old form here. I am still getting the hang of this
spi_device_id stuff. 
> 
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(spi, adgs1408_id);
> > +
> > +static struct spi_driver adgs1408_driver = {
> > +	.driver = {
> > +		.name = "adgs1408",
> 
> You have (silently) dropped .of_match_table since v1. Why is it not
> needed?
> I felt much more comfortable with what was in v1 for this area of the
> patch.

Yeah, my bad about dropping this. Will revert to the old form.
> 
> Cheers,
> Peter

Thanks, 
Mircea 
> 
> > +	},
> > +	.probe = adgs1408_probe,
> > +	.id_table = adgs1408_id,
> > +};
> > +module_spi_driver(adgs1408_driver);
> > +
> > +MODULE_AUTHOR("Mircea Caprioru <mircea.caprioru@analog.com>");
> > +MODULE_DESCRIPTION("Analog Devices ADGS1408 MUX driver");>
> > +MODULE_LICENSE("GPL v2");
> > 
> 
>