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_1/2]_mux:adgs1408/1409:_New_driver_for_Analog_?=
 =?UTF-8?Q?Devices_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: <20180713122738.5888-1-mircea.caprioru@analog.com>
From:   Peter Rosin <peda@axentia.se>
Organization: Axentia Technologies AB
Message-ID: <b3540119-73e1-38d5-1dc9-ce9f8bdad9e8@axentia.se>
Date:   Sat, 14 Jul 2018 14:04:08 +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: <20180713122738.5888-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: 14 Jul 2018 12:04:12.5503 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 1f16adce-95fa-4244-807c-08d5e981ea92
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4ee68585-03e1-4785-942a-df9c1871a234
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0201MB2460
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi!

Thanks for your patches!

Please add a space after "mux:" in the subject. And the second part should
generally be the driver name and the text should not start with a capital
letter. So, I'd like the subject to be:

[...] mux: adgs1408: new driver for Analog Devices ADGS1408/1409 mux

On 2018-07-13 14:27, 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>
> ---
>  MAINTAINERS            |   7 +++
>  drivers/mux/Kconfig    |  12 ++++
>  drivers/mux/Makefile   |   2 +
>  drivers/mux/adgs140x.c | 132 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 153 insertions(+)
>  create mode 100644 drivers/mux/adgs140x.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 192d7f73fd01..7aa68f38ea4b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -810,6 +810,13 @@ 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>
> +W:	http://wiki.analog.com/
> +W:	http://ez.analog.com/community/linux-device-drivers
> +S:	Supported
> +F:	drivers/mux/adgs140x.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..87b3fda56d8f 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -48,4 +48,16 @@ config MUX_MMIO
>  	  To compile the driver as a module, choose M here: the module will
>  	  be called mux-mmio.
>  
> +config MUX_ADGS140X

Please name the driver after one of the chips, i.e. without wildcard.
And keep the Kconfig entries sorted.

> +	tristate "Analog Devices ADGS1408/ADGS1409 Multiplexers"
> +	depends on SPI
> +	help
> +	  ADGS1408 and ADGS1409 4 independent single-pole/single-throw
> +	  switches.

That's wrong. ADGS1408 has one SP8T, ADGS1409 has two SPQT (or one DPQT).

> +
> +	  The driver suports driving each switch independently.

supports

> +
> +	  To compile the driver as a module, choose M here: the module will
> +	  be called mux-ads140x.

mux-adgs1408

> +
>  endmenu
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index c3d883955fd5..236e7738462a 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -5,10 +5,12 @@
>  
>  mux-core-objs			:= core.o
>  mux-adg792a-objs		:= adg792a.o
> +mux-adgs140x-objs		:= adgs140x.o

s/adgs140x/adgs1408/

Make this change throughout, of course. And this comment also applies
to the bindings patch.

>  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_ADGS140X)	+= mux-adgs140x.o
>  obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
>  obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
> diff --git a/drivers/mux/adgs140x.c b/drivers/mux/adgs140x.c
> new file mode 100644
> index 000000000000..13dd95acca20
> --- /dev/null
> +++ b/drivers/mux/adgs140x.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * ADG1408 SPI MUX driver
> + *
> + * Copyright 2018 Analog Devices Inc.
> + *

Remove the above empty line.

> + */
> +
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/mux/driver.h>
> +#include <linux/spi/spi.h>
> +#include <linux/property.h>

Keep the #includes sorted.

> +
> +#define ADGS1408_SW_DATA       (0x01)
> +#define ADGS1408_REG_READ(reg) ((reg) | 0x80)
> +#define ADGS1408_DISABLE       (0x00)
> +#define ADGS1408_MUX(state)    (((state) << 1) | 1)
> +
> +static int adgs140x_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);

	return spi_write(spi, tx_buf, sizeof(tx_buf));

> +}
> +
> +static int adgs140x_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 adgs140x_spi_reg_write(spi, ADGS1408_SW_DATA, reg);
> +}
> +
> +static const struct mux_control_ops adgs140x_ops = {
> +	.set = adgs140x_set,
> +};
> +
> +static int adgs140x_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct mux_chip *mux_chip;
> +	u32 idle_state[2];
> +	u32 cells;
> +	int ret;
> +	int i;
> +
> +	ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (cells >= 2)
> +		return -EINVAL;

cells should always be zero for ADGS1408.

> +
> +	mux_chip = devm_mux_chip_alloc(dev, cells ? 2 : 1, 0);
> +	if (IS_ERR(mux_chip))
> +		return PTR_ERR(mux_chip);
> +
> +	mux_chip->ops = &adgs140x_ops;
> +
> +	ret = adgs140x_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[0] = MUX_IDLE_AS_IS;
> +		idle_state[1] = MUX_IDLE_AS_IS;
> +	}
> +
> +	for (i = 0; i < mux_chip->controllers; ++i) {
> +		struct mux_control *mux = &mux_chip->mux[i];
> +
> +		if (mux_chip->controllers == 1)
> +			mux->states = 8;

8 states is never right for ADGS1409.

> +		else
> +			mux->states = 4;

4 states is never right for ADGS1408.

> +
> +		switch (idle_state[i]) {
> +		case MUX_IDLE_DISCONNECT:
> +		case MUX_IDLE_AS_IS:
> +		case 0 ... 8:

Hmmm, I realize that you have adapted this 0 ... 8 from the
0 ... 4 in the adg795a driver, but now I fail to see why it
isn't 0 ... 3 there. I think the 4 is a left-over from an
older encoding of the special idle states.

So, for ADGS1408, 0 ... 7 are valid, and for ADGS1409 it's 0 ... 3.

Cheers,
Peter

> +			mux->idle_state = idle_state[i];
> +			break;
> +		default:
> +			dev_err(dev, "invalid idle-state %d\n", idle_state[i]);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return devm_mux_chip_register(dev, mux_chip);
> +}
> +
> +static const struct spi_device_id adgs140x_id[] = {
> +	{ .name = "adgs1408", },
> +	{ .name = "adgs1409", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, adgs140x_id);
> +
> +static const struct of_device_id adgs140x_of_match[] = {
> +	{ .compatible = "adi,adgs1408", },
> +	{ .compatible = "adi,adgs1409", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, adgs140x_of_match);
> +
> +static struct spi_driver adgs140x_driver = {
> +	.driver = {
> +		.name = "adgs1408",
> +		.of_match_table = of_match_ptr(adgs140x_of_match),
> +	},
> +	.probe = adgs140x_probe,
> +	.id_table = adgs140x_id,
> +};
> +module_spi_driver(adgs140x_driver);
> +
> +MODULE_AUTHOR("Mircea Caprioru <mircea.caprioru@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices ADGS140x MUX driver");
> +MODULE_LICENSE("GPL v2");
>