Re: [PATCH v3 2/3] i2c: iproc: Add Broadcom iProc I2C Driver
Hello,
On Tue, Jan 13, 2015 at 06:14:17PM -0800, Ray Jui wrote:
> >> + irq = platform_get_irq(pdev, 0);
> >> + if (irq < 0) {
> > irq == 0 should be handled as error, too.
> >
> Ah. I thought zero is a valid global interrupt number, and I see other
> drivers checking against < 0 as well. Is my understanding incorrect?
These are wrong, too. 0 should never be a valid interrupt number. There
are some exceptions but mostly for historic reasons. The right handling
is used for example in drivers/i2c/busses/i2c-efm32.c.
> >> + dev_err(dev->device, "no irq resource\n");
> >> + return irq;
> >> + }
> [...]
> >> +static int bcm_iproc_i2c_remove(struct platform_device *pdev)
> >> +{
> >> + struct bcm_iproc_i2c_dev *dev = platform_get_drvdata(pdev);
> >> +
> >> + i2c_del_adapter(&dev->adapter);
> >> + bcm_iproc_i2c_disable(dev);
> > I think you have a problem here if bcm_iproc_i2c_remove is called while
> > an irq is still being serviced. I'm not sure how to prevent this
> > properly for a shared interrupt.
> >
> Can I grab i2c_lock_adapter to ensure the bus is locked (so there's no
> outstanding transactions or IRQs by the time we remove the adapter)? But
> I see no I2C bus driver does this in their remove function...
The problem I pointed out is the reason for some driver authors not to
use devm_request_irq. If you use plain request_irq and the matching
free_irq in the .remove callback you can be sure that the irq isn't
running any more as soon as free_irq returns.
BTW, if you use vim, you can add
set cinoptions=(,:
if has("autocmd")
filetype plugin indent on
endif
to your .vimrc. Then while typing vim does the indention right and
consistent, and with the = command you can reindent.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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: designware: use {readl|writel}_relaxed instead of readl/writel
Dear Wolfram, On Tue, 13 Jan 2015 06:36:54 -0800 Wolfram Sang wrote: > > On Thu, Dec 11, 2014 at 02:26:41PM +0800, Jisheng Zhang wrote: > > readl/writel is too expensive especially on Cortex A9 w/ outer L2 cache. > > This introduces i2c read/write errors on Marvell BG2/BG2Q SoCs when there > > are heavy L2 cache maintenance operations at the same time. > > Reading this again, I got a question: > > Really read/write errors? I would think that there is a performance > penalty because of the memory barriers. But errors? I dunno whether I can call the issue as error. The situation is one i2c client has a bit strict timing requirement. Without the patch, if there are heavy L2 cache maintenance operations at the same time, there may be long delay between each DW_IC_DATA_CMD write opeartions in i2c_dw_xfer_msg() in the DW_IC_INTR_TX_EMPTY isr. Thus about 1/300 i2c transactions may be ignored by the i2c client per my test. > > > The driver does not perform DMA, so it's safe to use the relaxed version. > > From another side, the relaxed io accessor macros are available on all > > architectures now, so we can use the relaxed versions instead. > > Can the designware core make use of DMA in theory? > the IP can do DMA in theory. But currently, there's no DMA support in the driver. Thanks for your review, Jisheng -- 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/3] i2c: iproc: Add Broadcom iProc I2C Driver
On 1/13/2015 2:50 PM, Uwe Kleine-König wrote: > Hello, > > On Tue, Dec 09, 2014 at 07:57:11PM -0800, Ray Jui wrote: >> Add initial support to the Broadcom iProc I2C controller found in the >> iProc family of SoCs. >> >> The iProc I2C controller has separate internal TX and RX FIFOs, each has >> a size of 64 bytes. The iProc I2C controller supports two bus speeds >> including standard mode (100kHz) and fast mode (400kHz) >> >> Signed-off-by: Ray Jui >> Reviewed-by: Scott Branden >> --- >> drivers/i2c/busses/Kconfig | 10 + >> drivers/i2c/busses/Makefile|1 + >> drivers/i2c/busses/i2c-bcm-iproc.c | 500 >> >> 3 files changed, 511 insertions(+) >> create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c >> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index c1351d9..df21366 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -372,6 +372,16 @@ config I2C_BCM2835 >>This support is also available as a module. If so, the module >>will be called i2c-bcm2835. >> >> +config I2C_BCM_IPROC >> +tristate "Broadcom iProc I2C controller" >> +depends on ARCH_BCM_IPROC >> +default y > It would be nice to have the following here to improve compile coverage > testing: > > depends on ARCH_BCM_IPROC || COMPILE_TEST > default ARCH_BCM_IPROC > Sure will do! >> +help >> + If you say yes to this option, support will be included for the >> + Broadcom iProc I2C controller. >> + >> + If you don't know what to do here, say N. >> + >> config I2C_BCM_KONA >> tristate "BCM Kona I2C adapter" >> depends on ARCH_BCM_MOBILE >> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile >> index 5e6c822..216e7be 100644 >> --- a/drivers/i2c/busses/Makefile >> +++ b/drivers/i2c/busses/Makefile >> @@ -33,6 +33,7 @@ obj-$(CONFIG_I2C_AT91) += i2c-at91.o >> obj-$(CONFIG_I2C_AU1550)+= i2c-au1550.o >> obj-$(CONFIG_I2C_AXXIA) += i2c-axxia.o >> obj-$(CONFIG_I2C_BCM2835) += i2c-bcm2835.o >> +obj-$(CONFIG_I2C_BCM_IPROC) += i2c-bcm-iproc.o >> obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o >> obj-$(CONFIG_I2C_CADENCE) += i2c-cadence.o >> obj-$(CONFIG_I2C_CBUS_GPIO) += i2c-cbus-gpio.o >> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c >> b/drivers/i2c/busses/i2c-bcm-iproc.c >> new file mode 100644 >> index 000..35ac497 >> --- /dev/null >> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c >> @@ -0,0 +1,500 @@ >> +/* >> + * Copyright (C) 2014 Broadcom Corporation >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation version 2. >> + * >> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any >> + * kind, whether express or implied; 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 >> +#include >> +#include >> + >> +#define CFG_OFFSET 0x00 >> +#define CFG_RESET_SHIFT 31 >> +#define CFG_EN_SHIFT 30 >> +#define CFG_M_RETRY_CNT_SHIFT16 >> +#define CFG_M_RETRY_CNT_MASK 0x0f >> + >> +#define TIM_CFG_OFFSET 0x04 >> +#define TIME_CFG_MODE_400_SHIFT 31 >> + >> +#define M_FIFO_CTRL_OFFSET 0x0c >> +#define M_FIFO_RX_FLUSH_SHIFT31 >> +#define M_FIFO_TX_FLUSH_SHIFT30 >> +#define M_FIFO_RX_CNT_SHIFT 16 >> +#define M_FIFO_RX_CNT_MASK 0x7f >> +#define M_FIFO_RX_THLD_SHIFT 8 >> +#define M_FIFO_RX_THLD_MASK 0x3f >> + >> +#define M_CMD_OFFSET 0x30 >> +#define M_CMD_START_BUSY_SHIFT 31 >> +#define M_CMD_STATUS_SHIFT 25 >> +#define M_CMD_STATUS_MASK0x07 >> +#define M_CMD_STATUS_SUCCESS 0x0 >> +#define M_CMD_STATUS_LOST_ARB0x1 >> +#define M_CMD_STATUS_NACK_ADDR 0x2 >> +#define M_CMD_STATUS_NACK_DATA 0x3 >> +#define M_CMD_STATUS_TIMEOUT 0x4 >> +#define M_CMD_PROTOCOL_SHIFT 9 >> +#define M_CMD_PROTOCOL_MASK 0xf >> +#define M_CMD_PROTOCOL_BLK_WR0x7 >> +#define M_CMD_PROTOCOL_BLK_RD0x8 >> +#define M_CMD_PEC_SHIFT 8 >> +#define M_CMD_RD_CNT_SHIFT 0 >> +#define M_CMD_RD_CNT_MASK0xff >> + >> +#define IE_OFFSET0x38 >> +#define IE_M_RX_FIFO_FULL_SHIFT 31 >> +#define IE_M_RX_THLD_SHIFT 30 >> +#define IE_M_START_BUSY_SHIFT28 >> + >> +#define IS_OFFSET0x3c >> +#define IS_M_RX_FIFO_FULL_SHIFT 31 >> +#define IS_M_RX_THLD_SHIFT 30 >> +#define IS_M_START_BUSY_SHIFT28 >> + >> +#defin
Re: [PATCH v3 2/3] i2c: iproc: Add Broadcom iProc I2C Driver
Hello, On Tue, Dec 09, 2014 at 07:57:11PM -0800, Ray Jui wrote: > Add initial support to the Broadcom iProc I2C controller found in the > iProc family of SoCs. > > The iProc I2C controller has separate internal TX and RX FIFOs, each has > a size of 64 bytes. The iProc I2C controller supports two bus speeds > including standard mode (100kHz) and fast mode (400kHz) > > Signed-off-by: Ray Jui > Reviewed-by: Scott Branden > --- > drivers/i2c/busses/Kconfig | 10 + > drivers/i2c/busses/Makefile|1 + > drivers/i2c/busses/i2c-bcm-iproc.c | 500 > > 3 files changed, 511 insertions(+) > create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index c1351d9..df21366 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -372,6 +372,16 @@ config I2C_BCM2835 > This support is also available as a module. If so, the module > will be called i2c-bcm2835. > > +config I2C_BCM_IPROC > + tristate "Broadcom iProc I2C controller" > + depends on ARCH_BCM_IPROC > + default y It would be nice to have the following here to improve compile coverage testing: depends on ARCH_BCM_IPROC || COMPILE_TEST default ARCH_BCM_IPROC > + help > + If you say yes to this option, support will be included for the > + Broadcom iProc I2C controller. > + > + If you don't know what to do here, say N. > + > config I2C_BCM_KONA > tristate "BCM Kona I2C adapter" > depends on ARCH_BCM_MOBILE > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > index 5e6c822..216e7be 100644 > --- a/drivers/i2c/busses/Makefile > +++ b/drivers/i2c/busses/Makefile > @@ -33,6 +33,7 @@ obj-$(CONFIG_I2C_AT91) += i2c-at91.o > obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o > obj-$(CONFIG_I2C_AXXIA) += i2c-axxia.o > obj-$(CONFIG_I2C_BCM2835)+= i2c-bcm2835.o > +obj-$(CONFIG_I2C_BCM_IPROC) += i2c-bcm-iproc.o > obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o > obj-$(CONFIG_I2C_CADENCE)+= i2c-cadence.o > obj-$(CONFIG_I2C_CBUS_GPIO) += i2c-cbus-gpio.o > diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c > b/drivers/i2c/busses/i2c-bcm-iproc.c > new file mode 100644 > index 000..35ac497 > --- /dev/null > +++ b/drivers/i2c/busses/i2c-bcm-iproc.c > @@ -0,0 +1,500 @@ > +/* > + * Copyright (C) 2014 Broadcom Corporation > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; 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 > +#include > +#include > + > +#define CFG_OFFSET 0x00 > +#define CFG_RESET_SHIFT 31 > +#define CFG_EN_SHIFT 30 > +#define CFG_M_RETRY_CNT_SHIFT16 > +#define CFG_M_RETRY_CNT_MASK 0x0f > + > +#define TIM_CFG_OFFSET 0x04 > +#define TIME_CFG_MODE_400_SHIFT 31 > + > +#define M_FIFO_CTRL_OFFSET 0x0c > +#define M_FIFO_RX_FLUSH_SHIFT31 > +#define M_FIFO_TX_FLUSH_SHIFT30 > +#define M_FIFO_RX_CNT_SHIFT 16 > +#define M_FIFO_RX_CNT_MASK 0x7f > +#define M_FIFO_RX_THLD_SHIFT 8 > +#define M_FIFO_RX_THLD_MASK 0x3f > + > +#define M_CMD_OFFSET 0x30 > +#define M_CMD_START_BUSY_SHIFT 31 > +#define M_CMD_STATUS_SHIFT 25 > +#define M_CMD_STATUS_MASK0x07 > +#define M_CMD_STATUS_SUCCESS 0x0 > +#define M_CMD_STATUS_LOST_ARB0x1 > +#define M_CMD_STATUS_NACK_ADDR 0x2 > +#define M_CMD_STATUS_NACK_DATA 0x3 > +#define M_CMD_STATUS_TIMEOUT 0x4 > +#define M_CMD_PROTOCOL_SHIFT 9 > +#define M_CMD_PROTOCOL_MASK 0xf > +#define M_CMD_PROTOCOL_BLK_WR0x7 > +#define M_CMD_PROTOCOL_BLK_RD0x8 > +#define M_CMD_PEC_SHIFT 8 > +#define M_CMD_RD_CNT_SHIFT 0 > +#define M_CMD_RD_CNT_MASK0xff > + > +#define IE_OFFSET0x38 > +#define IE_M_RX_FIFO_FULL_SHIFT 31 > +#define IE_M_RX_THLD_SHIFT 30 > +#define IE_M_START_BUSY_SHIFT28 > + > +#define IS_OFFSET0x3c > +#define IS_M_RX_FIFO_FULL_SHIFT 31 > +#define IS_M_RX_THLD_SHIFT 30 > +#define IS_M_START_BUSY_SHIFT28 > + > +#define M_TX_OFFSET 0x40 > +#define M_TX_WR_STATUS_SHIFT 31 > +#define M_TX_DATA_SHIFT 0 > +#define M_TX_DATA_MASK 0xff > + > +#define M
[PATCH v6] i2c: cadence: Check for errata condition involving master receive
Cadence I2C controller has the following bugs:
- completion indication is not given to the driver at the end of
a read/receive transfer with HOLD bit set.
- Invalid read transaction are generated on the bus when HW timeout
condition occurs with HOLD bit set.
As a result of the above, if a set of messages to be transferred with
repeated start includes any message following a read message,
completion is never indicated and timeout occurs.
Hence a check is implemented to return -EOPNOTSUPP for such sequences.
Signed-off-by: Harini Katakam
Signed-off-by: Vishnu Motghare
---
v6:
- Correct if condition - add braces to include both statements.
- Modify comments and warning message.
v5:
Make warning grepable in driver.
v4:
Use single dev_warn and make message grep-able.
v3:
Add warning in case of unsupported transfer.
v2:
Dont defeteature repeated start. Just check for unsupported conditions in the
driver and return error.
---
drivers/i2c/busses/i2c-cadence.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 5f5d4fa..c7be4fb 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -541,6 +541,19 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs,
* processed with a repeated start.
*/
if (num > 1) {
+ /*
+* This controller does not give completion interrupt after a
+* master receive message if HOLD bit is set (repeated start),
+* resulting in SW timeout. Hence, if a receive message is
+* followed by any other message, an error is returned
+* indicating that this sequence is not supported.
+*/
+ for (count = 0; count < num-1; count++) {
+ if (msgs[count].flags & I2C_M_RD) {
+ dev_warn(adap->dev.parent, "Can't do repeated
start after a receive message\n");
+ return -EOPNOTSUPP;
+ }
+ }
id->bus_hold_flag = 1;
reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
reg |= CDNS_I2C_CR_HOLD;
--
1.7.9.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 v5 1/2] i2c: cadence: Handle > 252 byte transfers
Hi,
On Tue, Jan 13, 2015 at 4:58 PM, Wolfram Sang wrote:
> On Fri, Dec 12, 2014 at 09:48:26AM +0530, Harini Katakam wrote:
>> The I2C controller sends a NACK to the slave when transfer size register
>> reaches zero, irrespective of the hold bit. So, in order to handle transfers
>> greater than 252 bytes, the transfer size register has to be maintained at a
>> value >= 1. This patch implements the same.
>> The interrupt status is cleared at the beginning of the isr instead of
>> the end, to avoid missing any interrupts.
>>
>> Signed-off-by: Harini Katakam
>
> Applied to for-next, thanks!
>
>> @@ -333,10 +359,11 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>>* receive if it is less than transfer size and transfer size if
>>* it is more. Enable the interrupts.
>>*/
>> - if (id->recv_count > CDNS_I2C_TRANSFER_SIZE)
>> + if (id->recv_count > CDNS_I2C_TRANSFER_SIZE) {
>> cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE,
>> CDNS_I2C_XFER_SIZE_OFFSET);
>> - else
>> + id->curr_recv_count = CDNS_I2C_TRANSFER_SIZE;
>> + } else
>> cdns_i2c_writereg(id->recv_count, CDNS_I2C_XFER_SIZE_OFFSET);
>
> else branch must have braces, too! I fixed that.
Thanks!
Regards,
Harini
--
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 2/2] i2c: cadence: Check for errata condition involving master receive
Hi,
On Tue, Jan 13, 2015 at 5:05 PM, Wolfram Sang wrote:
>> @@ -541,6 +541,18 @@ static int cdns_i2c_master_xfer(struct i2c_adapter
>> *adap, struct i2c_msg *msgs,
>>* processed with a repeated start.
>>*/
>> if (num > 1) {
>> + /*
>> + * This controller does not give completion interrupt after a
>> + * master receive transfer if HOLD bit is set (repeated start),
>> + * resulting in SW timeout. Hence, if a receive transfer is
>> + * followed by any other transfer, an error is returned
>> + * indicating that this sequence is not supported.
>> + */
>> + for (count = 0; count < num-1; count++) {
>> + if (msgs[count].flags & I2C_M_RD)
>> + dev_warn(adap->dev.parent, "No support for
>> repeated start when receive is followed by a transfer\n");
>> + return -EOPNOTSUPP;
>> + }
>> id->bus_hold_flag = 1;
>> reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
>> reg |= CDNS_I2C_CR_HOLD;
>
> Have you ever tested this code? There is a huge bug in it :(
I'm sorry. I'll send another patch with the braces for the if statement.
I seem to have tested with only the return statement and no dev_warn.
>
> Also, in the comment, use "message" instead of "transfer". One transfer
> consists of multiple messages.
>
> Maybe the warning can be simplified, too: "can't do repeated start after
> read messages".
>
I'll do that.
Regards,
Harini
--
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 / ACPI: Pick the first address if device has multiple
On Tue, Jan 13, 2015 at 05:48:29PM +0100, Wolfram Sang wrote: > On Tue, Jan 13, 2015 at 08:44:37AM -0800, Srinivas Pandruvada wrote: > > On Tue, 2015-01-13 at 16:50 +0100, Wolfram Sang wrote: > > > On Mon, Dec 29, 2014 at 03:48:48PM +0200, Mika Westerberg wrote: > > > > ACPI specification allows I2C devices with multiple addresses. The > > > > current > > > > implementation goes over all addresses and assigns the last one to the > > > > device. This is typically not the primary address of the device. > > > > > > > > Instead of doing that we assign the first address to the device and then > > > > let the driver handle rest of the addresses as it wishes. > > > > > > > > Signed-off-by: Mika Westerberg > > > > Cc: Srinivas Pandruvada > > > > > > Yes, seems better than what we do know. But maybe taking the lowest > > > address is a bit better heuristic than taking the first address? > > > Not sure, though... > > The problem in taking lowest is that in many cases in current devices, > > the lowest address may end being 0x0C, which is reserved address for > > SMBUS (ARA). This will require different handling. Unfortunately ACPI > > doesn't have a way to distinguish whether SMBUS support is desired or > > not. > > The other option is to skip all reserved addresses for SMBUS also and > > then create on the lowest. > > Well, this makes me think that Mika's approach is probably the sanest > one... Also I think it is more consistent that way. -- 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 / ACPI: Pick the first address if device has multiple
On Tue, Jan 13, 2015 at 08:44:37AM -0800, Srinivas Pandruvada wrote: > On Tue, 2015-01-13 at 16:50 +0100, Wolfram Sang wrote: > > On Mon, Dec 29, 2014 at 03:48:48PM +0200, Mika Westerberg wrote: > > > ACPI specification allows I2C devices with multiple addresses. The current > > > implementation goes over all addresses and assigns the last one to the > > > device. This is typically not the primary address of the device. > > > > > > Instead of doing that we assign the first address to the device and then > > > let the driver handle rest of the addresses as it wishes. > > > > > > Signed-off-by: Mika Westerberg > > > Cc: Srinivas Pandruvada > > > > Yes, seems better than what we do know. But maybe taking the lowest > > address is a bit better heuristic than taking the first address? > > Not sure, though... > The problem in taking lowest is that in many cases in current devices, > the lowest address may end being 0x0C, which is reserved address for > SMBUS (ARA). This will require different handling. Unfortunately ACPI > doesn't have a way to distinguish whether SMBUS support is desired or > not. > The other option is to skip all reserved addresses for SMBUS also and > then create on the lowest. Well, this makes me think that Mika's approach is probably the sanest one... signature.asc Description: Digital signature
Re: [PATCH] i2c / ACPI: Pick the first address if device has multiple
On Tue, 2015-01-13 at 16:50 +0100, Wolfram Sang wrote:
> On Mon, Dec 29, 2014 at 03:48:48PM +0200, Mika Westerberg wrote:
> > ACPI specification allows I2C devices with multiple addresses. The current
> > implementation goes over all addresses and assigns the last one to the
> > device. This is typically not the primary address of the device.
> >
> > Instead of doing that we assign the first address to the device and then
> > let the driver handle rest of the addresses as it wishes.
> >
> > Signed-off-by: Mika Westerberg
> > Cc: Srinivas Pandruvada
>
> Yes, seems better than what we do know. But maybe taking the lowest
> address is a bit better heuristic than taking the first address?
> Not sure, though...
The problem in taking lowest is that in many cases in current devices,
the lowest address may end being 0x0C, which is reserved address for
SMBUS (ARA). This will require different handling. Unfortunately ACPI
doesn't have a way to distinguish whether SMBUS support is desired or
not.
The other option is to skip all reserved addresses for SMBUS also and
then create on the lowest.
Thanks,
Srinivas
>
> > ---
> > drivers/i2c/i2c-core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index 39d25a8cb1ad..a06be43b7842 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -102,7 +102,7 @@ static int acpi_i2c_add_resource(struct acpi_resource
> > *ares, void *data)
> > struct acpi_resource_i2c_serialbus *sb;
> >
> > sb = &ares->data.i2c_serial_bus;
> > - if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
> > + if (!info->addr && sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
> > info->addr = sb->slave_address;
> > if (sb->access_mode == ACPI_I2C_10BIT_MODE)
> > info->flags |= I2C_CLIENT_TEN;
> > --
> > 2.1.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
--
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: designware: use {readl|writel}_relaxed instead of readl/writel
Hi Wolfram,
On Tue, Jan 13, 2015 at 03:36:54PM +0100, Wolfram Sang wrote:
> > The driver does not perform DMA, so it's safe to use the relaxed version.
> > From another side, the relaxed io accessor macros are available on all
> > architectures now, so we can use the relaxed versions instead.
>
> Can the designware core make use of DMA in theory?
According to the "DesignWare DW_apb_i2c Databook" version I have it can
(optionally).
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}ooO--U--Ooo{=
- bar...@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
--
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 / ACPI: Pick the first address if device has multiple
On Mon, Dec 29, 2014 at 03:48:48PM +0200, Mika Westerberg wrote:
> ACPI specification allows I2C devices with multiple addresses. The current
> implementation goes over all addresses and assigns the last one to the
> device. This is typically not the primary address of the device.
>
> Instead of doing that we assign the first address to the device and then
> let the driver handle rest of the addresses as it wishes.
>
> Signed-off-by: Mika Westerberg
> Cc: Srinivas Pandruvada
Yes, seems better than what we do know. But maybe taking the lowest
address is a bit better heuristic than taking the first address?
Not sure, though...
> ---
> drivers/i2c/i2c-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 39d25a8cb1ad..a06be43b7842 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -102,7 +102,7 @@ static int acpi_i2c_add_resource(struct acpi_resource
> *ares, void *data)
> struct acpi_resource_i2c_serialbus *sb;
>
> sb = &ares->data.i2c_serial_bus;
> - if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
> + if (!info->addr && sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
> info->addr = sb->slave_address;
> if (sb->access_mode == ACPI_I2C_10BIT_MODE)
> info->flags |= I2C_CLIENT_TEN;
> --
> 2.1.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
signature.asc
Description: Digital signature
Re: [PATCHv2 2/8] dt-bindings: use isil prefix for Intersil in I2C trivial-devices.txt
On Tue, Dec 16, 2014 at 10:19:53PM +0100, Arnaud Ebalard wrote:
>
> This patch fixes I2C trivial-devices.txt DT documentation file to
> reference isil (NASDAQ symbol and the most used prefix inside the
> kernel) for Intersil.
>
> It reverts 7c75c1d5e72b ("dt-bindings: Document deprecated device
> vendor name to fix related warning").
>
> Signed-off-by: Arnaud Ebalard
Applied to for-next, thanks!
signature.asc
Description: Digital signature
Re: [PATCH] [RFC] i2c: Don't wait for device release in i2c_del_adapter
On Mon, Jan 12, 2015 at 07:00:50PM +0200, Pantelis Antoniou wrote:
> Waiting for the device release method to be called when
> going through i2c_del_adapter is wrong and leads to deadlock
> when removing an i2c mux device.
Adding Jean to the list, maybe he knows more details from the past.
>
> For instance when using the OF i2c mux unitest removal we get this.
>
> [] (__schedule) from [] (schedule_timeout+0x18/0x198)
> [] (schedule_timeout) from [] (wait_for_common+0xf8/0x138)
> [] (wait_for_common) from [] (i2c_del_adapter+0x174/0x1cc)
> [] (i2c_del_adapter) from []
> (i2c_del_mux_adapter+0x48/0x60)
> [] (i2c_del_mux_adapter) from []
> (selftest_i2c_mux_remove+0x28/0x34)
> [] (selftest_i2c_mux_remove) from []
> (i2c_device_remove+0x34/0x70)
> [] (i2c_device_remove) from []
> (__device_release_driver+0x7c/0xc0)
> [] (__device_release_driver) from []
> (driver_detach+0x8c/0xb4)
> [] (driver_detach) from [] (bus_remove_driver+0x64/0x8c)
> [] (bus_remove_driver) from [] (of_selftest+0x1f84/0x20f0)
> [] (of_selftest) from [] (do_one_initcall+0x104/0x1b4)
> [] (do_one_initcall) from []
> (kernel_init_freeable+0x110/0x1d8)
> [] (kernel_init_freeable) from [] (kernel_init+0x8/0xe4)
> [] (kernel_init) from [] (ret_from_fork+0x14/0x3c)
> 2 locks held by swapper/0/1:
> #0: (&dev->mutex){..}, at: [] driver_detach+0x68/0xb4
> #1: (&dev->mutex){..}, at: [] driver_detach+0x78/0xb4
> Kernel panic - not syncing: hung_task: blocked tasks
> CPU: 0 PID: 16 Comm: khungtaskd Not tainted 3.19.0-rc4-00022-g261647c #443
> Hardware name: Generic AM33XX (Flattened Device Tree)
> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> [] (show_stack) from [] (dump_stack+0x70/0x8c)
> [] (dump_stack) from [] (panic+0x88/0x1f0)
> [] (panic) from [] (watchdog+0x2d4/0x350)
> [] (watchdog) from [] (kthread+0xd8/0xec)
> [] (kthread) from [] (ret_from_fork+0x14/0x3c)
>
> The device release method is never called and we hang waiting for it.
>
> This patch is marked as an [RFC] since the original code seems to
> really want to protect against a race from sysfs accessors, which
> I don't see how it could be a problem.
>
> Signed-off-by: Pantelis Antoniou
> ---
> drivers/i2c/i2c-core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 39d25a8..e020a16 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1185,7 +1185,7 @@ EXPORT_SYMBOL_GPL(i2c_new_dummy);
> static void i2c_adapter_dev_release(struct device *dev)
> {
> struct i2c_adapter *adap = to_i2c_adapter(dev);
> - complete(&adap->dev_released);
> + /* XXX complete(&adap->dev_released); */
> }
>
> /*
> @@ -1797,11 +1797,11 @@ void i2c_del_adapter(struct i2c_adapter *adap)
> dev_dbg(&adap->dev, "adapter [%s] unregistered\n", adap->name);
>
> /* clean up the sysfs representation */
> - init_completion(&adap->dev_released);
> + /* XXX init_completion(&adap->dev_released); */
> device_unregister(&adap->dev);
>
> /* wait for sysfs to drop all references */
> - wait_for_completion(&adap->dev_released);
> + /* XXX wait_for_completion(&adap->dev_released); */
>
> /* free bus id */
> mutex_lock(&core_lock);
> --
> 1.7.12
>
signature.asc
Description: Digital signature
Re: [PATCH v3 0/2] ARM: mediatek: Add driver for Mediatek I2C controller
> I think the quirks can describe our HW limitation without issue. > We are porting our i2c driver base on that. Will let you know the test > result. Great news, thanks for the update :) signature.asc Description: Digital signature
Re: [PATCH] at91: i2c-at91: improve time-out handling
On Wed, Jan 07, 2015 at 11:31:14AM +0100, Ludovic Desroches wrote:
> Hi Douglas,
>
> On Thu, Jan 01, 2015 at 01:02:13PM -0500, Douglas Gilbert wrote:
> > With lk 3.19.0-rc2 and a at91sam9g25 (9x5) based system I
> > connected a NXP SC16IS750 I2C to serial bridge. After
> > routing the 750's IRQ back to the sc16is7xx driver and some
> > simple successful test, it was time for some intense testing:
> > Tx looped back to Rx on the 750, open picocom on /dev/ttySC0
> > at 38400, and use hexdump to blast a binary file (in hex) at
> > ttySC0. The I2C SCL speed was 200,000 Hz.
> >
> > It worked as expected for a few seconds then it wedged the
> > I2C bus. That was repeatable. In the cases that I checked SCL
> > was high, SDA was low (driven by _both_ the G25's macrocell
> > and the 750!!) and IRQ was active (low). This patch stopped
> > the G25 macrocell from driving SDA low in the above wedge
> > (and stopped copious error reports going to the log). I was
> > surprised that a NXP I2C chip got into this situation, IMO
> > SDA on a slave should have a driven low timeout. IMO all
> > I2C master drivers should have provision to drive a gpio
> > connected to a (or all the) slave's RESET line(s).
> >
> >
> > ChangeLog:
> >when handling an I2C bus time-out, first clean-up the
> >DMA transfer, then do an I2C macrocell software reset
> >and restore some registers, including the interrupt
> >mask
> >
>
> I am wondering why you need to call at91_twi_irq_save() and
> at91_twi_irq_restore(). The interrupts enabled in the driver are
> AT91_TWI_TXCOMP, AT91_TWI_RXRDY and AT91_TWI_TXRDY and they are managed
> in at91_do_twi_transfer() so they would be set correctly for the next
> transfer.
Douglas, any more info you could provide?
>
> Regards
>
> Ludovic
>
> > Signed-off-by: Douglas Gilbert
>
> > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > index 636fd2e..4d78708 100644
> > --- a/drivers/i2c/busses/i2c-at91.c
> > +++ b/drivers/i2c/busses/i2c-at91.c
> > @@ -382,6 +382,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev
> > *dev)
> > {
> > int ret;
> > bool has_unre_flag = dev->pdata->has_unre_flag;
> > + bool timed_out = false;
> >
> > dev_dbg(dev->dev, "transfer: %s %d bytes.\n",
> > (dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len);
> > @@ -440,7 +441,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev
> > *dev)
> > dev->adapter.timeout);
> > if (ret == 0) {
> > dev_err(dev->dev, "controller timed out\n");
> > - at91_init_twi_bus(dev);
> > + timed_out = true;
> > ret = -ETIMEDOUT;
> > goto error;
> > }
> > @@ -471,6 +472,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev
> > *dev)
> >
> > error:
> > at91_twi_dma_cleanup(dev);
> > + if (timed_out) {
> > + at91_twi_irq_save(dev);
> > + at91_init_twi_bus(dev);
> > + at91_twi_irq_restore(dev);
> > + }
> > return ret;
> > }
> >
>
> --
> 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
signature.asc
Description: Digital signature
Re: [PATCH v2 -next] i2c: imx: fix handling of wait_for_completion_timeout result
On Sat, Dec 27, 2014 at 08:33:53AM -0500, Nicholas Mc Guire wrote: > wait_for_completion_timeout does not return negative values so > "result" handling here should be simplified to cover the actually > possible cases only. > > patch was only compile tested for imx_v6_v7_defconfig > > V2 spellchecked this time and proper labeling as >suggested by Sedat Dilek > > patch is against linux-next 3.19.0-rc1 -next-20141226 The last two paragraphs should have been below "---" but other than that, all is OK. > > Signed-off-by: Nicholas Mc Guire Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH v3 0/2] ARM: mediatek: Add driver for Mediatek I2C controller
On Tue, 2015-01-13 at 10:57 +0100, Wolfram Sang wrote: > On Tue, Jan 06, 2015 at 02:37:53PM +0100, Wolfram Sang wrote: > > > > > We've started upstream work for MT8173[1]. > > > > > > We've fixed these issues for new SoC, and we believe it is fully I2C > > > compatible now. We'll add mt8173 support to this driver, so this driver > > > will support both fully I2C compatible SoC and the current one. > > > > From what you tell, I'd rather add the MT8173 support incrementally. > > Let's get first the limited support properly implemented, then add > > another controller. > > > > I'll send out my draft for describing the quirks later today. > > What do you think about my two proposals? I think the quirks can describe our HW limitation without issue. We are porting our i2c driver base on that. Will let you know the test result. Thanks Joe.C -- 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: Mark instantiated device nodes with OF_POPULATE
On Mon, Jan 12, 2015 at 07:01:34PM +0200, Pantelis Antoniou wrote: > Mark (and unmark) device nodes with the POPULATE flag as appropriate. > This is required to avoid multi probing when using I2C and device > overlays containing a mux. > > Signed-off-by: Pantelis Antoniou Applied to for-current, thanks! signature.asc Description: Digital signature
Re: [PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel
On Thu, Dec 11, 2014 at 02:26:41PM +0800, Jisheng Zhang wrote: > readl/writel is too expensive especially on Cortex A9 w/ outer L2 cache. > This introduces i2c read/write errors on Marvell BG2/BG2Q SoCs when there > are heavy L2 cache maintenance operations at the same time. Reading this again, I got a question: Really read/write errors? I would think that there is a performance penalty because of the memory barriers. But errors? > The driver does not perform DMA, so it's safe to use the relaxed version. > From another side, the relaxed io accessor macros are available on all > architectures now, so we can use the relaxed versions instead. Can the designware core make use of DMA in theory? signature.asc Description: Digital signature
Re: [PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel
On Tue, Jan 13, 2015 at 12:52:05PM +0100, Wolfram Sang wrote: > On Fri, Dec 19, 2014 at 10:43:15AM +0800, Jisheng Zhang wrote: > > Dear all, > > > > Is there any issue I need to resolve so that the patch can be merged? > > Adding Mika to the loop. He uses the driver a lot (and knows other > people who do)... I don't see problems with this patch. On x86 we have readl_relaxed() the same as readl() so this patch does not change anything there. Feel free to add, Acked-by: Mika Westerberg -- 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: exynos5: Move initialization code to subsys_initcall()
On 12/01/15 10:43, Joonyoung Shim wrote:
> +Cc Tomi Valkeinen,
>
> Hi Uwe,
>
> On 01/12/2015 04:50 PM, Uwe Kleine-König wrote:
>> Hello,
>>
>> On Mon, Jan 12, 2015 at 11:53:02AM +0900, Joonyoung Shim wrote:
>>> This is required in order to ensure that core system devices such as
>>> voltage regulators attached via I2C are available early in boot.
>> Deferred probing isn't an option? If so I suggest adding the reasoning
>> in a comment to stop the next person converting it to that.
>> (And if not, please fix accordingly to use deferred probing.)
>>
>
> I couldn't get penguin logo since the commit 92b004d ("video/logo:
> prevent use of logos after they have been freed") and i just tried old
> way because i missed the flow to move to deferred probing.
>
> Fb driver probe seems to be ran between fb_logo_late_init late_initcall
> and the freeing of the logos.
>
> Any ideas?
Thierry mentioned on IRC that he encountered the same issue. And I
encountered it also.
So... I'd rather not revert the fix, as it's quite a nasty one, and it
happens while console lock is held, so it looks like the machine just
froze. But I don't know how it could be improved with the current kernel.
We could make the logos non-initdata, but I don't much like that option.
Or we could perhaps implement some new way to catch the freeing of initdata.
Any other ideas?
Tomi
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] i2c: Mark instantiated device nodes with OF_POPULATE
Hi Wolfram, > On Jan 13, 2015, at 14:05 , Wolfram Sang wrote: > > On Mon, Jan 12, 2015 at 07:01:34PM +0200, Pantelis Antoniou wrote: >> Mark (and unmark) device nodes with the POPULATE flag as appropriate. >> This is required to avoid multi probing when using I2C and device >> overlays containing a mux. >> >> Signed-off-by: Pantelis Antoniou > > Looks good. I'd think this is 3.19 material? > It’s a obvious bug fix. If you can queue this for 3.19 that’d be great. Regards — Pantelis -- 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: Mark instantiated device nodes with OF_POPULATE
On Mon, Jan 12, 2015 at 07:01:34PM +0200, Pantelis Antoniou wrote: > Mark (and unmark) device nodes with the POPULATE flag as appropriate. > This is required to avoid multi probing when using I2C and device > overlays containing a mux. > > Signed-off-by: Pantelis Antoniou Looks good. I'd think this is 3.19 material? signature.asc Description: Digital signature
Re: [PATCH] i2c: designware: use {readl|writel}_relaxed instead of readl/writel
On Fri, Dec 19, 2014 at 10:43:15AM +0800, Jisheng Zhang wrote:
> Dear all,
>
> Is there any issue I need to resolve so that the patch can be merged?
Adding Mika to the loop. He uses the driver a lot (and knows other
people who do)...
> On Wed, 10 Dec 2014 22:26:41 -0800
> Jisheng Zhang wrote:
>
> > readl/writel is too expensive especially on Cortex A9 w/ outer L2 cache.
> > This introduces i2c read/write errors on Marvell BG2/BG2Q SoCs when there
> > are heavy L2 cache maintenance operations at the same time.
> >
> > The driver does not perform DMA, so it's safe to use the relaxed version.
> > From another side, the relaxed io accessor macros are available on all
> > architectures now, so we can use the relaxed versions instead.
> >
> > Signed-off-by: Jisheng Zhang
> > ---
> > drivers/i2c/busses/i2c-designware-core.c | 12 ++--
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c
> > b/drivers/i2c/busses/i2c-designware-core.c index 23628b7..e279948 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -170,10 +170,10 @@ u32 dw_readl(struct dw_i2c_dev *dev, int offset)
> > u32 value;
> >
> > if (dev->accessor_flags & ACCESS_16BIT)
> > - value = readw(dev->base + offset) |
> > - (readw(dev->base + offset + 2) << 16);
> > + value = readw_relaxed(dev->base + offset) |
> > + (readw_relaxed(dev->base + offset + 2) << 16);
> > else
> > - value = readl(dev->base + offset);
> > + value = readl_relaxed(dev->base + offset);
> >
> > if (dev->accessor_flags & ACCESS_SWAP)
> > return swab32(value);
> > @@ -187,10 +187,10 @@ void dw_writel(struct dw_i2c_dev *dev, u32 b, int
> > offset) b = swab32(b);
> >
> > if (dev->accessor_flags & ACCESS_16BIT) {
> > - writew((u16)b, dev->base + offset);
> > - writew((u16)(b >> 16), dev->base + offset + 2);
> > + writew_relaxed((u16)b, dev->base + offset);
> > + writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
> > } else {
> > - writel(b, dev->base + offset);
> > + writel_relaxed(b, dev->base + offset);
> > }
> > }
> >
>
signature.asc
Description: Digital signature
Re: [PATCH v3] i2c: rk3x: Account for repeated start time requirement
On Thu, Dec 18, 2014 at 09:44:07AM -0800, Doug Anderson wrote: > On Rockchip I2C the controller drops SDA low slightly too soon to meet > the "repeated start" requirements. > > From my own experimentation over a number of rates: > - controller appears to drop SDA at .875x (7/8) programmed clk high. > - controller appears to keep SCL high for 2x programmed clk high. > > The first rule isn't enough to meet tSU;STA requirements in > Standard-mode on the system I tested on. The second rule is probably > enough to meet tHD;STA requirements in nearly all cases (especially > after accounting for the first), but it doesn't hurt to account for it > anyway just in case. > > Even though the repeated start requirement only need to be accounted > for during a small part of the transfer, we'll adjust the timings for > the whole transfer to meet it. I believe that adjusting the timings > in just the right place to switch things up for repeated start would > require several extra interrupts and that doesn't seem terribly worth > it. > > With this change and worst case rise/fall times, I see 100kHz i2c > going to ~85kHz. With slightly optimized rise/fall (800ns / 50ns) I > see i2c going to ~89kHz. Fast-mode isn't affected much because > tSU;STA is shorter relative to tHD;STA there. > > As part of this change we needed to account for the SDA falling time. > The specification indicates that this should be the same, but we'll > follow Designware's lead and add a binding. Note that we deviate from > Designware and assign the default SDA falling time to be the same as > the SCL falling time, which is incredibly likely. > > Signed-off-by: Doug Anderson Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH v5] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C specification
On Thu, Dec 11, 2014 at 07:02:40PM +0800, Addy Ke wrote: > The number of clock cycles to be written into the CLKDIV register > that determines the I2C clk high phase includes the rise time. > So to meet the timing requirements defined in the I2C specification > which defines the minimal time SCL has to be high, the rise time > has to taken into account. The same applies to the low phase with > falling time. > > In my test on RK3288-Pink2 board, which is not an upstream board yet, > if external pull-up resistor is 4.7K, rise_ns is about 700ns. > So the measured high_ns is about 3900ns, which is less than 4000ns > (the minimum high_ns in I2C specification for Standard-mode). > > To fix this bug min_low_ns should include fall time and min_high_ns > should include rise time. > > This patch merged the patch from chromium project which can get the > rise and fall times for signals from the device tree. This allows us > to more accurately calculate timings. see: > https://chromium-review.googlesource.com/#/c/232774/ > > Signed-off-by: Addy Ke Applied to for-next, thanks! I fixed the typo Doug mentioned. Please send new patches as seperate threads, not as a reply to the old patch. signature.asc Description: Digital signature
Re: [PATCH v5 2/2] i2c: cadence: Check for errata condition involving master receive
> @@ -541,6 +541,18 @@ static int cdns_i2c_master_xfer(struct i2c_adapter
> *adap, struct i2c_msg *msgs,
>* processed with a repeated start.
>*/
> if (num > 1) {
> + /*
> + * This controller does not give completion interrupt after a
> + * master receive transfer if HOLD bit is set (repeated start),
> + * resulting in SW timeout. Hence, if a receive transfer is
> + * followed by any other transfer, an error is returned
> + * indicating that this sequence is not supported.
> + */
> + for (count = 0; count < num-1; count++) {
> + if (msgs[count].flags & I2C_M_RD)
> + dev_warn(adap->dev.parent, "No support for
> repeated start when receive is followed by a transfer\n");
> + return -EOPNOTSUPP;
> + }
> id->bus_hold_flag = 1;
> reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
> reg |= CDNS_I2C_CR_HOLD;
Have you ever tested this code? There is a huge bug in it :(
Also, in the comment, use "message" instead of "transfer". One transfer
consists of multiple messages.
Maybe the warning can be simplified, too: "can't do repeated start after
read messages".
signature.asc
Description: Digital signature
Re: [PATCH v5 1/2] i2c: cadence: Handle > 252 byte transfers
On Fri, Dec 12, 2014 at 09:48:26AM +0530, Harini Katakam wrote:
> The I2C controller sends a NACK to the slave when transfer size register
> reaches zero, irrespective of the hold bit. So, in order to handle transfers
> greater than 252 bytes, the transfer size register has to be maintained at a
> value >= 1. This patch implements the same.
> The interrupt status is cleared at the beginning of the isr instead of
> the end, to avoid missing any interrupts.
>
> Signed-off-by: Harini Katakam
Applied to for-next, thanks!
> @@ -333,10 +359,11 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>* receive if it is less than transfer size and transfer size if
>* it is more. Enable the interrupts.
>*/
> - if (id->recv_count > CDNS_I2C_TRANSFER_SIZE)
> + if (id->recv_count > CDNS_I2C_TRANSFER_SIZE) {
> cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE,
> CDNS_I2C_XFER_SIZE_OFFSET);
> - else
> + id->curr_recv_count = CDNS_I2C_TRANSFER_SIZE;
> + } else
> cdns_i2c_writereg(id->recv_count, CDNS_I2C_XFER_SIZE_OFFSET);
else branch must have braces, too! I fixed that.
signature.asc
Description: Digital signature
Re: [PATCH 2/4] DT: i2c: Add devices handled by the da9063 MFD driver
On Tue, Dec 09, 2014 at 12:22:47PM +0100, Geert Uytterhoeven wrote: > This allows checkpatch to validate more DTSes. > > Signed-off-by: Geert Uytterhoeven > Cc: devicet...@vger.kernel.org Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [RFC 0/5] i2c: omap: new fixes 2
On Wed, Dec 03, 2014 at 06:33:57PM +0400, Alexander Kochetkov wrote: > This pacth series intended for fixing problem reported > by Tony Lindgren here[1] > > One of first four patched could fix the problem. > Last patch provide event trace so I could resolve problem. > It could be applied using 'git am' or 'patch -p1 ...' > > Patches are rebased on branch 'i2c/for-next' of > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux > (6e79807443cba7397cd855ed29d6faba51d4c893) > > Tony, could you check, does the series fix the problem reported[1]? > If yes, could you bisect and point commit that solve. > If no, could you provide trace output (with or without the patches > from series). > If no, could you check does i2c-omap.c from commit > ca1f8da9ac5ce6e63d8f6933f83fabc1f3f961f4. > (the commit before my changes to kernel) work for you? > > Thank you. > > Regards, > Alexander. Thanks for working hard on this driver. I'll mark these patches as RFC, since they did not get any additional tags from other people. If they are intended for upstream, please resend a rebased version. Wolfram signature.asc Description: Digital signature
Re: [RFC 0/2] i2c: omap: new fixes for driver
On Sun, Nov 30, 2014 at 01:00:01AM +0400, Alexander Kochetkov wrote: > The first patch fix i2c-omap driver for omap2420, found by code review and > later reported by Tony Lindgren here[1]. > Candidate for stable? > > The second patch unhide the reson of system lockup. > > The patch is rebased on branch 'i2c/for-next' of > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux > (6e79807443cba7397cd855ed29d6faba51d4c893) > > Tony, could you check, what the patches fix the problem reported[1]? > Kevin, could you run tests for patches on all omap boards? > > Regards, > Alexander. Thanks for working hard on this driver. I'll mark these patches as RFC, since they did not get any additional tags from other people. If they are intended for upstream, please resend a rebased version. Wolfram signature.asc Description: Digital signature
Re: [PATCH v3 0/2] ARM: mediatek: Add driver for Mediatek I2C controller
On Tue, Jan 06, 2015 at 02:37:53PM +0100, Wolfram Sang wrote: > > > We've started upstream work for MT8173[1]. > > > > We've fixed these issues for new SoC, and we believe it is fully I2C > > compatible now. We'll add mt8173 support to this driver, so this driver > > will support both fully I2C compatible SoC and the current one. > > From what you tell, I'd rather add the MT8173 support incrementally. > Let's get first the limited support properly implemented, then add > another controller. > > I'll send out my draft for describing the quirks later today. What do you think about my two proposals? signature.asc Description: Digital signature
Re: [PATCH V3 1/2] i2c-designware: Add i2c bus locking support
Hi Dave, > Timely reply. Around i2c_dw_init(), yes. I just discovered this as the source > of a recent hang that's occuring in the loop in __i2c_dw_enable(). > The hange occurs very infrequently and only, so far, when not built in. A > block around i2c_dw_disable_int() would make sense as well as a precaution. Any news on this or on a V4 of this series? Thanks, Wolfram signature.asc Description: Digital signature
Re: [PATCH] i2c: pmcmsp: remove dead code
On Thu, Jan 08, 2015 at 08:19:00PM +0100, Wolfram Sang wrote: > CPPCHECK rightfully says: > > drivers/i2c/busses/i2c-pmcmsp.c:151: style: The function > 'pmcmsptwi_reg_to_clock' is never used. > > Signed-off-by: Wolfram Sang Applied to for-next, thanks! signature.asc Description: Digital signature
