Re: [PATCH v3 00/11] i2c: rcar: tackle race conditions in the driver

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patches.

For the whole series,

Tested-by: Laurent Pinchart 

On Thursday 19 November 2015 16:56:40 Wolfram Sang wrote:
> Hello RCar Fans!
> 
> So, here is V3 of this series. After a debugging session with Laurent, we
> finally fixed his issue for good. It was not board dependent as we thought,
> but toolchain dependent! Hidden by a macro, the driver used a compound
> assignemt with a function call as the rvalue. After patch 6, this function
> also changed the flags which were to be changed by the compound assignment.
> Basically (after macro):
> 
>   priv->flags |= i_change_priv_flags(priv);
> 
> Which is undefined behaviour, I guess. However, after my refactoring, the
> called functions always returned 0, so we can simply do:
> 
>   i_change_priv_flags(priv);
> 
> Nasty one, but finally issue gone, for all toolchains and optimization
> settings. Furthermore, patch 11 has been added because HW engineers wanted
> it.
> 
> The branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
> renesas/rcar-i2c-rework-v3
> 
> Please test, test, test :)
> 
>Wolfram
> 
> 
> Changes since V2:
> * patch 6/11 was cleaned up to not use the compund assignment
> * patch 11/11 introduced as requested by HW engineers
> 
> Changes since V1:
> * new patch 1/10 to ensure clock is always on
> * rebased patch 2/10 to the new patch
> * some patch descriptions slightly reworded
> 
> 
> Here is the description of the V1 series for those who missed it:
> 
> Two issues people have seen with the i2c-rcar driver was:
> 
> a) immediately restarted messages after NACK from client
> b) duplicated data bytes in messages
> 
> Some people already worked on those and had a tough time because it was hard
> to reproduce these issues on non-customer setup. Luckily, I somewhen had a
> state where the first transfer after boot would always show the above
> issues on a plain Renesas Lager board. When measuring, I found a third
> issue thanks to my new tool 'i2ctransfer' (and thanks to projects like
> sigrok and OpenLogicSniffer, of course. Thank you very much!):
> 
> c) after read message, no repeated start was sent, but stop + start.
> 
> Due to some unlucky design choices in the IP core, it has some race windows
> which can cause problems if interrupts get delayed. Also, for every new
> message in one transfer, context switches between interrupt and process
> were needed.
> 
> So I refactored the driver to setup new messages in interrupt context, too.
> This avoids the race for b) because we are now setting up the new message
> before we release the i2c bus clock (before we released the clock and set up
> the message in process context). c) is also fixed, this was not a race but
> a bug in the state handling. a) however is not fixed 100% :( We have the
> race window as small as possible now when utilizing interrupts, so it is an
> improvement and worked for my test cases well. There were experiments by me
> and Renesas engineers to use polling to prevent the issue but this caused
> other side effects, sadly. So, let's improve the situation now and let's
> see where we get.
> 
> I did quite some lab testing here and also verified that slave support does
> not suffer from these changes. However, I'd really appreciate if people
> could give this real-world-testing which is always different.
> 
> Please have a look, a test, etc...
> 
> Thanks,
> 
>Wolfram
> 
> 
> Wolfram Sang (11):
>   i2c: rcar: make sure clocks are on when doing clock calculation
>   i2c: rcar: rework hw init
>   i2c: rcar: remove unused IOERROR state
>   i2c: rcar: remove spinlock
>   i2c: rcar: refactor setup of a msg
>   i2c: rcar: init new messages in irq
>   i2c: rcar: don't issue stop when HW does it automatically
>   i2c: rcar: check master irqs before slave irqs
>   i2c: rcar: revoke START request early
>   i2c: rcar: clean up after refactoring
>   i2c: rcar: handle difference in setting up non-first message
> 
>  drivers/i2c/busses/i2c-rcar.c | 249 +--
>  1 file changed, 106 insertions(+), 143 deletions(-)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 05/10] i2c: rcar: refactor setup of a msg

[Laurent Pinchart]

Hi Magnus,

On Wednesday 18 November 2015 11:18:00 Magnus Damm wrote:
> On Tue, Nov 17, 2015 at 4:00 PM, Wolfram Sang  wrote:
> > Hi Laurent,
> > 
> >> Sorry for bringing bad news, but as with v1, this patch breaks ADV7511
> >> detection on my Koelsch board. Reverting it on top of the series fixes
> >> the problem.
> > 
> > In v1, patch 5/9 was breaking. I hope in v2, it is 6/10 and not 5/10 as
> > you replied to (patch 1/10 is the new one)? This bug is strange enough,
> > but 5/10 breaking would be extremly crazy.
> > 
> >> You'll find the dmesg and trace logs with your debugging patch applied
> >> attached to this e-mail in two versions, one with the whole series
> >> applied (- bad) and one with this patch additionally reverted on top of
> >> the series (- good).
> > 
> > Thanks for doing this. I'll think about it some more, but it may be that
> > I am running out of ideas without being able to connect a scope. Would
> > it be possible to exchange our Koelsch and Lager boards for a while? Or
> > is your multimedia work Koelsch dependant?
> 
> Swapping boards is of course one option, but shouldn't it also be
> possible to reproduce the issue by creating a similar hardware setup
> using loopback adapters? The problem is that you cannot reproduce it
> on your current hardware, right?

If I remember correctly not only has Wolfram not been able to reproduce the 
problem on his Lager board, but he hasn't been able to reproduce it you the 
remote access Koelsch board. I'm not sure how he could try to reproduce it 
locally with a "similar hardware setup" on Lager if it can't be reproduced on 
a different Koelsch board :-)

> Judging by the Koelsch schematics port EXIO_C has GP6_22/GP6_23 routed
> as SD2_CD_3/SC2_WP_3 that may be possible to repurpose as
> IIC1_SCL_C/IIC1_SDA_C. So if you want to try an IIC master device on
> Koeslch with the I2C2 devices that hold the ADV7511 chip then you
> should be able to use those ZEBAX adapters and loopback wires. If you
> want to sniff the I2C2 signals on Koelsch then the I2C2 bus is exposed
> to EXIO_A and EXIO_D.
> 
> On Lager it seems that the GP5_5/GP_6 pins with the I2C2 bus for the
> ADV7511 chip has more flexible configuration, so using either IIC or
> I2C should be possible. You can sample those pins on EXIO_A with the
> ZEBAX break out adapter.
> 
> And either way you should be able to compare the results of the I2C
> and IIC masters with GPIO using the bitbang implementation.
> 
> Feel free to let me know exactly what the problem is and I will do my
> best to help you out!

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 05/10] i2c: rcar: refactor setup of a msg

[Laurent Pinchart]

Hi Wolfram,

On Tuesday 17 November 2015 08:00:53 Wolfram Sang wrote:
> Hi Laurent,
> 
> > Sorry for bringing bad news, but as with v1, this patch breaks ADV7511
> > detection on my Koelsch board. Reverting it on top of the series fixes the
> > problem.
> 
> In v1, patch 5/9 was breaking. I hope in v2, it is 6/10 and not 5/10 as
> you replied to (patch 1/10 is the new one)? This bug is strange enough,
> but 5/10 breaking would be extremly crazy.

My bad, it's indeed "[PATCH v2 06/10] i2c: rcar: init new messages in irq" 
that I have reverted on top of the series.

> > You'll find the dmesg and trace logs with your debugging patch applied
> > attached to this e-mail in two versions, one with the whole series applied
> > (- bad) and one with this patch additionally reverted on top of the
> > series (- good).
> 
> Thanks for doing this. I'll think about it some more, but it may be that
> I am running out of ideas without being able to connect a scope. Would
> it be possible to exchange our Koelsch and Lager boards for a while? Or
> is your multimedia work Koelsch dependant?

I'm currently doing multimedia development on Koelsch. I'll check whether I 
can continue that on Lager. In the meantime I could also capture I2C traces 
using a scope and send them to you.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 05/10] i2c: rcar: refactor setup of a msg

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

Sorry for bringing bad news, but as with v1, this patch breaks ADV7511 
detection on my Koelsch board. Reverting it on top of the series fixes the 
problem.

You'll find the dmesg and trace logs with your debugging patch applied 
attached to this e-mail in two versions, one with the whole series applied (-
bad) and one with this patch additionally reverted on top of the series (-
good).

On Thursday 12 November 2015 15:31:50 Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> We want to reuse this function later.
> 
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/i2c/busses/i2c-rcar.c | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 0f2dc74ab8bcc1..4bd3099865b485 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -106,7 +106,8 @@ enum rcar_i2c_type {
>  struct rcar_i2c_priv {
>   void __iomem *io;
>   struct i2c_adapter adap;
> - struct i2c_msg  *msg;
> + struct i2c_msg *msg;
> + int msgs_left;
>   struct clk *clk;
> 
>   wait_queue_head_t wait;
> @@ -255,6 +256,11 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv
> *priv) {
>   int read = !!rcar_i2c_is_recv(priv);
> 
> + priv->pos = 0;
> + priv->flags = 0;
> + if (priv->msgs_left == 1)
> + rcar_i2c_flags_set(priv, ID_LAST_MSG);
> +
>   rcar_i2c_write(priv, ICMAR, (priv->msg->addr << 1) | read);
>   rcar_i2c_write(priv, ICMSR, 0);
>   rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
> @@ -499,11 +505,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter
> *adap, }
> 
>   /* init each data */
> - priv->msg   = &msgs[i];
> - priv->pos   = 0;
> - priv->flags = 0;
> - if (i == num - 1)
> - rcar_i2c_flags_set(priv, ID_LAST_MSG);
> + priv->msg = &msgs[i];
> + priv->msgs_left = num - i;
> 
>   rcar_i2c_prepare_msg(priv);

-- 
Regards,

Laurent Pinchart
[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 4.3.0-08388-ga27958726b59-dirty (laurent@avalon) (gcc version 4.7.3 20130205 (prerelease) (crosstool-NG linaro-1.13.1-4.7-2013.02-01-20130221 - Linaro GCC 2013.02) ) #299 SMP Mon Nov 16 22:58:34 EET 2015
[0.00] CPU: ARMv7 Processor [413fc0f2] revision 2 (ARMv7), cr=30c5387d
[0.00] CPU: PIPT / VIPT nonaliasing data cache, PIPT instruction cache
[0.00] Machine model: Koelsch
[0.00] debug: ignoring loglevel setting.
[0.00] cma: Reserved 256 MiB at 0x7000
[0.00] cma: Reserved 64 MiB at 0x6c00
[0.00] Forcing write-allocate cache policy for SMP
[0.00] Memory policy: Data cache writealloc
[0.00] On node 0 totalpages: 524288
[0.00] free_area_init_node: node 0, pgdat c0688fc0, node_mem_map e7fd8000
[0.00]   DMA zone: 1536 pages used for memmap
[0.00]   DMA zone: 0 pages reserved
[0.00]   DMA zone: 196608 pages, LIFO batch:31
[0.00]   HighMem zone: 327680 pages, LIFO batch:31
[0.00] PERCPU: Embedded 12 pages/cpu @e7f9 s17920 r8192 d23040 u49152
[0.00] pcpu-alloc: s17920 r8192 d23040 u49152 alloc=12*4096
[0.00] pcpu-alloc: [0] 0 [0] 1 
[0.00] Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 522752
[0.00] Kernel command line: ignore_loglevel rw root=/dev/nfs ip=dhcp
[0.00] PID hash table entries: 4096 (order: 2, 16384 bytes)
[0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
[0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
[0.00] Memory: 1742912K/2097152K available (4308K kernel code, 305K rwdata, 1676K rodata, 400K init, 2183K bss, 26560K reserved, 327680K cma-reserved, 1048576K highmem)
[0.00] Virtual kernel memory layout:
[0.00] vector  : 0x - 0x1000   (   4 kB)
[0.00] fixmap  : 0xffc0 - 0xfff0   (3072 kB)
[0.00] vmalloc : 0xf080 - 0xff80   ( 240 MB)
[0.00] lowmem  : 0xc000 - 0xf000   ( 768 MB)
[0.00] pkmap   : 0xbfe0 - 0xc000   (   2 MB)
[0.00] modules : 0xbf00 - 0xbfe0   (  14 MB)
[0.00]   .text : 0xc0008000 - 0xc05e1034   (5989 kB)
[0.00]   .init : 0xc05e2000 - 0xc0646000   ( 400 kB)
[0.00]   .data : 0xc0646000 - 0xc0692794   ( 306 kB)
[0.00].bss : 0xc0695000 - 0xc08b6cdc   (2184 kB)
[0.00] Hierarchical RCU implementation.
[0.00] 	Build-time adjustment of 

Re: [PATCH v2] i2c: rcar: make sure clocks are on when doing hw init

[Laurent Pinchart]

Hi Geert,

On Friday 30 October 2015 09:16:40 Geert Uytterhoeven wrote:
> On Fri, Oct 30, 2015 at 5:35 AM, Laurent Pinchart wrote:
> > [9.641533] Unable to handle kernel paging request at virtual address
> > 
> > [9.648927] pgd = eb2adf40
> > [9.651695] [] *pgd=8040007003, *pmd=6bfde003,
> > *pte=
> > [9.658579] Internal error: Oops: a07 [#1] SMP ARM
> > [9.663475] Modules linked in: rcar_thermal adv7180(+) phy_rcar_gen2
> > soundcore udc_core
> > [9.671719] CPU: 0 PID: 552 Comm: udevd Not tainted 4.3.0-rc7-07332-
> > gfb990fd3ff96 #114
> > [9.679811] Hardware name: Generic R8A7791 (Flattened Device Tree)
> > [9.686125] task: eb11f400 ti: ea92e000 task.ti: ea92e000
> > [9.691653] PC is at rcar_i2c_irq+0xd4/0x3e4
> > [9.696018] LR is at rcar_i2c_write+0x28/0x38
> > [9.700471] pc : []lr : []psr: 8193
> > 
> > This only occurred once, I don't know how to reproduce it.
> 
> Do you still have the kernel binary, so you can see where exactly the crash
> happened?

I haven't kept it, but I'm pretty sure I was using renesas-devel-20151026-
v4.3-rc7 without any additional patch applied.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2] i2c: rcar: make sure clocks are on when doing hw init

[Laurent Pinchart]

 10.048245]  r4:c0689430 r3:eb0ec000
[   10.051920] [] (handle_IPI) from [] 
(gic_handle_irq+0x8c/0x98)
[   10.059656]  r10:f0803000 r9:c063c4e8 r8:eb0edf50 r7:c0656480 r6:c063cd10 
r5:f080200c
[   10.067706]  r4:f0802000 r3:
[   10.071381] [] (gic_handle_irq) from [] 
(__irq_svc+0x40/0x54)
[   10.079029] Exception stack(0xeb0edf50 to 0xeb0edf98)
[   10.084191] df40: 0001  
eb0edfb0 c0022e40
[   10.092551] df60: c067f908 c063c49c   c0637280 c063c4e8 
c063c4f0 eb0edfac
[   10.100909] df80: eb0edfb0 eb0edfa0 c0012444 c0012448 6013 
[   10.107665]  r10:c063c4f0 r8:c0637280 r7:eb0edf84 r6: r5:6013 
r4:c0012448
[   10.115727] [] (arch_cpu_idle) from [] 
(default_idle_call+0x30/0x3c)
[   10.124001] [] (default_idle_call) from [] 
(cpu_startup_entry+0x238/0x3a0)
[   10.132806] [] (cpu_startup_entry) from [] 
(secondary_start_kernel+0x154/0x188)
[   10.142050]  r7:c0689458
[   10.144652] [] (secondary_start_kernel) from [<4000a58c>] 
(0x4000a58c)
[   10.152211]  r5: r4:6b0818c0
[   10.155888] ---[ end Kernel panic - not syncing: Fatal exception in 
interrupt

This only occurred once, I don't know how to reproduce it.

I have also seen different errors:

[1.177690] i2c-rcar e653.i2c: error -16 : 0
[1.182534] adv7511: probe of 2-0039 failed with error -16
[1.188293] i2c-rcar e653.i2c: probed

-110 occurred as well.

This occurred when rebooting the board by pressing the reset button, and if I 
remember correctly moving from renesas-devel-20151013v2-v4.3-rc5 to renesas-
devel-20151026-v4.3-rc7. Even though it occurred several times in a row I 
can't seem to reproduce this issue now I'm afraid.

On Thursday 29 October 2015 23:46:31 Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> Reported-by: Kuninori Morimoto 
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/i2c/busses/i2c-rcar.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 616433d387cdb2..58dbd30c24d1cc 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -612,7 +612,10 @@ static int rcar_i2c_probe(struct platform_device *pdev)
> if (IS_ERR(priv->io))
>   return PTR_ERR(priv->io);
> 
> + pm_runtime_enable(dev);
> + pm_runtime_get_sync(dev);
>   rcar_i2c_init(priv);
> + pm_runtime_put(dev);
> 
>   irq = platform_get_irq(pdev, 0);
>   init_waitqueue_head(&priv->wait);
> @@ -631,22 +634,24 @@ static int rcar_i2c_probe(struct platform_device
> *pdev) dev_name(dev), priv);
>   if (ret < 0) {
>   dev_err(dev, "cannot get irq %d\n", irq);
> - return ret;
> + goto out_pm_disable;
>   }
> 
> - pm_runtime_enable(dev);
>   platform_set_drvdata(pdev, priv);
> 
>   ret = i2c_add_numbered_adapter(adap);
>   if (ret < 0) {
>   dev_err(dev, "reg adap failed: %d\n", ret);
> - pm_runtime_disable(dev);
> - return ret;
> + goto out_pm_disable;
>   }
> 
>   dev_info(dev, "probed\n");
> 
>   return 0;
> +
> + out_pm_disable:
> + pm_runtime_disable(dev);
> + return ret;
>  }
> 
>  static int rcar_i2c_remove(struct platform_device *pdev)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 5/9] i2c: rcar: init new messages in irq

[Laurent Pinchart]

Hi Wolfram,

On Friday 23 October 2015 14:14:39 Wolfram Sang wrote:
> > > :/ Let me know if I can send you debug output.
> > 
> > What's the exact issue ?
> 
> Best report ever: I simply don't get a picture :) No warnings, no
> messages. I think we want to make sure my HDMI->DVI converter is proper,
> though.

Does it work with other HDMI sources ?

> > > > .config attached.
> > > 
> > > That one also probes for me... I only disabled DHCP and added my
> > > initramfs. I patched the fbdev build error out and changed Lager dts to
> > > use i2c-rcar (instead of i2c-sh_mobile).
> 
> I made the ADV7180 and AK4642 built-in and this kernel still probes
> rcar-du on my Lager and Magnus' Koelsch...

It's CONFIG_DRM_I2C_ADV7511 that you need for HDMI output.

I have ADV7180 and AK4642 built as modules in my kernel.

> >   -0 [000] d.h1 1.498063: rcar_i2c_irq: msr 09090909
> >   -0 [000] d.h1 1.498075: rcar_i2c_irq: msr 08080808
> >   -0 [000] d.h1 1.498266: rcar_i2c_irq: msr 07070707
> >   -0 [000] d.h1 1.498348: rcar_i2c_irq: msr 06060606
> >   -0 [000] d.h1 1.498465: rcar_i2c_irq: msr 49494949
> 
> So, here is the NACK bit set. Which does not happen on my Lager and
> Magnus' Koelsch. I am starting to wonder if you have a board with HW
> issues?

It works fine when reverting this patch :-/

> Could it be another I2C device interfering?

I wouldn't rule anything out, but I don't see why reverting this patch would 
help then.

> Do you happen to know if you have an earlier revision of Koelsch?

RTPORC7791SEB00010S
KOELSCH SN.057

I'm not sure if that tells anything about the board revision.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 5/9] i2c: rcar: init new messages in irq

[Laurent Pinchart]

Hi Wolfram,

On Friday 23 October 2015 11:45:00 Wolfram Sang wrote:
> > I had CONFIG_DRM_FBDEV_EMULATION disabled. I've then enabled it but also
> > merged git://people.freedesktop.org/~airlied/linux next in my branch,
> > which probably fixes the problem.
> 
> So, your tree is not strictly renesas-drivers-2015-10-13-v4.3-rc5?

I've tested it on renesas-drivers-2015-10-13-v4.3-rc5 without 
CONFIG_DRM_FBDEV_EMULATION, explaining why it compiled properly for me.

> >> I fixed it locally and will see if I see ADV7511 problems. I cannot
> >> fully test HDMI probably, because it seems that this HDMI->DVI chain I
> >> have does not work.
> > 
> > It's supposed to be supported, so that might be something we need to fix.
> > More work for me \o/ :-)
>
> :/ Let me know if I can send you debug output.

What's the exact issue ?

> >> Okay, so before any 'modetest' activity. Which kernelconfig?
> >> shmobile_defconfig?
> > 
> > .config attached.
> 
> That one also probes for me... I only disabled DHCP and added my initramfs.
> I patched the fbdev build error out and changed Lager dts to use i2c-rcar
> (instead of i2c-sh_mobile).
> 
> Can you enable CONFIG_I2C_DEBUG_CORE in the config, apply this patch,
> and send me the trace output and bootlog?

Please find both attached to this e-mail.

-- 
Regards,

Laurent Pinchart
[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 4.3.0-rc5-04061-gb9653e9c000d-dirty (laurent@avalon) (gcc version 4.7.3 20130205 (prerelease) (crosstool-NG linaro-1.13.1-4.7-2013.02-01-20130221 - Linaro GCC 2013.02) ) #89 SMP Fri Oct 23 13:16:05 EEST 2015
[0.00] CPU: ARMv7 Processor [413fc0f2] revision 2 (ARMv7), cr=30c5387d
[0.00] CPU: PIPT / VIPT nonaliasing data cache, PIPT instruction cache
[0.00] Machine model: Koelsch
[0.00] debug: ignoring loglevel setting.
[0.00] cma: Reserved 256 MiB at 0x7000
[0.00] cma: Reserved 64 MiB at 0x6c00
[0.00] Forcing write-allocate cache policy for SMP
[0.00] Memory policy: Data cache writealloc
[0.00] On node 0 totalpages: 524288
[0.00] free_area_init_node: node 0, pgdat c0678ec0, node_mem_map e7fd8000
[0.00]   DMA zone: 1536 pages used for memmap
[0.00]   DMA zone: 0 pages reserved
[0.00]   DMA zone: 196608 pages, LIFO batch:31
[0.00]   HighMem zone: 327680 pages, LIFO batch:31
[0.00] PERCPU: Embedded 12 pages/cpu @e7f9 s17920 r8192 d23040 u49152
[0.00] pcpu-alloc: s17920 r8192 d23040 u49152 alloc=12*4096
[0.00] pcpu-alloc: [0] 0 [0] 1 
[0.00] Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 522752
[0.00] Kernel command line: ignore_loglevel rw root=/dev/nfs ip=dhcp
[0.00] PID hash table entries: 4096 (order: 2, 16384 bytes)
[0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
[0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
[0.00] Memory: 1742976K/2097152K available (4276K kernel code, 304K rwdata, 1652K rodata, 392K init, 2183K bss, 26496K reserved, 327680K cma-reserved, 1048576K highmem)
[0.00] Virtual kernel memory layout:
[0.00] vector  : 0x - 0x1000   (   4 kB)
[0.00] fixmap  : 0xffc0 - 0xfff0   (3072 kB)
[0.00] vmalloc : 0xf080 - 0xff80   ( 240 MB)
[0.00] lowmem  : 0xc000 - 0xf000   ( 768 MB)
[0.00] pkmap   : 0xbfe0 - 0xc000   (   2 MB)
[0.00] modules : 0xbf00 - 0xbfe0   (  14 MB)
[0.00]   .text : 0xc0008000 - 0xc05d3034   (5933 kB)
[0.00]   .init : 0xc05d4000 - 0xc0636000   ( 392 kB)
[0.00]   .data : 0xc0636000 - 0xc0682364   ( 305 kB)
[0.00].bss : 0xc0685000 - 0xc08a6c9c   (2184 kB)
[0.00] Hierarchical RCU implementation.
[0.00] 	Build-time adjustment of leaf fanout to 32.
[0.00] 	RCU restricting CPUs from NR_CPUS=8 to nr_cpu_ids=2.
[0.00] RCU: Adjusting geometry for rcu_fanout_leaf=32, nr_cpu_ids=2
[0.00] 
[0.00] **
[0.00] **   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **
[0.00] **  **
[0.00] ** trace_printk() being used. Allocating extra memory.  **
[0.00] **  **
[0.00] ** This means that this is a DEBUG kernel and it is **
[0.00] ** unsafe for production use.   **
[0.00] **  **
[0.00] ** If you see this message and you are not debugging**
[0.00] ** the 

Re: [PATCH 5/9] i2c: rcar: init new messages in irq

[Laurent Pinchart]

Hi Wolfram,

On Thursday 22 October 2015 13:05:05 Wolfram Sang wrote:
> On Thu, Oct 22, 2015 at 02:10:52AM +0300, Laurent Pinchart wrote:
> > On Thursday 03 September 2015 22:20:09 Wolfram Sang wrote:
> > > From: Wolfram Sang 
> > > 
> > > Setting up new messages was done in process context while handling a
> > > message was in interrupt context. Because of the HW design, this IP core
> > > is sensitive to timing, so the context switches were too expensive. Move
> > > this setup to interrupt context as well.
> > > 
> > > In my test setup, this fixed the occasional 'data byte sent twice' issue
> > > which a number of people have seen. It also fixes to send REP_START
> > > after a read message which was wrongly send as a STOP + START sequence
> > > before.
> > 
> > I'm afraid this patch has been found by git bisect to break HDMI on
> > Koelsch
> > 
> > :-(
> > 
> > The regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val) call in
> > drivers/gpu/drm/i2c/adv7511.c returns -ENXIO.
> > 
> > Reverting the patch on top of Geert's current drivers master branch fixes
> > the problem.
> 
> But HDMI worked on Koelsch in Dublin??

I know :-)

Do you have a Koelsch board now ? Could you try 
b9653e9c000dc2ebd9c8712442c659ccd1586e22 from Geert's drivers tree ? On my 
board the adv7511 fails to probe completely due to the regmap_read() failure.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 5/9] i2c: rcar: init new messages in irq

[Laurent Pinchart]

   }
