Based on i2c-mux-gpio driver, similarly the register based mux
switch from one bus to another by setting a single register.
The register can be on PCIe bus, local bus, or any memory-mapped
address.
Signed-off-by: York Sun <[email protected]>
CC: Wolfram Sang <[email protected]>
CC: Peter Korsgaard <[email protected]>
---
.../devicetree/bindings/i2c/i2c-mux-reg.txt | 69 ++++++
drivers/i2c/muxes/Kconfig | 11 +
drivers/i2c/muxes/Makefile | 1 +
drivers/i2c/muxes/i2c-mux-reg.c | 239 ++++++++++++++++++++
drivers/i2c/muxes/i2c-mux-reg.h | 38 ++++
5 files changed, 358 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
create mode 100644 drivers/i2c/muxes/i2c-mux-reg.c
create mode 100644 drivers/i2c/muxes/i2c-mux-reg.h
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
new file mode 100644
index 0000000..ad7cc4f
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
@@ -0,0 +1,69 @@
+Register-based I2C Bus Mux
+
+This binding describes an I2C bus multiplexer that uses a single regsiter
+to route the I2C signals.
+
+Required properties:
+- compatible: i2c-mux-reg
+- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
+ port is connected to.
+* Standard I2C mux properties. See mux.txt in this directory.
+* I2C child bus nodes. See mux.txt in this directory.
+
+Optional properties:
+- reg: this pair of <offset size> specifies the register to control the mux.
+ The <offset size> depends on its parent node. It can be any memory-mapped
+ address. If omitted, the resource of this device will be used.
+- idle-state: value to set the muxer to when idle. When no value is
+ given, it defaults to the last value used.
+
+For each i2c child node, an I2C child bus will be created. They will
+be numbered based on their order in the device tree.
+
+Whenever an access is made to a device on a child bus, the value set
+in the revelant node's reg property will be output to the register.
+
+If an idle state is defined, using the idle-state (optional) property,
+whenever an access is not being made to a device on a child bus, the
+register will be set according to the idle value.
+
+If an idle state is not defined, the most recently used value will be
+left programmed into the register.
+
+Example of a mux on PCIe card, the host is a powerpc SoC (big endian):
+
+ i2c-mux {
+ /* the <offset size> depends on the address translation
+ * of the parent device. If omitted, device resource
+ * will be used instead.
+ */
+ reg = <0x6028 0x4>;
+ compatible = "i2c-mux-reg";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ i2c-parent = <&i2c1>;
+ i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ si5338: clock-generator@70 {
+ compatible = "silabs,si5338";
+ reg = <0x70>;
+ /* other stuff */
+ };
+ };
+
+ i2c@1 {
+ /* data is in little endian on pcie bus */
+ reg = <0x01000000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ si5338: clock-generator@70 {
+ compatible = "silabs,si5338";
+ reg = <0x70>;
+ /* other stuff */
+ };
+ };
+ };
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index f6d313e..77c1257 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -29,6 +29,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_REG
+ tristate "Register-based I2C multiplexer"
+ help
+ If you say yes to this option, support will be included for a
+ register based I2C multiplexer. This driver provides access to
+ I2C busses connected through a MUX, which is controlled
+ by a sinple register.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mux-reg.
+
config I2C_MUX_PCA9541
tristate "NXP PCA9541 I2C Master Selector"
help
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 465778b..bc517bb 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -4,6 +4,7 @@
obj-$(CONFIG_I2C_ARB_GPIO_CHALLENGE) += i2c-arb-gpio-challenge.o
obj-$(CONFIG_I2C_MUX_GPIO) += i2c-mux-gpio.o
+obj-$(CONFIG_I2C_MUX_REG) += i2c-mux-reg.o
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-reg.c b/drivers/i2c/muxes/i2c-mux-reg.c
new file mode 100644
index 0000000..03ce858
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-mux-reg.c
@@ -0,0 +1,239 @@
+/*
+ * I2C multiplexer using a single register
+ *
+ * Copyright 2015 Freescale Semiconductor
+ * York Sun <[email protected]>
+ *
+ * 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/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include "i2c-mux-reg.h"
+
+struct regmux {
+ struct i2c_adapter *parent;
+ struct i2c_adapter **adap; /* child busses */
+ struct i2c_mux_reg_platform_data data;
+};
+
+static int i2c_mux_reg_set(const struct regmux *mux, unsigned int chan)
+{
+ if (!mux->data.reg || chan < 0 || chan > mux->data.n_values)
+ return -EINVAL;
+
+ *mux->data.reg = mux->data.values[chan];
+
+ return 0;
+}
+
+static int i2c_mux_reg_select(struct i2c_adapter *adap, void *data,
+ unsigned int chan)
+{
+ struct regmux *mux = data;
+
+ return i2c_mux_reg_set(mux, chan);
+}
+
+static int i2c_mux_reg_deselect(struct i2c_adapter *adap, void *data,
+ unsigned int chan)
+{
+ struct regmux *mux = data;
+
+ return i2c_mux_reg_set(mux, mux->data.idle);
+}
+
+#ifdef CONFIG_OF
+static int i2c_mux_reg_probe_dt(struct regmux *mux,
+ struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct device_node *adapter_np, *child;
+ struct i2c_adapter *adapter;
+ struct resource res;
+ unsigned *values;
+ int i = 0;
+
+ if (!np)
+ return -ENODEV;
+
+ adapter_np = of_parse_phandle(np, "i2c-parent", 0);
+ if (!adapter_np) {
+ dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
+ return -ENODEV;
+ }
+ adapter = of_find_i2c_adapter_by_node(adapter_np);
+ if (!adapter) {
+ dev_err(&pdev->dev, "Cannot find parent bus\n");
+ return -EPROBE_DEFER;
+ }
+ mux->parent = adapter;
+ mux->data.parent = i2c_adapter_id(adapter);
+ put_device(&adapter->dev);
+
+ mux->data.n_values = of_get_child_count(np);
+
+ values = devm_kzalloc(&pdev->dev,
+ sizeof(*mux->data.values) * mux->data.n_values,
+ GFP_KERNEL);
+ if (!values) {
+ dev_err(&pdev->dev, "Cannot allocate values array");
+ return -ENOMEM;
+ }
+
+ for_each_child_of_node(np, child) {
+ of_property_read_u32(child, "reg", values + i);
+ i++;
+ }
+ mux->data.values = values;
+
+ if (of_property_read_u32(np, "idle-state", &mux->data.idle))
+ mux->data.idle = I2C_MUX_REG_NO_IDLE;
+
+ /* map address from "reg" if exists */
+ if (!of_address_to_resource(np, 0, &res)) {
+ mux->data.reg = devm_ioremap_resource(&pdev->dev, &res);
+ if (IS_ERR(mux->data.reg))
+ return PTR_ERR(mux->data.reg);
+ }
+
+ return 0;
+}
+#else
+static int i2c_mux_reg_probe_dt(struct gpiomux *mux,
+ struct platform_device *pdev)
+{
+ return 0;
+}
+#endif
+
+static int i2c_mux_reg_probe(struct platform_device *pdev)
+{
+ struct regmux *mux;
+ struct i2c_adapter *parent;
+ struct resource *res;
+ int (*deselect)(struct i2c_adapter *, void *, u32);
+ unsigned int initial_state, class;
+ int i, ret, nr;
+
+ mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
+ if (!mux)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, mux);
+
+ if (dev_get_platdata(&pdev->dev)) {
+ memcpy(&mux->data, dev_get_platdata(&pdev->dev),
+ sizeof(mux->data));
+
+ parent = i2c_get_adapter(mux->data.parent);
+ if (!parent) {
+ dev_err(&pdev->dev, "Parent adapter (%d) not found\n",
+ mux->data.parent);
+ return -EPROBE_DEFER;
+ }
+ mux->parent = parent;
+ i2c_put_adapter(parent);
+ } else {
+ ret = i2c_mux_reg_probe_dt(mux, pdev);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Cannot locate device tree");
+ return ret;
+ }
+ }
+
+ if (!mux->data.reg) {
+ dev_info(&pdev->dev,
+ "Register not set, using platform resource\n");
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ mux->data.reg = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(mux->data.reg))
+ return PTR_ERR(mux->data.reg);
+ }
+
+ mux->adap = devm_kzalloc(&pdev->dev,
+ sizeof(*mux->adap) * mux->data.n_values,
+ GFP_KERNEL);
+ if (!mux->adap) {
+ dev_err(&pdev->dev, "Cannot allocate i2c_adapter structure");
+ return -ENOMEM;
+ }
+
+ if (mux->data.idle != I2C_MUX_REG_NO_IDLE) {
+ initial_state = mux->data.idle;
+ deselect = i2c_mux_reg_deselect;
+ } else {
+ initial_state = mux->data.values[0];
+ deselect = NULL;
+ }
+
+ for (i = 0; i < mux->data.n_values; i++) {
+ nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
+ class = mux->data.classes ? mux->data.classes[i] : 0;
+
+ mux->adap[i] = i2c_add_mux_adapter(mux->parent, &pdev->dev, mux,
+ nr, mux->data.values[i],
+ class, i2c_mux_reg_select,
+ deselect);
+ if (!mux->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",
+ mux->data.n_values, mux->parent->name);
+
+ return 0;
+
+add_adapter_failed:
+ for (; i > 0; i--)
+ i2c_del_mux_adapter(mux->adap[i - 1]);
+
+ return ret;
+}
+
+static int i2c_mux_reg_remove(struct platform_device *pdev)
+{
+ struct regmux *mux = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = 0; i < mux->data.n_values; i++)
+ i2c_del_mux_adapter(mux->adap[i]);
+
+ i2c_put_adapter(mux->parent);
+
+ dev_info(&pdev->dev, "Removed\n");
+
+ return 0;
+}
+
+static const struct of_device_id i2c_mux_reg_of_match[] = {
+ { .compatible = "i2c-mux-reg", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, i2c_mux_reg_of_match);
+
+static struct platform_driver i2c_mux_reg_driver = {
+ .probe = i2c_mux_reg_probe,
+ .remove = i2c_mux_reg_remove,
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = "i2c-mux-reg",
+ },
+};
+
+module_platform_driver(i2c_mux_reg_driver);
+
+MODULE_DESCRIPTION("Register-based I2C multiplexer driver");
+MODULE_AUTHOR("York Sun <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:i2c-mux-reg");
diff --git a/drivers/i2c/muxes/i2c-mux-reg.h b/drivers/i2c/muxes/i2c-mux-reg.h
new file mode 100644
index 0000000..e213988
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-mux-reg.h
@@ -0,0 +1,38 @@
+/*
+ * I2C multiplexer using a single register
+ *
+ * Copyright 2015 Freescale Semiconductor
+ * York Sun <[email protected]>
+ *
+ * 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_MUX_REG_H
+#define __LINUX_I2C_MUX_REG_H
+
+/* MUX has no specific idel mode */
+#define I2C_MUX_REG_NO_IDLE ((unsigned)-1)
+
+/**
+ * struct i2c_mux_reg_platform_data - Platform-dependent data for i2c-mux-reg
+ * @parent: Parent I2C bus adapter number
+ * @base_nr: Base I2C bus number to number adapters from or zero for dynamic
+ * @values: Array of value for each channel
+ * @n_values: Number of multiplexer channels
+ * @classes: Optional I2C auto-detection classes
+ * @idle: Value to write to mux when idle
+ * @reg: Virtual address of the register to switch channel
+ */
+struct i2c_mux_reg_platform_data {
+ int parent;
+ int base_nr;
+ const unsigned int *values;
+ int n_values;
+ const unsigned int *classes;
+ unsigned int idle;
+ unsigned int *reg;
+};
+
+#endif /* __LINUX_I2C_MUX_REG_H */
--
1.7.9.5
A nit and a question follow.
On Tue, 2015-06-16 at 10:28 -0700, York Sun wrote:
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-reg.c
> + * 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.
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
According to include/linux/module.h this sates the license is GPL v2 or
later. So I think that either the comment at the top of this file or the
ident used in the MODULE_LICENSE() macro needs to change.
> +MODULE_ALIAS("platform:i2c-mux-reg");
As far as I understand it, this alias is only useful if there's a
corresponding struct platform_device, somewhere. Ie, this alias needs a
platform_device that will fire of a "MODALIAS=platform:i2c-mux-reg"
uevent when it's created. (I don't know exactly how all that works, so
I'm handwaving quite a bit here.) That would be platform_device with a
"i2c-mux-reg" name.
Did I get this right? Because then I think this MODULE_ALIAS() isn't
needed, as I couldn't find such a corresponding platform_device.
Thanks,
Paul Bolle
Hi!
On 17/06/15 10:54, ext Paul Bolle wrote:
>> +MODULE_ALIAS("platform:i2c-mux-reg");
> As far as I understand it, this alias is only useful if there's a
> corresponding struct platform_device, somewhere. Ie, this alias needs a
> platform_device that will fire of a "MODALIAS=platform:i2c-mux-reg"
> uevent when it's created. (I don't know exactly how all that works, so
> I'm handwaving quite a bit here.) That would be platform_device with a
> "i2c-mux-reg" name.
>
> Did I get this right? Because then I think this MODULE_ALIAS() isn't
> needed, as I couldn't find such a corresponding platform_device.
You do not see the platform_device, because there are no users yet, put
this MODULE_ALIAS() is perfectly fine, it will allow automatic module loading
in non-DT case.
--
Best regards,
Alexander Sverdlin.
Hi!
On 16/06/15 19:28, ext York Sun wrote:
> Based on i2c-mux-gpio driver, similarly the register based mux
> switch from one bus to another by setting a single register.
> The register can be on PCIe bus, local bus, or any memory-mapped
> address.
>
> Signed-off-by: York Sun <[email protected]>
> CC: Wolfram Sang <[email protected]>
> CC: Peter Korsgaard <[email protected]>
> ---
> .../devicetree/bindings/i2c/i2c-mux-reg.txt | 69 ++++++
> drivers/i2c/muxes/Kconfig | 11 +
> drivers/i2c/muxes/Makefile | 1 +
> drivers/i2c/muxes/i2c-mux-reg.c | 239 ++++++++++++++++++++
> drivers/i2c/muxes/i2c-mux-reg.h | 38 ++++
> 5 files changed, 358 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
> create mode 100644 drivers/i2c/muxes/i2c-mux-reg.c
> create mode 100644 drivers/i2c/muxes/i2c-mux-reg.h
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
> new file mode 100644
> index 0000000..ad7cc4f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
> @@ -0,0 +1,69 @@
> +Register-based I2C Bus Mux
> +
> +This binding describes an I2C bus multiplexer that uses a single regsiter
^^^^^^^^
> +to route the I2C signals.
> +
> +Required properties:
> +- compatible: i2c-mux-reg
> +- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
> + port is connected to.
> +* Standard I2C mux properties. See mux.txt in this directory.
> +* I2C child bus nodes. See mux.txt in this directory.
> +
> +Optional properties:
> +- reg: this pair of <offset size> specifies the register to control the mux.
> + The <offset size> depends on its parent node. It can be any memory-mapped
> + address. If omitted, the resource of this device will be used.
> +- idle-state: value to set the muxer to when idle. When no value is
> + given, it defaults to the last value used.
> +
> +For each i2c child node, an I2C child bus will be created. They will
> +be numbered based on their order in the device tree.
> +
> +Whenever an access is made to a device on a child bus, the value set
> +in the revelant node's reg property will be output to the register.
> +
> +If an idle state is defined, using the idle-state (optional) property,
> +whenever an access is not being made to a device on a child bus, the
> +register will be set according to the idle value.
> +
> +If an idle state is not defined, the most recently used value will be
> +left programmed into the register.
> +
> +Example of a mux on PCIe card, the host is a powerpc SoC (big endian):
> +
> + i2c-mux {
> + /* the <offset size> depends on the address translation
> + * of the parent device. If omitted, device resource
> + * will be used instead.
> + */
> + reg = <0x6028 0x4>;
> + compatible = "i2c-mux-reg";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + i2c-parent = <&i2c1>;
> + i2c@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + si5338: clock-generator@70 {
> + compatible = "silabs,si5338";
> + reg = <0x70>;
> + /* other stuff */
> + };
> + };
> +
> + i2c@1 {
> + /* data is in little endian on pcie bus */
> + reg = <0x01000000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + si5338: clock-generator@70 {
> + compatible = "silabs,si5338";
> + reg = <0x70>;
> + /* other stuff */
> + };
> + };
> + };
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index f6d313e..77c1257 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -29,6 +29,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_REG
> + tristate "Register-based I2C multiplexer"
> + help
> + If you say yes to this option, support will be included for a
> + register based I2C multiplexer. This driver provides access to
> + I2C busses connected through a MUX, which is controlled
> + by a sinple register.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-mux-reg.
> +
> config I2C_MUX_PCA9541
> tristate "NXP PCA9541 I2C Master Selector"
> help
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index 465778b..bc517bb 100644
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -4,6 +4,7 @@
> obj-$(CONFIG_I2C_ARB_GPIO_CHALLENGE) += i2c-arb-gpio-challenge.o
>
> obj-$(CONFIG_I2C_MUX_GPIO) += i2c-mux-gpio.o
> +obj-$(CONFIG_I2C_MUX_REG) += i2c-mux-reg.o
> 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-reg.c b/drivers/i2c/muxes/i2c-mux-reg.c
> new file mode 100644
> index 0000000..03ce858
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-reg.c
> @@ -0,0 +1,239 @@
> +/*
> + * I2C multiplexer using a single register
> + *
> + * Copyright 2015 Freescale Semiconductor
> + * York Sun <[email protected]>
> + *
> + * 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/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include "i2c-mux-reg.h"
> +
> +struct regmux {
> + struct i2c_adapter *parent;
> + struct i2c_adapter **adap; /* child busses */
> + struct i2c_mux_reg_platform_data data;
> +};
> +
> +static int i2c_mux_reg_set(const struct regmux *mux, unsigned int chan)
> +{
> + if (!mux->data.reg || chan < 0 || chan > mux->data.n_values)
> + return -EINVAL;
> +
> + *mux->data.reg = mux->data.values[chan];
Oh, I believe, this will not work. This will not work for PCI, because it's
a "posted" bus, it will not work on certain high-performance SoC like Octeon,
where writes to the memory are buffered... Finally your I2C transfer can
be performed before this write will be completed (if at all), because there
is no synchronization here. You probably want to use some iowrite*(), but
maybe also accomplish it with readback. There are ARM architectures where
writes to memory mapped HW modules are only ordered inside one module, but
not ordered between the modules, etc... Without volatile a good compiler
will optimize this statement away completely as it produces no "side effect".
> +
> + return 0;
> +}
> +
> +static int i2c_mux_reg_select(struct i2c_adapter *adap, void *data,
> + unsigned int chan)
> +{
> + struct regmux *mux = data;
> +
> + return i2c_mux_reg_set(mux, chan);
> +}
> +
> +static int i2c_mux_reg_deselect(struct i2c_adapter *adap, void *data,
> + unsigned int chan)
> +{
> + struct regmux *mux = data;
> +
> + return i2c_mux_reg_set(mux, mux->data.idle);
> +}
> +
> +#ifdef CONFIG_OF
> +static int i2c_mux_reg_probe_dt(struct regmux *mux,
> + struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct device_node *adapter_np, *child;
> + struct i2c_adapter *adapter;
> + struct resource res;
> + unsigned *values;
> + int i = 0;
> +
> + if (!np)
> + return -ENODEV;
> +
> + adapter_np = of_parse_phandle(np, "i2c-parent", 0);
> + if (!adapter_np) {
> + dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
> + return -ENODEV;
> + }
> + adapter = of_find_i2c_adapter_by_node(adapter_np);
> + if (!adapter) {
> + dev_err(&pdev->dev, "Cannot find parent bus\n");
> + return -EPROBE_DEFER;
> + }
> + mux->parent = adapter;
> + mux->data.parent = i2c_adapter_id(adapter);
> + put_device(&adapter->dev);
> +
> + mux->data.n_values = of_get_child_count(np);
> +
> + values = devm_kzalloc(&pdev->dev,
> + sizeof(*mux->data.values) * mux->data.n_values,
> + GFP_KERNEL);
> + if (!values) {
> + dev_err(&pdev->dev, "Cannot allocate values array");
> + return -ENOMEM;
> + }
> +
> + for_each_child_of_node(np, child) {
> + of_property_read_u32(child, "reg", values + i);
> + i++;
> + }
> + mux->data.values = values;
> +
> + if (of_property_read_u32(np, "idle-state", &mux->data.idle))
> + mux->data.idle = I2C_MUX_REG_NO_IDLE;
> +
> + /* map address from "reg" if exists */
> + if (!of_address_to_resource(np, 0, &res)) {
> + mux->data.reg = devm_ioremap_resource(&pdev->dev, &res);
> + if (IS_ERR(mux->data.reg))
> + return PTR_ERR(mux->data.reg);
> + }
> +
> + return 0;
> +}
> +#else
> +static int i2c_mux_reg_probe_dt(struct gpiomux *mux,
> + struct platform_device *pdev)
> +{
> + return 0;
> +}
> +#endif
> +
> +static int i2c_mux_reg_probe(struct platform_device *pdev)
> +{
> + struct regmux *mux;
> + struct i2c_adapter *parent;
> + struct resource *res;
> + int (*deselect)(struct i2c_adapter *, void *, u32);
> + unsigned int initial_state, class;
> + int i, ret, nr;
> +
> + mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, mux);
> +
> + if (dev_get_platdata(&pdev->dev)) {
> + memcpy(&mux->data, dev_get_platdata(&pdev->dev),
> + sizeof(mux->data));
> +
> + parent = i2c_get_adapter(mux->data.parent);
> + if (!parent) {
> + dev_err(&pdev->dev, "Parent adapter (%d) not found\n",
> + mux->data.parent);
> + return -EPROBE_DEFER;
> + }
> + mux->parent = parent;
> + i2c_put_adapter(parent);
You should only give up this reference in remove() function of your driver.
> + } else {
> + ret = i2c_mux_reg_probe_dt(mux, pdev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Cannot locate device tree");
> + return ret;
> + }
> + }
> +
> + if (!mux->data.reg) {
> + dev_info(&pdev->dev,
> + "Register not set, using platform resource\n");
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + mux->data.reg = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(mux->data.reg))
> + return PTR_ERR(mux->data.reg);
> + }
> +
> + mux->adap = devm_kzalloc(&pdev->dev,
> + sizeof(*mux->adap) * mux->data.n_values,
> + GFP_KERNEL);
> + if (!mux->adap) {
> + dev_err(&pdev->dev, "Cannot allocate i2c_adapter structure");
> + return -ENOMEM;
> + }
> +
> + if (mux->data.idle != I2C_MUX_REG_NO_IDLE) {
> + initial_state = mux->data.idle;
> + deselect = i2c_mux_reg_deselect;
> + } else {
> + initial_state = mux->data.values[0];
> + deselect = NULL;
> + }
> +
> + for (i = 0; i < mux->data.n_values; i++) {
> + nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
> + class = mux->data.classes ? mux->data.classes[i] : 0;
> +
> + mux->adap[i] = i2c_add_mux_adapter(mux->parent, &pdev->dev, mux,
> + nr, mux->data.values[i],
> + class, i2c_mux_reg_select,
> + deselect);
> + if (!mux->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",
> + mux->data.n_values, mux->parent->name);
This could be dev_dbg() also, IMHO, as this information has very little value.
> +
> + return 0;
> +
> +add_adapter_failed:
> + for (; i > 0; i--)
> + i2c_del_mux_adapter(mux->adap[i - 1]);
> +
> + return ret;
> +}
> +
> +static int i2c_mux_reg_remove(struct platform_device *pdev)
> +{
> + struct regmux *mux = platform_get_drvdata(pdev);
> + int i;
> +
> + for (i = 0; i < mux->data.n_values; i++)
> + i2c_del_mux_adapter(mux->adap[i]);
> +
> + i2c_put_adapter(mux->parent);
> +
> + dev_info(&pdev->dev, "Removed\n");
I would say, only dev_dbg() is allowed here, otherwise it's a noise.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id i2c_mux_reg_of_match[] = {
> + { .compatible = "i2c-mux-reg", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, i2c_mux_reg_of_match);
> +
> +static struct platform_driver i2c_mux_reg_driver = {
> + .probe = i2c_mux_reg_probe,
> + .remove = i2c_mux_reg_remove,
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "i2c-mux-reg",
> + },
> +};
> +
> +module_platform_driver(i2c_mux_reg_driver);
> +
> +MODULE_DESCRIPTION("Register-based I2C multiplexer driver");
> +MODULE_AUTHOR("York Sun <[email protected]>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:i2c-mux-reg");
> diff --git a/drivers/i2c/muxes/i2c-mux-reg.h b/drivers/i2c/muxes/i2c-mux-reg.h
> new file mode 100644
> index 0000000..e213988
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-reg.h
> @@ -0,0 +1,38 @@
> +/*
> + * I2C multiplexer using a single register
> + *
> + * Copyright 2015 Freescale Semiconductor
> + * York Sun <[email protected]>
> + *
> + * 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_MUX_REG_H
> +#define __LINUX_I2C_MUX_REG_H
> +
> +/* MUX has no specific idel mode */
^^^^
> +#define I2C_MUX_REG_NO_IDLE ((unsigned)-1)
What if the idle state is exactly "all ones"? Quite important corner case actually...
> +
> +/**
> + * struct i2c_mux_reg_platform_data - Platform-dependent data for i2c-mux-reg
> + * @parent: Parent I2C bus adapter number
> + * @base_nr: Base I2C bus number to number adapters from or zero for dynamic
> + * @values: Array of value for each channel
> + * @n_values: Number of multiplexer channels
> + * @classes: Optional I2C auto-detection classes
> + * @idle: Value to write to mux when idle
> + * @reg: Virtual address of the register to switch channel
> + */
> +struct i2c_mux_reg_platform_data {
> + int parent;
> + int base_nr;
> + const unsigned int *values;
> + int n_values;
> + const unsigned int *classes;
> + unsigned int idle;
> + unsigned int *reg;
Yeah, this is really bad idea. You maybe want something like
__iomem "cookie" here instead of this bare pointer.
> +};
> +
> +#endif /* __LINUX_I2C_MUX_REG_H */
>
Other than the mentioned above, this is a useful code.
--
Best regards,
Alexander Sverdlin.
On Wed, 2015-06-17 at 17:00 +0200, Alexander Sverdlin wrote:
> You do not see the platform_device, because there are no users yet, put
> this MODULE_ALIAS() is perfectly fine, it will allow automatic module loading
> in non-DT case.
Do you mean that it will allow automatic module loading once the patch
that adds a struct platform_device with a "i2c-mux-reg" name lands?
Paul Bolle
On 06/17/2015 08:03 AM, Alexander Sverdlin wrote:
> Hi!
>
> On 16/06/15 19:28, ext York Sun wrote:
>> Based on i2c-mux-gpio driver, similarly the register based mux
>> switch from one bus to another by setting a single register.
>> The register can be on PCIe bus, local bus, or any memory-mapped
>> address.
>>
>> Signed-off-by: York Sun <[email protected]>
>> CC: Wolfram Sang <[email protected]>
>> CC: Peter Korsgaard <[email protected]>
>> ---
>> .../devicetree/bindings/i2c/i2c-mux-reg.txt | 69 ++++++
>> drivers/i2c/muxes/Kconfig | 11 +
>> drivers/i2c/muxes/Makefile | 1 +
>> drivers/i2c/muxes/i2c-mux-reg.c | 239 ++++++++++++++++++++
>> drivers/i2c/muxes/i2c-mux-reg.h | 38 ++++
>> 5 files changed, 358 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
>> create mode 100644 drivers/i2c/muxes/i2c-mux-reg.c
>> create mode 100644 drivers/i2c/muxes/i2c-mux-reg.h
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
>> new file mode 100644
>> index 0000000..ad7cc4f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
>> @@ -0,0 +1,69 @@
>> +Register-based I2C Bus Mux
>> +
>> +This binding describes an I2C bus multiplexer that uses a single regsiter
> ^^^^^^^^
Nice catch.
>
>> +to route the I2C signals.
>> +
>> +Required properties:
>> +- compatible: i2c-mux-reg
>> +- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
>> + port is connected to.
>> +* Standard I2C mux properties. See mux.txt in this directory.
>> +* I2C child bus nodes. See mux.txt in this directory.
>> +
>> +Optional properties:
>> +- reg: this pair of <offset size> specifies the register to control the mux.
>> + The <offset size> depends on its parent node. It can be any memory-mapped
>> + address. If omitted, the resource of this device will be used.
>> +- idle-state: value to set the muxer to when idle. When no value is
>> + given, it defaults to the last value used.
>> +
>> +For each i2c child node, an I2C child bus will be created. They will
>> +be numbered based on their order in the device tree.
>> +
>> +Whenever an access is made to a device on a child bus, the value set
>> +in the revelant node's reg property will be output to the register.
>> +
>> +If an idle state is defined, using the idle-state (optional) property,
>> +whenever an access is not being made to a device on a child bus, the
>> +register will be set according to the idle value.
>> +
>> +If an idle state is not defined, the most recently used value will be
>> +left programmed into the register.
>> +
>> +Example of a mux on PCIe card, the host is a powerpc SoC (big endian):
>> +
>> + i2c-mux {
>> + /* the <offset size> depends on the address translation
>> + * of the parent device. If omitted, device resource
>> + * will be used instead.
>> + */
>> + reg = <0x6028 0x4>;
>> + compatible = "i2c-mux-reg";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + i2c-parent = <&i2c1>;
>> + i2c@0 {
>> + reg = <0>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + si5338: clock-generator@70 {
>> + compatible = "silabs,si5338";
>> + reg = <0x70>;
>> + /* other stuff */
>> + };
>> + };
>> +
>> + i2c@1 {
>> + /* data is in little endian on pcie bus */
>> + reg = <0x01000000>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + si5338: clock-generator@70 {
>> + compatible = "silabs,si5338";
>> + reg = <0x70>;
>> + /* other stuff */
>> + };
>> + };
>> + };
>> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
>> index f6d313e..77c1257 100644
>> --- a/drivers/i2c/muxes/Kconfig
>> +++ b/drivers/i2c/muxes/Kconfig
>> @@ -29,6 +29,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_REG
>> + tristate "Register-based I2C multiplexer"
>> + help
>> + If you say yes to this option, support will be included for a
>> + register based I2C multiplexer. This driver provides access to
>> + I2C busses connected through a MUX, which is controlled
>> + by a sinple register.
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called i2c-mux-reg.
>> +
>> config I2C_MUX_PCA9541
>> tristate "NXP PCA9541 I2C Master Selector"
>> help
>> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
>> index 465778b..bc517bb 100644
>> --- a/drivers/i2c/muxes/Makefile
>> +++ b/drivers/i2c/muxes/Makefile
>> @@ -4,6 +4,7 @@
>> obj-$(CONFIG_I2C_ARB_GPIO_CHALLENGE) += i2c-arb-gpio-challenge.o
>>
>> obj-$(CONFIG_I2C_MUX_GPIO) += i2c-mux-gpio.o
>> +obj-$(CONFIG_I2C_MUX_REG) += i2c-mux-reg.o
>> 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-reg.c b/drivers/i2c/muxes/i2c-mux-reg.c
>> new file mode 100644
>> index 0000000..03ce858
>> --- /dev/null
>> +++ b/drivers/i2c/muxes/i2c-mux-reg.c
>> @@ -0,0 +1,239 @@
>> +/*
>> + * I2C multiplexer using a single register
>> + *
>> + * Copyright 2015 Freescale Semiconductor
>> + * York Sun <[email protected]>
>> + *
>> + * 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/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include "i2c-mux-reg.h"
>> +
>> +struct regmux {
>> + struct i2c_adapter *parent;
>> + struct i2c_adapter **adap; /* child busses */
>> + struct i2c_mux_reg_platform_data data;
>> +};
>> +
>> +static int i2c_mux_reg_set(const struct regmux *mux, unsigned int chan)
>> +{
>> + if (!mux->data.reg || chan < 0 || chan > mux->data.n_values)
>> + return -EINVAL;
>> +
>> + *mux->data.reg = mux->data.values[chan];
>
> Oh, I believe, this will not work. This will not work for PCI, because it's
> a "posted" bus, it will not work on certain high-performance SoC like Octeon,
> where writes to the memory are buffered... Finally your I2C transfer can
> be performed before this write will be completed (if at all), because there
> is no synchronization here. You probably want to use some iowrite*(), but
> maybe also accomplish it with readback. There are ARM architectures where
> writes to memory mapped HW modules are only ordered inside one module, but
> not ordered between the modules, etc... Without volatile a good compiler
> will optimize this statement away completely as it produces no "side effect".
Agree I should have used iowrite.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int i2c_mux_reg_select(struct i2c_adapter *adap, void *data,
>> + unsigned int chan)
>> +{
>> + struct regmux *mux = data;
>> +
>> + return i2c_mux_reg_set(mux, chan);
>> +}
>> +
>> +static int i2c_mux_reg_deselect(struct i2c_adapter *adap, void *data,
>> + unsigned int chan)
>> +{
>> + struct regmux *mux = data;
>> +
>> + return i2c_mux_reg_set(mux, mux->data.idle);
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static int i2c_mux_reg_probe_dt(struct regmux *mux,
>> + struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct device_node *adapter_np, *child;
>> + struct i2c_adapter *adapter;
>> + struct resource res;
>> + unsigned *values;
>> + int i = 0;
>> +
>> + if (!np)
>> + return -ENODEV;
>> +
>> + adapter_np = of_parse_phandle(np, "i2c-parent", 0);
>> + if (!adapter_np) {
>> + dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
>> + return -ENODEV;
>> + }
>> + adapter = of_find_i2c_adapter_by_node(adapter_np);
>> + if (!adapter) {
>> + dev_err(&pdev->dev, "Cannot find parent bus\n");
>> + return -EPROBE_DEFER;
>> + }
>> + mux->parent = adapter;
>> + mux->data.parent = i2c_adapter_id(adapter);
>> + put_device(&adapter->dev);
>> +
>> + mux->data.n_values = of_get_child_count(np);
>> +
>> + values = devm_kzalloc(&pdev->dev,
>> + sizeof(*mux->data.values) * mux->data.n_values,
>> + GFP_KERNEL);
>> + if (!values) {
>> + dev_err(&pdev->dev, "Cannot allocate values array");
>> + return -ENOMEM;
>> + }
>> +
>> + for_each_child_of_node(np, child) {
>> + of_property_read_u32(child, "reg", values + i);
>> + i++;
>> + }
>> + mux->data.values = values;
>> +
>> + if (of_property_read_u32(np, "idle-state", &mux->data.idle))
>> + mux->data.idle = I2C_MUX_REG_NO_IDLE;
>> +
>> + /* map address from "reg" if exists */
>> + if (!of_address_to_resource(np, 0, &res)) {
>> + mux->data.reg = devm_ioremap_resource(&pdev->dev, &res);
>> + if (IS_ERR(mux->data.reg))
>> + return PTR_ERR(mux->data.reg);
>> + }
>> +
>> + return 0;
>> +}
>> +#else
>> +static int i2c_mux_reg_probe_dt(struct gpiomux *mux,
>> + struct platform_device *pdev)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> +static int i2c_mux_reg_probe(struct platform_device *pdev)
>> +{
>> + struct regmux *mux;
>> + struct i2c_adapter *parent;
>> + struct resource *res;
>> + int (*deselect)(struct i2c_adapter *, void *, u32);
>> + unsigned int initial_state, class;
>> + int i, ret, nr;
>> +
>> + mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
>> + if (!mux)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, mux);
>> +
>> + if (dev_get_platdata(&pdev->dev)) {
>> + memcpy(&mux->data, dev_get_platdata(&pdev->dev),
>> + sizeof(mux->data));
>> +
>> + parent = i2c_get_adapter(mux->data.parent);
>> + if (!parent) {
>> + dev_err(&pdev->dev, "Parent adapter (%d) not found\n",
>> + mux->data.parent);
>> + return -EPROBE_DEFER;
>> + }
>> + mux->parent = parent;
>> + i2c_put_adapter(parent);
>
> You should only give up this reference in remove() function of your driver.
>
>> + } else {
>> + ret = i2c_mux_reg_probe_dt(mux, pdev);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "Cannot locate device tree");
>> + return ret;
>> + }
>> + }
>> +
>> + if (!mux->data.reg) {
>> + dev_info(&pdev->dev,
>> + "Register not set, using platform resource\n");
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + mux->data.reg = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(mux->data.reg))
>> + return PTR_ERR(mux->data.reg);
>> + }
>> +
>> + mux->adap = devm_kzalloc(&pdev->dev,
>> + sizeof(*mux->adap) * mux->data.n_values,
>> + GFP_KERNEL);
>> + if (!mux->adap) {
>> + dev_err(&pdev->dev, "Cannot allocate i2c_adapter structure");
>> + return -ENOMEM;
>> + }
>> +
>> + if (mux->data.idle != I2C_MUX_REG_NO_IDLE) {
>> + initial_state = mux->data.idle;
>> + deselect = i2c_mux_reg_deselect;
>> + } else {
>> + initial_state = mux->data.values[0];
>> + deselect = NULL;
>> + }
>> +
>> + for (i = 0; i < mux->data.n_values; i++) {
>> + nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
>> + class = mux->data.classes ? mux->data.classes[i] : 0;
>> +
>> + mux->adap[i] = i2c_add_mux_adapter(mux->parent, &pdev->dev, mux,
>> + nr, mux->data.values[i],
>> + class, i2c_mux_reg_select,
>> + deselect);
>> + if (!mux->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",
>> + mux->data.n_values, mux->parent->name);
>
> This could be dev_dbg() also, IMHO, as this information has very little value.
Sure.
>
>> +
>> + return 0;
>> +
>> +add_adapter_failed:
>> + for (; i > 0; i--)
>> + i2c_del_mux_adapter(mux->adap[i - 1]);
>> +
>> + return ret;
>> +}
>> +
>> +static int i2c_mux_reg_remove(struct platform_device *pdev)
>> +{
>> + struct regmux *mux = platform_get_drvdata(pdev);
>> + int i;
>> +
>> + for (i = 0; i < mux->data.n_values; i++)
>> + i2c_del_mux_adapter(mux->adap[i]);
>> +
>> + i2c_put_adapter(mux->parent);
>> +
>> + dev_info(&pdev->dev, "Removed\n");
>
> I would say, only dev_dbg() is allowed here, otherwise it's a noise.
>
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id i2c_mux_reg_of_match[] = {
>> + { .compatible = "i2c-mux-reg", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, i2c_mux_reg_of_match);
>> +
>> +static struct platform_driver i2c_mux_reg_driver = {
>> + .probe = i2c_mux_reg_probe,
>> + .remove = i2c_mux_reg_remove,
>> + .driver = {
>> + .owner = THIS_MODULE,
>> + .name = "i2c-mux-reg",
>> + },
>> +};
>> +
>> +module_platform_driver(i2c_mux_reg_driver);
>> +
>> +MODULE_DESCRIPTION("Register-based I2C multiplexer driver");
>> +MODULE_AUTHOR("York Sun <[email protected]>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:i2c-mux-reg");
>> diff --git a/drivers/i2c/muxes/i2c-mux-reg.h b/drivers/i2c/muxes/i2c-mux-reg.h
>> new file mode 100644
>> index 0000000..e213988
>> --- /dev/null
>> +++ b/drivers/i2c/muxes/i2c-mux-reg.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * I2C multiplexer using a single register
>> + *
>> + * Copyright 2015 Freescale Semiconductor
>> + * York Sun <[email protected]>
>> + *
>> + * 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_MUX_REG_H
>> +#define __LINUX_I2C_MUX_REG_H
>> +
>> +/* MUX has no specific idel mode */
> ^^^^
>
>> +#define I2C_MUX_REG_NO_IDLE ((unsigned)-1)
>
> What if the idle state is exactly "all ones"? Quite important corner case actually...
Good point. If all values are considered possible, I guess the solution will be
adding another "valid" variable to vouch for this one.
>
>> +
>> +/**
>> + * struct i2c_mux_reg_platform_data - Platform-dependent data for i2c-mux-reg
>> + * @parent: Parent I2C bus adapter number
>> + * @base_nr: Base I2C bus number to number adapters from or zero for dynamic
>> + * @values: Array of value for each channel
>> + * @n_values: Number of multiplexer channels
>> + * @classes: Optional I2C auto-detection classes
>> + * @idle: Value to write to mux when idle
>> + * @reg: Virtual address of the register to switch channel
>> + */
>> +struct i2c_mux_reg_platform_data {
>> + int parent;
>> + int base_nr;
>> + const unsigned int *values;
>> + int n_values;
>> + const unsigned int *classes;
>> + unsigned int idle;
>> + unsigned int *reg;
>
> Yeah, this is really bad idea. You maybe want something like
> __iomem "cookie" here instead of this bare pointer.
Let me try.
>
>> +};
>> +
>> +#endif /* __LINUX_I2C_MUX_REG_H */
>>
>
> Other than the mentioned above, this is a useful code.
>
Thanks for the encouragement. I will send an update soon.
York
Hi!
On 17/06/15 18:03, ext Paul Bolle wrote:
>> You do not see the platform_device, because there are no users yet, put
>> > this MODULE_ALIAS() is perfectly fine, it will allow automatic module loading
>> > in non-DT case.
> Do you mean that it will allow automatic module loading once the patch
> that adds a struct platform_device with a "i2c-mux-reg" name lands?
Any platform code which will register the platform_device will trigger uevent and
udevd will be able to find the module with this macro. This is a legacy alternative
to device-tree approach.
--
Best regards,
Alexander Sverdlin.
Hello!
On 17/06/15 18:43, ext York Sun wrote:
>> > Yeah, this is really bad idea. You maybe want something like
>> > __iomem "cookie" here instead of this bare pointer.
> Let me try.
>
Could you think about different access widths, please?
Not all buses are 32-bits wide and even on 64-bit CPUs one might have 16-bit bus
and 32 bits accesses are not allowed or perform two accesses, etc...
So to cover the use-cases which I see one needs to have a possibility to select between
__raw_writeb()/__raw_writew()/__raw_writel()/__raw_writeq() (now that I'm thinking about
it, I think these native-Endianness functions are preferred and if one has a bus with
different Endianness he should think about the conversion in the reg property of subnodes).
Very important is readback with corresponding __raw_read*(), but maybe you want to do
this optional via additional DT property...
--
Best regards,
Alexander Sverdlin.
Hi Alexander,
On Thu, 2015-06-18 at 09:40 +0200, Alexander Sverdlin wrote:
> On 17/06/15 18:03, ext Paul Bolle wrote:
> >> You do not see the platform_device, because there are no users yet, put
> >> > this MODULE_ALIAS() is perfectly fine, it will allow automatic module loading
> >> > in non-DT case.
> > Do you mean that it will allow automatic module loading once the patch
> > that adds a struct platform_device with a "i2c-mux-reg" name lands?
>
> Any platform code which will register the platform_device will trigger uevent and
> udevd will be able to find the module with this macro. This is a legacy alternative
> to device-tree approach.
That means I've correctly figured out the purpose of this
MODULE_ALIAS("platform:" stuff. Because it might actually be documented
somewhere but I managed to not stumble on that documentation.
With that out of the way: am I right in thinking there's currently no
platform code that triggers that uevent for
"MODALIAS=platform:i2c-mux-reg"? Because if there's no struct
platform_device taking care of that I think this MODULE_ALIAS() should
not be added, not yet.
Thanks,
Paul Bolle
Hi!
On 18/06/15 10:08, ext Paul Bolle wrote:
>>>> You do not see the platform_device, because there are no users yet, put
>>>>> > >> > this MODULE_ALIAS() is perfectly fine, it will allow automatic module loading
>>>>> > >> > in non-DT case.
>>> > > Do you mean that it will allow automatic module loading once the patch
>>> > > that adds a struct platform_device with a "i2c-mux-reg" name lands?
>> >
>> > Any platform code which will register the platform_device will trigger uevent and
>> > udevd will be able to find the module with this macro. This is a legacy alternative
>> > to device-tree approach.
> That means I've correctly figured out the purpose of this
> MODULE_ALIAS("platform:" stuff. Because it might actually be documented
> somewhere but I managed to not stumble on that documentation.
>
> With that out of the way: am I right in thinking there's currently no
> platform code that triggers that uevent for
> "MODALIAS=platform:i2c-mux-reg"? Because if there's no struct
> platform_device taking care of that I think this MODULE_ALIAS() should
> not be added, not yet.
Maybe (and hopefully) there will never be a legacy user of this driver. But this macro
is perfectly fine, adds no overhead (but modinfo) and make the module "complete" in a
sense that it supports both types of binding. There is a legacy probe function in it,
all the support for legacy binding with platform_data in it and this modalias is simply
the last part of it.
--
Best regards,
Alexander Sverdlin.
On Thu, 2015-06-18 at 11:04 +0200, Alexander Sverdlin wrote:
> Maybe (and hopefully) there will never be a legacy user of this driver. But this macro
> is perfectly fine, adds no overhead (but modinfo)
(The test for any line of code is whether it adds value. Adding no
overhead is not adding value. That goes for one line macros too.)
> and make the module "complete" in a
> sense that it supports both types of binding. There is a legacy probe function in it,
> all the support for legacy binding with platform_data in it and this modalias is simply
> the last part of it.
As I understand it, we've established that:
- this macro setups up udev, and modprobe, and friends, to listen to a
"MODALIAS=platform:i2c-mux-reg" message; and
- that there's currently no code in the kernel that will send out this
message.
Did I get that right? Because it follows from the above that this line
serves no purpose. Worse, it makes the code actually confusing. Because
it suggests the availability of functionality that in reality doesn't
exist.
Thanks,
Paul Bolle
Hi!
On 18/06/15 11:32, ext Paul Bolle wrote:
> As I understand it, we've established that:
> - this macro setups up udev, and modprobe, and friends, to listen to a
> "MODALIAS=platform:i2c-mux-reg" message; and
> - that there's currently no code in the kernel that will send out this
> message.
>
> Did I get that right? Because it follows from the above that this line
> serves no purpose. Worse, it makes the code actually confusing. Because
Are you kidding?
> it suggests the availability of functionality that in reality doesn't
> exist.
Most of the usual drivers in the Kernel have this line. Until now no animal
was hurt with it.
--
Best regards,
Alexander Sverdlin.
On Thu, 2015-06-18 at 11:42 +0200, Alexander Sverdlin wrote:
> Most of the usual drivers in the Kernel have this line.
Perhaps people copy and paste without really realizing what this macro
is supposed to be for. (I didn't know until a few days ago. I needed you
to confirm that I figured it out correctly.) Perhaps reviewers skip over
the boilerplate stuff.
> Until now no animal was hurt with it.
Sure, no overhead and all that. No value too.
Thanks,
Paul Bolle
On Tue, Jun 16, 2015 at 10:28:12AM -0700, York Sun wrote:
> Based on i2c-mux-gpio driver, similarly the register based mux
> switch from one bus to another by setting a single register.
> The register can be on PCIe bus, local bus, or any memory-mapped
> address.
>
> Signed-off-by: York Sun <[email protected]>
> CC: Wolfram Sang <[email protected]>
> CC: Peter Korsgaard <[email protected]>
Mostly good.
> +static int i2c_mux_reg_probe(struct platform_device *pdev)
> +{
> + struct regmux *mux;
> + struct i2c_adapter *parent;
> + struct resource *res;
> + int (*deselect)(struct i2c_adapter *, void *, u32);
> + unsigned int initial_state, class;
gcc says:
drivers/i2c/muxes/i2c-mux-reg.c:182:15: warning: variable 'initial_state' set but not used [-Wunused-but-set-variable]
It seens you prepared for setting the initial state but don't do the
actual set?
> +static struct platform_driver i2c_mux_reg_driver = {
> + .probe = i2c_mux_reg_probe,
> + .remove = i2c_mux_reg_remove,
> + .driver = {
> + .owner = THIS_MODULE,
coccicheck says:
drivers/i2c/muxes/i2c-mux-reg.c:288:3-8: No need to set .owner here. The core will do it.
> + .name = "i2c-mux-reg",
> + },
> +};
Thanks,
Wolfram
On 08/15/2015 01:23 PM, Wolfram Sang wrote:
> On Tue, Jun 16, 2015 at 10:28:12AM -0700, York Sun wrote:
>> Based on i2c-mux-gpio driver, similarly the register based mux
>> switch from one bus to another by setting a single register.
>> The register can be on PCIe bus, local bus, or any memory-mapped
>> address.
>>
>> Signed-off-by: York Sun <[email protected]>
>> CC: Wolfram Sang <[email protected]>
>> CC: Peter Korsgaard <[email protected]>
>
> Mostly good.
>
>> +static int i2c_mux_reg_probe(struct platform_device *pdev)
>> +{
>> + struct regmux *mux;
>> + struct i2c_adapter *parent;
>> + struct resource *res;
>> + int (*deselect)(struct i2c_adapter *, void *, u32);
>> + unsigned int initial_state, class;
>
> gcc says:
>
> drivers/i2c/muxes/i2c-mux-reg.c:182:15: warning: variable 'initial_state' set but not used [-Wunused-but-set-variable]
>
> It seens you prepared for setting the initial state but don't do the
> actual set?
Thanks for catching this. I set the initial state variable but used another
variable when it is actually used. Kernel Makefile disables
unused-but-set-variable by default. How did you enable this warning without
being flooded by the warnings? (I tried W=1)
>
>> +static struct platform_driver i2c_mux_reg_driver = {
>> + .probe = i2c_mux_reg_probe,
>> + .remove = i2c_mux_reg_remove,
>> + .driver = {
>> + .owner = THIS_MODULE,
>
> coccicheck says:
>
> drivers/i2c/muxes/i2c-mux-reg.c:288:3-8: No need to set .owner here. The core will do it.
Will drop it in next version.
York
> unused-but-set-variable by default. How did you enable this warning without
> being flooded by the warnings? (I tried W=1)
I use W=1. To prevent flooding, I used to replace all -I with -isystem
in the Kernel Makefile, but sadly this doesn't work anymore. And I had
no time to investigate why.