Message-ID: <a5127934-863a-64d6-88e6-b8119035baf9@axentia.se>
Date:   Thu, 6 Apr 2017 10:39:28 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     <michael.hennerich@...log.com>, <wsa@...-dreams.de>,
        <robh+dt@...nel.org>, <mark.rutland@....com>,
        <linus.walleij@...aro.org>
CC:     <linux-i2c@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-gpio@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] i2c: mux: ltc4306: LTC4306 and LTC4305 I2C
 multiplexer/switch

Hi Michael,

I would still like to hear from someone with more gpio experience.

Anyway, from my point of view, there's just a few minor things left,
with comments inline as usual.

Thanks for you patience!

Cheers,
peda

On 2017-04-05 15:07, michael.hennerich@...log.com wrote:
> From: Michael Hennerich <michael.hennerich@...log.com>
> 
> This patch adds support for the Analog Devices / Linear Technology
> LTC4306 and LTC4305 4/2 Channel I2C Bus Multiplexer/Switches.
> The LTC4306 optionally provides two general purpose input/output pins
> (GPIOs) that can be configured as logic inputs, opendrain outputs or
> push-pull outputs via the generic GPIOLIB framework.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@...log.com>
> 
> ---
> 
> Changes since v1:
> 
>  - Sort makefile entries
>  - Sort driver includes
>  - Use proper defines
>  - Miscellaneous coding style fixups
>  - Rename mux select callback
>  - Revise i2c-mux-idle-disconnect handling
>  - Add ENABLE GPIO handling on error and device removal.
>  - Remove surplus of_match_device call.
> 
> Changes since v2:
> 
>  - Stop double error reporting (i2c_mux_add_adapter)
>  - Change subject
>  - Split dt bindings to separate patch
> 
> Changes since v3:
> 
>  - Change subject and add spaces
>  - Convert to I2C_MUX_LOCKED
>  - Convert to regmap
>  - Remove local register cache
>  - Restore previous ENABLE GPIO handling
>  - Initially pulse ENABLE low
>  - Eliminate i2c client struct in driver state structure
>  - Simplify error return path
>  - Misc minor cleanups
> ---
>  MAINTAINERS                         |   8 +
>  drivers/i2c/muxes/Kconfig           |  11 ++
>  drivers/i2c/muxes/Makefile          |   1 +
>  drivers/i2c/muxes/i2c-mux-ltc4306.c | 310 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 330 insertions(+)
>  create mode 100644 drivers/i2c/muxes/i2c-mux-ltc4306.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c776906..9a27a19 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7698,6 +7698,14 @@ S:	Maintained
>  F:	Documentation/hwmon/ltc4261
>  F:	drivers/hwmon/ltc4261.c
>  
> +LTC4306 I2C MULTIPLEXER DRIVER
> +M:	Michael Hennerich <michael.hennerich@...log.com>
> +W:	http://ez.analog.com/community/linux-device-drivers
> +L:	linux-i2c@...r.kernel.org
> +S:	Supported
> +F:	drivers/i2c/muxes/i2c-mux-ltc4306.c
> +F:	Documentation/devicetree/bindings/i2c/i2c-mux-ltc4306.txt
> +
>  LTP (Linux Test Project)
>  M:	Mike Frysinger <vapier@...too.org>
>  M:	Cyril Hrubis <chrubis@...e.cz>
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 10b3d17..41153b4 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -30,6 +30,17 @@ config I2C_MUX_GPIO
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mux-gpio.
>  
> +config I2C_MUX_LTC4306
> +	tristate "LTC LTC4306/5 I2C multiplexer"
> +	select GPIOLIB
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for the LTC LTC4306 or LTC4305

This reads a bit funny, and I think you should just spell out the
first LTC? But perhaps not in the tristate above though, depending
on how long it gets?

