Re: [PATCH 3/4] i2c: added I2C_AQ_NO_CLK_STRETCH to i2c-bcm2835.c

[Stephen Warren]

On 10/27/2015 02:11 PM, Nicola Corna wrote:
> As reported here
> http://www.advamation.com/knowhow/raspberrypi/rpi-i2c-bug.html
> the BCM2835 has a bug in its i2c implementation which prevents a correct
> clock stretching.

I was going to ask for some more official confirmation of this bug (e.g.
update to BCM2835_Peripherals.pdf), but it looks like it's already been
confirmed; check Gert van Loo's responses at:

https://www.raspberrypi.org/forums/viewtopic.php?p=146272

It might be useful to add this link into the patch description.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] dt: paz00: define nvec as child of i2c bus

[Stephen Warren]

On 07/20/2015 02:35 PM, Andrey Danin wrote:

NVEC driver was reimplemented to use tegra i2c. Use common i2c bindings
for NVEC node.



diff --git a/arch/arm/boot/dts/tegra20-paz00.dts 
b/arch/arm/boot/dts/tegra20-paz00.dts



+   nvec: nvec@45 {
+   compatible = "nvidia,nvec-slave";
+   reg = <0x45>;


I think you need to or in I2C_OWN_SLAVE_ADDRESS from 
 here?

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] staging/nvec: reimplement on top of tegra i2c driver

[Stephen Warren]

On 07/20/2015 02:35 PM, Andrey Danin wrote:

Remove i2c controller related code and use tegra i2c driver in slave mode.
Update nvec documentation.



diff --git a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt 
b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt


I would expect this patch to add a new binding file 
nvidia,nvec-slave.txt so that the filename continues to match the 
compatible value it documents. This patch introduces a new binding for 
the NVEC slave as opposed to modifying the existing binding.



+- compatible : should be "nvidia,nvec-slave".
+- reg: the i2c address of the slave controller


I think "the I2C address to respond to" would be clearer? You might also 
mention that this needs to include flags from .

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/9] i2c: slave: improve i2c client address spaces and their DT support

[Stephen Warren]

On 07/17/2015 08:08 AM, Wolfram Sang wrote:

As promised here is my RFC to improve address spaces for I2C. This should give
i2c seperate address spaces for standard clients, 10 bit clients, and our own
slave clients. So, you can now have a 7 bit slave at 0x50 and a 10 bit slave at
0x050. Or, you can have a slave driver listening at some address and at the
same time have a client driver talking to this address. Note that this is only
the core support for that separation, I am still not sure if there is hardware
being able talking to its own slave address, but we will see.

This RFC and while I did some quick tests, it is not thoroughly tested. But I
wanted to push it out before I leave the computer for the weekend. It still
shows what path I chose to solve the problem. So, comments on that and further
testing are more than welcome!

BTW Andrey, I did not modify your patch and couldn't get the i2c-slave-eeprom 
driver
to work with my Jetson TK1. Does this work for you?


This approach makes sense to me.

I'd expect patch 2/9 "dt-bindings: add header for generic I2C flags in 
bindings" to document the flags (or at least mention their existence, 
and point at the new header file) in the core I2C bindings document.


Please consider the series,
Acked-by: Stephen Warren 

(ack rather than review since I didn't review patch 1, and mostly 
concentrated on reviewing the concepts of how slaves were represented 
rather than the coding details).

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 9/9] dts: tegra: WIP: hack dts to test new dt flags for i2c

[Stephen Warren]

On 07/20/2015 10:10 AM, Rob Herring wrote:

On Mon, Jul 20, 2015 at 3:45 AM, Wolfram Sang  wrote:



+
+   eeprom@42 {
+   compatible = "linux,slave-24c02";
+   //FIXME: Should be I2C_OWN_SLAVE_ADDRESS | 0x42
+   reg = <0xc042>;


The node name doesn't match the reg property anymore. Isn't that considered as
a problem ?


Hmm, true. So far, Rob (CCed) was fine with this approach:
http://www.spinics.net/lists/linux-tegra/msg22760.html

@Rob: If we introduce flag bits in the MSBs of an I2C address, the reg
property is different from the node name. Is this a problem?


No, I don't it is a problem.


The rule so far has been that the unit address (the value in the node 
name) must match the first value in the reg property. I don't see why 
this rule should change. To solve this, just name the node 
eeprom@c042 (or eeprom@4042 with the correction pointed out 
earlier in the thread).

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFT PATCH 1/2] i2c: tegra: update CONFIG_LOAD for new conifiguration

[Stephen Warren]

On 06/30/2015 04:54 AM, Laxman Dewangan wrote:

Once the new configuration is set on the conifg register of
I2C controller, it is require to update the CONFIG_LOAD register
to transfer the new SW configuration to actual HW internal
registers that would be used in the actual logic.

It is like, SW is programming only shadow registers through
regular configuration and when these load_config bit fields
are set to 1, it causes the regular/shadows registers
configuration transferred to the HW internal active registers.
So SW has to set these bit fields at the end of all regular
registers configuration. And these config_load bits are HW
auto-clear bits. HW clears these bit fields once the register
configuration is moved to HW internal active registers. So SW
has to wait until these bits are auto-cleared before going
for any further programming

This mechanism is supported on T124 and after this SoCs.

This is based on change done by
Chaitanya Bandi 

Signed-off-by: Laxman Dewangan 
Signed-off-by: Chaitanya Bandi 


I'm not sure why Chaitanya's S-o-b is there and listed last if he's not 
the patch author. If he wrote the patch, he should be the git author and 
his S-o-b should be first. If he didn't and you simply based this patch 
on work by Chaitanya, then his S-o-b probably shouldn't be present, and 
yours would be last since you're submitting the patch.



---
Stephen/Andrew,
I need help on testing this on other platform. I tested this on T210.


I'm puzzled how this was tested on T210, since it isn't supported 
upstream yet.


The series,
Tested-by: Stephen Warren 

(Tested audio playback and volume adjustment on Jetson TK1 which 
contains a Tegra124 SoC)

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: busses: i2c-bcm2835: S-Register clear reserved bits

[Stephen Warren]

On 06/16/2015 09:40 AM, Silvan Wicki wrote:
> The Datasheet mentions on page 31 that the bits 10-31 must be read as
> don't care and written as 0.
> https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
> 
> We cannot guarantee that we reed bits 10-31 as always 0 (becuase the
> datasheet says read as don't care). We clear the bits with a bitmask to
> prevent writing back unknown data at the reserved bits.

I guess that's true.
Acked-by: Stephen Warren 
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] i2c: busses: i2c-bcm2835: limits cdiv to allowed values

[Stephen Warren]

On 06/06/2015 12:18 PM, Silvan Wicki wrote:
> Adds: make sure bits 16-31 of DIV register are always 0
> Adds: assume minimal divider of 2 if divider resulted in 0
>   (bcm2835 sets divider to 32768 if cdiv is set to 0)
> 
> See page 33/34 of BCM2835-ARM-Peripherals.pdf for the DIV register.
> https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

> diff --git a/drivers/i2c/busses/i2c-bcm2835.c 
> b/drivers/i2c/busses/i2c-bcm2835.c

> @@ -252,12 +255,28 @@ static int bcm2835_i2c_probe(struct platform_device 
> *pdev)

> + if (divider > BCM2835_I2C_CDIV_MAX) {
> + dev_warn(&pdev->dev,
> +  "Clock too slow, falling back to min clock speed\n");
> + divider = BCM2835_I2C_CDIV_MAX;
> + }
>   bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider);

Shouldn't this be an error? i.e. dev_err() not dev_warn(), and return an
error code.

If the divider that's required to reach the requested clock is higher
than the HW can implement, that means the actual clock will be faster
than the requested clock. Presumably there's a reason the specific slow
rate was requested, and if we exceed it, there will be problems.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: busses: i2c-bcm2835: limits cdiv to allowed values

[Stephen Warren]

On 05/29/2015 04:26 AM, Silvan Wicki wrote:
> Adds: make sure bits 16-31 of DIV register are always 0
> Adds: assume minimal divider of 2 if divider resulted in 0
>   (bcm2835 sets divider to 32768 if cdiv is set to 0)
> 
> See page 33/34 of BCM2835-ARM-Peripherals.pdf for the DIV register.
> https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

> diff --git a/drivers/i2c/busses/i2c-bcm2835.c 
> b/drivers/i2c/busses/i2c-bcm2835.c

> @@ -252,12 +255,22 @@ static int bcm2835_i2c_probe(struct platform_device 
> *pdev)
>  
>   divider = DIV_ROUND_UP(clk_get_rate(i2c_dev->clk), bus_clk_rate);
>   /*
> +  * Divider results in 0 by extremely high bus_clk_rate values
> +  * such as bus_clk_rate >= 4044967297 and core_clock = 250MHz.
> +  * In such a case assume the minimal possible divider since
> +  * bcm2835 chip sets divisor internally to 32768 if cdiv is 0.
> +  */

I would rephrase that as:

/*
 * A divider of0 results in extremely high bus_clk_rate values
 * such as bus_clk_rate >= 4044967297 with core_clock = 250MHz.
 * In such a case assume the minimal possible divider, since
 * bcm2835 chip sets divisor internally to 32768 if cdiv is 0.
 */

> + if (divider < BCM2835_I2C_CDIV_MIN)
> + divider = BCM2835_I2C_CDIV_MIN;

This seems reasonable; if the calculated divider is too small, then it
means we can't run the clock that fas. Running it more slowly loses some
performance, but should work fine. I might suggest a dev_info() or
dev_dbg() here.

> + /*
>* Per the datasheet, the register is always interpreted as an even
>* number, by rounding down. In other words, the LSB is ignored. So,
>* if the LSB is set, increment the divider to avoid any issue.
>*/
>   if (divider & 1)
>   divider++;
> + if (divider > BCM2835_I2C_CDIV_MAX)
> + divider = BCM2835_I2C_CDIV_MAX;

I think this should be an error. If the calculated divider is too large,
it means we want to run the clock slower than we can. If we run the
clock faster than requested, the clock rate might exceed the attached
I2C devices' capabilities (otherwise, why would we want such a slow
clock?). As such, I'd suggest a dev_err() here, and to fail the probe()
by returning an error.

I'd echo Eric's question: I'm curious whether this patch solves a
real-world problem, or if you found it by code inspection?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: busses: i2c-bcm2835: limits cdiv to allowed values

[Stephen Warren]

On 05/22/2015 02:36 AM, s. wicki wrote:

fixes:  round down divider instead of incrementing
adds:   make sure bits 16-31 of cdiv register are always 0
adds:   assume minimal divider of 2 if divider resulted in 0
(bcm2835 sets divider to 32768 if cdiv is set to 0)


Do you have a reference to a specific document and section? That'd be 
good to include in the commit description for future reference.



Signed-off-by: s. wicki 


Using your full name rather than an abbreviation is preferred.

You didn't send this pach to the I2C maintainer so I imagine he won't 
see it and hence it won't be applied.


It'd also be a good idea to send this to the Raspberry Pi mailing list, 
linux-rpi-ker...@lists.infradead.org.




diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
+#define BCM2835_I2C_CDIV_MIN   0x0002
+#define BCM2835_I2C_CDIV_MAX   0xFFFE
+#define BCM2835_I2C_CDIV_BITMSK0xFFFE



divider = DIV_ROUND_UP(clk_get_rate(i2c_dev->clk), bus_clk_rate);
-   /*
-* Per the datasheet, the register is always interpreted as an even
-* number, by rounding down. In other words, the LSB is ignored. So,
-* if the LSB is set, increment the divider to avoid any issue.
-*/
-   if (divider & 1)
-   divider++;


The old code used to round the divider up in the case where the LSB was 
set. This ensures that the I2C clock rate is no faster than what was 
requested.



+   if (divider == 0) {
+   /*
+* divider results in 0 by extremely high bus_clk_rate values
+* such as bus_clk_rate >= 4044967297 and core_clock = 250MHz.
+* In such a case assume the minimal possible divider since
+* bcm2835 chip sets divisor internally to 32768 if cdiv is 0.
+*/
+   divider = BCM2835_I2C_CDIV_MIN;


What about when divider == 1? Shouldn't that if condition be:

if (divider < BCM2835_I2C_CDIV_MIN)

?


+   } else {
+   /* check if divider meets certain bcm2835 specific criterias */
+   if ((divider & BCM2835_I2C_CDIV_BITMSK) != divider) {
+   if (divider > BCM2835_I2C_CDIV_MAX)
+   /*
+* Per the datasheet it must be made sure
+* that bits 16-31 are set to 0. If that is
+* not the case, then set to the maximum
+* allowed value.
+*/
+   divider = BCM2835_I2C_CDIV_MAX;
+   else
+   /*
+* Per datasheet the cdiv is always rounded
+* down to an even number. Bitmask takes care
+* of this and clears the LSB
+*/


The datasheet describes how the HW interprets the register value (by 
ignoring the LSB). It doesn't say how SW must program the register. In 
other words, SW should round up, to avoid HW rounding down. It's not 
mandatory for SW to round down.



+   divider &= BCM2835_I2C_CDIV_BITMSK;
+   }
+   }
bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider);

irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);


I think this would be better as:

if (divider < BCM2835_I2C_CDIV_MIN)
divider = BCM2835_I2C_CDIV_MIN;
if (divider & 1)
divider++;
if (divider > BCM2835_I2C_CDIV_MAX)
divider = BCM2835_I2C_CDIV_MAX;

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] dt: paz00: define nvec as child of i2c bus

[Stephen Warren]

On 04/02/2015 03:37 AM, Marc Dietrich wrote:


Am Mittwoch, 1. April 2015, 11:28:32 schrieb Stephen Warren:

On 03/31/2015 09:46 AM, Andrey Danin wrote:

On 31.03.2015 17:09, Stephen Warren wrote:

On 03/31/2015 12:40 AM, Andrey Danin wrote:

Hi,

Thanks for the review.

On 03.02.2015 0:20, Stephen Warren wrote:


[ snipped old patch parts ]


diff --git a/arch/arm/boot/dts/tegra20-paz00.dts
b/arch/arm/boot/dts/tegra20-paz00.dts

-nvec@7000c500 {
-compatible = "nvidia,nvec";
-reg = <0x7000c500 0x100>;
-interrupts = ;
-#address-cells = <1>;
-#size-cells = <0>;
+i2c@7000c500 {
+status = "okay";

   clock-frequency = <8>;

-request-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_HIGH>;
-slave-addr = <138>;
-clocks = <&tegra_car TEGRA20_CLK_I2C3>,
- <&tegra_car TEGRA20_CLK_PLL_P_OUT3>;
-clock-names = "div-clk", "fast-clk";
-resets = <&tegra_car 67>;
-reset-names = "i2c";
+
+nvec: nvec@45 {


This doesn't feel correct. There's nothing here to indicate that this
child device is a slave that is implemented by the host SoC rather than
something external attached to the I2C bus.

Perhaps you can get away with this, since the driver for nvidia,nvec
only calls I2C APIs suitable for internal slaves rather than external
slaves? Even so though, I think the distinction needs to be clearly
marked in the DT so that any generic code outside the NVEC driver that
parses the DT can determine the difference.

I would recommend the I2C controller having #address-cells=<2> with
cell
0 being 0==master,1==slave, cell 1 being the I2C address. The I2C
driver
would need to support #address-cells=<1> for backwards-compatibility.


Stephen, we haven't used your suggestion because Wolfram disliked the idea in
e.g. http://lkml.iu.edu/hypermail/linux/kernel/1409.1/03446.html


As you said in the response you linked to, the objection is invalid 
since it won't break any DTs. The driver for a node is responsible for 
defining the meaning of its own reg properties. It should be pretty 
trivial to allow the Tegra I2C controller driver (or indeed any driver 
at all) to handle either #address-cells=<1> (the current setting) or 
#address-cells=<2> (a new value which enables adding a new flag cell) or 
even #address-cells=<1> with some of the upper bits of the reg value 
used as flags (which would default to 0 in all current DTs, so e.g. 
using the MSB==1 as a slave flag), all at run-time with complete 
backwards-compatibility.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 03/19/2015 10:02 AM, Wolfram Sang wrote:

-   /* Only register child devices if the adapter has a node pointer set */
-   if (!adap->dev.of_node)
+   /* Only register childs if adapter has a node pointer with enabled 
status */
+   if (!adap->dev.of_node || !of_device_is_available(adap->dev.of_node))
return;


That feels a bit odd to me. For a regular non-mux I2C controller, that extra
case would never trigger if the controller node was disabled, since the
device core would never probe the controller device itself. So, we'd end up
with inconsistent paths through the I2C core for regular controllers and
muxes.


I first thought the no-op for the non-mux case wouldn't hurt, but I
agree about the consistent code path. I mentioned in my previous mail
that i2c-mux might be a better place for this...


Perhaps better would be to have a mux-specific function to iterate over a
mux's child nodes and instantiate buses for those. That function would check
whether each bus node was disabled or not. That'd isolate the special case
into the place where it was relevant.


... so I wonder what you think about putting the
of_device_is_available() check into i2c_add_mux_adapter() once the
reg-property and chan_id have been matched?


I think that looks like a good place, yes.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 03/19/2015 04:09 AM, Wolfram Sang wrote:



Possible. But this change just makes i2c-mux-pinctrl honor status
property at all. There is no functional change except it now allows
you to disable any of the sub-busses.


Actually, this is the feature I like. However, I wonder if we shouldn't
have that in the core, say in of_i2c_register_devices()?


Hmm, looking at of_i2c_register_devices():

for_each_available_child_of_node(adap->dev.of_node, node)
of_i2c_register_device(adap, node);

already honors status properties by using for_each_available_foo.
Therefore, i2c-core will also skip i2c device nodes disabled by
status property.


Yes, but only child nodes, not the complete bus. Here is an RFC of what
I mean:

From: Wolfram Sang 
Subject: [RFC] i2c: of: always check if busses are enabled

Allow all busses to have a "status" property which allows busses to not
be probed when set to "disabled". Needed for DTS overlays with i2c mux
scenarios.



diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c



@@ -1305,8 +1305,8 @@ static void of_i2c_register_devices(struct i2c_adapter 
*adap)
  {
struct device_node *node;

-   /* Only register child devices if the adapter has a node pointer set */
-   if (!adap->dev.of_node)
+   /* Only register childs if adapter has a node pointer with enabled 
status */
+   if (!adap->dev.of_node || !of_device_is_available(adap->dev.of_node))
return;


That feels a bit odd to me. For a regular non-mux I2C controller, that 
extra case would never trigger if the controller node was disabled, 
since the device core would never probe the controller device itself. 
So, we'd end up with inconsistent paths through the I2C core for regular 
controllers and muxes.


Perhaps better would be to have a mux-specific function to iterate over 
a mux's child nodes and instantiate buses for those. That function would 
check whether each bus node was disabled or not. That'd isolate the 
special case into the place where it was relevant.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 03/09/2015 06:21 AM, Sebastian Hesselbarth wrote:

I2C mux pinctrl driver currently determines the number of sub-busses by
counting available pinctrl-names. Unfortunately, this requires each
incarnation of the devicetree node with different available sub-busses
to be rewritten.

This patch reworks i2c-mux-pinctrl driver to count the number of
available sub-nodes instead. The rework should be compatible to the old
way of probing for sub-busses and additionally allows to disable unused
sub-busses with standard DT property status = "disabled".

This also amends the corresponding devicetree binding documentation to
reflect the new functionality to disable unused sub-nodes. While at it,
also fix two references to binding documentation files that miss an "i2c-"
prefix.


The DT binding changes at least,
Acked-by: Stephen Warren 
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 02/27/2015 05:24 AM, Sebastian Hesselbarth wrote:

I2C mux pinctrl driver currently determines the number of sub-busses by
counting available pinctrl-names. Unfortunately, this requires each
incarnation of the devicetree node with different available sub-busses
to be rewritten.

This patch reworks i2c-mux-pinctrl driver to count the number of
available sub-nodes instead. The rework should be compatible to the old
way of probing for sub-busses and additionally allows to disable unused
sub-busses with standard DT property status = "disabled".

This also amends the corresponding devicetree binding documentation to
reflect the new functionality to disable unused sub-nodes. While at it,
also fix two references to binding documentation files that miss an "i2c-"
prefix.



diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt 
b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt



-For each named state defined in the pinctrl-names property, an I2C child bus
-will be created. I2C child bus numbers are assigned based on the index into
-the pinctrl-names property.
+For each enabled child node an I2C child bus will be created. I2C child bus
+numbers are assigned based on the order of child nodes.


I think that I2C bus numbers are an internal concept for the OS. As 
such, we should probably remove any mention re: the bus numbers from the 
binding.



-The only exception is that no bus will be created for a state named "idle". If
-such a state is defined, it must be the last entry in pinctrl-names. For
-example:
+There must be a corresponding pinctrl-names entry for each enabled child
+node at the position of the child node's "reg" property. Also, there can be
+an idle pinctrl state defined at the end of possible pinctrl states. If such
+a state is defined, it must be the last entry in pinctrl-names. For example:


What about gaps in the numbering sequence? IIRC, in a situation with 5 
nodes with reg 0, 1, 2, 3, 4 but where only the nodes with reg of 1, 3 
enabled, we only want 2 entries in pinctrl-names? If so, "at the 
position of the child node's "reg" property" isn't correct, since "at 
the position" implies there must be gaps in pinctrl-names. "In the same 
order as the reg property values for enabled subnodes" might be a better 
description.


Perhaps I'm misremembering and you explicitly didn't want to remove 
entries from pinctrl-names if child nodes were disabled? If so, then 
surely then in the text above, "for each enabled child" should be 
replaced with "for each child"?



@@ -68,6 +68,7 @@ Example:
pinctrl-1 = <&state_i2cmux_pta>;
pinctrl-2 = <&state_i2cmux_idle>;

+   /* Enabled child bus 0 */
i2c@0 {
reg = <0>;
#address-cells = <1>;
@@ -79,10 +80,12 @@ Example:
};
};

+   /* Disabled child bus 1 */
i2c@1 {
reg = <1>;
#address-cells = <1>;
#size-cells = <0>;
+   status = "disabled";


To make the example cover more cases, perhaps make child node i2c@0 
disabled and i2c@1 enabled. Then, the example would show what happens to 
pinctrl-names when there are gaps in the reg property numbering space of 
enabled children?

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 02/17/2015 11:52 AM, Sebastian Hesselbarth wrote:

I2C mux pinctrl driver currently determines the number of sub-busses by
counting available pinctrl-names. Unfortunately, this requires each
incarnation of the devicetree node with different available sub-busses
to be rewritten.

This patch reworks i2c-mux-pinctrl driver to count the number of
available sub-nodes instead. The rework should be compatible to the old
way of probing for sub-busses and additionally allows to disable unused
sub-busses with standard DT property status = "disabled".

This also amends the corresponding devicetree binding documentation to
reflect the new functionality to disable unused sub-nodes. While at it,
also fix two references to binding documentation files that miss an "i2c-"
prefix.


Tested-by: Stephen Warren 

(Both the HDMI and LCD panel DDC busses on NVIDIA Tegra 
Springbank/Seaboard, running next-20150224)

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/8] Add proper support for Compulab CM-A510/SBC-A510

[Stephen Warren]

On 02/26/2015 12:39 PM, Sebastian Hesselbarth wrote:

On 26.02.2015 18:55, Gregory CLEMENT wrote:

On 17/02/2015 19:52, Sebastian Hesselbarth wrote:

This patch set improves current mainline support for the Compulab
CM-A510 System-on-Module (SoM) and its default Compulab SBC-A510
base board. Thanks to Gabriel Dobato who agreed to remote debug and
test the provided DT changes.

[...]

Sebastian Hesselbarth (8):
   i2c: mux-pinctrl: Rework to honor disabled child nodes
   devicetree: vendor-prefixes: Add CompuLab to known vendors
   ARM: dts: dove: Fix uart[23] reg property
   ARM: dts: dove: Always include gpio and interrupt-controller headers
   ARM: dts: dove: Add node labels for PCIe ports 0 and 1
   ARM: dts: dove: Add some more common pinctrl settings
   ARM: dts: dove: Add internal i2c multiplexer node
   ARM: dts: dove: Add proper support for Compulab CM-A510/SBC-A510


Patches 3 to 6 applied on mvebu/dt

If I understood well patches 7 and 8 depend on patch 1 which had not been
taken yet.


Correct. Once I get a Tested-by from Stephen, I'll resend the 4
remaining patches.


Me? For which patch? I thought there was going to be a v2 of the i2c mux 
driver patch given the comments on it, but perhaps I'm misremembering?

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 02/17/2015 02:08 PM, Sebastian Hesselbarth wrote:

On 17.02.2015 21:46, Stephen Warren wrote:

On 02/17/2015 11:52 AM, Sebastian Hesselbarth wrote:

I2C mux pinctrl driver currently determines the number of sub-busses by
counting available pinctrl-names. Unfortunately, this requires each
incarnation of the devicetree node with different available sub-busses
to be rewritten.


Can you be more explicit about the problem here? Why does anything need
to be re-written if a child node is disabled; presumably there's no need
for the child bus numbers to be contiguous. In other words, with the
example in the existing DT binding doc:

 i2cmux {
 compatible = "i2c-mux-pinctrl";
...
 pinctrl-names = "ddc", "pta", "idle";
 pinctrl-0 = <&state_i2cmux_ddc>;
 pinctrl-1 = <&state_i2cmux_pta>;
 pinctrl-2 = <&state_i2cmux_idle>;

 i2c@0 {
 reg = <0>;
...
 i2c@1 {
 reg = <1>;
...

That would generate child busses 0 and 1. If I was to disable the i2c@0
node, then there would still be definitions for child busses 0 and 1 in
the DT, it's just that child bus 0 wouldn't actually exist at run-time.
I don't see what part of DT needs to be re-written to accomodate this?


The way the current driver works, to disable i2c@0 you'd have to remove
the pinctrl-0 state, pinctrl-names string at position 0, and the node
itself.

So, on Dove SoC there is three sub-busses, now consider one board A with
i2c0 and i2c1 enabled but board B with i2c0 and i2c2 enabled:

board-A.dts:

i2cmux {
 pinctrl-names = "i2c0", "i2c1", "idle";
 pinctrl-0 = <&state_for_i2c0>;
 pinctrl-1 = <&state_for_i2c1>;
};

but

board-B.dts:

i2cmux {
 pinctrl-names = "i2c0", "i2c2", "idle";
 pinctrl-0 = <&state_for_i2c0>;
 pinctrl-1 = <&state_for_i2c2>;
 /* Note that this ^^^ is state_for_i2c2 */
};

while the approach with status = "disabled" allows all properties for
both board remain the same - except you'll enable either i2c1 or i2c2
sub-node on board level:

i2cmux {
 pinctrl-names = "i2c0", "i2c1", "i2c2", "idle";
 pinctrl-0 = <&state_for_i2c0>;
 pinctrl-1 = <&state_for_i2c1>;
 pinctrl-2 = <&state_for_i2c2>;
};

board-A.dts:

i2cmux {
 i2c@0 { status = "okay"; };
 i2c@1 { status = "okay"; };
};

and

board-B.dts:

i2cmux {
 i2c@0 { status = "okay"; };
 i2c@2 { status = "okay"; };
};


OK, that all makes sense, but I don't think there's any change at all to 
the binding; this can all be fixed in the driver without affecting the 
definition of the binding at all. At most all that's needed in the 
binding is a note to the effect that if a particular child node is 
disabled, then this has no effect at all on the requirements for the 
pinctrl properties.



In general, it is less about the binding but how the driver is written:
Number of sub-busses is determined by elements in pinctrl-names not
available (enabled) sub-nodes.


This patch reworks i2c-mux-pinctrl driver to count the number of
available sub-nodes instead. The rework should be compatible to the old
way of probing for sub-busses and additionally allows to disable unused
sub-busses with standard DT property status = "disabled".

This also amends the corresponding devicetree binding documentation to
reflect the new functionality to disable unused sub-nodes. While at it,
also fix two references to binding documentation files that miss an
"i2c-"
prefix.



diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt



-For each named state defined in the pinctrl-names property, an I2C
child bus
-will be created. I2C child bus numbers are assigned based on the
index into
-the pinctrl-names property.
+For each child node that is not disabled by a status != "okay", an I2C
+child bus will be created. I2C child bus numbers are assigned based
on the
+order of child nodes.


I would have assumed that disabled sub-nodes was a global concept within
DT, and so wouldn't be mentioned in the binding. It would just be a bug
in the driver if it didn't ignore disabled sub-nodes.


Yep, the concept is very global. It is about the current driver and this
binding changes are just to make it a little more clear that the driver
should behave different, i.e. get rid of anything that implies that
pinctrl-names has any effect on the number of sub-busses registered.


-The only exception is that no bus will be created for a state named
"idle". If
-s

Re: [PATCH 1/8] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 02/17/2015 11:52 AM, Sebastian Hesselbarth wrote:

I2C mux pinctrl driver currently determines the number of sub-busses by
counting available pinctrl-names. Unfortunately, this requires each
incarnation of the devicetree node with different available sub-busses
to be rewritten.


Can you be more explicit about the problem here? Why does anything need 
to be re-written if a child node is disabled; presumably there's no need 
for the child bus numbers to be contiguous. In other words, with the 
example in the existing DT binding doc:


i2cmux {
compatible = "i2c-mux-pinctrl";
...
pinctrl-names = "ddc", "pta", "idle";
pinctrl-0 = <&state_i2cmux_ddc>;
pinctrl-1 = <&state_i2cmux_pta>;
pinctrl-2 = <&state_i2cmux_idle>;

i2c@0 {
reg = <0>;
...
i2c@1 {
reg = <1>;
...

That would generate child busses 0 and 1. If I was to disable the i2c@0 
node, then there would still be definitions for child busses 0 and 1 in 
the DT, it's just that child bus 0 wouldn't actually exist at run-time. 
I don't see what part of DT needs to be re-written to accomodate this?



This patch reworks i2c-mux-pinctrl driver to count the number of
available sub-nodes instead. The rework should be compatible to the old
way of probing for sub-busses and additionally allows to disable unused
sub-busses with standard DT property status = "disabled".

This also amends the corresponding devicetree binding documentation to
reflect the new functionality to disable unused sub-nodes. While at it,
also fix two references to binding documentation files that miss an "i2c-"
prefix.



diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt 
b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt



-For each named state defined in the pinctrl-names property, an I2C child bus
-will be created. I2C child bus numbers are assigned based on the index into
-the pinctrl-names property.
+For each child node that is not disabled by a status != "okay", an I2C
+child bus will be created. I2C child bus numbers are assigned based on the
+order of child nodes.


I would have assumed that disabled sub-nodes was a global concept within 
DT, and so wouldn't be mentioned in the binding. It would just be a bug 
in the driver if it didn't ignore disabled sub-nodes.







-The only exception is that no bus will be created for a state named "idle". If
-such a state is defined, it must be the last entry in pinctrl-names. For
-example:
-
-   pinctrl-names = "ddc", "pta", "idle"  ->  ddc = bus 0, pta = bus 1
-   pinctrl-names = "ddc", "idle", "pta"  ->  Invalid ("idle" not last)
-   pinctrl-names = "idle", "ddc", "pta"  ->  Invalid ("idle" not last)
+There must be a corresponding pinctrl-names entry for each enabled child
+node at the position of the child node's "reg" property.


The addition there seems fine, but the existing text re: the idle state 
seems clearer in the original text.


  Whenever an access is made to a device on a child bus, the relevant pinctrl
  state will be programmed into hardware.

-If an idle state is defined, whenever an access is not being made to a device
-on a child bus, the idle pinctrl state will be programmed into hardware.
+Also, there can be an idle pinctrl state defined at the end of possible
+pinctrl states. If an idle state is defined, whenever an access is not being
+made to a device on a child bus, the idle pinctrl state will be programmed
+into hardware.


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: i2c-tegra: Move clk_prepare/clk_set_rate to probe

[Stephen Warren]

On 09/05/2014 03:28 AM, Mikko Perttunen wrote:

From: Mikko Perttunen 

Currently the i2c-tegra bus driver prepares, enables
and set_rates its clocks separately for each transfer.
This causes locking problems when doing I2C transfers
from clock notifiers; see
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/268653.html

This patch moves clk_prepare/unprepare and clk_set_rate calls to
the probe function, leaving only clk_enable/disable to be
done on each transfer. This solves the locking issue.


Reviewed-by: Stephen Warren 
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: i2c-tegra: Move clk_prepare/clk_set_rate to probe

[Stephen Warren]

On 09/02/2014 05:56 AM, Wolfram Sang wrote:

On Fri, Aug 15, 2014 at 10:18:15AM -0600, Stephen Warren wrote:

On 08/15/2014 03:47 AM, Mikko Perttunen wrote:

Currently the i2c-tegra bus driver prepares, enables
and set_rates its clocks separately for each transfer.
This causes locking problems when doing I2C transfers

>from clock notifiers; see

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/268653.html

This patch moves clk_prepare/unprepare and clk_set_rate calls to
the probe function, leaving only clk_enable/disable to be
done on each transfer. This solves the locking issue.



diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c



@@ -380,34 +380,33 @@ static inline int tegra_i2c_clock_enable(struct 
tegra_i2c_dev *i2c_dev)
  {
int ret;
if (!i2c_dev->hw->has_single_clk_source) {
-   ret = clk_prepare_enable(i2c_dev->fast_clk);
+   ret = clk_enable(i2c_dev->fast_clk);


Here, both the prepare and enable wrap just the I2C transfer, ...


@@ -428,9 +427,6 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, val, I2C_CNFG);
i2c_writel(i2c_dev, 0, I2C_INT_MASK);

-   clk_multiplier *= (i2c_dev->hw->clk_divisor_std_fast_mode + 1);
-   clk_set_rate(i2c_dev->div_clk, i2c_dev->bus_clk_rate * clk_multiplier);


... whereas the rate is set up when the controller is initialized, i.e. much
earlier.


@@ -777,17 +774,39 @@ static int tegra_i2c_probe(struct platform_device *pdev)



+   if (!i2c_dev->hw->has_single_clk_source) {
+   ret = clk_prepare(i2c_dev->fast_clk);
+   if (ret < 0) {
+   dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
+   return ret;
+   }
+   }
+
+   ret = clk_prepare(i2c_dev->div_clk);
+   if (ret < 0) {
+   dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
+   goto unprepare_fast_clk;
+   }
+
+   clk_multiplier *= (i2c_dev->hw->clk_divisor_std_fast_mode + 1);
+   ret = clk_set_rate(i2c_dev->div_clk,
+  i2c_dev->bus_clk_rate * clk_multiplier);
+   if (ret) {
+   dev_err(i2c_dev->dev, "Clock rate change failed %d\n", ret);
+   goto unprepare_div_clk;
+   }


However, the new code sets the clock rate after the clock is prepared. I
think the rate should be set first, then the clock prepared. While this
likely doesn't apply to the Tegra clock controller, prepare() is allowed to
enable the clock if enable() can't be implemented in an atomic fashion (in
which case enable/disable would be no-ops), and we should make sure that the
driver correctly configures the clock before potentially enabling it.

I'm not sure if a similar change to our SPI drivers is possible; after all,
the SPI transfer rate can vary per message, so if clk_set_rate() acquires a
lock, it seems there's no way to avoid the issue there. Luckily, we don't
have any SPI-based chips that do anything related to clocks on any of our
current boards...

Aside from this issue, the patch looks fine to me.


May I count this as an Ack?


Sure, once the afore-mentioned issue is resolved.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: i2c-tegra: Move clk_prepare/clk_set_rate to probe

[Stephen Warren]

On 08/15/2014 03:34 PM, Peter De Schrijver wrote:

On Fri, Aug 15, 2014 at 09:45:46PM +0200, Peter De Schrijver wrote:

On Fri, Aug 15, 2014 at 08:07:01PM +0200, Stephen Warren wrote:

However, the new code sets the clock rate after the clock is prepared. I
think the rate should be set first, then the clock prepared. While this
likely doesn't apply to the Tegra clock controller, prepare() is allowed
to enable the clock if enable() can't be implemented in an atomic
fashion (in which case enable/disable would be no-ops), and we should
make sure that the driver correctly configures the clock before
potentially enabling it.

I'm not sure if a similar change to our SPI drivers is possible; after
all, the SPI transfer rate can vary per message, so if clk_set_rate()
acquires a lock, it seems there's no way to avoid the issue there.


Even for i2c this could be the case I think if you use the highspeed (3.4Mhz)
mode? From what I remember, a highspeed i2c transaction starts with a lower
speed preamble to make sure non highspeed slaves don't get confused? Which
means you could change the bus speed depending on the slave you're addressing.


Since there's no separate chip-select for I2C, I believe all I2C devices
need to be able to understand the entire transaction, so the I2C bus
speed is fixed.



Does it? I would assume the slave only needs to check if the address matches
its own address after a START condition and if not can just wait until the
STOP condition appears on the bus?



http://www.nxp.com/documents/user_manual/UM10204.pdf says you can mix them by
using an interconnect bridge between the highspeed and the non-highspeed
capable slaves. The bridge uses the special preamble to disconnect the non-
highspeed part of the bus when a highspeed transaction is ongoing. It's afaics
transparent to the master.


I expect that works by echoing the slow-speed pre-amble to the 
slow-speed bus segment, then emitting a stop and turning off the echo. 
For actual slow-speed transactions, the whole thing would be echo'd. 
That way the slow-speed devices don't ever see any high-speed pulses.


That all said, that does indeed imply that a master supporting the 
high-speed transactions would need to emit a varying-speed signal. My 
assumption would be that this happens inside the I2C HW, rather than 
under SW control though, since the transition would need to happen 
mid-protocol. Still, perhaps the selection between low-speed and 
high-speed-with-a-slow-preamble might need SW clock programming 
depending on the HW though... Who knows.


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: i2c-tegra: Move clk_prepare/clk_set_rate to probe

[Stephen Warren]

On 08/15/2014 12:02 PM, Peter De Schrijver wrote:

On Fri, Aug 15, 2014 at 06:18:15PM +0200, Stephen Warren wrote:

On 08/15/2014 03:47 AM, Mikko Perttunen wrote:

Currently the i2c-tegra bus driver prepares, enables
and set_rates its clocks separately for each transfer.
This causes locking problems when doing I2C transfers
from clock notifiers; see
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/268653.html

This patch moves clk_prepare/unprepare and clk_set_rate calls to
the probe function, leaving only clk_enable/disable to be
done on each transfer. This solves the locking issue.



diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c



@@ -380,34 +380,33 @@ static inline int tegra_i2c_clock_enable(struct 
tegra_i2c_dev *i2c_dev)
   {
int ret;
if (!i2c_dev->hw->has_single_clk_source) {
-   ret = clk_prepare_enable(i2c_dev->fast_clk);
+   ret = clk_enable(i2c_dev->fast_clk);


Here, both the prepare and enable wrap just the I2C transfer, ...


@@ -428,9 +427,6 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, val, I2C_CNFG);
i2c_writel(i2c_dev, 0, I2C_INT_MASK);

-   clk_multiplier *= (i2c_dev->hw->clk_divisor_std_fast_mode + 1);
-   clk_set_rate(i2c_dev->div_clk, i2c_dev->bus_clk_rate * clk_multiplier);


... whereas the rate is set up when the controller is initialized, i.e.
much earlier.


@@ -777,17 +774,39 @@ static int tegra_i2c_probe(struct platform_device *pdev)



+   if (!i2c_dev->hw->has_single_clk_source) {
+   ret = clk_prepare(i2c_dev->fast_clk);
+   if (ret < 0) {
+   dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
+   return ret;
+   }
+   }
+
+   ret = clk_prepare(i2c_dev->div_clk);
+   if (ret < 0) {
+   dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
+   goto unprepare_fast_clk;
+   }
+
+   clk_multiplier *= (i2c_dev->hw->clk_divisor_std_fast_mode + 1);
+   ret = clk_set_rate(i2c_dev->div_clk,
+  i2c_dev->bus_clk_rate * clk_multiplier);
+   if (ret) {
+   dev_err(i2c_dev->dev, "Clock rate change failed %d\n", ret);
+   goto unprepare_div_clk;
+   }


However, the new code sets the clock rate after the clock is prepared. I
think the rate should be set first, then the clock prepared. While this
likely doesn't apply to the Tegra clock controller, prepare() is allowed
to enable the clock if enable() can't be implemented in an atomic
fashion (in which case enable/disable would be no-ops), and we should
make sure that the driver correctly configures the clock before
potentially enabling it.

I'm not sure if a similar change to our SPI drivers is possible; after
all, the SPI transfer rate can vary per message, so if clk_set_rate()
acquires a lock, it seems there's no way to avoid the issue there.


Even for i2c this could be the case I think if you use the highspeed (3.4Mhz)
mode? From what I remember, a highspeed i2c transaction starts with a lower
speed preamble to make sure non highspeed slaves don't get confused? Which
means you could change the bus speed depending on the slave you're addressing.


Since there's no separate chip-select for I2C, I believe all I2C devices 
need to be able to understand the entire transaction, so the I2C bus 
speed is fixed.


At least, that's my understanding between 100KHz and 400KHz I2C. I don't 
know if 3.4MHz I2C introduced something new, although considering that 
slower I2C never had anything about being compatible with fast stuff in 
the spec AFAIK, and such speed-switching would only be useful for 
backwards-compatibility, I don't see how that would work.



Luckily, we don't have any SPI-based chips that do anything related to
clocks on any of our current boards...


And we don't use SPI to talk to the PMIC, which is the usecase were actually
run into problems with the locking.


IIRC, the I2C-based clock provider (or consumer?) issue was something 
mentioned (later on?) in the email thread linked by the patch description.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: i2c-tegra: Move clk_prepare/clk_set_rate to probe

[Stephen Warren]

On 08/15/2014 03:47 AM, Mikko Perttunen wrote:

Currently the i2c-tegra bus driver prepares, enables
and set_rates its clocks separately for each transfer.
This causes locking problems when doing I2C transfers
from clock notifiers; see
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/268653.html

This patch moves clk_prepare/unprepare and clk_set_rate calls to
the probe function, leaving only clk_enable/disable to be
done on each transfer. This solves the locking issue.



diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c



@@ -380,34 +380,33 @@ static inline int tegra_i2c_clock_enable(struct 
tegra_i2c_dev *i2c_dev)
  {
int ret;
if (!i2c_dev->hw->has_single_clk_source) {
-   ret = clk_prepare_enable(i2c_dev->fast_clk);
+   ret = clk_enable(i2c_dev->fast_clk);


Here, both the prepare and enable wrap just the I2C transfer, ...


@@ -428,9 +427,6 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, val, I2C_CNFG);
i2c_writel(i2c_dev, 0, I2C_INT_MASK);

-   clk_multiplier *= (i2c_dev->hw->clk_divisor_std_fast_mode + 1);
-   clk_set_rate(i2c_dev->div_clk, i2c_dev->bus_clk_rate * clk_multiplier);


... whereas the rate is set up when the controller is initialized, i.e. 
much earlier.



@@ -777,17 +774,39 @@ static int tegra_i2c_probe(struct platform_device *pdev)



+   if (!i2c_dev->hw->has_single_clk_source) {
+   ret = clk_prepare(i2c_dev->fast_clk);
+   if (ret < 0) {
+   dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
+   return ret;
+   }
+   }
+
+   ret = clk_prepare(i2c_dev->div_clk);
+   if (ret < 0) {
+   dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
+   goto unprepare_fast_clk;
+   }
+
+   clk_multiplier *= (i2c_dev->hw->clk_divisor_std_fast_mode + 1);
+   ret = clk_set_rate(i2c_dev->div_clk,
+  i2c_dev->bus_clk_rate * clk_multiplier);
+   if (ret) {
+   dev_err(i2c_dev->dev, "Clock rate change failed %d\n", ret);
+   goto unprepare_div_clk;
+   }


However, the new code sets the clock rate after the clock is prepared. I 
think the rate should be set first, then the clock prepared. While this 
likely doesn't apply to the Tegra clock controller, prepare() is allowed 
to enable the clock if enable() can't be implemented in an atomic 
fashion (in which case enable/disable would be no-ops), and we should 
make sure that the driver correctly configures the clock before 
potentially enabling it.


I'm not sure if a similar change to our SPI drivers is possible; after 
all, the SPI transfer rate can vary per message, so if clk_set_rate() 
acquires a lock, it seems there's no way to avoid the issue there. 
Luckily, we don't have any SPI-based chips that do anything related to 
clocks on any of our current boards...


Aside from this issue, the patch looks fine to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 6/7] i2c: ChromeOS EC tunnel driver

[Stephen Warren]

On 04/30/2014 11:44 AM, Doug Anderson wrote:
> On ARM Chromebooks we have a few devices that are accessed by both the
> AP (the main "Application Processor") and the EC (the Embedded
> Controller).  These are:
> * The battery (sbs-battery).
> * The power management unit tps65090.
...
> On the Samsung ARM Chromebook 2 the scheme is changed yet again, now:
> * The AP/EC comms are now using SPI for faster speeds.
> * The EC's i2c bus is exposed to the AP through a full i2c tunnel.
> 
> The upstream "tegra124-venice2" uses the same scheme as the Samsung
> ARM Chromebook 2, though it has a different set of components on the
> other side of the bus.
> 
> This driver supports the scheme used by the Samsung ARM Chromebook 2.
> Future patches to this driver could add support for the battery tunnel
> on the HP Chromebook 11 (and perhaps could even be used to access
> tps65090 on the HP Chromebook 11 instead of using a special driver,
> but I haven't researched that enough).

The binding looks reasonable to me, so that part,
Acked-by: Stephen Warren 

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/7] Add cros_ec changes for newer boards

[Stephen Warren]

On 04/23/2014 06:32 AM, Lee Jones wrote:
> On Tue, 22 Apr 2014, Doug Anderson wrote:
> 
>> This series adds the most critical cros_ec changes for newer boards
>> using cros_ec.  Specifically:
>> * Fixes timing/locking issues with the previously upstreamed (but
>>   never used upstream) cros_ec_spi driver.
>> * Updates the cros_ec header file to the latest version which allows
>>   us to use newer EC features like i2c tunneling.
>> * Adds an i2c tunnel driver to allow communication to the EC's i2c
>>   devices.
>>
>> This _doesn't_ get the EC driver fully up to speed with what's in the
>> current Chromium OS trees.  There are a whole slew of cleanup patches
>> there, an addition of an LPC transport mode, and exports of functions
>> to userspace.  Once these patches land and we have functionality we
>> can continue to pick more cleanup patches.
...
> Need to wait for the ARM, DT and I2C guys to review, at which point
> I'll be happy to take in and supply a branch for them to pull from if
> required. If there are no _true_ dependencies and the MFD changes can
> be added independently without fear of build breakages, let me know
> and I'll apply them separately.

I believe there aren't direct dependencies between the patches. So, the
MFD patches can be applied to the MFD tree and the DT patch applied to
the Tegra tree. I'm simply waiting for the MFD patches to be applied
before applying the DT patch so that I know the DT binding definition is
fully accepted before applying a patch that uses it.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH 6/7] i2c: ChromeOS EC tunnel driver

[Stephen Warren]

On 04/17/2014 12:36 PM, Doug Anderson wrote:
> On ARM Chromebooks we have a few devices that are accessed by both the
> AP (the main "Application Processor") and the EC (the Embedded
> Controller).  These are:
> * The battery (sbs-battery).
> * The power management unit tps65090.
...
> On the Samsung ARM Chromebook 2 the scheme is changed yet again, now:
> * The AP/EC comms are now using SPI for faster speeds.
> * The EC's i2c bus is exposed to the AP through a full i2c tunnel.
...
> This driver supports the scheme used by the Samsung ARM Chromebook 2.
> Future patches to this driver could add support for the battery tunnel
> on the HP Chromebook 11 (and perhaps could even be used to access
> tps65090 on the HP Chromebook 11 instead of using a special driver,
> but I haven't researched that enough).

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt 
> b/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

> +I2C bus that tunnels through the ChromeOS EC (cros-ec)
> +==
> +On some ChromeOS board designs we've got a connection to the EC (embedded
> +controller) but no direct connection to some devices on the other side of
> +the EC (like a battery and PMIC).  To get access to those devices we need
> +to tunnel our i2c commands through the EC.
> +
> +The node for this device should be under a cros-ec node like 
> google,cros-ec-spi
> +or google,cros-ec-i2c.
> +
> +
> +Required properties:
> +- compatible: google,cros-ec-i2c-tunnel
> +- google,remote-bus: The EC bus we'd like to talk to.

It's probably worth mentioning here that the node represents a single
I2C bus, and hence is expected to contain child nodes representing I2C
devices. Perhaps:

Optional child nodes:
- One node per I2C device connected to the tunnelled I2C bus.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: I2C dummy adapter driver ?

[Stephen Warren]

On 02/19/2014 04:03 PM, Sylwester Nawrocki wrote:
> Hi All,
> 
> I was wondering whether it would be reasonable to create a generic
> Linux dummy I2C bus controller driver. The rationale behind it is
> there might be hardware configurations where I2C communication is
> handled by firmware but still it is useful to have I2C slave devices
 

That doesn't sound like a dummy driver. Rather, it sounds like a driver
for the firmware(?) interface that's used to do the I2C transactions.

> instantiated by a Linux I2C bus adapter driver.
> It would be useful to have the common devicetree I2C binding scheme
> working regardless of where the I2C communication is handled - in
> firmware or by the host CPU.
> 
> Currently we have a somewhat dummy I2C adapter driver at drivers/media/
> platform/exynos4-is/fimc-is-i2c.c. But it is going to be needed by
> multiple SoCs and I thought about separating it or creating a generic
> dummy I2C adapter driver.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: bcm2835: Use devm_ioremap_resource()

[Stephen Warren]

On 02/11/2014 06:02 AM, Jingoo Han wrote:
> Use devm_ioremap_resource() in order to make the code simpler,
> and remove redundant return value check of platform_get_resource()
> because the value is checked by devm_ioremap_resource().

Acked-by: Stephen Warren 
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 16/17] i2c: i2c-tegra: deprecate class based instantiation

[Stephen Warren]

On 02/10/2014 03:04 AM, Wolfram Sang wrote:
> Warn users that class based instantiation is going away soon in favour
> of more robust probing and faster bootup times.

Acked-by: Stephen Warren 

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: Re-instate body of i2c_parent_is_i2c_adapter()

[Stephen Warren]

On 01/14/2014 09:12 AM, Wolfram Sang wrote:
> On Mon, Jan 13, 2014 at 02:29:04PM -0700, Stephen Warren wrote:
>> From: Stephen Warren 
>>
>> The body of i2c_parent_is_i2c_adapter() is currently guarded by
>> CONFIG_I2C_MUX instead.
> 
> This paragraph sounds strange to me. I'll update it a little. After that
> I'll go looking for a brown paper bag...
> 
>> Among potentially other problems, this resulted in i2c_lock_adapter()
>> only locking I2C mux child adapters, and not the parent adapter. In
>> turn, this could allow inter-mingling of mux child selection and I2C
>> transactions, which could result in I2C transactions being directed to
>> the wrong I2C bus, and possibly even switching between busses in the
>> middle of a transaction.
>>
>> One concrete issue caused by this bug was corrupted HDMI EDID reads
>> during boot on the NVIDIA Tegra Seaboard system, although this only
>> became apparent in recent linux-next, when the boot timing was changed
>> just enough to trigger the race condition.
>>
>> Fixes: 3923172b3d70 ("i2c: reduce parent checking to a NOOP in non-I2C_MUX 
>> case")
>> Cc: Phil Carmody 
>> Cc: 
>> Signed-off-by: Stephen Warren 
> 
> Applied to for-current, thanks for catching this one!

I do see this in for-current, but it looks like that branch isn't part
of linux-next. Should it be, or perhaps for-current should be merged
into for-next?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] i2c: Re-instate body of i2c_parent_is_i2c_adapter()

[Stephen Warren]

From: Stephen Warren 

The body of i2c_parent_is_i2c_adapter() is currently guarded by
CONFIG_I2C_MUX instead.

Among potentially other problems, this resulted in i2c_lock_adapter()
only locking I2C mux child adapters, and not the parent adapter. In
turn, this could allow inter-mingling of mux child selection and I2C
transactions, which could result in I2C transactions being directed to
the wrong I2C bus, and possibly even switching between busses in the
middle of a transaction.

One concrete issue caused by this bug was corrupted HDMI EDID reads
during boot on the NVIDIA Tegra Seaboard system, although this only
became apparent in recent linux-next, when the boot timing was changed
just enough to trigger the race condition.

Fixes: 3923172b3d70 ("i2c: reduce parent checking to a NOOP in non-I2C_MUX 
case")
Cc: Phil Carmody 
Cc: 
Signed-off-by: Stephen Warren 
---
 include/linux/i2c.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index eff50e062be8..d9c8dbd3373f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -445,7 +445,7 @@ static inline void i2c_set_adapdata(struct i2c_adapter 
*dev, void *data)
 static inline struct i2c_adapter *
 i2c_parent_is_i2c_adapter(const struct i2c_adapter *adapter)
 {
-#if IS_ENABLED(I2C_MUX)
+#if IS_ENABLED(CONFIG_I2C_MUX)
struct device *parent = adapter->dev.parent;
 
if (parent != NULL && parent->type == &i2c_adapter_type)
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dt: Update I2C trivial devices list

[Stephen Warren]

On 01/13/2014 07:25 AM, Ben Gamari wrote:
> Wolfram Sang  writes:
> 
>> On Tue, Dec 17, 2013 at 11:47:19AM -0500, Ben Gamari wrote:
>>> Stephen Warren  writes:
>>>
>>>> On 12/16/2013 05:12 PM, Ben Gamari wrote:
>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt 
>>>>> b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
>>>>
>>>>>  This is a list of trivial i2c devices that have simple device tree
>>>>>  bindings, consisting only of a compatible field, an address and
>>>>> -possibly an interrupt line.
>>>>> +possibly an interrupt line. The compatible field is used to lookup the
>>>>> +modalias of the driver which will handle the device. The compatible
>>>>> +string may begin with a manufacturer prefix (separated from the
>>>>> +modalias by a comma) which will be stripped off during lookup.
>>>>
>>>> This part of the patch described Linux-specific behaviour, whereas DT
>>>> bindings should be OS-agnostic.
>>>>
>>> I see your point but it would be nice to have language like this
>>> somewhere. One of the biggest challenges in diving in to writing a
>>> devicetree is figuring out details like this.
>>
>> What about putting this into a seperate paragraph prefixed with "Current
>> Linux specific implementation:"?
>>
> This would address my concern quite nicely.

The text should go into Documentation/ not the DT binding.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/13] i2c: bcm2835: Use devm_request_irq()

[Stephen Warren]

On 12/16/2013 11:46 PM, Jingoo Han wrote:
> Use devm_request_irq() to make cleanup paths simpler.

This may not be safe. The interrupt used by the I2C controllers on the
BCM2835 chip is shared between two controllers. In theory, you could run
into a condition where you're remove()ing the driver for one of the
controller (a), while the driver for controller (b) is still active, yet
the HW for controller (a) comes along and triggers an interrupt after
remove() but before the devm IRQ cleanup. That would result in the IRQ
handler for controller (a) executing after the remove() of the
associated device, which could cause a variety of problems.

You might be able to make this safe by explicitly clearing any IRQ
enable registers in remove(), but leaving the code using plain
request_irq() might be simpler.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dt: Update I2C trivial devices list

[Stephen Warren]

On 12/16/2013 05:12 PM, Ben Gamari wrote:

> diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt 
> b/Documentation/devicetree/bindings/i2c/trivial-devices.txt

>  This is a list of trivial i2c devices that have simple device tree
>  bindings, consisting only of a compatible field, an address and
> -possibly an interrupt line.
> +possibly an interrupt line. The compatible field is used to lookup the
> +modalias of the driver which will handle the device. The compatible
> +string may begin with a manufacturer prefix (separated from the
> +modalias by a comma) which will be stripped off during lookup.

This part of the patch described Linux-specific behaviour, whereas DT
bindings should be OS-agnostic.

BTW, you didn't CC the DT binding maintainers.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] I2C: BCM2835: Linking platform nodes to adapter nodes

[Stephen Warren]

On 11/08/2013 09:59 AM, Stephen Warren wrote:
> On 11/08/2013 02:49 AM, Florian Meier wrote:
>> In order to find I2C devices in the device tree, the platform nodes
>> have to be known by the I2C core. Analogous to the i2c-omap driver
>> this requires setting the dev.of_node parameter of the adapter.
> 
> (CCing the I2C maintainers...)
> 
>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c 
>> b/drivers/i2c/busses/i2c-bcm2835.c
> 
>> @@ -299,6 +299,7 @@ static int bcm2835_i2c_probe(struct platform_device 
>> *pdev)
>>  strlcpy(adap->name, "bcm2835 I2C adapter", sizeof(adap->name));
>>  adap->algo = &bcm2835_i2c_algo;
>>  adap->dev.parent = &pdev->dev;
>> +adap->dev.of_node = pdev->dev.of_node;
> 
> Ah, that makes sense. Thinking about it now, I'd only ever used i2cget
> etc. to access I2C devices, rather than instantiating drivers from DT.
> 
> That all said, I wonder if the I2C core shouldn't do something like the
> following inside i2c_add_adapter():
> 
> if (!adap->dev.of_node && adap->dev.parent)
>   adap->dev.of_node = adap->dev.parent->of_node;
> 
> That would save every single I2C driver from having to set up this field
> manually.

BTW, this should probably be:
Cc: sta...@vger.kernel.org
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] I2C: BCM2835: Linking platform nodes to adapter nodes

[Stephen Warren]

On 11/25/2013 01:01 AM, Florian Meier wrote:
> In order to find I2C devices in the device tree, the platform nodes
> have to be known by the I2C core. This requires setting the
> dev.of_node parameter of the adapter.

Acked-by: Stephen Warren 
Tested-by: Stephen Warren 

(Interestingly, I just attached an I2C light sensor to my Pi a couple
days back, so ended up needing this commit, and the patches someone else
had very recently sent to add DT support to the sensor driver. My timing
was impeccable:-)
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 20/31] i2c: tegra: use reset framework

[Stephen Warren]

From: Stephen Warren 

Tegra's clock driver now provides an implementation of the common
reset API (include/linux/reset.h). Use this instead of the old Tegra-
specific API; that will soon be removed.

Cc: tred...@nvidia.com
Cc: pdeschrij...@nvidia.com
Cc: linux-te...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: Wolfram Sang 
Cc: linux-i2c@vger.kernel.org
Signed-off-by: Stephen Warren 
---
This patch is part of a series with strong internal depdendencies. I'm
looking for an ack so that I can take the entire series through the Tegra
and arm-soc trees. The series will be part of a stable branch that can be
merged into other subsystems if needed to avoid/resolve dependencies.
---
 drivers/i2c/busses/i2c-tegra.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index e661edee4d0c..9704537aee3c 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -27,7 +27,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 
 #include 
 
@@ -160,6 +160,7 @@ struct tegra_i2c_dev {
struct i2c_adapter adapter;
struct clk *div_clk;
struct clk *fast_clk;
+   struct reset_control *rst;
void __iomem *base;
int cont_id;
int irq;
@@ -415,9 +416,9 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
return err;
}
 
-   tegra_periph_reset_assert(i2c_dev->div_clk);
+   reset_control_assert(i2c_dev->rst);
udelay(2);
-   tegra_periph_reset_deassert(i2c_dev->div_clk);
+   reset_control_deassert(i2c_dev->rst);
 
if (i2c_dev->is_dvc)
tegra_dvc_init(i2c_dev);
@@ -743,6 +744,12 @@ static int tegra_i2c_probe(struct platform_device *pdev)
i2c_dev->cont_id = pdev->id;
i2c_dev->dev = &pdev->dev;
 
+   i2c_dev->rst = devm_reset_control_get(&pdev->dev, "i2c");
+   if (IS_ERR(i2c_dev->rst)) {
+   dev_err(&pdev->dev, "missing controller reset");
+   return PTR_ERR(i2c_dev->rst);
+   }
+
ret = of_property_read_u32(i2c_dev->dev->of_node, "clock-frequency",
&i2c_dev->bus_clk_rate);
if (ret)
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] I2C: BCM2835: Linking platform nodes to adapter nodes

[Stephen Warren]

On 11/08/2013 02:49 AM, Florian Meier wrote:
> In order to find I2C devices in the device tree, the platform nodes
> have to be known by the I2C core. Analogous to the i2c-omap driver
> this requires setting the dev.of_node parameter of the adapter.

(CCing the I2C maintainers...)

> diff --git a/drivers/i2c/busses/i2c-bcm2835.c 
> b/drivers/i2c/busses/i2c-bcm2835.c

> @@ -299,6 +299,7 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
>   strlcpy(adap->name, "bcm2835 I2C adapter", sizeof(adap->name));
>   adap->algo = &bcm2835_i2c_algo;
>   adap->dev.parent = &pdev->dev;
> + adap->dev.of_node = pdev->dev.of_node;

Ah, that makes sense. Thinking about it now, I'd only ever used i2cget
etc. to access I2C devices, rather than instantiating drivers from DT.

That all said, I wonder if the I2C core shouldn't do something like the
following inside i2c_add_adapter():

if (!adap->dev.of_node && adap->dev.parent)
adap->dev.of_node = adap->dev.parent->of_node;

That would save every single I2C driver from having to set up this field
manually.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Stephen Warren]

On 10/01/2013 04:39 AM, Maxime COQUELIN wrote:
> This patch adds support to SSC (Synchronous Serial Controller)
> I2C driver. This IP also supports SPI protocol, but this is not
> the aim of this driver.
> 
> This IP is embedded in all ST SoCs for Set-top box platorms, and
> supports I2C Standard and Fast modes.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt 
> b/Documentation/devicetree/bindings/i2c/i2c-st.txt

> +Required properties :

> +- clocks : phandle to the I2C clock source
> +- clock-names : from common clock binding: Shall be "ssc"

I'd prefer to define that as:

clock-names: Must contain "ssc".
clocks: Must contain an entry for each name in clock-names. See the
common clock bindings.

That way, it makes it clear that clock-names is the primary lookup
mechanism, rather than some auxiliary documentation.

> +Recommended properties :
> +- clock-frequency : Desired I2C bus clock frequency in Hz. Otherwise

s/Otherwise/If not specified,/

> +  the default 100 kHz frequency will be used. As only Normal and Fast modes
> +  are supported, possible values are 10 and 40.

I think that's just optional. Since there's a well-defined sensible
default, there's no need to recommend it.

> +Optional properties :
> +- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is 
> allowed
> +  through the deglitch circuit. In units of us.
> +- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is 
> allowed
> +  through the deglitch circuit. In units of us.

Are those properties specific to this binding, or intended to be
generic? If specific to this binding, a vendor prefix should be present
in the property name. If not, you probably want to document the
properties in some common file.

> +Examples :

s/Examples/Example/ since there's just one.

> +i2c0: i2c@fed4 {
> + compatible  = "st,comms-ssc-i2c";
> + reg = <0xfed4 0x110>;
> + interrupts  =  ;
> + clocks  = <&CLK_S_ICN_REG_0>;
> + clock-names = "ssc";
> + clock-frequency = <40>;
> + pinctrl-names   = "default";
> + pinctrl-0   = <&pinctrl_i2c0_default>;

That wasn't mentioned in the binding definition. You'd probably want to
document the requirement for those two properties by saying something like:

A pinctrl state named "default" must be defined, using the bindings in
../pinctrl/pinctrl-binding.txt.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] binding for nvec mfd device

[Stephen Warren]

On 09/24/2013 03:33 AM, Marc Dietrich wrote:
...
> What I learned from the somehow unfortune discussion yesterday on IRC is that 
> we cannot describe the exact i2c topology in the device tree, especially if 
> it 
> is software configuable (slave/master changing roles).

Well, there's also no need to represent the exact I2C topology; we only
need to represent the parts that SW cares about. The existence of other
masters on the bus is not something that SW typically cares about. SW
can't directly interact with other masters (through just plain I2C
protocol at least), and can only observe their presence by potential
arbitration loss events.

> I also think there is a misunderstanding about how things get layered in 
> software (and which info from device tree the drivers need). In your example 
> above, there is a child node unter each controller which is responsible for 
> slave transfers. The nvec can be a separate node in the device tree outside 
> of 
> the i2c structure, but needs one or more pointers to slave drivers which he 
> can use to send his command byte streams to. E.g.

No, in my example, the slave node /is/ the NVEC protocol. There is no
separate NVEC node outside the I2C node/tree.

> i2c@ {
>   compatible = "nvidia,tegra-i2c";
>   // resources for the i2c controller
>   first_slave: slave@40 {
>   compatible = "nvidia,tegra-i2c-slave";

That's not what I wrote. This should be:

compatible = "nvidia,nvec-slave";

... which instantiates an I2C slave protocol driver, and hooks it up so
that if the I2C slave controller receives an externally-initiated I2C
transaction to I2C address 0x40, that transaction will be passed to the
NVEC driver for processing and response.


>   reg = <40>; // i2c client address
>   gpios = <4e>; // for side channel
>   };
> };

...
> external_master {
>   compatible = "ext,master";
>   slaves = &first_slave,&second_slave;
> };
> 
> or
> 
> nvec {
>   compatible = "nvidia,nvec";
>   slave = &first_slave;
>   keyboard {
>   // keyboard resources
>   };
> };

So that separate nvec node is not required. All configuration of the
NVEC slave protocol should be part of the slave@40 node above.

IIRC, the way NVEC works is that a GPIO is used to request that the NVEC
IC poll the slave device if the slave device wants to initiate a
transaction with the NVEC IC. If that's correct, the model above should
work fine.

However, if when Tegra wants to make a master transaction on the I2C bus
(which would therefore have to be to a device other than the NVEC IC),
if the NVEC IC and Tegra need to arbitrate for the bus outside the plain
I2C protocol, then you'd want to model this as an I2C bus mux instead:

i2c1: i2c@ {
compatible = "nvidia,tegra-i2c";

foo@40 {
reg = ;
compatible = "nvidia,nvec-slave";
keyboard {
...
};
// any other parameters NVEC slave
// protocol needs go here
}
};

i2cmux {
compatible = "nvidia,i2c-mux-nvec";
#address-cells = <1>;
#size-cells = <0>;
nvec-gpios = <&gpio1 22 0>;
i2c-parent = <&i2c1>;

// all other devices on the I2C bus go here:
ssd1307: oled@3c {
compatible = "solomon,ssd1307fb-i2c";
reg = <0x3c>;
pwms = <&pwm 4 3000>;
reset-gpios = <&gpio2 7 1>;
reset-active-low;
};
};
};

In this case, perhaps the drivers for the NVEC bus mux and NVEC slave
protocol might need to communicate with each-other. In which case, a
phandle between the two may be appropriate.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] binding for nvec mfd device

[Stephen Warren]

On 09/24/2013 01:19 AM, Andrey Danin wrote:
> On Mon, Sep 23, 2013 at 8:36 PM, Stephen Warren  I think you'd just have the following
> 
> /* master */
> i2c@x {
> foo@0x40 {
> reg = ;
> compatible = "nvidia,nvec";
> }
> };
> 
> i2c@ {
> foo@40 {
> reg = ;
> compatible = "nvidia,nvec-slave";
> }
> };
> 
> There's no need for the slave child node to know that it is mastered
> from the Tegra I2C controller; all it cares about is that there is some
> I2C bus that it needs to respond to transactions upon.
> 
> This binding describes only case, when I2C device are connected to I2C
> controller.
> 
> Assume that I2C controller #1 (@x), I2C controller #2 (@), and
> nvec I2C master device are connected to same bus.
> How dt must be composed in this case ? Must i2c@x and i2c@ be in
> parent/child relation (in terms of dt) ?

None of the I2C bindings currently allow one to specify that multiple of
the on-SoC controllers are connected to the same bus.

I'm not sure it's particularly useful to represent this anyway. Hardware
hooked up like this is pretty rare to start with (i.e. I know of no
board at all that's connected this way). I assume that if such HW did
exist, you'd simply assign each I2C slave to a particular I2C master,
and hence only put a DT node for it under a single DT master node.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Stephen Warren]

On 09/18/2013 04:01 AM, Maxime COQUELIN wrote:
> This patch adds support to SSC (Synchronous Serial Controller)
> I2C driver. This IP also supports SPI protocol, but this is not
> the aim of this driver.
> 
> This IP is embedded in all ST SoCs for Set-top box platorms, and
> supports I2C Standard and Fast modes.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt 
> b/Documentation/devicetree/bindings/i2c/i2c-st.txt

> +I2C for ST platforms

If the HW block supports both I2C and SPI, the DT binding ought to
support that too. It's be best to create a single DT binding that
represents the IP block, and include a property that indicates whether
the device should operate in I2C or SPI mode.

I suppose you could reasonably define different compatible values for
those two cases. However, the binding should be titled something more
like "ST SSC binding, for I2C mode operation" and "ST SSC binding, for
SPI mode operation", rather than "I2C for ST platforms", since the HW
includes an SSC block, not an I2C block.

> +Required properties :
> +- compatible : Must be "st,comms-i2c"
> +- reg : Offset and length of the register set for the device
> +- interrupts : the interrupt number

It's an interrupt specifier, not an interrupt number. The format is
defined by the interrupt controller, not this binding.

> +- clocks : phandle to the I2C clock source

What about clock-names?

> +Recommended (non-standard) properties :

Usually you'd just say "Optional properties:", or perhaps "Recommended
properties:". I don't think adding "(non-standard)" serves any purpose.

> +- clock-frequency : Desired I2C bus clock frequency in Hz. Otherwise
> +  the default 100 kHz frequency will be used. As only Normal and Fast modes
> +  are supported, possible values are 10 and 40.
> +
> +Optional (non-standard) properties:

Same here.

> +- st,glitches : Enable timing glitch suppression support.

That property name doesn't really convey the "enables" that appears in
the property description to me...

> +- st,glitch-clk : SCL line timinig glitch suppression value in ns.
> +- st,glitch-dat : SDA line timinig glitch suppression value in ns.

s/timinig/timing/

> +- st,hw-glitches : Enable filter glitch suppression support.
> +- st,hw-glitch-clk : SCL line filter glitch suppression value in us.
> +- st,hw-glitch-dat : SDA line filter glitch suppression value in us.

Those sound more like runtime configuration rather than HW description.
Can you rephrase the descriptions (and property names) more along the
lines of HW properties? Perhaps e.g.:

st,needs-glitch-suppression: The board design needs timing glitch
suppression enabled to operate reliably.

st,min-scl-pulse-width: The minimum valid SCL pulse width that is
allowed through the deglitch circuit. In units of ns.

(I just made up those descriptions to give a feel for the flavor of
description that I expect. They likely need some adjustment to reflect
whatever they're actually intended to represent in HW).

What is the difference between "glitch" and "hw-glitch"?

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Stephen Warren]

On 09/18/2013 06:47 AM, Gabriel FERNANDEZ wrote:
> On 18/09/2013 12:01, Maxime COQUELIN wrote:
>> This patch adds support to SSC (Synchronous Serial Controller)
>> I2C driver. This IP also supports SPI protocol, but this is not
>> the aim of this driver.
>>
>> This IP is embedded in all ST SoCs for Set-top box platorms, and
>> supports I2C Standard and Fast modes.
> ... [entire patch quoted for a one-line comment] ...

Please trim down the patch so you only quote relevant lines. Quoting the
entire patch for a 1-line comment makes people wast time searching for
your reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] binding for nvec mfd device

[Stephen Warren]

On 09/23/2013 08:52 AM, Marc Dietrich wrote:
> Am Dienstag, 17. September 2013, 15:48:12 schrieb Stephen Warren:
>> On 09/17/2013 01:53 AM, Marc Dietrich wrote:
>>> Hi Stephen,
>>>
>>> Am Mittwoch, 31. Juli 2013, 15:03:14 schrieb Stephen Warren:
>>>> The generic I2C bindings already define that the other chips on the I2C
>>>> bus appear directly underneath the I2C controller's DT node. Perhaps it
>>>> isn't a big issue to change that, since each I2C controller can define
>>>> the layout of its own node?
>>>>
>>>> Anyway, we can probably get away without introducing multiple levels by
>>>> adding some more bits or cells into the reg address for I2C child nodes:
>>>>
>>>> i2c@ {
>>>>
>>>>compatible = "nvidia,tegra20-i2c";
>>>>... resources
>>>>#address-cells = <2>;
>>>>
>>>>codec {
>>>>
>>>>// 0 means external slave, 0x1c is slave's address
>>>>reg = <0 0x1c>;
>>>>...
>>>>
>>>>};
>>>>
>>>>tegraslave {
>>>>
>>>>// 0 means internal slave, 0x80 is controller's address
>>>>reg = <1 0x80>;
>>>>...
>>>>
>>>>};
>>>>
>>>> };
>>>>
>>>> ... where each of those child nodes could be repeated N times. We could
>>>> also or in 0x8000 to the reg values in the child nodes rather than
>>>> using a separate cell if we wanted.
>>>
>>> thinking a little more about this, this is way to complicated.
>>
>> Really, this seems extremely simple to me.
>>
>>> "Master" and
>>> "Slave" functions are properties of the same i2c controller. Therefore,
>>> just adding a small property to the i2c controller node saying "enable
>>> slave support" is sufficient, as all the resources are shared (which is
>>> another good argument that it is the same device, hence same node). I
>>> would just add the slave i2c address, which is all the slave driver
>>> needs, e.g.
>>
>> Perhaps so for this one controller, but we should strive to create a
>> binding style that can work with any I2C controller that can be master
>> or slave. For a general binding, I think we need to support multiple
>> slave addresses. The style above seems to support that with little
>> complexity.
> 
> I have no idea how other implementations may look like. The downstream kernel 
> seems to program the slave address in a secondary step after the controller 
> is 
> initialized [1]. So every "client" which binds to the slave driver can use 
> its 
> own address, but only one client at the same time is allowed. As said before, 
> the may also exist devices (!tegra) with multiple slave addresses at the same 
> time.
> 
> Your approach above seems fulfill these properties (what about a single slave 
> controller without a master?),