> + }
> 
> - ret = i + 1; /* The number of transfer */
> + /* init data */
> + priv->msg = msgs;
> + priv->msgs_left = num;
> +
> + rcar_i2c_prepare_msg(priv);
> +
> + time_left = wait_event_timeout(priv->wait,
> +  rcar_i2c_flags_has(priv, ID_DONE),
> +      num * adap->timeout);
> + if (!time_left) {
> + rcar_i2c_init(priv);
> + ret = -ETIMEDOUT;
> + } else if (rcar_i2c_flags_has(priv, ID_NACK)) {
> + ret = -ENXIO;
> + } else if (rcar_i2c_flags_has(priv, ID_ARBLOST)) {
> + ret = -EAGAIN;
> + } else {
> + ret = num - priv->msgs_left; /* The number of transfer */
>   }
>  out:
>   pm_runtime_put(dev);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 0/9] i2c: rcar: tackle race conditions in the driver

[Laurent Pinchart]

Hi Wolfram,

On Thursday 03 September 2015 22:40:00 Wolfram Sang wrote:
> >> So I refactored the driver to setup new messages in interrupt context,
> >> too. This avoids the race for b) because we are now setting up the new
> >> message before we release the i2c bus clock (before we released the
> >> clock and set up the message in process context).
> > 
> > Could this fix the HDMI EDID read issue on Koelsch ?
> 
> I surely hope so. I can't test because I don't have Koelsch.

I'll test it ASAP, but that might take a while as I'm busy with VSP+DU on Gen3 
now.

> >> c) is also fixed, this was not a race but a bug in the state handling.
> >> a)> however is not fixed 100% :( We have the race window as small as
> >> possible now when utilizing interrupts, so it is an improvement and
> >> worked for my test cases well. There were experiments by me and Renesas
> >> engineers to use polling to prevent the issue but this caused other side
> >> effects, sadly. So, let's improve the situation now and let's see where
> >> we get.
> > 
> > Does that mean that, due to hardware design, it's impossible to use I2C
> > interrupts in a race-free way ? It would be interesting to document why in
> > a commit log message, or possibly in the code itself.
> 
> It can maybe be done when polling. However, this might need busy looping
> for ~1us. Also, the repeated start handling needs to be rewritten and I
> am not sure this goes well with polling.

I meant without polling. Does the hardware design prevent from using I2C in 
interrupt mode in a race-free way ?

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 0/9] i2c: rcar: tackle race conditions in the driver

[Laurent Pinchart]

Hi Wolfram,

On Thursday 03 September 2015 22:20:04 Wolfram Sang wrote:
> Hello RCar Fans!
> 
> Two issues people have seen with the i2c-rcar driver was:
> 
> a) immediately restarted messages after NACK from client
> b) duplicated data bytes in messages
> 
> Some people already worked on those and had a tough time because it was hard
> to reproduce these issues on non-customer setup. Luckily, I somewhen had a
> state where the first transfer after boot would always show the above
> issues on a plain Renesas Lager board. When measuring, I found a third
> issue thanks to my new tool 'i2ctransfer' (and thanks to projects like
> sigrok and OpenLogicSniffer, of course. Thank you very much!):
> 
> c) after read message, no repeated start was sent, but stop + start.
> 
> Due to some unlucky design choices in the IP core, it has some race windows
> which can cause problems if interrupts get delayed. Also, for every new
> message in one transfer, context switches between interrupt and process
> were needed.
> 
> So I refactored the driver to setup new messages in interrupt context, too.
> This avoids the race for b) because we are now setting up the new message
> before we release the i2c bus clock (before we released the clock and set up
> the message in process context).

Could this fix the HDMI EDID read issue on Koelsch ?

> c) is also fixed, this was not a race but a bug in the state handling. a)
> however is not fixed 100% :( We have the race window as small as possible
> now when utilizing interrupts, so it is an improvement and worked for my
> test cases well. There were experiments by me and Renesas engineers to use
> polling to prevent the issue but this caused other side effects, sadly. So,
> let's improve the situation now and let's see where we get.

Does that mean that, due to hardware design, it's impossible to use I2C 
interrupts in a race-free way ? It would be interesting to document why in a 
commit log message, or possibly in the code itself.

> I did quite some lab testing here and also verified that slave support does
> not suffer from these changes. However, I'd really appreciate if people
> could give this real-world-testing which is always different.
> 
> Please have a look, a test, etc...
> 
> Thanks,
> 
>Wolfram
> 
> 
> Wolfram Sang (9):
>   i2c: rcar: rework hw init
>   i2c: rcar: remove unused IOERROR state
>   i2c: rcar: remove spinlock
>   i2c: rcar: refactor setup of a msg
>   i2c: rcar: init new messages in irq
>   i2c: rcar: don't issue stop when HW does it automatically
>   i2c: rcar: check master irqs before slave irqs
>   i2c: rcar: revoke START request early
>   i2c: rcar: clean up after refactoring
> 
>  drivers/i2c/busses/i2c-rcar.c | 193 +++
>  1 file changed, 71 insertions(+), 122 deletions(-)

-- 
Regards,

Laurent Pinchart

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

Re: [RFT 1/2] i2c: rcar: add support for r8a7795 (R-Car H3)

[Laurent Pinchart]

On Thursday 06 August 2015 02:38:49 Wolfram Sang wrote:
> On Thu, Aug 06, 2015 at 03:34:05AM +0300, Laurent Pinchart wrote:
> > Hi Wolfram,
> > 
> > Thank you for the patch.
> > 
> > On Wednesday 05 August 2015 15:18:25 Wolfram Sang wrote:
> > > From: Wolfram Sang 
> > > 
> > > Enable the I2C core for this SoC. I add a new type because this version
> > > has new features (e.g. DMA) which will be added somewhen later.
> > > 
> > > Signed-off-by: Wolfram Sang 
> 
> ...
> 
> > > @@ -625,6 +627,7 @@ static const struct of_device_id rcar_i2c_dt_ids[] =
> > > {
> > >   { .compatible = "renesas,i2c-r8a7792", .data = (void *)I2C_RCAR_GEN2
> > >   },
> > >   { .compatible = "renesas,i2c-r8a7793", .data = (void *)I2C_RCAR_GEN2
> > >   },
> > >   { .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2
> > >   },
> > > + { .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3
> > > },
> > 
> > If H3 is compatible with Gen2, can't you just use I2C_RCAR_GEN2 here ? You
> > can always add I2C_RCAR_GEN3 later if you find differences.
> 
> Is the patch description not clear enough?

My bad. I should go to bed. And so should you by the way ;-)

-- 
Regards,

Laurent Pinchart

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

Re: [RFT 1/2] i2c: rcar: add support for r8a7795 (R-Car H3)

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Wednesday 05 August 2015 15:18:25 Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> Enable the I2C core for this SoC. I add a new type because this version
> has new features (e.g. DMA) which will be added somewhen later.
> 
> Signed-off-by: Wolfram Sang 
> ---
>  Documentation/devicetree/bindings/i2c/i2c-rcar.txt | 1 +
>  drivers/i2c/busses/i2c-rcar.c  | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
> b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt index
> 16b3e07aa98fdd..ea406eb20fa5ad 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
> @@ -10,6 +10,7 @@ Required properties:
>   "renesas,i2c-r8a7792"
>   "renesas,i2c-r8a7793"
>   "renesas,i2c-r8a7794"
> + "renesas,i2c-r8a7795"
>  - reg: physical base address of the controller and length of memory mapped
>region.
>  - interrupts: interrupt specifier.
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index d8361dada58455..3e125654bb288a 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -103,6 +103,7 @@
>  enum rcar_i2c_type {
>   I2C_RCAR_GEN1,
>   I2C_RCAR_GEN2,
> + I2C_RCAR_GEN3,
>  };
> 
>  struct rcar_i2c_priv {
> @@ -178,6 +179,7 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv
> *priv, cdf_width = 2;
>   break;
>   case I2C_RCAR_GEN2:
> + case I2C_RCAR_GEN3:
>   cdf_width = 3;
>   break;
>   default:
> @@ -625,6 +627,7 @@ static const struct of_device_id rcar_i2c_dt_ids[] = {
>   { .compatible = "renesas,i2c-r8a7792", .data = (void *)I2C_RCAR_GEN2 },
>   { .compatible = "renesas,i2c-r8a7793", .data = (void *)I2C_RCAR_GEN2 },
>   { .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
> + { .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },

If H3 is compatible with Gen2, can't you just use I2C_RCAR_GEN2 here ? You can 
always add I2C_RCAR_GEN3 later if you find differences.

>   {},
>  };
>  MODULE_DEVICE_TABLE(of, rcar_i2c_dt_ids);

-- 
Regards,

Laurent Pinchart

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

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

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Friday 17 July 2015 16:08:29 Wolfram Sang wrote:
> Not-Signed-off-by: Wolfram Sang 
> ---
>  arch/arm/boot/dts/tegra124-jetson-tk1.dts | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> b/arch/arm/boot/dts/tegra124-jetson-tk1.dts index
> bd43ed6d6ec7c0..4d5f2a4c4da1ce 100644
> --- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> +++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> @@ -1,5 +1,6 @@
>  /dts-v1/;
> 
> +#include 
>  #include 
>  #include "tegra124.dtsi"
> 
> @@ -1390,6 +1391,12 @@
>   reg = <0x56>;
>   pagesize = <8>;
>   };
> +
> + eeprom@42 {
> + compatible = "linux,slave-24c02";
> + //FIXME: Should be I2C_OWN_SLAVE_ADDRESS | 0x42
> + reg = <0xc042>;

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

> + };
>   };
> 
>   /* Expansion GEN2_I2C_* */

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 3/5] i2c: emev2: add driver

[Laurent Pinchart]

On Saturday 11 July 2015 12:03:13 Wolfram Sang wrote:
> > +   dev_info(&pdev->dev, "Added i2c controller %d irq %d @ 0x%zx\n",
> > +   priv->adap.nr, irq, r->start);
> 
> I removed the address printout entirely. It is part of the dev_*
> printout already and I got a format warning from buildbot.

With that change,

Reviewed-by: Laurent Pinchart 

-- 
Regards,

Laurent Pinchart

--
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/5] i2c: emev2: add binding documentation

[Laurent Pinchart]

On Tuesday 07 July 2015 21:53:13 Wolfram Sang wrote:
> > > I follow what is done for the other EMEV2 IP cores currently supported.
> > > All cores I checked have PCLK and SCLK but only SCLK is referenced in
> > > the DTS.
> > 
> > Could you briefly explain what the two clocks are for ? Or point me to the
> > right documentation ?
> 
> PCLK is the APB clock. See R19UH0037EJ1200_SMU.pdf, chapter 4.13.

Is that the functional clock, while SCLK is the clock used to control serial 
communication ?

-- 
Regards,

Laurent Pinchart

--
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/5] i2c: emev2: add binding documentation

[Laurent Pinchart]

Hi Wolfram,

On Tuesday 07 July 2015 21:43:13 Wolfram Sang wrote:
> > > +Device tree configuration for Renesas EMEV2 IIC driver
> > 
> > s/driver/controller/
> 
> Yes,
> 
> > > +- interrupts  : 1 interrupt
> > 
> > I'd write this as "specifier for the IIC controller interrupt".
> 
> Yes.
> 
> > > +- clocks  : phandle to the IP core SCLK
> > 
> > The datasheet mentions PCLK and IIC_SCLK but doesn't provide details. Do
> > you have more information ?
> 
> I follow what is done for the other EMEV2 IP cores currently supported.
> All cores I checked have PCLK and SCLK but only SCLK is referenced in
> the DTS.

Could you briefly explain what the two clocks are for ? Or point me to the 
right documentation ?

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 3/5] i2c: emev2: add driver

[Laurent Pinchart]

}
> +
> + /* Write data */
> + writeb(msg->buf[count], priv->base + I2C_OFS_IIC0);
> + }
> +
> + /* Wait for R/W transaction */
> + status = em_i2c_wait_for_event(priv);
> + if (status < 0)
> + goto out_reset;
> + }
> +
> + if (stop)
> + em_i2c_stop(priv);
> +
> + return count;
> +
> +out_reset:
> + em_i2c_reset(adap);
> +out:
> + return status < 0 ? status : -ENXIO;
> +}
> +
> +static int em_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
> + struct em_i2c_device *priv = i2c_get_adapdata(adap);
> + int ret, i;
> +
> + if (readb(priv->base + I2C_OFS_IICF0) & I2C_BIT_IICBSY)
> + return -EAGAIN;
> +
> + for (i = 0; i < num; i++) {
> + ret = __em_i2c_xfer(adap, &msgs[i], (i == (num - 1)));
> + if (ret < 0)
> + return ret;
> + }
> +
> + /* I2C transfer completed */
> + return num;
> +}
> +
> +static irqreturn_t em_i2c_irq_handler(int this_irq, void *dev_id)
> +{
> + struct em_i2c_device *priv = dev_id;
> +
> + complete(&priv->msg_done);
> + return IRQ_HANDLED;
> +}
> +
> +static u32 em_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm em_i2c_algo = {
> + .master_xfer = em_i2c_xfer,
> + .functionality = em_i2c_func,
> +};
> +
> +static int em_i2c_probe(struct platform_device *pdev)
> +{
> + struct em_i2c_device *priv;
> + struct resource *r;
> + int irq, ret;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(struct em_i2c_device), 
GFP_KERNEL);

I'd use sizeof(*priv).

> + if (!priv)
> + return -ENOMEM;
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->base = devm_ioremap_resource(&pdev->dev, r);
> + if (IS_ERR(priv->base))
> + return PTR_ERR(priv->base);
> +
> + strlcpy(priv->adap.name, "EMEV2 I2C", sizeof(priv->adap.name));
> +
> + priv->sclk = devm_clk_get(&pdev->dev, "sclk");
> + if (IS_ERR(priv->sclk))
> + return PTR_ERR(priv->sclk);
> +
> + clk_prepare_enable(priv->sclk);
> +
> + irq = platform_get_irq(pdev, 0);

I'd move this call right before devm_request_irq() below. devm_request_irq() 
should handle invalid IRQs, but won't print an error message. I'd let you 
decide whether that's a problem.

> + priv->adap.timeout = msecs_to_jiffies(100);
> + priv->adap.retries = 5;

Is there a particular reason for setting the number of retries to 5 ?

> + priv->adap.dev.parent = &pdev->dev;
> + priv->adap.algo = &em_i2c_algo;
> + priv->adap.owner = THIS_MODULE;
> + priv->adap.dev.of_node = pdev->dev.of_node;
> +
> + init_completion(&priv->msg_done);
> +
> + platform_set_drvdata(pdev, priv);
> + i2c_set_adapdata(&priv->adap, priv);
> +
> + em_i2c_reset(&priv->adap);
> +
> + ret = devm_request_irq(&pdev->dev, irq, em_i2c_irq_handler, 0,
> + "em_i2c", priv);
> + if (ret)
> + goto exit_clk;

Nitpicking, I'd call this error_clk to show that the label is used in case of 
error only. You could also just call it error as there's no other error-
related label.

> +
> + ret = i2c_add_adapter(&priv->adap);
> +
> + if (ret)
> + goto exit_clk;
> +
> + dev_info(&pdev->dev, "Added i2c controller %d irq %d @ 0x%p\n",
> + priv->adap.nr, irq, priv->base);

Is priv->base useful here ? The physical address of the registers block could 
be, but its kernel virtual address doesn't seem very interesting to me.

> +
> + return 0;
> +
> +exit_clk:
> + clk_disable_unprepare(priv->sclk);
> + return ret;
> +}
> +
> +static int em_i2c_remove(struct platform_device *dev)
> +{
> + struct em_i2c_device *priv = platform_get_drvdata(dev);
> +
> + i2c_del_adapter(&priv->adap);
> + clk_disable_unprepare(priv->sclk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id em_i2c_ids[] = {
> + { .compatible = "renesas,iic-emev2", },
> + { }
> +};
> +
> +static struct platform_driver em_i2c_driver = {
> + .probe = em_i2c_probe,
> + .remove = em_i2c_remove,
> + .driver = {
> + .name = "em-i2c",
> + .of_match_table = em_i2c_ids,
> + }
> +};
> +module_platform_driver(em_i2c_driver);
> +
> +MODULE_DESCRIPTION("EMEV2 I2C bus driver");
> +MODULE_AUTHOR("Ian Molton and Wolfram Sang ");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DEVICE_TABLE(of, em_i2c_ids);

-- 
Regards,

Laurent Pinchart

--
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/5] i2c: emev2: add binding documentation

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Monday 06 July 2015 23:46:06 Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> Signed-off-by: Wolfram Sang 
> ---
>  .../devicetree/bindings/i2c/i2c-emev2.txt  | 22 ++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt new file mode 100644
> index 00..a6740066835056
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-emev2.txt
> @@ -0,0 +1,22 @@
> +Device tree configuration for Renesas EMEV2 IIC driver

s/driver/controller/

> +
> +Required properties:
> +- compatible  : "renesas,iic-". "renesas,iic-emev2" as
> fallback
> +- reg : address start and address range size of device
> +- interrupts  : 1 interrupt

I'd write this as "specifier for the IIC controller interrupt".

> +- clocks  : phandle to the IP core SCLK

The datasheet mentions PCLK and IIC_SCLK but doesn't provide details. Do you 
have more information ?

> +- clock-names : must be "sclk"
> +- #address-cells  : should be <1>
> +- #size-cells : should be <0>
> +
> +Example:
> +
> + iic0: i2c@e007 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "renesas,iic-emev2";
> + reg = <0xe0070000 0x28>;
> + interrupts = <0 32 IRQ_TYPE_EDGE_RISING>;
> + clocks = <&iic0_sclk>;
> + clock-names = "sclk";
> + };

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 0/4] generic timeout handling for Renesas drivers

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patches.

On Saturday 20 June 2015 21:03:18 Wolfram Sang wrote:
> Here is a small patch series to make I2C timeout handling easier for users.
> It is not so amazingly huge anymore and it can be modified via i2c-dev if
> wanted. While at it, fix the type of the timeout variable to the proper
> one. This time build-tested twice!
> 
> Wolfram Sang (4):
>   i2c: rcar: use adapter default for timeout
>   i2c: rcar: use proper type for timeout
>   i2c: sh_mobile: use adapter default for timeout
>   i2c: sh_mobile: use proper type for timeout

For the whole series,

Acked-by: Laurent Pinchart 

>  drivers/i2c/busses/i2c-rcar.c  | 5 +++--
>  drivers/i2c/busses/i2c-sh_mobile.c | 9 +
>  2 files changed, 8 insertions(+), 6 deletions(-)

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in

Re: [RFC PATCH 2/2] i2c: mux: pca954x: Use __i2c_transfer because of quirks

[Laurent Pinchart]

Hi Alexander,

Thank you for the patch.

On Friday 12 June 2015 14:41:00 Alexander Sverdlin wrote:
> pca9541 and pca954x are calling master_xfer() of the parent adapter directly
> thus bypassing the quirks checks of the adapter. Use __i2c_transfer()
> instead.
> 
> Signed-off-by: Alexander Sverdlin 
> Tested-by: Łukasz Gemborowski 

Acked-by: Laurent Pinchart 

