[PATCH v2 1/3] i2c: xilinx: Fix i2c checkpatch warnings

[PATCH v2 1/3] i2c: xilinx: Fix i2c checkpatch warnings

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2899 bytes --]

From: Kedareswara rao Appana <appana.durga.rao-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>

Code changes to fix checkpatch warnings listed below.
- WARNING: please, no space before tabs
- WARNING: quoted string split across lines

Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---
Changes in v2:
- It is checkpatch warnings not sparse
- Extend patch description with checkpatch warnings

Tested by:
./scripts/checkpatch.pl -f drivers/i2c/busses/i2c-xiic.c

---
 drivers/i2c/busses/i2c-xiic.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 4c8b368..5eb0a8b 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -68,7 +68,7 @@ struct xiic_i2c {
 	struct i2c_adapter	adap;
 	struct i2c_msg		*tx_msg;
 	spinlock_t		lock;
-	unsigned int 		tx_pos;
+	unsigned int		tx_pos;
 	unsigned int		nmsgs;
 	enum xilinx_i2c_state	state;
 	struct i2c_msg		*rx_msg;
@@ -271,11 +271,8 @@ static void xiic_read_rx(struct xiic_i2c *i2c)

 	bytes_in_fifo = xiic_getreg8(i2c, XIIC_RFO_REG_OFFSET) + 1;

-	dev_dbg(i2c->adap.dev.parent, "%s entry, bytes in fifo: %d, msg: %d"
-		", SR: 0x%x, CR: 0x%x\n",
-		__func__, bytes_in_fifo, xiic_rx_space(i2c),
-		xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
-		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
+	dev_dbg(i2c->adap.dev.parent, "%s entry, bytes in fifo: %d, msg: %d",
+		__func__, bytes_in_fifo, xiic_rx_space(i2c));

 	if (bytes_in_fifo > xiic_rx_space(i2c))
 		bytes_in_fifo = xiic_rx_space(i2c);
@@ -339,9 +336,10 @@ static void xiic_process(struct xiic_i2c *i2c)
 	ier = xiic_getreg32(i2c, XIIC_IIER_OFFSET);
 	pend = isr & ier;

-	dev_dbg(i2c->adap.dev.parent, "%s entry, IER: 0x%x, ISR: 0x%x, "
-		"pend: 0x%x, SR: 0x%x, msg: %p, nmsgs: %d\n",
-		__func__, ier, isr, pend, xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
+	dev_dbg(i2c->adap.dev.parent, "%s: IER: 0x%x, ISR: 0x%x, pend: 0x%x\n",
+		__func__, ier, isr, pend);
+	dev_dbg(i2c->adap.dev.parent, "%s: SR: 0x%x, msg: %p, nmsgs: %d\n",
+		__func__, xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
 		i2c->tx_msg, i2c->nmsgs);

 	/* Do not processes a devices interrupts if the device has no
@@ -541,9 +539,10 @@ static void xiic_start_send(struct xiic_i2c *i2c)

 	xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK);

-	dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, len: %d, "
-		"ISR: 0x%x, CR: 0x%x\n",
-		__func__, msg, msg->len, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
+	dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, len: %d",
+		__func__, msg, msg->len);
+	dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n",
+		__func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
 		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));

 	if (!(msg->flags & I2C_M_NOSTART)) {
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

From: Kedareswara rao Appana <appana.durga.rao@xilinx.com>

The patch fixes the problem with i2c eeprom memories
where controller is not properly setup to transmit mode.

This problem is fixed in write operation, after filling
address byte to tx fifo, set the direction of transfer
to tx using control register.

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- Extend patch description as Wolfram asked for.

 drivers/i2c/busses/i2c-xiic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 5eb0a8b..44e6ae7 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -536,6 +536,7 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 static void xiic_start_send(struct xiic_i2c *i2c)
 {
 	struct i2c_msg *msg = i2c->tx_msg;
+	u32 cr;

 	xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK);

@@ -556,6 +557,10 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
 	}

+	cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET);
+	cr |= XIIC_CR_DIR_IS_TX_MASK;
+	xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr);
+
 	xiic_fill_tx_fifo(i2c);

 	/* Clear any pending Tx empty, Tx Error and then enable them. */
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 259 bytes --]


> +	cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET);
> +	cr |= XIIC_CR_DIR_IS_TX_MASK;
> +	xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr);
> +

