Linux I2C development
 help / color / mirror / Atom feed
* Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported
From: Mark Rutland @ 2016-10-14 17:30 UTC (permalink / raw)
  To: Luis.Oliveira
  Cc: jarkko.nikula, andriy.shevchenko, mika.westerberg, wsa, linux-i2c,
	linux-kernel, robh+dt, devicetree, CARLOS.PALMINHA,
	Ramiro.Oliveira
In-Reply-To: <36f4b1736e36b039af15b13645df0de492f1b6e1.1476462204.git.lolivei@synopsys.com>

On Fri, Oct 14, 2016 at 05:52:50PM +0100, Luis.Oliveira@synopsys.com wrote:
> -		is_slave = device_property_read_bool(&pdev->dev, "isslave");

Which tree is this based on? I cant see the existing isslave property in
mainline HEAD (commit 29fbff8698fc0ac1).

> +#ifndef CONFIG_ACPI
> +		is_slave = device_property_read_bool(&pdev->dev, "is-slave");
> +#endif

This ifdef is broken. At least for arm64, a single kernel image can be
booted with either ACPI or DT. We need separate accessors for DT and
ACPI to handle these differently, or you need to explicitly check
whether or not you have ACPI or DT at runtime.

Thanks,
Mark.

^ permalink raw reply

* Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported
From: Wolfram Sang @ 2016-10-14 18:20 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Luis.Oliveira-HKixBCOQz3hWk0Htik3J/w,
	jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
	andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA,
	mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w,
	Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <20161014173014.GA26269@leverpostej>

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

On Fri, Oct 14, 2016 at 06:30:15PM +0100, Mark Rutland wrote:
> On Fri, Oct 14, 2016 at 05:52:50PM +0100, Luis.Oliveira-HKixBCOQz3hWk0Htik3J/w@public.gmane.org wrote:
> > -		is_slave = device_property_read_bool(&pdev->dev, "isslave");
> 
> Which tree is this based on? I cant see the existing isslave property in
> mainline HEAD (commit 29fbff8698fc0ac1).

Same surprise here. Because I likely would have NAKed the binding.

Why is it needed? Doesn't the driver allow to be master/slave on the
same bus?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply

* [PATCH 0/2] constify i2c_adapter_quirks structures
From: Julia Lawall @ 2016-10-15 17:32 UTC (permalink / raw)
  To: linux-arm-kernel; +Cc: kernel-janitors, Wolfram Sang, linux-i2c, linux-kernel

Constify i2c_adapter_quirks structures.

---

 drivers/i2c/busses/i2c-axxia.c      |    2 +-
 drivers/i2c/busses/i2c-bcm-iproc.c  |    2 +-
 drivers/i2c/busses/i2c-dln2.c       |    2 +-
 drivers/i2c/busses/i2c-viperboard.c |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

^ permalink raw reply

* [PATCH 1/2] i2c: bcm-iproc: constify i2c_adapter_quirks structures
From: Julia Lawall @ 2016-10-15 17:32 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Scott Branden, Jon Mason, Ray Jui, kernel-janitors, linux-kernel,
	bcm-kernel-feedback-list, linux-i2c, linux-arm-kernel
In-Reply-To: <1476552722-12352-1-git-send-email-Julia.Lawall@lip6.fr>

Check for i2c_adapter_quirks structures that are only stored in the
quirks field of an i2c_adapter structure.  This field is declared
const, so i2c_adapter_quirks structures that have this property can be
declared as const also.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r disable optional_qualifier@
identifier i;
position p;
@@
static struct i2c_adapter_quirks i@p = { ... };

@ok@
identifier r.i;
struct i2c_adapter e;
position p;
@@
e.quirks = &i@p;

@bad@
position p != {r.p,ok.p};
identifier r.i;
struct i2c_adapter_quirks e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
 struct i2c_adapter_quirks i = { ... };
// </smpl>

The effect on the layout of the .o file is shown by the following
output of the size command, first before then after the
transformation:

   text    data     bss     dec     hex filename
   3458     744       8    4210    1072 drivers/i2c/busses/i2c-bcm-iproc.o
   3490     720       8    4218    107a drivers/i2c/busses/i2c-bcm-iproc.o

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/i2c/busses/i2c-bcm-iproc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 326b3db..318df55 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -395,7 +395,7 @@ static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
 	.functionality = bcm_iproc_i2c_functionality,
 };
 
-static struct i2c_adapter_quirks bcm_iproc_i2c_quirks = {
+static const struct i2c_adapter_quirks bcm_iproc_i2c_quirks = {
 	/* need to reserve one byte in the FIFO for the slave address */
 	.max_read_len = M_TX_RX_FIFO_SIZE - 1,
 };

^ permalink raw reply related

* [PATCH 2/2] i2c: constify i2c_adapter_quirks structures
From: Julia Lawall @ 2016-10-15 17:32 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: kernel-janitors, linux-i2c, linux-kernel
In-Reply-To: <1476552722-12352-1-git-send-email-Julia.Lawall@lip6.fr>

Check for i2c_adapter_quirks structures that are only stored in the
quirks field of an i2c_adapter structure.  This field is declared
const, so i2c_adapter_quirks structures that have this property can be
declared as const also.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r disable optional_qualifier@
identifier i;
position p;
@@
static struct i2c_adapter_quirks i@p = { ... };

@ok@
identifier r.i;
struct i2c_adapter e;
position p;
@@
e.quirks = &i@p;

@bad@
position p != {r.p,ok.p};
identifier r.i;
struct i2c_adapter_quirks e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
 struct i2c_adapter_quirks i = { ... };
// </smpl>

The effect on the layout of the .o files is shown by the following
output of the size command, first before then after the
transformation:

   text    data     bss     dec     hex filename
   3651     472       8    4131    1023 drivers/i2c/busses/i2c-axxia.o
   3683     440       8    4131    1023 drivers/i2c/busses/i2c-axxia.o

   text    data     bss     dec     hex filename
   1069     216       0    1285     505 drivers/i2c/busses/i2c-dln2.o
   1101     192       0    1293     50d drivers/i2c/busses/i2c-dln2.o

   text    data     bss     dec     hex filename
   3211     484       1    3696     e70 drivers/i2c/busses/i2c-viperboard.o
   3243     460       1    3704     e78 drivers/i2c/busses/i2c-viperboard.o

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/i2c/busses/i2c-axxia.c      |    2 +-
 drivers/i2c/busses/i2c-dln2.c       |    2 +-
 drivers/i2c/busses/i2c-viperboard.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-axxia.c b/drivers/i2c/busses/i2c-axxia.c
index 4351a93..13f0748 100644
--- a/drivers/i2c/busses/i2c-axxia.c
+++ b/drivers/i2c/busses/i2c-axxia.c
@@ -489,7 +489,7 @@ static u32 axxia_i2c_func(struct i2c_adapter *adap)
 	.functionality = axxia_i2c_func,
 };
 
-static struct i2c_adapter_quirks axxia_i2c_quirks = {
+static const struct i2c_adapter_quirks axxia_i2c_quirks = {
 	.max_read_len = 255,
 	.max_write_len = 255,
 };
diff --git a/drivers/i2c/busses/i2c-dln2.c b/drivers/i2c/busses/i2c-dln2.c
index 8acda2a..69075a3 100644
--- a/drivers/i2c/busses/i2c-dln2.c
+++ b/drivers/i2c/busses/i2c-dln2.c
@@ -182,7 +182,7 @@ static u32 dln2_i2c_func(struct i2c_adapter *a)
 	.functionality = dln2_i2c_func,
 };
 
-static struct i2c_adapter_quirks dln2_i2c_quirks = {
+static const struct i2c_adapter_quirks dln2_i2c_quirks = {
 	.max_read_len = DLN2_I2C_MAX_XFER_SIZE,
 	.max_write_len = DLN2_I2C_MAX_XFER_SIZE,
 };
diff --git a/drivers/i2c/busses/i2c-viperboard.c b/drivers/i2c/busses/i2c-viperboard.c
index 543456a..e4be86b 100644
--- a/drivers/i2c/busses/i2c-viperboard.c
+++ b/drivers/i2c/busses/i2c-viperboard.c
@@ -354,7 +354,7 @@ static u32 vprbrd_i2c_func(struct i2c_adapter *i2c)
 	.functionality	= vprbrd_i2c_func,
 };
 