There would simply be a node for the controller, and a single child node
for the slave setup, and no child nodes for mastered devices.

> but it will be tricky for the slave to find the
> controller resources it needs.

I don't understand this. The resources are only needed by I2C driver,
and all come from the I2C driver node. If the I2C driver ends up being
split into separate master/slave portions, that information can easily
be passed from the main driver probe()/... to those other sub-parts of
the driver.

> I'm thinking of an insane (but valid)
> configuration where all i2c ports of the SoC are connected together and one 
> port plays the master and all others are slaves. In this case we need to add 
> a 
> property to the slave node which points to his controller instance.

Why do the slave nodes care where they're mastered from?

I think you'd just have the following

/* master */
i2c@x {
foo@0x40 {
reg = ;
compatible = "nvidia,nvec";
}
};

i2c@ {
foo@40 {
reg = ;
compatible = "nvidia,nvec-slave";
}
};

There's no need for the slave child node to know that it is mastered
from the Tegra I2C controller; all it cares about is that there is some
I2C bus that it needs to respond to transactions upon.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] binding for nvec mfd device

[Stephen Warren]

On 09/17/2013 01:53 AM, Marc Dietrich wrote:
> Hi Stephen,
> 
> Am Mittwoch, 31. Juli 2013, 15:03:14 schrieb Stephen Warren:
>> The generic I2C bindings already define that the other chips on the I2C
>> bus appear directly underneath the I2C controller's DT node. Perhaps it
>> isn't a big issue to change that, since each I2C controller can define
>> the layout of its own node?
>>
>> Anyway, we can probably get away without introducing multiple levels by
>> adding some more bits or cells into the reg address for I2C child nodes:
>>
>> i2c@ {
>>  compatible = "nvidia,tegra20-i2c";
>>  ... resources
>>  #address-cells = <2>;
>>
>>  codec {
>>  // 0 means external slave, 0x1c is slave's address
>>  reg = <0 0x1c>;
>>  ...
>>  };
>>
>>  tegraslave {
>>  // 0 means internal slave, 0x80 is controller's address
>>  reg = <1 0x80>;
>>  ...
>>  };
>> };
>>
>> ... where each of those child nodes could be repeated N times. We could
>> also or in 0x8000 to the reg values in the child nodes rather than
>> using a separate cell if we wanted.
> 
> thinking a little more about this, this is way to complicated.