> ---
>  drivers/i2c/muxes/i2c-mux-pca9541.c |4 ++--
>  drivers/i2c/muxes/i2c-mux-pca954x.c |2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c
> b/drivers/i2c/muxes/i2c-mux-pca9541.c index cb77277..0c8d4d2 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -104,7 +104,7 @@ static int pca9541_reg_write(struct i2c_client *client,
> u8 command, u8 val) buf[0] = command;
>   buf[1] = val;
>   msg.buf = buf;
> - ret = adap->algo->master_xfer(adap, &msg, 1);
> + ret = __i2c_transfer(adap, &msg, 1);
>   } else {
>   union i2c_smbus_data data;
> 
> @@ -144,7 +144,7 @@ static int pca9541_reg_read(struct i2c_client *client,
> u8 command) .buf = &val
>   }
>   };
> - ret = adap->algo->master_xfer(adap, msg, 2);
> + ret = __i2c_transfer(adap, msg, 2);
>   if (ret == 2)
>   ret = val;
>   else if (ret >= 0)
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> b/drivers/i2c/muxes/i2c-mux-pca954x.c index bea0d2d..ea4aa9d 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -134,7 +134,7 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
>   msg.len = 1;
>   buf[0] = val;
>   msg.buf = buf;
> - ret = adap->algo->master_xfer(adap, &msg, 1);
> + ret = __i2c_transfer(adap, &msg, 1);
>   } else {
>   union i2c_smbus_data data;
>   ret = adap->algo->smbus_xfer(adap, client->addr,

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 0/2] ARM: shmobile: r8a7740: move I2C errata handling into the driver

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patches.

On Tuesday 09 June 2015 16:14:38 Wolfram Sang wrote:
> This series moves the errata handling from the setup function into the
> driver. It has been tested that this changeset does not cause a regression
> on the armadillo board we had access to. Unfortunately, this board did not
> need the workaround at all, only some revisions of the r8a7740 are
> affected.
> 
> After a short discussion with Simon, we agreed this should go via I2C.

Looks good to me.

Acked-by: Laurent Pinchart 

> Wolfram Sang (2):
>   i2c: sh_mobile: add errata workaround
>   ARM: shmobile: r8a7740: remove I2C errata handling
> 
>  arch/arm/mach-shmobile/setup-r8a7740.c | 55 ---
>  drivers/i2c/busses/i2c-sh_mobile.c | 40 +
>  2 files changed, 40 insertions(+), 55 deletions(-)

-- 
Regards,

Laurent Pinchart

--
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: Bug in i2c-core?

[Laurent Pinchart]

Hi Wolfram,

On Sunday 08 March 2015 09:26:17 Wolfram Sang wrote:
> On Wed, Mar 04, 2015 at 09:22:37AM +0100, Wolfram Sang wrote:
> >>>> I am writing an I2C touchscreen driver for an i.MX6 based board. I
> >>>> compiled it as a module and when I unload it, I get the following
> >>>> warning:
> >>>> 
> >>>> # modprobe sx8654
> >>>> [   46.261494] input: SX8654 I2C Touchscreen as
> >>>> /devices/soc0/soc/210.aips-bus/21a.i2c/i2c-0/0-0048/input/in
> >>>> put1
> >>>> # rmmod sx8654
> >>>> [   76.435223] [ cut here ]
> >>>> [   76.439909] WARNING: CPU: 0 PID: 134 at fs/proc/generic.c:552
> >>>> remove_proc_entry+0x148/0x164()
> >>>> [   76.448582] remove_proc_entry: removing non-empty directory
> >>>> 'irq/208', leaking at least 'sx8654'
> >>> 
> >>> ...
> >>> 
> >>>> When I revert commit e4df3a0 (i2c: core: Dispose OF IRQ mapping at
> >>>> client removal time) I don't get the warning.
> >>>> 
> >>>> Is this a bug in the i2c-core or am I doing something wrong in my
> >>>> driver?
> >>> 
> >>> Yes, this commit breaks all drivers using devm* for IRQ management on
> >>> OF-based systemsi because devm* cleanup happens in device code, after
> >>> bus's remove() method returns. I'd recommend reverting and finding a
> >>> better way (making cleanup a custom devm action as well?).
> >> 
> >> Ouch, my bad.
> >> 
> >> Wolfram, any opinion ? The original patch fixes a real bug, so we
> >> shouldn't just revert it.
> > 
> > Looking at it some more: What bug does it fix? Anything you experienced?

Good question, and I have to confess that I don't really remember :-/

> > I wonder if we really need e4df3a0 because I can't see where
> > platform_get_irq, the major user of of_irq_get, disposes the mapping.
> > irq_create_of_mapping() will return an already assigned mapping if
> > called twice.

I've reached the same conclusion after reading the code. I was concerned about 
resource leakage, but that doesn't seem to be an issue.

> > I don't know yet, though, if mappings are static or if a mapping can be
> > routed to another irq controller over some time because theoretically they
> > can be dynamically added/removed.
> > 
> > Adding Rob to CC as he wrote of_irq_get and put it into
> > platform_get_irq. Rob, we use of_irq_get() in the I2C core and the
> > question is now if we need to dispose the mapping and if so what would
> > be a good place for it so managed devices will not have their mappings
> > removed before the managed irq is removed.
> 
> Ping. Just so you know: Without further information, I will revert the
> patch in question around rc4/rc5. I'd still like to know if the
> non-disposing of the mapping in platform_get_irq() is intentional.

I'll defer that to Rob. I'm fine with the revert at the moment.

-- 
Regards,

Laurent Pinchart

--
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: pca954x: Defer probing due to reset GPIO

[Laurent Pinchart]

Hi Uwe,

On Thursday 05 March 2015 09:53:53 Uwe Kleine-König wrote:
> On Wed, Mar 04, 2015 at 04:44:28PM +0200, Laurent Pinchart wrote:
> > If a reset GPIO is specified but not available we must defer probing, as
> > the device could be held in reset. Use devm_gpiod_request_optional() to
> > handle this automatically.
> > 
> > Signed-off-by: Laurent Pinchart 
> 
> I sent the same patch with a slightly different motivation two weeks
> ago: http://article.gmane.org/gmane.linux.drivers.i2c/21972

And I've even acked it... Well, I guess it means I really like the patch ;-) 
Let's get your version merged.

-- 
Regards,

Laurent Pinchart

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

[PATCH] i2c: pca954x: Defer probing due to reset GPIO

[Laurent Pinchart]

If a reset GPIO is specified but not available we must defer probing, as
the device could be held in reset. Use devm_gpiod_request_optional() to
handle this automatically.

Signed-off-by: Laurent Pinchart 
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c 
b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 3d8f4fe..bea0d2d 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -204,9 +204,9 @@ static int pca954x_probe(struct i2c_client *client,
i2c_set_clientdata(client, data);
 
/* Get the mux out of reset if a reset GPIO is specified. */
-   gpio = devm_gpiod_get(&client->dev, "reset");
-   if (!IS_ERR(gpio))
-   gpiod_direction_output(gpio, 0);
+   gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW);
+   if (IS_ERR(gpio))
+   return PTR_ERR(gpio);
 
/* Write the mux register at addr to verify
 * that the mux is in fact present. This also
-- 
Regards,

Laurent Pinchart

--
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: Bug in i2c-core?

[Laurent Pinchart]

Hi Dmitry,

On Friday 27 February 2015 08:59:44 Dmitry Torokhov wrote:
> On Fri, Feb 27, 2015 at 12:09:51PM +0100, Sébastien SZYMANSKI wrote:
> > Hi,
> > 
> > I am writing an I2C touchscreen driver for an i.MX6 based board. I
> > compiled it as a module and when I unload it, I get the following warning:
> > 
> > # modprobe sx8654
> > [   46.261494] input: SX8654 I2C Touchscreen as
> > /devices/soc0/soc/210.aips-bus/21a.i2c/i2c-0/0-0048/input/input1
> > # rmmod sx8654
> > [   76.435223] [ cut here ]
> > [   76.439909] WARNING: CPU: 0 PID: 134 at fs/proc/generic.c:552
> > remove_proc_entry+0x148/0x164()
> > [   76.448582] remove_proc_entry: removing non-empty directory
> > 'irq/208', leaking at least 'sx8654'
> 
> ...
> 
> > When I revert commit e4df3a0 (i2c: core: Dispose OF IRQ mapping at
> > client removal time) I don't get the warning.
> > 
> > Is this a bug in the i2c-core or am I doing something wrong in my driver?
> 
> Yes, this commit breaks all drivers using devm* for IRQ management on
> OF-based systemsi because devm* cleanup happens in device code, after
> bus's remove() method returns. I'd recommend reverting and finding a
> better way (making cleanup a custom devm action as well?).

Ouch, my bad.

Wolfram, any opinion ? The original patch fixes a real bug, so we shouldn't 
just revert it.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 1/2] DT: i2c: Deprecate adi,adxl34x compatible string

[Laurent Pinchart]

(CC'ing Dmitry)

On Monday 02 March 2015 07:40:49 Wolfram Sang wrote:
> On Thu, Feb 26, 2015 at 04:27:49PM +0200, Laurent Pinchart wrote:
> > On Monday 26 January 2015 13:09:47 Wolfram Sang wrote:
> >>>>> If you drop adi,adxl346, checkpatch will start complaining if it
> >>>>> encounters it in a .dts.
> >>>> 
> >>>> Boah, this is annoying. That means we need an 346 entry even if it
> >>>> is not different from 345 (which is fine by me).
> >>>
> >>> To be clear: you need the entry in the documentation. It can be
> >>> omitted from the driver if it's not (yet) used for matching.
> >> 
> >> Well, I don't really like that behaviour, but I don't like
> >> i2c/trivial-devices.txt as well, so I'll just apply the patch and live
> >> with it :)
> > 
> > What happened to this patch ?
> 
> My idea was that Dmitry takes them both because they are related:
> 
> http://article.gmane.org/gmane.linux.drivers.i2c/21763

I'm fine with that. Dmitry ?

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 1/2] DT: i2c: Deprecate adi,adxl34x compatible string

[Laurent Pinchart]

Hi Wolfram,

On Monday 26 January 2015 13:09:47 Wolfram Sang wrote:
> > >> If you drop adi,adxl346, checkpatch will start complaining if it
> > >> encounters
> > >> it in a .dts.
> > > 
> > > Boah, this is annoying. That means we need an 346 entry even if it is
> > > not different from 345 (which is fine by me).
> > 
> > To be clear: you need the entry in the documentation. It can be omitted
> > from the driver if it's not (yet) used for matching.
> 
> Well, I don't really like that behaviour, but I don't like
> i2c/trivial-devices.txt as well, so I'll just apply the patch and live
> with it :)

What happened to this patch ?

-- 
Regards,

Laurent Pinchart

--
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: pca954x: improve usage of gpiod API

[Laurent Pinchart]

Hi Uwe,

Thank you for the patch.

On Tuesday 17 February 2015 10:12:08 Uwe Kleine-König wrote:
> Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
> which appeared in v3.17-rc1, the gpiod_get* functions take an additional
> parameter that allows to specify direction and initial value for
> outputs.
> 
> Also there is an *_optional variant that serves well here.  The sematics
> is slightly changed here by using it. Now if a reset gpio is specified
> and getting hold on it fails, pca954x_probe fails, too.
> 
> Signed-off-by: Uwe Kleine-König 

Acked-by: Laurent Pinchart 

> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b404b433..0042cf3d77db
> 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -201,9 +201,9 @@ static int pca954x_probe(struct i2c_client *client,
>   i2c_set_clientdata(client, data);
> 
>   /* Get the mux out of reset if a reset GPIO is specified. */
> - gpio = devm_gpiod_get(&client->dev, "reset");
> - if (!IS_ERR(gpio))
> - gpiod_direction_output(gpio, 0);
> + gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(gpio))
> + return PTR_ERR(gpio);
> 
>   /* Write the mux register at addr to verify
>* that the mux is in fact present. This also

-- 
Regards,

Laurent Pinchart

--
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: slave-eeprom: fix boundary check when using sysfs

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Monday 19 January 2015 17:22:55 Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> Due to a copy&paste error, the last byte of the shared memory was not
> accessible via sysfs.
> 
> Reported-by: Debora Grosse 
> Signed-off-by: Wolfram Sang 

Acked-by: Laurent Pinchart 

> ---
>  drivers/i2c/i2c-slave-eeprom.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
> index f432f81b0414..8f417e9054ac 100644
> --- a/drivers/i2c/i2c-slave-eeprom.c
> +++ b/drivers/i2c/i2c-slave-eeprom.c
> @@ -74,7 +74,7 @@ static ssize_t i2c_slave_eeprom_bin_read(struct file
> *filp, struct kobject *kobj struct eeprom_data *eeprom;
>   unsigned long flags;
> 
> - if (off + count >= attr->size)
> + if (off + count > attr->size)
>   return -EFBIG;
> 
>   eeprom = dev_get_drvdata(container_of(kobj, struct device, kobj));
> @@ -92,7 +92,7 @@ static ssize_t i2c_slave_eeprom_bin_write(struct file
> *filp, struct kobject *kob struct eeprom_data *eeprom;
>   unsigned long flags;
> 
> - if (off + count >= attr->size)
> + if (off + count > attr->size)
>       return -EFBIG;
> 
>   eeprom = dev_get_drvdata(container_of(kobj, struct device, kobj));

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 2/2] input: adxl34x: Add OF match support

[Laurent Pinchart]

Hi Dmitry,

On Thursday 15 January 2015 13:50:53 Dmitry Torokhov wrote:
> On Thu, Jan 15, 2015 at 11:34:00PM +0200, Laurent Pinchart wrote:
> > On Thursday 15 January 2015 13:06:32 Dmitry Torokhov wrote:
> >> On Thu, Jan 15, 2015 at 10:34:29PM +0200, Laurent Pinchart wrote:
> >>> On Thursday 15 January 2015 21:00:37 Geert Uytterhoeven wrote:
> >>>> On Thu, Jan 15, 2015 at 7:54 PM, Dmitry Torokhov wrote:
> >>>>> I still do not understand what we are trying to fix here. Why is
> >>>>> "adi,adxl34x" compatible string no good anymore? If we start using
> >>>>> exact models and the physical device does not match do we abort
> >>>>> probe? What is the problem that we are solving here?
> >>>> 
> >>>> Because there's no guarantee that the driver actually supports all
> >>>> "adi,adxl34" with  = 0..9, some of which don't exist yet.
> >> 
> >> So, what? When we encounter such devices and decide that we actually
> >> want a different driver for them (instead of enhancing the existing one)
> >> we'll give them their own compatible string. It's not like "adi,adxl348"
> >> will automatically match with "adi,adxl34x", is it?
> > 
> > Please remember that compatible strings shouldn't be decided based on a
> > particular operating system implementation.
> 
> In this case we can never have generic compatible strings of whatever
> since in 10 years there might appear an OS that handles them
> differently. Let's be a bit realistic here as well.

I don't agree with you. The generic compatible strings should express 
compatibility with a hardware interface to the system, not compatibility with 
particular drivers on particular operating systems. We can thus have generic 
compatible strings without taking the OS into account.

> >>> That's one of the reasons. Another one is that the adxl34x driver
> >>> won't match DT nodes that list the "adi,adxl34x" compatible value in
> >>> positions other than the first.
> >> 
> >> Will it match anything else in the position other than 1st (i.e.
> >> if device has compatible sting like "adi,adxl345-1", "adi,adxl345")?
> > > Why "adi,adxl34x" is special?
> > 
> > The problem on the driver side is that the automatic I2C DT compatible to
> > device name matching implementation only tries to match with the first
> > compatible string without looking at the other ones. The driver thus needs
> > to be enhanced with an explicit OF match table to be able to match on DT
> > nodes that specify "adi,adxl345" or "adi,adxl346" as the first compatible
> > entry.
>
> Why don't we enhance I2C core instead to do proper matching?

That would also be possible, but I believe that patch 1/2 is still the right 
thing to do, in which case patch 2/2 is required anyway.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 2/2] input: adxl34x: Add OF match support

[Laurent Pinchart]

On Thursday 15 January 2015 13:06:32 Dmitry Torokhov wrote:
> On Thu, Jan 15, 2015 at 10:34:29PM +0200, Laurent Pinchart wrote:
> > On Thursday 15 January 2015 21:00:37 Geert Uytterhoeven wrote:
> > > On Thu, Jan 15, 2015 at 7:54 PM, Dmitry Torokhov wrote:
> > > > I still do not understand what we are trying to fix here. Why is
> > > > "adi,adxl34x" compatible string no good anymore? If we start using
> > > > exact models and the physical device does not match do we abort probe?
> > > > What is the problem that we are solving here?
> > > 
> > > Because there's no guarantee that the driver actually supports all
> > > "adi,adxl34" with  = 0..9, some of which don't exist yet.
> 
> So, what? When we encounter such devices and decide that we actually
> want a different driver for them (instead of enhancing the existing one)
> we'll give them their own compatible string. It's not like "adi,adxl348"
> will automatically match with "adi,adxl34x", is it?

Please remember that compatible strings shouldn't be decided based on a 
particular operating system implementation.

> > That's one of the reasons. Another one is that the adxl34x driver won't
> > match DT nodes that list the "adi,adxl34x" compatible value in positions
> > other than the first.
> 
> Will it match anything else in the position other than 1st (i.e.
> if device has compatible sting like "adi,adxl345-1", "adi,adxl345")?
> Why "adi,adxl34x" is special?

The problem on the driver side is that the automatic I2C DT compatible to 
device name matching implementation only tries to match with the first 
compatible string without looking at the other ones. The driver thus needs to 
be enhanced with an explicit OF match table to be able to match on DT nodes 
that specify "adi,adxl345" or "adi,adxl346" as the first compatible entry.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 1/2] DT: i2c: Deprecate adi,adxl34x compatible string

[Laurent Pinchart]

Hi Wolfram,

On Thursday 15 January 2015 18:43:33 Wolfram Sang wrote:
> On Thu, Jan 15, 2015 at 06:32:31PM +0100, Geert Uytterhoeven wrote:
> > On Thu, Jan 15, 2015 at 6:02 PM, Wolfram Sang  wrote:
> >>> --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> >>> +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> >>> @@ -18,8 +18,7 @@ adi,adt7475 +/-1C TDM Extended Temp Range I.C
> >>> 
> >>>  adi,adt7476  +/-1C TDM Extended Temp Range I.C
> >>>  adi,adt7490  +/-1C TDM Extended Temp Range I.C
> >>>  adi,adxl345  Three-Axis Digital Accelerometer
> >>> 
> >>> -adi,adxl346  Three-Axis Digital Accelerometer
> >>> -adi,adxl34x  Three-Axis Digital Accelerometer
> >>> +adi,adxl346  Three-Axis Digital Accelerometer
> >>> (backward-compatibility value "adi,adxl345" must be listed too)
> >>
> >> I'd rather drop 346 because there is no compatible for that one
> >> anywhere. No need to resend, I can fix it here...
> > 
> > If you drop adi,adxl346, checkpatch will start complaining if it
> > encounters it in a .dts.
> 
> Boah, this is annoying. That means we need an 346 entry even if it is
> not different from 345 (which is fine by me).
> 
> If checkpatch does it this way, that means the rule of thumb is to
> *always* have a dedicated compatible entry? Can someone confirm this?

I believe we should register a new compatible entry for a device when the 
device isn't identical, from a compatibility point of view, to an already 
registered device. In this specific case the adxl346 offers addition features 
compared to the adxl345. It thus qualify for its own compatible string. Its DT 
nodes should list both the adi,adxl346 and adi,adxl345 compatible strings in 
that order, as the chip is compatible with the adxl345. On the driver side, 
given that we don't need to differentiate between the devices based on the 
compatible string (as runtime model detection is possible) we don't need to 
add a match entry for adi,adxl346.

> Why did we discuss then? Now, I am confused as well...

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 2/2] input: adxl34x: Add OF match support

[Laurent Pinchart]

On Thursday 15 January 2015 21:00:37 Geert Uytterhoeven wrote:
> On Thu, Jan 15, 2015 at 7:54 PM, Dmitry Torokhov wrote:
> > I still do not understand what we are trying to fix here. Why is
> > "adi,adxl34x" compatible string no good anymore? If we start using exact
> > models and the physical device does not match do we abort probe? What is
> > the problem that we are solving here?
> 
> Because there's no guarantee that the driver actually supports all
> "adi,adxl34" with  = 0..9, some of which don't exist yet.

That's one of the reasons. Another one is that the adxl34x driver won't match 
DT nodes that list the "adi,adxl34x" compatible value in positions other than 
the first.

-- 
Regards,

Laurent Pinchart

--
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 1/2] DT: i2c: Deprecate adi,adxl34x compatible string

[Laurent Pinchart]

DT nodes should use the more specific adi,adxl345 and adi,adxl346
compatible values instead. As the ADXL346 is backward-compatible with
the ADXL345, ADXL346 nodes must list both adi,adxl346 and adi,adxl345,
in that order.

Signed-off-by: Laurent Pinchart 
---
 Documentation/devicetree/bindings/i2c/trivial-devices.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt 
b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
index 9f41d05be3be..757e42510517 100644
--- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
@@ -18,8 +18,7 @@ adi,adt7475   +/-1C TDM Extended Temp Range I.C
 adi,adt7476+/-1C TDM Extended Temp Range I.C
 adi,adt7490+/-1C TDM Extended Temp Range I.C
 adi,adxl345Three-Axis Digital Accelerometer
-adi,adxl346Three-Axis Digital Accelerometer
-adi,adxl34xThree-Axis Digital Accelerometer
+adi,adxl346Three-Axis Digital Accelerometer 
(backward-compatibility value "adi,adxl345" must be listed too)
 at,24c08   i2c serial eeprom  (24cxx)
 atmel,24c00i2c serial eeprom  (24cxx)
 atmel,24c01i2c serial eeprom  (24cxx)
-- 
2.0.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

[PATCH v2 2/2] input: adxl34x: Add OF match support

[Laurent Pinchart]

The I2C subsystem can match devices without explicit OF support based on
the part of their compatible property after the comma. However, this
mechanism uses the first compatible value only. For adxl34x OF device
nodes the compatible property will contain the more specific
"adi,adxl345" or "adi,adxl346" value first. This prevents the device
node from being matched with the adxl34x driver.

Fix this by adding an OF match table with an "adi,adxl345" compatible
entry. There's no need to add the "adi,adxl346" entry as the ADXL346 is
backward-compatible with the ADXL345 with differences handled by runtime
detection of the device model.

Signed-off-by: Laurent Pinchart 
---
 drivers/input/misc/adxl34x-i2c.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/drivers/input/misc/adxl34x-i2c.c b/drivers/input/misc/adxl34x-i2c.c
index 416f47ddcc90..1e028f1f221c 100644
--- a/drivers/input/misc/adxl34x-i2c.c
+++ b/drivers/input/misc/adxl34x-i2c.c
@@ -10,6 +10,7 @@
 #include/* BUS_I2C */
 #include 
 #include 
+#include 
 #include 
 #include 
 #include "adxl34x.h"
@@ -137,11 +138,31 @@ static const struct i2c_device_id adxl34x_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, adxl34x_id);
 
+#ifdef CONFIG_OF
+static const struct of_device_id adxl34x_of_id[] = {
+   /*
+* The ADXL346 is backward-compatible with the ADXL345. Differences are
+* handled by runtime detection of the device model, there's thus no
+* need for listing the "adi,adxl346" compatible value explicitly.
+*/
+   { .compatible = "adi,adxl345", },
+   /*
+* Deprecated, DT nodes should use one or more of the device-specific
+* compatible values "adi,adxl345" and "adi,adxl346".
+*/
+   { .compatible = "adi,adxl34x", },
+   { }
+};
+
+MODULE_DEVICE_TABLE(of, adxl34x_of_id);
+#endif
+
 static struct i2c_driver adxl34x_driver = {
.driver = {
.name = "adxl34x",
.owner = THIS_MODULE,
.pm = &adxl34x_i2c_pm,
+   .of_match_table = of_match_ptr(adxl34x_of_id),
},
.probe= adxl34x_i2c_probe,
.remove   = adxl34x_i2c_remove,
-- 
2.0.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

[PATCH v2 0/2] Fix OF match for adxl34x driver

[Laurent Pinchart]

Hello,

This patch set fixes OF matching for the adxl34x driver when the DT node lists
a device-specific "adi,adxl345" or "adi,adxl346" compatible value first.

The first version (see http://www.spinics.net/lists/linux-i2c/msg18107.html)
added an OF match entry for the "adi,adxl34x" compatible string. The
discussion that followed concluded that that compatible string should be
deprecated and that the driver should match the device-specific strings
instead.

The first patch thus deprecates the "adi,adxl34x" compatible string by
removing it the DT trivial devices list, and the second patch then adds an OF
match table to the adxl34x driver.

Laurent Pinchart (2):
  DT: i2c: Deprecate adi,adxl34x compatible string
  input: adxl34x: Add OF match support

 .../devicetree/bindings/i2c/trivial-devices.txt |  3 +--
 drivers/input/misc/adxl34x-i2c.c| 21 +
 2 files changed, 22 insertions(+), 2 deletions(-)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2] of: i2c: Add idle-disconnect DT property to PCA954x mux driver

[Laurent Pinchart]

Hi Wolfram,

On Thursday 15 January 2015 14:19:35 Wolfram Sang wrote:
> On Thu, Jan 15, 2015 at 02:09:09PM +0100, Alexander Sverdlin wrote:
> > On 15/01/15 13:32, ext Wolfram Sang wrote:
> >> On Fri, Dec 19, 2014 at 06:00:10PM +0100, Alexander Sverdlin wrote:
> >>> of: i2c: Add idle-disconnect DT property to PCA954x mux driver
> >>> 
> >>> Add idle-disconnect device tree property to PCA954x mux driver. The new
> >>> property forces the multiplexer to disconnect child buses in idle
> >>> state. This is used, for example, when there are several multiplexers
> >>> on the same bus and the devices on the underlying buses might have
> >>> same I2C addresses.
> >> 
> >> Basically OK. Question to DT maintainers: "idle-disconnect",
> >> "i2c-mux-idle-disconnect", or is there another existing binding we could
> >> use?
> >> 
> >>> At the same time old (and not used in the tree) platform data binding
> >>> deselect_on_exit is removed to simplify the implementation. Old binding
> >>> has different (per-channel) semantics and doesn't fit well in the new
> >>> concept.
> >> 
> >> I'd prefer to keep it. It should be only one || more. It is not really
> >> in the way IMO.
> > 
> > It complicates the implementation 3x times :) This is part of our
> > discussion with Laurent:
>
> Does it? I don't want DT and platform_data to behave equally. I just
> want to keep being backwards compatible. So, I'd suggest:
> 
> (pdata && pdata->modes[num].deselect_on_exit) || idle_disconnect ?
> pca954x_deselect_mux : NULL);
>
> >> I'm not keen to brake out-of-tree code (if any), but may be it will be
> >> decided to drop this per-channel deselect_on_exit, because it's not used
> >> at least in the kernel tree...
> 
> I couldn't find a user of the platform_data, at all. But removing
> platform_data support is a seperate patch, and deprecating platform_data
> is a seperate and general issue IMO.

Sure, but it could be a preliminary patch on top of which to add idle-
disconnect DT support :-)

-- 
Regards,

Laurent Pinchart

--
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] input: adxl34x: Add OF match support

[Laurent Pinchart]

On Thursday 15 January 2015 15:36:37 Wolfram Sang wrote:
> >>> Been there, got bitten. We only found out too late, because one driver
> >>> was in i2c and the other in GPIO (or LED even?), both using "953x" :(
> >> 
> >> That seems like a development, review and/or merge process failure to
> >> me, I wouldn't avoid generic compatible strings for that reason only.
> 
> Well, I think different here, but let's skip this discussion as it is
> not really needed right now...
> 
> >> As the ADXL346 is backward-compatible with the ADXL345, and as the
> >> driver doesn't support the ADXL346-specific features, how about adding
> >> only the adxl345 for now, and using compatible = "adi,adxl346",
> >> "adi,adxl345"; for the ADXL346 ?
> > 
> > I spoke too fast. The driver supports ADXL346-specific features, but does
> > so by detecting the device model at runtime.
> > 
> > I still believe it would make sense to list both the 346 and 345 models in
> > DT for 346 devices, as they're compatible with the 345.
> 
> I agree.
> 
> >>> 2) also add "34x" as a compatible but mark it as deprecateed
> >>> 3) delete "34x" from trivial devices
> >> 
> >> OK.
> 
> Yay :)

I'll submit patches very soon.

-- 
Regards,

Laurent Pinchart

--
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] input: adxl34x: Add OF match support

[Laurent Pinchart]

