Messages in this thread | | | From | "Caprioru, Mircea" <> | Subject | Re: [PATCH V2 1/2] mux: adgs1408: new driver for Analog Devices ADGS1408/1409 mux … | Date | Thu, 19 Jul 2018 07:37:57 +0000 |
| |
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"); > > > > | |