Hi Peter,
Sorry and the late answer.
On Wed, 3 Nov 2010 16:14:03 +0100, Peter Korsgaard wrote:
> From: Peter Korsgaard <peter.korsgaard@xxxxxxxxx>
>
> Add an i2c mux driver providing virtual i2c busses using a hardware MUX
> sitting on a master bus and controlled through gpio pins.
>
> E.G. something like:
>
> ---------- ---------- Virtual bus 1 - - - - -
> | | SCL/SDA | |-------------- | |
> | |------------| |
> | | | | Virtual bus 2 | |
> | Linux | GPIO 1..N | MUX |--------------- Devices
> | |------------| | | |
> | | | | Virtual bus M
> | | | |---------------| |
> ---------- ---------- - - - - -
>
> SCL/SDA of the master I2C bus is multiplexed to virtual bus 1..M
> according to the settings of the GPIO pins 1..N.
>
> Signed-off-by: Peter Korsgaard <peter.korsgaard@xxxxxxxxx>
> ---
> Changes since v1:
> - Use i2c-mux infrastructure
> - Move to drivers/i2c/muxes
Thanks for doing this.
Overall the driver design looks good and your code looks pretty clean
too. Review:
>
> Documentation/i2c/busses/i2c-gpiomux | 65 +++++++++++++
> MAINTAINERS | 7 ++
> drivers/i2c/muxes/Kconfig | 12 +++
> drivers/i2c/muxes/Makefile | 1 +
> drivers/i2c/muxes/i2c-gpiomux.c | 172 ++++++++++++++++++++++++++++++++++
> include/linux/i2c-gpiomux.h | 35 +++++++
> 6 files changed, 292 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/i2c/busses/i2c-gpiomux
> create mode 100644 drivers/i2c/muxes/i2c-gpiomux.c
> create mode 100644 include/linux/i2c-gpiomux.h
>
> diff --git a/Documentation/i2c/busses/i2c-gpiomux b/Documentation/i2c/busses/i2c-gpiomux
> new file mode 100644
> index 0000000..b0a7746
> --- /dev/null
> +++ b/Documentation/i2c/busses/i2c-gpiomux
> @@ -0,0 +1,65 @@
> +Kernel driver i2c-gpiomux
> +
> +Author: Peter Korsgaard <peter.korsgaard@xxxxxxxxx>
> +
> +Description
> +-----------
> +
> +i2c-gpiomux is an i2c bus driver providing virtual I2C busses from a
It is an i2c mux driver now.
> +master I2C bus and a hardware MUX controlled through GPIO pins.
> +
> +E.G.:
> +
> + ---------- ---------- Virtual bus 1 - - - - -
> + | | SCL/SDA | |-------------- | |
> + | |------------| |
> + | | | | Virtual bus 2 | |
> + | Linux | GPIO 1..N | MUX |--------------- Devices
> + | |------------| | | |
> + | | | | Virtual bus M
> + | | | |---------------| |
> + ---------- ---------- - - - - -
> +
> +SCL/SDA of the master I2C bus is multiplexed to virtual bus 1..M
> +according to the settings of the GPIO pins 1..N.
> +
> +Usage
> +-----
> +
> +i2c-gpiomux uses the platform bus, so you need to provide a struct
> +platform_device with the platform_data pointing to a struct
> +gpiomux_i2c_platform_data with the I2C adapter number of the master
The structure should be named i2c_gpiomux_platform_data, so that the
prefix matches the driver name (but see below).
> +bus, the number of virtual busses to create and the GPIO pins used
> +to control it. See include/linux/i2c-gpiomux.h for details.
> +
> +E.G. something like this for a MUX providing 4 virtual busses
> +controlled through 3 GPIO pins:
> +
> +#include <linux/i2c-gpiomux.h>
> +#include <linux/platform_device.h>
> +
> +static unsigned myboard_gpiomux_pins[] = {
> + AT91_PIN_PC26, AT91_PIN_PC25, AT91_PIN_PC24
> +};
> +
> +static unsigned myboard_gpiomux_values[] = {
> + 0, 1, 2, 3
> +};
> +
> +static struct gpiomux_i2c_platform_data myboard_i2cmux_data = {
> + .parent = 1,
> + .base_nr = 2,
There might be no need to specify the numbers of the child I2C segments.
Your driver should handle the case where base_nr isn't set, and not ask
for specific i2c_adpater numbers then.
> + .busses = ARRAY_SIZE(myboard_gpiomux_values),
> + .gpios = ARRAY_SIZE(myboard_gpiomux_pins),
> + .gpio = myboard_gpiomux_pins,
> + .values = myboard_gpiomux_values,
I find your naming convention (or lack thereof) confusing. What about:
.values = myboard_gpiomux_values,
.n_values = ARRAY_SIZE(myboard_gpiomux_values),
.pins = myboard_gpiomux_pins,
.n_pins = ARRAY_SIZE(myboard_gpiomux_pins),
This would be more consistent and obvious.
> + .idle = 4,
This should be optional. Not all hardware setup can actually disable
all children.
> +};
> +
> +static struct platform_device myboard_i2cmux = {
> + .name = "i2c-gpiomux",
> + .id = 0,
> + .dev = {
> + .platform_data = &myboard_i2cmux_data,
> + },
> +};
All these structures can presumably be marked const.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0094224..ffe181a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2567,6 +2567,13 @@ S: Supported
> F: drivers/i2c/busses/i2c-gpio.c
> F: include/linux/i2c-gpio.h
>
> +GENERIC GPIO I2C MULTIPLEXER DRIVER
> +M: Peter Korsgaard <peter.korsgaard@xxxxxxxxx>
> +L: linux-i2c@xxxxxxxxxxxxxxx
> +S: Supported
> +F: drivers/i2c/muxes/i2c-gpiomux.c
> +F: include/linux/i2c-gpiomux.h
> +
> GENERIC HDLC (WAN) DRIVERS
> M: Krzysztof Halasa <khc@xxxxxxxxx>
> W: http://www.kernel.org/pub/linux/utils/net/hdlc/
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 4d91d80..0345e37 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -25,4 +25,16 @@ config I2C_MUX_PCA954x
> This driver can also be built as a module. If so, the module
> will be called pca954x.
>
> + config I2C_GPIOMUX
> + tristate "GPIO-based I2C multiplexer"
> + depends on GENERIC_GPIO
> + help
> + If you say yes to this option, support will be included for a
> + GPIO based I2C multiplexer. This driver provides virtual I2C
> + busses from a master bus through a MUX controlled through
There is nothing virtual about these bus segments. They are very real.
> + the generic GPIO interface.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-gpiomux.
> +
> endmenu
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index d743806..5143bc0 100644
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -3,5 +3,6 @@
>
> obj-$(CONFIG_I2C_MUX_PCA9541) += pca9541.o
> obj-$(CONFIG_I2C_MUX_PCA954x) += pca954x.o
> +obj-$(CONFIG_I2C_GPIOMUX) += i2c-gpiomux.o
It should be obvious from the above that your config option should be
named CONFIG_I2C_MUX_GPIO.
Not sure about the driver name. i2c-gpiomux makes it look like an i2c core or
i2c bus driver, which it is not. Maybe gpio-i2cmux would be better? I
see we already have drivers named gpio-fan for fans controlled over
GPIOs, as well as gpio_mouse and gpio_keys.
>
> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/muxes/i2c-gpiomux.c b/drivers/i2c/muxes/i2c-gpiomux.c
> new file mode 100644
> index 0000000..6e19ef4
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-gpiomux.c
> @@ -0,0 +1,172 @@
> +/*
> + * I2C multiplexer using GPIO API
> + *
> + * Peter Korsgaard <peter.korsgaard@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/i2c-mux.h>
> +#include <linux/i2c-gpiomux.h>
> +#include <linux/platform_device.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>
> +
> +struct gpiomux_i2c {
> + struct i2c_adapter *parent;
> + struct i2c_adapter **adap; /* child busses */
> + struct gpiomux_i2c_platform_data data;
> +};
> +
> +static void gpiomux_set(struct gpiomux_i2c *i2c, unsigned val)
i2c could be a const pointer.
> +{
> + int i;
> +
> + for (i = 0; i < i2c->data.gpios; i++)
Double space after the first ";".
> + gpio_set_value(i2c->data.gpio[i], val & (1<<i));
Although checkpatch.pl surprisingly doesn't complain, the usual style
is to have spaces around "<<".
> +}
> +
> +static int gpiomux_select(struct i2c_adapter *adap, void *data, u32 chan)
> +{
> + struct gpiomux_i2c *i2c = data;
> +
> + gpiomux_set(i2c, i2c->data.values[chan]);
> +
> + return 0;
> +}
> +
> +static int gpiomux_deselect(struct i2c_adapter *adap, void *data, u32 chan)
> +{
> + struct gpiomux_i2c *i2c = data;
> +
> + gpiomux_set(i2c, i2c->data.idle);
> +
> + return 0;
> +}
> +
> +static int __devinit gpiomux_probe(struct platform_device *pdev)
> +{
> + struct gpiomux_i2c *i2c;
> + struct gpiomux_i2c_platform_data *pdata;
> + struct i2c_adapter *parent;
> + int i, ret;
> +
> + pdata = pdev->dev.platform_data;
> + if (!pdata) {
> + dev_err(&pdev->dev, "Missing platform data\n");
> + return -ENODEV;
> + }
> +
> + parent = i2c_get_adapter(pdata->parent);
> + if (!parent) {
> + dev_err(&pdev->dev, "Parent adapter (%d) not found\n",
> + pdata->parent);
> + return -ENODEV;
> + }
> +
> + i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
> + if (!i2c) {
> + ret = -ENOMEM;
> + goto alloc_failed;
> + }
> +
> + i2c->parent = parent;
> + i2c->data = *pdata;
> + i2c->adap = kzalloc(sizeof(struct i2c_adapter *) * pdata->busses,
> + GFP_KERNEL);
> + if (!i2c->adap) {
> + ret = -ENOMEM;
> + goto alloc_failed2;
> + }
> +
> + for (i = 0; i < pdata->gpios; i++) {
> + ret = gpio_request(pdata->gpio[i], "i2c-gpiomux");
> + if (ret)
> + goto err_request_gpio;
> + gpio_direction_output(pdata->gpio[i], pdata->idle & (1<<i));
> + }
> +
> + for (i = 0; i < pdata->busses; i++) {
> + i2c->adap[i] =
> + i2c_add_mux_adapter(parent, i2c, pdata->base_nr + i, i,
> + gpiomux_select, gpiomux_deselect);
> + if (!i2c->adap[i]) {
> + ret = -ENODEV;
> + dev_err(&pdev->dev, "Failed to add adapter %d\n", i);
> + goto add_adapter_failed;
> + }
> + }
> +
> + dev_info(&pdev->dev, "%d port mux on %s adapter\n",
> + pdata->busses, parent->name);
> +
> + platform_set_drvdata(pdev, i2c);
> +
> + return 0;
> +
> +add_adapter_failed:
> + for (; i > 0; i--)
> + i2c_del_mux_adapter(i2c->adap[i - 1]);
> + i = pdata->gpios;
> +err_request_gpio:
> + for (; i > 0; i--)
> + gpio_free(pdata->gpio[i - 1]);
> + kfree(i2c->adap);
> +alloc_failed2:
> + kfree(i2c);
> +alloc_failed:
> + i2c_put_adapter(parent);
> +
> + return ret;
> +}
> +
> +static int __devexit gpiomux_remove(struct platform_device *pdev)
> +{
> + struct gpiomux_i2c *i2c = platform_get_drvdata(pdev);
> + int i;
> +
> + for (i = 0; i < i2c->data.busses; i++)
> + i2c_del_mux_adapter(i2c->adap[i]);
> +
> + for (i = 0; i < i2c->data.gpios; i++)
> + gpio_free(i2c->data.gpio[i]);
> +
> + platform_set_drvdata(pdev, NULL);
> + i2c_put_adapter(i2c->parent);
> + kfree(i2c->adap);
> + kfree(i2c);
> +
> + return 0;
> +}
> +
> +static struct platform_driver gpiomux_driver = {
> + .probe = gpiomux_probe,
> + .remove = __devexit_p(gpiomux_remove),
> + .driver = {
Please use tabs to align on "=".
> + .owner = THIS_MODULE,
> + .name = "i2c-gpiomux",
And please do the same here, for consistency.
> + },
> +};
> +
> +static int __init gpiomux_init(void)
> +{
> + return platform_driver_register(&gpiomux_driver);
> +}
> +
> +static void __exit gpiomux_exit(void)
> +{
> + platform_driver_unregister(&gpiomux_driver);
> +}
> +
> +module_init(gpiomux_init);
> +module_exit(gpiomux_exit);
> +
> +MODULE_DESCRIPTION("GPIO-based I2C multiplexer driver");
> +MODULE_AUTHOR("Peter Korsgaard <peter.korsgaard@xxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:i2c-gpiomux");
> diff --git a/include/linux/i2c-gpiomux.h b/include/linux/i2c-gpiomux.h
> new file mode 100644
> index 0000000..2388ff2
> --- /dev/null
> +++ b/include/linux/i2c-gpiomux.h
> @@ -0,0 +1,35 @@
> +/*
> + * i2c-gpiomux interface to platform code
> + *
> + * Peter Korsgaard <peter.korsgaard@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _LINUX_I2C_GPIOMUX_H
> +#define _LINUX_I2C_GPIOMUX_H
> +
> +/**
> + * struct gpiomux_i2c_platform_data - Platform-dependent data for i2c-gpiomux
> + * @parent: Parent I2C bus adapter number
> + * @base_nr: Base bus number value to number adapters from
> + * @busses: Number of multiplexer positions (busses to instantiate)
> + * @gpios: Number of gpios used to control MUX
> + * @gpio: Array of @gpios gpio numbers used to control MUX
> + * @values: Array of @bussed bitmasks of gpio settings (low/high) for each
> + * position
> + * @idle: Bitmask to write to MUX when idle
> + */
> +struct gpiomux_i2c_platform_data {
> + int parent;
> + int base_nr;
> + int busses;
> + int gpios;
> + unsigned *gpio;
> + unsigned *values;
> + unsigned idle;
> +};
> +
> +#endif /* _LINUX_I2C_GPIOMUX_H */
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