On Thursday 15 January 2015 16:19:19 Laurent Pinchart wrote:
> On Thursday 15 January 2015 13:53:22 Wolfram Sang wrote:
> > On Thu, Dec 18, 2014 at 02:49:28PM +0200, Laurent Pinchart wrote:
> >> On Thursday 18 December 2014 09:21:51 Wolfram Sang wrote:
> >>> On Thu, Dec 18, 2014 at 04:15:23AM +0200, Laurent Pinchart wrote:
> >>>> The I2C subsystem can match devices without explicit OF support based
> >>>> on the part of their compatible property after the comma. However,
> >>>> this mechanism uses the first compatible value only. For adxl34x OF
> >>>> device nodes the compatible property should list the more specific
> >>>> "adi,adxl345" or "adi,adxl346" value first and the "adi,adxl34x"
> >>>> fallback value second. This prevents the device node from being
> >>>> matched with the adxl34x driver.
> >>>> 
> >>>> Fix this by adding an OF match table with an "adi,adxl34x" compatible
> >>>> entry.
> >>>> 
> >>>> Signed-off-by: Laurent Pinchart
> >>>> 
> >>>> ---
> >>>> 
> >>>>  drivers/input/misc/adxl34x-i2c.c | 11 +++
> >>>>  1 file changed, 11 insertions(+)
> >>>> 
> >>>> Another option would have been to add "adxl325" and "adxl326" entries
> >>>> to the adxl34x_id I2C match table, but it would have had the drawback
> >>>> of requiring a driver update for every new device.
> >>> 
> >>> AFAIK this is even required for compatible entries, to be as specific
> >>> as possible. I think this makes sense. With platform_ids, we already
> >>> had the problem that pca954x was too generic and was used for both GPIO
> >>> extenders and I2C muxers (IIRC).
> >> 
> >> There are three compatible strings defined for the ADXL345 and ADXL346
> >> in Documentation/devicetree/bindings/i2c/trivial-devices.txt:
> >> "adi,adxl345", "adi,adxl346", "adi,adxl34x". Given that the last one is
> >> a fallback for the first two I don't see a need to add the specific
> >> compatible strings to the driver for now. If a new totally incompatible
> >> chip named ADXL347 comes out we will need a new driver which won't be
> >> allowed to use the "adi,adxl34x" compatible string.
> > 
> > Been there, got bitten. We only found out too late, because one driver
> > was in i2c and the other in GPIO (or LED even?), both using "953x" :(
> 
> That seems like a development, review and/or merge process failure to me, I
> wouldn't avoid generic compatible strings for that reason only.
> 
> > > An option would be to remove "adi,adxl34x" from
> > > Documentation/devicetree/bindings/i2c/trivial-devices.txt, in which case
> > > the driver should match explicitly on "adi,adxl345" and "adi,adxl346".
> > > That might clash with the DT ABI stability requirements though.
> > 
> > I do prefer this:
> > 
> > 1) add specific compatible values to the driver. We do those updates for
> > new devices all the time
> 
> Do you mean OF compatible values, or I2C match table entries ? I assume OF
> compatible values.
> 
> As the ADXL346 is backward-compatible with the ADXL345, and as the driver
> doesn't support the ADXL346-specific features, how about adding only the
> adxl345 for now, and using compatible = "adi,adxl346", "adi,adxl345"; for
> the ADXL346 ?

I spoke too fast. The driver supports ADXL346-specific features, but does so 
by detecting the device model at runtime.

I still believe it would make sense to list both the 346 and 345 models in DT 
for 346 devices, as they're compatible with the 345.

> > 2) also add "34x" as a compatible but mark it as deprecateed
> > 3) delete "34x" from trivial devices
> 
> OK.
> 
> > Everyone OK with that?

-- 
Regards,

Laurent Pinchart

--
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] input: adxl34x: Add OF match support

[Laurent Pinchart]

Hi Wolfram,

On Thursday 15 January 2015 13:53:22 Wolfram Sang wrote:
> On Thu, Dec 18, 2014 at 02:49:28PM +0200, Laurent Pinchart wrote:
> > On Thursday 18 December 2014 09:21:51 Wolfram Sang wrote:
> >> On Thu, Dec 18, 2014 at 04:15:23AM +0200, Laurent Pinchart wrote:
> >>> The I2C subsystem can match devices without explicit OF support based
> >>> on the part of their compatible property after the comma. However,
> >>> this mechanism uses the first compatible value only. For adxl34x OF
> >>> device nodes the compatible property should list the more specific
> >>> "adi,adxl345" or "adi,adxl346" value first and the "adi,adxl34x"
> >>> fallback value second. This prevents the device node from being
> >>> matched with the adxl34x driver.
> >>> 
> >>> Fix this by adding an OF match table with an "adi,adxl34x" compatible
> >>> entry.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> 
> >>> ---
> >>> 
> >>>  drivers/input/misc/adxl34x-i2c.c | 11 +++
> >>>  1 file changed, 11 insertions(+)
> >>> 
> >>> Another option would have been to add "adxl325" and "adxl326" entries
> >>> to the adxl34x_id I2C match table, but it would have had the drawback
> >>> of requiring a driver update for every new device.
> >> 
> >> AFAIK this is even required for compatible entries, to be as specific as
> >> possible. I think this makes sense. With platform_ids, we already had
> >> the problem that pca954x was too generic and was used for both GPIO
> >> extenders and I2C muxers (IIRC).
> > 
> > There are three compatible strings defined for the ADXL345 and ADXL346 in
> > Documentation/devicetree/bindings/i2c/trivial-devices.txt: "adi,adxl345",
> > "adi,adxl346", "adi,adxl34x". Given that the last one is a fallback for
> > the first two I don't see a need to add the specific compatible strings to
> > the driver for now. If a new totally incompatible chip named ADXL347 comes
> > out we will need a new driver which won't be allowed to use the
> > "adi,adxl34x" compatible string.
> 
> Been there, got bitten. We only found out too late, because one driver
> was in i2c and the other in GPIO (or LED even?), both using "953x" :(

That seems like a development, review and/or merge process failure to me, I 
wouldn't avoid generic compatible strings for that reason only.

> > An option would be to remove "adi,adxl34x" from
> > Documentation/devicetree/bindings/i2c/trivial-devices.txt, in which case
> > the driver should match explicitly on "adi,adxl345" and "adi,adxl346".
> > That might clash with the DT ABI stability requirements though.
> 
> I do prefer this:
> 
> 1) add specific compatible values to the driver. We do those updates for
> new devices all the time

Do you mean OF compatible values, or I2C match table entries ? I assume OF 
compatible values.

As the ADXL346 is backward-compatible with the ADXL345, and as the driver 
doesn't support the ADXL346-specific features, how about adding only the 
adxl345 for now, and using compatible = "adi,adxl346", "adi,adxl345"; for the 
ADXL346 ?

> 2) also add "34x" as a compatible but mark it as deprecateed
> 3) delete "34x" from trivial devices

OK.

> Everyone OK with that?

-- 
Regards,

Laurent Pinchart

--
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] input: adxl34x: Add OF match support

[Laurent Pinchart]

Hi Geert,

On Thursday 18 December 2014 21:23:46 Laurent Pinchart wrote:
> On Thursday 18 December 2014 14:03:18 Geert Uytterhoeven wrote:
> > On Thu, Dec 18, 2014 at 1:49 PM, Laurent Pinchart wrote:
> > > There are three compatible strings defined for the ADXL345 and ADXL346
> > > in Documentation/devicetree/bindings/i2c/trivial-devices.txt:
> > > "adi,adxl345", "adi,adxl346", "adi,adxl34x". Given that the last one is
> > > a fallback for the first two I don't see a need to add the specific
> > > compatible strings to the driver for now. If a new totally incompatible
> > > chip named ADXL347 comes out we will need a new driver which won't be
> > > allowed to use the "adi,adxl34x" compatible string.
> > 
> > FWIW, I'm the evil person who added the adxl entries to that file...
> 
> git-blame had already reported you.
> 
> Do you think we should remove that compatible string ?

Ping ?

-- 
Regards,

Laurent Pinchart

--
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: sh_mobile: fix uninitialized var when debug is enabled

[Laurent Pinchart]

Hi Wolfram,

Thank you for tha patch.

On Saturday 20 December 2014 09:36:42 Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> Signed-off-by: Wolfram Sang 
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/i2c/busses/i2c-sh_mobile.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-sh_mobile.c
> b/drivers/i2c/busses/i2c-sh_mobile.c index a12297e7680a..440d5dbc8b5f
> 100644
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -550,6 +550,7 @@ static struct dma_chan
> *sh_mobile_i2c_request_dma_chan(struct device *dev,
> 
>   chan = dma_request_slave_channel_reason(dev, chan_name);
>   if (IS_ERR(chan)) {
> + ret = PTR_ERR(chan);
>   dev_dbg(dev, "request_channel failed for %s (%d)\n", chan_name, 
> ret);

You could just do

dev_dbg(dev, "request_channel failed for %s (%d)\n", chan_name,
    PTR_ERR(chan));

but that's up to you.

Acked-by: Laurent Pinchart 

>   return chan;
>   }

-- 
Regards,

Laurent Pinchart

--
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] input: adxl34x: Add OF match support

[Laurent Pinchart]

Hi Geert,

On Thursday 18 December 2014 14:03:18 Geert Uytterhoeven wrote:
> On Thu, Dec 18, 2014 at 1:49 PM, Laurent Pinchart wrote:
> > There are three compatible strings defined for the ADXL345 and ADXL346 in
> > Documentation/devicetree/bindings/i2c/trivial-devices.txt: "adi,adxl345",
> > "adi,adxl346", "adi,adxl34x". Given that the last one is a fallback for
> > the first two I don't see a need to add the specific compatible strings to
> > the driver for now. If a new totally incompatible chip named ADXL347 comes
> > out we will need a new driver which won't be allowed to use the
> > "adi,adxl34x" compatible string.
> 
> FWIW, I'm the evil person who added the adxl entries to that file...

git-blame had already reported you.

Do you think we should remove that compatible string ?

-- 
Regards,

Laurent Pinchart

--
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] input: adxl34x: Add OF match support

[Laurent Pinchart]

Hi Wolfram,

On Thursday 18 December 2014 09:21:51 Wolfram Sang wrote:
> On Thu, Dec 18, 2014 at 04:15:23AM +0200, Laurent Pinchart wrote:
> > The I2C subsystem can match devices without explicit OF support based on
> > the part of their compatible property after the comma. However, this
> > mechanism uses the first compatible value only. For adxl34x OF device
> > nodes the compatible property should list the more specific
> > "adi,adxl345" or "adi,adxl346" value first and the "adi,adxl34x"
> > fallback value second. This prevents the device node from being matched
> > with the adxl34x driver.
> > 
> > Fix this by adding an OF match table with an "adi,adxl34x" compatible
> > entry.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> > ---
> > 
> >  drivers/input/misc/adxl34x-i2c.c | 11 +++
> >  1 file changed, 11 insertions(+)
> > 
> > Another option would have been to add "adxl325" and "adxl326" entries to
> > the adxl34x_id I2C match table, but it would have had the drawback of
> > requiring a driver update for every new device.
> 
> AFAIK this is even required for compatible entries, to be as specific as
> possible. I think this makes sense. With platform_ids, we already had
> the problem that pca954x was too generic and was used for both GPIO
> extenders and I2C muxers (IIRC).

There are three compatible strings defined for the ADXL345 and ADXL346 in 
Documentation/devicetree/bindings/i2c/trivial-devices.txt: "adi,adxl345", 
"adi,adxl346", "adi,adxl34x". Given that the last one is a fallback for the 
first two I don't see a need to add the specific compatible strings to the 
driver for now. If a new totally incompatible chip named ADXL347 comes out we 
will need a new driver which won't be allowed to use the "adi,adxl34x" 
compatible string.

An option would be to remove "adi,adxl34x" from 
Documentation/devicetree/bindings/i2c/trivial-devices.txt, in which case the 
driver should match explicitly on "adi,adxl345" and "adi,adxl346". That might 
clash with the DT ABI stability requirements though.

-- 
Regards,

Laurent Pinchart

--
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] input: adxl34x: Add OF match support

[Laurent Pinchart]

The I2C subsystem can match devices without explicit OF support based on
the part of their compatible property after the comma. However, this
mechanism uses the first compatible value only. For adxl34x OF device
nodes the compatible property should list the more specific
"adi,adxl345" or "adi,adxl346" value first and the "adi,adxl34x"
fallback value second. This prevents the device node from being matched
with the adxl34x driver.

Fix this by adding an OF match table with an "adi,adxl34x" compatible
entry.

Signed-off-by: Laurent Pinchart 
---
 drivers/input/misc/adxl34x-i2c.c | 11 +++
 1 file changed, 11 insertions(+)

Another option would have been to add "adxl325" and "adxl326" entries to the
adxl34x_id I2C match table, but it would have had the drawback of requiring a
driver update for every new device.

diff --git a/drivers/input/misc/adxl34x-i2c.c b/drivers/input/misc/adxl34x-i2c.c
index 416f47ddcc90..22687b738811 100644
--- a/drivers/input/misc/adxl34x-i2c.c
+++ b/drivers/input/misc/adxl34x-i2c.c
@@ -10,6 +10,7 @@
 #include/* BUS_I2C */
 #include 
 #include 
+#include 
 #include 
 #include 
 #include "adxl34x.h"
@@ -137,11 +138,21 @@ static const struct i2c_device_id adxl34x_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, adxl34x_id);
 
+#ifdef CONFIG_OF
+static const struct of_device_id adxl34x_of_id[] = {
+   { .compatible = "adi,adxl34x", },
+   { }
+};
+
+MODULE_DEVICE_TABLE(of, adxl34x_of_id);
+#endif
+
 static struct i2c_driver adxl34x_driver = {
.driver = {
.name = "adxl34x",
.owner = THIS_MODULE,
.pm = &adxl34x_i2c_pm,
+   .of_match_table = of_match_ptr(adxl34x_of_id),
},
    .probe    = adxl34x_i2c_probe,
.remove   = adxl34x_i2c_remove,
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH] of: i2c: Add DT bindings for idle states to PCA954x mux driver

[Laurent Pinchart]

Hi Alexander,

On Tuesday 16 December 2014 13:49:13 Alexander Sverdlin wrote:
> On 16/12/14 12:07, ext Laurent Pinchart wrote:
> >>>> of: i2c: Add DT bindings for idle states to PCA954x mux driver
> >>>> 
> >>>> Introduce two new device tree bindings to specify idle state of PCA954x
> >>>> 
> >>>> family of I2C multiplexors:
> >>>>   - idle-state: specifies particular child bus to be selected in idle;
> >>>>   - idle-disconnect: signals that mux should disconnect all child buses
> >>>>   in idle;
> >>> 
> >>> Could you please briefly explain your use case(s) for those two
> >>> properties ?
> >> 
> >> idle-state specifies which bus will be connected when there is no data
> >> transfer. i2c-mux-gpio also has "idle-state" (though, it can replace both
> >> connect and disconnect cases in my patch). i2c-mux-pinctrl has special
> >> "idle" convention, specifying idle state as last in the list of states.
> >> Again, it can serve for both my properties.
> >> 
> >> idle-disconnect is what we actually use for some security reasons (so
> >> that I2C bus of the external SFP cage is not connected to the rest of I2C
> >> bus in idle).
> > 
> > Thank you for the explanation. I would add it to the DT bindings
> > documentation, or at least to the commit message.
> 
> Ok, I'll send v2 with all your comments addressed.

Thank you.

> > I understand the use cases of the idle-disconnect property. What do you
> > use idle-state for, when the idle state is different from disconnecting
> > the bus ?
>
> We do not have a use case for that. It was done only for the sake of
> completeness. Because other MUXes actually offer user this possibility. I
> was initially thinking about providing this on i2c-mux level for all of
> them, but unfortunately they all use deselect callback in different way and
> pinctrl is the worst case, as idle state cannot be represented with an int,
> but should be a pointer.
> 
> Anyway, if you think there is no use-case for this, I can drop this part.

Adding a DT property without a clear use case usually makes me a bit wary. I 
would thus prefer going for either idle-disconnect alone, or for an idle-state 
property that would allow selecting disconnection as one of the possible 
values.
 
> >>>> Signed-off-by: Alexander Sverdlin 
> >>>> ---
> >>>> 
> >>>>  .../devicetree/bindings/i2c/i2c-mux-pca954x.txt|3 +
> >>>>  drivers/i2c/muxes/i2c-mux-pca954x.c|   51
> >>>>  --
> >>>>  2 files changed, 48 insertions(+), 6 deletions(-)
> >>>> 
> >>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> >>>> b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index
> >>>> 34a3fb6..1fbe287 100644
> >>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> >>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> >>>> 
> >>>> @@ -16,6 +16,9 @@ Required Properties:
> >>>>  Optional Properties:
> >>>>- reset-gpios: Reference to the GPIO connected to the reset input.
> >>>> 
> >>>> +  - idle-state: Child bus connected in idle state (specified by its
> >>>> "reg" value)
> >>>> +  - idle-disconnect: Boolean; if defined, forces mux to disconnect all
> >>>> children
> >>>> +in idle state
> >>>> 
> >>>>  Example:
> >>>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> >>>> b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644
> >>>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> >>>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> >>>> @@ -43,6 +43,7 @@
> >>>> 
> >>>>  #include 
> >>>>  #include 
> >>>>  #include 
> >>>> 
> >>>> +#include 
> >>> 
> >>> Could you please keep headers sorted alphabetically ?
> >> 
> >> Yes...
> >> 
> >>>>  #define PCA954X_MAX_NCHANS 8
> >>>> 
> >>>> @@ -62,6 +63,8 @@ struct pca954x {
> >>>> 
> >>>>  struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
> >>>>  
> >>>>  u8 last_chan;   /* last register value */
> >>>&g

Re: [PATCH] of: i2c: Add DT bindings for idle states to PCA954x mux driver

[Laurent Pinchart]

Hi Alexander,

On Tuesday 16 December 2014 09:09:22 Alexander Sverdlin wrote:
> Hello Laurent,
> 
> On 15/12/14 09:40, ext Laurent Pinchart wrote:
> >> of: i2c: Add DT bindings for idle states to PCA954x mux driver
> >> 
> >> Introduce two new device tree bindings to specify idle state of PCA954x
> >> 
> >> family of I2C multiplexors:
> >>   - idle-state: specifies particular child bus to be selected in idle;
> >>   - idle-disconnect: signals that mux should disconnect all child buses
> >>   in idle;
> > 
> > Could you please briefly explain your use case(s) for those two properties
> > ?
>
> idle-state specifies which bus will be connected when there is no data
> transfer. i2c-mux-gpio also has "idle-state" (though, it can replace both
> connect and disconnect cases in my patch). i2c-mux-pinctrl has special
> "idle" convention, specifying idle state as last in the list of states.
> Again, it can serve for both my properties.
> 
> idle-disconnect is what we actually use for some security reasons (so that
> I2C bus of the external SFP cage is not connected to the rest of I2C bus
> in idle).

Thank you for the explanation. I would add it to the DT bindings 
documentation, or at least to the commit message.

I understand the use cases of the idle-disconnect property. What do you use 
idle-state for, when the idle state is different from disconnecting the bus ?

> >> Signed-off-by: Alexander Sverdlin 
> >> ---
> >> 
> >>  .../devicetree/bindings/i2c/i2c-mux-pca954x.txt|3 +
> >>  drivers/i2c/muxes/i2c-mux-pca954x.c|   51 --
> >>  2 files changed, 48 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> >> b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index
> >> 34a3fb6..1fbe287 100644
> >> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> >> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> >> 
> >> @@ -16,6 +16,9 @@ Required Properties:
> >>  Optional Properties:
> >>- reset-gpios: Reference to the GPIO connected to the reset input.
> >> 
> >> +  - idle-state: Child bus connected in idle state (specified by its
> >> "reg" value)
> >> +  - idle-disconnect: Boolean; if defined, forces mux to disconnect all
> >> children
> >> +in idle state
> >> 
> >>  Example:
> >> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> >> b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644
> >> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> >> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> >> @@ -43,6 +43,7 @@
> >>  #include 
> >>  #include 
> >>  #include 
> >> +#include 
> > 
> > Could you please keep headers sorted alphabetically ?
> 
> Yes...
> 
> >>  #define PCA954X_MAX_NCHANS 8
> >> 
> >> @@ -62,6 +63,8 @@ struct pca954x {
> >> 
> >>struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
> >>
> >>u8 last_chan;   /* last register value */
> >> 
> >> +  bool idle_disconnect;
> >> +  s8 idle_chan;   /* valid if not negative */
> >> 
> >>  };
> >>  
> >>  struct chip_desc {
> >> 
> >> @@ -172,10 +175,20 @@ static int pca954x_deselect_mux(struct i2c_adapter
> >> *adap, void *client, u32 chan)
> >> 
> >>  {
> >>  
> >>struct pca954x *data = i2c_get_clientdata(client);
> >> 
> >> +  struct pca954x_platform_data *pdata =
> >> +  dev_get_platdata(&((struct i2c_client *)client)->dev);
> >> +
> >> +  if ((pdata && pdata->modes[chan].deselect_on_exit) ||
> >> +  data->idle_disconnect) {
> > 
> > I would copy pdata->modes[chan].deselect_on_exit to data->idle_disconnect
> > in the probe function, so you could avoiding accessing pdata here.
> 
> Unfortunately, this pdata has different (per-channel) semantics. I cannot
> really understand, why it was done this way, but anyway it's not possible
> to use one global bit to represent per-channel bits without changing the
> behavior.
> 
> I'm not keen to brake out-of-tree code (if any), but may be it will be
> decided to drop this per-channel deselect_on_exit, because it's not used at
> least in the kernel tree...

I'd vote for removing deselect_on_exit from platform data, but I won't insist.

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

Hi Vinod,

On Monday 15 December 2014 14:43:14 Vinod Koul wrote:
> On Mon, Dec 15, 2014 at 09:31:01AM +0100, Geert Uytterhoeven wrote:
> > Please let me summarize...
> 
> Thanks :)
> 
> > During probe of a DMA client driver, the DMA engine driver may not be
> > available, causing dma_request_slave_channel*() to return -EPROBE-DEFER.
> > There are actually two different reasons that the DMA engine driver may
> > not be available:
> >
> > 1. The DMA engine driver hasn't been initialized yet, due to probe order.
> >This is more likely to happen with i2c client drivers, as they are
> >initialized from subsys_initcall() instead of module_init() (E.g. I
> >never saw it with the spi-rspi driver).
> >=> The DMA client driver wants to return -EPROBE_DEFER too, and
> >   retry later.
> >   
> > 2. The DMA engine driver is not included in the kernel build.
> >=> The DMA client driver wants to fall back to PIO.
> > 
> > Now, how to distinguish between the two cases above?
> 
> Quite right, this is a good question. Today we cannot distinguish between
> the two. Should we improve the deferred probe to tell us when the init is
> complete and all the modules have been initialized?

I don't think that's possible, as you can never know when a module will be 
loaded.

> If we ever have such a mechanism to check then we know no modules are to be
> inserted then we can fall back to PIO mode. Without that we should use some
> timeout counter to fall back on, say try requesting 5 times and give up and
> move to PIO after that

That could be a performance improvement, but I wonder whether it's worth it. 
If DT specifies DMA channels for the I2C controller, and the DMA core is 
compiled in, and the DMA engine driver is compiled as a module and never 
loaded, then yes, there will be a small overhead for each I2C transaction, but 
I'd argue that such a combination of conditions is asking for trouble anyway 
:-)

> > Currently, e.g. i2c-sh_mobile always returns -EPROBE_DEFER, never falling
> > back to PIO, breaking case 2. While e.g. spi-rspi always falls back to
> > PIO, which is suboptimal in case 1 (but I never encountered that case with
> > spi).
> > 
> > Solutions under consideration:
> > 1. Wolfram posted a patch to make i2c-sh_mobile fall back to PIO, and
> >retry DMA initialization in every request, so it will switch to DMA
> >when it becomes available. But this is suboptimal, as it adds overhead
> >to every request (and DMA may never become available in case 2).
> >   
> > 2. Delay i2c initialization, by moving from subsys_initcall() to
> >module_init(), in the hope the i2c client driver will be initialized
> >    after the DMA engine.
> > 
> >This is being discussed in the thread you quoted above.
> > 
> > I hope this explains the problem well.
> 
> Yes and it has nothing to do with channels being exhausted, the problem I
> assumed earlier!!

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH] of: i2c: Add DT bindings for idle states to PCA954x mux driver

[Laurent Pinchart]

t; 
>   /* Now create an adapter for each channel */
>   for (num = 0; num < chips[data->type].nchans; num++) {
> @@ -234,8 +269,7 @@ static int pca954x_probe(struct i2c_client *client,
>   data->virt_adaps[num] =
>   i2c_add_mux_adapter(adap, &client->dev, client,
>   force, num, class, pca954x_select_chan,
> - (pdata && pdata->modes[num].deselect_on_exit)
> - ? pca954x_deselect_mux : NULL);
> + pca954x_deselect_mux);
> 
>   if (data->virt_adaps[num] == NULL) {
>   ret = -ENODEV;
> @@ -281,7 +315,12 @@ static int pca954x_resume(struct device *dev)
>   struct pca954x *data = i2c_get_clientdata(client);
> 
>   data->last_chan = 0;
> - return i2c_smbus_write_byte(client, 0);
> + /* Restore idle state on resume */
> + if (data->idle_chan >= 0)
> + return pca954x_select_chan(to_i2c_adapter(client->dev.parent),
> +client, data->idle_chan);
> + else
> + return i2c_smbus_write_byte(client, 0);
>  }
>  #endif

-- 
Regards,

Laurent Pinchart

--
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/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

On Thursday 11 December 2014 22:47:32 Wolfram Sang wrote:
> > > Note that the I2C drives uses subsys_initcall() for historic reasons,
> > > while the DMA driver uses module_init(). This is hard to revert without
> > > introducing potential regressions on older boards. So, the I2C DMA
> > > support needs to handle deferred probe definately. I am with Laurent, I
> > > don't see any other way, but I'd be glad to be enlightened...
> > 
> > While I believe that requesting the channel at transfer time is the good
> > solution, I think we should still try to move to module initcalls where
> > possible. The risk of regressions is real so proper testing is needed. My
> > question is, have you tried it ?
> 
> I would need to test all boards using this driver to not fail booting.
> Usually I2C drivers are moved to subsys_initcall because they need
> access to something critical (PMIC, GPIOs...) early. I don't see a sane
> way to do that testing.

Still, I would like to get a better view on the problems we should expect, by 
testing this on the latest boards for instance.

> Other than that, even if we move to module_init, we reduce the chance of
> getting a deferred probe, but we do not eliminate it...

Sure, but reducing the chance of deferred probe is a good idea in my opinion 
:-)

-- 
Regards,

Laurent Pinchart

--
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/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

Hi Wolfram,

On Wednesday 10 December 2014 15:23:15 Wolfram Sang wrote:
> On Wed, Dec 10, 2014 at 04:19:36PM +0200, Laurent Pinchart wrote:
> > (CC'ing the dmaengine mailing list)
> 
> Thanks!
> 
> > On Wednesday 10 December 2014 09:01:55 Wolfram Sang wrote:
> > > On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> > > > On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang wrote:
> > > >> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> > > >>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang wrote:
> > > >>>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct
> > > >>>> platform_device *dev)
> > > >>>> 
> > > >>>> if (ret)
> > > >>>> 
> > > >>>> return ret;
> > > >>>> 
> > > >>>> +   /* Init DMA */
> > > >>>> +   sg_init_table(&pd->sg, 1);
> > > >>>> +   pd->dma_direction = DMA_NONE;
> > > >>>> +   ret = sh_mobile_i2c_request_dma_chan(pd->dev,
> > > >>>> DMA_DEV_TO_MEM,
> > > >>>> +res->start + ICDR,
> > > >>>> &pd->dma_rx);
> > > >>>> +   if (ret == -EPROBE_DEFER)
> > > >>>> +   return ret;
> > > >>>> +
> > > >>>> +   ret = sh_mobile_i2c_request_dma_chan(pd->dev,
> > > >>>> DMA_MEM_TO_DEV,
> > > >>>> +res->start + ICDR,
> > > >>>> &pd->dma_tx);
> > > >>>> +   if (ret == -EPROBE_DEFER) {
> > > >>>> +   sh_mobile_i2c_release_dma(pd);
> > > >>>> +   return ret;
> > > >>>> +   }
> > > >>>> +
> > > >>> 
> > > >>> If the DTS contains "dma" and "dma-names" properties, but
> > > >>> CONFIG_RCAR_DMAC is disabled, sh_mobile_i2c_request_dma_chan()
> > > >>> returns -EPROBE_DEFER, and the driver fails to initialize.
> > > >>> 
> > > >>> If I remove the "dma" and "dma-names" properties, the driver does
> > > >>> fall back to PIO mode.
> > > >>> 
> > > >>> I think this is a regression.
> > > >> 
> > > >> The only solution I can think of is to not bail out here and retry
> > > >> again before every transfer? Doesn't sound elegant, though...
> > > > 
> > > > I think we have to request for each and every transfer. And fall back
> > > > to PIO as default in a transparent way. This because the number of DMA
> > > > channels are limited compared to number of potential consumers, so
> > > > request failure may happen at any time.
> > > 
> > > AFAIR this scenario happens when submitting the transfer. The check
> > > for this is already in place. Requesting the channel is a different
> > > matter. Still, I'll cook up a patch and we will see what it looks
> > > like...
> > 
> > We could fix part of the issue by using virtual dma channels. In that case
> > channel requests wouldn't fail anymore due to resource starvation with a
> > large number of consumers. However, the request could still fail with
> > -EPROBE_DEFER. For a driver that wants to fall back to PIO when DMA is
> > unavailable I currently don't see another way than moving the channel
> > request at the time of the transfer.
> 
> Note that the I2C drives uses subsys_initcall() for historic reasons,
> while the DMA driver uses module_init(). This is hard to revert without
> introducing potential regressions on older boards. So, the I2C DMA
> support needs to handle deferred probe definately. I am with Laurent, I
> don't see any other way, but I'd be glad to be enlightened...

While I believe that requesting the channel at transfer time is the good 
solution, I think we should still try to move to module initcalls where 
possible. The risk of regressions is real so proper testing is needed. My 
question is, have you tried it ?

-- 
Regards,

Laurent Pinchart

--
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/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

On Thursday 11 December 2014 08:37:27 Wolfram Sang wrote:
> > I think this is a limitation of driver may not be for HW. The right model
> > for dma_chan is to be viewed as SW channels and not the ones of HW (yes
> > that is how most of the drivers use that, but we can improve upon)
> > 
> > If we rework the driver to view dma_chan as SW channels, then you can
> > accept multiple channel requests and accept based on if we are able link
> > the channel to that peripheral or not.
> 
> In my understanding, the DMA driver does exactly that.

Actually it doesn't at the moment, I should implement that.

> However, it is not even loaded at the time the I2C driver wants a channel,
> so the dmaengine core defers the probe. That is the problem for optional DMA
> channels: we can't know when deferring probe won't help anymore and don't
> know when it is time to fall back to PIO.

This is true regardless of the whether the driver exposes HW or SW channels.

-- 
Regards,

Laurent Pinchart

--
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/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

(CC'ing the dmaengine mailing list)

On Wednesday 10 December 2014 09:01:55 Wolfram Sang wrote:
> On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> > On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang  wrote:
> >> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> >>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang wrote:
> >>>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> >>>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> >>>> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct
> >>>> platform_device *dev)
> >>>> if (ret)
> >>>> return ret;
> >>>> 
> >>>> +   /* Init DMA */
> >>>> +   sg_init_table(&pd->sg, 1);
> >>>> +   pd->dma_direction = DMA_NONE;
> >>>> +   ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> >>>> +res->start + ICDR,
> >>>> &pd->dma_rx);
> >>>> +   if (ret == -EPROBE_DEFER)
> >>>> +   return ret;
> >>>> +
> >>>> +   ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> >>>> +res->start + ICDR,
> >>>> &pd->dma_tx);
> >>>> +   if (ret == -EPROBE_DEFER) {
> >>>> +   sh_mobile_i2c_release_dma(pd);
> >>>> +   return ret;
> >>>> +   }
> >>>> +
> >>> 
> >>> If the DTS contains "dma" and "dma-names" properties, but
> >>> CONFIG_RCAR_DMAC is disabled, sh_mobile_i2c_request_dma_chan() returns
> >>> -EPROBE_DEFER, and the driver fails to initialize.
> >>> 
> >>> If I remove the "dma" and "dma-names" properties, the driver does fall
> >>> back to PIO mode.
> >>> 
> >>> I think this is a regression.
> >> 
> >> The only solution I can think of is to not bail out here and retry again
> >> before every transfer? Doesn't sound elegant, though...
> > 
> > I think we have to request for each and every transfer. And fall back
> > to PIO as default in a transparent way. This because the number of DMA
> > channels are limited compared to number of potential consumers, so
> > request failure may happen at any time.
> 
> AFAIR this scenario happens when submitting the transfer. The check
> for this is already in place. Requesting the channel is a different
> matter. Still, I'll cook up a patch and we will see what it looks
> like...

We could fix part of the issue by using virtual dma channels. In that case 
channel requests wouldn't fail anymore due to resource starvation with a large 
number of consumers. However, the request could still fail with -EPROBE_DEFER. 
For a driver that wants to fall back to PIO when DMA is unavailable I 
currently don't see another way than moving the channel request at the time of 
the transfer.

-- 
Regards,

Laurent Pinchart

--
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: core: Fix probing of i2c slaves without interrupts

[Laurent Pinchart]

Hi Geert,

On Monday 17 November 2014 12:43:00 Geert Uytterhoeven wrote:
> Since commit 2fd36c55264926e2 ("i2c: core: Map OF IRQ at probe time"),
> i2c slaves without interrupts (e.g. da9210 and at24 on r8a7791/koelsch)
> fail to probe:
> 
> at24: probe of 2-0050 failed with error -22
> 
> da9210: probe of 6-0068 failed with error -22
> 
> This happens because the call to of_irq_get() in i2c_device_probe()
> returns -EINVAL.
> 
> If a device node does not have an "interrupts" property,
> of_irq_parse_one() fails. Unlike irq_of_parse_and_map(), of_irq_get()
> does not ignore errors from of_irq_parse_one(), but forwards them.
> 
> Make i2c_device_probe() ignore all errors but -EPROBE_DEFER to fix this,
> just like platform_get_irq() and platform_get_irq_byname() already do.

Thank you for fixing my mistake.

> Fixes: 2fd36c55264926e2 ("i2c: core: Map OF IRQ at probe time")
> Signed-off-by: Geert Uytterhoeven 

Just for the record, even if it's too late,

Acked-by: Laurent Pinchart 

> ---
>  drivers/i2c/i2c-core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index a0768d6dffc28aae..cf830915713b387a 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -629,8 +629,10 @@ static int i2c_device_probe(struct device *dev)
>   if (!client->irq && dev->of_node) {
>   int irq = of_irq_get(dev->of_node, 0);
> 
> - if (irq < 0)
> + if (irq == -EPROBE_DEFER)
>       return irq;
> + if (irq < 0)
> + irq = 0;
> 
>   client->irq = irq;
>   }

-- 
Regards,

Laurent Pinchart

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

Re: [RFC 1/3] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

 bool do_init)
>  {
> @@ -510,6 +612,9 @@ static int start_ch(struct sh_mobile_i2c_data *pd,
> struct i2c_msg *usr_msg, pd->pos = -1;
>   pd->sr = 0;
> 
> + if (pd->msg->len > 4)
> + sh_mobile_i2c_xfer_dma(pd);
> +
>   /* Enable all interrupts to begin with */
>   iic_wr(pd, ICIC, ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE);
>   return 0;
> @@ -593,6 +698,9 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter
> *adapter, 5 * HZ);
>   if (!k) {
>   dev_err(pd->dev, "Transfer request timed out\n");
> + if (pd->dma_direction != DMA_NONE)
> + sh_mobile_i2c_cleanup_dma(pd);
> +
>   err = -ETIMEDOUT;
>   break;
>   }
> @@ -641,6 +749,70 @@ static const struct of_device_id sh_mobile_i2c_dt_ids[]
> = { };
>  MODULE_DEVICE_TABLE(of, sh_mobile_i2c_dt_ids);
> 
> +static struct dma_chan *sh_mobile_i2c_request_dma_chan(struct device *dev,
> +   enum dma_transfer_direction dir,
> +   dma_addr_t port_addr)
> +{
> + dma_cap_mask_t mask;
> + struct dma_chan *chan;
> + struct dma_slave_config cfg;
> + int ret;
> +
> + dma_cap_zero(mask);
> + dma_cap_set(DMA_SLAVE, mask);
> +
> + chan = dma_request_slave_channel_compat(mask, shdma_chan_filter,
> + (void *)0UL, dev, dir == DMA_MEM_TO_DEV ? "tx" 
> : "rx");
> +
> + if (!chan)
> + return NULL;
> +
> + memset(&cfg, 0, sizeof(cfg));
> + cfg.direction = dir;
> + if (dir == DMA_MEM_TO_DEV) {
> + cfg.dst_addr = port_addr;
> + cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> + } else {
> + cfg.src_addr = port_addr;
> + cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> + }
> +
> + ret = dmaengine_slave_config(chan, &cfg);
> + if (ret) {
> + dev_warn(dev, "dmaengine_slave_config failed %d\n", ret);
> + dma_release_channel(chan);
> + return NULL;
> + }
> +
> + return chan;
> +}
> +
> +static void sh_mobile_i2c_request_dma(struct sh_mobile_i2c_data *pd, const
> struct resource *res)
> +{
> + pd->buf_mapped = false;
> + pd->dma_direction = DMA_NONE;
> + sg_init_table(&pd->sg, 1);
> +
> + pd->dma_rx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> +res->start + ICDR);
> +
> + pd->dma_tx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> +res->start + ICDR);
> +}
> +
> +static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
> +{
> + if (pd->dma_tx) {
> + dma_release_channel(pd->dma_tx);
> + pd->dma_tx = NULL;
> + }
> +
> + if (pd->dma_rx) {
> + dma_release_channel(pd->dma_rx);
> + pd->dma_rx = NULL;
> + }
> +}
> +
>  static int sh_mobile_i2c_hook_irqs(struct platform_device *dev)
>  {
>   struct resource *res;
> @@ -727,6 +899,8 @@ static int sh_mobile_i2c_probe(struct platform_device
> *dev) if (ret)
>   return ret;
> 
> + sh_mobile_i2c_request_dma(pd, res);
> +
>   /* Enable Runtime PM for this device.
>*
>* Also tell the Runtime PM core to ignore children
> @@ -758,6 +932,7 @@ static int sh_mobile_i2c_probe(struct platform_device
> *dev)
> 
>   ret = i2c_add_numbered_adapter(adap);
>   if (ret < 0) {
> + sh_mobile_i2c_release_dma(pd);
>   dev_err(&dev->dev, "cannot add numbered adapter\n");
>   return ret;
>   }
> @@ -774,6 +949,7 @@ static int sh_mobile_i2c_remove(struct platform_device
> *dev) struct sh_mobile_i2c_data *pd = platform_get_drvdata(dev);
> 
>   i2c_del_adapter(&pd->adap);
> + sh_mobile_i2c_release_dma(pd);
>   pm_runtime_disable(&dev->dev);
>   return 0;
>  }
> @@ -805,19 +981,8 @@ static struct platform_driver sh_mobile_i2c_driver = {
>   .probe  = sh_mobile_i2c_probe,
>   .remove = sh_mobile_i2c_remove,
>  };
> +module_platform_driver(sh_mobile_i2c_driver);
> 
> -static int __init sh_mobile_i2c_adap_init(void)
> -{
> - return platform_driver_register(&sh_mobile_i2c_driver);
> -}
> -
> -static void __exit sh_mobile_i2c_adap_exit(void)
> -{
> - platform_driver_unregister(&sh_mobile_i2c_driver);
> -}
> -
> -subsys_initcall(sh_mobile_i2c_adap_init);
> -module_exit(sh_mobile_i2c_adap_exit);
> 

Extra blank line here.

>  MODULE_DESCRIPTION("SuperH Mobile I2C Bus Controller driver");
>  MODULE_AUTHOR("Magnus Damm");

-- 
Regards,

Laurent Pinchart

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

Re: [RFC 3/3] ARM: shmobile: r8a7791: add DMA nodes for IIC

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Friday 31 October 2014 11:51:18 Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> Signed-off-by: Wolfram Sang 

Acked-by: Laurent Pinchart 

> ---
>  arch/arm/boot/dts/r8a7791.dtsi | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
> index 9a57215f54f7..213b6bda6201 100644
> --- a/arch/arm/boot/dts/r8a7791.dtsi
> +++ b/arch/arm/boot/dts/r8a7791.dtsi
> @@ -371,6 +371,8 @@
>   reg = <0 0xe60b 0 0x425>;
>   interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>;
>   clocks = <&mstp9_clks R8A7791_CLK_IICDVFS>;
> + dmas = <&dmac0 0x77>, <&dmac0 0x78>;
> + dma-names = "tx", "rx";
>   status = "disabled";
>   };
> 
> @@ -381,6 +383,8 @@
>   reg = <0 0xe650 0 0x425>;
>   interrupts = <0 174 IRQ_TYPE_LEVEL_HIGH>;
>   clocks = <&mstp3_clks R8A7791_CLK_IIC0>;
> + dmas = <&dmac0 0x61>, <&dmac0 0x62>;
> + dma-names = "tx", "rx";
>   status = "disabled";
>   };
> 
> @@ -391,6 +395,8 @@
>   reg = <0 0xe651 0 0x425>;
>   interrupts = <0 175 IRQ_TYPE_LEVEL_HIGH>;
>   clocks = <&mstp3_clks R8A7791_CLK_IIC1>;
> + dmas = <&dmac0 0x65>, <&dmac0 0x66>;
> + dma-names = "tx", "rx";
>   status = "disabled";
>   };

-- 
Regards,

Laurent Pinchart

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

Re: [RFC 2/3] ARM: shmobile: r8a7790: add DMA nodes for IIC

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Friday 31 October 2014 11:51:17 Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> Signed-off-by: Wolfram Sang 

Acked-by: Laurent Pinchart 

> ---
>  arch/arm/boot/dts/r8a7790.dtsi | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
> index 69b7cd0e7fb3..7e836cc86098 100644
> --- a/arch/arm/boot/dts/r8a7790.dtsi
> +++ b/arch/arm/boot/dts/r8a7790.dtsi
> @@ -359,6 +359,8 @@
>   reg = <0 0xe650 0 0x425>;
>   interrupts = <0 174 IRQ_TYPE_LEVEL_HIGH>;
>   clocks = <&mstp3_clks R8A7790_CLK_IIC0>;
> + dmas = <&dmac0 0x61>, <&dmac0 0x62>;
> + dma-names = "tx", "rx";
>   status = "disabled";
>   };
> 
> @@ -369,6 +371,8 @@
>   reg = <0 0xe651 0 0x425>;
>   interrupts = <0 175 IRQ_TYPE_LEVEL_HIGH>;
>   clocks = <&mstp3_clks R8A7790_CLK_IIC1>;
> + dmas = <&dmac0 0x65>, <&dmac0 0x66>;
> + dma-names = "tx", "rx";
>   status = "disabled";
>   };
> 
> @@ -379,6 +383,8 @@
>   reg = <0 0xe652 0 0x425>;
>   interrupts = <0 176 IRQ_TYPE_LEVEL_HIGH>;
>   clocks = <&mstp3_clks R8A7790_CLK_IIC2>;
> + dmas = <&dmac0 0x69>, <&dmac0 0x6a>;
> + dma-names = "tx", "rx";
>   status = "disabled";
>   };
> 
> @@ -389,6 +395,8 @@
>   reg = <0 0xe60b 0 0x425>;
>   interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>;
>   clocks = <&mstp9_clks R8A7790_CLK_IICDVFS>;
> + dmas = <&dmac0 0x77>, <&dmac0 0x78>;
> + dma-names = "tx", "rx";
>   status = "disabled";
>   };

-- 
Regards,

Laurent Pinchart

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

Re: [RFC 0/3] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

Hi Wolfram,

On Friday 31 October 2014 11:51:15 Wolfram Sang wrote:
> Here is my RFC to support DMA with the i2c-sh_mobile core. DMA works nicely
> with my tests so far and we save 1 interrupt per transferred byte, yay!

Do we have an idea of how much power (or CPU time ?) DMA support could save ?

> DMA is opt-in, so if setting it up fails, we will fall back to PIO. The
> threshold for selecting DMA is still under test, but probably good enough
> already. The major issue currently: This driver uses subsys_initcall() but
> at that time DMA is not available, and there is no deferred probe for DMA.
> So, switching to module_init() makes DMA work, but this may cause
> side-effects for older boards which rely on I2C being available early (to
> control some PMIC, for example). This needs some more investigation. Also,
> the driver (like all I2C DMA drivers currently) assumes that i2c message
> buffers are DMA capable. This is not always true and might need some
> assistance from the I2C core.

Given the amount of data we could probably use bounce buffers.

> Other than that, please test, review, comment. The series is based on
> renesas-devel-20141030-v3.18-rc2 with Laurent's series "[PATCH v4 0/5] R-Car
> Gen2 DMA Controller driver" on top of it. A git tree can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
> renesas/i2c-shmobile-dma-experimental
> 
> Thanks,
> 
> Wolfram
> 
> 
> Wolfram Sang (3):
>   i2c: sh_mobile: add DMA support
>   ARM: shmobile: r8a7790: add DMA nodes for IIC
>   ARM: shmobile: r8a7791: add DMA nodes for IIC
> 
>  .../devicetree/bindings/i2c/i2c-sh_mobile.txt  |   5 +
>  arch/arm/boot/dts/r8a7790.dtsi |   8 +
>  arch/arm/boot/dts/r8a7791.dtsi |   6 +
>  drivers/i2c/busses/i2c-sh_mobile.c     | 203 --
>  4 files changed, 203 insertions(+), 19 deletions(-)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/3] of/irq: Export of_irq_get()

[Laurent Pinchart]

On Thursday 30 October 2014 15:16:44 Wolfram Sang wrote:
> On Thu, Oct 30, 2014 at 03:59:36PM +0200, Laurent Pinchart wrote:
> > The function will be used by the I2C core which can be compiled as a
> > module.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> I think it makes sense if I take this via I2C to get the dependencies
> for the later patches right?

It would be easier, yes.

> > ---
> > 
> >  drivers/of/irq.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index 1471e0a223a5..0d7765807f49 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -405,6 +405,7 @@ int of_irq_get(struct device_node *dev, int index)
> > 
> > return irq_create_of_mapping(&oirq);
> >  
> >  }
> > 
> > +EXPORT_SYMBOL_GPL(of_irq_get);
> > 
> >  /**
> >  
> >   * of_irq_get_byname - Decode a node's IRQ and return it as a Linux irq
> >   number

-- 
Regards,

Laurent Pinchart

--
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 0/3] Delay I2C OF IRQ mapping until probe time

[Laurent Pinchart]

Hello,

I recently ran into an issue with the OF IRQ parsing code in the I2C core
(of_i2c_register_devices in drivers/i2c/i2c-core.c).

My DT contains the following nodes.

gpio1: gpio@e6051000 {
...
#interrupt-cells = <2>;
interrupt-controller;
clocks = <&mstp9_clks R8A7790_CLK_GPIO1>;
};

iic2: i2c@e652 {
#address-cells = <1>;
#size-cells = <0>;
...
hdmi@39 {
compatible = "adi,adv7511w";
reg = <0x39>;
interrupt-parent = <&gpio1>;
interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
...
};
};

mstp9_clks: mstp9_clks@e6150994 {
...
};

The i2c@e652 node is probed before the gpio@e6051000 node. The
of_i2c_register_devices() function tries to register all children, including
hdmi@39. It tries to parse and map the I2C client IRQ by calling
irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't
probed yet. The adv7511 driver later probes the hdmi@39 device and gets
client->irq set to 0.

As we can't control the probe order in the general case we need to rely on
deferred probing. This patch series is an attempt to do so, by moving IRQ
mapping from device registration time to device probe time were probing can be
deferred.

Laurent Pinchart (3):
  of/irq: Export of_irq_get()
  i2c: core: Dispose OF IRQ mapping at client removal time
  i2c: core: Map OF IRQ at probe time

 drivers/i2c/i2c-core.c | 14 --
 drivers/of/irq.c   |  1 +
 2 files changed, 13 insertions(+), 2 deletions(-)

-- 
Regards,

Laurent Pinchart

--
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/3] i2c: core: Dispose OF IRQ mapping at client removal time

[Laurent Pinchart]

Clients instantiated from OF get an IRQ mapping created at device
registration time. Dispose the mapping when the client is removed.

Signed-off-by: Laurent Pinchart 
---
 drivers/i2c/i2c-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6a7f79..258765b29684 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -670,6 +670,9 @@ static int i2c_device_remove(struct device *dev)
status = driver->remove(client);
}
 
+   if (dev->of_node)
+   irq_dispose_mapping(client->irq);
+
dev_pm_domain_detach(&client->dev, true);
return status;
 }
-- 
2.0.4

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

[PATCH 3/3] i2c: core: Map OF IRQ at probe time

[Laurent Pinchart]

I2C clients instantiated from OF get their IRQ mapped at device
registration time. This leads to the IRQ being silently ignored if the
related irqchip hasn't been proved yet.

Fix this by moving IRQ mapping at probe time using of_get_irq(). The
function operates as irq_of_parse_and_map() but additionally returns
-EPROBE_DEFER if the irqchip isn't available, allowing us to defer I2C
client probing.

Signed-off-by: Laurent Pinchart 
---
 drivers/i2c/i2c-core.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 258765b29684..c6694f232240 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -631,6 +631,15 @@ static int i2c_device_probe(struct device *dev)
if (!client)
return 0;
 
+   if (!client->irq && dev->of_node) {
+   int irq = of_irq_get(dev->of_node, 0);
+
+   if (irq < 0)
+   return irq;
+
+   client->irq = irq;
+   }
+
driver = to_i2c_driver(dev->driver);
if (!driver->probe || !driver->id_table)
return -ENODEV;
@@ -1412,7 +1421,6 @@ static void of_i2c_register_devices(struct i2c_adapter 
*adap)
continue;
}
 
-   info.irq = irq_of_parse_and_map(node, 0);
info.of_node = of_node_get(node);
info.archdata = &dev_ad;
 
@@ -1426,7 +1434,6 @@ static void of_i2c_register_devices(struct i2c_adapter 
*adap)
dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
node->full_name);
of_node_put(node);
-   irq_dispose_mapping(info.irq);
continue;
}
}
-- 
2.0.4

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

[PATCH 1/3] of/irq: Export of_irq_get()

[Laurent Pinchart]

The function will be used by the I2C core which can be compiled as a
module.

Signed-off-by: Laurent Pinchart 
---
 drivers/of/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 1471e0a223a5..0d7765807f49 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -405,6 +405,7 @@ int of_irq_get(struct device_node *dev, int index)
 
return irq_create_of_mapping(&oirq);
 }
+EXPORT_SYMBOL_GPL(of_irq_get);
 
 /**
  * of_irq_get_byname - Decode a node's IRQ and return it as a Linux irq number
-- 
2.0.4

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

Re: I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hi Wolfram,

On Thursday 30 October 2014 14:21:36 Wolfram Sang wrote:
> > > See:
> > >   9ec36cafe43b of/irq: do irq resolution in platform_get_irq
> > > 
> > > I suspect a similar thing could be done for I2C.
> > 
> > That could work. We would need to introduce a new i2c_get_irq() function
> > though. Wolfram, would you be fine with that ?
> 
> I'd think it will look pretty similar to platform_get_irq, no? That is
> fine with me.

It would, but as Thierry pointed out it should be possible to hide the details 
in the I2C core. I'll submit a patch shortly.

-- 
Regards,

Laurent Pinchart

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

Re: I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hi Thierry,

On Thursday 30 October 2014 14:02:09 Thierry Reding wrote:
> On Thu, Oct 30, 2014 at 02:53:42PM +0200, Laurent Pinchart wrote:
> > On Monday 27 October 2014 13:58:19 Wolfram Sang wrote:
> > > > The i2c@e652 node is probed before the gpio@e6051000 node. The
> > > > of_i2c_register_devices() function tries to register all children,
> > > > including hdmi@39. It tries to parse and map the I2C client IRQ by
> > > > calling irq_of_parse_and_map(), which returns 0 as the interrupt
> > > > controller isn't probed yet. The adv7511 driver later probes the
> > > > hdmi@39
> > > > device and gets client->irq set to 0.
> > > 
> > > I've got this strange feeling of deja vu... Ah, here: Thierry Reding
> > 
> > > tackled this problem a year ago. His series:
> > Thanks for the pointer.
> > 
> > > https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference
> > > resolution)
> > > 
> > > He did a V2 (which never made it to the i2c list). Seems like the first
> > > two patches made it and the rest got stalled without discussion?
> > > 
> > > https://lkml.org/lkml/2013/9/18/216
> > > 
> > > Adding Thierry to the queue. Maybe he can bring some light to what
> > > happened to his series.
> > 
> > That's exactly what I need :-) Thierry, do you plan to respin the series ?
> 
> Not really. Like I said in my reply to Wolfram, a different set of
> patches was merged subsequently to solve this issue for platform
> devices, which makes my patchset mostly obsolete. But I think the
> solution that you proposed would work well if you do:
> 
> - int irq = irq_of_parse_and_map(dev->of_node, 0);
> + int irq = of_irq_get(dev->of_node, 0);
> 
> And then changing irq_create_of_mapping() should no longer be necessary.

It looks like it would work, yes. I'll test it and will resubmit my patch.

> I still think it's kind of lame to handle -EPROBE_DEFER specially as was
> done in of_irq_get(), but given how worried people were about the more
> invasive changes needed to propagate the correct error code all the way
> up it seems like that's as good as it's going to get.

-- 
Regards,

Laurent Pinchart

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

Re: I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hi Thierry,

On Thursday 30 October 2014 13:56:46 Thierry Reding wrote:
> On Mon, Oct 27, 2014 at 01:58:19PM +0100, Wolfram Sang wrote:
> > > The i2c@e652 node is probed before the gpio@e6051000 node. The
> > > of_i2c_register_devices() function tries to register all children,
> > > including hdmi@39. It tries to parse and map the I2C client IRQ by
> > > calling irq_of_parse_and_map(), which returns 0 as the interrupt
> > > controller isn't probed yet. The adv7511 driver later probes the hdmi@39
> > > device and gets client->irq set to 0.
> > 
> > I've got this strange feeling of deja vu... Ah, here: Thierry Reding
> > tackled this problem a year ago. His series:
> > 
> > https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference
> > resolution)
> > 
> > He did a V2 (which never made it to the i2c list). Seems like the first
> > two patches made it and the rest got stalled without discussion?
> > 
> > https://lkml.org/lkml/2013/9/18/216
> > 
> > Adding Thierry to the queue. Maybe he can bring some light to what
> > happened to his series.
> 
> I tried to fix it in a proper way, but it seems people were uneasy with
> how invasive the change was.

It was a bit invasive indeed and I can share the uneasiness, but on the other 
hand there was no real nack. I think I still prefer your approach, but can 
live with something simpler.

> At some point I lost interest. People ended up merging something that was
> similar, but side-stepped the issue of propagating error codes all the way
> up by introducing a new API and in case of of_irq_parse_one() failing doing
> an additional check to see if the reason was the missing IRQ domain.
> 
> See:
> 
>   9ec36cafe43b of/irq: do irq resolution in platform_get_irq
> 
> I suspect a similar thing could be done for I2C.

That could work. We would need to introduce a new i2c_get_irq() function 
though. Wolfram, would you be fine with that ?

-- 
Regards,

Laurent Pinchart

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

Re: I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hi Wolfram and Thierry,

On Monday 27 October 2014 13:58:19 Wolfram Sang wrote:
> > The i2c@e652 node is probed before the gpio@e6051000 node. The
> > of_i2c_register_devices() function tries to register all children,
> > including hdmi@39. It tries to parse and map the I2C client IRQ by
> > calling irq_of_parse_and_map(), which returns 0 as the interrupt
> > controller isn't probed yet. The adv7511 driver later probes the hdmi@39
> > device and gets client->irq set to 0.
> 
> I've got this strange feeling of deja vu... Ah, here: Thierry Reding
> tackled this problem a year ago. His series:

Thanks for the pointer.

> https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference
> resolution)
> 
> He did a V2 (which never made it to the i2c list). Seems like the first
> two patches made it and the rest got stalled without discussion?
> 
> https://lkml.org/lkml/2013/9/18/216
> 
> Adding Thierry to the queue. Maybe he can bring some light to what
> happened to his series.

That's exactly what I need :-) Thierry, do you plan to respin the series ?

-- 
Regards,

Laurent Pinchart

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

Re: I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hi Ezequiel,

On Thursday 30 October 2014 08:58:28 Ezequiel Garcia wrote:
> On 10/25/2014 07:13 PM, Laurent Pinchart wrote:
> > Hello,
> > 
> > I recently ran into an issue with the OF IRQ parsing code in the I2C core
> > (of_i2c_register_devices in drivers/i2c/i2c-core.c).
> > 
> > My DT contains the following nodes.
> > 
> > gpio1: gpio@e6051000 {
> > ...
> > #interrupt-cells = <2>;
> > interrupt-controller;
> > clocks = <&mstp9_clks R8A7790_CLK_GPIO1>;
> > };
> > 
> > iic2: i2c@e652 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > ...
> > hdmi@39 {
> > compatible = "adi,adv7511w";
> > reg = <0x39>;
> > interrupt-parent = <&gpio1>;
> > interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
> > ...
> > };
> > };
> > 
> > mstp9_clks: mstp9_clks@e6150994 {
> > ...
> > };
> > 
> > The i2c@e652 node is probed before the gpio@e6051000 node. The
> > of_i2c_register_devices() function tries to register all children,
> > including hdmi@39. It tries to parse and map the I2C client IRQ by
> > calling irq_of_parse_and_map(), which returns 0 as the interrupt
> > controller isn't probed yet. The adv7511 driver later probes the hdmi@39
> > device and gets client->irq set to 0.
> > 
> > We can't control the probe order.
> 
> Maybe I'm missing something, but I think your i2c adapter is probed with
> a subsys_initcall (as many other adapters). Otherwise, I can't see why
> it would be probed before the the gpio controller.
> 
> I think this initcall is your problem. Have you tried just using
> platform_driver's probe?

My I2C controller driver uses module_platform_driver(). The reason why the 
GPIO controller is probed later is because the GPIO requires a clock, and the 
clock device is probed after the I2C controller, resulting in a deferred 
probing the first time the GPIO controller is probed.

In the general case probe ordering can't be controlled, especially with DT. I 
believe we thus need a generic solution.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/2] i2c: Add generic support passing secondary devices addresses

[Laurent Pinchart]

Hi Jean-Michel,

On Thursday 23 October 2014 07:59:53 Jean-Michel Hautbois wrote:
> 2014-10-23 1:37 GMT+02:00 Laurent Pinchart:
> > On Wednesday 22 October 2014 17:30:47 Jean-Michel Hautbois wrote:
> >> Some I2C devices have multiple addresses assigned, for example each
> >> address corresponding to a different internal register map page of the
> >> device. So far drivers which need support for this have handled this with
> >> a driver specific and non-generic implementation, e.g. passing the
> >> additional address via platform data.
> >> 
> >> This patch provides a new helper function called
> >> i2c_new_secondary_device()
> >> which is intended to provide a generic way to get the secondary address
> >> as well as instantiate a struct i2c_client for the secondary address.
> >> 
> >> The function expects a pointer to the primary i2c_client, a name
> >> for the secondary address and an optional default address. The name is
> >> used as a handle to specify which secondary address to get.
> >> 
> >> The default address is used as a fallback in case no secondary address
> >> was explicitly specified. In case no secondary address and no default
> >> address were specified the function returns NULL.
> >> 
> >> For now the function only supports look-up of the secondary address
> >> from devicetree, but it can be extended in the future
> >> to for example support board files and/or ACPI.
> > 
> > As this is core code I believe the DT bindings should be documented
> > somewhere in Documentation/devicetree/bindings/i2c/.
> 
> Mmh, probably yes, but I don't know where precisely, as all the
> bindings are devices specific here...

Lucky you, you can create the I2C core DT bindings documentation :-) 
Documentation/devicetree/bindings/i2c/i2c.txt sounds like a good candidate.

> >> Signed-off-by: Jean-Michel Hautbois 
> >> ---
> >> 
> >>  drivers/i2c/i2c-core.c | 40 
> >>  include/linux/i2c.h|  8 
> >>  2 files changed, 48 insertions(+)
> >> 
> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >> index 2f90ac6..fd3b07c 100644
> >> --- a/drivers/i2c/i2c-core.c
> >> +++ b/drivers/i2c/i2c-core.c
> >> @@ -1166,6 +1166,46 @@ struct i2c_client *i2c_new_dummy(struct
> >> i2c_adapter
> >> *adapter, u16 address) }
> >> 
> >>  EXPORT_SYMBOL_GPL(i2c_new_dummy);
> >> 
> >> +/**
> >> + * i2c_new_secondary_device - Helper to get the instantiated secondary
> >> address
> > 
> > It does more than that, it also creates the device.
> 
> Right, how about :
> + * i2c_new_secondary_device - Helper to get the instantiated secondary
> address
> + * and create the associated device
> 
> >> + * @client: Handle to the primary client
> >> + * @name: Handle to specify which secondary address to get
> >> + * @default_addr: Used as a fallback if no secondary address was
> >> specified
> >> + * Context: can sleep
> >> + *
> >> + * This returns an I2C client bound to the "dummy" driver based on DT
> >> parsing.
> > 
> > Could you elaborate on that ? I would explain that the address is
> > retrieved from the firmware based on the name, and that default_addr is
> > used in case the firmware doesn't provide any information.
> 
> Something like that ?
> + * This returns an I2C client bound to the "dummy" driver based on DT
> parsing.
> + * It retrieves the address based on the name.
> + * It uses default_addr if no information is provided by firmware.

"I2C clients can be composed of multiple I2C slaves bound together in a single 
component. The I2C client driver then binds to the master I2C slave and needs 
to create I2C dummy clients to communicate with all the other slaves.

This function creates an returns an I2C dummy client whose I2C address is 
retrieved from the platform firmware based on the given slave name. If no 
address is specified by the firmware default_addr is used.

On DT-based platforms the address is retrieved from the "reg" property entry 
cell whose "reg-names" value matches the slave name."

> >> + *
> >> + * This returns the new i2c client, which should be saved for later use
> >> with
> >> + * i2c_unregister_device(); or NULL to indicate an error.
> >> + */
> >> +struct i2c_client *i2c_new_secondary_device(struct i2c_client *client,
> >> + const cha

I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hello,

I recently ran into an issue with the OF IRQ parsing code in the I2C core
(of_i2c_register_devices in drivers/i2c/i2c-core.c).

My DT contains the following nodes.

gpio1: gpio@e6051000 {
...
#interrupt-cells = <2>;
interrupt-controller;
clocks = <&mstp9_clks R8A7790_CLK_GPIO1>;
};

iic2: i2c@e652 {
#address-cells = <1>;
#size-cells = <0>;
...
hdmi@39 {
compatible = "adi,adv7511w";
reg = <0x39>;
interrupt-parent = <&gpio1>;
interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
...
};
};

mstp9_clks: mstp9_clks@e6150994 {
...
};

The i2c@e652 node is probed before the gpio@e6051000 node. The
of_i2c_register_devices() function tries to register all children, including
hdmi@39. It tries to parse and map the I2C client IRQ by calling
irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't
probed yet. The adv7511 driver later probes the hdmi@39 device and gets
client->irq set to 0.

We can't control the probe order. One option to solve the problem would be to
defer probing of the I2C client until the interrupt control is available. The
following patch is a naive implementation. Before turning it into a real
upstream candidate (changing the return type of irq_create_of_mapping without
checking the callers is a no-go) I'd like to get feedback on the approach.

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6a7f79..73ff1e7d2382 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -631,6 +631,14 @@ static int i2c_device_probe(struct device *dev)
if (!client)
return 0;
 
+   if (!client->irq && dev->of_node) {
+   int irq = irq_of_parse_and_map(dev->of_node, 0);
+   if (irq < 0)
+   return irq;
+
+   client->irq = irq;
+   }
+
driver = to_i2c_driver(dev->driver);
if (!driver->probe || !driver->id_table)
return -ENODEV;
@@ -1409,7 +1417,6 @@ static void of_i2c_register_devices(struct i2c_adapter 
*adap)
continue;
}
 
-   info.irq = irq_of_parse_and_map(node, 0);
info.of_node = of_node_get(node);
info.archdata = &dev_ad;
 
@@ -1423,7 +1430,6 @@ static void of_i2c_register_devices(struct i2c_adapter 
*adap)
dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
node->full_name);
of_node_put(node);
-   irq_dispose_mapping(info.irq);
continue;
}
}
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index bfec136a6d1e..81a14d89a5bd 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -34,7 +34,7 @@ static inline int of_irq_parse_oldworld(struct device_node 
*device, int index,
 extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args 
*out_irq);
 extern int of_irq_parse_one(struct device_node *device, int index,
  struct of_phandle_args *out_irq);
-extern unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data);
+extern int irq_create_of_mapping(struct of_phandle_args *irq_data);
 extern int of_irq_to_resource(struct device_node *dev, int index,
  struct resource *r);
 extern int of_irq_to_resource_table(struct device_node *dev,
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 6534ff6ce02e..386fd09f4c85 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -466,7 +466,7 @@ int irq_create_strict_mappings(struct irq_domain *domain, 
unsigned int irq_base,
 }
 EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
 
-unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
+int irq_create_of_mapping(struct of_phandle_args *irq_data)
 {
struct irq_domain *domain;
irq_hw_number_t hwirq;
@@ -477,7 +477,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args 
*irq_data)
if (!domain) {
pr_warn("no irq domain found for %s !\n",
of_node_full_name(irq_data->np));
-   return 0;
+   return -EPROBE_DEFER;
}
 
/* If domain has no translation, then we assume interrupt line */

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 2/2] adv7604: Add support for i2c_new_secondary_device

[Laurent Pinchart]

te) }
> 
>  static struct i2c_client *adv7604_dummy_client(struct v4l2_subdev *sd,
> - u8 addr, u8 io_reg)
> + unsigned int i)
>  {
>   struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct adv7604_platform_data *pdata = client->dev.platform_data;
> + unsigned int io_reg = 0xf2 + i;
> + unsigned int default_addr = io_read(sd, io_reg) >> 1;

The variable isn't used.

> + struct i2c_client *new_client;
> +
> + if (pdata && pdata->i2c_addresses[i])
> + new_client = i2c_new_dummy(client->adapter,
> + pdata->i2c_addresses[i]);
> + else
> + new_client = i2c_new_secondary_device(client,
> + adv7604_secondary_names[i].name,
> + adv7604_secondary_names[i].default_addr);
> +
> + if (new_client)
> + io_write(sd, io_reg, new_client->addr << 1);
> 
> - if (addr)
> - io_write(sd, io_reg, addr << 1);
> - return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
> + return new_client;
>  }
> 
>  static const struct adv7604_reg_seq adv7604_recommended_settings_afe[] = {
> @@ -2718,20 +2752,6 @@ static int adv7604_parse_dt(struct adv7604_state
> *state) /* Disable the interrupt for now as no DT-based board uses it. */
> state->pdata.int1_config = ADV7604_INT1_CONFIG_DISABLED;
> 
> - /* Use the default I2C addresses. */
> - state->pdata.i2c_addresses[ADV7604_PAGE_AVLINK] = 0x42;
> - state->pdata.i2c_addresses[ADV7604_PAGE_CEC] = 0x40;
> - state->pdata.i2c_addresses[ADV7604_PAGE_INFOFRAME] = 0x3e;
> - state->pdata.i2c_addresses[ADV7604_PAGE_ESDP] = 0x38;
> - state->pdata.i2c_addresses[ADV7604_PAGE_DPP] = 0x3c;
> - state->pdata.i2c_addresses[ADV7604_PAGE_AFE] = 0x26;
> - state->pdata.i2c_addresses[ADV7604_PAGE_REP] = 0x32;
> - state->pdata.i2c_addresses[ADV7604_PAGE_EDID] = 0x36;
> - state->pdata.i2c_addresses[ADV7604_PAGE_HDMI] = 0x34;
> - state->pdata.i2c_addresses[ADV7604_PAGE_TEST] = 0x30;
> - state->pdata.i2c_addresses[ADV7604_PAGE_CP] = 0x22;
> - state->pdata.i2c_addresses[ADV7604_PAGE_VDP] = 0x24;
> -
>   /* Hardcode the remaining platform data fields. */
>   state->pdata.disable_pwrdnb = 0;
>   state->pdata.disable_cable_det_rst = 0;
> @@ -2892,8 +2912,7 @@ static int adv7604_probe(struct i2c_client *client,
>   continue;
> 
>   state->i2c_clients[i] =
> - adv7604_dummy_client(sd, state->pdata.i2c_addresses[i],
> -  0xf2 + i);
> + adv7604_dummy_client(sd, i);
>   if (state->i2c_clients[i] == NULL) {
>   err = -ENOMEM;
>   v4l2_err(sd, "failed to create i2c client %u\n", i);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/2] i2c: Add generic support passing secondary devices addresses

[Laurent Pinchart]

 generic way to get the secondary address
> + * as well as a client handle to this extra address.
> + */

The function is already documented in i2c-core.c, I would ditch this comment.

> +extern struct i2c_client *
> +i2c_new_secondary_device(struct i2c_client *client,
> + const char *name,
> + u16 default_addr);
> +
>  extern void i2c_unregister_device(struct i2c_client *);
>  #endif /* I2C */

-- 
Regards,

Laurent Pinchart

--
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: rcar: remove sign-compare flaw

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Saturday 20 September 2014 12:07:37 Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> gcc rightfully says:
> 
> drivers/i2c/busses/i2c-rcar.c:198:10: warning: comparison between signed and
> unsigned integer expressions [-Wsign-compare]
> 
> Signed-off-by: Wolfram Sang 

Acked-by: Laurent Pinchart 

> ---
>  drivers/i2c/busses/i2c-rcar.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 4c6fa78063fb..1eed463662c4 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -195,7 +195,7 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv
> *priv, */
>   rate = clk_get_rate(priv->clk);
>   cdf = rate / 2000;
> - if (cdf >= 1 << cdf_width) {
> + if (cdf >= 1U << cdf_width) {
>   dev_err(dev, "Input clock %lu too high\n", rate);
>   return -EIO;
>   }

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 2/2] adv7604: Use DT parsing in dummy creation

[Laurent Pinchart]

tform_data;
> + unsigned int io_reg = 0xf2 + i;
> + unsigned int default_addr = io_read(sd, io_reg) >> 1;

This modifies the behaviour of the driver. It previously used fixed default 
addresses in the DT case, and now defaults to whatever has been programmed in 
the chip. This might not be an issue in itself, but it should be documented in 
the commit message (and possibly split to a separate patch).

> + struct i2c_client *new_client;
> +
> + if (IS_ENABLED(CONFIG_OF)) {
> + /* Try to find it in DT */
> + new_client = i2c_new_secondary_device(client,
> + adv7604_secondary_names[i], default_addr);
> + } else if (pdata) {
> + if (pdata->i2c_addresses[i])
> + new_client = i2c_new_dummy(client->adapter,
> + pdata->i2c_addresses[i]);
> + else
> + new_client = i2c_new_dummy(client->adapter,
> + default_addr);
> + }
> 
> - if (addr)
> - io_write(sd, io_reg, addr << 1);
> - return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
> + if (new_client)
> + io_write(sd, io_reg, new_client->addr << 1);
> +
> + return new_client;
>  }
> 
>  static const struct adv7604_reg_seq adv7604_recommended_settings_afe[] = {
> @@ -2677,6 +2711,7 @@ MODULE_DEVICE_TABLE(i2c, adv7604_i2c_id);
> 
>  static struct of_device_id adv7604_of_id[] __maybe_unused = {
>   { .compatible = "adi,adv7611", .data = &adv7604_chip_info[ADV7611] },
> + { .compatible = "adi,adv7604", .data = &adv7604_chip_info[ADV7604] },
>   { }
>  };
>  MODULE_DEVICE_TABLE(of, adv7604_of_id);
> @@ -2717,20 +2752,6 @@ static int adv7604_parse_dt(struct adv7604_state
> *state) /* Disable the interrupt for now as no DT-based board uses it. */
> state->pdata.int1_config = ADV7604_INT1_CONFIG_DISABLED;
> 
> - /* Use the default I2C addresses. */
> - state->pdata.i2c_addresses[ADV7604_PAGE_AVLINK] = 0x42;
> - state->pdata.i2c_addresses[ADV7604_PAGE_CEC] = 0x40;
> - state->pdata.i2c_addresses[ADV7604_PAGE_INFOFRAME] = 0x3e;
> - state->pdata.i2c_addresses[ADV7604_PAGE_ESDP] = 0x38;
> - state->pdata.i2c_addresses[ADV7604_PAGE_DPP] = 0x3c;
> - state->pdata.i2c_addresses[ADV7604_PAGE_AFE] = 0x26;
> - state->pdata.i2c_addresses[ADV7604_PAGE_REP] = 0x32;
> - state->pdata.i2c_addresses[ADV7604_PAGE_EDID] = 0x36;
> - state->pdata.i2c_addresses[ADV7604_PAGE_HDMI] = 0x34;
> - state->pdata.i2c_addresses[ADV7604_PAGE_TEST] = 0x30;
> - state->pdata.i2c_addresses[ADV7604_PAGE_CP] = 0x22;
> - state->pdata.i2c_addresses[ADV7604_PAGE_VDP] = 0x24;
> -
>   /* Hardcode the remaining platform data fields. */
>   state->pdata.disable_pwrdnb = 0;
>   state->pdata.disable_cable_det_rst = 0;
> @@ -2891,8 +2912,7 @@ static int adv7604_probe(struct i2c_client *client,
>   continue;
> 
>   state->i2c_clients[i] =
> - adv7604_dummy_client(sd, state->pdata.i2c_addresses[i],
> -  0xf2 + i);
> + adv7604_dummy_client(sd, i);
>   if (state->i2c_clients[i] == NULL) {
>   err = -ENOMEM;
>   v4l2_err(sd, "failed to create i2c client %u\n", i);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2] i2c: pca954x: put the mux to disconnected state after resume

[Laurent Pinchart]

Hi Jisheng,

Thank you for the patch.

On Friday 25 July 2014 19:57:46 Jisheng Zhang wrote:
> pca954x may be power lost during suspend, so after resume we also suffer
> the issue fixed by commit cd823db8b1161ef0d756514d280715a576d65cc3,
> 
>  "pca954x power-on default is channel 0 connected. If multiple pca954x
>  muxes are connected to the same physical I2C bus, the parent bus will
>  see channel 0 devices behind both muxes by default."
> 
> What's more, when resume bootloader may also operate the mux, so the
> the channel connected after that may not be the one driver thought.
> 
> We fix this problem by putting the mux to disconnected state and
> clearing last_chan in the resume hook.
> 
> Signed-off-by: Jisheng Zhang 

Acked-by: Laurent Pinchart 

> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> b/drivers/i2c/muxes/i2c-mux-pca954x.c index 9bd4212..ec11b40 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -41,6 +41,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> 
>  #define PCA954X_MAX_NCHANS 8
> @@ -273,9 +274,23 @@ static int pca954x_remove(struct i2c_client *client)
>   return 0;
>  }
> 
> +#ifdef CONFIG_PM_SLEEP
> +static int pca954x_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct pca954x *data = i2c_get_clientdata(client);
> +
> + data->last_chan = 0;
> + return i2c_smbus_write_byte(client, 0);
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(pca954x_pm, NULL, pca954x_resume);
> +
>  static struct i2c_driver pca954x_driver = {
>   .driver = {
>   .name   = "pca954x",
> + .pm = &pca954x_pm,
>   .owner  = THIS_MODULE,
>   },
>   .probe  = pca954x_probe,

-- 
Regards,

Laurent Pinchart

--
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: randconfig build error with next-20140604, in drivers/i2c/muxes/i2c-mux-pca954x.c

[Laurent Pinchart]

On Monday 09 June 2014 15:12:01 Linus Walleij wrote:
> On Wed, Jun 4, 2014 at 6:44 PM, Jim Davis  wrote:
> > Building with the attached random configuration file,
> > 
> > drivers/i2c/muxes/i2c-mux-pca954x.c: In function ‘pca954x_probe’:
> > drivers/i2c/muxes/i2c-mux-pca954x.c:204:2: error: implicit declaration
> > of function ‘devm_gpiod_get’ [-Werror=implicit-function-declaration]
> > 
> >   gpio = devm_gpiod_get(&client->dev, "reset");
> >   ^
> > 
> > drivers/i2c/muxes/i2c-mux-pca954x.c:204:7: warning: assignment makes
> > pointer from integer without a cast [enabled by default]
> > 
> >   gpio = devm_gpiod_get(&client->dev, "reset");
> >   
> >^
> > 
> > drivers/i2c/muxes/i2c-mux-pca954x.c:206:3: error: implicit declaration
> > of function ‘gpiod_direction_output’
> > [-Werror=implicit-function-declaration]
> > 
> >gpiod_direction_output(gpio, 0);
> >^
> > 
> > cc1: some warnings being treated as errors
> > make[3]: *** [drivers/i2c/muxes/i2c-mux-pca954x.o] Error 1
> 
> I've sent a patch for this to Wolfram and the i2c discuss list.

https://git.kernel.org/cgit/linux/kernel/git/wsa/linux.git/commit/?h=i2c/for-next&id=642653d16a0f8e78b7a25d930b62aa771ebc939c

-- 
Regards,

Laurent Pinchart

--
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: mux: pca954x needs gpiolib

[Laurent Pinchart]

Hi Arnd,

On Thursday 05 June 2014 13:24:16 Arnd Bergmann wrote:
> On Thursday 05 June 2014 12:56:08 Laurent Pinchart wrote:
> > On Thursday 05 June 2014 12:44:47 Arnd Bergmann wrote:
> > > commit 4807e8459bce ("i2c: mux: pca954x: Use the descriptor-based GPIO
> > > API") moved this driver over to the gpio descriptor API, which means
> > > we now have a dependency on GPIOLIB and get this build error when
> > > it is disabled:
> > > 
> > > i2c/muxes/i2c-mux-pca954x.c: In function 'pca954x_probe':
> > > i2c/muxes/i2c-mux-pca954x.c:204:2: error: implicit declaration of
> > > function
> > > 'devm_gpiod_get' [-Werror=implicit-function-declaration] gpio =
> > > devm_gpiod_get(&client->dev, "reset");
> > >   ^
> > > 
> > > i2c/muxes/i2c-mux-pca954x.c:204:7: warning: assignment makes pointer
> > > from
> > > integer without a cast [enabled by default] gpio =
> > > devm_gpiod_get(&client->dev, "reset");
> > >^
> > > 
> > > i2c/muxes/i2c-mux-pca954x.c:206:3: error: implicit declaration of
> > > function
> > > 'gpiod_direction_output' [-Werror=implicit-function-declaration]
> > > gpiod_direction_output(gpio, 0);
> > >^
> > > 
> > > This adds the dependency in Kconfig as we do for other similar drivers.
> > 
> > I've sent "i2c: pca954x: Fix compilation without CONFIG_GPIOLIB"
> > yesterday, which fixes the compilation issue by including
> > . When CONFIG_GPIOLIB isn't set the header defines
> > stub functions, keeping the driver usable without GPIOLIB support.
> 
> Ok, makes sense. Should we remove the 'depends on GPIOLIB' from other
> drivers doing the same, too?

When the GPIO is optional I think so.

-- 
Regards,

Laurent Pinchart

--
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: mux: pca954x needs gpiolib

[Laurent Pinchart]

Hi Arnd,

On Thursday 05 June 2014 12:44:47 Arnd Bergmann wrote:
> commit 4807e8459bce ("i2c: mux: pca954x: Use the descriptor-based GPIO
> API") moved this driver over to the gpio descriptor API, which means
> we now have a dependency on GPIOLIB and get this build error when
> it is disabled:
> 
> i2c/muxes/i2c-mux-pca954x.c: In function 'pca954x_probe':
> i2c/muxes/i2c-mux-pca954x.c:204:2: error: implicit declaration of function
> 'devm_gpiod_get' [-Werror=implicit-function-declaration] gpio =
> devm_gpiod_get(&client->dev, "reset");
>   ^
> i2c/muxes/i2c-mux-pca954x.c:204:7: warning: assignment makes pointer from
> integer without a cast [enabled by default] gpio =
> devm_gpiod_get(&client->dev, "reset");
>^
> i2c/muxes/i2c-mux-pca954x.c:206:3: error: implicit declaration of function
> 'gpiod_direction_output' [-Werror=implicit-function-declaration]
> gpiod_direction_output(gpio, 0);
>^
> 
> This adds the dependency in Kconfig as we do for other similar drivers.

I've sent "i2c: pca954x: Fix compilation without CONFIG_GPIOLIB" yesterday, 
which fixes the compilation issue by including . When 
CONFIG_GPIOLIB isn't set the header defines stub functions, keeping the driver 
usable without GPIOLIB support.

> Signed-off-by: Arnd Bergmann 
> Cc: Laurent Pinchart 
> Cc: Linus Walleij 
> Cc: Wolfram Sang 
> 
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index f7f9865b..f6d313e 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -40,6 +40,7 @@ config I2C_MUX_PCA9541
> 
>  config I2C_MUX_PCA954x
>   tristate "Philips PCA954x I2C Mux/switches"
> + depends on GPIOLIB
>   help
> If you say yes here you get support for the Philips PCA954x
> I2C mux/switch devices.

-- 
Regards,

Laurent Pinchart

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

[PATCH] i2c: pca954x: Fix compilation without CONFIG_GPIOLIB

[Laurent Pinchart]

The pca954x driver recently switched to the GPIO descriptor API without
including the correct  header. This breaks
compilation without CONFIG_GPIOLIB.

drivers/i2c/muxes/i2c-mux-pca954x.c: In function ‘pca954x_probe’:
drivers/i2c/muxes/i2c-mux-pca954x.c:204:2: error: implicit declaration
of function ‘devm_gpiod_get’ [-Werror=implicit-function-declaration]
  gpio = devm_gpiod_get(&client->dev, "reset");
  ^
drivers/i2c/muxes/i2c-mux-pca954x.c:204:7: warning: assignment makes
pointer from integer without a cast [enabled by default]
  gpio = devm_gpiod_get(&client->dev, "reset");
   ^
drivers/i2c/muxes/i2c-mux-pca954x.c:206:3: error: implicit declaration
of function ‘gpiod_direction_output’
[-Werror=implicit-function-declaration]
   gpiod_direction_output(gpio, 0);
   ^
cc1: some warnings being treated as errors
make[3]: *** [drivers/i2c/muxes/i2c-mux-pca954x.o] Error 1

Fix it by including the right header.

Reported-by: Jim Davis 
Signed-off-by: Laurent Pinchart 
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c 
b/drivers/i2c/muxes/i2c-mux-pca954x.c
index c2c443f..9bd4212 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -36,12 +36,11 @@
  */
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #define PCA954X_MAX_NCHANS 8
-- 
Regards,

Laurent Pinchart

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

[PATCH v2] i2c: pca954x: Use the descriptor-based GPIO API

[Laurent Pinchart]

The ID-based GPIO API pushes handling of GPIO polarity to drivers.
Simplify the driver by switching to the descriptor-based GPIO API.

This also fixes a mismatch between the pca954x DT bindings that document
a "reset-gpios" property and the driver that requests a "reset-gpio"
property.

Signed-off-by: Laurent Pinchart 
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 21 +
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c 
b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 550bd36..c2c443f 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -186,7 +186,7 @@ static int pca954x_probe(struct i2c_client *client,
 {
struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
-   struct device_node *np = client->dev.of_node;
+   struct gpio_desc *gpio;
int num, force, class;
struct pca954x *data;
int ret;
@@ -200,21 +200,10 @@ static int pca954x_probe(struct i2c_client *client,
 
i2c_set_clientdata(client, data);
 
-   if (IS_ENABLED(CONFIG_OF) && np) {
-   enum of_gpio_flags flags;
-   int gpio;
-
-   /* Get the mux out of reset if a reset GPIO is specified. */
-   gpio = of_get_named_gpio_flags(np, "reset-gpio", 0, &flags);
-   if (gpio_is_valid(gpio)) {
-   ret = devm_gpio_request_one(&client->dev, gpio,
-   flags & OF_GPIO_ACTIVE_LOW ?
-   GPIOF_OUT_INIT_HIGH : 
GPIOF_OUT_INIT_LOW,
-   "pca954x reset");
-   if (ret < 0)
-   return ret;
-   }
-   }
+   /* Get the mux out of reset if a reset GPIO is specified. */
+   gpio = devm_gpiod_get(&client->dev, "reset");
+   if (!IS_ERR(gpio))
+   gpiod_direction_output(gpio, 0);
 
/* Write the mux register at addr to verify
 * that the mux is in fact present. This also
-- 
Regards,

Laurent Pinchart

--
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: pca954x: Fix reset GPIO property name

[Laurent Pinchart]

Hi Alexandre,

On Tuesday 03 June 2014 10:38:53 Alexandre Courbot wrote:
> On 06/03/2014 09:22 AM, Laurent Pinchart wrote:
> > The DT bindings for the pca954x document that the reset GPIO is
> > specified through the "reset-gpios" property. However, the driver
> > erroneously uses a property name of "reset-gpio".
> > 
> > The GPIO DT bindings documentation mentions that
> > 
> > "GPIO properties should be named "[-]gpios". The exact meaning of
> > each gpios property must be documented in the device tree binding for
> > each device."
> > 
> > The correct reset GPIO property name is thus "reset-gpios". Fix the
> > driver accordingly.
> 
> The plural form should be preferred indeed.
> 
> > Signed-off-by: Laurent Pinchart 
> > ---
> > 
> >   drivers/i2c/muxes/i2c-mux-pca954x.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > While commit dd34c37aa3e81715a1ed8da61fa438072428e188
> > 
> > Author: Thierry Reding 
> > Date:   Wed Apr 23 17:28:09 2014 +0200
> > 
> >  gpio: of: Allow -gpio suffix for property names
> >  
> >  Many bindings use the -gpio suffix in property names. Support this in
> >  addition to the -gpios suffix when requesting GPIOs using the new
> >  descriptor-based API.
> > 
> > adds support for the singular form of the property, the GPIO DT bindings
> > document the plural form only. I've thus decided to fix the driver instead
> > of the bindings, even though it could be argued that the singular form
> > makes more sense in this case. I've CC'ed the people involved with the
> > above commit for comments on that, and have no issue fixing the bindings
> > instead of the driver if the singular form is preferred.
> > 
> > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > b/drivers/i2c/muxes/i2c-mux-pca954x.c index 550bd36..73491ca 100644
> > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > @@ -205,7 +205,7 @@ static int pca954x_probe(struct i2c_client *client,
> > 
> > int gpio;
> > 
> > /* Get the mux out of reset if a reset GPIO is specified. */
> > 
> > -   gpio = of_get_named_gpio_flags(np, "reset-gpio", 0, &flags);
> > +   gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags);
> 
> Won't this break DT compatibility for potential users of this driver? (I
> cannot find any in mainline though)

Only if those users don't conform to the DT bindings documentation that 
mandates the plural form, and are thus broken already ;-)

> Considering that this GPIO is only acquired and set once and for all
> during probe (a very simple use-case), how about switching this to the
> gpiod interface instead? That way, you can take advantage of Thierry's
> patch and the plural form will work while also maintaining the singular
> form for backward compatibility.

Regardless of the singular vs. plural debate I think that's a good idea. I'll 
thus rework this patch.

-- 
Regards,

Laurent Pinchart

--
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 10/16] i2c: sh_mobile: remove unnecessary OOM messages

[Laurent Pinchart]

Hi Jingoo,

Thank you for the patch.

On Wednesday 07 May 2014 13:27:22 Jingoo Han wrote:
> The site-specific OOM messages are unnecessary, because they
> duplicate the MM subsystem generic OOM message.
> 
> Signed-off-by: Jingoo Han 

Acked-by: Laurent Pinchart 

> ---
>  drivers/i2c/busses/i2c-sh_mobile.c |4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sh_mobile.c
> b/drivers/i2c/busses/i2c-sh_mobile.c index 1d79585..fbc13e9 100644
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -665,10 +665,8 @@ static int sh_mobile_i2c_probe(struct platform_device
> *dev) int ret;
> 
>   pd = kzalloc(sizeof(struct sh_mobile_i2c_data), GFP_KERNEL);
> - if (pd == NULL) {
> - dev_err(&dev->dev, "cannot allocate private data\n");
> + if (pd == NULL)
>   return -ENOMEM;
> - }
> 
>   pd->clk = clk_get(&dev->dev, NULL);
>   if (IS_ERR(pd->clk)) {

-- 
Regards,

Laurent Pinchart

--
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-mux-pca954x: allow downstream bus numbers to be specified in the dts

[Laurent Pinchart]

Hi Frank,

On Thursday 17 April 2014 16:26:04 Frank Bormann wrote:
> A user-space application, which is being configured with the first mux bus
> number, executes i2cget/i2cset commands, specifying the i2c bus number and
> expecting them to be incremental from the first number.

I won't venture to comment on whether bus numbering has ever been considered 
to be a kernel ABI, other people should be able to comment on that. Any chance 
to fix the application to find the bus numbers dynamically ?

> On Thu, Apr 17, 2014 at 4:15 PM, Laurent Pinchart wrote:
> > On Thursday 17 April 2014 13:54:44 Frank Bormann wrote:
> > > Hi Laurent,
> > > 
> > > I have a pca9546 on one of my i2c buses. This will create four mux buses
> > > in Linux. One of those mux buses then has a pca9542 connected to it,
> > > creating another two mux buses on top of the first ones.
> > > 
> > > The 3.8 kernel, I was initially using enumerated, this as:
> > > 
> > > pca9546: 3, 4, 5, 6
> > > pca9542: 7, 8
> > > 
> > > However, the new 3.12 kernel, I am using now, has changed that
> > > enumeration to:
> > > 
> > > pca9546: 3, 4, 7, 8
> > > pca9542: 5, 6
> > > 
> > > As you may imagine, the pca9542 is connected to bus 4. Apparently,
> > > previously, it would finish the initalization of pca9546 before dealing
> > > with the pca9542. Now it seems to do the pca9542 initialization as soon
> > > as it sees it on mux bus 4. My application however expects the pca9546
> > > buses to be on incremental bus numbers.
> > 
> > I understand that the bus numbers changed, but you still haven't explain
> > *why* you need to have fixed bus numbers. Why do you need to know the bus
> > number at all ?

-- 
Regards,

Laurent Pinchart

--
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-mux-pca954x: allow downstream bus numbers to be specified in the dts

[Laurent Pinchart]

Hi Frank,

On Thursday 17 April 2014 13:54:44 Frank Bormann wrote:
> Hi Laurent,
> 
> I have a pca9546 on one of my i2c buses. This will create four mux buses in
> Linux. One of those mux buses then has a pca9542 connected to it, creating
> another two mux buses on top of the first ones.
> 
> The 3.8 kernel, I was initially using enumerated, this as:
> 
> pca9546: 3, 4, 5, 6
> pca9542: 7, 8
> 
> However, the new 3.12 kernel, I am using now, has changed that enumeration
> to:
> 
> pca9546: 3, 4, 7, 8
> pca9542: 5, 6
> 
> As you may imagine, the pca9542 is connected to bus 4. Apparently,
> previously, it would finish the initalization of pca9546 before dealing
> with the pca9542. Now it seems to do the pca9542 initialization as soon as
> it sees it on mux bus 4. My application however expects the pca9546 buses
> to be on incremental bus numbers.

I understand that the bus numbers changed, but you still haven't explain *why* 
you need to have fixed bus numbers. Why do you need to know the bus number at 
all ?

-- 
Regards,

Laurent Pinchart

--
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-mux-pca954x: allow downstream bus numbers to be specified in the dts

[Laurent Pinchart]

to the
> >> driver seems to indicate that the driver's probe function is in fact the
> >> right place to read additional configuration from the dtb.
> >> 
> >> Any help is greatly appreciated.
> > 
> > You're looking at two different configuration mechanisms, which probably
> > explains your confusion.
> > 
> > Platform data is the oldest mechanism used to pass configuration
> > information to the driver. This is largely used through the Linux kernel
> > on a wide range of architectures. The idea is to store device-specific
> > configuration information in board files (as you mentioned DT I'll assume
> > you're working on ARM, so that would be arch/arm/mach-*/board-*.c) using
> > driver-specific structures and associate those structures with devices.
> > Drivers can then retrieve the platform data at probe time and configure
> > the device accordingly.
> > 
> > The way platform data is associated with devices depends on the bus type.
> > For I2C the i2c_board_info structure, used to instantiate I2C devices in
> > board code, has a void *platform_data field that can be set to point to a
> > platform data structure. You can find an example of this in the
> > i2c3_devices array in arch/arm/mach-shmobile/board-kzm9g.c.
> > 
> > Device tree (DT) is a newer mechanism to specify hardware configuration.
> > Instead of relying on C board files that contain a mix a code and data,
> > the
> > platform is described in a tree-like structure of devices with properties
> > associated to all those devices. That structure is called the device tree
> > and is compiled as a binary that gets passed to the kernel at boot time.
> > When using the device tree, drivers don't receive platform data anymore
> > but are responsible for parsing the content of the device tree to read
> > platform- specific hardware configuration information. The content of
> > device tree nodes is defined in documents called DT bindings that can be
> > found in
> > Documentation/devicetree/bindings/ (i2c/i2c-mux-pca954x.txt in this case).
> > 
> > A NULL platform_data pointer can then mean either that your platform boots
> > using the device tree, or that the board file doesn't specify any platform
> > data for the device in case of legacy (non-DT) boot. There's also a hybrid
> > method that can be used to associated platform data declared in C files to
> > DT nodes, but that's for special cases only and shouldn't be used for the
> > pca954x.
> > 
> > This being said, if your platform uses the device tree, you shouldn't (in
> > theory at least) need static I2C bus numbers. This is why there is no DT
> > property similar to the platform data adap_id field defined in the pca954x
> > DT bindings.

-- 
Regards,

Laurent Pinchart

--
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: platform_data in i2c device drivers

[Laurent Pinchart]

Hi Frank,

On Friday 21 March 2014 11:55:24 Frank Bormann wrote:
> Hi Laurent,
> 
> Thank you again for explaining. One last question if that's okay.
> 
> If I have i2c_board_info being passed in to the probe function through the
> platform_data pointer and I also have the same, similar or maybe even
> conflicting configuration information in the DTB, is there any of the two
> that is supposed to take precedence or should that be an error case?

That shouldn't happen (except in very special cases using OF_DEV_AUXDATA, but 
you really should think twice before going that way). If the I2C device is 
instantiated in board code with i2c_board_info then there will be no DT node 
corresponding to the device (client->dev.of_node will be NULL). If the device 
is instead instantiated from DT there will be no platform data (client-
>dev.platform_data will be NULL).

> On 20/03/14 01:51 PM, Laurent Pinchart wrote:
> > On Thursday 20 March 2014 13:16:35 Frank Bormann wrote:
> >> Hi Ben,
> >> 
> >>> I think client->dev should be avoided if at-all possible. Many
> >>> drivers keep their own local copy of platform data or the pointer
> >>> to it in their driver private information.
> >> 
> >> I was thinking about that. But then again, I'd either have to copy the
> >> client->dev.platform_data pointer over to the private data, if it is
> >> non-null, or I would have to use some variation of
> >> 
> >> pdata = client->dev.platform_data ? client->dev.platform_data : priv_pd;
> >> 
> >> every time I want to access the configuration. Guess, that's not so bad
> >> though.
> > 
> > Most drivers store a pointer to the platform data or a copy of the
> > platform data in a per-device driver private structure. They populate that
> > pointer or copy from the platform data pointer (in the legacy case) or
> > from the device tree content. Outside of the probe function the driver
> > thus only accesses its private pointer or copy.

-- 
Regards,

Laurent Pinchart

--
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: platform_data in i2c device drivers

[Laurent Pinchart]

Hi Frank,

On Thursday 20 March 2014 13:16:35 Frank Bormann wrote:
> Hi Ben,
> 
> > I think client->dev should be avoided if at-all possible. Many
> > drivers keep their own local copy of platform data or the pointer
> > to it in their driver private information.
> 
> I was thinking about that. But then again, I'd either have to copy the
> client->dev.platform_data pointer over to the private data, if it is
> non-null, or I would have to use some variation of
> 
> pdata = client->dev.platform_data ? client->dev.platform_data : priv_pd;
> 
> every time I want to access the configuration. Guess, that's not so bad
> though.

Most drivers store a pointer to the platform data or a copy of the platform 
data in a per-device driver private structure. They populate that pointer or 
copy from the platform data pointer (in the legacy case) or from the device 
tree content. Outside of the probe function the driver thus only accesses its 
private pointer or copy.

-- 
Regards,

Laurent Pinchart

--
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: platform_data in i2c device drivers

[Laurent Pinchart]

Hi Frank,

On Wednesday 19 March 2014 18:32:50 Frank Bormann wrote:
> Hi Everyone,
> 
> I am looking at the i2c_pca954x i2c bus mux driver in linux-stable. My goal
> is to have the slave buses show up in Linux with static bus numbers.
> Ideally, I want to define the first bus number to use in the dtb.
> 
> It seems, the driver has already some support for static bus numbers as its
> probe function checks for the existence of a struct pca954x_platform_data
> instance in client->dev.platform_data and pca954x_platform_mode struct it
> points to has a member adap_id that seems to be doing exactly that judging
> by its documentation. However, calls made to pca954x_probe always have to
> platform_data pointer being passed in through client set to null.
> 
> In addition to that, the recent addition to the driver of a reset gpio to be
> configured reads directly from the dtb in the probe function.
> 
> I am unsure about how to set this up properly. On one hand, platform_data is
> being passed in to the probe function, which seem to indicate, there may be
> some generic place in the i2c core code to set driver-specific
> configuration, on the other hand, the recent reset gpio addition to the
> driver seems to indicate that the driver's probe function is in fact the
> right place to read additional configuration from the dtb.
> 
> Any help is greatly appreciated.

You're looking at two different configuration mechanisms, which probably 
explains your confusion.

Platform data is the oldest mechanism used to pass configuration information 
to the driver. This is largely used through the Linux kernel on a wide range 
of architectures. The idea is to store device-specific configuration 
information in board files (as you mentioned DT I'll assume you're working on 
ARM, so that would be arch/arm/mach-*/board-*.c) using driver-specific 
structures and associate those structures with devices. Drivers can then 
retrieve the platform data at probe time and configure the device accordingly.

The way platform data is associated with devices depends on the bus type. For 
I2C the i2c_board_info structure, used to instantiate I2C devices in board 
code, has a void *platform_data field that can be set to point to a platform 
data structure. You can find an example of this in the i2c3_devices array in 
arch/arm/mach-shmobile/board-kzm9g.c.

Device tree (DT) is a newer mechanism to specify hardware configuration. 
Instead of relying on C board files that contain a mix a code and data, the 
platform is described in a tree-like structure of devices with properties 
associated to all those devices. That structure is called the device tree and 
is compiled as a binary that gets passed to the kernel at boot time. When 
using the device tree, drivers don't receive platform data anymore but are 
responsible for parsing the content of the device tree to read platform-
specific hardware configuration information. The content of device tree nodes 
is defined in documents called DT bindings that can be found in 
Documentation/devicetree/bindings/ (i2c/i2c-mux-pca954x.txt in this case).

A NULL platform_data pointer can then mean either that your platform boots 
using the device tree, or that the board file doesn't specify any platform 
data for the device in case of legacy (non-DT) boot. There's also a hybrid 
method that can be used to associated platform data declared in C files to DT 
nodes, but that's for special cases only and shouldn't be used for the 
pca954x.

This being said, if your platform uses the device tree, you shouldn't (in 
theory at least) need static I2C bus numbers. This is why there is no DT 
property similar to the platform data adap_id field defined in the pca954x DT 
bindings.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 01/15] i2c: shmobile/rcar: Restrict non-COMPILE_TEST compilation

[Laurent Pinchart]

Hi Wolfram,

On Wednesday 11 December 2013 13:47:34 Laurent Pinchart wrote:
> Hi Wolfram,
> 
> Could you please pick this patch up for v3.14 ?

Ping ?

> On Thursday 28 November 2013 16:36:21 Simon Horman wrote:
> > On Wed, Nov 27, 2013 at 02:18:23AM +0100, Laurent Pinchart wrote:
> > > Hardware supported by the i2c sh_mobile and rcar drivers is only found
> > > on SUPERH or ARCH_SHMOBILE platforms. Restrict non-COMPILE_TEST
> > > compilation to them.
> > > 
> > > Cc: Wolfram Sang 
> > > Cc: linux-i2c@vger.kernel.org
> > > Signed-off-by: Laurent Pinchart
> > > 
> > 
> > Acked-by: Simon Horman 
> > 
> > > ---
> > > 
> > >  drivers/i2c/busses/Kconfig | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > > index 3b26129..92bdcf9 100644
> > > --- a/drivers/i2c/busses/Kconfig
> > > +++ b/drivers/i2c/busses/Kconfig
> > > @@ -683,7 +683,7 @@ config I2C_SH7760
> > > 
> > >  config I2C_SH_MOBILE
> > >  
> > >   tristate "SuperH Mobile I2C Controller"
> > > 
> > > - depends on SUPERH || ARM || COMPILE_TEST
> > > + depends on SUPERH || ARCH_SHMOBILE || COMPILE_TEST
> > > 
> > >   help
> > >   
> > > If you say yes to this option, support will be included for the
> > > built-in I2C interface on the Renesas SH-Mobile processor.
> > > 
> > > @@ -796,7 +796,7 @@ config I2C_XLR
> > > 
> > >  config I2C_RCAR
> > >  
> > >   tristate "Renesas R-Car I2C Controller"
> > > 
> > > - depends on ARM || COMPILE_TEST
> > > + depends on ARCH_SHMOBILE || COMPILE_TEST
> > > 
> > >   help
> > >   
> > > If you say yes to this option, support will be included for the
> > > R-Car I2C controller.
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH V2 5/5] i2c: riic: add driver

[Laurent Pinchart]

Hi Wolfram,

On Thursday 19 December 2013 13:06:47 Wolfram Sang wrote:
> Hi Laurent,
> 
> thanks for the review!
> 
> > > +/*
> > > + * This i2c core has a lot of interrupts, namely 8. We use their
> > > chaining as some kind of state machine.
> > 
> > I have mixed feelings about this. Wouldn't it be more efficient to have an
> > internal state machine (which you partially have already, using
> > RIIC_INIT_MSG for instance) instead of relying on enabling/disabling
> > interrupts ? The latter has a larger overhead.
> 
> I am not sure I get you here. I need the interrupts anyhow. For example,
> after the last byte has been written to the 1-byte-FIFO in the
> transmission_irq, I need to wait for the transmission_end_irq to ensure the
> bits are already on the wire before I mark the message completed.
> 
> Polling for that condition is more overhead than just enabling the proper
> interrupt (one write to ICIER). I don't need to switch ISR since all the
> interrupts are seperate and have dedicated ISR.

I haven't expressed myself clearly. Polling is of course a bad option. My 
point was that I understood your comment as meaning that you enable and 
disable interrupts at runtime and use that as a state machine, while I was 
wondering whether it wouldn't be simpler to keep all interrupts enabled at all 
time and handle the synchronization explicitly.

Please scratch the comment about the larger overhead though, that was a 
mistake due to reading the code too fast.

> > > +static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> > > int num)
> > > +{
> > > + struct riic_dev *riic = i2c_get_adapdata(adap);
> > > + int i, ret;
> > 
> > One of my favorite bikeshedding comments is to ask for unsigned int when
> > the variable can't be negative :-)
> 
> OK.
> 
> > > + /*
> > > +  * TODO: Implement formula to calculate the timing values depending 
on
> > > +  * variable parent clock rate and arbitrary bus speed
> > > +  */
> > > + rate = clk_get_rate(riic->clk);
> > > + if (rate != 33325000) {
> > > + dev_err(&riic->adapter.dev,
> > > + "invalid parent clk (%lu). Must be 33325000Hz\n", rate);
> > 
> > What about a "goto done;" here and below to avoid repeating the
> > clk_disable_unprepare() call ?
> 
> Yeah, can be argued that way. I was fine with both.
> 
> > > + clk_disable_unprepare(riic->clk);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* Changing the order of accessing IICRST and ICE may break things! 
*/
> > > + writeb(ICCR1_IICRST | ICCR1_SOWP, riic->base + RIIC_ICCR1);
> > > + riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
> > > +
> > > + switch (spd) {
> > > + case 10:
> > > + writeb(ICMR1_CKS(3), riic->base + RIIC_ICMR1);
> > > + writeb(ICBRH_SP100K, riic->base + RIIC_ICBRH);
> > > + writeb(ICBRL_SP100K, riic->base + RIIC_ICBRL);
> > > + break;
> > > + case 40:
> > > + writeb(ICMR1_CKS(1), riic->base + RIIC_ICMR1);
> > > + writeb(ICBRH_SP400K, riic->base + RIIC_ICBRH);
> > > + writeb(ICBRL_SP400K, riic->base + RIIC_ICBRL);
> > 
> > Couldn't you compute the ICMR1, ICBRH and ICBRL values at runtime instead
> > ?
> 
> As mentioned in the TODO above, this is scheduled for an incremental
> update to this driver.

Nice :-)

> > > + of_property_read_u32(np, "clock-frequency", &bus_rate);
> > 
> > As the property is mandatory, shouldn't you check the return value of this
> > function ? Another option would be to make the clock-frequency property
> > optional and use a default value. What do the other I2C bus drivers
> > usually do ?
> 
> bus_rate is initialized to 0 and if read_u32 fails, it will stay this
> way. Then, the call to riic_init_hw() will fail and report the error.

That's the part I wasn't sure to like, but it will be reworked when making 
clock speed computation dynamic anyway, so we can keep it as-is for now.

> There is no standard behaviour (use sane default or fail) yet. It is
> somewhere on the I2C todo list :/

-- 
Regards,

Laurent Pinchart


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH V2 5/5] i2c: riic: add driver

[Laurent Pinchart]

k things! */
> + writeb(ICCR1_IICRST | ICCR1_SOWP, riic->base + RIIC_ICCR1);
> + riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
> +
> + switch (spd) {
> + case 10:
> + writeb(ICMR1_CKS(3), riic->base + RIIC_ICMR1);
> + writeb(ICBRH_SP100K, riic->base + RIIC_ICBRH);
> + writeb(ICBRL_SP100K, riic->base + RIIC_ICBRL);
> + break;
> + case 40:
> + writeb(ICMR1_CKS(1), riic->base + RIIC_ICMR1);
> + writeb(ICBRH_SP400K, riic->base + RIIC_ICBRH);
> + writeb(ICBRL_SP400K, riic->base + RIIC_ICBRL);

Couldn't you compute the ICMR1, ICBRH and ICBRL values at runtime instead ?

> + break;
> + default:
> + dev_err(&riic->adapter.dev,
> + "unsupported bus speed (%dHz). Use 10 or 40\n", 
> spd);
> + clk_disable_unprepare(riic->clk);
> + return -EINVAL;
> + }
> +
> + writeb(0, riic->base + RIIC_ICSER);
> + writeb(ICMR3_ACKWP | ICMR3_RDRFS, riic->base + RIIC_ICMR3);
> +
> + riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
> +

... with

done:

here, and a return ret.

> + clk_disable_unprepare(riic->clk);
> +
> + return 0;
> +}
> +
> +static struct riic_irq_desc riic_irqs[] = {
> + { .res_num = 0, .isr = riic_tend_isr, .name = "riic-tend" },
> + { .res_num = 1, .isr = riic_rdrf_isr, .name = "riic-rdrf" },
> + { .res_num = 2, .isr = riic_tdre_isr, .name = "riic-tdre" },
> + { .res_num = 5, .isr = riic_tend_isr, .name = "riic-nack" },
> +};
> +
> +static int riic_i2c_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct riic_dev *riic;
> + struct i2c_adapter *adap;
> + struct resource *res;
> + u32 bus_rate = 0;
> + int i, ret;
> +
> + riic = devm_kzalloc(&pdev->dev, sizeof(*riic), GFP_KERNEL);
> + if (!riic)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + riic->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(riic->base))
> + return PTR_ERR(riic->base);
> +
> + riic->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(riic->clk)) {
> + dev_err(&pdev->dev, "missing controller clock");
> + return PTR_ERR(riic->clk);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) {
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 
riic_irqs[i].res_num);
> + if (!res)
> + return -ENODEV;
> +
> + ret = devm_request_irq(&pdev->dev, res->start, riic_irqs[i].isr,
> + 0, riic_irqs[i].name, riic);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request irq %s\n", 
riic_irqs[i].name);
> + return ret;
> + }
> + }
> +
> + adap = &riic->adapter;
> + i2c_set_adapdata(adap, riic);
> + strlcpy(adap->name, "Renesas RIIC adapter", sizeof(adap->name));
> + adap->owner = THIS_MODULE;
> + adap->algo = &riic_algo;
> + adap->dev.parent = &pdev->dev;
> + adap->dev.of_node = pdev->dev.of_node;
> +
> + init_completion(&riic->msg_done);
> +
> + of_property_read_u32(np, "clock-frequency", &bus_rate);