Really, this seems extremely simple to me.

> "Master" and 
> "Slave" functions are properties of the same i2c controller. Therefore, just 
> adding a small property to the i2c controller node saying "enable slave 
> support" is sufficient, as all the resources are shared (which is another 
> good 
> argument that it is the same device, hence same node). I would just add the 
> slave i2c address, which is all the slave driver needs, e.g.

Perhaps so for this one controller, but we should strive to create a
binding style that can work with any I2C controller that can be master
or slave. For a general binding, I think we need to support multiple
slave addresses. The style above seems to support that with little
complexity.

> "slave-address = <8c>;" to enable slave mode operation. Both modes can be 
> enabled at the same time (for loopback comm). AFAIK, if only the slave part 
> is 
> used, the master part shouldn't hurt if programmed (same as other way around).
> 
> The difficult part is how to specify the master if only the slave is used. As 
> the master is naturally on the same bus as the slave, it should be a child 
> node of the controller. Therefore it needs a special i2c address, maybe 

Why would the external I2C master be represented in the DT bindings at
all? It's not really a SW-accessible entity; its presence is only
observable not something you can initiate interaction with.

I assume you're not talking about the Tegra I2C controller when you say
"master" above; that is already represented in the DT as the top-level
I2C node.

> 0x8000 as you mentioned above, but this will be rejected by the i2c core. 
> Instantiating the nvec as an i2c client could be another problem.
...
> It looks like that the i2c subsystem needs some modifications to allow to 
> specify a master device which is attached to a slave controller. Maybe 
> someone 
> from the i2c folks (cc'ed) can comment on this?

Yes, you certainly will need some enhancements to the I2C core to
support slave mode. I suggest that I2C drivers gain an .of_xlate
callback which translates child node DT reg values (addresses) into the
(master-vs-slave, I2C address) pair.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Stephen Warren]

On 08/13/2013 01:46 AM, s.ha...@pengutronix.de wrote:
> On Mon, Aug 12, 2013 at 05:23:35PM -0600, Stephen Warren wrote:
>> On 08/12/2013 10:43 AM, Mark Rutland wrote:
>>>> The binding string for i2c-imx driver in 
>>>> Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
>>>> of "- compatible : Should be "fsl,-i2c" " for device using this 
>>>> driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
>>>> is described in the binding document. So I just leave the vf610 i2c 
>>>> compatible with this. 
>>>
>>> I'm not a big fan on wildcards in bindings, as it leaves people free to
>>> put anything in and claim it's a documented binding, and makes it far
>>> harder for an os to actually implement drivers for said binding, as
>>> there's no canonical reference for the set of valid variations.
>>>
>>> Obviously there is some precedent, but I'm not sure it's something we
>>> want to stick with, and we can prevent it my updating the documentation
>>> now.
>>>
>>> Does anyone else have an opinion?
>>
>> I suppose technically we should list out every exact string in the
>> binding, but it's a little annoying to have to update the binding doc
>> every time a new chip comes out (and I expect that'll happen more and
>> more!) just to add a new compatible value since all the differences are
>> known internally to the driver and don't impact the binding...
> 
> We would only have to update the the docs when an incompatible SoC comes
> out. For this particular driver this would be all marked with a star:
> 
> * i.MX1
> * i.MX21
>   i.MX25
>   i.MX27
>   i.MX31
>   i.MX35
>   i.MX51
>   i.MX53
>   i.MX6
> * Vybrid
> 
> That's not too many updates to the binding docs since 2001.
> (The SPI core changed with nearly every SoC version though)

So, the SPI core changed its HW implementation, or changed its
SW-visible interface? If the latter, then you need a separate compatible
value for each, which was my point.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Stephen Warren]

On 08/12/2013 10:43 AM, Mark Rutland wrote:
> [Adding other devicetree maintainers to Cc]
> 
> On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
>>
>> 
>>> 发件人: Mark Rutland [mark.rutl...@arm.com]
>>> 发送时间: 2013年8月10日 22:08
>>> 收件人: Lu Jingchang-B35083
>>> 抄送: w...@the-dreams.de; Estevam Fabio-R49496; Li Xiaochun-B41219; 
>>> s.ha...@pengutronix.de; linux-i2c@vger.kernel.org; Jin Zhengxiong-R64188; 
>>> shawn@linaro.org; linux-arm-ker...@lists.infradead.org
>>> 主题: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
>>
>>> On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
   Add Freescale Vybrid VF610 I2C controller support to
 imx I2C driver framework.
   Some operation is different from imx I2C controller.
 The register offset, the i2c clock divider value table,
 the module enabling(I2CR_IEN) which is just invert with imx,
 and the interrupt flag(I2SR) clearing opcode is w1c on VF610
 but w0c on imx.

  static const struct of_device_id i2c_imx_dt_ids[] = {
 { .compatible = "fsl,imx1-i2c", .data = 
 &imx_i2c_devtype[IMX1_I2C], },
 { .compatible = "fsl,imx21-i2c", .data = 
 &imx_i2c_devtype[IMX21_I2C], },
 +   { .compatible = "fsl,vf610-i2c", .data = 
 &imx_i2c_devtype[VF610_I2C], },
>>>
>>> That string doesn't seem to be documented anywhere (from a quick grep of
>>> Documentation/devicetree), and there's no binding update included
>>> here. It would be nice for that to be fixed :)
>>
>> [Lu Jingchang]
>> The binding string for i2c-imx driver in 
>> Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
>> of "- compatible : Should be "fsl,-i2c" " for device using this 
>> driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
>> is described in the binding document. So I just leave the vf610 i2c 
>> compatible with this. 
> 
> I'm not a big fan on wildcards in bindings, as it leaves people free to
> put anything in and claim it's a documented binding, and makes it far
> harder for an os to actually implement drivers for said binding, as
> there's no canonical reference for the set of valid variations.
> 
> Obviously there is some precedent, but I'm not sure it's something we
> want to stick with, and we can prevent it my updating the documentation
> now.
> 
> Does anyone else have an opinion?

I suppose technically we should list out every exact string in the
binding, but it's a little annoying to have to update the binding doc
every time a new chip comes out (and I expect that'll happen more and
more!) just to add a new compatible value since all the differences are
known internally to the driver and don't impact the binding...

Kumar's idea could be a reasonable compromise, although I guess it
doesn't technically cover the case of there being 10 chips, each of
which has *some* new/different IP block, yet some IP blocks not changing
in every chip, and hence drivers might only directly binding to 5 of the
10 possible compatible values in some cases...
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] mfd: tps65910: Fix crash in i2c_driver .probe

[Stephen Warren]

On 06/18/2013 04:14 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.
> 
> Moves the of_match_device call from tps65910_parse_dt to .probe to
> allow the chip type to be detected from device tree but with the
> device parameters coming from platform data.

Reviewed-by: Stephen Warren 

It's a pity that every driver that supports DT is going to need this
boiler-plate though. Perhaps the I2C core can acquire a follow-on patch
that does this for drivers in the future.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: core: make it possible to match a pure device tree driver

[Stephen Warren]