Is there no need to clear the bit again when receiving? And did
transferring ever work if this bit was never set before?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On 10/04/2013 07:46 AM, Wolfram Sang wrote:
> 
>> +	cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET);
>> +	cr |= XIIC_CR_DIR_IS_TX_MASK;
>> +	xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr);
>> +
> 
> Is there no need to clear the bit again when receiving?

This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit()

xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK);


> And did
> transferring ever work if this bit was never set before?

I really don't know. We have switched from old driver to this new mainline one
and based on our eeprom testing we have found that this bit hasn't been setup properly.

It is described here.
http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf
page 28 - step 3.

IIC Master Transmitter with a Repeated Start
1. Write the IIC device address to the TX_FIFO.
2. Write data to TX_FIFO.
3. Write to Control Register (CR) to set MSMS = 1 and TX = 1.
4. Continue writing data to TX_FIFO.
5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
6. Write to CR to set RSTA = 1.
7. Write IIC device address to TX_FIFO.
8. Write all data except last byte to TX_FIFO.
9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte.
11. Write last byte of data to TX_FIFO.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]

On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote:
> On 10/04/2013 07:46 AM, Wolfram Sang wrote:
> > 
> >> +	cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET);
> >> +	cr |= XIIC_CR_DIR_IS_TX_MASK;
> >> +	xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr);
> >> +
> > 
> > Is there no need to clear the bit again when receiving?
> 
> This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit()
> 
> xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK);

A bit implicit, but OK.

> > And did
> > transferring ever work if this bit was never set before?
> 
> I really don't know. We have switched from old driver to this new mainline one
> and based on our eeprom testing we have found that this bit hasn't been setup properly.
> 
> It is described here.
> http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf
> page 28 - step 3.
> 
> IIC Master Transmitter with a Repeated Start
> 1. Write the IIC device address to the TX_FIFO.
> 2. Write data to TX_FIFO.
> 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1.
> 4. Continue writing data to TX_FIFO.
> 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
> 6. Write to CR to set RSTA = 1.

Repeated start is not happening in the driver as well, or am I
overlooking something?

> 7. Write IIC device address to TX_FIFO.
> 8. Write all data except last byte to TX_FIFO.
> 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
> 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte.
> 11. Write last byte of data to TX_FIFO.

CCing more people who worked on the driver in the past and might have
experiences

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation

[Lars-Peter Clausen]

On 10/04/2013 01:55 PM, Wolfram Sang wrote:
> On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote:
>> On 10/04/2013 07:46 AM, Wolfram Sang wrote:
>>>
>>>> +	cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET);
>>>> +	cr |= XIIC_CR_DIR_IS_TX_MASK;
>>>> +	xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr);
>>>> +
>>>
>>> Is there no need to clear the bit again when receiving?
>>
>> This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit()
>>
>> xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK);
> 
> A bit implicit, but OK.
> 
>>> And did
>>> transferring ever work if this bit was never set before?
>>
>> I really don't know. We have switched from old driver to this new mainline one
>> and based on our eeprom testing we have found that this bit hasn't been setup properly.
>>
>> It is described here.
>> http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf
>> page 28 - step 3.
>>
>> IIC Master Transmitter with a Repeated Start
>> 1. Write the IIC device address to the TX_FIFO.
>> 2. Write data to TX_FIFO.
>> 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1.
>> 4. Continue writing data to TX_FIFO.
>> 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
>> 6. Write to CR to set RSTA = 1.
> 
> Repeated start is not happening in the driver as well, or am I
> overlooking something?
> 
>> 7. Write IIC device address to TX_FIFO.
>> 8. Write all data except last byte to TX_FIFO.
>> 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
>> 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte.
>> 11. Write last byte of data to TX_FIFO.
> 
> CCing more people who worked on the driver in the past and might have
> experiences

