[RFC PATCH] add slave mode support to tegra's i2c controller

[RFC PATCH] add slave mode support to tegra's i2c controller

[Marc Dietrich]

Hi,

the patch below adds slave mode to tegra's i2c controller. I tried to make it as 
small as possible. It is needed to allow the nvec driver to deligate the 
initialization to the i2c-tegra driver as part of its cleanup. Patch is tested 
with our version of nvec. Patch is against arm-soc + boards-3.2. Please review 
and consider it for merge. 

Thanks

Marc

===

    ARM: tegra: add slave mode support to tegra's i2c controller
    
    This adds support for tegra's i2c-controller slave mode operation.
    It programs the slave address provided by the platform data and skips
    the interrupt registration which is deligated to the master device
    driver. Additionaly, the i2c device structure definition is moved to
    the header file, so it can be used by the master device driver.

	Signed-off-by: Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 2440b74..b4c23d5 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -99,45 +99,6 @@
 #define I2C_HEADER_MASTER_ADDR_SHIFT		12
 #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
 
-/**
- * struct tegra_i2c_dev	- per device i2c context
- * @dev: device reference for power management
- * @adapter: core i2c layer adapter information
- * @clk: clock reference for i2c controller
- * @i2c_clk: clock reference for i2c bus
- * @iomem: memory resource for registers
- * @base: ioremapped registers cookie
- * @cont_id: i2c controller id, used for for packet header
- * @irq: irq number of transfer complete interrupt
- * @is_dvc: identifies the DVC i2c controller, has a different register layout
- * @msg_complete: transfer completion notifier
- * @msg_err: error code for completed message
- * @msg_buf: pointer to current message data
- * @msg_buf_remaining: size of unsent data in the message buffer
- * @msg_read: identifies read transfers
- * @bus_clk_rate: current i2c bus clock rate
- * @is_suspended: prevents i2c controller accesses after suspend is called
- */
-struct tegra_i2c_dev {
-	struct device *dev;
-	struct i2c_adapter adapter;
-	struct clk *clk;
-	struct clk *i2c_clk;
-	struct resource *iomem;
-	void __iomem *base;
-	int cont_id;
-	int irq;
-	bool irq_disabled;
-	int is_dvc;
-	struct completion msg_complete;
-	int msg_err;
-	u8 *msg_buf;
-	size_t msg_buf_remaining;
-	int msg_read;
-	unsigned long bus_clk_rate;
-	bool is_suspended;
-};
-
 static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long 