On 06/17/2013 04:15 PM, Grant Likely wrote:
> On Mon, Jun 17, 2013 at 5:33 PM, Linus Walleij  
> wrote:
>> On Mon, Jun 17, 2013 at 5:48 PM, Stephen Warren  
>> wrote:
>>
>>> This has just shown up in next-20130617, and breaks at least the
>>> TPS65910 and TPS62360 drivers, since they assume that the id parameter
>>> passed to probe is non-NULL. However, now the parameter is NULL since
>>> these drivers have both an ID table and an OF match table.
>>
>> So you mean they come in through the DT boot path and assume
>> that parameter is non-null even though they should not make use of
>> it?
>>
>>> I'd like to suggest this patch be reverted an re-introduced immediately
>>> after the merge window. That should give enough time for everyone to get
>>> a heads-up on fixing any drivers with a similar problem, rather than
>>> trying to cram all that in immediately before the merge window.
>>
>> OK that works for me, I'm not in any hurry.
> 
> Deferring by a merge window isn't going to make it any less painful.

It'll give a lot more time for people to be exposed to the change and
hence find/fix it linux-next rather than seeing it for the first time in
Linus's tree.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe

[Stephen Warren]

On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.

Tested-by: Stephen Warren 

(although I imagine I'd need to retest if there was a v2 to address my
previous comments)
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] regulator: tps62360: Fix crash in i2c_driver .probe

[Stephen Warren]

On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.

Tested-by: Stephen Warren 
Reviewed-by: Stephen Warren 

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mfd: tps65910: Fix crash in i2c_driver .probe

[Stephen Warren]

On 06/17/2013 11:47 AM, Tuomas Tynkkynen wrote:
> Commit "i2c: core: make it possible to match a pure device tree driver"
> changed semantics of the i2c probing for device tree devices.
> Device tree probed devices now get a NULL i2c_device_id pointer.
> This caused kernel panics due to NULL dereference.

> diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c

>   pmic_plat_data = dev_get_platdata(&i2c->dev);
>  
> - if (!pmic_plat_data && i2c->dev.of_node) {
> + if (id) {
> + chip_id = id->driver_data;
> + } else if (i2c->dev.of_node) {
>   pmic_plat_data = tps65910_parse_dt(i2c, &chip_id);

That over-writes pmic_plat_data even if it was already set above. This
should really only happen if the earlier assignment didn't find any
pdata, i.e. if (!pmic_plat_data) here.

Looking at patch 2/2, the structure in that driver is correct, and
perhaps could be implemented the same or similarly here?

>   of_pmic_plat_data = pmic_plat_data;

Or just swap those assignments:

of_pmic_plat_data = tps65910_parse_dt(...);
if (!pmic_plat_data)
pmic_plat_data = of_pmic_plat_data;

(although there's perhaps little point parsing the pdata from DT if it's
already provided through the device object)

>   }
>  
> - if (!pmic_plat_data)
> + if (!pmic_plat_data || chip_id < 0)
>   return -EINVAL;

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: core: make it possible to match a pure device tree driver

[Stephen Warren]

On 06/17/2013 10:33 AM, Linus Walleij wrote:
> On Mon, Jun 17, 2013 at 5:48 PM, Stephen Warren  wrote:
> 
>> This has just shown up in next-20130617, and breaks at least the
>> TPS65910 and TPS62360 drivers, since they assume that the id parameter
>> passed to probe is non-NULL. However, now the parameter is NULL since
>> these drivers have both an ID table and an OF match table.
> 
> So you mean they come in through the DT boot path and assume
> that parameter is non-null even though they should not make use of
> it?

Yes. Since this wasn't true/enforced before, it probably wasn't clear
that rule existed.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: core: make it possible to match a pure device tree driver

[Stephen Warren]

On 05/22/2013 01:56 AM, Linus Walleij wrote:
> On Mon, May 13, 2013 at 10:18 PM, Linus Walleij
>  wrote:
> 
>> This tries to address an issue found when writing an MFD driver
>> for the Nomadik STw481x PMICs: as the platform is using device
>> tree exclusively I want to specify the driver matching like
>> this:
> (...)
>> ---
>> ChangeLog v1->v2:
>> - Use of_match_device() to determine if there is a DT match in
>>   the probe code. If there is a match we pass NULL for the
>>   id_table match parameter.
> 
> Ping on this.
> 
> v2 should be doing what Grant suggested...

This has just shown up in next-20130617, and breaks at least the
TPS65910 and TPS62360 drivers, since they assume that the id parameter
passed to probe is non-NULL. However, now the parameter is NULL since
these drivers have both an ID table and an OF match table.

I'd like to suggest this patch be reverted an re-introduced immediately
after the merge window. That should give enough time for everyone to get
a heads-up on fixing any drivers with a similar problem, rather than
trying to cram all that in immediately before the merge window. I'd also
suggest that this patch should be accompanied by fixes for any similarly
broken drivers, based on code inspection.

Do people agree? If not, please let us know ASAP so we can post patches
to fix this.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/3] i2c: mux: Add i2c-arb-gpio-challenge 'mux' driver

[Stephen Warren]

On 04/16/2013 03:36 AM, Wolfram Sang wrote:
> Doug,
> 
> On Tue, Apr 09, 2013 at 02:34:28PM -0700, Doug Anderson wrote:
>> The i2c-arb-gpio-challenge driver implements an I2C arbitration scheme
>> where masters need to claim the bus with a GPIO before they can start
>> a transcation.  This should generally only be used when standard I2C
>> multimaster isn't appropriate for some reason (errata/bugs).
>>
>> This driver is based on code that Simon Glass added to the i2c-s3c2410
>> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
>> mux driver is as suggested by Grant Likely.  See
>>  for some history.

>> diff --git 
>> a/Documentation/devicetree/bindings/i2c/i2c-arb-gpio-challenge.txt 
>> b/Documentation/devicetree/bindings/i2c/i2c-arb-gpio-challenge.txt

>> +GPIO-based I2C Arbitration
>> +==
>> +This uses GPIO lines to arbitrate who is the master of an I2C bus in a
>> +multimaster situation.
> 
> "uses GPIO lines and a challange & response mechanism" or something like
> that. There might be other GPIO based arbitrations in the future (though
> I hope there won't).

The existing text appears clearer to me; this document should spell out
the exact details of the protocol in later paragraphs, so there's no
need to try and spell it out here.

>> +- their-claim-gpios: The GPIOs that the other sides use the claim the bus.
>> +  Note that some implementations may only support a single other master.
> 
> Stronger? "Currently, only one other master is supported"?

The DT binding documentation, which should be OS-/driver-agnostic,
should describe the binding, not the implementation. The limitation that
Linux imposes is OS-specific and hence should not be mentioned here as
an absolute, or perhaps even at all.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] i2c: mux: Add i2c-arbitrator-cros-ec 'mux' driver

[Stephen Warren]

On 04/05/2013 01:37 PM, Simon Glass wrote:
> HI Wolfram,
> 
> On Wed, Apr 3, 2013 at 12:19 PM, Wolfram Sang  wrote:
>> Doug,
>>
>>> Separately from a discussion of the technical merits, I'd say that
>>> this patch is needed because the Embedded Controller (EC) on the ARM
>>> Chromebook shipped expecting to communicate with this scheme.  While
>>
>> Uhrm, with all respect, "we already shipped it" is not a convincing
>> argument regarding inclusion. Benefit for the kernel is.

I'm not quite sure why that isn't a convincing argument.

The hardware has shipped. I don't know whether the EC microcode can be
updated in the field; it seems risky to do so even if it's possible. So,
it either gets supported or not; the HW/ucode isn't going to change I
suspect.

Hence, it seems that the decision would be:

a) Disallow the implementation of the arbitration scheme in the kernel,
and hence don't support this board in the kernel. (or at least some very
core features of this board)

b) Allow the implementation of the arbitration scheme in the kernel, and
hence make possible support this board in the kernel.

>From that perspective, the benefit for the kernel question comes down
to: do we see a benefit for the kernel to support this board? I can't
see why that wouldn't be a benefit.

The only disadvantage would be having to carrying code to support that
board. That same argument can be made for any board, and I think
typically doesn't cause any issue. The code for this I2C mux seems
pretty self-contained, so even if it was absolutely terrible (which I
don't think it is), it still wouldn't cause any wide-spread issues, I think.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2] i2c: tegra: assume CONFIG_OF, remove platform data

[Stephen Warren]

From: Stephen Warren 

Tegra only supports, and always enables, device tree. Remove all ifdefs
and runtime checks for DT support from the driver. Platform data is
therefore no longer required. Delete the header that defines it.

Signed-off-by: Stephen Warren 
---
V2: Rebased onto v3.9-rc1; hopefully this is far easier to apply to the
I2C tree than the previous version.

 arch/arm/mach-tegra/board-dt-tegra20.c |2 --
 drivers/i2c/busses/i2c-tegra.c |   26 +++---
 include/linux/i2c-tegra.h  |   25 -
 3 files changed, 7 insertions(+), 46 deletions(-)
 delete mode 100644 include/linux/i2c-tegra.h

diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c 
b/arch/arm/mach-tegra/board-dt-tegra20.c
index a0edf25..6e1c9a9 100644
--- a/arch/arm/mach-tegra/board-dt-tegra20.c
+++ b/arch/arm/mach-tegra/board-dt-tegra20.c
@@ -30,8 +30,6 @@
 #include 
 #include 
 #include 
-#include 
-#include 
 #include 
 
 #include 
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 36704e3..865f885 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -25,7 +25,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -172,7 +171,7 @@ struct tegra_i2c_dev {
u8 *msg_buf;
size_t msg_buf_remaining;
int msg_read;
-   unsigned long bus_clk_rate;
+   u32 bus_clk_rate;
bool is_suspended;
 };
 
@@ -685,7 +684,6 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
.clk_divisor_std_fast_mode = 0x19,
 };
 