The current version works fine here. The driver uses whats described in the
datasheet as "dynamic controller logic flow" and not the "standard
controller logic flow". The sequence Michal mentioned above is from the
"standard controller logic flow" section.

- Lars

Re: [PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]



On 10/04/2013 02:12 PM, Lars-Peter Clausen wrote:
> On 10/04/2013 01:55 PM, Wolfram Sang wrote:
>> On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote:
>>> On 10/04/2013 07:46 AM, Wolfram Sang wrote:
>>>>
>>>>> +	cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET);
>>>>> +	cr |= XIIC_CR_DIR_IS_TX_MASK;
>>>>> +	xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr);
>>>>> +
>>>>
>>>> Is there no need to clear the bit again when receiving?
>>>
>>> This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit()
>>>
>>> xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK);
>>
>> A bit implicit, but OK.
>>
>>>> And did
>>>> transferring ever work if this bit was never set before?
>>>
>>> I really don't know. We have switched from old driver to this new mainline one
>>> and based on our eeprom testing we have found that this bit hasn't been setup properly.
>>>
>>> It is described here.
>>> http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf
>>> page 28 - step 3.
>>>
>>> IIC Master Transmitter with a Repeated Start
>>> 1. Write the IIC device address to the TX_FIFO.
>>> 2. Write data to TX_FIFO.
>>> 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1.
>>> 4. Continue writing data to TX_FIFO.
>>> 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
>>> 6. Write to CR to set RSTA = 1.
>>
>> Repeated start is not happening in the driver as well, or am I
>> overlooking something?
>>
>>> 7. Write IIC device address to TX_FIFO.
>>> 8. Write all data except last byte to TX_FIFO.
>>> 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
>>> 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte.
>>> 11. Write last byte of data to TX_FIFO.
>>
>> CCing more people who worked on the driver in the past and might have
>> experiences
> 
> The current version works fine here. The driver uses whats described in the
> datasheet as "dynamic controller logic flow" and not the "standard
> controller logic flow". The sequence Michal mentioned above is from the
> "standard controller logic flow" section.

Does this change break "dynamic controller logic flow"?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation

[Lars-Peter Clausen]

On 10/04/2013 03:09 PM, Michal Simek wrote:
> 
> 
> On 10/04/2013 02:12 PM, Lars-Peter Clausen wrote:
>> On 10/04/2013 01:55 PM, Wolfram Sang wrote:
>>> On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote:
>>>> On 10/04/2013 07:46 AM, Wolfram Sang wrote:
>>>>>
>>>>>> +	cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET);
>>>>>> +	cr |= XIIC_CR_DIR_IS_TX_MASK;
>>>>>> +	xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr);
>>>>>> +
>>>>>
>>>>> Is there no need to clear the bit again when receiving?
>>>>
>>>> This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit()
>>>>
>>>> xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK);
>>>
>>> A bit implicit, but OK.
>>>
>>>>> And did
>>>>> transferring ever work if this bit was never set before?
>>>>
>>>> I really don't know. We have switched from old driver to this new mainline one
>>>> and based on our eeprom testing we have found that this bit hasn't been setup properly.
>>>>
>>>> It is described here.
>>>> http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf
>>>> page 28 - step 3.
>>>>
>>>> IIC Master Transmitter with a Repeated Start
>>>> 1. Write the IIC device address to the TX_FIFO.
>>>> 2. Write data to TX_FIFO.
>>>> 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1.
>>>> 4. Continue writing data to TX_FIFO.
>>>> 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
>>>> 6. Write to CR to set RSTA = 1.
>>>
>>> Repeated start is not happening in the driver as well, or am I
>>> overlooking something?
>>>
>>>> 7. Write IIC device address to TX_FIFO.
>>>> 8. Write all data except last byte to TX_FIFO.
>>>> 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
>>>> 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte.
>>>> 11. Write last byte of data to TX_FIFO.
>>>
>>> CCing more people who worked on the driver in the past and might have
>>> experiences
>>
>> The current version works fine here. The driver uses whats described in the
>> datasheet as "dynamic controller logic flow" and not the "standard
>> controller logic flow". The sequence Michal mentioned above is from the
>> "standard controller logic flow" section.
> 
> Does this change break "dynamic controller logic flow"?

