| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5e4b224d-4dba-e183-4eda-d68f57faff7e@axentia.se>
Date: Fri, 24 Mar 2017 09:01:08 +0100
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] i2c/muxes/i2c-mux-ltc4306: LTC4306 and LTC4305 I2C
multiplexer/switch
On 2017-03-23 15:22, 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.
Hmmm, I'm not sure if it is ok to implement a gpio provider outside of
drivers/gpio like this? Should it be done with MFD, or is that just
adding needless complexity?
I also wonder if you have thought about implementing support for
alerts? And do you need recovery if things get stuck? I see timeout
bits and failure to connect bits in the register map...
>
> Signed-off-by: Michael Hennerich <michael.hennerich@...log.com>
> ---
> .../devicetree/bindings/i2c/i2c-mux-ltc4306.txt | 61 ++++
> MAINTAINERS | 8 +
> drivers/i2c/muxes/Kconfig | 10 +
> drivers/i2c/muxes/Makefile | 1 +
> drivers/i2c/muxes/i2c-mux-ltc4306.c | 365 +++++++++++++++++++++
> 5 files changed, 445 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-ltc4306.txt
> create mode 100644 drivers/i2c/muxes/i2c-mux-ltc4306.c
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-ltc4306.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-ltc4306.txt
> new file mode 100644
> index 0000000..1e98c6b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-ltc4306.txt
> @@ -0,0 +1,61 @@
> +* Linear Technology / Analog Devices I2C bus switch
> +
> +Required Properties:
> +
> + - compatible: Must contain one of the following.
> + "lltc,ltc4305", "lltc,ltc4306"
> + - reg: The I2C address of the device.
> +
> + The following required properties are defined externally:
> +
> + - Standard I2C mux properties. See i2c-mux.txt in this directory.
> + - I2C child bus nodes. See i2c-mux.txt in this directory.
> +
> +Optional Properties:
> +
> + - enable-gpios: Reference to the GPIO connected to the enable input.
> + - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
> + children in idle state. This is necessary for example, if there are several
> + multiplexers on the bus and the devices behind them use same I2C addresses.
> + - gpio-controller: Marks the device node as a GPIO Controller.
> + - #gpio-cells: Should be two. The first cell is the pin number and
> + the second cell is used to specify flags.
> + See ../gpio/gpio.txt for more information.
> + - ltc,downstream-accelerators-enable: Enables the rise time accelerators
> + on the downstream port.
> + - ltc,upstream-accelerators-enable: Enables the rise time accelerators
> + on the upstream port.
> +
> +Example:
> +
> + ltc4306: i2c-mux@4a {
> + compatible = "lltc,ltc4306";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x4a>;
> +
> + gpio-controller;
> + #gpio-cells = <2>;
> +
> + i2c@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> +
> + eeprom@50 {
> + compatible = "at,24c02";
> + reg = <0x50>;
> + };
> + };
> +
> + i2c@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> +
> + eeprom@50 {
> + compatible = "at,24c02";
> + reg = <0x50>;
> + };
> + };
> + };
> 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..f501b3b 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -30,6 +30,16 @@ 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
> + help
> + If you say yes here you get support for the LTC LTC4306 or LTC4305
> + 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..a452d09 100644
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_I2C_DEMUX_PINCTRL) += i2c-demux-pinctrl.o
>
> obj-$(CONFIG_I2C_MUX_GPIO) += i2c-mux-gpio.o
> obj-$(CONFIG_I2C_MUX_MLXCPLD) += i2c-mux-mlxcpld.o
> +obj-$(CONFIG_I2C_MUX_LTC4306) += i2c-mux-ltc4306.o
Keep the list sorted, please.
> obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o
> obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o
> obj-$(CONFIG_I2C_MUX_PINCTRL) += i2c-mux-pinctrl.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..f0fd4d1
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-ltc4306.c
> @@ -0,0 +1,365 @@
> +/*
> + * 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/i2c.h>
> +#include <linux/i2c-mux.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/consumer.h>
Sort the includes, please.
> +
> +#define LTC4306_MAX_NCHANS 4
Why not a define for LTC4305_MAX_NCHANS as well?
> +
> +#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
> +
> +enum ltc_type {
> + ltc_4305,
> + ltc_4306,
> +};
> +
> +struct chip_desc {
> + u8 nchans;
> + u8 num_gpios;
> +};
> +
> +struct ltc4306 {
> + struct i2c_client *client;
> + struct gpio_chip gpiochip;
> + const struct chip_desc *chip;
> +
> + u8 deselect;
> + u8 regs[LTC_REG_SWITCH + 1];
Feels like a generic register cache, can you use regmap?
> +};
> +
> +/* Provide specs for the PCA954x types we know about */
> +static const struct chip_desc chips[] = {
> + [ltc_4305] = {
> + .nchans = 2,
> + },
> + [ltc_4306] = {
> + .nchans = LTC4306_MAX_NCHANS,
> + .num_gpios = 2,
> + },
> +};
> +
> +static int ltc4306_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct ltc4306 *data = gpiochip_get_data(chip);
> + int ret = 0;
> +
> + if (gpiochip_line_is_open_drain(chip, offset) ||
> + (data->regs[LTC_REG_MODE] & BIT(7 - offset))) {
Alignment should match open parenthesis.
> + /* Open Drain or Input */
> + ret = i2c_smbus_read_byte_data(data->client, LTC_REG_CONFIG);
> + if (ret < 0)
> + return ret;
> +
> + return !!(ret & BIT(1 - offset));
> + } else {
> + return !!(data->regs[LTC_REG_CONFIG] & BIT(5 - offset));
> + }
> +}
> +
> +static void ltc4306_gpio_set(struct gpio_chip *chip, unsigned int offset,
> + int value)
> +{
> + struct ltc4306 *data = gpiochip_get_data(chip);
> +
> + if (value)
> + data->regs[LTC_REG_CONFIG] |= BIT(5 - offset);
> + else
> + data->regs[LTC_REG_CONFIG] &= ~BIT(5 - offset);
> +
> +
Please don't use multiple blank lines.
> + i2c_smbus_write_byte_data(data->client, LTC_REG_CONFIG,
> + data->regs[LTC_REG_CONFIG]);
Alignment should match open parenthesis.
> +}
> +
> +static int ltc4306_gpio_direction_input(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + struct ltc4306 *data = gpiochip_get_data(chip);
> +
> + data->regs[LTC_REG_MODE] |= BIT(7 - offset);
> +
> + return i2c_smbus_write_byte_data(data->client, LTC_REG_MODE,
> + data->regs[LTC_REG_MODE]);
> +}
> +
> +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);
> + data->regs[LTC_REG_MODE] &= ~BIT(7 - offset);
> +
> + return i2c_smbus_write_byte_data(data->client, LTC_REG_MODE,
> + data->regs[LTC_REG_MODE]);
> +}
> +
> +static int ltc4306_gpio_set_config(struct gpio_chip *chip,
> + unsigned int offset, unsigned long config)
> +{
> + struct ltc4306 *data = gpiochip_get_data(chip);
> +
> + switch (pinconf_to_config_param(config)) {
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + data->regs[LTC_REG_MODE] &= ~BIT(4 - offset);
> + break;
> + case PIN_CONFIG_DRIVE_PUSH_PULL:
> + data->regs[LTC_REG_MODE] |= BIT(4 - offset);
> + break;
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return i2c_smbus_write_byte_data(data->client, LTC_REG_MODE,
> + data->regs[LTC_REG_MODE]);
> +}
> +
> +static int ltc4306_gpio_init(struct ltc4306 *data)
> +{
> + if (!data->chip->num_gpios)
> + return 0;
> +
> + data->gpiochip.label = dev_name(&data->client->dev);
> + data->gpiochip.base = -1;
> + data->gpiochip.ngpio = data->chip->num_gpios;
> + data->gpiochip.parent = &data->client->dev;
> + data->gpiochip.can_sleep = true;
> + data->gpiochip.direction_input = ltc4306_gpio_direction_input;
> + data->gpiochip.direction_output = ltc4306_gpio_direction_output;
> + 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 */
> + data->regs[LTC_REG_MODE] |= LTC_GPIO_ALL_INPUT;
> + i2c_smbus_write_byte_data(data->client, LTC_REG_MODE,
> + data->regs[LTC_REG_MODE]);
Alignment should match open parenthesis.
> +
> + return devm_gpiochip_add_data(&data->client->dev,
> + &data->gpiochip, data);
> +}
> +
> +/*
> + * Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
> + * as they will try to lock the adapter a second time.
> + */
> +static int ltc4306_reg_write(struct i2c_adapter *adap,
> + struct i2c_client *client, u8 reg, u8 val)
> +{
> + int ret;
> +
> + if (adap->algo->master_xfer) {
> + struct i2c_msg msg;
> + char buf[2];
> +
> + msg.addr = client->addr;
> + msg.flags = 0;
> + msg.len = 2;
> + buf[0] = reg;
> + buf[1] = val;
> + msg.buf = buf;
> + ret = __i2c_transfer(adap, &msg, 1);
> + } else {
> + union i2c_smbus_data data;
> +
> + data.byte = val;
> + ret = adap->algo->smbus_xfer(adap, client->addr,
> + client->flags,
> + I2C_SMBUS_WRITE,
> + reg,
> + I2C_SMBUS_BYTE_DATA, &data);
> + }
> +
> + return ret;
> +}
You have selected to go parent-locked, but you can get rid of the above
workaround if you go mux-locked. See Documentation/i2c/i2c-topology for
how these differ. Overall, since you do have the GPIO possibility, I think
you are closing some possibilities by going parent-locked. Those GPIOs
will more easily get locked out if you build a complex tree involving
these muxes, when the mux is parent-locked.
But on the other hand, mux-locked doesn't work everywhere either, the person
who needs mux-locked can add that option if you don't want to...
> +static int ltc4306_select_chan(struct i2c_mux_core *muxc, u32 chan)
> +{
> + struct ltc4306 *data = i2c_mux_priv(muxc);
> + struct i2c_client *client = data->client;
> + u8 regval;
> + int ret = 0;
> +
> + regval = BIT(7 - chan);
> +
> + /* Only select the channel if its different from the last channel */
> + if (data->regs[LTC_REG_SWITCH] != regval) {
> + ret = ltc4306_reg_write(muxc->parent, client,
> + LTC_REG_SWITCH, regval);
> + data->regs[LTC_REG_SWITCH] = ret < 0 ? 0 : regval;
> + }
> +
> + return ret;
> +}
> +
> +static int ltc4306_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
> +{
> + struct ltc4306 *data = i2c_mux_priv(muxc);
> + struct i2c_client *client = data->client;
> +
> + if (!(data->deselect & BIT(chan)))
> + return 0;
> +
> + /* Deselect active channel */
> + data->regs[LTC_REG_SWITCH] = 0;
> +
> + return ltc4306_reg_write(muxc->parent, client,
> + LTC_REG_SWITCH, data->regs[LTC_REG_SWITCH]);
> +}
> +
> +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;
> + bool idle_disconnect_dt = false;
> + struct gpio_desc *gpio;
> + struct i2c_mux_core *muxc;
> + struct ltc4306 *data;
> + const struct of_device_id *match;
> + int num, ret;
> +
> + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
> + return -ENODEV;
> +
> + muxc = i2c_mux_alloc(adap, &client->dev,
> + LTC4306_MAX_NCHANS, sizeof(*data), 0,
> + ltc4306_select_chan, ltc4306_deselect_mux);
Where's the symmetry in ..._chan for select and ..._mux for deselect?
> + if (!muxc)
> + return -ENOMEM;
> + data = i2c_mux_priv(muxc);
> +
> + i2c_set_clientdata(client, muxc);
> + data->client = client;
> +
> + /* Enable the mux if a enable GPIO is specified. */
s/ a / an /
> + gpio = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
> + if (IS_ERR(gpio))
> + return PTR_ERR(gpio);
Should you not use this pin later as well? In suspend/resume if you add
handling for that?
> +
> + /* Write the mux register at addr to verify
> + * that the mux is in fact present. This also
> + * initializes the mux to disconnected state.
> + */
Multi-line comments should start with a /* on its own line.
> + if (i2c_smbus_write_byte_data(client, LTC_REG_SWITCH, 0) < 0) {
> + dev_warn(&client->dev, "probe failed\n");
> + return -ENODEV;
> + }
> +
> + match = of_match_device(of_match_ptr(ltc4306_of_match), &client->dev);
Just call of_device_get_match_data() directly.
> + if (match)
> + data->chip = of_device_get_match_data(&client->dev);
> + else
> + data->chip = &chips[id->driver_data];
> +
> + if (of_node) {
> + idle_disconnect_dt =
> + of_property_read_bool(of_node,
> + "i2c-mux-idle-disconnect");
> + if (of_property_read_bool(of_node,
> + "ltc,downstream-accelerators-enable"))
> + data->regs[LTC_REG_CONFIG] |= LTC_DOWNSTREAM_ACCL_EN;
> +
> + if (of_property_read_bool(of_node,
> + "ltc,upstream-accelerators-enable"))
> + data->regs[LTC_REG_CONFIG] |= LTC_UPSTREAM_ACCL_EN;
> +
> + if (i2c_smbus_write_byte_data(client, LTC_REG_CONFIG,
> + data->regs[LTC_REG_CONFIG]) < 0) {
> + dev_warn(&client->dev, "probe failed\n");
> + return -ENODEV;
> + }
> + }
> +
> + 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++) {
> +
Blank lines aren't necessary after an open brace.
> + data->deselect |= idle_disconnect_dt << num;
Either *all* relevant bits are 1 or *all* relevant bits are 0. You
can get rid of this variable and invoke i2c_mux_alloc with NULL
instead of ltc4306_deselect_mux when not configured to disconnect
the mux. You have to delay the mux allocation to later in probe,
but I don't think it will be too hairy to accomplish that?
Cheers,
peda
> +
> + ret = i2c_mux_add_adapter(muxc, 0, num, 0);
> + if (ret) {
> + dev_err(&client->dev,
> + "failed to register multiplexed adapter %d\n",
> + num);
> + goto virt_reg_failed;
> + }
> + }
> +
> + dev_info(&client->dev,
> + "registered %d multiplexed busses for I2C switch %s\n",
> + num, client->name);
> +
> + return 0;
> +
> +virt_reg_failed:
> + i2c_mux_del_adapters(muxc);
> + return ret;
> +}
> +
> +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