-#if defined(CONFIG_OF)
 /* Match table for of_platform binding */
 static const struct of_device_id tegra_i2c_of_match[] = {
{ .compatible = "nvidia,tegra114-i2c", .data = &tegra114_i2c_hw, },
@@ -695,16 +693,13 @@ static const struct of_device_id tegra_i2c_of_match[] = {
{},
 };
 MODULE_DEVICE_TABLE(of, tegra_i2c_of_match);
-#endif
 
 static int tegra_i2c_probe(struct platform_device *pdev)
 {
struct tegra_i2c_dev *i2c_dev;
-   struct tegra_i2c_platform_data *pdata = pdev->dev.platform_data;
struct resource *res;
struct clk *div_clk;
struct clk *fast_clk;
-   const unsigned int *prop;
void __iomem *base;
int irq;
int ret = 0;
@@ -745,23 +740,16 @@ static int tegra_i2c_probe(struct platform_device *pdev)
i2c_dev->cont_id = pdev->id;
i2c_dev->dev = &pdev->dev;
 
-   i2c_dev->bus_clk_rate = 10; /* default clock rate */
-   if (pdata) {
-   i2c_dev->bus_clk_rate = pdata->bus_clk_rate;
-
-   } else if (i2c_dev->dev->of_node) {/* if there is a device tree 
node ... */
-   prop = of_get_property(i2c_dev->dev->of_node,
-   "clock-frequency", NULL);
-   if (prop)
-   i2c_dev->bus_clk_rate = be32_to_cpup(prop);
-   }
+   ret = of_property_read_u32(i2c_dev->dev->of_node, "clock-frequency",
+   &i2c_dev->bus_clk_rate);
+   if (ret)
+   i2c_dev->bus_clk_rate = 10; /* default clock rate */
 
i2c_dev->hw = &tegra20_i2c_hw;
 
if (pdev->dev.of_node) {
const struct of_device_id *match;
-   match = of_match_device(of_match_ptr(tegra_i2c_of_match),
-   &pdev->dev);
+   match = of_match_device(tegra_i2c_of_match, &pdev->dev);
i2c_dev->hw = match->data;
i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
"nvidia,tegra20-i2c-dvc");
@@ -867,7 +855,7 @@ static struct platform_driver tegra_i2c_driver = {
.driver  = {
.name  = "tegra-i2c",
.owner = THIS_MODULE,
-   .of_match_table = of_match_ptr(tegra_i2c_of_match),
+   .of_match_table = tegra_i2c_of_match,
.pm= TEGRA_I2C_PM,
},
 };
diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
deleted file mode 100644
index 9c85da4..000
--- a/include/linux/i2c-tegra.h
+++ /dev/null
@@ -1,25 +0,0 @@
-/*
- * drivers/i2c/busses/i2c-tegra.c
- *
- * Copyright (C) 2010 Google, Inc.
- * Author: Colin Cross 
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
-#ifndef _LINUX_I2C_TEGRA_H
-#define _LINUX_I2C_T

Re: [PATCH] i2c: tegra: check the clk_prepare_enable() return value

[Stephen Warren]

On 03/15/2013 09:34 AM, Laxman Dewangan wrote:
> NVIDIA's Tegra SoC allows read/write of controller register only
> if controller clock is enabled. System hangs if read/write happens
> to registers without enabling clock.
> 
> clk_prepare_enable() can be fail due to unknown reason and hence
> adding check for return value of this function. If this function
> success then only access register otherwise return to caller with
> error.

Wolfram,

Reviewed-by: Stephen Warren 

This is probably suitable for Cc: stable.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] i2c: mux: Add i2c-arbitrator-cros-ec 'mux' driver

[Stephen Warren]

On 03/13/2013 10:59 AM, Doug Anderson wrote:
> Stephen,
> 
> On Wed, Mar 13, 2013 at 9:53 AM, Stephen Warren  wrote:
>>> Changes in v4: None
>>
>> Isn't this 'PATCH V3 REPOST' then?
> 
> In this case part 2 in the patch series changes but not parts 1 and 3.
>  I could have just reposted part 2 with a higher version, but that
> makes it a little harder to piece together all of the parts of the
> series so I decided to repost all 3.  I can do differently in the
> future if you prefer, but my understanding was that it was a matter of
> preference/judgement call.

Oh no you're quite right. I didn't notice it was a 3-part series, since
I only got patch 1/3 filtered into my inbox; I guess I wasn't CC'd on
the rest. Sorry for the noise.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] i2c: mux: Add i2c-arbitrator-cros-ec 'mux' driver

[Stephen Warren]

On 03/13/2013 10:36 AM, Doug Anderson wrote:
> The i2c-arbitrator-cros-ec driver implements the arbitration scheme
> that the Embedded Controller (EC) on the ARM Chromebook expects to use
> for bus multimastering.  This i2c-arbitrator-cros-ec driver could also
> be used in other places where standard I2C bus arbitration can't be
> used and two extra GPIOs are available for arbitration.
> 
> This driver is based on code that Simon Glass added to the i2c-s3c2410
> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
> mux driver is as suggested by Grant Likely.  See
>  for some history.
...
> Changes in v4: None

Isn't this 'PATCH V3 REPOST' then?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH REPOST] i2c: tegra: assume CONFIG_OF, remove platform data

[Stephen Warren]

From: Stephen Warren 

Tegra only supports, and always enables, device tree. Remove all ifdefs
and runtime checks for DT support from the driver. Platform data is
therefore no longer required. Delete the header that defines it.

Signed-off-by: Stephen Warren 
---
 arch/arm/mach-tegra/tegra.c|2 --
 drivers/i2c/busses/i2c-tegra.c |   26 +++---
 include/linux/i2c-tegra.h  |   25 -
 3 files changed, 7 insertions(+), 46 deletions(-)
 delete mode 100644 include/linux/i2c-tegra.h

diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
index 27232c9..f68c9f6 100644
--- a/arch/arm/mach-tegra/tegra.c
+++ b/arch/arm/mach-tegra/tegra.c
@@ -31,8 +31,6 @@
 #include 
 #include 
 #include 
-#include 
-#include 
 #include 
 
 #include 
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 36704e3..865f885 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -25,7 +25,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -172,7 +171,7 @@ struct tegra_i2c_dev {
u8 *msg_buf;
size_t msg_buf_remaining;
int msg_read;
-   unsigned long bus_clk_rate;
+   u32 bus_clk_rate;
bool is_suspended;
 };
 
@@ -685,7 +684,6 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
.clk_divisor_std_fast_mode = 0x19,
 };
 
-#if defined(CONFIG_OF)
 /* Match table for of_platform binding */
 static const struct of_device_id tegra_i2c_of_match[] = {
{ .compatible = "nvidia,tegra114-i2c", .data = &tegra114_i2c_hw, },
@@ -695,16 +693,13 @@ static const struct of_device_id tegra_i2c_of_match[] = {
{},
 };
 MODULE_DEVICE_TABLE(of, tegra_i2c_of_match);
-#endif
 
 static int tegra_i2c_probe(struct platform_device *pdev)
 {
struct tegra_i2c_dev *i2c_dev;
-   struct tegra_i2c_platform_data *pdata = pdev->dev.platform_data;
struct resource *res;
struct clk *div_clk;
struct clk *fast_clk;
-   const unsigned int *prop;
void __iomem *base;
int irq;
int ret = 0;
@@ -745,23 +740,16 @@ static int tegra_i2c_probe(struct platform_device *pdev)
i2c_dev->cont_id = pdev->id;
i2c_dev->dev = &pdev->dev;
 
-   i2c_dev->bus_clk_rate = 10; /* default clock rate */
-   if (pdata) {
-   i2c_dev->bus_clk_rate = pdata->bus_clk_rate;
-
-   } else if (i2c_dev->dev->of_node) {/* if there is a device tree 
node ... */
-   prop = of_get_property(i2c_dev->dev->of_node,
-   "clock-frequency", NULL);
-   if (prop)
-   i2c_dev->bus_clk_rate = be32_to_cpup(prop);
-   }
+   ret = of_property_read_u32(i2c_dev->dev->of_node, "clock-frequency",
+   &i2c_dev->bus_clk_rate);
+   if (ret)
+   i2c_dev->bus_clk_rate = 10; /* default clock rate */
 
i2c_dev->hw = &tegra20_i2c_hw;
 
if (pdev->dev.of_node) {
const struct of_device_id *match;
-   match = of_match_device(of_match_ptr(tegra_i2c_of_match),
-   &pdev->dev);
+   match = of_match_device(tegra_i2c_of_match, &pdev->dev);
i2c_dev->hw = match->data;
i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
"nvidia,tegra20-i2c-dvc");
@@ -867,7 +855,7 @@ static struct platform_driver tegra_i2c_driver = {
.driver  = {
.name  = "tegra-i2c",
.owner = THIS_MODULE,
-   .of_match_table = of_match_ptr(tegra_i2c_of_match),
+   .of_match_table = tegra_i2c_of_match,
.pm= TEGRA_I2C_PM,
},
 };
diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
deleted file mode 100644
index 9c85da4..000
--- a/include/linux/i2c-tegra.h
+++ /dev/null
@@ -1,25 +0,0 @@
-/*
- * drivers/i2c/busses/i2c-tegra.c
- *
- * Copyright (C) 2010 Google, Inc.
- * Author: Colin Cross 
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
-#ifndef _LINUX_I2C_TEGRA_H
-#define _LINUX_I2C_TEGRA_H
-
-struct tegra_i2c_platform_data {
-   unsigned long bus_clk_rate;
-};
-
-#endif /* _LINUX_I2C_TEGRA_H */
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: I2C and devicetrees

[Stephen Warren]

On 03/01/2013 02:47 PM, Mitch Bradley wrote:
> On 3/1/2013 9:56 AM, Thomas De Schampheleire wrote:
>> Hi,
>>
>> On Tue, Feb 12, 2013 at 5:34 PM, Gerlando Falauto
>>  wrote:
>>> Hi everyone,
>>>
>>> I have a similar question.
>>> I'd like to "name" i2c devices so that a userspace application can somehow
>>> identify those devices with the same function across different boards (which
>>> may have different addresses or be connected to a different i2c bus, or be a
>>> physically different chip).
>>>
>>> For instance "hwmon" devices get instantiated within sysfs under
>>> /sys/class/hwmon/hwmonX
>>>
>>> # cat /sys/class/hwmon/hwmon0/device/name
>>> lm75
>>>
>>> which I would like to be identified by the name "switch" (as in "switch
>>> temperature"). I was thinking about instantiating it as something like
>>> "lm75:switch" within i2c_board_info.type. For device-tree-less
>>> architectures, a trivial change within i2c_match_id() so to ignore the part
>>> following ":" seems to do the trick. Don't know about devicetree but I guess
>>> a similar approach could be imagined.
>>>
>>> Another example would be given by EEPROMs: all boards are equipped with an
>>> EEPROM containing inventory management, which I would like to identify as
>>> "ivm". So something like "24c08:ivm".
>>> After all, I'd like to be able to achive something like "named MTD
>>> partitions" which you can identify by looking at "/proc/mtd".
>>>
>>> Maybe some other symbol could be used instead of ":", but anyhow, does the
>>> above make any sense at all to you?
>>>
>>
>> I have exactly the same request: I would like to put logical names in
>> the device tree for various devices (i2c, spi, ...) which are in some
>> way easily retrievable from a userspace application.
>> The purpose seems to be the same as Gerlando's: different boards have
>> different physical configuration but logically each has the same set
>> of devices.
>>
>> How can one achieve that?
> 
> Unless I am misunderstanding your request, that is what the /aliases
> node is intended for.  Each property name in /aliases is a logical name,
> and the value refers to the corresponding device node.
> 
> I'm not sure about all the different ways that Linux exports information
> in /aliases to userspace.  I do know that, in the case of some i2c,
> serial, and ethernet devices, aliases like:
> 
>   serial1 = &uart1;
> 
> cause the assignment of small-integer unit numbers to specific device
> instances.  That "serial" pattern is somewhat of a Linux-specific
> special case.  In general, the Open Firmware standard just says that
> /aliases maps logical names to longer strings  representing fuller
> pathnames, without imposing any structure (e.g. serial) on the
> logical names.

I'm not sure if aliases solve all scenarios.

If you have a DT node that's a single UART, then aliases work OK, as in
your example above.

However, what if you have a single thermal sensor ID that has 4
channels. There will be a single DT node that represents this device.
However, the 4 channels could be hooked up arbitrarily on a board, and
you really want to name the individual channels, not just the IC that
contains those channels. Can you do that; will the following work?

/aliases {
cpu-temp = <&i2cdev 1>; # 1 is the channel ID on the chip
ambient-temp = <&i2cdev 3>;
};

All the code I recall so far that handles aliases searches for alias
entries with a specific prefix for the name (e.g. "i2c*", and then finds
"i2c0", "i2c1", etc.). In order to support the syntax above, you'd have
to instead search for all aliases that include the phandle of the node
in question. I guess it's easy enough to implement that, but it's quite
a different way of thinking about aliases, I think.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] i2c: tegra: assume CONFIG_OF, remove platform data

[Stephen Warren]

From: Stephen Warren 

Tegra only supports, and always enables, device tree. Remove all ifdefs
and runtime checks for DT support from the driver. Platform data is
therefore no longer required. Delete the header that defines it.

Signed-off-by: Stephen Warren 
---
 arch/arm/mach-tegra/board-dt-tegra20.c |2 --
 drivers/i2c/busses/i2c-tegra.c |   26 +++---
 include/linux/i2c-tegra.h  |   25 -
 3 files changed, 7 insertions(+), 46 deletions(-)
 delete mode 100644 include/linux/i2c-tegra.h

diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c 
b/arch/arm/mach-tegra/board-dt-tegra20.c
index a0edf25..6e1c9a9 100644
--- a/arch/arm/mach-tegra/board-dt-tegra20.c
+++ b/arch/arm/mach-tegra/board-dt-tegra20.c
@@ -30,8 +30,6 @@
 #include 
 #include 
 #include 
-#include 
-#include 
 #include 
 
 #include 
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index ae2e027..587ba7a 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -25,7 +25,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -172,7 +171,7 @@ struct tegra_i2c_dev {
u8 *msg_buf;
size_t msg_buf_remaining;
int msg_read;
-   unsigned long bus_clk_rate;
+   u32 bus_clk_rate;
bool is_suspended;
 };
 
@@ -685,7 +684,6 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
.clk_divisor_std_fast_mode = 0x19,
 };
 
-#if defined(CONFIG_OF)
 /* Match table for of_platform binding */
 static const struct of_device_id tegra_i2c_of_match[] = {
{ .compatible = "nvidia,tegra114-i2c", .data = &tegra114_i2c_hw, },
@@ -695,16 +693,13 @@ static const struct of_device_id tegra_i2c_of_match[] = {
{},
 };
 MODULE_DEVICE_TABLE(of, tegra_i2c_of_match);
-#endif
 
 static int tegra_i2c_probe(struct platform_device *pdev)
 {
struct tegra_i2c_dev *i2c_dev;
-   struct tegra_i2c_platform_data *pdata = pdev->dev.platform_data;
struct resource *res;
struct clk *div_clk;
struct clk *fast_clk;
-   const unsigned int *prop;
void __iomem *base;
int irq;
int ret = 0;
@@ -745,23 +740,16 @@ static int tegra_i2c_probe(struct platform_device *pdev)
i2c_dev->cont_id = pdev->id;
i2c_dev->dev = &pdev->dev;
 
-   i2c_dev->bus_clk_rate = 10; /* default clock rate */
-   if (pdata) {
-   i2c_dev->bus_clk_rate = pdata->bus_clk_rate;
-
-   } else if (i2c_dev->dev->of_node) {/* if there is a device tree 
node ... */
-   prop = of_get_property(i2c_dev->dev->of_node,
-   "clock-frequency", NULL);
-   if (prop)
-   i2c_dev->bus_clk_rate = be32_to_cpup(prop);
-   }
+   ret = of_property_read_u32(i2c_dev->dev->of_node, "clock-frequency",
+   &i2c_dev->bus_clk_rate);
+   if (ret)
+   i2c_dev->bus_clk_rate = 10; /* default clock rate */
 
i2c_dev->hw = &tegra20_i2c_hw;
 
if (pdev->dev.of_node) {
const struct of_device_id *match;
-   match = of_match_device(of_match_ptr(tegra_i2c_of_match),
-   &pdev->dev);
+   match = of_match_device(tegra_i2c_of_match, &pdev->dev);
i2c_dev->hw = match->data;
i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
"nvidia,tegra20-i2c-dvc");
@@ -867,7 +855,7 @@ static struct platform_driver tegra_i2c_driver = {
.driver  = {
.name  = "tegra-i2c",
.owner = THIS_MODULE,
-   .of_match_table = of_match_ptr(tegra_i2c_of_match),
+   .of_match_table = tegra_i2c_of_match,
.pm= TEGRA_I2C_PM,
},
 };
diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
deleted file mode 100644
index 9c85da4..000
--- a/include/linux/i2c-tegra.h
+++ /dev/null
@@ -1,25 +0,0 @@
-/*
- * drivers/i2c/busses/i2c-tegra.c
- *
- * Copyright (C) 2010 Google, Inc.
- * Author: Colin Cross 
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
-#ifndef _LINUX_I2C_TEGRA_H
-#define _LINUX_I2C_TEGRA_H
-
-struct tegra_i2c_platform_data {
-   unsigned long bus_clk_rate;
-};
-
-#endif /* _LINUX_I2C_TEGRA_

Re: [PATCH v3 1/3] i2c: mux: Add i2c-arbitrator-cros-ec 'mux' driver

[Stephen Warren]

On 02/15/2013 12:46 PM, Doug Anderson wrote:
> The i2c-arbitrator-cros-ec driver implements the arbitration scheme
> that the Embedded Controller (EC) on the ARM Chromebook expects to use
> for bus multimastering.  This i2c-arbitrator-cros-ec driver could also
> be used in other places where standard I2C bus arbitration can't be
> used and two extra GPIOs are available for arbitration.
> 
> This driver is based on code that Simon Glass added to the i2c-s3c2410
> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
> mux driver is as suggested by Grant Likely.  See
> <https://patchwork.kernel.org/patch/1877311/> for some history.

Reviewed-by: Stephen Warren 

> diff --git a/drivers/i2c/muxes/i2c-arbitrator-cros-ec.c 
> b/drivers/i2c/muxes/i2c-arbitrator-cros-ec.c

> +static int i2c_arbitrator_probe(struct platform_device *pdev)

> + /* Request GPIOs */
> + ret = of_get_named_gpio_flags(np, "ap-claim-gpio", 0, &gpio_flags);
> + if (ret == -EPROBE_DEFER || WARN_ON(!gpio_is_valid(ret)))
> + return ret;

I think by the time that doesn't return -EPROBE_DEFER ...

> + arb->ap_gpio = ret;
> + arb->ap_gpio_release = !!(gpio_flags & OF_GPIO_ACTIVE_LOW);
> + out_init = (gpio_flags & OF_GPIO_ACTIVE_LOW) ?
> + GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
> + ret = devm_gpio_request_one(&pdev->dev, arb->ap_gpio, out_init,
> + "ap-claim-gpio");
> + if (ret == -EPROBE_DEFER || WARN_ON(ret))
> + return ret;

... that won't either, since of_get_named_gpio_flags()'s implementation
requires the relevant GPIO driver to be active already.

Still, there's no harm in the code as it stands, so no need to change.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: tegra: remove warning dump if timeout happen in transfer

[Stephen Warren]

On 02/15/2013 12:18 PM, Wolfram Sang wrote:
> On Thu, Feb 14, 2013 at 06:13:33PM +0530, Laxman Dewangan wrote:
>> If timeout error occurs in the i2c transfer then it was dumping warning
>> of call stack.
>>
>> Remove the warning dump as there is may be possibility that some slave
>> devices are busy and not responding the i2c communication.
>>
>> Signed-off-by: Laxman Dewangan 
> 
> I'd like to have the patch extended. Currently, i2c-tegra is the only
> I2C user of BUG and BUG_ON. Could you maybe extend the patch to handle
> those situations more gracefully while we are at it? From a glimpse,
> these situations don't need a complete halt of the kernel?

While that's probably useful, surely that's a separate patch?

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] i2c: mux: Add i2c-arbitrator-cros-ec 'mux' driver

[Stephen Warren]

On 02/14/2013 05:21 PM, Doug Anderson wrote:
> The i2c-arbitrator-cros-ec driver implements the arbitration scheme
> that the Embedded Controller (EC) on the ARM Chromebook expects to use
> for bus multimastering.  This i2c-arbitrator-cros-ec driver could also
> be used in other places where standard I2C bus arbitration can't be
> used and two extra GPIOs are available for arbitration.
> 
> This driver is based on code that Simon Glass added to the i2c-s3c2410
> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
> mux driver is as suggested by Grant Likely.  See
>  for some history.

> diff --git a/drivers/i2c/muxes/i2c-arbitrator-cros-ec.c 
> b/drivers/i2c/muxes/i2c-arbitrator-cros-ec.c

> +static int i2c_arbitrator_probe(struct platform_device *pdev)

> + arb->parent = of_find_i2c_adapter_by_node(parent_np);
> + if (WARN_ON(!arb->parent))
> + return -EINVAL;

I think for all error paths after this point, a call to
i2c_put_adapter() is needed.

> + /* Request GPIOs */
> + ret = of_get_named_gpio_flags(np, "ap-claim-gpio", 0, &gpio_flags);
> + if (WARN_ON(!gpio_is_valid(ret)))
> + return ret;

You shouldn't warn in all cases; -EPROBE_DEFER can quite legitimately
happen and only temporarily prevents probe() - it's retried later.

> + arb->ap_gpio = ret;

Nit: Perhaps simply assign arb->ap_gpio directly from the
of_get_named_gpio_flags call? It would make that call slightly clearer.

> + arb->ap_gpio_release = !!(gpio_flags & OF_GPIO_ACTIVE_LOW);
> + out_init = (gpio_flags & OF_GPIO_ACTIVE_LOW) ?
> + GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;

Nit: I'd be tempted to test arb->ap_gpio_release here, rather than
extracting the value from the flags again. Semantically equivalent, but
it seems to slightly more directly show why it's being tested.

> +subsys_initcall(i2c_arbitrator_init);

You mentioned that you only saw problems using
module_init/module_platform_driver in your downstream tree, so the
problem doesn't affect upstream. Presumably those problems would be
fixed when upstreaming any other drivers into the mainline kernel. I'd
still be tempted to just use module_platform_driver here. But, I guess
I'm fine with the patch either way; I'll leave the call to Wolfram.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 4/4] i2c-mux: i2c_add_mux_adapter() should use -1 for auto bus num

[Stephen Warren]

On 02/13/2013 11:02 AM, Doug Anderson wrote:
> The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
> we don't want to force the bus number of the adapter.  This is
> non-ideal because:
> * 0 is actually a valid bus number to request
> * i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
>   -1 to mean the same thing.  That means extra logic in
>   i2c_add_mux_adapter().
> 
> Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
> accordingly.
> 
> Signed-off-by: Doug Anderson 
> ---
> Notes:
> - If there's a good reason that force_nr uses 0 for auto then feel
>   free to drop this patch.  I've place it at the end of the series to
>   make it easy to just drop it.

IIRC (and I only vaguely do...) it's because:

> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c 
> b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 9f50ef0..301ed0b 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device 
> *pdev)
>   }
>  
>   for (i = 0; i < mux->data.n_values; i++) {
> - u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
> + int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;

Here, mux->data.base_nr is platform data (or copied directly from it),
and any field in a platform data struct stored in a global variable not
explicitly initialized would be 0, hence 0 would typically mean "no
explicit bus number desired". Since a mux can't exist without a parent
I2C bus, it's unlikely anyone would want a mux to be I2C bus 0, but
rather the parent to have that number.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Stephen Warren]

On 02/13/2013 11:02 AM, Doug Anderson wrote:
> The i2c-arbitrator driver implements the arbitration scheme that the
> Embedded Controller (EC) on the ARM Chromebook expects to use for bus
> multimastering.  This i2c-arbitrator driver could also be used in
> other places where standard i2c bus arbitration can't be used and two
> extra GPIOs are available for arbitration.
> 
> This driver is based on code that Simon Glass added to the i2c-s3c2410
> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
> mux driver is as suggested by Grant Likely.  See
>  for some history.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt 
> b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt

> +This mechanism is used instead of standard i2c multimaster to avoid some of 
> the
> +subtle driver and silicon bugs that are often present with i2c multimaster.

Being really pick here, but I2C should be capitalized in free-form text.
There are a few other places where the comment applies.

> +Required properties:
> +- compatible: i2c-arbitrator

That seems pretty generic. What if there are other arbitration schemes?

> +- bus-arbitration-gpios: Two GPIOs to use with the GPIO-based bus
> +arbitration protocol.  The first should be an output, and is used to
> +claim the I2C bus for us (AP_CLAIM).  The second should be an input and
> +signals that the other side wants to claim the bus (EC_CLAIM).

Is it worth using two separate properties here, so they each get a
unique name. That way, nobody has the remember which order the two GPIOs
come in.

> diff --git a/drivers/i2c/muxes/i2c-arbitrator.c 
> b/drivers/i2c/muxes/i2c-arbitrator.c

> +enum {
> + I2C_ARB_GPIO_AP,/* AP claims i2c bus */
> + I2C_ARB_GPIO_EC,/* EC claims i2c bus */
> +
> + I2C_ARB_GPIO_COUNT,
> +};

Oh, I see from later code that those are indices into the
bus-arbitration-gpios DT property. I thought they were states in some
state machine at first. A comment might help here, if you continue to
use one property.

> +static int i2c_arbitrator_select(struct i2c_adapter *adap, void *data, u32 
> chan)
> +{
> + const struct i2c_arbitrator_data *arb = data;
> + unsigned long stop_retry, stop_time;
> +
> + /* Start a round of trying to claim the bus */
> + stop_time = jiffies + usecs_to_jiffies(arb->wait_free_us) + 1;
> + do {
> + /* Indicate that we want to claim the bus */
> + gpio_set_value(arb->ap_gpio, 0);

The GPIO signals appear to be active low in the code. Instead, I think
it'd make more sense to extract the polarity of the GPIO from DT, using
of_get_named_gpio_flags().

> +static int i2c_arbitrator_probe(struct platform_device *pdev)

> + /* Request GPIOs */
> + ret = of_get_named_gpio(np, "bus-arbitration-gpios", I2C_ARB_GPIO_AP);
> + if (gpio_is_valid(ret)) {
> + arb->ap_gpio = ret;
> + ret = gpio_request_one(arb->ap_gpio, GPIOF_OUT_INIT_HIGH,
> +"bus-arbitration-ap");
> + }
> + if (WARN_ON(ret))
> + goto ap_request_failed;

you could use devm_gpio_request_one() and save some cleanup logic.

> + /* Arbitration parameters */
> + if (of_property_read_u32(np, "bus-arbitration-slew-delay-us",
> +  &arb->slew_delay_us))
> + arb->slew_delay_us = 10;

The DT binding document says that property is required. Either the code
should error out here, or the document updated to indicate that the
properties are optional, and specify what the defaults are.

> +static int i2c_arbitrator_remove(struct platform_device *pdev)

> + platform_set_drvdata(pdev, NULL);

You shouldn't have to do that; nothing should care what the pdata value
is while the device isn't probed anyway.

> +static int __init i2c_arbitrator_init(void)
> +{
> + return platform_driver_register(&i2c_arbitrator_driver);
> +}
> +subsys_initcall(i2c_arbitrator_init);
> +
> +static void __exit i2c_arbitrator_exit(void)
> +{
> + platform_driver_unregister(&i2c_arbitrator_driver);
> +}
> +module_exit(i2c_arbitrator_exit);

You should be able to replace all that with:

module_platform_driver(&i2c_arbitrator_driver);

> +MODULE_LICENSE("GPL");

The header says GPL v2 only, so "GPL v2".
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V4] i2c: add bcm2835 driver

[Stephen Warren]

This implements a very basic I2C host driver for the BCM2835 SoC. Missing
features so far are:

* 10-bit addressing.
* DMA.

Reviewed-by: Grant Likely 
Signed-off-by: Stephen Warren 
---
v4:
* Convert FIFO fill/drain loops from for to while.
* Don't WARN_ON transfer timeout.
* Use regular request_irq not devm_request_irq.
* Use i2c_add_adapter not i2c_add_numbered_adapter.
v3:
* Return i2c_add_numbered_adapter() from probe() directly.
v2:
* Implemented clock divider configuration based on desired bus rate.
* Make use of module_platform_driver().
* Removed use of devinit.
---
 .../devicetree/bindings/i2c/brcm,bcm2835-i2c.txt   |   20 ++
 drivers/i2c/busses/Kconfig |   12 +
 drivers/i2c/busses/Makefile|1 +
 drivers/i2c/busses/i2c-bcm2835.c   |  342 
 4 files changed, 375 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
 create mode 100644 drivers/i2c/busses/i2c-bcm2835.c

diff --git a/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt 
b/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
new file mode 100644
index 000..e9de375
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
@@ -0,0 +1,20 @@
+Broadcom BCM2835 I2C controller
+
+Required properties:
+- compatible : Should be "brcm,bcm2835-i2c".
+- reg: Should contain register location and length.
+- interrupts: Should contain interrupt.
+- clocks : The clock feeding the I2C controller.
+
+Recommended properties:
+- clock-frequency : desired I2C bus clock frequency in Hz.
+
+Example:
+
+i2c@20205000 {
+   compatible = "brcm,bcm2835-i2c";
+   reg = <0x7e205000 0x1000>;
+   interrupts = <2 21>;
+   clocks = <&clk_i2c>;
+   clock-frequency = <10>;
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8bb810e..3cc40ab 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -319,6 +319,18 @@ config I2C_AU1550
  This driver can also be built as a module.  If so, the module
  will be called i2c-au1550.
 
+config I2C_BCM2835
+   tristate "Broadcom BCM2835 I2C controller"
+   depends on ARCH_BCM2835
+   help
+ If you say yes to this option, support will be included for the
+ BCM2835 I2C controller.
+
+ If you don't know what to do here, say N.
+
+ This support is also available as a module.  If so, the module
+ will be called i2c-bcm2835.
+
 config I2C_BLACKFIN_TWI
tristate "Blackfin TWI I2C support"
depends on BLACKFIN
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 6181f3f..a52a891 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_I2C_POWERMAC)+= i2c-powermac.o
 # Embedded system I2C/SMBus host controller drivers
 obj-$(CONFIG_I2C_AT91) += i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)   += i2c-au1550.o
+obj-$(CONFIG_I2C_BCM2835)  += i2c-bcm2835.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
 obj-$(CONFIG_I2C_CBUS_GPIO)+= i2c-cbus-gpio.o
 obj-$(CONFIG_I2C_CPM)  += i2c-cpm.o
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
new file mode 100644
index 000..ea4b08f
--- /dev/null
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -0,0 +1,342 @@
+/*
+ * BCM2835 master mode driver
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define BCM2835_I2C_C  0x0
+#define BCM2835_I2C_S  0x4
+#define BCM2835_I2C_DLEN   0x8
+#define BCM2835_I2C_A  0xc
+#define BCM2835_I2C_FIFO   0x10
+#define BCM2835_I2C_DIV0x14
+#define BCM2835_I2C_DEL0x18
+#define BCM2835_I2C_CLKT   0x1c
+
+#define BCM2835_I2C_C_READ BIT(0)
+#define BCM2835_I2C_C_CLEARBIT(4) /* bits 4 and 5 both clear */
+#define BCM2835_I2C_C_ST   BIT(7)
+#define BCM2835_I2C_C_INTD BIT(8)
+#define BCM2835_I2C_C_INTT BIT(9)
+#define BCM2835_I2C_C_INTR BIT(10)
+#define BCM2835_I2C_C_I2CENBIT(15)
+
+#define BCM2835_I2C_S_TA   BIT(0)
+#define BCM2835_I2C_S_DONE BIT(1)
+#define BCM2835_I2C_S_TXW  BIT(2)
+#define BCM2835_I2C_S_RXR  BIT(3)
+#define BCM2835_I2C_S_TXD  BIT(4)
+#define BCM2835_I2C_S_RXD  BIT(5)
+#define BCM2835_I2C_S_TXE  BIT(6)
+#define

Re: [PATCH V3] i2c: add bcm2835 driver

[Stephen Warren]

On 02/11/2013 03:52 PM, Wolfram Sang wrote:
> Hi Stephen,
> 
> On Fri, Feb 08, 2013 at 08:52:58PM -0700, Stephen Warren wrote:
>> This implements a very basic I2C host driver for the BCM2835 SoC. Missing
>> features so far are:
>>
>> * 10-bit addressing.
>> * DMA.
...
>> +ret = wait_for_completion_timeout(&i2c_dev->completion,
>> +  BCM2835_I2C_TIMEOUT);
>> +bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
>> +if (WARN_ON(ret == 0)) {
>> +dev_err(i2c_dev->dev, "i2c transfer timed out\n");
>> +return -ETIMEDOUT;
>> +}
> 
> I'd suggest to skip the WARN_ON. Timeout is the expected case when you
> want to read from an EEPROM which is just in the process of
> erasing/writing data from the previous command.

I copied that from Tegra. Should that driver be changed too?

>> +adap = &i2c_dev->adapter;
>> +i2c_set_adapdata(adap, i2c_dev);
>> +adap->owner = THIS_MODULE;
>> +adap->class = I2C_CLASS_HWMON;
> 
> Do you really need the class?

If I grep for I2C_CLASS_HWMON, I see a ton of hits. I think I'll keep it
unless you strongly object, since it seems typical.

I'll fix up the other points you mentioned.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V3] i2c: add bcm2835 driver

[Stephen Warren]

This implements a very basic I2C host driver for the BCM2835 SoC. Missing
features so far are:

* 10-bit addressing.
* DMA.

Reviewed-by: Grant Likely 
Signed-off-by: Stephen Warren 
---
v3:
* Return i2c_add_numbered_adapter() from probe() directly.
v2:
* Implemented clock divider configuration based on desired bus rate.
* Make use of module_platform_driver().
* Removed use of devinit.
---
 .../devicetree/bindings/i2c/brcm,bcm2835-i2c.txt   |   20 ++
 drivers/i2c/busses/Kconfig |   12 +
 drivers/i2c/busses/Makefile|1 +
 drivers/i2c/busses/i2c-bcm2835.c   |  340 
 4 files changed, 373 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
 create mode 100644 drivers/i2c/busses/i2c-bcm2835.c

diff --git a/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt 
b/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
new file mode 100644
index 000..e9de375
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
@@ -0,0 +1,20 @@
+Broadcom BCM2835 I2C controller
+
+Required properties:
+- compatible : Should be "brcm,bcm2835-i2c".
+- reg: Should contain register location and length.
+- interrupts: Should contain interrupt.
+- clocks : The clock feeding the I2C controller.
+
+Recommended properties:
+- clock-frequency : desired I2C bus clock frequency in Hz.
+
+Example:
+
+i2c@20205000 {
+   compatible = "brcm,bcm2835-i2c";
+   reg = <0x7e205000 0x1000>;
+   interrupts = <2 21>;
+   clocks = <&clk_i2c>;
+   clock-frequency = <10>;
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8bb810e..3cc40ab 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -319,6 +319,18 @@ config I2C_AU1550
  This driver can also be built as a module.  If so, the module
  will be called i2c-au1550.
 
+config I2C_BCM2835
+   tristate "Broadcom BCM2835 I2C controller"
+   depends on ARCH_BCM2835
+   help
+ If you say yes to this option, support will be included for the
+ BCM2835 I2C controller.
+
+ If you don't know what to do here, say N.
+
+ This support is also available as a module.  If so, the module
+ will be called i2c-bcm2835.
+
 config I2C_BLACKFIN_TWI
tristate "Blackfin TWI I2C support"
depends on BLACKFIN
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 6181f3f..a52a891 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_I2C_POWERMAC)+= i2c-powermac.o
 # Embedded system I2C/SMBus host controller drivers
 obj-$(CONFIG_I2C_AT91) += i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)   += i2c-au1550.o