As the property is mandatory, shouldn't you check the return value of this 
function ? Another option would be to make the clock-frequency property 
optional and use a default value. What do the other I2C bus drivers usually do 
?

> + ret = riic_init_hw(riic, bus_rate);
> + if (ret)
> + return ret;
> +
> +
> + ret = i2c_add_adapter(adap);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to add adapter\n");
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, riic);
> +
> + dev_info(&pdev->dev, "registered with %dHz bus speed\n", bus_rate);
> + return 0;
> +}
> +
> +static int riic_i2c_remove(struct platform_device *pdev)
> +{
> + struct riic_dev *riic = platform_get_drvdata(pdev);
> +
> + writeb(0, riic->base + RIIC_ICIER);
> + i2c_del_adapter(&riic->adapter);
> +
> + return 0;
> +}
> +
> +static struct of_device_id riic_i2c_dt_ids[] = {
> + { .compatible = "renesas,riic-rz" },
> + { /* Sentinel */ },
> +};
> +
> +static struct platform_driver riic_i2c_driver = {
> + .probe  = riic_i2c_probe,
> + .remove = riic_i2c_remove,
> + .driver = {
> + .name   = "i2c-riic",
> + .owner  = THIS_MODULE,
> + .of_match_table = riic_i2c_dt_ids,
> + },
> +};
> +
> +module_platform_driver(riic_i2c_driver);
> +
> +MODULE_DESCRIPTION("Renesas RIIC adapter");
> +MODULE_AUTHOR("Wolfram Sang ");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DEVICE_TABLE(of, riic_i2c_dt_ids);
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 3/5] arm: shmobile: r7s72100: add nodes for i2c controllers to dtsi

[Laurent Pinchart]

Hi Wolfram,

On Tuesday 17 December 2013 23:13:09 Wolfram Sang wrote:
> On Tue, Dec 17, 2013 at 10:55:31PM +0100, Laurent Pinchart wrote:
> > On Tuesday 17 December 2013 22:44:36 Wolfram Sang wrote:
> > > From: Wolfram Sang 
> > > 
> > > I decided to put the pinmuxing into the dtsi file since there is only
> > > one pinmux posiibility which one probably wants to have when using the
> > > bus.
> 
> I tried to explain here why the pin-groups are in the dtsi.
> 
> > > + riic0_pins: i2c0 {
> > > + renesas,groups = "riic0_scl_p1_0", "riic0_sda_p1_1";
> > > + renesas,function = "riic0";
> > > + };
> > > +
> > > + riic1_pins: i2c1 {
> > > + renesas,groups = "riic1_scl_p1_2", "riic1_sda_p1_3";
> > > + renesas,function = "riic1";
> > > + };
> > > +
> > > + riic2_pins: i2c2 {
> > > + renesas,groups = "riic2_scl_p1_4", "riic2_sda_p1_5";
> > > + renesas,function = "riic2";
> > > + };
> > > +
> > > + riic3_pins: i2c3 {
> > > + renesas,groups = "riic3_scl_p1_6", "riic3_sda_p1_7";
> > > + renesas,function = "riic3";
> > > + };
> > 
> > The SoC allows other options for the I2C pin groups. Instead of declaring
> > all possible groups here, I think we should thus only add the groups that
> > are really used to the board .dts files.
> 
> Really? Couldn't find any. This is why I included it here: since it is
> the only option, people probably want to use it when they activate the
> bus.

My bad, sorry, I've misread patch 1/5 and spoke too fast :-/ I thus makes 
sense to add the groups here.

> > > + clock-frequency = <10>;
> > 
> > Isn't that a board-specific property ?
> 
> It is meant as a sane default. Can be overridden.

OK.

-- 
Regards,

Laurent Pinchart


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 4/5] arm: shmobile: genmai: adapt dts to use native i2c driver

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Tuesday 17 December 2013 22:44:37 Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> Switch from the gpio-driver to the shiny new native driver. Tested by
> accessing the eeprom on the genmai board.
> 
> Signed-off-by: Wolfram Sang 
> Acked-by: Magnus Damm 
> ---
>  arch/arm/boot/dts/r7s72100-genmai-reference.dts | 29 +-
>  1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> b/arch/arm/boot/dts/r7s72100-genmai-reference.dts index ce5da0b..739448a
> 100644
> --- a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> +++ b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> @@ -39,24 +39,6 @@
>   gpios = <&port4 11 GPIO_ACTIVE_LOW>;
>   };
>   };
> -
> - i2c@0 {
> - compatible = "i2c-gpio";
> - gpios = <&port1 5 GPIO_ACTIVE_HIGH /* sda */
> -  &port1 4 GPIO_ACTIVE_HIGH /* scl */
> - >;
> - i2c-gpio,sda-open-drain;
> - i2c-gpio,scl-open-drain;
> - i2c-gpio,delay-us = <5>;/* ~100 kHz */
> - #address-cells = <1>;
> - #size-cells = <0>;
> -
> - flash@50 {
> - compatible = "renesas,24c128";
> - reg = <0x50>;
> - pagesize = <64>;
> - };
> - };
>  };
> 
>  &pfc {
> @@ -68,3 +50,14 @@
>   renesas,function = "scif2";
>   };
>  };
> +
> +&i2c2 {
> + status = "okay";
> + clock-frequency = <40>;
> +
> + eeprom: 24c128@50 {

That should be "24c128: eeprom@50". You can actually omit the "24c128:" alias 
until it gets needed.

> + compatible = "renesas,24c128";
> + reg = <0x50>;
> + pagesize = <64>;
> + };
> +};

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 3/5] arm: shmobile: r7s72100: add nodes for i2c controllers to dtsi

[Laurent Pinchart]

 };
> +
> + i2c2: i2c@fcfee800 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "renesas,riic-r7s72100", "renesas,riic-rz";
> + reg = <0xfcfee800 0x44>;
> + interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>,
> +  <0 174 IRQ_TYPE_EDGE_RISING>,
> +  <0 175 IRQ_TYPE_EDGE_RISING>,
> +  <0 176 IRQ_TYPE_LEVEL_HIGH>,
> +  <0 177 IRQ_TYPE_LEVEL_HIGH>,
> +  <0 178 IRQ_TYPE_LEVEL_HIGH>,
> +  <0 179 IRQ_TYPE_LEVEL_HIGH>,
> +  <0 180 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-0 = <&riic2_pins>;
> + pinctrl-names = "default";
> + clock-frequency = <10>;
> + status = "disabled";
> + };
> +
> + i2c3: i2c@fcfeec00 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "renesas,riic-r7s72100", "renesas,riic-rz";
> + reg = <0xfcfeec00 0x44>;
> + interrupts = <0 181 IRQ_TYPE_LEVEL_HIGH>,
> +  <0 182 IRQ_TYPE_EDGE_RISING>,
> +  <0 183 IRQ_TYPE_EDGE_RISING>,
> +  <0 184 IRQ_TYPE_LEVEL_HIGH>,
> +  <0 185 IRQ_TYPE_LEVEL_HIGH>,
> +  <0 186 IRQ_TYPE_LEVEL_HIGH>,
> +  <0 187 IRQ_TYPE_LEVEL_HIGH>,
> +  <0 188 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-0 = <&riic3_pins>;
> + pinctrl-names = "default";
> + clock-frequency = <10>;
> + status = "disabled";
> + };
>  };
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/5] pinctrl: r7s72100: add riic groups

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Tuesday 17 December 2013 22:44:34 Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> Tested RIIC2 on a genmai board. Other riic groups are untested but seem
> trivial enough to be added.
> 
> Signed-off-by: Wolfram Sang 
> Acked-by: Magnus Damm 
> ---
> 
> Note: With the current PFC driver as posted by Magnus, it needs another
> patch to work. Yet, I think this is a seperate PFC issue which needs to be
> sorted out seperately and shouldn't affect these declarations. I'll add the
> needed patch as a response to this mail.
> 
>  drivers/pinctrl/sh-pfc/pfc-r7s72100.c | 45 
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/pinctrl/sh-pfc/pfc-r7s72100.c
> b/drivers/pinctrl/sh-pfc/pfc-r7s72100.c index a662876..2b716d1 100644
> --- a/drivers/pinctrl/sh-pfc/pfc-r7s72100.c
> +++ b/drivers/pinctrl/sh-pfc/pfc-r7s72100.c
> @@ -229,6 +229,27 @@ SCIF5(RZ_PIN_AND_MUX)
>  SCIF6(RZ_PIN_AND_MUX)
>  SCIF7(RZ_PIN_AND_MUX)
> 
> +#define RIIC0(fn)\
> + fn(riic0, scl, 1, 0, 1) \
> + fn(riic0, sda, 1, 1, 1)
> +
> +#define RIIC1(fn)\
> + fn(riic1, scl, 1, 2, 1) \
> + fn(riic1, sda, 1, 3, 1)
> +
> +#define RIIC2(fn)\
> + fn(riic2, scl, 1, 4, 1) \
> + fn(riic2, sda, 1, 5, 1)
> +
> +#define RIIC3(fn)\
> + fn(riic3, scl, 1, 6, 1) \
> + fn(riic3, sda, 1, 7, 1)
> +
> +RIIC0(RZ_PIN_AND_MUX)
> +RIIC1(RZ_PIN_AND_MUX)
> +RIIC2(RZ_PIN_AND_MUX)
> +RIIC3(RZ_PIN_AND_MUX)

Could you please move this above the SCIF declarations (same comment below) ? 
The pfc-*.c files tend to grow pretty big, and keeping entries sorted 
alphabetically helps.

> +
>  static const struct sh_pfc_pin_group pinmux_groups[] = {
>   SCIF0(RZ_PMX_GROUP)
>   SCIF1(RZ_PMX_GROUP)
> @@ -238,6 +259,10 @@ static const struct sh_pfc_pin_group pinmux_groups[] =
> { SCIF5(RZ_PMX_GROUP)
>   SCIF6(RZ_PMX_GROUP)
>   SCIF7(RZ_PMX_GROUP)
> + RIIC0(RZ_PMX_GROUP)
> + RIIC1(RZ_PMX_GROUP)
> + RIIC2(RZ_PMX_GROUP)
> + RIIC3(RZ_PMX_GROUP)
>  };
> 
>  static const char * const scif0_groups[] = {
> @@ -272,6 +297,22 @@ static const char * const scif7_groups[] = {
>   SCIF7(RZ_GROUPS)
>  };
> 
> +static const char * const riic0_groups[] = {
> + RIIC0(RZ_GROUPS)
> +};
> +
> +static const char * const riic1_groups[] = {
> + RIIC1(RZ_GROUPS)
> +};
> +
> +static const char * const riic2_groups[] = {
> + RIIC2(RZ_GROUPS)
> +};
> +
> +static const char * const riic3_groups[] = {
> + RIIC3(RZ_GROUPS)
> +};
> +
>  static const struct sh_pfc_function pinmux_functions[] = {
>   SH_PFC_FUNCTION(scif0),
>   SH_PFC_FUNCTION(scif1),
> @@ -281,6 +322,10 @@ static const struct sh_pfc_function pinmux_functions[]
> = { SH_PFC_FUNCTION(scif5),
>   SH_PFC_FUNCTION(scif6),
>       SH_PFC_FUNCTION(scif7),
> + SH_PFC_FUNCTION(riic0),
> + SH_PFC_FUNCTION(riic1),
> + SH_PFC_FUNCTION(riic2),
> + SH_PFC_FUNCTION(riic3),
>  };
> 
>  #define PFC_REG(idx, name, reg)  
> \
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 01/15] i2c: shmobile/rcar: Restrict non-COMPILE_TEST compilation

[Laurent Pinchart]

Hi Wolfram,

Could you please pick this patch up for v3.14 ?

On Thursday 28 November 2013 16:36:21 Simon Horman wrote:
> On Wed, Nov 27, 2013 at 02:18:23AM +0100, Laurent Pinchart wrote:
> > Hardware supported by the i2c sh_mobile and rcar drivers is only found
> > on SUPERH or ARCH_SHMOBILE platforms. Restrict non-COMPILE_TEST
> > compilation to them.
> > 
> > Cc: Wolfram Sang 
> > Cc: linux-i2c@vger.kernel.org
> > Signed-off-by: Laurent Pinchart
> > 
> 
> Acked-by: Simon Horman 
> 
> > ---
> > 
> >  drivers/i2c/busses/Kconfig | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 3b26129..92bdcf9 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -683,7 +683,7 @@ config I2C_SH7760
> > 
> >  config I2C_SH_MOBILE
> > tristate "SuperH Mobile I2C Controller"
> > -   depends on SUPERH || ARM || COMPILE_TEST
> > +   depends on SUPERH || ARCH_SHMOBILE || COMPILE_TEST
> > help
> >   If you say yes to this option, support will be included for the
> >   built-in I2C interface on the Renesas SH-Mobile processor.
> > @@ -796,7 +796,7 @@ config I2C_XLR
> > 
> >  config I2C_RCAR
> > tristate "Renesas R-Car I2C Controller"
> > -   depends on ARM || COMPILE_TEST
> > +   depends on ARCH_SHMOBILE || COMPILE_TEST
> > help
> >   If you say yes to this option, support will be included for the
> >   R-Car I2C controller.

-- 
Regards,

Laurent Pinchart

--
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 3/4] i2c: pca954x: Add device tree bindings documentation

[Laurent Pinchart]

Cc: devicet...@vger.kernel.org
Signed-off-by: Laurent Pinchart 
---
 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt| 46 ++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt 
b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
new file mode 100644
index 000..cc7e7fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -0,0 +1,46 @@
+* NXP PCA954x I2C bus switch
+
+Required Properties:
+
+  - compatible: Must contain one of the following.
+"nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
+"nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
+
+  - reg: The I2C address of the device.
+
+  The following required properties are defined externally:
+
+  - Standard I2C mux properties. See i2c-mux.txt in this directory.
+  - I2C child bus nodes. See i2c-mux.txt in this directory.
+
+
+Example:
+
+   i2c-switch@74 {
+   compatible = "nxp,pca9548";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0x74>;
+
+   i2c@2 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <2>;
+
+   eeprom@54 {
+   compatible = "at,24c08";
+   reg = <0x54>;
+   };
+   };
+
+   i2c@4 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <4>;
+
+   rtc@51 {
+   compatible = "nxp,pcf8563";
+   reg = <0x51>;
+   };
+   };
+   };
-- 
1.8.3.2

--
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/4] i2c: pca954x: Use devm_kzalloc managed allocator

[Laurent Pinchart]

This simplifies error and removal paths.

Signed-off-by: Laurent Pinchart 
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c 
b/drivers/i2c/muxes/i2c-mux-pca954x.c
index ce740f1..2880c38 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -187,16 +187,14 @@ static int pca954x_probe(struct i2c_client *client,
struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
int num, force, class;
struct pca954x *data;
-   int ret = -ENODEV;
+   int ret;
 
if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE))
-   goto err;
+   return -ENODEV;
 
-   data = kzalloc(sizeof(struct pca954x), GFP_KERNEL);
-   if (!data) {
-   ret = -ENOMEM;
-   goto err;
-   }
+   data = devm_kzalloc(&client->dev, sizeof(struct pca954x), GFP_KERNEL);
+   if (!data)
+   return -ENOMEM;
 
i2c_set_clientdata(client, data);
 
@@ -206,7 +204,7 @@ static int pca954x_probe(struct i2c_client *client,
 */
if (i2c_smbus_write_byte(client, 0) < 0) {
dev_warn(&client->dev, "probe failed\n");
-   goto exit_free;
+   return -ENODEV;
}
 
data->type = id->driver_data;
@@ -251,9 +249,6 @@ static int pca954x_probe(struct i2c_client *client,
 virt_reg_failed:
for (num--; num >= 0; num--)
i2c_del_mux_adapter(data->virt_adaps[num]);
-exit_free:
-   kfree(data);
-err:
return ret;
 }
 
@@ -269,7 +264,6 @@ static int pca954x_remove(struct i2c_client *client)
data->virt_adaps[i] = NULL;
}
 
-   kfree(data);
return 0;
 }
 
-- 
1.8.3.2

--
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