Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: =?UTF-8?Q?Re:_[PATCH_V2_1/2]_mux:_adgs1408:_new_driver_for_Analog_D?=
 =?UTF-8?Q?evices_ADGS1408/1409_mux_=e2=80=a6?=
To:     Mircea Caprioru <mircea.caprioru@analog.com>
Cc:     davem@davemloft.net, mchehab+samsung@kernel.org,
        akpm@linux-foundation.org, rdunlap@infradead.org,
        linux-kernel@vger.kernel.org
References: <20180717134218.5642-1-mircea.caprioru@analog.com>
From:   Peter Rosin <peda@axentia.se>
Organization: Axentia Technologies AB
Message-ID: <e2682f87-f1c9-de44-d543-6437f29ac638@axentia.se>
Date:   Wed, 18 Jul 2018 14:49:41 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <20180717134218.5642-1-mircea.caprioru@analog.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: axentia.se does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jul 2018 12:49:45.7908 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: dd379c97-84ff-4379-77d5-08d5ecacf0ea
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4ee68585-03e1-4785-942a-df9c1871a234
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0201MB2461
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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!

> - 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.

> +
> +	  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

> +/*
> + * ADG1408 SPI MUX driver

ADGS1408; you are missing the S.

And I think you could/should mention ADGS1409 here.

> + *
> + * 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.

> +};
> +
> +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.

> +
> +	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 */

> +	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".

> +	{ }
> +};
> +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.

Cheers,
Peter

> +	},
> +	.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");
>