-static struct i2c_adapter_quirks vprbrd_quirks = {
+static const struct i2c_adapter_quirks vprbrd_quirks = {
 	.max_read_len = 2048,
 	.max_write_len = 2048,
 };

^ permalink raw reply related

* Re: [PATCH] spi: mark device nodes only in case of successful instantiation
From: Geert Uytterhoeven @ 2016-10-16  8:49 UTC (permalink / raw)
  To: Ralf Ramsauer
  Cc: Mark Brown, linux-spi, linux-kernel@vger.kernel.org,
	Geert Uytterhoeven, Wolfram Sang, Linux I2C
In-Reply-To: <20161014193113.29275-1-ralf@ramses-pyramidenbau.de>

Hi Ralf,

(Cc i2c)

On Fri, Oct 14, 2016 at 9:31 PM, Ralf Ramsauer
<ralf@ramses-pyramidenbau.de> wrote:
> Instantiated SPI device nodes are marked with OF_POPULATE. This was
> introduced in bd6c164. On unloading, loaded device nodes will of course
> be unmarked. The problem are nodes the fail during initialisation: If a
> node failed during registration, it won't be unloaded and hence never be
> unmarked again.
>
> So if a SPI driver module is unloaded and reloaded, it will skip nodes
> that failed before.
>
> Skip device nodes that are already populated and mark them only in case
> of success.
>
> Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Hi,
>
> imagine the following situation: you loaded a spi driver as module, but
> it fails to instantiate, because of some reasons (e.g. some resources,
> like gpios, might be in use in userspace).
>
> When reloading the driver, _all_ nodes, including previously failed
> ones, should be probed again. This is not the case at the moment.
> Current behaviour only re-registers nodes that were previously
> successfully loaded.
>
> This small patches fixes this behaviour. I stumbled over this while
> working on a spi driver.

Thanks for your patch!

>  drivers/spi/spi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 200ca22..f96a04e 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1604,12 +1604,15 @@ static void of_register_spi_devices(struct spi_master *master)
>                 return;
>
>         for_each_available_child_of_node(master->dev.of_node, nc) {
> -               if (of_node_test_and_set_flag(nc, OF_POPULATED))
> +               if (of_node_check_flag(nc, OF_POPULATED))
>                         continue;
>                 spi = of_register_spi_device(master, nc);
> -               if (IS_ERR(spi))
> +               if (IS_ERR(spi)) {
>                         dev_warn(&master->dev, "Failed to create SPI device for %s\n",
>                                 nc->full_name);
> +                       continue;
> +               }
> +               of_node_set_flag(nc, OF_POPULATED);

I think it's safer to keep the atomic test-and-set, but clear the flag on
failure, cfr. of_platform_device_create_pdata() and of_amba_device_create().

Shouldn't of_spi_notify() be fixed, too?

The same issue exists for i2c in of_i2c_register_devices() and of_i2c_notify(),
which is what I had used as an example.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* Re: [PATCH] spi: mark device nodes only in case of successful instantiation
From: Wolfram Sang @ 2016-10-16  9:32 UTC (permalink / raw)
  To: Geert Uytterhoeven, Pantelis Antoniou
  Cc: Ralf Ramsauer, Mark Brown, linux-spi,
	linux-kernel@vger.kernel.org, Geert Uytterhoeven, Linux I2C
In-Reply-To: <CAMuHMdUMvjZvwBMUrWTk+obbUYsfRJxPTmdnv7Fz6uzkQ8nbbQ@mail.gmail.com>

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

On Sun, Oct 16, 2016 at 10:49:11AM +0200, Geert Uytterhoeven wrote:
> Hi Ralf,
> 
> (Cc i2c)

Thanks for letting me know! Adding Pantelis to CC, as he is the original
author of OF_DYNAMIC. Please keep me in the loop.

> 
> On Fri, Oct 14, 2016 at 9:31 PM, Ralf Ramsauer
> <ralf@ramses-pyramidenbau.de> wrote:
> > Instantiated SPI device nodes are marked with OF_POPULATE. This was
> > introduced in bd6c164. On unloading, loaded device nodes will of course
> > be unmarked. The problem are nodes the fail during initialisation: If a
> > node failed during registration, it won't be unloaded and hence never be
> > unmarked again.
> >
> > So if a SPI driver module is unloaded and reloaded, it will skip nodes
> > that failed before.
> >
> > Skip device nodes that are already populated and mark them only in case
> > of success.
> >
> > Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
> > Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Hi,
> >
> > imagine the following situation: you loaded a spi driver as module, but
> > it fails to instantiate, because of some reasons (e.g. some resources,
> > like gpios, might be in use in userspace).
> >
> > When reloading the driver, _all_ nodes, including previously failed
> > ones, should be probed again. This is not the case at the moment.
> > Current behaviour only re-registers nodes that were previously
> > successfully loaded.
> >
> > This small patches fixes this behaviour. I stumbled over this while
> > working on a spi driver.
> 
> Thanks for your patch!
> 
> >  drivers/spi/spi.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 200ca22..f96a04e 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1604,12 +1604,15 @@ static void of_register_spi_devices(struct spi_master *master)
> >                 return;
> >
> >         for_each_available_child_of_node(master->dev.of_node, nc) {
> > -               if (of_node_test_and_set_flag(nc, OF_POPULATED))
> > +               if (of_node_check_flag(nc, OF_POPULATED))
> >                         continue;
> >                 spi = of_register_spi_device(master, nc);
> > -               if (IS_ERR(spi))
> > +               if (IS_ERR(spi)) {
> >                         dev_warn(&master->dev, "Failed to create SPI device for %s\n",
> >                                 nc->full_name);
> > +                       continue;
> > +               }
> > +               of_node_set_flag(nc, OF_POPULATED);
> 
> I think it's safer to keep the atomic test-and-set, but clear the flag on
> failure, cfr. of_platform_device_create_pdata() and of_amba_device_create().
> 
> Shouldn't of_spi_notify() be fixed, too?
> 
> The same issue exists for i2c in of_i2c_register_devices() and of_i2c_notify(),
> which is what I had used as an example.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply

* Re: [PATCH] spi: mark device nodes only in case of successful instantiation
From: Ralf Ramsauer @ 2016-10-16  9:55 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, linux-spi,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Geert Uytterhoeven, Wolfram Sang, Linux I2C
In-Reply-To: <CAMuHMdUMvjZvwBMUrWTk+obbUYsfRJxPTmdnv7Fz6uzkQ8nbbQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

Hi Geert,

On 10/16/2016 10:49 AM, Geert Uytterhoeven wrote:
> Hi Ralf,
> 
> (Cc i2c)
> 
> On Fri, Oct 14, 2016 at 9:31 PM, Ralf Ramsauer
> <ralf-ex4EvEgDZR1o7HI0AxOeTKwczVv4G7md@public.gmane.org> wrote:
>> Instantiated SPI device nodes are marked with OF_POPULATE. This was
>> introduced in bd6c164. On unloading, loaded device nodes will of course
>> be unmarked. The problem are nodes the fail during initialisation: If a
>> node failed during registration, it won't be unloaded and hence never be
>> unmarked again.
>>
>> So if a SPI driver module is unloaded and reloaded, it will skip nodes
>> that failed before.
>>
>> Skip device nodes that are already populated and mark them only in case
>> of success.
>>
>> Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
>> Signed-off-by: Ralf Ramsauer <ralf-ex4EvEgDZR1o7HI0AxOeTKwczVv4G7md@public.gmane.org>
>> Cc: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
>> ---
>> Hi,
>>
>> imagine the following situation: you loaded a spi driver as module, but
>> it fails to instantiate, because of some reasons (e.g. some resources,
>> like gpios, might be in use in userspace).
>>
>> When reloading the driver, _all_ nodes, including previously failed
>> ones, should be probed again. This is not the case at the moment.
>> Current behaviour only re-registers nodes that were previously
>> successfully loaded.
>>
>> This small patches fixes this behaviour. I stumbled over this while
>> working on a spi driver.
> 
> Thanks for your patch!
> 
>>  drivers/spi/spi.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 200ca22..f96a04e 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -1604,12 +1604,15 @@ static void of_register_spi_devices(struct spi_master *master)
>>                 return;
>>
>>         for_each_available_child_of_node(master->dev.of_node, nc) {
>> -               if (of_node_test_and_set_flag(nc, OF_POPULATED))
>> +               if (of_node_check_flag(nc, OF_POPULATED))
>>                         continue;
>>                 spi = of_register_spi_device(master, nc);
>> -               if (IS_ERR(spi))
>> +               if (IS_ERR(spi)) {
>>                         dev_warn(&master->dev, "Failed to create SPI device for %s\n",
>>                                 nc->full_name);
>> +                       continue;
>> +               }
>> +               of_node_set_flag(nc, OF_POPULATED);
> 
> I think it's safer to keep the atomic test-and-set, but clear the flag on
> failure, cfr. of_platform_device_create_pdata() and of_amba_device_create().
Ack, no prob. Let me change this in the next version.
> 
> Shouldn't of_spi_notify() be fixed, too?
Right, that's almost the same path.
> 
> The same issue exists for i2c in of_i2c_register_devices() and of_i2c_notify(),
> which is what I had used as an example.
Good old c&p ;-)
I'll fix and test that tomorrow and come back with two patches, as it
touches different subsystems.

Best
  Ralf
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 


-- 
Ralf Ramsauer
GPG: 0x8F10049B
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH] i2c: allow configuration of imx driver for ColdFire architecture
From: Greg Ungerer @ 2016-10-17  1:54 UTC (permalink / raw)
  To: linux-i2c, angelo; +Cc: Greg Ungerer

The i2c controller used by Freescales iMX processors is the same
hardware module used on Freescales ColdFire family of processors.

We can use the existing i2c-imx driver on ColdFire family members.
Modify the configuration to allow it to be selected when compiling
for ColdFire targets.

Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
---
 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 6d94e2e..cc9adf6 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -589,10 +589,10 @@ config I2C_IMG
 
 config I2C_IMX
 	tristate "IMX I2C interface"
-	depends on ARCH_MXC || ARCH_LAYERSCAPE
+	depends on ARCH_MXC || ARCH_LAYERSCAPE || COLDFIRE
 	help
 	  Say Y here if you want to use the IIC bus controller on
-	  the Freescale i.MX/MXC or Layerscape processors.
+	  the Freescale i.MX/MXC, Layerscape or ColdFire processors.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-imx.
-- 
1.9.1

^ permalink raw reply related

* Re: [PATCH v2 1/2] Documentation: tpm: add the IBM Virtual TPM device tree binding documentation
From: Nayna @ 2016-10-17  2:08 UTC (permalink / raw)
  To: Rob Herring
  Cc: mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA,
	pawel.moll-5wv7dgnIgG8, wsa-z923LK4zBo2bacvFa/9K2g,
	ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
	honclo-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, galak-sgV2jX0FEOL9JmXXK+q4OQ,
	cclaudio-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8
In-Reply-To: <20161008210112.GA22112@rob-hp-laptop>



On 10/09/2016 02:31 AM, Rob Herring wrote:
> On Thu, Oct 06, 2016 at 01:44:32AM +0530, Nayna wrote:
>>
>>
>> On 09/29/2016 04:34 PM, Jarkko Sakkinen wrote:
>>> On Wed, Sep 28, 2016 at 04:30:40AM -0400, Nayna Jain wrote:
>>>> Virtual TPM, which is being used on IBM POWER7+ and POWER8 systems running
>>>> POWERVM, is currently supported by tpm device driver but lacks the
>>>> documentation. This patch adds the missing documentation for the existing
>>>> support.
>>>>
>>>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>>>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>>>> ---
>>>> Changelog v2:
>>>>
>>>> - New Patch
>>>>
>>>>   .../devicetree/bindings/security/tpm/ibmvtpm.txt   | 41 ++++++++++++++++++++++
>>>>   1 file changed, 41 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt b/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
>>>> new file mode 100644
>>>> index 0000000..d89f999
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
>>>> @@ -0,0 +1,41 @@
>>>> +* Device Tree Bindings for IBM Virtual Trusted Platform Module(vtpm)
>>>> +
>>>> +Required properties:
>>>> +
>>>> +- compatible            : property name that conveys the platform architecture
>>>> +                          identifiers, as 'IBM,vtpm'
>>>> +- device_type           : specifies type of virtual device
>>>
>>> A generic device tree question. What is the difference between
>>> these fields? Why the I2C one does have 'device_type'?
>>
>> Please find the details as below:
>>
>> compatible - Standard property name as per IEEE 1275, specifying the
>> interface compatible with this device. This property is consumed by linux
>> kernel for selection of device driver.
>>
>> device_type - Standard property name as per IEEE 1275, specifying the device
>> type. This property MAY be used by linux kernel for device driver selection.
>> It is used in the case of IBM virtual TPM driver.
>
> AIUI, this should be a standard value such as serial, pci, etc. I don't
> think your use here is correct, but I could be wrong. I'm not certain
> what you do with devices that don't have a standard type.

Actually, the vtpm node is a unique device and did not have a standard 
type that could be used.  That is why the definition in the PAPR gave it 
the a new value.  The intention was to provide a reasonable type value. 
  The "IBM," portion of the property value is intended to identify it as 
a specific IBM implementation.

Thanks & Regards,
    - Nayna

>
>>
>> /**
>>   * vio_register_device_node: - Register a new vio device.
>>   * @of_node:    The OF node for this device.
>>   *
>>   * Creates and initializes a vio_dev structure from the data in
>>   * of_node and adds it to the list of virtual devices.
>>   * Returns a pointer to the created vio_dev or NULL if node has
>>   * NULL device_type or compatible fields.
>>   */
>> struct vio_dev *vio_register_device_node(struct device_node *of_node)
>>
>> and vtpm device table being defined as below:
>>
>> static struct vio_device_id tpm_ibmvtpm_device_table[] = {
>>          { "IBM,vtpm", "IBM,vtpm"}, ----------------------------------->
>> type,compat
>>          { "", "" }
>> };
>>
>> So, vio (virtual) devices uses both device_type and compatible property for
>> device registration and driver selection.
>> In case of physical TPM, it is only compatible property being used for
>> device driver selection
>>
>> Also, please note that device_type property is now deprecated in latest
>> Device Tree specs.
>
> Deprecated for Flattened DT only. OpenFirmware implementations
> can/should still use this.
>
> Rob
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

^ permalink raw reply

* Re: [PATCH v2 1/2] Documentation: tpm: add the IBM Virtual TPM device tree binding documentation
From: Nayna @ 2016-10-17  2:11 UTC (permalink / raw)
  To: Rob Herring
  Cc: mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA,
	pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
	wsa-z923LK4zBo2bacvFa/9K2g,
	honclo-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, galak-sgV2jX0FEOL9JmXXK+q4OQ,
	cclaudio-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8
In-Reply-To: <20161008211123.GA30452@rob-hp-laptop>



On 10/09/2016 02:41 AM, Rob Herring wrote:
> On Wed, Sep 28, 2016 at 04:30:40AM -0400, Nayna Jain wrote:
>> Virtual TPM, which is being used on IBM POWER7+ and POWER8 systems running
>> POWERVM, is currently supported by tpm device driver but lacks the
>> documentation. This patch adds the missing documentation for the existing
>> support.
>>
>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> ---
>> Changelog v2:
>>
>> - New Patch
>>
>>   .../devicetree/bindings/security/tpm/ibmvtpm.txt   | 41 ++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt b/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
>> new file mode 100644
>> index 0000000..d89f999
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
>> @@ -0,0 +1,41 @@
>> +* Device Tree Bindings for IBM Virtual Trusted Platform Module(vtpm)
>> +
>> +Required properties:
>> +
>> +- compatible            : property name that conveys the platform architecture
>> +                          identifiers, as 'IBM,vtpm'
>
> You use IBM here, but...
>
>> +- device_type           : specifies type of virtual device
>> +- interrupts            : property specifying the interrupt source number and
>> +                          sense code associated with this virtual I/O Adapters
>> +- ibm,my-drc-index      : integer index for the connector between the device
>
> use ibm here. These should be the same unless there's some history I'm
> not aware of.

This is how the property was defined when the change was submitted to 
the PAPR. It was most likely an oversight that the IBM was not changed 
to ibm to be consistent with other property definitions.

>
>> +                          and its parent - present only if Dynamic
>> +                          Reconfiguration(DR) Connector is enabled
>> +- ibm,#dma-address-cells: specifies the number of cells that are used to
>> +                          encode the physical address field of dma-window
>> +                          properties
>> +- ibm,#dma-size-cells   : specifies the number of cells that are used to
>> +                          encode the size field of dma-window properties
>> +- ibm,my-dma-window     : specifies DMA window associated with this virtual
>> +                          IOA
>
> Are these "standard" IBM properties? Does dma-ranges not work for you?


Yes, these are the standard properties as defined in the PAPR.  Again 
the "ibm" prefix indicates that they are specific to IBM system 
implementation. They have been in existence for many years.


Thanks & Regards,
    - Nayna

>
>> +- ibm,loc-code          : specifies the unique and persistent location code
>> +                          associated with this virtual I/O Adapters
>> +- linux,sml-base        : 64-bit base address of the reserved memory allocated
>> +                          for the firmware event log
>> +- linux,sml-size        : size of the memory allocated for the firmware event log
>> +
>> +Example (IBM Virtual Trusted Platform Module)
>> +---------------------------------------------
>> +
>> +                vtpm@30000003 {
>> +                        ibm,#dma-size-cells = <0x2>;
>> +                        compatible = "IBM,vtpm";
>> +                        device_type = "IBM,vtpm";
>> +                        ibm,my-drc-index = <0x30000003>;
>> +                        ibm,#dma-address-cells = <0x2>;
>> +                        linux,sml-base = <0xc60e 0x0>;
>> +                        interrupts = <0xa0003 0x0>;
>> +                        ibm,my-dma-window = <0x10000003 0x0 0x0 0x0 0x10000000>;
>> +                        ibm,loc-code = "U8286.41A.10082DV-V3-C3";
>> +                        reg = <0x30000003>;
>> +                        linux,sml-size = <0xbce10200>;
>> +                };
>> --
>> 2.5.0
>>
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

^ permalink raw reply

* Re: [PATCH v2 2/2] Documentation: tpm: add the Physical TPM device tree binding documentation
From: Nayna @ 2016-10-17  2:14 UTC (permalink / raw)
  To: Rob Herring
  Cc: mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA,
	pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
	wsa-z923LK4zBo2bacvFa/9K2g,
	honclo-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, galak-sgV2jX0FEOL9JmXXK+q4OQ,
	cclaudio-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8
In-Reply-To: <20161008210522.GB22112@rob-hp-laptop>



On 10/09/2016 02:35 AM, Rob Herring wrote:
> On Wed, Sep 28, 2016 at 04:30:41AM -0400, Nayna Jain wrote:
>> Newly added support of TPM 2.0 eventlog securityfs pseudo files in tpm
>> device driver consumes device tree bindings representing I2C based
>> Physical TPM. This patch adds the documentation for corresponding device
>> tree bindings of I2C based Physical TPM. These bindings are similar to
>> vtpm device tree bindings being used on IBM Power7+ and Power8 Systems
>> running PowerVM.
>>
>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> ---
>> Changelog v2:
>>
>> - Include review feedbacks.
>>    - Move the doc within bindings/security/tpm.
>>    - Add example for compatible property in description.
>>    - Delete implicit properties like status, label from description.
>>    - Redefine linux,sml-base description.
>>
>>   .../devicetree/bindings/security/tpm/tpm-i2c.txt     | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt
>>
>> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt
>> new file mode 100644
>> index 0000000..16df8bb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt
>> @@ -0,0 +1,20 @@
>> +* Device Tree Bindings for I2C based Trusted Platform Module(TPM)
>> +
>> +Required properties:
>> +
>> +- compatible     : 'manufacturer,model', eg. nuvoton,npct650
>> +- linux,sml-base : 64-bit base address of the reserved memory allocated for
>> +                   the firmware event log
>> +- linux,sml-size : size of the memory allocated for the firmware event log
>> +
>> +Example (for OpenPower Systems with Nuvoton TPM 2.0 on I2C)
>> +----------------------------------------------------------
>> +
>> +tpm@57 {
>> +	reg = <0x57>;
>> +	label = "tpm";
>
> Not documented, but why? label is really only useful when you have
> multiple items that humans need to identify like ports on ethernet
> switch or LEDs.

Sorry, I think I misinterpreted one of the earliar feedback. I will 
document it again.

Thanks & Regards,
    - Nayna

>
>> +	compatible = "nuvoton,npct650", "nuvoton,npct601";
>> +	linux,sml-base = <0x7f 0xfd450000>;
>> +	linux,sml-size = <0x10000>;
>> +	status = "okay";
>> +};
>> --
>> 2.5.0
>>
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

^ permalink raw reply

* Re: [RFC 1/1] drivers: i2c: omap: Add slave support
From: Matthijs van Duin @ 2016-10-17  6:15 UTC (permalink / raw)
  To: Ravikumar
  Cc: Wolfram Sang, Tony Lindgren, linux-omap@vger.kernel.org,
	linux-i2c, lkml
In-Reply-To: <ee8779c1-6203-b694-84ac-7f6780eafc79@ti.com>

On 14 October 2016 at 09:56, Ravikumar <a0131654@ti.com> wrote:
> Dynamic switching on OMAP I2C IP could be a difficult task.

In fact I wouldn't even bother trying. The thread I linked to received
some follow-up by someone wrestling with this and it actually resulted
in the suggestion to use separate master-only and slave-only peripherals
on the same bus just to avoid the horrible race conditions:
https://e2e.ti.com/support/arm/sitara_arm/f/791/p/514961/1990938#1990938


On 14 October 2016 at 10:57, Ravikumar <rk@ti.com> wrote:
>
> On Monday 29 August 2016 09:13 AM, Matthijs van Duin wrote:
>> its irq registers *look* like the usual set { rawstatus, status, en, dis }
>> that's their current standard ("Highlander") for peripherals. They do
>> not however *behave* like the standard set however:
>>    1. status isn't always (rawstatus & enabled)
>>    2. status != 0 does not always imply the irq output is asserted
>>    3. some enable-bits also change the behaviour of rawstatus
>> All of these misbehaviours are unprecedented afaik.
>
> If I understand #1 correctly, you mean that bit value is different in raw vs
> status registers. I've seen some times there was a delay in the value
> reflecting the status register.

I've never seen that myself, in fact how do you even observe that?  I'd
expect such propagation to require at most one cycle, and I don't expect
an irq output would get updated until it does.

> Now #2 and #3 would be crazy, do you have further notes on this?

I'll clarify what I meant.

Normally you can partition the irq bits into two types: level and event.
Level bits reflect some internal status line and cannot be manually set
or cleared.  Handling these involves taking some action to clear the
underlying condition, or disabling its reporting if you no longer care.

Event bits are set by a peripheral when some event occurs, and need to
be manually cleared as part of irq handling.  They can also be manually
set, e.g. for debugging.

The standard irq combiner in modern ("Highlander") TI peripherals can be
modeled roughly by the following code:


#define IRQ_BITS (EVENT_BITS | LEVEL_BITS)

// irq combiner private state
static u32 latched;	// subset of EVENT_BITS
static u32 rawstatus;	// subset of IRQ_BITS
static u32 enabled;	// subset of IRQ_BITS

// signals from peripheral
extern u32 input;	// subset of IRQ_BITS

// irq output signal
bool output;

// propagate signals
void update()
{
	latched |= input & EVENT_BITS;
	rawstatus = latched | input;
	output = !!(rawstatus & enabled);
}

// register interface
u32 read_rawstatus() {  return rawstatus;  }
u32 read_status() {  return rawstatus & enabled;  }
u32 read_enable() {  return enabled;  }
u32 read_disable() {  return enabled;  }
void write_rawstatus(u32 x) {  latched |= x & EVENT_BITS;  update();  }
void write_status(u32 x) {  latched &= ~(x & EVENT_BITS);  update();  }
void write_enable(u32 x) {  enabled |= x & IRQ_BITS;  update();  }
void write_disable(u32 x) {  enabled &= ~(x & IRQ_BITS);  update();  }


(Some variations exist, e.g. u64 instead of u32.)



Now the omap-i2c peripheral's irq combiner looks like this at first
sight, but it violates its semantics in several ways:


#define LEVEL_BITS (BB | ((XUDF | ROVR) & ~enabled))
#define EVENT_BITS (0x7fff & ~BB)
#define IRQ_BITS EVENT_BITS  // BB isn't an irq at all

void update()
{
	latched |= input & EVENT_BITS;
	rawstatus = (latched & ~LEVEL_BITS) | (input & LEVEL_BITS);
	// note: (rawstatus & enabled) == (latched & enabled)
	output = !!(rawstatus & enabled);
}

u32 read_status() {  return rawstatus & (enabled | LEVEL_BITS);  }


Hence:

1. level irqs are visible in status even if not enabled.

2. as direct consequence, read_status() != 0 doesn't necessarily mean
irq output is asserted.

3. when XUDF or ROVR is disabled it shows the level version in both
rawstatus and status.  It is then however still latched, and the latched
version will show up when you enable it.  You can set and clear the
latched bit also when it is disabled (hence invisible).

Also:

4. BB isn't even something reported via irq, what is it doing here?

5. Even though all inputs are latched, some of them are actually level
status signals from the peripheral, which means that to handle them you
have to clear the underlying condition and then additionally clear the
latch.  Very convenient.  XRDY behaves like this in master mode, but is
a normal event (pulsed) in slave mode.  Yay for consistency!

I also have more comments on it in this thread:

https://e2e.ti.com/support/arm/sitara_arm/f/791/p/533394/1966122#1966122

> But, at least the description has been updated on Jacinto 6 device.
>
> I see all 'status' bits are write 1 to clear except for Bus Busy (intended).
>
> While the 'raw' status register bits can not be cleared by writing 1,
> the description says write 1 to set the bit for debug purpose.

That looks like mostly a copy-paste of the documentation of the standard
register interface.  It still appears to be wrong, unless the behaviour
has actually changed since the dm814x.


>> * No ability to selectively ACK/NACK when addressed as slave. If you're
>> unable to respond for some time then you'd end up blocking the bus with
>> clock stretching.  You could temporarily deconfigure your slave address
>> but the TRM states changing slave address is forbidden while bus busy.
>
> Does this lead to bus lock up?

No idea.


Matthijs

^ permalink raw reply

* Re: [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
From: Jarkko Nikula @ 2016-10-17 12:38 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Linux I2C, Mika Westerberg, Wolfram Sang
In-Reply-To: <20161011202045.43787f2e@endymion>

Hi

On 10/11/2016 09:20 PM, Jean Delvare wrote:
>>> Jarkko, still no information about this from your Windows or hardware
>>> folks?
>>
>> No update, in fact some of the contacts have left the company :-(
>
> What do we do then? Without this patch, I2C Block Read doesn't work at
> all on recent chipsets. I don't think it can be worse with the patch.
> And it works fine on the few machines I tested on. So I would like this
> patch to go upstream.
>
> My coverage is limited though. Did anyone at Intel test this patch on
> other (recent) systems?
>
Sorry the delay. I have only handful of machine that have on-board 
EEPROM connected to SMBUS in our lab and they are relatively new so only 
limited testing from my side.

In fact those had the SPD Write Disable set so attempt to use kernel's 
at24 driver didn't register the EEPROM at all without your patch since 
one-byte test read in drivers/misc/eeprom/at24.c: at24_probe() failed.

echo "24c02 0x56" >/sys/bus/i2c/devices/i2c-0/new_device

With your patch at24 probes and I can dump the 0xff content (I guess 
it's unprogrammed) with the hexdump:

hexdump -C /sys/bus/i2c/devices/i2c-0/0-0056/eeprom

I think we are better to have this patch as we know it makes things 
better. I'll add my tested-by to your patch with one comment.

-- 
Jarkko

^ permalink raw reply

* Re: [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
From: Jarkko Nikula @ 2016-10-17 12:38 UTC (permalink / raw)
  To: Jean Delvare, Linux I2C; +Cc: Mika Westerberg, Wolfram Sang
In-Reply-To: <20161011131327.0eabd227@endymion>

On 10/11/2016 02:13 PM, Jean Delvare wrote:
>  drivers/i2c/busses/i2c-i801.c |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> --- linux-4.8.orig/drivers/i2c/busses/i2c-i801.c	2016-10-11 10:41:43.984120197 +0200
> +++ linux-4.8/drivers/i2c/busses/i2c-i801.c	2016-10-11 10:42:46.648783059 +0200
> @@ -146,6 +146,7 @@
>  #define SMBHSTCFG_HST_EN	BIT(0)
>  #define SMBHSTCFG_SMB_SMI_EN	BIT(1)
>  #define SMBHSTCFG_I2C_EN	BIT(2)
> +#define SMBHSTCFG_SPD_WD	BIT(4)
>
Looks like you had some local BIT() cleanup patch before this and patch 
doesn't apply. I fixed it locally and you can add my tested-by to this 
patch.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

^ permalink raw reply

* [PATCH v2 0/2] spi, i2c: mark device nodes only in case of successful instantiation
From: Ralf Ramsauer @ 2016-10-17 13:59 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Wolfram Sang, Linux SPI,
	Linux I2C
  Cc: Ralf Ramsauer, linux-kernel @ vger . kernel . org,
	Pantelis Antoniou

Hi,

this one fixes initialisation of I2C/SPI nodes.  Upon failure during
intialisation, nodes were erroneously populated and never unmarked.

This lead to the problem that re-loaded drivers will never probe those devices
again and can easily be fixed by clearing the OF_POPULATE flag when the node
doesn't successfully initialise.

For the discussion of v1, see
https://lkml.org/lkml/2016/10/14/483

  Ralf

changes since v1:
  - also fix I2C core driver
  - keep the atomic test-and-set, as Geert suggested

Ralf Ramsauer (2):
  spi: mark device nodes only in case of successful instantiation
  i2c: mark device nodes only in case of successful instantiation

 drivers/i2c/i2c-core.c | 11 ++++++++++-
 drivers/spi/spi.c      |  5 ++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.10.1

^ permalink raw reply

* [PATCH v2 1/2] spi: mark device nodes only in case of successful instantiation
From: Ralf Ramsauer @ 2016-10-17 13:59 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Wolfram Sang, Linux SPI,
	Linux I2C
  Cc: Ralf Ramsauer, linux-kernel @ vger . kernel . org,
	Pantelis Antoniou
In-Reply-To: <20161017135957.20297-1-ralf@ramses-pyramidenbau.de>

Instantiated SPI device nodes are marked with OF_POPULATE. This was
introduced in bd6c164. On unloading, loaded device nodes will of course
be unmarked. The problem are nodes that fail during initialisation: If a
node fails, it won't be unloaded and hence not be unmarked.

If a SPI driver module is unloaded and reloaded, it will skip nodes that
failed before.

Skip device nodes that are already populated and mark them only in case
of success.

Note that the same issue exists for I2C.

Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
---
 drivers/spi/spi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 5787b72..838783c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1618,9 +1618,11 @@ static void of_register_spi_devices(struct spi_master *master)
 		if (of_node_test_and_set_flag(nc, OF_POPULATED))
 			continue;
 		spi = of_register_spi_device(master, nc);
-		if (IS_ERR(spi))
+		if (IS_ERR(spi)) {
 			dev_warn(&master->dev, "Failed to create SPI device for %s\n",
 				nc->full_name);
+			of_node_clear_flag(nc, OF_POPULATED);
+		}
 	}
 }
 #else
@@ -3131,6 +3133,7 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,
 		if (IS_ERR(spi)) {
 			pr_err("%s: failed to create for '%s'\n",
 					__func__, rd->dn->full_name);
+			of_node_clear_flag(rd->dn, OF_POPULATED);
 			return notifier_from_errno(PTR_ERR(spi));
 		}
 		break;
-- 
2.10.1

^ permalink raw reply related

* [PATCH v2 2/2] i2c: mark device nodes only in case of successful instantiation
From: Ralf Ramsauer @ 2016-10-17 13:59 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Wolfram Sang, Linux SPI,
	Linux I2C
  Cc: Ralf Ramsauer, linux-kernel @ vger . kernel . org,
	Pantelis Antoniou
In-Reply-To: <20161017135957.20297-1-ralf@ramses-pyramidenbau.de>

Instantiated I2C device nodes are marked with OF_POPULATE. This was
introduced in 4f001fd. On unloading, loaded device nodes will of course
be unmarked. The problem are nodes that fail during initialisation: If a
node fails, it won't be unloaded and hence not be unmarked.

If a I2C driver module is unloaded and reloaded, it will skip nodes that
failed before.

Skip device nodes that are already populated and mark them only in case
of success.

Note that the same issue exists for SPI.

Fixes: 4f001fd ("i2c: Mark instantiated device nodes with OF_POPULATE")
Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
---
 drivers/i2c/i2c-core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 5ab6721..1704fc8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1681,6 +1681,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
 static void of_i2c_register_devices(struct i2c_adapter *adap)
 {
 	struct device_node *bus, *node;
+	struct i2c_client *client;
 
 	/* Only register child devices if the adapter has a node pointer set */
 	if (!adap->dev.of_node)
@@ -1695,7 +1696,14 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
 	for_each_available_child_of_node(bus, node) {
 		if (of_node_test_and_set_flag(node, OF_POPULATED))
 			continue;
-		of_i2c_register_device(adap, node);
+
+		client = of_i2c_register_device(adap, node);
+		if (IS_ERR(client)) {
+			dev_warn(&adap->dev,
+				 "Failed to create I2C device for %s\n",
+				 node->full_name);
+			of_node_clear_flag(node, OF_POPULATED);
+		}
 	}
 
 	of_node_put(bus);
@@ -2299,6 +2307,7 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
 		if (IS_ERR(client)) {
 			dev_err(&adap->dev, "failed to create client for '%s'\n",
 				 rd->dn->full_name);
+			of_node_clear_flag(rd->dn, OF_POPULATED);
 			return notifier_from_errno(PTR_ERR(client));
 		}
 		break;
-- 
2.10.1

^ permalink raw reply related

* Re: [PATCH v2 1/2] spi: mark device nodes only in case of successful instantiation
From: Geert Uytterhoeven @ 2016-10-17 14:15 UTC (permalink / raw)
  To: Ralf Ramsauer
  Cc: Mark Brown, Wolfram Sang, Linux SPI, Linux I2C,
	linux-kernel @ vger . kernel . org, Pantelis Antoniou
In-Reply-To: <20161017135957.20297-2-ralf@ramses-pyramidenbau.de>

On Mon, Oct 17, 2016 at 3:59 PM, Ralf Ramsauer
<ralf@ramses-pyramidenbau.de> wrote:
> Instantiated SPI device nodes are marked with OF_POPULATE. This was
> introduced in bd6c164. On unloading, loaded device nodes will of course
> be unmarked. The problem are nodes that fail during initialisation: If a
> node fails, it won't be unloaded and hence not be unmarked.
>
> If a SPI driver module is unloaded and reloaded, it will skip nodes that
> failed before.
>
> Skip device nodes that are already populated and mark them only in case
> of success.
>
> Note that the same issue exists for I2C.
>
> Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* Re: [PATCH v2 2/2] i2c: mark device nodes only in case of successful instantiation
From: Geert Uytterhoeven @ 2016-10-17 14:16 UTC (permalink / raw)
  To: Ralf Ramsauer
  Cc: Mark Brown, Wolfram Sang, Linux SPI, Linux I2C,
	linux-kernel @ vger . kernel . org, Pantelis Antoniou
In-Reply-To: <20161017135957.20297-3-ralf@ramses-pyramidenbau.de>

On Mon, Oct 17, 2016 at 3:59 PM, Ralf Ramsauer
<ralf@ramses-pyramidenbau.de> wrote:
> Instantiated I2C device nodes are marked with OF_POPULATE. This was
> introduced in 4f001fd. On unloading, loaded device nodes will of course
> be unmarked. The problem are nodes that fail during initialisation: If a
> node fails, it won't be unloaded and hence not be unmarked.
>
> If a I2C driver module is unloaded and reloaded, it will skip nodes that
> failed before.
>
> Skip device nodes that are already populated and mark them only in case
> of success.
>
> Note that the same issue exists for SPI.
>
> Fixes: 4f001fd ("i2c: Mark instantiated device nodes with OF_POPULATE")
> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c

> @@ -1695,7 +1696,14 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
>         for_each_available_child_of_node(bus, node) {
>                 if (of_node_test_and_set_flag(node, OF_POPULATED))
>                         continue;
> -               of_i2c_register_device(adap, node);
> +
> +               client = of_i2c_register_device(adap, node);
> +               if (IS_ERR(client)) {
> +                       dev_warn(&adap->dev,
> +                                "Failed to create I2C device for %s\n",
> +                                node->full_name);

I don't think there's a need to add this warning, as of_i2c_register_device()
already prints messages in all failure paths.

> +                       of_node_clear_flag(node, OF_POPULATED);
> +               }
>         }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* Re: [PATCH v2 2/2] i2c: mark device nodes only in case of successful instantiation
From: Ralf Ramsauer @ 2016-10-17 14:37 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Wolfram Sang, Linux SPI, Linux I2C,
	linux-kernel @ vger . kernel . org, Pantelis Antoniou
In-Reply-To: <CAMuHMdXsGXV-udnNXmrUet6rDgiW5dYYam5vXg7450POALXkVQ@mail.gmail.com>



On 10/17/2016 04:16 PM, Geert Uytterhoeven wrote:
> On Mon, Oct 17, 2016 at 3:59 PM, Ralf Ramsauer
> <ralf@ramses-pyramidenbau.de> wrote:
>> Instantiated I2C device nodes are marked with OF_POPULATE. This was
>> introduced in 4f001fd. On unloading, loaded device nodes will of course
>> be unmarked. The problem are nodes that fail during initialisation: If a
>> node fails, it won't be unloaded and hence not be unmarked.
>>
>> If a I2C driver module is unloaded and reloaded, it will skip nodes that
>> failed before.
>>
>> Skip device nodes that are already populated and mark them only in case
>> of success.
>>
>> Note that the same issue exists for SPI.
>>
>> Fixes: 4f001fd ("i2c: Mark instantiated device nodes with OF_POPULATE")
>> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
> 
>> @@ -1695,7 +1696,14 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
>>         for_each_available_child_of_node(bus, node) {
>>                 if (of_node_test_and_set_flag(node, OF_POPULATED))
>>                         continue;
>> -               of_i2c_register_device(adap, node);
>> +
>> +               client = of_i2c_register_device(adap, node);
>> +               if (IS_ERR(client)) {
>> +                       dev_warn(&adap->dev,
>> +                                "Failed to create I2C device for %s\n",
>> +                                node->full_name);
> 
> I don't think there's a need to add this warning, as of_i2c_register_device()
> already prints messages in all failure paths.
So does of_register_spi_device(). And there's a dev_warn() in
of_spi_register_devices() as well...
But yes, I was also thinking of this.

Nevertheless, I added it as I thought this would keep both drivers
somehow consistent. And it's a very rare error case, though.

  Ralf
> 
>> +                       of_node_clear_flag(node, OF_POPULATED);
>> +               }
>>         }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

-- 
Ralf Ramsauer
PGP: 0x8F10049B

^ permalink raw reply

* Re: [PATCH 1/2] i2c: bcm-iproc: constify i2c_adapter_quirks structures
From: Ray Jui @ 2016-10-17 17:49 UTC (permalink / raw)
  To: Julia Lawall, Wolfram Sang
  Cc: kernel-janitors, Ray Jui, Scott Branden, Jon Mason,
	bcm-kernel-feedback-list, linux-i2c, linux-arm-kernel,
	linux-kernel
In-Reply-To: <1476552722-12352-2-git-send-email-Julia.Lawall@lip6.fr>

Hi Julia,

On 10/15/2016 10:32 AM, Julia Lawall wrote:
> Check for i2c_adapter_quirks structures that are only stored in the
> quirks field of an i2c_adapter structure.  This field is declared
> const, so i2c_adapter_quirks structures that have this property can be
> declared as const also.
>
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r disable optional_qualifier@
> identifier i;
> position p;
> @@
> static struct i2c_adapter_quirks i@p = { ... };
>
> @ok@
> identifier r.i;
> struct i2c_adapter e;
> position p;
> @@
> e.quirks = &i@p;
>
> @bad@
> position p != {r.p,ok.p};
> identifier r.i;
> struct i2c_adapter_quirks e;
> @@
> e@i@p
>
> @depends on !bad disable optional_qualifier@
> identifier r.i;
> @@
> static
> +const
>  struct i2c_adapter_quirks i = { ... };
> // </smpl>
>
> The effect on the layout of the .o file is shown by the following
> output of the size command, first before then after the
> transformation:
>
>    text    data     bss     dec     hex filename
>    3458     744       8    4210    1072 drivers/i2c/busses/i2c-bcm-iproc.o
>    3490     720       8    4218    107a drivers/i2c/busses/i2c-bcm-iproc.o
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
>  drivers/i2c/busses/i2c-bcm-iproc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index 326b3db..318df55 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -395,7 +395,7 @@ static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
>  	.functionality = bcm_iproc_i2c_functionality,
>  };
>
> -static struct i2c_adapter_quirks bcm_iproc_i2c_quirks = {
> +static const struct i2c_adapter_quirks bcm_iproc_i2c_quirks = {
>  	/* need to reserve one byte in the FIFO for the slave address */
>  	.max_read_len = M_TX_RX_FIFO_SIZE - 1,
>  };
>

Change looks good. Thanks.

Acked-by: Ray Jui <ray.jui@broadcom.com>

^ permalink raw reply

* Re: [PATCH 06/10] gpio: sam: Document bindings of SAM FPGA GPIO block
From: Pantelis Antoniou @ 2016-10-17 19:01 UTC (permalink / raw)
  To: Rob Herring
  Cc: Lee Jones, Linus Walleij, Alexandre Courbot, Mark Rutland,
	Frank Rowand, Wolfram Sang, David Woodhouse, Brian Norris,
	Florian Fainelli, Wim Van Sebroeck, Peter Rosin, Debjit Ghosh,
	Georgi Vlaev, Guenter Roeck, Maryam Seraj, devicetree,
	linux-kernel, linux-gpio, linux-i2c, linux-mtd, linux-watchdog,
	netdev
In-Reply-To: <20161010200346.GA24239@rob-hp-laptop>

Hi Rob,

> On Oct 10, 2016, at 23:03 , Rob Herring <robh@kernel.org> wrote:
> 
> On Fri, Oct 07, 2016 at 06:18:34PM +0300, Pantelis Antoniou wrote:
>> From: Georgi Vlaev <gvlaev@juniper.net>
>> 
>> Add device tree bindings document for the GPIO driver of
>> Juniper's SAM FPGA.
>> 
>> Signed-off-by: Georgi Vlaev <gvlaev@juniper.net>
>> [Ported from Juniper kernel]
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> .../devicetree/bindings/gpio/jnx,gpio-sam.txt      | 110 +++++++++++++++++++++
>> 1 file changed, 110 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/gpio/jnx,gpio-sam.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/gpio/jnx,gpio-sam.txt b/Documentation/devicetree/bindings/gpio/jnx,gpio-sam.txt
>> new file mode 100644
>> index 0000000..514c350
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/jnx,gpio-sam.txt
>> @@ -0,0 +1,110 @@
>> +Juniper SAM FPGA GPIO block
>> +
>> +The controller's registers are organized as sets of eight 32-bit
>> +registers with each set controlling a bank of up to 32 pins.  A single
>> +interrupt is shared for all of the banks handled by the controller.
>> +
>> +Required properties:
>> +
>> +- compatible:
>> +    Must be "jnx,gpio-sam"
>> +
>> +- #gpio-cells:
>> +    Should be <2>.  The first cell is the pin number (within the controller's
>> +    pin space), and the second is used for the following flags:
>> +	bit[0]: direction (0 = out, 1 = in)
>> +	bit[1]: init high
>> +	bit[2]: active low
>> +	bit[3]: open drain
>> +	bit[4]: open drain
> 
> Use and/or add to standard flags.
> 

OK.

>> +
>> +- gpio-controller:
>> +    Specifies that the node is a GPIO controller.
>> +
>> +Optional properties:
>> +
>> +- reg:
>> +    This driver is part of the SAM FPGA MFD driver, so the
>> +    address range is supplied by that driver. However you can
>> +    override using this property.
>> +
>> +- gpio-base:
>> +    Base of the GPIO pins of this instance. If not present use system allocated.
> 
> This probably needs to go.
> 

OK.

>> +
>> +- gpio-count:
> 
> ngpios instead.
> 

OK.

>> +    Number of GPIO pins of this instance. If not present read the number from
>> +    the one configured in the FPGA data. Maximum number is 512.
>> +
>> +- #interrupt-cells:
>> +    Should be <2>.  The first cell is the GPIO number, the second should specify
>> +    flags.  The following subset of flags is supported:
>> +    - bits[16,4:0] trigger type and level flags
>> +	bit  0: rising edge interrupt
>> +	bit  1: falling edge interrupt
>> +	bit  2: active high interrupt
>> +	bit  3: active low interrupt
>> +	bit  4: enable debounce
>> +	bit 16: signal is active low
> 
> What does this mean?
> 

I will reword.

>> +    See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> +
>> +- gpio-interrupts:
>> +    A number of triples that define the mapping of interrupt groupsb to a range of
>> +    pins. The first cell defines the interrupt group, the second is the start of
>> +    the pin range and the third the number of pins in the range.
> 
> Needs a vendor prefix.
> 

OK.

>> +
>> +- gpio-exports:
>> +    A subnode containing the list of pins that will be exported to user-space.
> 
> DT doesn't know about userspace. Drop this.
> 

OK, the export bit should go. 

>> +    Each subnode contains:
>> +    Required properties:
>> +	- pin: The gpio to be exported and the relevant flags.
>> +    Optional properties:
>> +        - label: The label to use for export; if not supplied use the node name.
>> +
>> +Example:
>> +
>> +gpio20: gpio-sam {
>> +	compatible = "jnx,gpio-sam";
>> +	gpio-controller;
>> +	interrupt-controller;
>> +	/* 1st cell: gpio pin
>> +	 * 2nd cell: flags (bit mask)
>> +	 * bit  0: rising edge interrupt
>> +	 * bit  1: falling edge interrupt
>> +	 * bit  2: active high interrupt
>> +	 * bit  3: active low interrupt
>> +	 * bit  4: enable debounce
>> +	 * bit 16: signal is active low
>> +	 */
>> +	#interrupt-cells = <2>;
>> +	#gpio-cells = <2>;
>> +	gpio-count = <340>;
>> +	/* 1st cell: gpio interrupt status bit
>> +	 * 2nd cell: 1st pin
>> +	 * 3rd cell: # of pins
>> +	 */
>> +	gpio-interrupts =
>> +		<0 0 32>,	/* TL / TQ */
>> +		<1 32 32>,	/* PIC 1 */
>> +		<2 32 32>,	/* PIC 1 spare */
>> +		<7 148 32>,	/* PIC 0 */
>> +		<8 170 32>,	/* PIC 0 spare */
>> +		<16 318 22>;	/* FPC */
>> +
>> +	gpio-exports {
>> +		/*
>> +		 * flags:
>> +		 * GPIOF_DIR_IN			bit 0=1
>> +		 * GPIOF_DIR_OUT		bit 0=0
>> +		 * GPIOF_INIT_HIGH		bit 1=1
>> +		 *   GPIOF_INIT_HIGH is raw, not translated
>> +		 * GPIOF_ACTIVE_LOW		bit 2=1
>> +		 * GPIOF_OPEN_DRAIN		bit 3=1
>> +		 * GPIOF_OPEN_SOURCE		bit 4=1
>> +		 * GPIOF_EXPORT			bit 5=1
>> +		 * GPIOF_EXPORT_CHANGEABLE      bit 6=1
>> +		 */
>> +		tl0-rst {
>> +			pin = < 8 0x24 >;
>> +		};
>> +	};
>> +};
>> -- 
>> 1.9.1

^ permalink raw reply

* Re: [PATCH 08/10] mtd: flash-sam: Bindings for Juniper's SAM FPGA flash
From: Pantelis Antoniou @ 2016-10-17 19:03 UTC (permalink / raw)
  To: Rob Herring
  Cc: Lee Jones, Linus Walleij, Alexandre Courbot, Mark Rutland,
	Frank Rowand, Wolfram Sang, David Woodhouse, Brian Norris,
	Florian Fainelli, Wim Van Sebroeck, Peter Rosin, Debjit Ghosh,
	Georgi Vlaev, Guenter Roeck, Maryam Seraj, devicetree,
	linux-kernel, linux-gpio, linux-i2c, linux-mtd, linux-watchdog,
	netdev
In-Reply-To: <20161010200704.GA28870@rob-hp-laptop>

Hi Rob,

> On Oct 10, 2016, at 23:07 , Rob Herring <robh@kernel.org> wrote:
> 
> gOn Fri, Oct 07, 2016 at 06:18:36PM +0300, Pantelis Antoniou wrote:
>> From: Georgi Vlaev <gvlaev@juniper.net>
>> 
>> Add binding document for Junipers Flash IP block present
>> in the SAM FPGA on PTX series of routers.
>> 
>> Signed-off-by: Georgi Vlaev <gvlaev@juniper.net>
>> [Ported from Juniper kernel]
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> .../devicetree/bindings/mtd/flash-sam.txt          | 31 ++++++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mtd/flash-sam.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/mtd/flash-sam.txt b/Documentation/devicetree/bindings/mtd/flash-sam.txt
>> new file mode 100644
>> index 0000000..bdf1d78
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/flash-sam.txt
>> @@ -0,0 +1,31 @@
>> +Flash device on a Juniper SAM FPGA
>> +
>> +These flash chips are found in the PTX series of Juniper routers.
>> +
>> +They are regular CFI compatible (Intel or AMD extended) flash chips with
>> +some special write protect/VPP bits that can be controlled by the machine's
>> +system controller.
> 
> And where's the description of the sys ctrlr?
> 

The system controller is Juniper IP. We’ll have to ask around about
specifics, and it’s pretty uninspiring stuff.

>> +
>> +Required properties:
>> +- compatible : must be "jnx,flash-sam"
>> +
>> +Optional properties:
>> +- reg : memory address for the flash chip, note that this is not
>> +required since usually the device is a subdevice of the SAM MFD
>> +driver which fills in the register fields.
>> +
>> +For the rest of the properties, see mtd-physmap.txt.
>> +
>> +The device tree may optionally contain sub-nodes describing partitions of the
>> +address space. See partition.txt for more detail.
>> +
>> +Example:
>> +
>> +flash_sam {
>> +	compatible = "jnx,flash-sam";
>> +	partition@0 {
> 
> This should have a heirarchy of a controller node, a flash child node, 
> partitions child node, and partition child nodes.
> 

OK.

>> +		reg = <0x0 0x400000>;
>> +		label = "pic0-golden";
>> +		read-only;
>> +	};
>> +};
>> -- 
>> 1.9.1

^ permalink raw reply

* Re: [PATCH 2/4] mfd: ptxpmb-ext-cpld: Add documentation for PTXPMB extended CPLD
From: Pantelis Antoniou @ 2016-10-17 19:08 UTC (permalink / raw)
  To: Rob Herring
  Cc: Lee Jones, Linus Walleij, Alexandre Courbot, Mark Rutland,
	Frank Rowand, Georgi Vlaev, Guenter Roeck,
	JawaharBalaji Thirumalaisamy, devicetree, linux-kernel,
	linux-gpio, linux-i2c, linux-mtd, linux-watchdog, netdev
In-Reply-To: <20161010201031.GA31318@rob-hp-laptop>

Hi Rob,

> On Oct 10, 2016, at 23:10 , Rob Herring <robh@kernel.org> wrote:
> 
> On Fri, Oct 07, 2016 at 06:19:32PM +0300, Pantelis Antoniou wrote:
>> From: Georgi Vlaev <gvlaev@juniper.net>
>> 
>> Add DT bindings document for the PTXPMB extended CPLD device.
>> 
>> Signed-off-by: Georgi Vlaev <gvlaev@juniper.net>
>> [Ported from Juniper kernel]
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> .../bindings/mfd/jnx-ptxpmb-ext-cpld.txt           | 35 ++++++++++++++++++++++
>> 1 file changed, 35 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mfd/jnx-ptxpmb-ext-cpld.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/mfd/jnx-ptxpmb-ext-cpld.txt b/Documentation/devicetree/bindings/mfd/jnx-ptxpmb-ext-cpld.txt
>> new file mode 100644
>> index 0000000..098a548a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/jnx-ptxpmb-ext-cpld.txt
>> @@ -0,0 +1,35 @@
>> +* Device tree bindings for Juniper's PTXPMB Extended CPLD FPGA MFD driver
>> +
>> +The device supports a gpio block which is described in the
>> +jnx-gpio-ptxpmb-ext-cpld document.
>> +
>> +Required properties:
>> +
>> +- compatible:		"jnx,ptxpmb-ext-cpld"
>> +
>> +- reg:			contains offset/length value for device state control
>> +			registers space.
>> +
>> +Optional properties:
>> +
>> +- interrupts:		The interrupt line(s) the /IRQ signal(s) for the device is
>> +			connected to.
>> +
>> +- interrupt-parent:	The parent interrupt controller.
>> +
>> +Example:
>> +
>> +ext-cpld@1,0 {
>> +	compatible = "jnx,ptxpmb-ext-cpld";
>> +	reg = <0x1 0 0x1000>;
> 
> What's the bus type here? Unit address is probably wrong.
> 

localbus on a gpmc memory controller.

>> +	interrupt-parent = <&mpic>;
>> +	interrupts = <7 2>, <8 2>;
>> +
>> +	gpio_ext_cpld: cpld-ext-gpio {
>> +		compatible = "jnx,gpio-ptxpmb-ext-cpld";
>> +		#gpio-cells = <2>;
>> +		#interrupt-cells = <2>;
>> +		gpio-controller;
>> +		interrupt-controller;
>> +	};
>> +};
>> -- 
>> 1.9.1


^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox