Message-ID: <5581A3CA.4050805@freescale.com>
Date:	Wed, 17 Jun 2015 09:43:54 -0700
From:	York Sun <yorksun@...escale.com>
To:	Alexander Sverdlin <alexander.sverdlin@...ia.com>,
	<wsa@...-dreams.de>
CC:	<linux-kernel@...r.kernel.org>, <linux-i2c@...r.kernel.org>,
	Peter Korsgaard <peter.korsgaard@...co.com>
Subject: Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg



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 <yorksun@...escale.com>
>> CC: Wolfram Sang <wsa@...-dreams.de>
>> CC: Peter Korsgaard <peter.korsgaard@...co.com>
>> ---
>>  .../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  <yorksun@...escale.com>
>> + *
>> + * 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 <yorksun@...escale.com>");
>> +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 <yorksun@...escale.com>
>> + *
>> + * 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists