Re: [PATCH v2 3/4] i2c: omap: don't reset controller if Arbitration Lost detected
On Sat, Nov 22, 2014 at 02:51:47AM +0400, Alexander Kochetkov wrote:
> Arbitration Lost is an expected situation in a multimaster
> environment. I2C controller (IP) correctly detect and report AL.
>
> The only one visible reason for reseting IP in the AL case is
> to avoid advisory 1.94 (omap3) and errata i595 (omap4): "I2C:
> After an Arbitration is Lost the Module Incorrectly Starts
> the Next Transfer".
>
> Errata workaround states: "The MST and STT bits inside I2C_CON
> should be set to 1 at the same moment (avoid setting the MST bit
> to 1 while STT = 0)." The driver never set MST and STT bits
> separately and doesn't create condition for errata. So the reset
> is not necessary.
>
> Also corrected return value for AL to -EAGAIN.
>
> Tested on Beagleboard XM C.
>
> Signed-off-by: Alexander Kochetkov
you could have kept my tested-by and reviewed-by:
Tested-by: Felipe Balbi
Reviewed-by: Felipe Balbi
> On 21.10.2014 21:11, Wolfram Sang wrote:
> > The errno for AL is -EAGAIN. Curly braces are not needed.
>
> Thank you, Wolfram, fixed.
>
> drivers/i2c/busses/i2c-omap.c |6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 3ffb9c0..02da567 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -707,13 +707,15 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> return 0;
>
> /* We have an error */
> - if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> - OMAP_I2C_STAT_XUDF)) {
> + if (dev->cmd_err & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_XUDF)) {
> omap_i2c_reset(dev);
> __omap_i2c_init(dev);
> return -EIO;
> }
>
> + if (dev->cmd_err & OMAP_I2C_STAT_AL)
> + return -EAGAIN;
> +
> if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
> if (msg->flags & I2C_M_IGNORE_NAK)
> return 0;
> --
> 1.7.9.5
>
--
balbi
signature.asc
Description: Digital signature
RE: [PATCH v4 1/2] i2c: s3c2410: Handle i2c sys_cfg register in i2c driver
Wolfram Sang wrote: > Hi Wolfram, > > >I usually don't take DTS patches. They should go via arm-soc. Please say > > >so if there are reasons I should take them. > > > > I CC'ed to you because same patch contains changes in i2c driver. > > Yes, those should absolutely go via my I2C tree. You need to make a > seperate patch out of the dts changes which then also should go via > samsung-soc, unless Kukjin says he really wants to go the via I2C. But I > guess the latter will just create merge conflicts. Hmm...I think, Pankaj needs to submit separated patches 1) driver change, 2) dt change and then 3) remove change. And 2nd and 3rd changes should be handed in Samsung tree together after landing 1) change in -next. Of course, 1) change should be handled in i2c tree ;) Thanks, Kukjin -- 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/2] i2c: s3c2410: Handle i2c sys_cfg register in i2c driver
Pankaj Dubey wrote: > > On Friday 21 November 2014 12:55 PM, Wolfram Sang wrote: > > On Thu, Oct 30, 2014 at 01:34:29PM +0530, Pankaj Dubey wrote: > >> Let's handle i2c interrupt re-configuration in i2c driver. This will > >> help us in removing some soc specific checks from machine files and > >> will help in removing static iomapping of SYS register in exynos.c > >> > >> Since only Exynos5250, and Exynos5420 has i2c nodes in DT, added syscon > >> based phandle to i2c device nodes of respective SoC DT files. > >> Well...actually there are 4ch i2c in exynos5410 and exynos5800 as well, i2c nodes are not added in dt files though. > >> Also handle saving and restoring of SYS_I2C_CFG register during > >> suspend and resume of i2c driver. > >> > >> CC: Rob Herring > >> CC: Randy Dunlap > >> CC: Wolfram Sang > >> CC: Russell King > >> CC: devicet...@vger.kernel.org > >> CC: linux-...@vger.kernel.org > >> CC: linux-i2c@vger.kernel.org > >> Signed-off-by: Pankaj Dubey > >> --- > >> .../devicetree/bindings/i2c/i2c-s3c2410.txt|1 + > >> arch/arm/boot/dts/exynos5250.dtsi |4 +++ > >> arch/arm/boot/dts/exynos5420.dtsi |4 +++ > > > > I usually don't take DTS patches. They should go via arm-soc. Please say > > so if there are reasons I should take them. > > I CC'ed to you because same patch contains changes in i2c driver. > I am not very sure via which tree this should go. May be I can ask > samsung SoC maintainer Kukjin to look into this, as patch 2/2 has > changes in mach-exynos which should go via Kukjin's tree. > I'll reply on other email. Thanks, Kukjin -- 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: omap: fix i207 errata handling
21 нояб. 2014 г., в 3:29, Alexander Kochetkov написал(а): >> >> Found by code review. Real impact haven't seen. >> Tested on Beagleboard XM C. > > Does anybody know the "certain rare conditions" when RDR errata appears? > I tested without luck (Beagleboard XM C). Spent half a day trying to catch the errata without luck. Tried to simulate noise on the bus in hope it may happen. Tried to run with OMAP_I2C_FLAG_NO_FIFO flag. What a mystery errata. Hiding. Anyway, thanks! Have a nice weekend! -- 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: omap: fix i207 errata handling
21 нояб. 2014 г., в 19:08, Felipe Balbi написал(а): > Tested on BBB and AM437x Starter Kit > > Tested-by: Felipe Balbi > Reviewed-by: Felipe Balbi 21 нояб. 2014 г., в 0:10, Aaro Koskinen написал(а): > I could not see any breakage or anything wrong on OMAP2 & OMAP3. > On OMAP1 I don't have anything on the OMAP I2C bus, so cannot really > test anything there. > > Tested-by: Aaro Koskinen 21 нояб. 2014 г., в 21:11, Wolfram Sang написал(а): > The errno for AL is -EAGAIN. Curly braces are not needed. Guys, I really appreciate you help. So much testing and review. I could not have done one. Thank you! Alexander. -- 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 3/4] i2c: omap: don't reset controller if Arbitration Lost detected
Arbitration Lost is an expected situation in a multimaster
environment. I2C controller (IP) correctly detect and report AL.
The only one visible reason for reseting IP in the AL case is
to avoid advisory 1.94 (omap3) and errata i595 (omap4): "I2C:
After an Arbitration is Lost the Module Incorrectly Starts
the Next Transfer".
Errata workaround states: "The MST and STT bits inside I2C_CON
should be set to 1 at the same moment (avoid setting the MST bit
to 1 while STT = 0)." The driver never set MST and STT bits
separately and doesn't create condition for errata. So the reset
is not necessary.
Also corrected return value for AL to -EAGAIN.
Tested on Beagleboard XM C.
Signed-off-by: Alexander Kochetkov
---
On 21.10.2014 21:11, Wolfram Sang wrote:
> The errno for AL is -EAGAIN. Curly braces are not needed.
Thank you, Wolfram, fixed.
drivers/i2c/busses/i2c-omap.c |6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 3ffb9c0..02da567 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -707,13 +707,15 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
return 0;
/* We have an error */
- if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
- OMAP_I2C_STAT_XUDF)) {
+ if (dev->cmd_err & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_XUDF)) {
omap_i2c_reset(dev);
__omap_i2c_init(dev);
return -EIO;
}
+ if (dev->cmd_err & OMAP_I2C_STAT_AL)
+ return -EAGAIN;
+
if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
if (msg->flags & I2C_M_IGNORE_NAK)
return 0;
--
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: [4/5] i2c: davinci: use bus recovery infrastructure
Hi Uwe,
On 11/21/2014 09:07 PM, Uwe Kleine-König wrote:
> On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
>> This patch converts Davinci I2C driver to use I2C bus recovery
>> infrastructure, introduced by commit 5f9296ba21b3 ("i2c: Add
>> bus recovery infrastructure").
>>
>> The i2c_bus_recovery_info is configured for Davinci I2C adapter
>> only in case if scl_pin is provided in Platform data at least.
> s/Platform/platform/
>
>>
>> Because the controller must be held in reset while doing so, the
> s/Because/As/
>
>> recovery routine must re-init the controller. Since this was already
>> being done after each call to i2c_recover_bus, move those calls into
>> the recovery_prepare/unprepare routines and as well.
>>
>> CC: Sekhar Nori
>> CC: Kevin Hilman
>> CC: Santosh Shilimkar
>> CC: Murali Karicheri
>> Signed-off-by: Grygorii Strashko
>> ---
>> drivers/i2c/busses/i2c-davinci.c | 76
>> ++--
>> 1 file changed, 35 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c
>> b/drivers/i2c/busses/i2c-davinci.c
>> index 2cef115..db2a2cd 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct
>> davinci_i2c_dev *i2c_dev, int reg)
>> return readw_relaxed(i2c_dev->base + reg);
>> }
>>
>> -/* Generate a pulse on the i2c clock pin. */
>> -static void davinci_i2c_clock_pulse(unsigned int scl_pin)
>> -{
>> -u16 i;
>> -
>> -if (scl_pin) {
>> -/* Send high and low on the SCL line */
>> -for (i = 0; i < 9; i++) {
>> -gpio_set_value(scl_pin, 0);
>> -udelay(20);
>> -gpio_set_value(scl_pin, 1);
>> -udelay(20);
>> -}
>> -}
>> -}
>> -
>> -/* This routine does i2c bus recovery as specified in the
>> - * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
>> - */
>> -static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
>> -{
>> -u32 flag = 0;
>> -struct davinci_i2c_platform_data *pdata = dev->pdata;
>> -
>> -dev_err(dev->dev, "initiating i2c bus recovery\n");
>> -/* Send NACK to the slave */
>> -flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> -flag |= DAVINCI_I2C_MDR_NACK;
>> -/* write the data into mode register */
>> -davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
>> -davinci_i2c_clock_pulse(pdata->scl_pin);
>> -/* Send STOP */
>> -flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> -flag |= DAVINCI_I2C_MDR_STP;
>> -davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
>> -}
>> -
>> static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
>> int val)
>> {
>> @@ -266,6 +229,33 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>> return 0;
>> }
>>
>> +/* This routine does i2c bus recovery as specified in the
>> + * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
>> + */
> This comment is wrong. The actual bus clear is implemented by
> i2c_generic_gpio_recovery. Also while touching this comment, convert it
> to the usual format with /* on its own line. (The file in question has
> already both types of comment, so consistency is not a reason to keep it
> as is.)
It has been just copy-pasted, but ok.
I'll change this comment as following:
/*
* This routine does i2c bus recovery by using i2c_generic_gpio_recovery
* which is provided by I2C Bus recovery infrastructure.
*/
Is it ok?
>
> Even though I remember that I reviewed this bus recovery patch (that
> resulted in 5f9296ba21b3) back then, I don't remember why it was split
> in prepare + recover + unprepare. But that is unrelated to this patch.
>
>> +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
>> +{
>> +struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
>> +
>> +dev_err(dev->dev, "initiating i2c bus recovery\n");
>> +/* Disable interrupts */
>> +davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
>> +
>> +/* put I2C into reset */
>> +davinci_i2c_reset_ctrl(dev, 0);
>> +}
>> +
>> +static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
>> +{
>> +struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
>> +
>> +i2c_davinci_init(dev);
>> +}
>> +
>> +static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
> I'd call this only davinci_i2c_recovery_info.
No. Pls, see next patch.
>
>> +.recover_bus = i2c_generic_gpio_recovery,
>> +.prepare_recovery = davinci_i2c_prepare_recovery,
>> +.unprepare_recovery = davinci_i2c_unprepare_recovery,
>> +};
> new line here please
:) Ok.
Fixed in next patch.
>
>> /*
>>* Waiting for bus not busy
>>*/
>> @@ -286,8 +276,7 @@ static int i2c_davinci_wait_bus_not_busy(struct
>> davinci_i2c_dev *dev,
>>
Re: [4/5] i2c: davinci: use bus recovery infrastructure
On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
> This patch converts Davinci I2C driver to use I2C bus recovery
> infrastructure, introduced by commit 5f9296ba21b3 ("i2c: Add
> bus recovery infrastructure").
>
> The i2c_bus_recovery_info is configured for Davinci I2C adapter
> only in case if scl_pin is provided in Platform data at least.
s/Platform/platform/
>
> Because the controller must be held in reset while doing so, the
s/Because/As/
> recovery routine must re-init the controller. Since this was already
> being done after each call to i2c_recover_bus, move those calls into
> the recovery_prepare/unprepare routines and as well.
>
> CC: Sekhar Nori
> CC: Kevin Hilman
> CC: Santosh Shilimkar
> CC: Murali Karicheri
> Signed-off-by: Grygorii Strashko
> ---
> drivers/i2c/busses/i2c-davinci.c | 76
> ++--
> 1 file changed, 35 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c
> b/drivers/i2c/busses/i2c-davinci.c
> index 2cef115..db2a2cd 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct
> davinci_i2c_dev *i2c_dev, int reg)
> return readw_relaxed(i2c_dev->base + reg);
> }
>
> -/* Generate a pulse on the i2c clock pin. */
> -static void davinci_i2c_clock_pulse(unsigned int scl_pin)
> -{
> - u16 i;
> -
> - if (scl_pin) {
> - /* Send high and low on the SCL line */
> - for (i = 0; i < 9; i++) {
> - gpio_set_value(scl_pin, 0);
> - udelay(20);
> - gpio_set_value(scl_pin, 1);
> - udelay(20);
> - }
> - }
> -}
> -
> -/* This routine does i2c bus recovery as specified in the
> - * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
> - */
> -static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
> -{
> - u32 flag = 0;
> - struct davinci_i2c_platform_data *pdata = dev->pdata;
> -
> - dev_err(dev->dev, "initiating i2c bus recovery\n");
> - /* Send NACK to the slave */
> - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> - flag |= DAVINCI_I2C_MDR_NACK;
> - /* write the data into mode register */
> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> - davinci_i2c_clock_pulse(pdata->scl_pin);
> - /* Send STOP */
> - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> - flag |= DAVINCI_I2C_MDR_STP;
> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> -}
> -
> static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
> int val)
> {
> @@ -266,6 +229,33 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
> return 0;
> }
>
> +/* This routine does i2c bus recovery as specified in the
> + * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
> + */
This comment is wrong. The actual bus clear is implemented by
i2c_generic_gpio_recovery. Also while touching this comment, convert it
to the usual format with /* on its own line. (The file in question has
already both types of comment, so consistency is not a reason to keep it
as is.)
Even though I remember that I reviewed this bus recovery patch (that
resulted in 5f9296ba21b3) back then, I don't remember why it was split
in prepare + recover + unprepare. But that is unrelated to this patch.
> +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
> +{
> + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> + dev_err(dev->dev, "initiating i2c bus recovery\n");
> + /* Disable interrupts */
> + davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
> +
> + /* put I2C into reset */
> + davinci_i2c_reset_ctrl(dev, 0);
> +}
> +
> +static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
> +{
> + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> + i2c_davinci_init(dev);
> +}
> +
> +static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
I'd call this only davinci_i2c_recovery_info.
> + .recover_bus = i2c_generic_gpio_recovery,
> + .prepare_recovery = davinci_i2c_prepare_recovery,
> + .unprepare_recovery = davinci_i2c_unprepare_recovery,
> +};
new line here please
> /*
> * Waiting for bus not busy
> */
> @@ -286,8 +276,7 @@ static int i2c_davinci_wait_bus_not_busy(struct
> davinci_i2c_dev *dev,
> return -ETIMEDOUT;
> } else {
> to_cnt = 0;
> - davinci_i2c_recover_bus(dev);
> - i2c_davinci_init(dev);
> + i2c_recover_bus(&dev->adapter);
> }
> }
> if (allow_sleep)
> @@ -376,8 +365,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
> i2c_msg
Re: [3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery
On Thu, Nov 20, 2014 at 12:03:06PM +0200, Grygorii Strashko wrote: > This patch changes type of input parameter for .prepare/unprepare_recovery() > callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *. > This allows to simplify implementation of these callbacks and avoid > type conversations from i2c_bus_recovery_info to i2c_adapter. > The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter > which contains pointer on it. > > CC: Sekhar Nori > CC: Kevin Hilman > CC: Santosh Shilimkar > CC: Murali Karicheri > Signed-off-by: Grygorii Strashko Sounds and looks sensible Acked-by: Uwe Kleine-König -- 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 v7] i2c: rk3x: handle dynamic clock rate changes correctly
Wolfram, On Fri, Nov 21, 2014 at 10:18 AM, Wolfram Sang wrote: > On Thu, Nov 20, 2014 at 10:26:50AM +0100, Max Schwarz wrote: >> The i2c input clock can change dynamically, e.g. on the RK3066 where >> pclk_i2c0 and pclk_i2c1 are connected to the armclk, which changes >> rate on cpu frequency scaling. >> >> Until now, we incorrectly called clk_get_rate() while holding the >> i2c->lock in rk3x_i2c_xfer() to adapt to clock rate changes. >> Thanks to Huang Tao for reporting this issue. >> >> Do it properly now using the clk notifier framework. The callback >> logic was taken from i2c-cadence.c. >> >> Also rename all misleading "i2c_rate" variables to "clk_rate", as they >> describe the *input* clk rate. >> >> Signed-off-by: Max Schwarz >> Tested-by: Max Schwarz on RK3188 > > Superfluous, I assume that you tested a patch when you send it :) > >> Tested-by: Doug Anderson on RK3288 >> Reviewed-by: Doug Anderson > > Please drop such tags when there are significant changes between the > revisions (which happened here). They need to be sent again explicitly, > so I know really this version has been worked on. > > So, Doug, please confirm those tags. Consider them confirmed. I like this new version and it seems to be working well for me. I don't use the dynamic clock changes but it at least doesn't crash or break in any other ways. I've landed it locally in my tree. -Doug -- 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 v7] i2c: rk3x: handle dynamic clock rate changes correctly
On Thu, Nov 20, 2014 at 10:26:50AM +0100, Max Schwarz wrote: > The i2c input clock can change dynamically, e.g. on the RK3066 where > pclk_i2c0 and pclk_i2c1 are connected to the armclk, which changes > rate on cpu frequency scaling. > > Until now, we incorrectly called clk_get_rate() while holding the > i2c->lock in rk3x_i2c_xfer() to adapt to clock rate changes. > Thanks to Huang Tao for reporting this issue. > > Do it properly now using the clk notifier framework. The callback > logic was taken from i2c-cadence.c. > > Also rename all misleading "i2c_rate" variables to "clk_rate", as they > describe the *input* clk rate. > > Signed-off-by: Max Schwarz > Tested-by: Max Schwarz on RK3188 Superfluous, I assume that you tested a patch when you send it :) > Tested-by: Doug Anderson on RK3288 > Reviewed-by: Doug Anderson Please drop such tags when there are significant changes between the revisions (which happened here). They need to be sent again explicitly, so I know really this version has been worked on. So, Doug, please confirm those tags. signature.asc Description: Digital signature
Re: [PATCH 3/4] i2c: omap: don't reset controller if Arbitration Lost detected
On Fri, Nov 21, 2014 at 01:28:44AM +0400, Alexander Kochetkov wrote:
> Arbitration Lost is a expected situation in a multimaster environment.
> IP correctly detect it.
>
> The only reason for reseting IP in the AL case is to be sure to
> avoid advisory 1.94 (omap3) and errata i595 (omap4):
> "I2C: After an Arbitration is Lost the Module Incorrectly Starts
> the Next Transfer" with workaround: "The MST and STT bits inside
> I2C_CON should be set to 1 at the same moment (avoid setting the
> MST bit to 1 while STT = 0)."
>
> The driver never writes MST and STT bits separately and doesn't
> create condition for errata. So the reset is not necessary.
>
> Tested on Beagleboard XM C.
>
> Signed-off-by: Alexander Kochetkov
> ---
> drivers/i2c/busses/i2c-omap.c |6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 3ffb9c0..47103e7 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -707,13 +707,17 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> return 0;
>
> /* We have an error */
> - if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> + if (dev->cmd_err & (OMAP_I2C_STAT_ROVR |
> OMAP_I2C_STAT_XUDF)) {
> omap_i2c_reset(dev);
> __omap_i2c_init(dev);
> return -EIO;
> }
>
> + if (dev->cmd_err & OMAP_I2C_STAT_AL) {
> + return -EIO;
> + }
The errno for AL is -EAGAIN. Curly braces are not needed.
> +
> if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
> if (msg->flags & I2C_M_IGNORE_NAK)
> return 0;
> --
> 1.7.9.5
>
signature.asc
Description: Digital signature
Re: [PATCH 1/2] i2c: at91: remove legacy DMA support
On Fri, Nov 21, 2014 at 02:44:31PM +0100, Ludovic Desroches wrote: > From: Arnd Bergmann > > Since at91sam9g45 is now DT-only, all DMA capable users of this driver > are using the DT case, and the legacy support can be removed. > > Signed-off-by: Arnd Bergmann > Signed-off-by: Ludovic Desroches Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH 2/2] i2c: at91: enable probe deferring on dma channel request
On Fri, Nov 21, 2014 at 02:44:32PM +0100, Ludovic Desroches wrote: > If dma controller is not probed, defer i2c probe. > > Signed-off-by: Ludovic Desroches Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH] of: spi: Export single device registration method and accessors (v2)
On Fri, Nov 21, 2014 at 3:44 PM, Pantelis Antoniou wrote: > Hi Grant, > >> On Nov 21, 2014, at 17:33 , Grant Likely wrote: >> >> On Wed, 29 Oct 2014 12:22:04 + >> , Mark Brown >> wrote: >>> On Wed, Oct 29, 2014 at 01:48:06PM +0200, Pantelis Antoniou wrote: > On Oct 29, 2014, at 12:14 , Mark Brown wrote: >>> > This feels like there is an abstraction problem somewhere, whatever code > is supposed to use this is going to need to be taught about each > individual bus which is going to be tedious, I would expect that we'd > have something like the bus being able to provide a callback which will > get invoked whenever a new node appears on the parent node for the bus. >>> Thereâ EURO (tm)s a whole patchset that does exactly this. Look at "OF: spi: Add OF notifier handlerâ EURO and youâ EURO (tm)ll where this is used. >>> >>> I deleted that unread I'm afraid; one of the reasons that you should use >>> subject lines matching the styles for the subsystems is that it's one of >>> the things people use to filter out things that actually need attention, >>> if things are busy things that at first glance don't look terribly >>> relevant (like changes to the OF core in this case) are likely to get >>> looked at less urgently or just skipped. >>> >>> A quick glance suggests that this is adding code inside the SPI core so >>> it's still not explaining why anything is being exported, can you >>> clarify please? >> >> I have the same question. This doesn't look like it should be exporting >> symbols. >> >> Also, the way the patch is written causes a lot of code changes to get >> interleaved in the diff. It would be better to split into two patches; >> one that creates the new of_register_spi_device(), and a separate patch >> to add the other new functions. It would be certainly easier to review >> that way. >> > > The diff does make a mess of things; it's not that complex of a patch. > > Your wish shall be granted. I'll respin this over the weekend. I've fixed it in my tree by moving the match functions into the second patch. The diffs are much easier to read now. I'll post the new versions to the list shortly, but if you can test that would be fantastic. g. > >>> > SubmittingPatches says. Please also try to keep your CC list sane, > CCing random people just means that you're increasing the volume of mail > they have to process. I'm surprised kernel.org accepts so many CCs. >>> > I have to say I don't recall ever seeing v1... >>> All of them are in the CC list for a reason. >>> >>> This is a single, standalone SPI patch - you didn't send it as part of a >>> series (which is the only reason I read it). >> >> Yes, this is part of the OF overlay series. It should have at least been >> marked as [PATCH 7/8] and that it replaced the previous, buggy, patch 7. >> >> g. >> > > 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 v4 1/2] i2c: s3c2410: Handle i2c sys_cfg register in i2c driver
> >I usually don't take DTS patches. They should go via arm-soc. Please say > >so if there are reasons I should take them. > > I CC'ed to you because same patch contains changes in i2c driver. Yes, those should absolutely go via my I2C tree. You need to make a seperate patch out of the dts changes which then also should go via samsung-soc, unless Kukjin says he really wants to go the via I2C. But I guess the latter will just create merge conflicts. signature.asc Description: Digital signature
Re: [PATCH 3/4] i2c: omap: don't reset controller if Arbitration Lost detected
On Fri, Nov 21, 2014 at 01:28:44AM +0400, Alexander Kochetkov wrote:
> Arbitration Lost is a expected situation in a multimaster environment.
> IP correctly detect it.
>
> The only reason for reseting IP in the AL case is to be sure to
> avoid advisory 1.94 (omap3) and errata i595 (omap4):
> "I2C: After an Arbitration is Lost the Module Incorrectly Starts
> the Next Transfer" with workaround: "The MST and STT bits inside
> I2C_CON should be set to 1 at the same moment (avoid setting the
> MST bit to 1 while STT = 0)."
>
> The driver never writes MST and STT bits separately and doesn't
> create condition for errata. So the reset is not necessary.
>
> Tested on Beagleboard XM C.
>
> Signed-off-by: Alexander Kochetkov
Tested on BBB and AM437x Starter Kit
Tested-by: Felipe Balbi
Reviewed-by: Felipe Balbi
> ---
> drivers/i2c/busses/i2c-omap.c |6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 3ffb9c0..47103e7 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -707,13 +707,17 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> return 0;
>
> /* We have an error */
> - if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> + if (dev->cmd_err & (OMAP_I2C_STAT_ROVR |
> OMAP_I2C_STAT_XUDF)) {
> omap_i2c_reset(dev);
> __omap_i2c_init(dev);
> return -EIO;
> }
>
> + if (dev->cmd_err & OMAP_I2C_STAT_AL) {
> + return -EIO;
> + }
> +
> if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
> if (msg->flags & I2C_M_IGNORE_NAK)
> return 0;
> --
> 1.7.9.5
>
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH] i2c: omap: fix i207 errata handling
On Fri, Nov 21, 2014 at 04:16:51AM +0400, Alexander Kochetkov wrote:
> commit 6d9939f651419a63e091105663821f9c7d3fec37 (i2c: omap: split out [XR]DR
> and [XR]RDY) changed the way how errata i207 (I2C: RDR Flag May Be Incorrectly
> Set) get handled. 6d9939f6514 code doesn't correspond to workaround provided
> by
> errata.
>
> According to errata ISR must filter out spurious RDR before data read not
> after.
> ISR must read RXSTAT to get number of bytes available to read. Because RDR
> could be set while there could no data in the receive FIFO.
>
> Restored pre 6d9939f6514 way of handling errata.
>
> Found by code review. Real impact haven't seen.
> Tested on Beagleboard XM C.
>
> Signed-off-by: Alexander Kochetkov
> Fixes: 6d9939f651419a63e09110 i2c: omap: split out [XR]DR and [XR]RDY
Tested on BBB and AM437x Starter Kit
Tested-by: Felipe Balbi
Reviewed-by: Felipe Balbi
> ---
> drivers/i2c/busses/i2c-omap.c |8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 90dcc2e..e7cbcb0 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -958,11 +958,13 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
> if (dev->fifo_size)
> num_bytes = dev->buf_len;
>
> - omap_i2c_receive_data(dev, num_bytes, true);
> -
> - if (dev->errata & I2C_OMAP_ERRATA_I207)
> + if (dev->errata & I2C_OMAP_ERRATA_I207) {
> i2c_omap_errata_i207(dev, stat);
> + num_bytes = (omap_i2c_read_reg(dev,
> + OMAP_I2C_BUFSTAT_REG) >> 8) & 0x3F;
> + }
>
> + omap_i2c_receive_data(dev, num_bytes, true);
> omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
> continue;
> }
> --
> 1.7.9.5
>
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 2/4] i2c: omap: implement workaround for handling invalid BB-bit values
On Fri, Nov 21, 2014 at 01:28:43AM +0400, Alexander Kochetkov wrote:
> In a multimaster environment, after IP software reset, BB-bit value doesn't
> correspond to the current bus state. It may happen what BB-bit will be 0,
> while the bus is busy due to another I2C master activity.
>
> Any transfer started when BB=0 and bus is busy wouldn't be completed by IP
> and results in controller timeout. More over, in some cases IP could
> interrupt another master's transfer and corrupt data on wire.
>
> The commit implement method allowing to prevent IP from entering into
> "controller timeout" state and from "data corruption" state.
>
> The one drawback is the need to wait for 10ms before the first transfer.
>
> Tested on Beagleboard XM C.
>
> Signed-off-by: Alexander Kochetkov
Tested on BBB and AM437x Starter Kit
Tested-by: Felipe Balbi
Reviewed-by: Felipe Balbi
> ---
> drivers/i2c/busses/i2c-omap.c | 103
> +
> 1 file changed, 103 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index a021733..3ffb9c0 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -58,6 +58,9 @@
> /* timeout for pm runtime autosuspend */
> #define OMAP_I2C_PM_TIMEOUT 1000/* ms */
>
> +/* timeout for making decision on bus free status */
> +#define OMAP_I2C_BUS_FREE_TIMEOUT (msecs_to_jiffies(10))
> +
> /* For OMAP3 I2C_IV has changed to I2C_WE (wakeup enable) */
> enum {
> OMAP_I2C_REV_REG = 0,
> @@ -210,6 +213,9 @@ struct omap_i2c_dev {
>*/
> u32 rev;
> unsignedb_hw:1; /* bad h/w fixes */
> + unsignedbb_valid:1; /* true when BB-bit reflects
> + * the I2C bus state
> + */
> unsignedreceiver:1; /* true when we're in receiver
> mode */
> u16 iestate;/* Saved interrupt register */
> u16 pscstate;
> @@ -336,7 +342,10 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
> /* SYSC register is cleared by the reset; rewrite it */
> omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, sysc);
>
> + /* Schedule I2C-bus monitoring on the next transfer */
> + dev->bb_valid = 0;
> }
> +
> return 0;
> }
>
> @@ -449,6 +458,11 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
> dev->scllstate = scll;
> dev->sclhstate = sclh;
>
> + if (dev->rev < OMAP_I2C_OMAP1_REV_2) {
> + /* Not implemented */
> + dev->bb_valid = 1;
> + }
> +
> __omap_i2c_init(dev);
>
> return 0;
> @@ -473,6 +487,91 @@ static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
> return 0;
> }
>
> +/*
> + * Wait while BB-bit doesn't reflect the I2C bus state
> + *
> + * In a multimaster environment, after IP software reset, BB-bit value
> doesn't
> + * correspond to the current bus state. It may happen what BB-bit will be 0,
> + * while the bus is busy due to another I2C master activity.
> + * Here are BB-bit values after reset:
> + * SDA SCL BB NOTES
> + * 0 00 1, 2
> + * 1 00 1, 2
> + * 0 11
> + * 1 10 3
> + * Later, if IP detect SDA=0 and SCL=1 (ACK) or SDA 1->0 while SCL=1 (START)
> + * combinations on the bus, it set BB-bit to 1.
> + * If IP detect SDA 0->1 while SCL=1 (STOP) combination on the bus,
> + * it set BB-bit to 0 and BF to 1.
> + * BB and BF bits correctly tracks the bus state while IP is suspended
> + * BB bit became valid on the next FCLK clock after CON_EN bit set
> + *
> + * NOTES:
> + * 1. Any transfer started when BB=0 and bus is busy wouldn't be
> + *completed by IP and results in controller timeout.
> + * 2. Any transfer started when BB=0 and SCL=0 results in IP
> + *starting to drive SDA low. In that case IP corrupt data
> + *on the bus.
> + * 3. Any transfer started in the middle of another master's transfer
> + *results in unpredictable results and data corruption
> + */
> +static int omap_i2c_wait_for_bb_valid(struct omap_i2c_dev *dev)
> +{
> + unsigned long bus_free_timeout = 0;
> + unsigned long timeout;
> + int bus_free = 0;
> + u16 stat, systest;
> +
> + if (dev->bb_valid)
> + return 0;
> +
> + timeout = jiffies + OMAP_I2C_TIMEOUT;
> + while (1) {
> + stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> + /*
> + * We will see BB or BF event in a case IP had detected any
> + * activity on the I2C bus. Now IP correctly tracks the bus
> + * state. BB-bit value is valid.
> + */
> + if (stat & (OMAP_I2C_STAT_BB | OMAP_I2C_STAT_BF))
> + break;
>
Re: [PATCH 4/4] i2c: omap: add notes related to i2c multimaster mode
On Fri, Nov 21, 2014 at 01:28:45AM +0400, Alexander Kochetkov wrote: > No functional changes. > > Signed-off-by: Alexander Kochetkov heh: Tested on BBB and AM437x Starter Kit Tested-by: Felipe Balbi Reviewed-by: Felipe Balbi > --- > drivers/i2c/busses/i2c-omap.c | 12 +++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 47103e7..4e3642c 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -304,6 +304,12 @@ static void __omap_i2c_init(struct omap_i2c_dev *dev) > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); > > /* > + * NOTE: right after setting CON_EN, STAT_BB could be 0 while the > + * bus is busy. It will be changed to 1 on the next IP FCLK clock. > + * udelay(1) will be enough to fix that. > + */ > + > + /* >* Don't write to this register if the IE state is 0 as it can >* cause deadlock. >*/ > @@ -664,7 +670,11 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, > > if (!dev->b_hw && stop) > w |= OMAP_I2C_CON_STP; > - > + /* > + * NOTE: STAT_BB bit could became 1 here if another master occupy > + * the bus. IP successfully complete transfer when the bus will be > + * free again (BB reset to 0). > + */ > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w); > > /* > -- > 1.7.9.5 > -- balbi signature.asc Description: Digital signature
Re: [PATCH 1/4] i2c: omap: cleanup register definitions
On Fri, Nov 21, 2014 at 01:28:42AM +0400, Alexander Kochetkov wrote:
> Delete STAT_AD0 mask as unrelated to current IP (omap1?).
> Delete DEBUG conditional around SYSTEST masks group.
> Add SYSTEST functional mode masks for SCL and SDA.
> Add STAT_BF mask.
>
> Signed-off-by: Alexander Kochetkov
Tested on BBB and AM437x Starter Kit
Tested-by: Felipe Balbi
Reviewed-by: Felipe Balbi
> ---
> drivers/i2c/busses/i2c-omap.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 9af7095..a021733 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -102,7 +102,7 @@ enum {
> #define OMAP_I2C_STAT_ROVR (1 << 11) /* Receive overrun */
> #define OMAP_I2C_STAT_XUDF (1 << 10) /* Transmit underflow */
> #define OMAP_I2C_STAT_AAS(1 << 9)/* Address as slave */
> -#define OMAP_I2C_STAT_AD0(1 << 8)/* Address zero */
> +#define OMAP_I2C_STAT_BF (1 << 8)/* Bus Free */
> #define OMAP_I2C_STAT_XRDY (1 << 4)/* Transmit data ready */
> #define OMAP_I2C_STAT_RRDY (1 << 3)/* Receive data ready */
> #define OMAP_I2C_STAT_ARDY (1 << 2)/* Register access ready */
> @@ -150,16 +150,20 @@ enum {
> #define OMAP_I2C_SCLH_HSSCLH 8
>
> /* I2C System Test Register (OMAP_I2C_SYSTEST): */
> -#ifdef DEBUG
> #define OMAP_I2C_SYSTEST_ST_EN (1 << 15) /* System test
> enable */
> #define OMAP_I2C_SYSTEST_FREE(1 << 14) /* Free running
> mode */
> #define OMAP_I2C_SYSTEST_TMODE_MASK (3 << 12) /* Test mode select */
> #define OMAP_I2C_SYSTEST_TMODE_SHIFT (12)/* Test mode select */
> +/* Functional mode */
> +#define OMAP_I2C_SYSTEST_SCL_I_FUNC (1 << 8)/* SCL line input value
> */
> +#define OMAP_I2C_SYSTEST_SCL_O_FUNC (1 << 7)/* SCL line output
> value */
> +#define OMAP_I2C_SYSTEST_SDA_I_FUNC (1 << 6)/* SDA line input value
> */
> +#define OMAP_I2C_SYSTEST_SDA_O_FUNC (1 << 5)/* SDA line output
> value */
> +/* SDA/SCL IO mode */
> #define OMAP_I2C_SYSTEST_SCL_I (1 << 3)/* SCL line
> sense in */
> #define OMAP_I2C_SYSTEST_SCL_O (1 << 2)/* SCL line
> drive out */
> #define OMAP_I2C_SYSTEST_SDA_I (1 << 1)/* SDA line
> sense in */
> #define OMAP_I2C_SYSTEST_SDA_O (1 << 0)/* SDA line
> drive out */
> -#endif
>
> /* OCP_SYSSTATUS bit definitions */
> #define SYSS_RESETDONE_MASK (1 << 0)
> --
> 1.7.9.5
>
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH] of: spi: Export single device registration method and accessors (v2)
Hi Grant, > On Nov 21, 2014, at 17:33 , Grant Likely wrote: > > On Wed, 29 Oct 2014 12:22:04 + > , Mark Brown > wrote: >> On Wed, Oct 29, 2014 at 01:48:06PM +0200, Pantelis Antoniou wrote: On Oct 29, 2014, at 12:14 , Mark Brown wrote: >> This feels like there is an abstraction problem somewhere, whatever code is supposed to use this is going to need to be taught about each individual bus which is going to be tedious, I would expect that we'd have something like the bus being able to provide a callback which will get invoked whenever a new node appears on the parent node for the bus. >> >>> Thereâs a whole patchset that does exactly this. >>> Look at "OF: spi: Add OF notifier handlerâ and youâll where this is >>> used. >> >> I deleted that unread I'm afraid; one of the reasons that you should use >> subject lines matching the styles for the subsystems is that it's one of >> the things people use to filter out things that actually need attention, >> if things are busy things that at first glance don't look terribly >> relevant (like changes to the OF core in this case) are likely to get >> looked at less urgently or just skipped. >> >> A quick glance suggests that this is adding code inside the SPI core so >> it's still not explaining why anything is being exported, can you >> clarify please? > > I have the same question. This doesn't look like it should be exporting > symbols. > > Also, the way the patch is written causes a lot of code changes to get > interleaved in the diff. It would be better to split into two patches; > one that creates the new of_register_spi_device(), and a separate patch > to add the other new functions. It would be certainly easier to review > that way. > The diff does make a mess of things; it’s not that complex of a patch. Your wish shall be granted. I’ll respin this over the weekend. >> SubmittingPatches says. Please also try to keep your CC list sane, CCing random people just means that you're increasing the volume of mail they have to process. I'm surprised kernel.org accepts so many CCs. >> I have to say I don't recall ever seeing v1... >> >>> All of them are in the CC list for a reason. >> >> This is a single, standalone SPI patch - you didn't send it as part of a >> series (which is the only reason I read it). > > Yes, this is part of the OF overlay series. It should have at least been > marked as [PATCH 7/8] and that it replaced the previous, buggy, patch 7. > > g. > 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: [2/5] i2c: davinci: query STP always when NACK is received
Hi Uwe,
On 11/21/2014 03:10 PM, Uwe Kleine-König wrote:
> On Fri, Nov 21, 2014 at 02:48:57PM +0200, Grygorii Strashko wrote:
>> On 11/21/2014 12:19 AM, Uwe Kleine-König wrote:
diff --git a/drivers/i2c/busses/i2c-davinci.c
b/drivers/i2c/busses/i2c-davinci.c
index 9bbfb8f..2cef115 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
if (msg->flags & I2C_M_IGNORE_NAK)
return msg->len;
- if (stop) {
- w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
- w |= DAVINCI_I2C_MDR_STP;
- davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
- }
+ w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
+ w |= DAVINCI_I2C_MDR_STP;
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>>> I think this is a good change, but I wonder if the handling of
>>> I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
>>> for the 2nd byte of a 5-byte-message, the transfer supposed to
>>> continue, right? (Hmm, maybe the framework handle this and restarts the
>>> transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
>>> handle this flag?)
>>
>> Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
>> change current behavior - davinci driver will interrupt transfer of i2c_msg
>> always
>> in case of NACK and start transfer of the next i2c_msg (if exist).
>> In my opinion, Above question is out of scope of this patch.
> Yeah right, that's exactly what I thought.
>
> Thinking again I wonder if with your change handling is correct when the
> sender wants to do a repeated start. That would need a more detailed
> look into the driver.
Davinci driver will always abort transfer with error -EREMOTEIO in case if
NACK received from I2C slave device. And the next omap_i2c_xfer() call may
be *not* targeted to the same I2C slave device.
^ if !I2C_M_IGNORE_NAK
This discussion is absolutely similar to https://lkml.org/lkml/2013/7/16/235
So, I'm just copy-pasting my answers from there ;)
regards,
-grygorii
--
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: spi: Export single device registration method and accessors (v2)
On Wed, 29 Oct 2014 12:22:04 + , Mark Brown wrote: > On Wed, Oct 29, 2014 at 01:48:06PM +0200, Pantelis Antoniou wrote: > > > On Oct 29, 2014, at 12:14 , Mark Brown wrote: > > > > This feels like there is an abstraction problem somewhere, whatever code > > > is supposed to use this is going to need to be taught about each > > > individual bus which is going to be tedious, I would expect that we'd > > > have something like the bus being able to provide a callback which will > > > get invoked whenever a new node appears on the parent node for the bus. > > > Thereâs a whole patchset that does exactly this. > > Look at "OF: spi: Add OF notifier handlerâ and youâll where this is > > used. > > I deleted that unread I'm afraid; one of the reasons that you should use > subject lines matching the styles for the subsystems is that it's one of > the things people use to filter out things that actually need attention, > if things are busy things that at first glance don't look terribly > relevant (like changes to the OF core in this case) are likely to get > looked at less urgently or just skipped. > > A quick glance suggests that this is adding code inside the SPI core so > it's still not explaining why anything is being exported, can you > clarify please? I have the same question. This doesn't look like it should be exporting symbols. Also, the way the patch is written causes a lot of code changes to get interleaved in the diff. It would be better to split into two patches; one that creates the new of_register_spi_device(), and a separate patch to add the other new functions. It would be certainly easier to review that way. > > > > SubmittingPatches says. Please also try to keep your CC list sane, > > > CCing random people just means that you're increasing the volume of mail > > > they have to process. I'm surprised kernel.org accepts so many CCs. > > > > I have to say I don't recall ever seeing v1... > > > All of them are in the CC list for a reason. > > This is a single, standalone SPI patch - you didn't send it as part of a > series (which is the only reason I read it). Yes, this is part of the OF overlay series. It should have at least been marked as [PATCH 7/8] and that it replaced the previous, buggy, patch 7. g. -- 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 v8 6/8] OF: i2c: Add OF notifier handler
Hi Grant,
> On Nov 21, 2014, at 17:08 , Grant Likely wrote:
>
> On Thu, 20 Nov 2014 18:03:33 -0800
> , Guenter Roeck
> wrote:
>> On 11/20/2014 05:53 PM, Grant Likely wrote:
>>> On Tue, 28 Oct 2014 22:36:03 +0200
>>> , Pantelis Antoniou
>>> wrote:
Add OF notifier handler needed for creating/destroying i2c devices
according to dynamic runtime changes in the DT live tree.
Signed-off-by: Pantelis Antoniou
---
drivers/i2c/i2c-core.c | 79
+-
1 file changed, 78 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e6da9d3..e751b78 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1470,6 +1470,7 @@ struct i2c_adapter
*of_find_i2c_adapter_by_node(struct device_node *node)
return i2c_verify_adapter(dev);
}
EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
+
#else
static void of_i2c_register_devices(struct i2c_adapter *adap) { }
#endif /* CONFIG_OF */
@@ -1955,6 +1956,71 @@ void i2c_clients_command(struct i2c_adapter *adap,
unsigned int cmd, void *arg)
}
EXPORT_SYMBOL(i2c_clients_command);
+#if IS_ENABLED(CONFIG_OF)
+
+static int of_i2c_notify(struct notifier_block *nb,
+ unsigned long action, void *arg)
+{
+ struct device_node *dn;
+ struct i2c_adapter *adap;
+ struct i2c_client *client;
+ int state;
+
+ state = of_reconfig_get_state_change(action, arg);
+ if (state == -1)
+ return NOTIFY_OK;
+
+ switch (action) {
+ case OF_RECONFIG_ATTACH_NODE:
+ case OF_RECONFIG_DETACH_NODE:
+ dn = arg;
+ break;
+ case OF_RECONFIG_ADD_PROPERTY:
+ case OF_RECONFIG_REMOVE_PROPERTY:
+ case OF_RECONFIG_UPDATE_PROPERTY:
+ dn = ((struct of_prop_reconfig *)arg)->dn;
+ break;
+ default:
+ return NOTIFY_OK;
+ }
+
+ if (state) {
+
+ adap = of_find_i2c_adapter_by_node(dn->parent);
+ if (adap == NULL)
+ return NOTIFY_OK; /* not for us */
+
+ client = of_i2c_register_device(adap, dn);
+ put_device(&adap->dev);
+
+ if (IS_ERR(client)) {
+ pr_err("%s: failed to create for '%s'\n",
+ __func__, dn->full_name);
+ return notifier_from_errno(PTR_ERR(client));
+ }
+
+ } else {
+
+ /* find our device by node */
+ client = of_find_i2c_device_by_node(dn);
+ if (client == NULL)
+ return NOTIFY_OK; /* no? not meant for us */
+
+ /* unregister takes one ref away */
+ i2c_unregister_device(client);
+
+ /* and put the reference of the find */
+ put_device(&client->dev);
+
+ }
>>>
>>> Nit: odd whitespace
>>>
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block i2c_of_notifier;
+
+#endif
+
static int __init i2c_init(void)
{
int retval;
@@ -1972,8 +2038,19 @@ static int __init i2c_init(void)
retval = i2c_add_driver(&dummy_driver);
if (retval)
goto class_err;
- return 0;
+#if IS_ENABLED(CONFIG_OF)
+ i2c_of_notifier.notifier_call = of_i2c_notify;
>>
>> Wouldn't it be easier to just initialize i2c_of_notifier above instead of
>> here in the code ? Or is there a reason for doing it here ?
>
> Yes, absolutely. This code should also depend on CONFIG_OF_DYNAMIC too,
> not merely CONFIG_OF.
>
>>
No reason; it just seemed like the most opportune place to put it at the time.
>> Thanks,
>> Guenter
>>
+ retval = of_reconfig_notifier_register(&i2c_of_notifier);
+ if (retval)
+ goto notifier_err;
+#endif
>
> We can also get rid of the ugly #ifdef in the body by creating empty
> stubs for of_reconfig_notify_{register,unregister}() and doing the
> following:
>
> if (IS_ENABLED(CONFIG_OF_DYNAMIC))
> WARN_ON(of_reconfig_notifier_register(&i2c_of_notifier))
>
> I picked up the patch into my tree and made the above changes because
> they're pretty trivial. If Wolfram is okay with it then I can take the
> whole series through my tree. Otherwise I'll put the of_reconfig_* empty
> stubs into a separate branch that he and broonie can both pull.
>
I’m fine with doing this if you’d like, it’s pretty simple.
Regards
— Pantelis
> g.
>
+
+ return 0;
+#if IS_ENABLED(CONFIG_OF)
+notifier_err:
+ i2c_del_driver(&dummy_driver);
+#endif
>>>
>>> Similar to my comment on the platform bus, don't break the entire bus if
Re: [PATCH v8 6/8] OF: i2c: Add OF notifier handler
On Thu, 20 Nov 2014 18:03:33 -0800
, Guenter Roeck
wrote:
> On 11/20/2014 05:53 PM, Grant Likely wrote:
> > On Tue, 28 Oct 2014 22:36:03 +0200
> > , Pantelis Antoniou
> > wrote:
> >> Add OF notifier handler needed for creating/destroying i2c devices
> >> according to dynamic runtime changes in the DT live tree.
> >>
> >> Signed-off-by: Pantelis Antoniou
> >> ---
> >> drivers/i2c/i2c-core.c | 79
> >> +-
> >> 1 file changed, 78 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >> index e6da9d3..e751b78 100644
> >> --- a/drivers/i2c/i2c-core.c
> >> +++ b/drivers/i2c/i2c-core.c
> >> @@ -1470,6 +1470,7 @@ struct i2c_adapter
> >> *of_find_i2c_adapter_by_node(struct device_node *node)
> >>return i2c_verify_adapter(dev);
> >> }
> >> EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
> >> +
> >> #else
> >> static void of_i2c_register_devices(struct i2c_adapter *adap) { }
> >> #endif /* CONFIG_OF */
> >> @@ -1955,6 +1956,71 @@ void i2c_clients_command(struct i2c_adapter *adap,
> >> unsigned int cmd, void *arg)
> >> }
> >> EXPORT_SYMBOL(i2c_clients_command);
> >>
> >> +#if IS_ENABLED(CONFIG_OF)
> >> +
> >> +static int of_i2c_notify(struct notifier_block *nb,
> >> + unsigned long action, void *arg)
> >> +{
> >> + struct device_node *dn;
> >> + struct i2c_adapter *adap;
> >> + struct i2c_client *client;
> >> + int state;
> >> +
> >> + state = of_reconfig_get_state_change(action, arg);
> >> + if (state == -1)
> >> + return NOTIFY_OK;
> >> +
> >> + switch (action) {
> >> + case OF_RECONFIG_ATTACH_NODE:
> >> + case OF_RECONFIG_DETACH_NODE:
> >> + dn = arg;
> >> + break;
> >> + case OF_RECONFIG_ADD_PROPERTY:
> >> + case OF_RECONFIG_REMOVE_PROPERTY:
> >> + case OF_RECONFIG_UPDATE_PROPERTY:
> >> + dn = ((struct of_prop_reconfig *)arg)->dn;
> >> + break;
> >> + default:
> >> + return NOTIFY_OK;
> >> + }
> >> +
> >> + if (state) {
> >> +
> >> + adap = of_find_i2c_adapter_by_node(dn->parent);
> >> + if (adap == NULL)
> >> + return NOTIFY_OK; /* not for us */
> >> +
> >> + client = of_i2c_register_device(adap, dn);
> >> + put_device(&adap->dev);
> >> +
> >> + if (IS_ERR(client)) {
> >> + pr_err("%s: failed to create for '%s'\n",
> >> + __func__, dn->full_name);
> >> + return notifier_from_errno(PTR_ERR(client));
> >> + }
> >> +
> >> + } else {
> >> +
> >> + /* find our device by node */
> >> + client = of_find_i2c_device_by_node(dn);
> >> + if (client == NULL)
> >> + return NOTIFY_OK; /* no? not meant for us */
> >> +
> >> + /* unregister takes one ref away */
> >> + i2c_unregister_device(client);
> >> +
> >> + /* and put the reference of the find */
> >> + put_device(&client->dev);
> >> +
> >> + }
> >
> > Nit: odd whitespace
> >
> >> +
> >> + return NOTIFY_OK;
> >> +}
> >> +
> >> +static struct notifier_block i2c_of_notifier;
> >> +
> >> +#endif
> >> +
> >> static int __init i2c_init(void)
> >> {
> >>int retval;
> >> @@ -1972,8 +2038,19 @@ static int __init i2c_init(void)
> >>retval = i2c_add_driver(&dummy_driver);
> >>if (retval)
> >>goto class_err;
> >> - return 0;
> >>
> >> +#if IS_ENABLED(CONFIG_OF)
> >> + i2c_of_notifier.notifier_call = of_i2c_notify;
>
> Wouldn't it be easier to just initialize i2c_of_notifier above instead of
> here in the code ? Or is there a reason for doing it here ?
Yes, absolutely. This code should also depend on CONFIG_OF_DYNAMIC too,
not merely CONFIG_OF.
>
> Thanks,
> Guenter
>
> >> + retval = of_reconfig_notifier_register(&i2c_of_notifier);
> >> + if (retval)
> >> + goto notifier_err;
> >> +#endif
We can also get rid of the ugly #ifdef in the body by creating empty
stubs for of_reconfig_notify_{register,unregister}() and doing the
following:
if (IS_ENABLED(CONFIG_OF_DYNAMIC))
WARN_ON(of_reconfig_notifier_register(&i2c_of_notifier))
I picked up the patch into my tree and made the above changes because
they're pretty trivial. If Wolfram is okay with it then I can take the
whole series through my tree. Otherwise I'll put the of_reconfig_* empty
stubs into a separate branch that he and broonie can both pull.
g.
> >> +
> >> + return 0;
> >> +#if IS_ENABLED(CONFIG_OF)
> >> +notifier_err:
> >> + i2c_del_driver(&dummy_driver);
> >> +#endif
> >
> > Similar to my comment on the platform bus, don't break the entire bus if
> > registration of the notifier fails. I would drop the error case and just
> > do a WARN_ON() if it fails.
> >
> > Otherwise:
> >
> > Acked-by: Grant Likely
> >
> >> class_err:
> >> #ifdef CONFIG_I2C_COMPAT
> >>class_compat_unregister(i2c_adapter_compat_class)
Re: [PATCH 2/2] i2c: at91: enable probe deferring on dma channel request
On Friday 21 November 2014 14:44:32 Ludovic Desroches wrote: > If dma controller is not probed, defer i2c probe. > > Signed-off-by: Ludovic Desroches > --- Reviewed-by: Arnd Bergmann > > Arnd, > > It's a combination of the first patch I sent and yours. As you said that my > patch "looks wrong but actually it's ok" I didn't dare to add your > signed-off-by. I think a good way to deal with this is to explain in the patch description who wrote what part. Arnd -- 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: [1/5] i2c: i2c-davinci: switch to use platform_get_irq
On 11/21/2014 04:03 PM, Rob Herring wrote:
On Fri, Nov 21, 2014 at 5:01 AM, Grygorii Strashko
wrote:
On 11/20/2014 11:48 PM, Uwe Kleine-König wrote:
Hello Grygorii,
On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
Switch Davinci I2C driver to use platform_get_irq(), because
- it is not recommened to use
platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
resources any more, as they can be not ready yet in case of DT-booting.
- it makes code simpler
CC: Sekhar Nori
CC: Kevin Hilman
CC: Santosh Shilimkar
CC: Murali Karicheri
Signed-off-by: Grygorii Strashko
---
drivers/i2c/busses/i2c-davinci.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..9bbfb8f 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
-struct resource *mem, *irq;
-int r;
+struct resource *mem;
+int r, irq;
-irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-if (!irq) {
-dev_err(&pdev->dev, "no irq resource?\n");
-return -ENODEV;
+irq = platform_get_irq(pdev, 0);
One bad thing about platform_get_irq is its unusual handling of irq=0.
I'm pretty sure you don't want to use this value, so adding something
like:
if (!irq)
irq = -ENXIO
I'll add this check in driver.
would be welcome because the usual value for "invalid irq" is 0 and not
-ESOMETHING. platform_get_irq is one of the very few functions that
don't adhere to this convention. With handling <= 0 as error your code
is immune to changes in this area. Although I notice that
platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
Apart from your change I wonder if platform_get_irq should handle
of_irq_get returning 0 as an error.
I think you are right and It seems like, the check for !irq should
be added/restored for OF case in platform_get_irq() too.
Changing the return values of platform_get_irq is tricky as it would
potentially break drivers because NO_IRQ can be 0 or -1 depending on
the arch. Drivers checking against specific values of NO_IRQ would
break. We've done some clean-up in this area, but I suspect more is
needed.
Thanks for your comment.
regards,
-grygorii
--
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: slave-eeprom: add eeprom simulator driver
On Fri, Nov 21, 2014 at 08:19:41AM +0100, Uwe Kleine-König wrote:
> Hello Wolfram,
>
> this mail is thematically more a reply to patch 1 and maybe just serves
> my understanding of the slave support.
>
> On Tue, Nov 18, 2014 at 05:04:54PM +0100, Wolfram Sang wrote:
> > From: Wolfram Sang
> >
> > The first user of the i2c-slave interface is an eeprom simulator. It is
> > a shared memory which can be accessed by the remote master via I2C and
> > locally via sysfs.
> >
> > Signed-off-by: Wolfram Sang
> > ---
> >
> > Changes since RFC:
> > * fix build error for modules
> > * don't hardcode size
> > * add boundary checks for sysfs access
> > * use a spinlock
> > * s/at24s/eeprom/g
> > * add some docs
> > * clean up exit paths
> > * use size-variable instead of driver_data
> >
> > drivers/i2c/Kconfig| 10 +++
> > drivers/i2c/Makefile | 1 +
> > drivers/i2c/i2c-slave-eeprom.c | 170
> > +
> > 3 files changed, 181 insertions(+)
> > create mode 100644 drivers/i2c/i2c-slave-eeprom.c
> >
> > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> > index b51a402752c4..8c9e619f3026 100644
> > --- a/drivers/i2c/Kconfig
> > +++ b/drivers/i2c/Kconfig
> > @@ -110,6 +110,16 @@ config I2C_STUB
> >
> > If you don't know what to do here, definitely say N.
> >
> > +config I2C_SLAVE
> > + bool "I2C slave support"
> > +
> > +if I2C_SLAVE
> > +
> > +config I2C_SLAVE_EEPROM
> > + tristate "I2C eeprom slave driver"
> > +
> > +endif
> > +
> > config I2C_DEBUG_CORE
> > bool "I2C Core debugging messages"
> > help
> > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> > index 1722f50f2473..45095b3d16a9 100644
> > --- a/drivers/i2c/Makefile
> > +++ b/drivers/i2c/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o
> > obj-$(CONFIG_I2C_MUX) += i2c-mux.o
> > obj-y += algos/ busses/ muxes/
> > obj-$(CONFIG_I2C_STUB) += i2c-stub.o
> > +obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o
> >
> > ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
> > CFLAGS_i2c-core.o := -Wno-deprecated-declarations
> > diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
> > new file mode 100644
> > index ..6631400b5f02
> > --- /dev/null
> > +++ b/drivers/i2c/i2c-slave-eeprom.c
> > @@ -0,0 +1,170 @@
> > +/*
> > + * I2C slave mode EEPROM simulator
> > + *
> > + * Copyright (C) 2014 by Wolfram Sang, Sang Engineering
> >
> > + * Copyright (C) 2014 by Renesas Electronics 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 of the License.
> > + *
> > + * Because most IP blocks can only detect one I2C slave address anyhow,
> > this
> > + * driver does not support simulating EEPROM types which take more than one
> > + * address. It is prepared to simulate bigger EEPROMs with an internal 16
> > bit
> > + * pointer, yet implementation is deferred until the need actually arises.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +struct eeprom_data {
> > + struct bin_attribute bin;
> > + bool first_write;
> > + spinlock_t buffer_lock;
> > + u8 buffer_idx;
> > + u8 buffer[];
> > +};
> > +
> > +static int i2c_slave_eeprom_slave_cb(struct i2c_client *client,
> > +enum i2c_slave_event event, u8 *val)
> > +{
> > + struct eeprom_data *eeprom = i2c_get_clientdata(client);
> > +
> > + switch (event) {
> > + case I2C_SLAVE_REQ_WRITE_END:
> > + if (eeprom->first_write) {
> > + eeprom->buffer_idx = *val;
> > + eeprom->first_write = false;
> > + } else {
> > + spin_lock(&eeprom->buffer_lock);
> > + eeprom->buffer[eeprom->buffer_idx++] = *val;
> > + spin_unlock(&eeprom->buffer_lock);
> > + }
> > + break;
> > +
> > + case I2C_SLAVE_REQ_READ_START:
> > + spin_lock(&eeprom->buffer_lock);
> > + *val = eeprom->buffer[eeprom->buffer_idx];
> > + spin_unlock(&eeprom->buffer_lock);
> > + break;
> > +
> > + case I2C_SLAVE_REQ_READ_END:
> > + eeprom->buffer_idx++;
> You don't check here for buffer_idx >= ARRAY_SIZE(buffer)?
> Ditto in the I2C_SLAVE_REQ_WRITE_END case.
I just noticed that buffer_idx is an u8, so it overflows at 255+1. So
the probe routine should error out if size is bigger than 256.
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 ma
Re: [1/5] i2c: i2c-davinci: switch to use platform_get_irq
On Fri, Nov 21, 2014 at 5:01 AM, Grygorii Strashko
wrote:
> On 11/20/2014 11:48 PM, Uwe Kleine-König wrote:
>> Hello Grygorii,
>>
>> On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
>>> Switch Davinci I2C driver to use platform_get_irq(), because
>>> - it is not recommened to use
>>>platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
>>>resources any more, as they can be not ready yet in case of DT-booting.
>>> - it makes code simpler
>>>
>>> CC: Sekhar Nori
>>> CC: Kevin Hilman
>>> CC: Santosh Shilimkar
>>> CC: Murali Karicheri
>>> Signed-off-by: Grygorii Strashko
>>> ---
>>> drivers/i2c/busses/i2c-davinci.c | 14 +++---
>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-davinci.c
>>> b/drivers/i2c/busses/i2c-davinci.c
>>> index 4d96147..9bbfb8f 100644
>>> --- a/drivers/i2c/busses/i2c-davinci.c
>>> +++ b/drivers/i2c/busses/i2c-davinci.c
>>> @@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device
>>> *pdev)
>>> {
>>> struct davinci_i2c_dev *dev;
>>> struct i2c_adapter *adap;
>>> -struct resource *mem, *irq;
>>> -int r;
>>> +struct resource *mem;
>>> +int r, irq;
>>>
>>> -irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>> -if (!irq) {
>>> -dev_err(&pdev->dev, "no irq resource?\n");
>>> -return -ENODEV;
>>> +irq = platform_get_irq(pdev, 0);
>> One bad thing about platform_get_irq is its unusual handling of irq=0.
>> I'm pretty sure you don't want to use this value, so adding something
>> like:
>>
>> if (!irq)
>> irq = -ENXIO
>>
>> would be welcome because the usual value for "invalid irq" is 0 and not
>> -ESOMETHING. platform_get_irq is one of the very few functions that
>> don't adhere to this convention. With handling <= 0 as error your code
>> is immune to changes in this area. Although I notice that
>> platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
>>
>> Apart from your change I wonder if platform_get_irq should handle
>> of_irq_get returning 0 as an error.
>
> I think you are right and It seems like, the check for !irq should
> be added/restored for OF case in platform_get_irq() too.
Changing the return values of platform_get_irq is tricky as it would
potentially break drivers because NO_IRQ can be 0 or -1 depending on
the arch. Drivers checking against specific values of NO_IRQ would
break. We've done some clean-up in this area, but I suspect more is
needed.
Rob
--
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 2/2] i2c: at91: enable probe deferring on dma channel request
If dma controller is not probed, defer i2c probe.
Signed-off-by: Ludovic Desroches
---
Arnd,
It's a combination of the first patch I sent and yours. As you said that my
patch "looks wrong but actually it's ok" I didn't dare to add your
signed-off-by.
drivers/i2c/busses/i2c-at91.c | 25 ++---
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index b69ea9b..3e56d73 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -635,17 +635,17 @@ static int at91_twi_configure_dma(struct at91_twi_dev
*dev, u32 phy_addr)
slave_config.dst_maxburst = 1;
slave_config.device_fc = false;
- dma->chan_tx = dma_request_slave_channel(dev->dev, "tx");
- if (!dma->chan_tx) {
- dev_err(dev->dev, "can't get a DMA channel for tx\n");
- ret = -EBUSY;
+ dma->chan_tx = dma_request_slave_channel_reason(dev->dev, "tx");
+ if (IS_ERR(dma->chan_tx)) {
+ ret = PTR_ERR(dma->chan_tx);
+ dma->chan_tx = NULL;
goto error;
}
- dma->chan_rx = dma_request_slave_channel(dev->dev, "rx");
- if (!dma->chan_rx) {
- dev_err(dev->dev, "can't get a DMA channel for rx\n");
- ret = -EBUSY;
+ dma->chan_rx = dma_request_slave_channel_reason(dev->dev, "rx");
+ if (IS_ERR(dma->chan_rx)) {
+ ret = PTR_ERR(dma->chan_rx);
+ dma->chan_rx = NULL;
goto error;
}
@@ -666,6 +666,7 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev,
u32 phy_addr)
sg_init_table(&dma->sg, 1);
dma->buf_mapped = false;
dma->xfer_in_progress = false;
+ dev->use_dma = true;
dev_info(dev->dev, "using %s (tx) and %s (rx) for DMA transfers\n",
dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx));
@@ -673,7 +674,8 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev,
u32 phy_addr)
return ret;
error:
- dev_info(dev->dev, "can't use DMA\n");
+ if (ret != -EPROBE_DEFER)
+ dev_info(dev->dev, "can't use DMA, error %d\n", ret);
if (dma->chan_rx)
dma_release_channel(dma->chan_rx);
if (dma->chan_tx)
@@ -742,8 +744,9 @@ static int at91_twi_probe(struct platform_device *pdev)
clk_prepare_enable(dev->clk);
if (dev->dev->of_node) {
- if (at91_twi_configure_dma(dev, phy_addr) == 0)
- dev->use_dma = true;
+ rc = at91_twi_configure_dma(dev, phy_addr);
+ if (rc == -EPROBE_DEFER)
+ return rc;
}
rc = of_property_read_u32(dev->dev->of_node, "clock-frequency",
--
2.0.3
--
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 1/2] i2c: at91: remove legacy DMA support
From: Arnd Bergmann
Since at91sam9g45 is now DT-only, all DMA capable users of this driver
are using the DT case, and the legacy support can be removed.
Signed-off-by: Arnd Bergmann
Signed-off-by: Ludovic Desroches
---
Hi,
I have split the legacy dma support removing and probe deferring. I have also
fixed the compilation issue.
drivers/i2c/busses/i2c-at91.c | 37 +++--
1 file changed, 3 insertions(+), 34 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 917d545..b69ea9b 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -72,7 +72,6 @@ struct at91_twi_pdata {
unsigned clk_max_div;
unsigned clk_offset;
bool has_unre_flag;
- bool has_dma_support;
struct at_dma_slave dma_slave;
};
@@ -541,35 +540,30 @@ static struct at91_twi_pdata at91rm9200_config = {
.clk_max_div = 5,
.clk_offset = 3,
.has_unre_flag = true,
- .has_dma_support = false,
};
static struct at91_twi_pdata at91sam9261_config = {
.clk_max_div = 5,
.clk_offset = 4,
.has_unre_flag = false,
- .has_dma_support = false,
};
static struct at91_twi_pdata at91sam9260_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
- .has_dma_support = false,
};
static struct at91_twi_pdata at91sam9g20_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
- .has_dma_support = false,
};
static struct at91_twi_pdata at91sam9g10_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
- .has_dma_support = false,
};
static const struct platform_device_id at91_twi_devtypes[] = {
@@ -598,7 +592,6 @@ static struct at91_twi_pdata at91sam9x5_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
- .has_dma_support = true,
};
static const struct of_device_id atmel_twi_dt_ids[] = {
@@ -627,30 +620,11 @@ static const struct of_device_id atmel_twi_dt_ids[] = {
MODULE_DEVICE_TABLE(of, atmel_twi_dt_ids);
#endif
-static bool filter(struct dma_chan *chan, void *pdata)
-{
- struct at91_twi_pdata *sl_pdata = pdata;
- struct at_dma_slave *sl;
-
- if (!sl_pdata)
- return false;
-
- sl = &sl_pdata->dma_slave;
- if (sl && (sl->dma_dev == chan->device->dev)) {
- chan->private = sl;
- return true;
- } else {
- return false;
- }
-}
-
static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
{
int ret = 0;
- struct at91_twi_pdata *pdata = dev->pdata;
struct dma_slave_config slave_config;
struct at91_twi_dma *dma = &dev->dma;
- dma_cap_mask_t mask;
memset(&slave_config, 0, sizeof(slave_config));
slave_config.src_addr = (dma_addr_t)phy_addr + AT91_TWI_RHR;
@@ -661,19 +635,14 @@ static int at91_twi_configure_dma(struct at91_twi_dev
*dev, u32 phy_addr)
slave_config.dst_maxburst = 1;
slave_config.device_fc = false;
- dma_cap_zero(mask);
- dma_cap_set(DMA_SLAVE, mask);
-
- dma->chan_tx = dma_request_slave_channel_compat(mask, filter, pdata,
- dev->dev, "tx");
+ dma->chan_tx = dma_request_slave_channel(dev->dev, "tx");
if (!dma->chan_tx) {
dev_err(dev->dev, "can't get a DMA channel for tx\n");
ret = -EBUSY;
goto error;
}
- dma->chan_rx = dma_request_slave_channel_compat(mask, filter, pdata,
- dev->dev, "rx");
+ dma->chan_rx = dma_request_slave_channel(dev->dev, "rx");
if (!dma->chan_rx) {
dev_err(dev->dev, "can't get a DMA channel for rx\n");
ret = -EBUSY;
@@ -772,7 +741,7 @@ static int at91_twi_probe(struct platform_device *pdev)
}
clk_prepare_enable(dev->clk);
- if (dev->pdata->has_dma_support) {
+ if (dev->dev->of_node) {
if (at91_twi_configure_dma(dev, phy_addr) == 0)
dev->use_dma = true;
}
--
2.0.3
--
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: [2/5] i2c: davinci: query STP always when NACK is received
Hello,
On Fri, Nov 21, 2014 at 02:48:57PM +0200, Grygorii Strashko wrote:
> On 11/21/2014 12:19 AM, Uwe Kleine-König wrote:
> >> diff --git a/drivers/i2c/busses/i2c-davinci.c
> >> b/drivers/i2c/busses/i2c-davinci.c
> >> index 9bbfb8f..2cef115 100644
> >> --- a/drivers/i2c/busses/i2c-davinci.c
> >> +++ b/drivers/i2c/busses/i2c-davinci.c
> >> @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
> >> i2c_msg *msg, int stop)
> >>if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
> >>if (msg->flags & I2C_M_IGNORE_NAK)
> >>return msg->len;
> >> - if (stop) {
> >> - w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> >> - w |= DAVINCI_I2C_MDR_STP;
> >> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> >> - }
> >> + w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> >> + w |= DAVINCI_I2C_MDR_STP;
> >> + davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> > I think this is a good change, but I wonder if the handling of
> > I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
> > for the 2nd byte of a 5-byte-message, the transfer supposed to
> > continue, right? (Hmm, maybe the framework handle this and restarts the
> > transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
> > handle this flag?)
>
> Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
> change current behavior - davinci driver will interrupt transfer of i2c_msg
> always
> in case of NACK and start transfer of the next i2c_msg (if exist).
> In my opinion, Above question is out of scope of this patch.
Yeah right, that's exactly what I thought.
Thinking again I wonder if with your change handling is correct when the
sender wants to do a repeated start. That would need a more detailed
look into the driver.
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: [2/5] i2c: davinci: query STP always when NACK is received
Hi Uwe,
On 11/21/2014 12:19 AM, Uwe Kleine-König wrote:
> On Thu, Nov 20, 2014 at 12:03:05PM +0200, Grygorii Strashko wrote:
>> According to I2C specification the NACK should be handled as folowing:
> s/folowing/follows/
>
>> "When SDA remains HIGH during this ninth clock pulse, this is defined as the
>> Not
>> Acknowledge signal. The master can then gene rate either a STOP condition to
> s/gene rate/generate/
>
>> abort the transfer, or a repeated START condition to start a new transfer."
>> [http://www.nxp.com/documents/user_manual/UM10204.pdf]
> The link is nice, but pointing out that this is the i2c spec would be
> nice.
>
>> The same is recomened by TI I2C wiki:
> s/recomened/recommended/
>
>> http://processors.wiki.ti.com/index.php/I2C_Tips
> If the specification tells what to do, there is no need to further
> support your claim.
>
>> Currently, the Davinci I2C driver interrupts I2C trunsfer in case of NACK,
>> but
> s/trunsfer/transfer/
>
>> It queries Stop condition DAVINCI_I2C_MDR_REG.STP=1 only if NACK has been
>> received
> s/It/it/
>
>> during the last message transmitting/recieving.
> s/transmitting/transmitted/; s/recieving/received/
>
> I think I don't understand this sentence even with the typos corrected.
> Do you want to say:
>
> Currently the Davinci i2c driver interrupts the transfer on receipt of a
> NACK but fails to send a STOP in some situations and so makes the bus
> stuck.
>
>> This may lead to Bus stuck in "Bus Busy" until I2C IP reset (idle/enable) if
>> during SMBus reading transaction the first I2C message is NACKed.
> Did you hit this problem, or is this a theoretical issue?
I've hit this issue on OMAP board and the Davinci I2C driver is implemented in
a similar way.
Now I've no HW which would allow me to reproduce it on Davinci.
quotes from https://lkml.org/lkml/2013/7/16/235:
>>>
The problem is, that lm75 device is SmBus compatible and its driver has
.detect() function implemented. During detection it tries to scan some
registers which might be not present in current device - in my case
it's tmp105.
For example to read regA in tmp105 following is done:
1) do write in "Index" register (val RegA index) (I2C 1st message)
2) do read (I2C 2d message)
the message 1 is Nacked by lm75 type of device in case if register index is
wrong,
but i2c-omap don't send NACK (or STP). As result, bus stack in Bus Busy
state.
For SMBus devices the specification states (http://smbus.org/specs/)
"4.2.Acknowledge (ACK) and not acknowledge (NACK)":
- "The slave device detects an invalid command or invalid data. In this
case the slave device must not acknowledge the received byte. The
master upon detection of this condition must generate a STOP condition
and retry the transaction"
<<<
>
> Assuming this is a candidate for stable, adding a Fixes:-footer would be
> nice.
>
>> Hence, fix it by querying Stop condition (STP) always when NACK is received.
>>
>> This patch fixes Davinci I2C in the same way it was done for OMAP I2C
>> commit cda2109a26eb ("i2c: omap: query STP always when NACK is received").
>>
>> More info can be found at:
>> https://lkml.org/lkml/2013/7/16/159
>> http://patchwork.ozlabs.org/patch/249790/
> I'd drop this "more info" paragraph.
>
>> CC: Sekhar Nori
>> CC: Kevin Hilman
>> CC: Santosh Shilimkar
>> CC: Murali Karicheri
>> Reported-by: Hein Tibosch
> Is this Reported-by tag reused from the omap issue?
>
>> Signed-off-by: Grygorii Strashko
>> ---
>> drivers/i2c/busses/i2c-davinci.c | 8 +++-
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c
>> b/drivers/i2c/busses/i2c-davinci.c
>> index 9bbfb8f..2cef115 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
>> i2c_msg *msg, int stop)
>> if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
>> if (msg->flags & I2C_M_IGNORE_NAK)
>> return msg->len;
>> -if (stop) {
>> -w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> -w |= DAVINCI_I2C_MDR_STP;
>> -davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>> -}
>> +w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> +w |= DAVINCI_I2C_MDR_STP;
>> +davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> I think this is a good change, but I wonder if the handling of
> I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
> for the 2nd byte of a 5-byte-message, the transfer supposed to
> continue, right? (Hmm, maybe the framework handle this and restarts the
> transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
> handle this flag?)
Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
change current behavior - davinci driver will interrupt transfer of i2c_msg
always
in case of NACK a
Re: [1/5] i2c: i2c-davinci: switch to use platform_get_irq
On 11/20/2014 11:48 PM, Uwe Kleine-König wrote:
> Hello Grygorii,
>
> On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
>> Switch Davinci I2C driver to use platform_get_irq(), because
>> - it is not recommened to use
>>platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
>>resources any more, as they can be not ready yet in case of DT-booting.
>> - it makes code simpler
>>
>> CC: Sekhar Nori
>> CC: Kevin Hilman
>> CC: Santosh Shilimkar
>> CC: Murali Karicheri
>> Signed-off-by: Grygorii Strashko
>> ---
>> drivers/i2c/busses/i2c-davinci.c | 14 +++---
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c
>> b/drivers/i2c/busses/i2c-davinci.c
>> index 4d96147..9bbfb8f 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> {
>> struct davinci_i2c_dev *dev;
>> struct i2c_adapter *adap;
>> -struct resource *mem, *irq;
>> -int r;
>> +struct resource *mem;
>> +int r, irq;
>>
>> -irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> -if (!irq) {
>> -dev_err(&pdev->dev, "no irq resource?\n");
>> -return -ENODEV;
>> +irq = platform_get_irq(pdev, 0);
> One bad thing about platform_get_irq is its unusual handling of irq=0.
> I'm pretty sure you don't want to use this value, so adding something
> like:
>
> if (!irq)
> irq = -ENXIO
>
> would be welcome because the usual value for "invalid irq" is 0 and not
> -ESOMETHING. platform_get_irq is one of the very few functions that
> don't adhere to this convention. With handling <= 0 as error your code
> is immune to changes in this area. Although I notice that
> platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
>
> Apart from your change I wonder if platform_get_irq should handle
> of_irq_get returning 0 as an error.
I think you are right and It seems like, the check for !irq should
be added/restored for OF case in platform_get_irq() too.
Also, I've simulated irq == 0 case - the .probe() failed with error -EINVAL
which is returned by request_threaded_irq() because of
!irq_settings_can_request(desc).
i2c_davinci 253.i2c: failure requesting irq 0
i2c_davinci: probe of 253.i2c failed with error -22
I'm not sure that above will work for everyone because it depends on
ARCH_IRQ_INIT_FLAGS
and ARCH_IRQ_INIT_FLAGS = (IRQ_NOREQUEST | IRQ_NOPROBE) for ARM.
>
>> +if (irq < 0) {
>> +dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
> Please don't print an error if irq=-EPROBE_DEFER.
ok.
regards,
-grygorii
--
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 v5 1/2] i2c: s3c2410: Handle i2c sys_cfg register in i2c driver
Let's handle i2c interrupt re-configuration in i2c driver. This will
help us in removing some soc specific checks from machine files and
will help in removing static iomapping of SYS register in exynos.c
Since only Exynos5250, and Exynos5420 has i2c nodes in DT, added syscon
based phandle to i2c device nodes of respective SoC DT files.
Also handle saving and restoring of SYS_I2C_CFG register during
suspend and resume of i2c driver.
CC: Rob Herring
CC: Randy Dunlap
CC: Wolfram Sang
CC: Russell King
CC: devicet...@vger.kernel.org
CC: linux-...@vger.kernel.org
CC: linux-i2c@vger.kernel.org
Signed-off-by: Pankaj Dubey
---
.../devicetree/bindings/i2c/i2c-s3c2410.txt|1 +
arch/arm/boot/dts/exynos5250.dtsi |4 +++
arch/arm/boot/dts/exynos5420.dtsi |4 +++
drivers/i2c/busses/i2c-s3c2410.c | 29
4 files changed, 38 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-s3c2410.txt
b/Documentation/devicetree/bindings/i2c/i2c-s3c2410.txt
index 278de8e..89b3250 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-s3c2410.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-s3c2410.txt
@@ -32,6 +32,7 @@ Optional properties:
specified, default value is 0.
- samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
specified, the default value in Hz is 10.
+ - samsung,sysreg-phandle - handle to syscon used to control the system
registers
Example:
diff --git a/arch/arm/boot/dts/exynos5250.dtsi
b/arch/arm/boot/dts/exynos5250.dtsi
index 0a588b4..d45a07e 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -293,6 +293,7 @@
clock-names = "i2c";
pinctrl-names = "default";
pinctrl-0 = <&i2c0_bus>;
+ samsung,sysreg-phandle = <&sysreg_system_controller>;
status = "disabled";
};
@@ -306,6 +307,7 @@
clock-names = "i2c";
pinctrl-names = "default";
pinctrl-0 = <&i2c1_bus>;
+ samsung,sysreg-phandle = <&sysreg_system_controller>;
status = "disabled";
};
@@ -319,6 +321,7 @@
clock-names = "i2c";
pinctrl-names = "default";
pinctrl-0 = <&i2c2_bus>;
+ samsung,sysreg-phandle = <&sysreg_system_controller>;
status = "disabled";
};
@@ -332,6 +335,7 @@
clock-names = "i2c";
pinctrl-names = "default";
pinctrl-0 = <&i2c3_bus>;
+ samsung,sysreg-phandle = <&sysreg_system_controller>;
status = "disabled";
};
diff --git a/arch/arm/boot/dts/exynos5420.dtsi
b/arch/arm/boot/dts/exynos5420.dtsi
index 8617a03..90bf401 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -560,6 +560,7 @@
clock-names = "i2c";
pinctrl-names = "default";
pinctrl-0 = <&i2c0_bus>;
+ samsung,sysreg-phandle = <&sysreg_system_controller>;
status = "disabled";
};
@@ -573,6 +574,7 @@
clock-names = "i2c";
pinctrl-names = "default";
pinctrl-0 = <&i2c1_bus>;
+ samsung,sysreg-phandle = <&sysreg_system_controller>;
status = "disabled";
};
@@ -586,6 +588,7 @@
clock-names = "i2c";
pinctrl-names = "default";
pinctrl-0 = <&i2c2_bus>;
+ samsung,sysreg-phandle = <&sysreg_system_controller>;
status = "disabled";
};
@@ -599,6 +602,7 @@
clock-names = "i2c";
pinctrl-names = "default";
pinctrl-0 = <&i2c3_bus>;
+ samsung,sysreg-phandle = <&sysreg_system_controller>;
status = "disabled";
};
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 6524477..09a6bac 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -35,6 +35,8 @@
#include
#include
#include
+#include
+#include
#include
@@ -87,6 +89,9 @@
/* Max time to wait for bus to become idle after a xfer (in us) */
#define S3C2410_IDLE_TIMEOUT 5000
+/* Exynos5 Sysreg offset */
+#define EXYNOS5_SYS_I2C_CFG0x0234
+
/* i2c controller state */
enum s3c24xx_i2c_state {
STATE_IDLE,
@@ -123,6 +128,8 @@ struct s3c24xx_i2c {
#if defined(CONFIG_ARM_S3C24XX_CPUFREQ)
struct notifier_block freq_transition;
#endif
+ struct regmap *sysreg;
+ unsigned intsys_i2c_cfg;
};
static struct platform_device_id s3c24xx_driver_ids[] = {
@@ -1071,6 +1078,7 @@ static void
s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
{
struct s3c2410_platform_i2c *pdata = i2c->pdata;
+ int
Re: [PATCH v4 1/2] i2c: s3c2410: Handle i2c sys_cfg register in i2c driver
On Friday 21 November 2014 12:55 PM, Wolfram Sang wrote:
On Thu, Oct 30, 2014 at 01:34:29PM +0530, Pankaj Dubey wrote:
Let's handle i2c interrupt re-configuration in i2c driver. This will
help us in removing some soc specific checks from machine files and
will help in removing static iomapping of SYS register in exynos.c
Since only Exynos5250, and Exynos5420 has i2c nodes in DT, added syscon
based phandle to i2c device nodes of respective SoC DT files.
Also handle saving and restoring of SYS_I2C_CFG register during
suspend and resume of i2c driver.
CC: Rob Herring
CC: Randy Dunlap
CC: Wolfram Sang
CC: Russell King
CC: devicet...@vger.kernel.org
CC: linux-...@vger.kernel.org
CC: linux-i2c@vger.kernel.org
Signed-off-by: Pankaj Dubey
---
.../devicetree/bindings/i2c/i2c-s3c2410.txt|1 +
arch/arm/boot/dts/exynos5250.dtsi |4 +++
arch/arm/boot/dts/exynos5420.dtsi |4 +++
I usually don't take DTS patches. They should go via arm-soc. Please say
so if there are reasons I should take them.
I CC'ed to you because same patch contains changes in i2c driver.
I am not very sure via which tree this should go. May be I can ask
samsung SoC maintainer Kukjin to look into this, as patch 2/2 has
changes in mach-exynos which should go via Kukjin's tree.
@@ -1084,6 +1092,23 @@ s3c24xx_i2c_parse_dt(struct device_node *np, struct
s3c24xx_i2c *i2c)
of_property_read_u32(np, "samsung,i2c-slave-addr", &pdata->slave_addr);
of_property_read_u32(np, "samsung,i2c-max-bus-freq",
(u32 *)&pdata->frequency);
+ /*
+* Exynos5's legacy i2c controller and new high speed i2c
+* controller have muxed interrupt sources. By default the
+* interrupts for 4-channel HS-I2C controller are enabled.
+* If node for first four channels of legacy i2c controller
s/node/nodes/
OK.
+* are available then re-configure the interrupts via the
+* system register.
+*/
+ id = of_alias_get_id(np, "i2c");
+ i2c->sysreg = syscon_regmap_lookup_by_phandle(np,
+ "samsung,sysreg-phandle");
+ if (IS_ERR(i2c->sysreg)) {
+ /* As this is not compulsory do not return error */
+ pr_info("i2c-%d skipping re-configuration of interrutps\n", id);
I'd say drop this message. If you want to keep it, it should be dev_dbg.
OK.
+ return;
+ }
+ regmap_update_bits(i2c->sysreg, EXYNOS5_SYS_I2C_CFG, BIT(id), 0);
}
Rest looks good, thanks!
Thanks for review.
Pankaj Dubey
--
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