Not sure, but I would assume that the bit is ignored in this mode. But I
don't think the patch should be applied since this step is not in the
sequence of steps that should be done.

- Lars

Re: [PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 3103 bytes --]

On 10/04/2013 03:38 PM, Lars-Peter Clausen wrote:
> On 10/04/2013 03:09 PM, Michal Simek wrote:
>>
>>
>> On 10/04/2013 02:12 PM, Lars-Peter Clausen wrote:
>>> On 10/04/2013 01:55 PM, Wolfram Sang wrote:
>>>> On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote:
>>>>> On 10/04/2013 07:46 AM, Wolfram Sang wrote:
>>>>>>
>>>>>>> +	cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET);
>>>>>>> +	cr |= XIIC_CR_DIR_IS_TX_MASK;
>>>>>>> +	xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr);
>>>>>>> +
>>>>>>
>>>>>> Is there no need to clear the bit again when receiving?
>>>>>
>>>>> This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit()
>>>>>
>>>>> xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK);
>>>>
>>>> A bit implicit, but OK.
>>>>
>>>>>> And did
>>>>>> transferring ever work if this bit was never set before?
>>>>>
>>>>> I really don't know. We have switched from old driver to this new mainline one
>>>>> and based on our eeprom testing we have found that this bit hasn't been setup properly.
>>>>>
>>>>> It is described here.
>>>>> http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf
>>>>> page 28 - step 3.
>>>>>
>>>>> IIC Master Transmitter with a Repeated Start
>>>>> 1. Write the IIC device address to the TX_FIFO.
>>>>> 2. Write data to TX_FIFO.
>>>>> 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1.
>>>>> 4. Continue writing data to TX_FIFO.
>>>>> 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
>>>>> 6. Write to CR to set RSTA = 1.
>>>>
>>>> Repeated start is not happening in the driver as well, or am I
>>>> overlooking something?
>>>>
>>>>> 7. Write IIC device address to TX_FIFO.
>>>>> 8. Write all data except last byte to TX_FIFO.
>>>>> 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus.
>>>>> 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte.
>>>>> 11. Write last byte of data to TX_FIFO.
>>>>
>>>> CCing more people who worked on the driver in the past and might have
>>>> experiences
>>>
>>> The current version works fine here. The driver uses whats described in the
>>> datasheet as "dynamic controller logic flow" and not the "standard
>>> controller logic flow". The sequence Michal mentioned above is from the
>>> "standard controller logic flow" section.
>>
>> Does this change break "dynamic controller logic flow"?
> 
> Not sure, but I would assume that the bit is ignored in this mode. But I
> don't think the patch should be applied since this step is not in the
> sequence of steps that should be done.

Kedar: Can you please look at both these modes and provide feedback?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

RE: [PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation

[Appana Durga Kedareswara Rao]

Hi Michal,

> -----Original Message-----
> From: Michal Simek [mailto:monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org]
> Sent: Friday, October 04, 2013 7:08 PM
> To: Lars-Peter Clausen
> Cc: Wolfram Sang; Michal Simek; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Appana
> Durga Kedareswara Rao; Appana Durga Kedareswara Rao; Jean Delvare;
> Peter Korsgaard; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Richard Röjfors; Steven A. Falco
> Subject: Re: [PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation
>
> On 10/04/2013 03:38 PM, Lars-Peter Clausen wrote:
> > On 10/04/2013 03:09 PM, Michal Simek wrote:
> >>
> >>
> >> On 10/04/2013 02:12 PM, Lars-Peter Clausen wrote:
> >>> On 10/04/2013 01:55 PM, Wolfram Sang wrote:
> >>>> On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote:
> >>>>> On 10/04/2013 07:46 AM, Wolfram Sang wrote:
> >>>>>>
> >>>>>>> +     cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET);
> >>>>>>> +     cr |= XIIC_CR_DIR_IS_TX_MASK;
> >>>>>>> +     xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr);
> >>>>>>> +
> >>>>>>
> >>>>>> Is there no need to clear the bit again when receiving?
> >>>>>
> >>>>> This bit is cleared in xiic_xfer() -> xiic_start_xfer()
> >>>>> ->xiic_reinit()
> >>>>>
> >>>>> xiic_setreg8(i2c, XIIC_CR_REG_OFFSET,
> XIIC_CR_TX_FIFO_RESET_MASK);
> >>>>
> >>>> A bit implicit, but OK.
> >>>>
> >>>>>> And did
> >>>>>> transferring ever work if this bit was never set before?
> >>>>>
> >>>>> I really don't know. We have switched from old driver to this new
> >>>>> mainline one and based on our eeprom testing we have found that
> this bit hasn't been setup properly.
> >>>>>
> >>>>> It is described here.
> >>>>>
> http://www.xilinx.com/support/documentation/ip_documentation/axi_i
> >>>>> ic/v1_02_a/axi_iic_ds756.pdf
> >>>>> page 28 - step 3.
> >>>>>
> >>>>> IIC Master Transmitter with a Repeated Start 1. Write the IIC
> >>>>> device address to the TX_FIFO.
> >>>>> 2. Write data to TX_FIFO.
> >>>>> 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1.
> >>>>> 4. Continue writing data to TX_FIFO.
> >>>>> 5. Wait for transmit FIFO empty interrupt. This implies the IIC has
> throttled the bus.
> >>>>> 6. Write to CR to set RSTA = 1.
> >>>>
> >>>> Repeated start is not happening in the driver as well, or am I
> >>>> overlooking something?
> >>>>
> >>>>> 7. Write IIC device address to TX_FIFO.
> >>>>> 8. Write all data except last byte to TX_FIFO.
> >>>>> 9. Wait for transmit FIFO empty interrupt. This implies the IIC has
> throttled the bus.
> >>>>> 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at
> the end of the last byte.
> >>>>> 11. Write last byte of data to TX_FIFO.
> >>>>
> >>>> CCing more people who worked on the driver in the past and might
> >>>> have experiences
> >>>
> >>> The current version works fine here. The driver uses whats described
> >>> in the datasheet as "dynamic controller logic flow" and not the
> >>> "standard controller logic flow". The sequence Michal mentioned
> >>> above is from the "standard controller logic flow" section.
> >>
> >> Does this change break "dynamic controller logic flow"?
> >
> > Not sure, but I would assume that the bit is ignored in this mode. But
> > I don't think the patch should be applied since this step is not in
> > the sequence of steps that should be done.
>
> Kedar: Can you please look at both these modes and provide feedback?

The driver is following the Dynamic controller logic flow as per the dynamic
Controller logic flow. These bits need not to be set.

It was my mistake. Please ignore this patch.
There was an incorrect h/w design on which I tested this that's why I thought it is a
Solution for this.

Now I tested with the proper h/w design it is working fine for me without setting this bit.
Thanks lars for the feedback.

Regards,
Kedar.
>
> Thanks,
> Michal
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
> Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-
> BOOT custodian and responsible for u-boot arm zynq platform
>



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

[PATCH v2 3/3] i2c: xilinx: Use devm_* functions

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 3595 bytes --]