+obj-$(CONFIG_I2C_BCM2835)  += i2c-bcm2835.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
 obj-$(CONFIG_I2C_CBUS_GPIO)+= i2c-cbus-gpio.o
 obj-$(CONFIG_I2C_CPM)  += i2c-cpm.o
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
new file mode 100644
index 000..1a399c3
--- /dev/null
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -0,0 +1,340 @@
+/*
+ * BCM2835 master mode driver
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define BCM2835_I2C_C  0x0
+#define BCM2835_I2C_S  0x4
+#define BCM2835_I2C_DLEN   0x8
+#define BCM2835_I2C_A  0xc
+#define BCM2835_I2C_FIFO   0x10
+#define BCM2835_I2C_DIV0x14
+#define BCM2835_I2C_DEL0x18
+#define BCM2835_I2C_CLKT   0x1c
+
+#define BCM2835_I2C_C_READ BIT(0)
+#define BCM2835_I2C_C_CLEARBIT(4) /* bits 4 and 5 both clear */
+#define BCM2835_I2C_C_ST   BIT(7)
+#define BCM2835_I2C_C_INTD BIT(8)
+#define BCM2835_I2C_C_INTT BIT(9)
+#define BCM2835_I2C_C_INTR BIT(10)
+#define BCM2835_I2C_C_I2CENBIT(15)
+
+#define BCM2835_I2C_S_TA   BIT(0)
+#define BCM2835_I2C_S_DONE BIT(1)
+#define BCM2835_I2C_S_TXW  BIT(2)
+#define BCM2835_I2C_S_RXR  BIT(3)
+#define BCM2835_I2C_S_TXD  BIT(4)
+#define BCM2835_I2C_S_RXD  BIT(5)
+#define BCM2835_I2C_S_TXE  BIT(6)
+#define BCM2835_I2C_S_RXF  BIT(7)
+#define BCM2835_I2C_S_ERR  BIT(8)
+#define BCM2835_I2C_S_CLKT BIT(9)
+#define BCM2835_I2C_S_LEN  BIT(10) /* Fake bit for SW error reporting */
+
+#defi

[PATCH V2 REPOST] i2c: add bcm2835 driver

[Stephen Warren]

This implements a very basic I2C host driver for the BCM2835 SoC. Missing
features so far are:

* 10-bit addressing.
* DMA.

Signed-off-by: Stephen Warren 
---
v2:
* Implemented clock divider configuration based on desired bus rate.
* Make use of module_platform_driver().
* Removed use of devinit.
---
 .../devicetree/bindings/i2c/brcm,bcm2835-i2c.txt   |   20 ++
 drivers/i2c/busses/Kconfig |   12 +
 drivers/i2c/busses/Makefile|1 +
 drivers/i2c/busses/i2c-bcm2835.c   |  346 
 4 files changed, 379 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
 create mode 100644 drivers/i2c/busses/i2c-bcm2835.c

diff --git a/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt 
b/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
new file mode 100644
index 000..e9de375
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
@@ -0,0 +1,20 @@
+Broadcom BCM2835 I2C controller
+
+Required properties:
+- compatible : Should be "brcm,bcm2835-i2c".
+- reg: Should contain register location and length.
+- interrupts: Should contain interrupt.
+- clocks : The clock feeding the I2C controller.
+
+Recommended properties:
+- clock-frequency : desired I2C bus clock frequency in Hz.
+
+Example:
+
+i2c@20205000 {
+   compatible = "brcm,bcm2835-i2c";
+   reg = <0x7e205000 0x1000>;
+   interrupts = <2 21>;
+   clocks = <&clk_i2c>;
+   clock-frequency = <10>;
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8bb810e..3cc40ab 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -319,6 +319,18 @@ config I2C_AU1550
  This driver can also be built as a module.  If so, the module
  will be called i2c-au1550.
 
+config I2C_BCM2835
+   tristate "Broadcom BCM2835 I2C controller"
+   depends on ARCH_BCM2835
+   help
+ If you say yes to this option, support will be included for the
+ BCM2835 I2C controller.
+
+ If you don't know what to do here, say N.
+
+ This support is also available as a module.  If so, the module
+ will be called i2c-bcm2835.
+
 config I2C_BLACKFIN_TWI
tristate "Blackfin TWI I2C support"
depends on BLACKFIN
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 6181f3f..a52a891 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_I2C_POWERMAC)+= i2c-powermac.o
 # Embedded system I2C/SMBus host controller drivers
 obj-$(CONFIG_I2C_AT91) += i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)   += i2c-au1550.o
+obj-$(CONFIG_I2C_BCM2835)  += i2c-bcm2835.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
 obj-$(CONFIG_I2C_CBUS_GPIO)+= i2c-cbus-gpio.o
 obj-$(CONFIG_I2C_CPM)  += i2c-cpm.o
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
new file mode 100644
index 000..22a29de
--- /dev/null
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -0,0 +1,346 @@
+/*
+ * BCM2835 master mode driver
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define BCM2835_I2C_C  0x0
+#define BCM2835_I2C_S  0x4
+#define BCM2835_I2C_DLEN   0x8
+#define BCM2835_I2C_A  0xc
+#define BCM2835_I2C_FIFO   0x10
+#define BCM2835_I2C_DIV0x14
+#define BCM2835_I2C_DEL0x18
+#define BCM2835_I2C_CLKT   0x1c
+
+#define BCM2835_I2C_C_READ BIT(0)
+#define BCM2835_I2C_C_CLEARBIT(4) /* bits 4 and 5 both clear */
+#define BCM2835_I2C_C_ST   BIT(7)
+#define BCM2835_I2C_C_INTD BIT(8)
+#define BCM2835_I2C_C_INTT BIT(9)
+#define BCM2835_I2C_C_INTR BIT(10)
+#define BCM2835_I2C_C_I2CENBIT(15)
+
+#define BCM2835_I2C_S_TA   BIT(0)
+#define BCM2835_I2C_S_DONE BIT(1)
+#define BCM2835_I2C_S_TXW  BIT(2)
+#define BCM2835_I2C_S_RXR  BIT(3)
+#define BCM2835_I2C_S_TXD  BIT(4)
+#define BCM2835_I2C_S_RXD  BIT(5)
+#define BCM2835_I2C_S_TXE  BIT(6)
+#define BCM2835_I2C_S_RXF  BIT(7)
+#define BCM2835_I2C_S_ERR  BIT(8)
+#define BCM2835_I2C_S_CLKT BIT(9)
+#define BCM2835_I2C_S_LEN  BIT(10) /* Fake bit for SW error reporting */
+
+#define BCM2835_I2C_TIMEOUT (msecs_to_jiffies(1000))
+
+struct bcm2835_i2c_dev {
+ 

Re: [PATCH] i2c: tegra: add support for Tegra114 SoC

[Stephen Warren]

On 01/05/2013 05:04 AM, Laxman Dewangan wrote:
> NVIDIA's Tegra114 has following enhanced feature in i2c controller:
> - Enable/disable control for per packet transfer complete interrupt.
>   Earlier SoCs could not disable this.
> - Single clock source for standard/fast and HS mode clock speed.
>   The clock divisor for fast/standard mode is added into the i2c
>   controller to meet the HS and standard/fast mode of clock speed
>   from single source.
> 
> Add support for the above feature to make it functional on T114 SOCs.

Reviewed-by: Stephen Warren 

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] i2c: add bcm2835 driver

[Stephen Warren]

This implements a very basic I2C host driver for the BCM2835 SoC. Missing
features so far are:

* Clock rate adjustment.
* 10-bit addressing.
* DMA.

Signed-off-by: Stephen Warren 
---
 .../devicetree/bindings/i2c/brcm,bcm2835-i2c.txt   |   20 ++
 drivers/i2c/busses/Kconfig |   12 +
 drivers/i2c/busses/Makefile|1 +
 drivers/i2c/busses/i2c-bcm2835.c   |  331 
 4 files changed, 364 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
 create mode 100644 drivers/i2c/busses/i2c-bcm2835.c

diff --git a/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt 
b/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
new file mode 100644
index 000..e9de375
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
@@ -0,0 +1,20 @@
+Broadcom BCM2835 I2C controller
+
+Required properties:
+- compatible : Should be "brcm,bcm2835-i2c".
+- reg: Should contain register location and length.
+- interrupts: Should contain interrupt.
+- clocks : The clock feeding the I2C controller.
+
+Recommended properties:
+- clock-frequency : desired I2C bus clock frequency in Hz.
+
+Example:
+
+i2c@20205000 {
+   compatible = "brcm,bcm2835-i2c";
+   reg = <0x7e205000 0x1000>;
+   interrupts = <2 21>;
+   clocks = <&clk_i2c>;
+   clock-frequency = <10>;
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index bdca511..5aab774 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -319,6 +319,18 @@ config I2C_AU1550
  This driver can also be built as a module.  If so, the module
  will be called i2c-au1550.
 
+config I2C_BCM2835
+   tristate "Broadcom BCM2835 I2C controller"
+   depends on ARCH_BCM2835
+   help
+ If you say yes to this option, support will be included for the
+ BCM2835 I2C controller.
+
+ If you don't know what to do here, say N.
+
+ This support is also available as a module.  If so, the module
+ will be called i2c-bcm2835.
+
 config I2C_BLACKFIN_TWI
tristate "Blackfin TWI I2C support"
depends on BLACKFIN
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 6181f3f..a52a891 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_I2C_POWERMAC)+= i2c-powermac.o
 # Embedded system I2C/SMBus host controller drivers
 obj-$(CONFIG_I2C_AT91) += i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)   += i2c-au1550.o
+obj-$(CONFIG_I2C_BCM2835)  += i2c-bcm2835.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
 obj-$(CONFIG_I2C_CBUS_GPIO)+= i2c-cbus-gpio.o
 obj-$(CONFIG_I2C_CPM)  += i2c-cpm.o
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
new file mode 100644
index 000..c212471
--- /dev/null
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -0,0 +1,331 @@
+/*
+ * BCM2835 master mode driver
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define BCM2835_I2C_C  0x0
+#define BCM2835_I2C_S  0x4
+#define BCM2835_I2C_DLEN   0x8
+#define BCM2835_I2C_A  0xc
+#define BCM2835_I2C_FIFO   0x10
+#define BCM2835_I2C_DIV0x14
+#define BCM2835_I2C_DEL0x18
+#define BCM2835_I2C_CLKT   0x1c
+
+#define BCM2835_I2C_C_READ BIT(0)
+#define BCM2835_I2C_C_CLEARBIT(4) /* bits 4 and 5 both clear */
+#define BCM2835_I2C_C_ST   BIT(7)
+#define BCM2835_I2C_C_INTD BIT(8)
+#define BCM2835_I2C_C_INTT BIT(9)
+#define BCM2835_I2C_C_INTR BIT(10)
+#define BCM2835_I2C_C_I2CENBIT(15)
+
+#define BCM2835_I2C_S_TA   BIT(0)
+#define BCM2835_I2C_S_DONE BIT(1)
+#define BCM2835_I2C_S_TXW  BIT(2)
+#define BCM2835_I2C_S_RXR  BIT(3)
+#define BCM2835_I2C_S_TXD  BIT(4)
+#define BCM2835_I2C_S_RXD  BIT(5)
+#define BCM2835_I2C_S_TXE  BIT(6)
+#define BCM2835_I2C_S_RXF  BIT(7)
+#define BCM2835_I2C_S_ERR  BIT(8)
+#define BCM2835_I2C_S_CLKT BIT(9)
+#define BCM2835_I2C_S_LEN  BIT(10) /* Fake bit for SW error reporting */
+
+#define BCM2835_I2C_TIMEOUT (msecs_to_jiffies(1000))
+
+struct bcm2835_i2c_dev {
+   struct device *dev;
+   void __iomem *regs;
+   struct i2c_adapter adapter;
+   struct completion completion;
+

Re: [PATCH 1/2] i2c-core: Add gpio based bus arbitration implementation

[Stephen Warren]

On 12/13/2012 10:50 PM, Naveen Krishna Chatradhi wrote:
> The arbitrator is a general purpose function which uses two GPIOs to
> communicate with another device to claim/release a bus.

> diff --git a/Documentation/devicetree/bindings/i2c/arbitrator-i2c.txt 
> b/Documentation/devicetree/bindings/i2c/arbitrator-i2c.txt
> new file mode 100644
> index 000..cb91ea8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/arbitrator-i2c.txt
> @@ -0,0 +1,56 @@
> +Device-Tree bindings for i2c gpio based bus arbitrator
> +
> +bus-arbitration-gpios :
> + Two GPIOs to use with the GPIO-based bus arbitration protocol
> +(see below).
> +The first should be an output, and is used to claim the I2C bus,
> +the second should be an input, and signals that the other side (Client)
> +wants to claim the bus. This allows two masters to share the same I2C bus.

I'm confused why this is even needed; the I2C protocol itself defines
how multi-master is supposed to work, just using the regular SCL/SDA lines.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: I2C and devicetrees

[Stephen Warren]

On 12/11/2012 01:21 PM, Olof Johansson wrote:
> [+devicetree-discuss]
> 
> On Wed, Dec 5, 2012 at 2:36 PM, Peter Huewe  wrote:
>> Hi,
>>
>> I have a short question about the relations between i2c and devicetrees.
>>
>> I was wondering
>> is the device part of the compatible string of a (trivial) i2c device
>> instanciated via devicetree _always_ identical to name in i2c_client.name ?
>> Or can it be somehow different?
> 
> It can be different, but the driver will then need to add a OF table
> that matches the probing. By default the i2c/dt core code will strip
> off the vendor prefix (before ",") and try probing with the rest of
> the device name. If that doesn't match the client name, that is when
> you need the additional table.

While relying on the prefix stripping works, I think I recall Grant
mentioning that people shouldn't rely on it - namely that any I2C device
that gets instantiated from DT should contain the OF match table
explicitly. I CC'd Grant in case I'm mis-quoting him.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of_i2c: Honour "status=disabled" property of device

[Stephen Warren]

On 11/28/2012 07:21 AM, Alexander Sverdlin wrote:
> of_i2c: Honour "status=disabled" property of device
> 
> Currently of_i2c_register_devices() function registers all i2c devices,
> independently from their status property in device tree. According to
> "ePAPR 1.1" spec, device should only be registered if there is no
> "status" property, or it has "ok" (or "okay") value (see
> of_device_is_available()). In case of "platform devices",
> of_platform_device_create_pdata() checks for "status" and ensures
> that disabled devices are not populated. But such check for i2c buses
> was missing until now. Fix it.

Do SPI and other buses (e.g. MDIO/MDIO mux?) need a similar change?

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: mux: Add dt support to i2c-mux-gpio driver

[Stephen Warren]

On 10/22/2012 06:53 AM, Maxime Ripard wrote:
> Allow the i2c-mux-gpio to be used by a device tree enabled device. The
> bindings are inspired by the one found in the i2c-mux-pinctrl driver.

Reviewed-by: Stephen Warren 

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: mux: Add dt support to i2c-mux-gpio driver

[Stephen Warren]

On 10/18/2012 08:13 AM, Maxime Ripard wrote:
> Allow the i2c-mux-gpio to be used by a device tree enabled device. The
> bindings are inspired by the one found in the i2c-mux-pinctrl driver.

> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt

> +Optional properties:
> +- idle-state: value to set to the muxer when idle. When no value is
> +  given, it defaults to the first value in the array.

That's inconsistent with the following text that appears later (and
describes what the driver actually does):

> +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
> +idle value will be programmed into the GPIOs.
> +
> +If an idle state is not defined, the most recently used value will be
> +left programmed into hardware whenever no access is being made of a
> +device on a child bus.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: i2c-gpio: fix name issue when i2c gpio node GT 1

[Stephen Warren]

On 10/15/2012 01:51 AM, Bo Shen wrote:
> When i2c-gpio node number is greater than 1, the name for each is same
> So add the patch to fix it.
> 
> The adap->name printing information is add by myself
> 
> without this patch the log information is as following
> ---<8---
> adap->name = i2c-gpio-1
> i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
> adap->name = i2c-gpio-1
> i2c-gpio i2c.3: using pins 64 (SDA) and 65 (SCL)
> --->8---
> 
> with this patch, the log information is as following
> ---<8---
> adap->name = i2c.2
> i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
> adap->name = i2c.3
> i2c-gpio i2c.3: using pins 64 (SDA) and 65 (SCL)
> --->8---

I think this is reasonable, although I imagine this has zero effect on
the I2C bus number that's assigned to the device, so I'm not sure how it
interacts with or supports the other Atmel patch series you sent, which
I didn't really look at in much detail.

Reviewed-by: Stephen Warren 
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] i2c: mux: Add dt support to i2c-mux-gpio driver

[Stephen Warren]

On 09/21/2012 09:32 AM, Maxime Ripard wrote:
> Allow the i2c-mux-gpio to be used by a device tree enabled device. The
> bindings are inspired by the one found in the i2c-mux-pinctrl driver.
> 
> Signed-off-by: Maxime Ripard 

> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
> @@ -0,0 +1,86 @@
> +GPIO-based I2C Bus Mux
> +
> +This binding describes an I2C bus multiplexer that uses GPIOs to
> +route the I2C signals.
> +
> + +-+  +-+
> + | dev |  | dev |
> +++   +-+  +-+
> +| SoC|  ||
> +|   /|--++
> +|   +---+   +--+ | child bus A, on GPIO value set to 0
> +|   |I2C|---| Mux  | |
> +|   +---+   +--+---+ | child bus B, on GPIO value set to 1
> +|  |\|--+++
> +|   +--+   | |  |||
> +|   | GPIO |---+ |  +-+  +-+  +-+
> +|   +--+ |  | dev |  | dev |  | dev |
> +++  +-+  +-+  +-+

