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.henner...@analog.com wrote: > From: Michael Hennerich <michael.henner...@analog.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.henner...@analog.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.henner...@analog.com> > +W: http://ez.analog.com/community/linux-device-drivers > +L: linux-...@vger.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 <vap...@gentoo.org> > M: Cyril Hrubis <chru...@suse.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, <c4306_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.henner...@analog.com>"); > +MODULE_DESCRIPTION("Linear Technology LTC4306, LTC4305 I2C mux/switch > driver"); > +MODULE_LICENSE("GPL v2"); >