From: Kedareswara rao Appana <appana.durga.rao-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>

Simplified the probe and remove functions using devm_* functions

Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---
Changes in v2: None

 drivers/i2c/busses/i2c-xiic.c | 69 +++++++++++--------------------------------
 1 file changed, 18 insertions(+), 51 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 44e6ae7..6dd3212 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -32,6 +32,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/errno.h>
+#include <linux/err.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/i2c.h>
@@ -698,32 +699,28 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 	int ret, irq;
 	u8 i;

+	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		goto resource_missing;
+	i2c->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(i2c->base)) {
+		dev_err(&pdev->dev, "Could not allocate iomem\n");
+		return PTR_ERR(i2c->base);
+	}

 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
-		goto resource_missing;
-
-	pdata = (struct xiic_i2c_platform_data *)dev_get_platdata(&pdev->dev);
+		return irq;

-	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
-	if (!i2c)
-		return -ENOMEM;
-
-	if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
-		dev_err(&pdev->dev, "Memory region busy\n");
-		ret = -EBUSY;
-		goto request_mem_failed;
+	ret = devm_request_irq(&pdev->dev, irq, xiic_isr, 0, pdev->name, i2c);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Cannot claim IRQ\n");
+		return ret;
 	}

-	i2c->base = ioremap(res->start, resource_size(res));
-	if (!i2c->base) {
-		dev_err(&pdev->dev, "Unable to map registers\n");
-		ret = -EIO;
-		goto map_failed;
-	}
+	pdata = (struct xiic_i2c_platform_data *)dev_get_platdata(&pdev->dev);

 	/* hook up driver to tree */
 	platform_set_drvdata(pdev, i2c);
@@ -736,17 +733,13 @@ static int xiic_i2c_probe(struct platform_device *pdev)

 	spin_lock_init(&i2c->lock);
 	init_waitqueue_head(&i2c->wait);
-	ret = request_irq(irq, xiic_isr, 0, pdev->name, i2c);
-	if (ret) {
-		dev_err(&pdev->dev, "Cannot claim IRQ\n");
-		goto request_irq_failed;
-	}

 	/* add i2c adapter to i2c tree */
 	ret = i2c_add_adapter(&i2c->adap);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add adapter\n");
-		goto add_adapter_failed;
+		xiic_deinit(i2c);
+		return ret;
 	}

 	if (pdata) {
@@ -756,43 +749,17 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 	}

 	return 0;
-
-add_adapter_failed:
-	free_irq(irq, i2c);
-request_irq_failed:
-	xiic_deinit(i2c);
-	iounmap(i2c->base);
-map_failed:
-	release_mem_region(res->start, resource_size(res));
-request_mem_failed:
-	kfree(i2c);
-
-	return ret;
-resource_missing:
-	dev_err(&pdev->dev, "IRQ or Memory resource is missing\n");
-	return -ENOENT;
 }

 static int xiic_i2c_remove(struct platform_device *pdev)
 {
 	struct xiic_i2c *i2c = platform_get_drvdata(pdev);
-	struct resource *res;

 	/* remove adapter & data */
 	i2c_del_adapter(&i2c->adap);

 	xiic_deinit(i2c);

-	free_irq(platform_get_irq(pdev, 0), i2c);
-
-	iounmap(i2c->base);
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (res)
-		release_mem_region(res->start, resource_size(res));
-
-	kfree(i2c);
-
 	return 0;
 }

--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 3/3] i2c: xilinx: Use devm_* functions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 416 bytes --]


> +	i2c->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(i2c->base)) {
> +		dev_err(&pdev->dev, "Could not allocate iomem\n");

devm_ioremap_resource already prints error messages.

> +	ret = devm_request_irq(&pdev->dev, irq, xiic_isr, 0, pdev->name, i2c);

This is too early. Can you find out why?

> +	pdata = (struct xiic_i2c_platform_data *)dev_get_platdata(&pdev->dev);

Casting a void pointer?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 3/3] i2c: xilinx: Use devm_* functions

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