The "Mux" box should be outside the SoC, since it isn't part of the SoC
itself but something external.

> +Required properties:
> +- compatible: i2c-mux-gpio
> +- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
> +  port is connected to.
> +
> +Also required are:
> +
> +- muxer-gpios: list of gpios to use to control the muxer

Perhaps just "mux-gpios"? Bikeshedding, I know...

> +* Standard I2C mux properties. See mux.txt in this directory.
> +
> +* I2C child bus nodes. See mux.txt in this directory.
> +
> +For each i2c child nodes, an I2C child bus will be created. They will
> +be numbered based on the reg property of each nodes.

s/nodes/node/ in both of the two lines above.

> +Whenever an access is made to a device on a child bus, the value set
> +in the revelant node's reg property will be outputed using the list of

s/outputed/output/

> +GPIOs, the first in the list holding the most-significant value.
> +
> +If an idle state is defined, using the idle-state (optional) property,

The idle-state property isn't documented in the list of properties above.

> +whenever an access is not being made to a device on a child bus, the
> +idle value will be programmed into the GPIOs.
> +
> +If an idle state is not defined, the most recently used value will be
> +left programmed into hardware whenever no access is being made of a
> +device on a child bus.
> +
> +Example:
> + i2cmux {
> + compatible = "i2c-mux-gpio";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + muxer-gpios = <&gpio1 22 0 &gpio1 23 0>;
> + i2c-parent = <&i2c1>;
> +
> + i2c@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> +
> + i2c@1 {
> +   reg = <1>;
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> + };

The indentation above is a mix of TABs and spaces, and is inconsistent
between nodes; just TABs would be best.

> +
> + i2c@2 {
> + reg = <2>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };

I'm not sure it's a good idea to have example bus nodes that are empty;
why not leave out two of the options, and put some device on each of the
buses that is defined?


> + i2c@3 {
> + reg = <3>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pca9555: pca9555@20 {
> + compatible = "nxp,pca9555";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x20>;
> + };
> + };
> + };

> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c 
> b/drivers/i2c/muxes/i2c-mux-gpio.c

> +#ifdef CONFIG_OF
> +static int __devinit i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> + struct platform_device *pdev)

> + mux->data->n_values = of_get_child_count(np);
> +
> + values = devm_kzalloc(&pdev->dev, sizeof(*mux->data->values), 
> GFP_KERNEL);

Don't you need to multiply the size by mux->data->n_values?

> + gpios = devm_kzalloc(&pdev->dev, mux->data->n_gpios, GFP_KERNEL);

Don't you need to multiple the size by sizeof(*gpios) here?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Stephen Warren]

On 09/18/2012 07:15 PM, Bo Shen wrote:
> Hi Stephen Warren,
> 
> On 9/18/2012 22:51, Stephen Warren wrote:
>> On 09/18/2012 03:02 AM, Bo Shen wrote:
>>> When i2c-gpio node number is greater than 1, the second can not
>>> sucessfully
>>> regiter
>>>
>>> Using alias method to get the pdev->id, or else the second device
>>> will use
>>> the same id which will cause fail to register
>>
>> pdev->id shouldn't be used at all with device tree. Why does
>> registration fail without this change?
> 
> I add the debug info, and it give the following error without this patch.
> 
> --<8--
> adap->name = i2c-gpio-1
> i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
> adap->name = i2c-gpio-1
> i2c-gpio: probe of i2c.3 failed with error -16
> -->8--
> 
> With this patch, it successfully registered.
> --<8--
> adap->name = i2c-gpio0
> i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
> adap->name = i2c-gpio1
> i2c-gpio i2c.3: using pins 90 (SDA) and 91 (SCL)
> -->8--

Yes, that explains why the registration fails, but not why this patch is
the correct solution to the problem.

The problematic code appears to be:

snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);

Instead, I think that should be something more like:

adap->name = dev_name(&pdev->dev);

or perhaps:

if (pdev->dev.of_node)
  /* named will be based on DT node name */
  adap->name = dev_name(&pdev->dev);
else
  snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);

?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Stephen Warren]

On 09/18/2012 03:02 AM, Bo Shen wrote:
> When i2c-gpio node number is greater than 1, the second can not sucessfully
> regiter
> 
> Using alias method to get the pdev->id, or else the second device will use
> the same id which will cause fail to register

pdev->id shouldn't be used at all with device tree. Why does
registration fail without this change?

> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index e62d2d9..051fbb3 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -136,6 +136,7 @@ static int __devinit i2c_gpio_probe(struct 
> platform_device *pdev)
>   ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata);
>   if (ret)
>   return ret;
> + pdev->id = of_alias_get_id(pdev->dev.of_node, "i2c-gpio");
>   } else {
>   if (!pdev->dev.platform_data)
>   return -ENXIO;
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ARM: dts: exynos4: allow i2c0 bus to be configured using pinctrl interface

[Stephen Warren]

On 09/06/2012 05:14 AM, Thomas Abraham wrote:
> On 6 September 2012 15:43, Tomasz Figa  wrote:
>> Hi Thomas,
>>
>> On Thursday 06 of September 2012 14:53:01 Thomas Abraham wrote:
>>>   compatible = "samsung,s3c2440-i2c";
>>>   reg = <0x1386 0x100>;
>>>   interrupts = <0 58 0>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&i2c0_bus>;
>>
>> If pinctrl-names property is omitted then the state index is used as a name
>> (e.g. pinctrl-0 would be named "0"). Maybe it would be better to use this
>> approach (with respective adjustment in first patch)? What do you think?
> 
> I tend to prefer to name the states because it is easier to
> cross-reference code and dts files. i2c was a simple one, but for mmc
> controllers, there will 1-bit state, 4-bit state and 8-bit state, and
> it will be nicer to name then accordingly. So I prefer to use names
> but if there is wider consensus on not using names, we can drop names.

I would only expect to see multiple states defined in a single board
.dts file /if/ runtime muxing is required. Given MMC doesn't runtime
mux, I wouldn't expect there to be multiple states for different bus
widths; it's just that the "default" state would point at whatever
single configuration is appropriate for the board.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: s3c2410: add optional pin configuration using pinctrl interface

[Stephen Warren]

On 09/06/2012 05:06 AM, Thomas Abraham wrote:
> On 6 September 2012 15:04, Tomasz Figa  wrote:
>> Hi,
>>
>> This patch shows the problem of the need to explicitly migrate all drivers
>> to pinctrl.
>>
>> Maybe we should consider extending the pinctrl subsystem to set the default
>> state automatically before binding a driver to a device, at least in case
>> of DT-based platforms?
> 
> The pinctrl driver allows for activating default pin configuration
> when the pinctrl driver loads. This is referred to as "hogging". But
> should hog be used or not is something that needs to be decided. Some
> of the factors which favor the driver explicitly setting up the pin
> configuration are
> 
> 1. After a suspend and resume cycle, the pin configuration registers
> may be reset to default values. Hence, during resume, the pin
> configuration has be redone.

I'd think it's the pinctrl driver's responsibility to make hogging work
correctly across suspend/resume.

> 2. Runtime muxing/config is possible.

The "client" driver would definitely have to be involved there, I agree.

> 3. Setting some of the config options such as pull-up by default might
> start consuming power from boot time itself, which could be avoided if
> such setup is done only when needed.

Well, the difference in time between "just before driver binding" and
"during probe" is minimal. If the driver/HW really needs to explicitly
differentiate between those states to save power, I'd assert that it's
covered by case (2) above.

> Adding pinctrl driver support in device drivers seems to be simple
> task. And it is just one time effort which can be reused on multiple
> SoC's.
> 
>>
>> This would be similar to what is done currently with samsung-gpio bindings
>> - the pin is being configured by custom xlate callback based on additional
>> cells in GPIO specifier, when the driver retrieves the pin using
>> of_get{_named,}_gpio without the need of setting it up in the driver.
> 
> The Samsung gpio dt bindings was just a bootstrap method to get device
> tree support going for Samsung platforms. The gpio xlate callback was
> used as a back door to setup the pinmux/pinconfig due to lack of
> generic driver interface to setup the pinmux/pinconfig for Samsung
> platforms. From a linux perspective, gpio and pinmux/pinconfig are
> separate entities. So using gpio xlate to setup pinmux/pinconfig was
> not correct but helped getting device tree enabled for Samsung
> platforms. With the pinctrl framework available now, there are generic
> interfaces to setup gpio and pinmux /pinconfig.

I agree; the Samsung GPIO bindings were surprising to me when I first
realized what was in the GPIO specifiers...

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Stephen Warren]

On 09/03/2012 05:58 AM, Linus Walleij wrote:
> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones  wrote:
> 
>> No, this is wrong. Platform data should not override DT.
>>
>> If DT is enabled and passed, it should have highest priority.

No, that's wrong. If platform data is specified, it overrides DT, so
that if the DT needs any fixup, it can be provided using platform data.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20

[Stephen Warren]

On 08/20/2012 11:10 AM, Laxman Dewangan wrote:
> On Monday 20 August 2012 10:45 PM, Stephen Warren wrote:
>> On 08/18/2012 01:17 PM, Laxman Dewangan wrote:
>>> Tegra20 i2c controller does not support the continue transfer
>>> which implements the I2C_M_NOSTART functionality of i2c
>>> protocol mangling.
>>> Removing the I2C_M_NOSTART functionality support for Tegra20.
>> Thanks, applied the series to Tegra's for-3.7/drivers-i2c branch.
>>
>> Note that I had to fix up patch 1 to remove const on the tegra*_i2c_hw
>> variable declarations to avoid compiler warnings when assigning pointers
>> into tegra_i2c_of_match[].data.
>
> Thanks for correction but
> I did not get this warning and when saw the structure, it is declared as
> const type only..

Ah. This is something new in linux-next but not in 3.6-rc2. It was
introduced by:

98d7bbb of: add const to struct *of_device_id.data

I'll revert the modifications I made to the patch, and ignore the
warnings in Tegra's for-next; they won't be present in 3.7 presumably.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20

[Stephen Warren]

On 08/18/2012 01:17 PM, Laxman Dewangan wrote:
> Tegra20 i2c controller does not support the continue transfer
> which implements the I2C_M_NOSTART functionality of i2c
> protocol mangling.
> Removing the I2C_M_NOSTART functionality support for Tegra20.

Thanks, applied the series to Tegra's for-3.7/drivers-i2c branch.

Note that I had to fix up patch 1 to remove const on the tegra*_i2c_hw
variable declarations to avoid compiler warnings when assigning pointers
into tegra_i2c_of_match[].data.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20

[Stephen Warren]

On 08/17/2012 01:02 PM, Laxman Dewangan wrote:
> Tegra20 i2c controller does not support the continue transfer
> which implements the I2C_M_NOSTART functionality of i2c
> protocol mangling.
> Removing the I2C_M_NOSTART functionality for Tegra20.
> 
> Signed-off-by: Laxman Dewangan 

I tested both these patches and they work fine. I'll hold off applying
them for a while for Wolfram to ack them.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: tegra: dynamically control fast clk

[Stephen Warren]

On 08/17/2012 12:45 PM, Laxman Dewangan wrote:
> On Saturday 18 August 2012 12:11 AM, Stephen Warren wrote:
>> On 08/17/2012 01:48 AM, Laxman Dewangan wrote:
>>> Tegra I2C driver enables the fast clock during initialization
>>> and does not disable till driver removed.
>>> Enable this clock before transfer and disable after transfer done.
>>>
>>> Signed-off-by: Laxman Dewangan
>>> ---
>>> This patch is on top of the clock chnages which is in Tegra sub
>>> system and
>>> based on
>>>   i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20
>>> So recommend to go on tegra sub-system.
>> What exactly is this patch based on? I checked out Tegra's
>> for-3.7/drivers-i2c, cherry-picked the M_NOSTART patch you mentioned,
>> and attempted to apply this patch. It doesn't apply. Same if I don't
>> cherry-pick the M_NOSTART patch, and same for next-20120816 with/without
>> the M_NOSTART patch.
> 
> Then It seems I need to create the patch again and send it.  The
> M_NOSTART patch was on tot before clock related change and that is the
> reason it is not applying.
> Should I re-send these two patches together as 1/2 and 2/2 to maintain
> sequence?
> I can create based on your clock tree.

For any I2C-related changes I take through the Tegra tree, basing them
on top of for-3.7/drivers-i2c would be best. Feel free to send both
patches together again if that's easiest.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: tegra: dynamically control fast clk

[Stephen Warren]

On 08/17/2012 01:48 AM, Laxman Dewangan wrote:
> Tegra I2C driver enables the fast clock during initialization
> and does not disable till driver removed.
> Enable this clock before transfer and disable after transfer done.
> 
> Signed-off-by: Laxman Dewangan 
> ---
> This patch is on top of the clock chnages which is in Tegra sub system and
> based on 
>  i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20
> So recommend to go on tegra sub-system.

What exactly is this patch based on? I checked out Tegra's
for-3.7/drivers-i2c, cherry-picked the M_NOSTART patch you mentioned,
and attempted to apply this patch. It doesn't apply. Same if I don't
cherry-pick the M_NOSTART patch, and same for next-20120816 with/without
the M_NOSTART patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20

[Stephen Warren]

On 08/16/2012 01:07 AM, Laxman Dewangan wrote:
> On Tuesday 14 August 2012 10:19 PM, Stephen Warren wrote:
>> On 08/14/2012 03:19 AM, Laxman Dewangan wrote:
>>> Tegra20 i2c controller does not support the continue transfer
>>> which implements the I2C_M_NOSTART functionality of i2c
>>> protocol mangling.
>>> Removing the I2C_M_NOSTART functionality for Tegra20.
>>>
>>> Signed-off-by: Laxman Dewangan
>>> Reported-by: Stephen Warren
>> Tested-by: Stephen Warren
>>
>> Note that if I take Laxman's I2C driver clock patches through the Tegra
>> tree, and Wolfram takes this patch through the I2C tree, there will be a
>> very slight conflict, since adjacent lines are touched. However, the
>> resolution is simple and obvious, so I think that's fine.
> 
> Stephen/Wolfram,
> I have 2 more change to implement runtime PM and dynamic clock control
> for fast clock which I have planned for 3.7.
> I think it will be better if this also goes on same tree where the clock
> related change are available to avoid the merge conflict.

OK, I guess I can take all the Tegra I2C driver changes through the
Tegra tree if needed. This patch is less trivial than the last, so I
would definitely need Wolfram's ack to do so. I suppose if this ends up
conflicting with e.g. any core work in the I2C tree, I can always give
Wolfram a Tegra branch to merge in first to avoid any issues; If needed,
I'd rather wait until after rc2 or perhaps rc3 before doing that though.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] i2c: tegra: required clock support for controller

[Stephen Warren]

On 08/08/2012 11:57 AM, Stephen Warren wrote:
> On 08/08/2012 01:51 AM, Laxman Dewangan wrote:
>> The Tegra's i2c controller required two clock sources for proper
>> operation named as div-clk and fast-clk.
>>
>> Adding support to make sure that driver will get these clocks and 
>> enable before any transfer and disable after transfer completed.
> 
> This works fine on Cardhu and Ventana for me. I'll apply it once I get
> an ack from Wolfram to take the I2C driver patch through the Tegra tree.

I haven't heard from Wolfram, but since the I2C driver patch is pretty
trivial, and pretty much has to go through the Tegra tree due to
dependencies on other Tegra patches, I went ahead and applied this
series to Tegra's for-3.7/clock branch.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20

[Stephen Warren]

On 08/14/2012 03:19 AM, Laxman Dewangan wrote:
> Tegra20 i2c controller does not support the continue transfer
> which implements the I2C_M_NOSTART functionality of i2c
> protocol mangling.
> Removing the I2C_M_NOSTART functionality for Tegra20.
> 
> Signed-off-by: Laxman Dewangan 
> Reported-by: Stephen Warren 

Tested-by: Stephen Warren 

Note that if I take Laxman's I2C driver clock patches through the Tegra
tree, and Wolfram takes this patch through the I2C tree, there will be a
very slight conflict, since adjacent lines are touched. However, the
resolution is simple and obvious, so I think that's fine.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] i2c: tegra: required clock support for controller

[Stephen Warren]

On 08/08/2012 01:51 AM, Laxman Dewangan wrote:
> The Tegra's i2c controller required two clock sources for proper
> operation named as div-clk and fast-clk.
> 
> Adding support to make sure that driver will get these clocks and 
> enable before any transfer and disable after transfer completed.

This works fine on Cardhu and Ventana for me. I'll apply it once I get
an ack from Wolfram to take the I2C driver patch through the Tegra tree.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] i2c: tegra: convert normal suspend/resume to *_noirq

[Stephen Warren]

On 07/10/2012 05:20 AM, Laxman Dewangan wrote:
> To provide the late suspend and early resume for i2c
> driver, convert the suspend/resume as
>   suspend-> suspend_noirq
>   resume -> resume_noirq
> 
> Signed-off-by: Laxman Dewangan 

Why do we need this change?

IIRC, I proposed it before solely to solve some suspend/resume ordering
issues, and Colin Cross NAKd it. These days, deferred probe should make
this change unnecessary.

Unless there's a really good reason for this change, I'd tend to request
reverting it.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: tegra: use clk_disable_unprepare in place of clk_disable

[Stephen Warren]

On 06/26/2012 12:27 AM, Laxman Dewangan wrote:
> On Monday 25 June 2012 09:25 PM, Stephen Warren wrote:
>> On 06/25/2012 03:46 AM, Laxman Dewangan wrote:
>>> Stephen,
>>>
>>> On Wednesday 20 June 2012 09:57 PM, Stephen Warren wrote:
>>>> On 06/20/2012 10:26 AM, Stephen Warren wrote:
>>>>> On 06/20/2012 06:56 AM, Laxman Dewangan wrote:
>>>>>> Use clk_disable_unprepare() inplace of clk_disable().
>>>>>> This was missed as part of moving clock enable/disable to
>>>>>> prepare/unprepare for using the common clock framework.
>>>> ...
>>>>> I see no reason not to take the second patch in the series through the
>>>>> I2C tree though.
>>>> Uggh. Ignore that paragraph - the other patch was sent separately
>>>> not as
>>>> a series.
>>> so are you taking care of this patch or do I need to send the patch
>>> based on your tree in place of linux-next?
>> Yes, this patch should be applied through the Tegra tree, since it will
>> be a dependency of the common clock framework switchover there, which I
>> hope to take place this kernel cycle.
>>
>> I did just attempt to apply this patch to the for-3.6/common-clk branch,
>> but it doesn't apply:-( Could you please rebase and resend. Thanks.
> 
> Looked at your common_clk branch and the related code is not there.
> The clk_disable() in the particular case is introduced by change
> i2c: tegra: make all resource allocation through devm_*
> which is not in your branch.
> 
> Then later Prashant post the change as
> i2c: tegra: Add clk_prepare/clk_unprepare
> and it does not accounted for the above patch.
> 
> So none of your local tree will have this issue.

OK. In that case, it's best if this patch goes through the I2C tree
since that's where the code is that it's modifying. This might not be
optimal for runtime git bisection depending on the order Linus ends up
merging things, but it's probably as good as we can do without
inter-twining the I2C and Tegra trees a lot.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: tegra: use clk_disable_unprepare in place of clk_disable

[Stephen Warren]

On 06/25/2012 03:46 AM, Laxman Dewangan wrote:
> Stephen,
> 
> On Wednesday 20 June 2012 09:57 PM, Stephen Warren wrote:
>> On 06/20/2012 10:26 AM, Stephen Warren wrote:
>>> On 06/20/2012 06:56 AM, Laxman Dewangan wrote:
>>>> Use clk_disable_unprepare() inplace of clk_disable().
>>>> This was missed as part of moving clock enable/disable to
>>>> prepare/unprepare for using the common clock framework.
>> ...
>>> I see no reason not to take the second patch in the series through the
>>> I2C tree though.
>> Uggh. Ignore that paragraph - the other patch was sent separately not as
>> a series.
> 
> so are you taking care of this patch or do I need to send the patch
> based on your tree in place of linux-next?

Yes, this patch should be applied through the Tegra tree, since it will
be a dependency of the common clock framework switchover there, which I
hope to take place this kernel cycle.

I did just attempt to apply this patch to the for-3.6/common-clk branch,
but it doesn't apply:-( Could you please rebase and resend. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: tegra: use clk_disable_unprepare in place of clk_disable

[Stephen Warren]

On 06/20/2012 10:26 AM, Stephen Warren wrote:
> On 06/20/2012 06:56 AM, Laxman Dewangan wrote:
>> Use clk_disable_unprepare() inplace of clk_disable().
>> This was missed as part of moving clock enable/disable to
>> prepare/unprepare for using the common clock framework.
...
> I see no reason not to take the second patch in the series through the
> I2C tree though.

Uggh. Ignore that paragraph - the other patch was sent separately not as
a series.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html