> +	  I2C mux/switch devices.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-mux-ltc4306.
> +
>  config I2C_MUX_PCA9541
>  	tristate "NXP PCA9541 I2C Master Selector"
>  	help
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index 9948fa4..ff7618c 100644
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_I2C_ARB_GPIO_CHALLENGE)	+= i2c-arb-gpio-challenge.o
>  obj-$(CONFIG_I2C_DEMUX_PINCTRL)		+= i2c-demux-pinctrl.o
>  
>  obj-$(CONFIG_I2C_MUX_GPIO)	+= i2c-mux-gpio.o
> +obj-$(CONFIG_I2C_MUX_LTC4306)	+= i2c-mux-ltc4306.o
>  obj-$(CONFIG_I2C_MUX_MLXCPLD)	+= i2c-mux-mlxcpld.o
>  obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
>  obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
> diff --git a/drivers/i2c/muxes/i2c-mux-ltc4306.c b/drivers/i2c/muxes/i2c-mux-ltc4306.c
> new file mode 100644
> index 0000000..7d34434
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-ltc4306.c
> @@ -0,0 +1,310 @@
> +/*
> + * Linear Technology LTC4306 and LTC4305 I2C multiplexer/switch
> + *
> + * Copyright (C) 2017 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + *
> + * Based on: i2c-mux-pca954x.c
> + *
> + * Datasheet: http://cds.linear.com/docs/en/datasheet/4306.pdf
> + */
> +
> +#include <linux/device.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/i2c-mux.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define LTC4305_MAX_NCHANS 2
> +#define LTC4306_MAX_NCHANS 4
> +
> +#define LTC_REG_STATUS	0x0
> +#define LTC_REG_CONFIG	0x1
> +#define LTC_REG_MODE	0x2
> +#define LTC_REG_SWITCH	0x3
> +
> +#define LTC_DOWNSTREAM_ACCL_EN	BIT(6)
> +#define LTC_UPSTREAM_ACCL_EN	BIT(7)
> +
> +#define LTC_GPIO_ALL_INPUT	0xC0
> +#define LTC_SWITCH_MASK		0xF0
> +
> +enum ltc_type {
> +	ltc_4305,
> +	ltc_4306,
> +};
> +
> +struct chip_desc {
> +	u8 nchans;
> +	u8 num_gpios;
> +};
> +
> +struct ltc4306 {
> +	struct regmap *regmap;
> +	struct gpio_chip gpiochip;
> +	const struct chip_desc *chip;
> +};
> +
> +static const struct chip_desc chips[] = {
> +	[ltc_4305] = {
> +		.nchans = LTC4305_MAX_NCHANS,
> +	},
> +	[ltc_4306] = {
> +		.nchans = LTC4306_MAX_NCHANS,
> +		.num_gpios = 2,
> +	},
> +};
> +
> +static bool ltc4306_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	return (reg == LTC_REG_CONFIG) ? true : false;
> +}
> +
> +static const struct regmap_config ltc4306_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = LTC_REG_SWITCH,
> +	.volatile_reg = ltc4306_is_volatile_reg,
> +	.cache_type = REGCACHE_RBTREE,

Did you consider REGCACHE_FLAT? There are very few registers and no hole
in the map, and maintaining a tree seems like total overkill.

> +};
> +
> +static int ltc4306_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct ltc4306 *data = gpiochip_get_data(chip);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, LTC_REG_CONFIG, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return (val & BIT(1 - offset));

The outer parentheses do not add anything, and I think they might remain
from when you just removed a double negation at some point. But is it
good practice to indicate "high" with anything other than one? Sure, the
gpiolib function that wraps the ->get() op does the !! dance for you,
but even so, every single one of the half dozen random gpio providers I
looked at had code to coerce the value to 0/1 (or error). Which makes me
think you should also have it. And the gpio_chip documentation on ->get()
agrees with me...

> +}
> +
> +static void ltc4306_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +			     int value)
> +{
> +	struct ltc4306 *data = gpiochip_get_data(chip);
> +
> +	regmap_update_bits(data->regmap, LTC_REG_CONFIG, BIT(5 - offset),
> +			   value ? BIT(5 - offset) : 0);
> +}
> +
> +static int ltc4306_gpio_direction_input(struct gpio_chip *chip,
> +					unsigned int offset)
> +{
> +	struct ltc4306 *data = gpiochip_get_data(chip);
> +
> +	return regmap_update_bits(data->regmap, LTC_REG_MODE,
> +				  BIT(7 - offset), BIT(7 - offset));
> +}
> +
> +static int ltc4306_gpio_direction_output(struct gpio_chip *chip,
> +					 unsigned int offset, int value)
> +{
> +	struct ltc4306 *data = gpiochip_get_data(chip);
> +
> +	ltc4306_gpio_set(chip, offset, value);
> +	return regmap_update_bits(data->regmap, LTC_REG_MODE,
> +				  BIT(7 - offset), 0);
> +}
> +
> +static int ltc4306_gpio_set_config(struct gpio_chip *chip,
> +				   unsigned int offset, unsigned long config)
> +{
> +	struct ltc4306 *data = gpiochip_get_data(chip);
> +	unsigned int val;
> +
> +	switch (pinconf_to_config_param(config)) {
> +	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +		val = 0;
> +		break;
> +	case PIN_CONFIG_DRIVE_PUSH_PULL:
> +		val = BIT(4 - offset);
> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	return regmap_update_bits(data->regmap, LTC_REG_MODE,
> +				  BIT(4 - offset), val);
> +}
> +
> +static int ltc4306_gpio_init(struct ltc4306 *data)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +
> +	if (!data->chip->num_gpios)
> +		return 0;
> +
> +	data->gpiochip.label = dev_name(dev);
> +	data->gpiochip.base = -1;
> +	data->gpiochip.ngpio = data->chip->num_gpios;
> +	data->gpiochip.parent = dev;
> +	data->gpiochip.can_sleep = true;
> +	data->gpiochip.direction_input = ltc4306_gpio_direction_input;
> +	data->gpiochip.direction_output = ltc4306_gpio_direction_output;

I'm missing a get_direction op?

> +	data->gpiochip.get = ltc4306_gpio_get;
> +	data->gpiochip.set = ltc4306_gpio_set;
> +	data->gpiochip.set_config = ltc4306_gpio_set_config;
> +	data->gpiochip.owner = THIS_MODULE;
> +
> +	/* gpiolib assumes all GPIOs default input */
> +	regmap_write(data->regmap, LTC_REG_MODE, LTC_GPIO_ALL_INPUT);
> +
> +	return devm_gpiochip_add_data(dev, &data->gpiochip, data);
> +}
> +
> +static int ltc4306_select_mux(struct i2c_mux_core *muxc, u32 chan)
> +{
> +	struct ltc4306 *data = i2c_mux_priv(muxc);
> +
> +	return regmap_update_bits(data->regmap, LTC_REG_SWITCH,
> +				  LTC_SWITCH_MASK, BIT(7 - chan));

Since the bits outside the mask are ignored for writes, I'd go with
regmap_write. Especially since those bits are volatile, which admittedly
will not have much impact until there is a need to read those volatile
bits outside the mask. But still.

> +}
> +
> +static int ltc4306_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
> +{
> +	struct ltc4306 *data = i2c_mux_priv(muxc);
> +
> +	return regmap_update_bits(data->regmap, LTC_REG_SWITCH,
> +				  LTC_SWITCH_MASK, 0);

Dito.

> +}
> +
> +static const struct i2c_device_id ltc4306_id[] = {
> +	{ "ltc4305", ltc_4305 },
> +	{ "ltc4306", ltc_4306 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc4306_id);
> +
> +static const struct of_device_id ltc4306_of_match[] = {
> +	{ .compatible = "lltc,ltc4305", .data = &chips[ltc_4305] },
> +	{ .compatible = "lltc,ltc4306", .data = &chips[ltc_4306] },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ltc4306_of_match);
> +
> +static int ltc4306_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
> +	struct device_node *of_node = client->dev.of_node;
> +	struct i2c_mux_core *muxc;
> +	struct ltc4306 *data;
> +	struct gpio_desc *gpio;
> +	bool idle_disc = false;
> +	int num, ret;
> +
> +	if (of_node)
> +		idle_disc = of_property_read_bool(of_node,
> +						  "i2c-mux-idle-disconnect");
> +
> +	muxc = i2c_mux_alloc(adap, &client->dev,
> +			     LTC4306_MAX_NCHANS, sizeof(*data),

Hmmm, I didn't see this before, but if you do some more rearranging, it
should be possible to replace LTC4306_MAX_NCHANS with data->chip->nchans
and reduce resource waste for ltc4305. But it's just storage for two
pointers which is really really minor... Feel free to ignore.

But you want to set a good example, right :-)

> +			     I2C_MUX_LOCKED, ltc4306_select_mux,
> +			     idle_disc ? ltc4306_deselect_mux : NULL);
> +	if (!muxc)
> +		return -ENOMEM;
> +	data = i2c_mux_priv(muxc);
> +
> +	i2c_set_clientdata(client, muxc);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &ltc4306_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		ret = PTR_ERR(data->regmap);
> +		dev_err(&client->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	/* Reset and enable the mux if an enable GPIO is specified. */
> +	gpio = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpio))
> +		return PTR_ERR(gpio);
> +
> +	if (gpio) {
> +		udelay(1);
> +		gpiod_set_value(gpio, 1);
> +	}
> +
> +	/*
> +	 * Write the mux register at addr to verify
> +	 * that the mux is in fact present. This also
> +	 * initializes the mux to disconnected state.
> +	 */
> +	if (regmap_write(data->regmap, LTC_REG_SWITCH, 0) < 0) {
> +		dev_warn(&client->dev, "probe failed\n");
> +		return -ENODEV;
> +	}
> +
> +	if (of_node) {
> +		unsigned int val = 0;
> +
> +		data->chip = of_device_get_match_data(&client->dev);
> +
> +		if (of_property_read_bool(of_node,
> +					  "ltc,downstream-accelerators-enable"))
> +			val |= LTC_DOWNSTREAM_ACCL_EN;
> +
> +		if (of_property_read_bool(of_node,
> +					  "ltc,upstream-accelerators-enable"))
> +			val |= LTC_UPSTREAM_ACCL_EN;
> +
> +		if (regmap_write(data->regmap, LTC_REG_CONFIG, val) < 0)
> +			return -ENODEV;
> +	} else {
> +		data->chip = &chips[id->driver_data];
> +	}
> +
> +	ret = ltc4306_gpio_init(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Now create an adapter for each channel */
> +	for (num = 0; num < data->chip->nchans; num++) {
> +		ret = i2c_mux_add_adapter(muxc, 0, num, 0);
> +		if (ret) {
> +			i2c_mux_del_adapters(muxc);
> +			return ret;
> +		}
> +	}
> +
> +	dev_info(&client->dev,
> +		 "registered %d multiplexed busses for I2C switch %s\n",
> +		 num, client->name);
> +
> +	return 0;
> +}
> +
> +static int ltc4306_remove(struct i2c_client *client)
> +{
> +	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> +
> +	i2c_mux_del_adapters(muxc);
> +
> +	return 0;
> +}
> +
> +static struct i2c_driver ltc4306_driver = {
> +	.driver		= {
> +		.name	= "ltc4306",
> +		.of_match_table = of_match_ptr(ltc4306_of_match),
> +	},
> +	.probe		= ltc4306_probe,
> +	.remove		= ltc4306_remove,
> +	.id_table	= ltc4306_id,
> +};
> +
> +module_i2c_driver(ltc4306_driver);
> +
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@...log.com>");
> +MODULE_DESCRIPTION("Linear Technology LTC4306, LTC4305 I2C mux/switch driver");
> +MODULE_LICENSE("GPL v2");
> 

Powered by blists - more mailing lists