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_V4_2/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: <20180720081101.27317-1-mircea.caprioru@analog.com>
From:   Peter Rosin <peda@axentia.se>
Organization: Axentia Technologies AB
Message-ID: <e623b01d-c880-028c-bd38-4f23b3e46785@axentia.se>
Date:   Fri, 20 Jul 2018 16:29:32 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.0
MIME-Version: 1.0
In-Reply-To: <20180720081101.27317-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: 20 Jul 2018 14:29:34.7300 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 5b3a3b88-8392-49ae-1c94-08d5ee4d373c
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4ee68585-03e1-4785-942a-df9c1871a234
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0201MB2457
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-20 10:11, 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 V3 -> V4
> - named enum to adgs1408_chip_id
> - added .data with id enum values
> - added chip id verification for of_device_get_match_data
> 
>  drivers/mux/Kconfig    |  10 ++++
>  drivers/mux/Makefile   |   2 +
>  drivers/mux/adgs1408.c | 132 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 144 insertions(+)
>  create mode 100644 drivers/mux/adgs1408.c
> 
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index 6241678e99af..7659d6c5f718 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -21,6 +21,16 @@ 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.
> +
> +	  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..9cc749e10383
> --- /dev/null
> +++ b/drivers/mux/adgs1408.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * ADGS1408/ADGS1409 SPI MUX driver
> + *
> + * Copyright 2018 Analog Devices Inc.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/mux/driver.h>
> +#include <linux/of_platform.h>
> +#include <linux/property.h>
> +#include <linux/spi/spi.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_chip_id {
> +	ADGS1408 = 1,
> +	ADGS1409,
> +};
> +
> +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)
> +{
> +	struct device *dev = &spi->dev;
> +	struct mux_chip *mux_chip;
> +	struct mux_control *mux;
> +	int ret, idle_state;
> +	enum adgs1408_chip_id chip_id;

For bonus points, order this block from longer to shorter lines when you
switch idle_state to s32 (see below).

> +
> +	chip_id = (enum adgs1408_chip_id)of_device_get_match_data(dev);
> +	if (!chip_id)
> +		chip_id = spi_get_device_id(spi)->driver_data;
> +
> +	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);

I just noticed this, which is a big no-no. 'int *' is not compatible with
'u32 *' since int and u32 are of different size on 64-bit arches. You need
idle_state to be 's32', and you need the cast to avoid warnings.

And, you don't need to read an array. So, please use:

	ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);

Sorry for not noticing this earlier.

Cheers,
Peter

> +	if (ret < 0)
> +		idle_state = MUX_IDLE_AS_IS;
> +
> +	mux = mux_chip->mux;
> +
> +	if (chip_id == 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 (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_spi_id[] = {
> +	{ "adgs1408", ADGS1408 },
> +	{ "adgs1409", ADGS1409 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, adgs1408_spi_id);
> +
> +static const struct of_device_id adgs1408_of_match[] = {
> +	{ .compatible = "adi,adgs1408", .data = (void *)ADGS1408, },
> +	{ .compatible = "adi,adgs1409", .data = (void *)ADGS1409, },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, adgs1408_of_match);
> +
> +static struct spi_driver adgs1408_driver = {
> +	.driver = {
> +		.name = "adgs1408",
> +		.of_match_table = of_match_ptr(adgs1408_of_match),
> +	},
> +	.probe = adgs1408_probe,
> +	.id_table = adgs1408_spi_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");
>