Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On 06/26/2013 02:25 PM, Christian Ruppert wrote: > This patch makes the SDA hold time configurable through device tree. > > Signed-off-by: Christian Ruppert > Signed-off-by: Pierrick Hascoet > --- > .../devicetree/bindings/i2c/i2c-designware.txt | 15 +++ > arch/arc/boot/dts/abilis_tb100_dvk.dts | 10 +- > arch/arc/boot/dts/abilis_tb101_dvk.dts | 10 +- > drivers/i2c/busses/i2c-designware-core.c | 13 + > drivers/i2c/busses/i2c-designware-core.h |1 + > drivers/i2c/busses/i2c-designware-platdrv.c| 10 ++ > 6 files changed, 49 insertions(+), 10 deletions(-) Acked-by: Vineet Gupta for arch/arc bits -Vineet ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 26 June 2013, Wolfram Sang wrote:
> On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > This patch makes the SDA hold time configurable through device tree.
> >
> > Signed-off-by: Christian Ruppert
> > Signed-off-by: Pierrick Hascoet
>
> Applied to for-next, thanks for keeping at it and providing lots of
> useful information. Much appreciated!
Sorry, but I got a regression that I didn't find reported elsewhere
so far, even though it breaks a lot of the ARM defconfig builds:
drivers/built-in.o: In function `dw_i2c_probe':
/git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined
reference to `__udivdi3'
I suspect you want something like the change below.
Signed-off-by: Arnd Bergmann
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index def79b5..57e3a07 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -119,10 +119,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
if (pdev->dev.of_node) {
u32 ht = 0;
u32 ic_clk = dev->get_clk_rate_khz(dev);
+ u64 hold_time;
of_property_read_u32(pdev->dev.of_node,
"i2c-sda-hold-time-ns", &ht);
- dev->sda_hold_time = ((u64)ic_clk * ht + 50) / 100;
+ hold_time = (u64)ic_clk * ht + 50;
+ do_div(hold_time, 100);
+ dev->sda_hold_time = hold_time;
}
dev->functionality =
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Christian Ruppert wrote:
> On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote:
> > On Wednesday 26 June 2013, Wolfram Sang wrote:
> > > On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > > > This patch makes the SDA hold time configurable through device tree.
> > > >
> > > > Signed-off-by: Christian Ruppert
> > > > Signed-off-by: Pierrick Hascoet
> > >
> > > Applied to for-next, thanks for keeping at it and providing lots of
> > > useful information. Much appreciated!
> >
> > Sorry, but I got a regression that I didn't find reported elsewhere
> > so far, even though it breaks a lot of the ARM defconfig builds:
> >
> > drivers/built-in.o: In function `dw_i2c_probe':
> > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined
> > reference to `__udivdi3'
> >
> > I suspect you want something like the change below.
>
> This looks similar to a patch Vincent Stehle submitted yesterday, see
> https://lkml.org/lkml/2013/7/2/145
Thanks for the link. Actually his patch looks wrong to me, because
dev->sda_hold_time = div_u64((u64)ic_clk * ht + 50, 100);
assigns the division remainder to sda_hold_time, not the quotient.
Arnd
> > Signed-off-by: Arnd Bergmann
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index def79b5..57e3a07 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -119,10 +119,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
> > if (pdev->dev.of_node) {
> > u32 ht = 0;
> > u32 ic_clk = dev->get_clk_rate_khz(dev);
> > + u64 hold_time;
> >
> > of_property_read_u32(pdev->dev.of_node,
> > "i2c-sda-hold-time-ns", &ht);
> > - dev->sda_hold_time = ((u64)ic_clk * ht + 50) / 100;
> > + hold_time = (u64)ic_clk * ht + 50;
> > + do_div(hold_time, 100);
> > + dev->sda_hold_time = hold_time;
> > }
> >
> > dev->functionality =
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Christian Ruppert wrote: > On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote: > > On Wednesday 03 July 2013, Christian Ruppert wrote: > > > On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote: > > > > On Wednesday 26 June 2013, Wolfram Sang wrote: > > > > > On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote: > > > > > > This patch makes the SDA hold time configurable through device tree. > > > > > > > > > > > > Signed-off-by: Christian Ruppert > > > > > > Signed-off-by: Pierrick Hascoet > > > > > > > > > > Applied to for-next, thanks for keeping at it and providing lots of > > > > > useful information. Much appreciated! > > > > > > > > Sorry, but I got a regression that I didn't find reported elsewhere > > > > so far, even though it breaks a lot of the ARM defconfig builds: > > > > > > > > drivers/built-in.o: In function `dw_i2c_probe': > > > > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined > > > > reference to `__udivdi3' > > > > > > > > I suspect you want something like the change below. > > > > > > This looks similar to a patch Vincent Stehle submitted yesterday, see > > > https://lkml.org/lkml/2013/7/2/145 > > > > Thanks for the link. Actually his patch looks wrong to me, because > > > > dev->sda_hold_time = div_u64((u64)ic_clk * ht + 50, 100); > > > > assigns the division remainder to sda_hold_time, not the quotient. > > Hrmmm... At least when I tested it this morning on an ARC architecture > it worked as intended and returned the quotient. Does that mean we have > an issue with this function on ARC? Can anyone who knows these functions > better than I comment? ARC just uses the generic version of div_u64, which is defined in lib/div64.c. I suspect that the division remainder just happens to work well enough for you to not cause any run-time error. You could try adding a printk in that function to show the values you get on ARC. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Christian Ruppert wrote: > On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote: > > On Wednesday 03 July 2013, Christian Ruppert wrote: > > > On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote: > > > > On Wednesday 26 June 2013, Wolfram Sang wrote: > > > > > On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote: > > > > > > This patch makes the SDA hold time configurable through device tree. > > > > > > > > > > > > Signed-off-by: Christian Ruppert > > > > > > Signed-off-by: Pierrick Hascoet > > > > > > > > > > Applied to for-next, thanks for keeping at it and providing lots of > > > > > useful information. Much appreciated! > > > > > > > > Sorry, but I got a regression that I didn't find reported elsewhere > > > > so far, even though it breaks a lot of the ARM defconfig builds: > > > > > > > > drivers/built-in.o: In function `dw_i2c_probe': > > > > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined > > > > reference to `__udivdi3' > > > > > > > > I suspect you want something like the change below. > > > > > > This looks similar to a patch Vincent Stehle submitted yesterday, see > > > https://lkml.org/lkml/2013/7/2/145 > > > > Thanks for the link. Actually his patch looks wrong to me, because > > > > dev->sda_hold_time = div_u64((u64)ic_clk * ht + 50, 100); > > > > assigns the division remainder to sda_hold_time, not the quotient. > > Hrmmm... At least when I tested it this morning on an ARC architecture > it worked as intended and returned the quotient. Does that mean we have > an issue with this function on ARC? Can anyone who knows these functions > better than I comment? ARC just uses the generic version of div_u64, which is defined in lib/div64.c. I suspect that the division remainder just happens to work well enough for you to not cause any run-time error. You could try adding a printk in that function to show the values you get on ARC. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Stehle Vincent-B46079 wrote:
> > From: Christian Ruppert [mailto:christian.rupp...@abilis.com]
> (..)
> > Although this doesn't explicitly state what the function returns to me
> > this sounds more like the quotient is returned rather than the remainder?
>
> Hi,
>
> Yes div_u64() returns the quotient.
>
> It is defined in linux/math64.h as:
>
> static inline u64 div_u64(u64 dividend, u32 divisor)
> {
> u32 remainder;
> return div_u64_rem(dividend, divisor, &remainder);
> }
>
> The remainder is probably fully optimized out.
Ah, you are right, sorry for the confusion on my part.
I thought you had used do_div() rather than div_u64().
Everything's fine then, feel free to add my
Acked-by: Arnd Bergmann
to your patch.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