On 10/04/2013 07:33 AM, Wolfram Sang wrote:
> 
>> +	i2c->base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(i2c->base)) {
>> +		dev_err(&pdev->dev, "Could not allocate iomem\n");
> 
> devm_ioremap_resource already prints error messages.

you are right.

> 
>> +	ret = devm_request_irq(&pdev->dev, irq, xiic_isr, 0, pdev->name, i2c);
> 
> This is too early. Can you find out why?

Why do you think that it is too early?
I am looking at origin code again and I think that the code
is also problematic because in xiic_reinit() interrupts are enabled
but they are requested later.
Shouldn't be there a logic that interrupts should be enabled when
interrupts are registered by the kernel?

>> +	pdata = (struct xiic_i2c_platform_data *)dev_get_platdata(&pdev->dev);
> 
> Casting a void pointer?

No problem with that.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 3/3] i2c: xilinx: Use devm_* functions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

On Fri, Oct 04, 2013 at 11:16:20AM +0200, Michal Simek wrote:
> On 10/04/2013 07:33 AM, Wolfram Sang wrote:
> > 
> >> +	i2c->base = devm_ioremap_resource(&pdev->dev, res);
> >> +	if (IS_ERR(i2c->base)) {
> >> +		dev_err(&pdev->dev, "Could not allocate iomem\n");
> > 
> > devm_ioremap_resource already prints error messages.
> 
> you are right.
> 
> > 
> >> +	ret = devm_request_irq(&pdev->dev, irq, xiic_isr, 0, pdev->name, i2c);
> > 
> > This is too early. Can you find out why?
> 
> Why do you think that it is too early?

The ISR uses spinlocks which are not initialized by then.

> I am looking at origin code again and I think that the code
> is also problematic because in xiic_reinit() interrupts are enabled
> but they are requested later.
> Shouldn't be there a logic that interrupts should be enabled when
> interrupts are registered by the kernel?

First register the handler, then activate interrupts. You are right,
this needs to be fixed, too.

Regards,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 3/3] i2c: xilinx: Use devm_* functions

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]

On 10/04/2013 01:58 PM, Wolfram Sang wrote:
> On Fri, Oct 04, 2013 at 11:16:20AM +0200, Michal Simek wrote:
>> On 10/04/2013 07:33 AM, Wolfram Sang wrote:
>>>
>>>> +	i2c->base = devm_ioremap_resource(&pdev->dev, res);
>>>> +	if (IS_ERR(i2c->base)) {
>>>> +		dev_err(&pdev->dev, "Could not allocate iomem\n");
>>>
>>> devm_ioremap_resource already prints error messages.
>>
>> you are right.
>>
>>>
>>>> +	ret = devm_request_irq(&pdev->dev, irq, xiic_isr, 0, pdev->name, i2c);
>>>
>>> This is too early. Can you find out why?
>>
>> Why do you think that it is too early?
> 
> The ISR uses spinlocks which are not initialized by then.

And waitqueue too.  Ok I will keep that request irq in current location
and I will add xiic_reinit() below this code.

>> I am looking at origin code again and I think that the code
>> is also problematic because in xiic_reinit() interrupts are enabled
>> but they are requested later.
>> Shouldn't be there a logic that interrupts should be enabled when
>> interrupts are registered by the kernel?
> 
> First register the handler, then activate interrupts. You are right,
> this needs to be fixed, too.

Do you want me to create separate patch just about moving request irq in front of
xiic_reinit()? And then devm_ conversion?

Thanks,
Michal


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 264 bytes --]

Re: [PATCH v2 3/3] i2c: xilinx: Use devm_* functions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]


> > First register the handler, then activate interrupts. You are right,
> > this needs to be fixed, too.
> 
> Do you want me to create separate patch just about moving request irq in front of
> xiic_reinit()? And then devm_ conversion?

Yes, seperate, please.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]