reg)
 {
 	writel(val, i2c_dev->base + reg);
@@ -322,6 +283,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 {
 	u32 val;
 	int err = 0;
+	int slave_addr = i2c_dev->is_slave ? 0xfc : i2c_dev->slave_addr;
 
 	clk_enable(i2c_dev->clk);
 
@@ -342,8 +304,8 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 		u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
 		sl_cfg |= I2C_SL_CNFG_NACK | I2C_SL_CNFG_NEWSL;
 		i2c_writel(i2c_dev, sl_cfg, I2C_SL_CNFG);
-		i2c_writel(i2c_dev, 0xfc, I2C_SL_ADDR1);
-		i2c_writel(i2c_dev, 0x00, I2C_SL_ADDR2);
+		i2c_writel(i2c_dev, slave_addr & 0xff, I2C_SL_ADDR1);
+		i2c_writel(i2c_dev, slave_addr >> 8, I2C_SL_ADDR2);
 
 	}
 
@@ -609,6 +571,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->bus_clk_rate = 100000; /* default clock rate */
 	if (pdata) {
 		i2c_dev->bus_clk_rate = pdata->bus_clk_rate;
+		i2c_dev->is_slave = pdata->is_slave;
+		i2c_dev->slave_addr = pdata->slave_addr;
 
 	} else if (i2c_dev->dev->of_node) {    /* if there is a device tree node ... 
*/
 		prop = of_get_property(i2c_dev->dev->of_node,
@@ -629,10 +593,12 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		goto err_free;
 	}
 
-	ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
-		goto err_free;
+	if (!i2c_dev->is_slave) {
+		ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
+			goto err_free;
+		}
 	}
 
 	clk_enable(i2c_dev->i2c_clk);
diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
index 9c85da4..2729863 100644
--- a/include/linux/i2c-tegra.h
+++ b/include/linux/i2c-tegra.h
@@ -18,8 +18,55 @@
 #ifndef _LINUX_I2C_TEGRA_H
 #define _LINUX_I2C_TEGRA_H
 
+#include <linux/i2c.h>
+
 struct tegra_i2c_platform_data {
 	unsigned long bus_clk_rate;
+	bool is_slave;
+	u16 slave_addr;
+};
+
+/**
+ * struct tegra_i2c_dev	- per device i2c context
+ * @dev: device reference for power management
+ * @adapter: core i2c layer adapter information
+ * @clk: clock reference for i2c controller
+ * @i2c_clk: clock reference for i2c bus
+ * @iomem: memory resource for registers
+ * @base: ioremapped registers cookie
+ * @cont_id: i2c controller id, used for for packet header
+ * @irq: irq number of transfer complete interrupt
+ * @is_dvc: identifies the DVC i2c controller, has a different register layout
+ * @is_slave: identifies a controller in slave mode operation
+ * @slave_addr: the slave address if in slave mode
+ * @msg_complete: transfer completion notifier
+ * @msg_err: error code for completed message
+ * @msg_buf: pointer to current message data
+ * @msg_buf_remaining: size of unsent data in the message buffer
+ * @msg_read: identifies read transfers
+ * @bus_clk_rate: current i2c bus clock rate
+ * @is_suspended: prevents i2c controller accesses after suspend is called
+ */
+struct tegra_i2c_dev {
+	struct device *dev;
+	struct i2c_adapter adapter;
+	struct clk *clk;
+	struct clk *i2c_clk;
+	struct resource *iomem;
+	void __iomem *base;
+	int cont_id;
+	int irq;
+	u16 slave_addr;
+	bool irq_disabled;
+	bool is_dvc;
+	bool is_slave;
+	struct completion msg_complete;
+	int msg_err;
+	u8 *msg_buf;
+	size_t msg_buf_remaining;
+	int msg_read;
+	unsigned long bus_clk_rate;
+	bool is_suspended;
 };
 
 #endif /* _LINUX_I2C_TEGRA_H */

RE: [RFC PATCH] add slave mode support to tegra's i2c controller

[Stephen Warren]

Marc Dietrich wrote at Wednesday, August 17, 2011 1:01 PM:
> the patch below adds slave mode to tegra's i2c controller. I tried to make it as
> small as possible. It is needed to allow the nvec driver to deligate the
> initialization to the i2c-tegra driver as part of its cleanup. Patch is tested
> with our version of nvec. Patch is against arm-soc + boards-3.2. Please review
> and consider it for merge.

Forgive my lack of familiarity with the Tegra HW, but is the slave
controller completely autonomous? I'm curious what data slave transactions
access, since there's no actual code here to respond to transactions
directed at the slave address and provide data in response.

Or put another way, is this patch really adding slave mode support, or
is it just providing a way to program the slave controller's I2C address?

Some comments below.

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
...
> -struct tegra_i2c_dev {
> -	struct device *dev;
> -	struct i2c_adapter adapter;
...
> -};

Nothing got added that needed that struct definition in a header file;
was there meant to be a new file included in the patch that implements
the slave driver (and makefile and perhaps Kconfig changes?)

> +	int slave_addr = i2c_dev->is_slave ? 0xfc : i2c_dev->slave_addr;

Isn't that backwards; don't you want to hard-code the slave address as 0xfc only
when the controller isn't intended to be used in slave mode?

>  	i2c_dev->bus_clk_rate = 100000; /* default clock rate */
>  	if (pdata) {
>  		i2c_dev->bus_clk_rate = pdata->bus_clk_rate;
> +		i2c_dev->is_slave = pdata->is_slave;
> +		i2c_dev->slave_addr = pdata->slave_addr;

Maybe store a pointer to pdata itself rather than having to copy this
manually; it'll be one less thing to worry about when adding fields to
pdata.

...
> -	ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> -		goto err_free;
> +	if (!i2c_dev->is_slave) {
> +		ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> +			goto err_free;
> +		}
>  	}

This appears to make master and slave mode mutually exclusive. Doesn't
the HW allow use of both controllers on the same bus?

-- 
nvpublic

Re: [RFC PATCH] add slave mode support to tegra's i2c controller

[Marc Dietich]

Hi Stephen,

> Marc Dietrich wrote at Wednesday, August 17, 2011 1:01 PM:
> > the patch below adds slave mode to tegra's i2c controller. I tried to
> > make it as small as possible. It is needed to allow the nvec driver to
> > deligate the initialization to the i2c-tegra driver as part of its
> > cleanup. Patch is tested with our version of nvec. Patch is against
> > arm-soc + boards-3.2. Please review and consider it for merge.
> 
> Forgive my lack of familiarity with the Tegra HW, but is the slave
> controller completely autonomous? I'm curious what data slave transactions
> access, since there's no actual code here to respond to transactions
> directed at the slave address and provide data in response.
> 
> Or put another way, is this patch really adding slave mode support, or
> is it just providing a way to program the slave controller's I2C address?

well, documentation still hasn't spontanously materialized here, so I cannot 
answer you questions regarding hw. What I understood is that slave and master 
sit on the same bus, so either of them needs to be shut down. Sorry for my poor 
wording.

We (the AC100 team) wrote a simple driver for dealing with the communication. 
You may find the latest version at 
http://gitorious.org/~marvin24/ac100/marvin24s-kernel/trees/chromeos-
ac100-2.6.38/drivers/staging/nvec which I'm trying to push to mainline kernel. 
The changes to i2c-tegra are required to make some progress on our side. I'm 
also planing to reuse more of i2c-tegra init stuff.

> Some comments below.
> 
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c
> 
> ...
> 
> > -struct tegra_i2c_dev {
> > -	struct device *dev;
> > -	struct i2c_adapter adapter;
> 
> ...
> 
> > -};
> 
> Nothing got added that needed that struct definition in a header file;
> was there meant to be a new file included in the patch that implements
> the slave driver (and makefile and perhaps Kconfig changes?)

it is added, but not to i2c-tegra, but nvec.c in drivers/staging/nvec. I think 
it's better to split these functions because a master can be anything so there 
is no common protocol which can be implemented in i2c-tegra.

> 
> > +	int slave_addr = i2c_dev->is_slave ? 0xfc : i2c_dev->slave_addr;
> 
> Isn't that backwards; don't you want to hard-code the slave address as 0xfc
> only when the controller isn't intended to be used in slave mode?

Doh, yes. I think I just didn't noticed in my testing because we still reprogram 
the slave address in our init (which should be removed in the near future).

> >  	i2c_dev->bus_clk_rate = 100000; /* default clock rate */
> >  	if (pdata) {
> >  	
> >  		i2c_dev->bus_clk_rate = pdata->bus_clk_rate;
> > 
> > +		i2c_dev->is_slave = pdata->is_slave;
> > +		i2c_dev->slave_addr = pdata->slave_addr;
> 
> Maybe store a pointer to pdata itself rather than having to copy this
> manually; it'll be one less thing to worry about when adding fields to
> pdata.

well, pdata is normally freed after init so we cannot use it. The current code 
in chromeos also adds a slave_addr so I just bodly added it here. It won't have 
any effect on the board files because all field are normally initilized with 
zeros which is a fine default.

> 
> ...
> 
> > -	ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> > -		goto err_free;
> > +	if (!i2c_dev->is_slave) {
> > +		ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name,
> > i2c_dev); +		if (ret) {
> > +			dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> > +			goto err_free;
> > +		}
> > 
> >  	}
> 
> This appears to make master and slave mode mutually exclusive. Doesn't
> the HW allow use of both controllers on the same bus?

I cannot think of how both modes can exist at the same time (maybe isr can 
detect what was meant), but well, as I said already, I'm also not familar with 
the hw. But you may be right that the i2c master mode should be disabled somehow 
if we are in slave mode (like slave is normally disabled via I2C_SL_CNFG_NACK). 

Thanks

Marc

RE: [RFC PATCH] add slave mode support to tegra's i2c controller

[Stephen Warren]

Marc Dietich wrote at Thursday, August 18, 2011 1:49 AM:
> Hi Stephen,
> 
> > Marc Dietrich wrote at Wednesday, August 17, 2011 1:01 PM:
> > > the patch below adds slave mode to tegra's i2c controller. I tried to
> > > make it as small as possible. It is needed to allow the nvec driver to
> > > deligate the initialization to the i2c-tegra driver as part of its
> > > cleanup. Patch is tested with our version of nvec. Patch is against
> > > arm-soc + boards-3.2. Please review and consider it for merge.
> >
> > Forgive my lack of familiarity with the Tegra HW, but is the slave
> > controller completely autonomous? I'm curious what data slave transactions
> > access, since there's no actual code here to respond to transactions
> > directed at the slave address and provide data in response.
> >
> > Or put another way, is this patch really adding slave mode support, or
> > is it just providing a way to program the slave controller's I2C address?
> 
> well, documentation still hasn't spontanously materialized here, so I cannot
> answer you questions regarding hw. What I understood is that slave and master
> sit on the same bus, so either of them needs to be shut down. Sorry for my poor
> wording.

If this patch goes ahead, I think split it out into logically separate
patches, and them something like:

i2c/tegra: Allow configuration of slave address
i2c/tegra: Allow tegra_i2c_dev to be used by nvec/slave driver

> We (the AC100 team) wrote a simple driver for dealing with the communication.
> You may find the latest version at
> http://gitorious.org/~marvin24/ac100/marvin24s-kernel/trees/chromeos-
> ac100-2.6.38/drivers/staging/nvec which I'm trying to push to mainline kernel.
> The changes to i2c-tegra are required to make some progress on our side. I'm
> also planing to reuse more of i2c-tegra init stuff.

It looks like part of the nvec code (e.g. i2c_interrupt()) is simply a
driver for the slave I2C controller; this should really be moved into
drivers/i2c/busses/i2c-tegra.c rather than exposing the internals of that
driver for use by the nvec driver. The nvec driver could then wrap this
and implement the GPIO-related logic and higher-level protocol.

Yes, it's entirely possible our various product kernels don't do this
correctly, and may not be a good model to follow. I'm haven't taken an
in depth look at them to determine either way though.

> > Some comments below.
> >
> > > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > > b/drivers/i2c/busses/i2c-tegra.c
> > ...
> > > -struct tegra_i2c_dev {
> > > -	struct device *dev;
> > > -	struct i2c_adapter adapter;
> >
> > ...
> >
> > > -};
> >
> > Nothing got added that needed that struct definition in a header file;
> > was there meant to be a new file included in the patch that implements
> > the slave driver (and makefile and perhaps Kconfig changes?)
> 
> it is added, but not to i2c-tegra, but nvec.c in drivers/staging/nvec.

Ah.

> I think
> it's better to split these functions because a master can be anything so there
> is no common protocol which can be implemented in i2c-tegra.

If we do end up implementing separate drivers for the I2C master and slave,
whether the slave is part of nvec or in drivers/i2c, I still don't think
it's a good idea to share tegra_i2c_dev; if the drivers are truly separate,
then they can have their own device structure. If they're common enough
that sharing the device structure is useful, it seems like the two drivers
should be fully unified.

...
> > >  	i2c_dev->bus_clk_rate = 100000; /* default clock rate */
> > >  	if (pdata) {
> > >
> > >  		i2c_dev->bus_clk_rate = pdata->bus_clk_rate;
> > >
> > > +		i2c_dev->is_slave = pdata->is_slave;
> > > +		i2c_dev->slave_addr = pdata->slave_addr;
> >
> > Maybe store a pointer to pdata itself rather than having to copy this
> > manually; it'll be one less thing to worry about when adding fields to
> > pdata.
> 
> well, pdata is normally freed after init so we cannot use it.

Good point.

> > ...
> >
> > > -	ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
> > > -	if (ret) {
> > > -		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> > > -		goto err_free;
> > > +	if (!i2c_dev->is_slave) {
> > > +		ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
> > > +		if (ret) {
> > > +			dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> > > +			goto err_free;
> > > +		}
> > >  	}
> >
> > This appears to make master and slave mode mutually exclusive. Doesn't
> > the HW allow use of both controllers on the same bus?
> 
> I cannot think of how both modes can exist at the same time (maybe isr can
> detect what was meant), but well, as I said already, I'm also not familar with
> the hw. But you may be right that the i2c master mode should be disabled somehow
> if we are in slave mode (like slave is normally disabled via I2C_SL_CNFG_NACK).

I don't see any HW reason why Tegra's master and slave I2C controllers
could not both be usable at the same time on the same bus. At least, not
coming from a pure I2C background; perhaps there is some reason in Tegra
HW, but given a quick look at the registers that the two drivers use, I
don't see any problematic overlaps, except during initialization.

The primary reason the SW wouldn't allow both devices to be used at once
is that there are separate master/slave drivers which both need to handle
the same interrupt. If the drivers were unified (or at least a shared
common core created), then I think there wouldn't be any issue having both
drivers active at once.

-- 
nvpublic

Re: [RFC PATCH] add slave mode support to tegra's i2c controller

[Marc Dietrich]

On Monday 22 August 2011 21:23:52 Stephen Warren wrote:
> Marc Dietich wrote at Thursday, August 18, 2011 1:49 AM:
> > Hi Stephen,
> > 
> > > Marc Dietrich wrote at Wednesday, August 17, 2011 1:01 PM:
> > > > the patch below adds slave mode to tegra's i2c controller. I tried 
to
> > > > make it as small as possible. It is needed to allow the nvec driver
> > > > to deligate the initialization to the i2c-tegra driver as part of
> > > > its cleanup. Patch is tested with our version of nvec. Patch is
> > > > against arm-soc + boards-3.2. Please review and consider it for
> > > > merge.
> > > > [...]
>
> If this patch goes ahead, I think split it out into logically separate
> patches, and them something like:
> 
> i2c/tegra: Allow configuration of slave address
> i2c/tegra: Allow tegra_i2c_dev to be used by nvec/slave driver

ok, but lets see...
 
> > We (the AC100 team) wrote a simple driver for dealing with the
> > communication. You may find the latest version at
> > http://gitorious.org/~marvin24/ac100/marvin24s-kernel/trees/chromeos-
> > ac100-2.6.38/drivers/staging/nvec which I'm trying to push to mainline
> > kernel. The changes to i2c-tegra are required to make some progress on
> > our side. I'm also planing to reuse more of i2c-tegra init stuff.
> 
> It looks like part of the nvec code (e.g. i2c_interrupt()) is simply a
> driver for the slave I2C controller; this should really be moved into
> drivers/i2c/busses/i2c-tegra.c rather than exposing the internals of that
> driver for use by the nvec driver. The nvec driver could then wrap this
> and implement the GPIO-related logic and higher-level protocol.

I agree that exposing the internal data structures is not the best way to 
go. So lets see were the alternatives are. 

First, I definitely don't want to add our interrupt code to i2c-tegra. 
Partly because the ISR is specific to the master/slave protocol used, 
partly because I cannot do a clean implementation myself (no docu, no time, 
no experience).

Second, I aggree that we should use as much of i2c-tegra's code as possible 
(assuming the i2c-tegra maintainers support this idea). This way we may 
also get rid for the need of the internal data structure.

i2c-tegra uses its own readl and writel functions which we could use (thus 
getting rid of knowing the base address) but we would need some kind of 
handle (e.g. a pointer to i2c_dev). Such a handle could also be used to get 
the other required info for the ISR (irq, clock and slave address). This 
could be implemented relative fast without much collateral damage.

A clean solution (longterm) on the other hand would try to split out the 
master/slave protocol out of the ISR and make it as minimal as possible, 
leaving the protocol to the nvec driver. I'm not sure if this is possible 
or would work at all. 

> Yes, it's entirely possible our various product kernels don't do this
> correctly, and may not be a good model to follow. I'm haven't taken an
> in depth look at them to determine either way though.
> 
> > > Some comments below.
> > > 
> > > > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > > > b/drivers/i2c/busses/i2c-tegra.c
> > > 
> > > ...
> > > 
> > > > -struct tegra_i2c_dev {
> > > > -	struct device *dev;
> > > > -	struct i2c_adapter adapter;
> > > 
> > > ...
> > > 
> > > > -};
> > > 
> > > Nothing got added that needed that struct definition in a header 
file;
> > > was there meant to be a new file included in the patch that 
implements
> > > the slave driver (and makefile and perhaps Kconfig changes?)
> > 
> > it is added, but not to i2c-tegra, but nvec.c in drivers/staging/nvec.
> 
> Ah.
> 
> > I think
> > it's better to split these functions because a master can be anything 
so
> > there is no common protocol which can be implemented in i2c-tegra.
> 
> If we do end up implementing separate drivers for the I2C master and 
slave,
> whether the slave is part of nvec or in drivers/i2c, I still don't think
> it's a good idea to share tegra_i2c_dev; if the drivers are truly 
separate,
> then they can have their own device structure. If they're common enough
> that sharing the device structure is useful, it seems like the two 
drivers
> should be fully unified.

Both structures share some information (mostly the hardware info) but also 
some info related to the master or slave protocol respectively (message 
buffers, position pointers or status flags). Given that, we may also just 
split out the protocol related info out of the i2c_dev struct and share the 
latter one, which would be an alternative to the idea descriped above.

> > > ...
> > > 
> > > > -	ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name,
> > > > i2c_dev); -	if (ret) {
> > > > -		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev-
>irq);
> > > > -		goto err_free;
> > > > +	if (!i2c_dev->is_slave) {
> > > > +		ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev-
>name,
> > > > i2c_dev); +		if (ret) {
> > > > +			dev_err(&pdev->dev, "Failed to request irq %i\n", 
i2c_dev-
>irq);
> > > > +			goto err_free;
> > > > +		}
> > > >  	}
> > > 
> > > This appears to make master and slave mode mutually exclusive. 
Doesn't
> > > the HW allow use of both controllers on the same bus?
> > 
> > I cannot think of how both modes can exist at the same time (maybe isr
> > can detect what was meant), but well, as I said already, I'm also not
> > familar with the hw. But you may be right that the i2c master mode
> > should be disabled somehow if we are in slave mode (like slave is
> > normally disabled via I2C_SL_CNFG_NACK).
> 
> I don't see any HW reason why Tegra's master and slave I2C controllers
> could not both be usable at the same time on the same bus. At least, not
> coming from a pure I2C background; perhaps there is some reason in Tegra
> HW, but given a quick look at the registers that the two drivers use, I
> don't see any problematic overlaps, except during initialization.
> 
> The primary reason the SW wouldn't allow both devices to be used at once
> is that there are separate master/slave drivers which both need to handle
> the same interrupt. If the drivers were unified (or at least a shared
> common core created), then I think there wouldn't be any issue having 
both
> drivers active at once.

Well, the ISR could look for the I2C_SL_IRQ (1<<3) status flag in the 
I2C_SL_STATUS (0x28) register and branch accordingly. But again, I don't 
want to bloat the i2c-tegra code with our experimental/unstable driver. 

Any ideas on how to proceed?

	Marc

RE: [RFC PATCH] add slave mode support to tegra's i2c controller

[Stephen Warren]

Marc Dietrich wrote at Sunday, August 28, 2011 10:02 AM:
> On Monday 22 August 2011 21:23:52 Stephen Warren wrote:
> > Marc Dietich wrote at Thursday, August 18, 2011 1:49 AM:
...
> > > We (the AC100 team) wrote a simple driver for dealing with the
> > > communication. You may find the latest version at
> > > http://gitorious.org/~marvin24/ac100/marvin24s-kernel/trees/chromeos-
> > > ac100-2.6.38/drivers/staging/nvec which I'm trying to push to mainline
> > > kernel. The changes to i2c-tegra are required to make some progress on
> > > our side. I'm also planing to reuse more of i2c-tegra init stuff.
> >
> > It looks like part of the nvec code (e.g. i2c_interrupt()) is simply a
> > driver for the slave I2C controller; this should really be moved into
> > drivers/i2c/busses/i2c-tegra.c rather than exposing the internals of that
> > driver for use by the nvec driver. The nvec driver could then wrap this
> > and implement the GPIO-related logic and higher-level protocol.
...
> A clean solution (longterm) on the other hand would try to split out the
> master/slave protocol out of the ISR and make it as minimal as possible,
> leaving the protocol to the nvec driver. I'm not sure if this is possible
> or would work at all.
...
> Well, the ISR could look for the I2C_SL_IRQ (1<<3) status flag in the
> I2C_SL_STATUS (0x28) register and branch accordingly. But again, I don't
> want to bloat the i2c-tegra code with our experimental/unstable driver.
> 
> Any ideas on how to proceed?

I was assuming that the I2C core already had some infra-structure in
place for abstracting I2C slave controllers. A quick look implies that
isn't actually the case.

However, I do see that the PXA I2C driver does implement slave mode,
such that the I2C controller implements all the register twiddling,
and some callback implements the protocol required by the system. Take
a look at:

include/linux/i2c-pxa.h (struct i2c_slave_client)

drivers/i2c/busses/i2c-pxa.c (i2c_pxa_slave_*())

I'm not sure how the callback model will fit into devicetree, but PAZ00
isn't devicetree-enabled (yet) anyway, and various other drivers need
that problem solved anyway, so I wouldn't let that stop you.

I think even without HW documentation or much experience, it shouldn't be
that big a deal to take the existing slave code from the nvec staging
driver and integrate it into the existing I2C master driver. I suppose
making master vs. slave mode exclusive as a first step actually wouldn't
be so bad. I can test any such patches for regressions on Harmony,
Seaboard, and Trimslice, if you need.

-- 
nvpublic