lkml.org 
[lkml]   [2017]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4 2/2] i2c: mux: ltc4306: LTC4306 and LTC4305 I2C multiplexer/switch
    From
    Date
    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@analog.com wrote:
    > From: Michael Hennerich <michael.hennerich@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.hennerich@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.hennerich@analog.com>
    > +W: http://ez.analog.com/community/linux-device-drivers
    > +L: linux-i2c@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 <vapier@gentoo.org>
    > M: Cyril Hrubis <chrubis@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, &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@analog.com>");
    > +MODULE_DESCRIPTION("Linear Technology LTC4306, LTC4305 I2C mux/switch driver");
    > +MODULE_LICENSE("GPL v2");
    >

    \
     
     \ /
      Last update: 2017-04-06 11:38    [W:3.332 / U:0.136 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site