[PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Alexander Shiyan]

This patch takes the CS active level from the GPIO bindings,
so we remove the special property "spi-cs-high" for chipselects
that use GPIO.

Signed-off-by: Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org>
---
 drivers/spi/spi.c       | 28 ++++++++++++++++++----------
 include/linux/spi/spi.h |  2 ++
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 98f4b77..d2ba1ec 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -413,8 +413,12 @@ int spi_add_device(struct spi_device *spi)
 		goto done;
 	}
 
-	if (master->cs_gpios)
+	if (master->cs_gpios) {
 		spi->cs_gpio = master->cs_gpios[spi->chip_select];
+		if (!(master->cs_gpios_flags[spi->chip_select] &
+		      OF_GPIO_ACTIVE_LOW))
+			spi->mode |= SPI_CS_HIGH;
+	}
 
 	/* Drivers may modify this initial i/o setup, but will
 	 * normally rely on the device being setup.  Devices
@@ -1027,10 +1031,14 @@ static void of_register_spi_devices(struct spi_master *master)
 			spi->mode |= SPI_CPHA;
 		if (of_find_property(nc, "spi-cpol", NULL))
 			spi->mode |= SPI_CPOL;
-		if (of_find_property(nc, "spi-cs-high", NULL))
-			spi->mode |= SPI_CS_HIGH;
 		if (of_find_property(nc, "spi-3wire", NULL))
 			spi->mode |= SPI_3WIRE;
+		if (of_find_property(nc, "spi-cs-high", NULL)) {
+			if (master->cs_gpios)
+				dev_notice(&master->dev,
+					"\"spi-cs-high\" property is obsolete.\n");
+			spi->mode |= SPI_CS_HIGH;
+		}
 
 		/* Device DUAL/QUAD mode */
 		if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
@@ -1260,7 +1268,7 @@ EXPORT_SYMBOL_GPL(spi_alloc_master);
 #ifdef CONFIG_OF
 static int of_spi_register_master(struct spi_master *master)
 {
-	int nb, i, *cs;
+	int nb, i;
 	struct device_node *np = master->dev.of_node;
 
 	if (!np)
@@ -1275,19 +1283,19 @@ static int of_spi_register_master(struct spi_master *master)
 	else if (nb < 0)
 		return nb;
 
-	cs = devm_kzalloc(&master->dev,
-			  sizeof(int) * master->num_chipselect,
-			  GFP_KERNEL);
-	master->cs_gpios = cs;
+	master->cs_gpios = devm_kzalloc(&master->dev, master->num_chipselect *
+		(sizeof(int) + sizeof(enum of_gpio_flags)), GFP_KERNEL);
 
 	if (!master->cs_gpios)
 		return -ENOMEM;
 
 	for (i = 0; i < master->num_chipselect; i++)
-		cs[i] = -ENOENT;
+		master->cs_gpios[i] = -ENOENT;
 
 	for (i = 0; i < nb; i++)
-		cs[i] = of_get_named_gpio(np, "cs-gpios", i);
+		master->cs_gpios[i] =
+			of_get_named_gpio_flags(np, "cs-gpios", i,
+						&master->cs_gpios_flags[i]);
 
 	return 0;
 }
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 8c62ba7..c0cade2 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -423,6 +423,8 @@ struct spi_master {
 
 	/* gpio chip select */
 	int			*cs_gpios;
+	/* GPIO flags */
+	enum of_gpio_flags	*cs_gpios_flags;
 };
 
 static inline void *spi_master_get_devdata(struct spi_master *master)
-- 
1.8.3.2

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

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Mark Brown]

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

On Sun, Dec 01, 2013 at 11:59:16AM +0400, Alexander Shiyan wrote:
> This patch takes the CS active level from the GPIO bindings,
> so we remove the special property "spi-cs-high" for chipselects
> that use GPIO.

Unfortuantely this would introduce an incompatible change in the DT
bindings so we can't do this.  Updating the documentation to deprecate
the old binding would be OK, though I'm not sure it's a great advantage
since it means you wouldn't be able to use the same binding with GPIOs
as you could with controller integrated chip selects.

Adding support for using the GPIO binding information is fine but
removing the old method (especially without a binding update) doesn't
seem great.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Alexander Shiyan]

> On Sun, Dec 01, 2013 at 11:59:16AM +0400, Alexander Shiyan wrote:
> > This patch takes the CS active level from the GPIO bindings,
> > so we remove the special property "spi-cs-high" for chipselects
> > that use GPIO.
> 
> Unfortuantely this would introduce an incompatible change in the DT
> bindings so we can't do this.  Updating the documentation to deprecate
> the old binding would be OK, though I'm not sure it's a great advantage
> since it means you wouldn't be able to use the same binding with GPIOs
> as you could with controller integrated chip selects.
> 
> Adding support for using the GPIO binding information is fine but
> removing the old method (especially without a binding update) doesn't
> seem great.

Old method still remains (especially for integrated CS), we just warn the
GPIO-CS users that it is better to use GPIO-bindings.
Code is constructed so that there is no incompatibility between the
use of old and new methods.

---

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Mark Brown]

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

On Mon, Dec 02, 2013 at 03:58:33PM +0400, Alexander Shiyan wrote:

> Old method still remains (especially for integrated CS), we just warn the
> GPIO-CS users that it is better to use GPIO-bindings.
> Code is constructed so that there is no incompatibility between the
> use of old and new methods.

My point is that I don't think it's especially useful to deprecate the
existing method - telling people that they are using a deprecated
method is a kind of incompatibility and it suggests an intention to
remove in the future.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Alexander Shiyan]

> On Mon, Dec 02, 2013 at 03:58:33PM +0400, Alexander Shiyan wrote:
> 
> > Old method still remains (especially for integrated CS), we just warn the
> > GPIO-CS users that it is better to use GPIO-bindings.
> > Code is constructed so that there is no incompatibility between the
> > use of old and new methods.
> 
> My point is that I don't think it's especially useful to deprecate the
> existing method - telling people that they are using a deprecated
> method is a kind of incompatibility and it suggests an intention to
> remove in the future.

OK. What about "Add possibility to use GPIO-bindings for define chipselect
active level" in the patch name and remove warning in the patch?
Of course we need to make some changes in the documentation also.

---

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Mark Brown]

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

On Mon, Dec 02, 2013 at 04:42:32PM +0400, Alexander Shiyan wrote:

> OK. What about "Add possibility to use GPIO-bindings for define chipselect
> active level" in the patch name and remove warning in the patch?
> Of course we need to make some changes in the documentation also.

Yes, that sounds good.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Jonas Gorski]

On Sun, Dec 1, 2013 at 8:59 AM, Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org> wrote:
> This patch takes the CS active level from the GPIO bindings,
> so we remove the special property "spi-cs-high" for chipselects
> that use GPIO.
>
> Signed-off-by: Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org>
> ---
>  drivers/spi/spi.c       | 28 ++++++++++++++++++----------
>  include/linux/spi/spi.h |  2 ++
>  2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 98f4b77..d2ba1ec 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -413,8 +413,12 @@ int spi_add_device(struct spi_device *spi)
>                 goto done;
>         }
>
> -       if (master->cs_gpios)
> +       if (master->cs_gpios) {
>                 spi->cs_gpio = master->cs_gpios[spi->chip_select];
> +               if (!(master->cs_gpios_flags[spi->chip_select] &
> +                     OF_GPIO_ACTIVE_LOW))
> +                       spi->mode |= SPI_CS_HIGH;
> +       }
>
>         /* Drivers may modify this initial i/o setup, but will
>          * normally rely on the device being setup.  Devices
> @@ -1027,10 +1031,14 @@ static void of_register_spi_devices(struct spi_master *master)
>                         spi->mode |= SPI_CPHA;
>                 if (of_find_property(nc, "spi-cpol", NULL))
>                         spi->mode |= SPI_CPOL;
> -               if (of_find_property(nc, "spi-cs-high", NULL))
> -                       spi->mode |= SPI_CS_HIGH;
>                 if (of_find_property(nc, "spi-3wire", NULL))
>                         spi->mode |= SPI_3WIRE;
> +               if (of_find_property(nc, "spi-cs-high", NULL)) {
> +                       if (master->cs_gpios)
> +                               dev_notice(&master->dev,
> +                                       "\"spi-cs-high\" property is obsolete.\n");
> +                       spi->mode |= SPI_CS_HIGH;
> +               }
>
>                 /* Device DUAL/QUAD mode */
>                 if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
> @@ -1260,7 +1268,7 @@ EXPORT_SYMBOL_GPL(spi_alloc_master);
>  #ifdef CONFIG_OF
>  static int of_spi_register_master(struct spi_master *master)
>  {
> -       int nb, i, *cs;
> +       int nb, i;
>         struct device_node *np = master->dev.of_node;
>
>         if (!np)
> @@ -1275,19 +1283,19 @@ static int of_spi_register_master(struct spi_master *master)
>         else if (nb < 0)
>                 return nb;
>
> -       cs = devm_kzalloc(&master->dev,
> -                         sizeof(int) * master->num_chipselect,
> -                         GFP_KERNEL);
> -       master->cs_gpios = cs;
> +       master->cs_gpios = devm_kzalloc(&master->dev, master->num_chipselect *
> +               (sizeof(int) + sizeof(enum of_gpio_flags)), GFP_KERNEL);
>
>         if (!master->cs_gpios)
>                 return -ENOMEM;
>
>         for (i = 0; i < master->num_chipselect; i++)
> -               cs[i] = -ENOENT;
> +               master->cs_gpios[i] = -ENOENT;
>
>         for (i = 0; i < nb; i++)
> -               cs[i] = of_get_named_gpio(np, "cs-gpios", i);
> +               master->cs_gpios[i] =
> +                       of_get_named_gpio_flags(np, "cs-gpios", i,
> +                                               &master->cs_gpios_flags[i]);

I don't see you initializing master->cs_gpio_flags anywhere, so won't
this cause a null/unitialized pointer access?


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

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Alexander Shiyan]

> On Sun, Dec 1, 2013 at 8:59 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> > This patch takes the CS active level from the GPIO bindings,
> > so we remove the special property "spi-cs-high" for chipselects
> > that use GPIO.
...

> > -       cs = devm_kzalloc(&master->dev,
> > -                         sizeof(int) * master->num_chipselect,
> > -                         GFP_KERNEL);
> > -       master->cs_gpios = cs;
> > +       master->cs_gpios = devm_kzalloc(&master->dev, master->num_chipselect *
> > +               (sizeof(int) + sizeof(enum of_gpio_flags)), GFP_KERNEL);
> >
> >         if (!master->cs_gpios)
> >                 return -ENOMEM;
> >
> >         for (i = 0; i < master->num_chipselect; i++)
> > -               cs[i] = -ENOENT;
> > +               master->cs_gpios[i] = -ENOENT;
> >
> >         for (i = 0; i < nb; i++)
> > -               cs[i] = of_get_named_gpio(np, "cs-gpios", i);
> > +               master->cs_gpios[i] =
> > +                       of_get_named_gpio_flags(np, "cs-gpios", i,
> > +                                               &master->cs_gpios_flags[i]);
> 
> I don't see you initializing master->cs_gpio_flags anywhere, so won't
> this cause a null/unitialized pointer access?

A few lines above. Memory is being allocated together with cs_gpios.

---

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Gerhard Sittig]

On Sun, Dec 01, 2013 at 11:59 +0400, Alexander Shiyan wrote:
> 
> This patch takes the CS active level from the GPIO bindings,
> so we remove the special property "spi-cs-high" for chipselects
> that use GPIO.

I'm afraid this description does not reflect what the patch does.
This is not saying that the patch is wrong, but suggests that the
description may be in need of an update.

Whether there is some gain in the change is a different matter,
see below.


> Signed-off-by: Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org>
> ---
>  drivers/spi/spi.c       | 28 ++++++++++++++++++----------
>  include/linux/spi/spi.h |  2 ++
>  2 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 98f4b77..d2ba1ec 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -413,8 +413,12 @@ int spi_add_device(struct spi_device *spi)
>  		goto done;
>  	}
>  
> -	if (master->cs_gpios)
> +	if (master->cs_gpios) {
>  		spi->cs_gpio = master->cs_gpios[spi->chip_select];
> +		if (!(master->cs_gpios_flags[spi->chip_select] &
> +		      OF_GPIO_ACTIVE_LOW))
> +			spi->mode |= SPI_CS_HIGH;
> +	}
>  
>  	/* Drivers may modify this initial i/o setup, but will
>  	 * normally rely on the device being setup.  Devices

OK, so you accept GPIO flags as an _additional_ source of
information, to derive "CS high" attributes in the SPI subsystem
in the (potential) absence of explicit specs in the device tree.

I'm not quite certain whether the GPIO pin's "not being inverted"
(not being low-active, the de-facto default for a GPIO pin) and
the SPI chip select's being high-active really are the same
thing.  Unless I'm missing something, this is quite a change in
semantics.  (Should this automatic deriving attributes emit a
message as well, to raise awareness?  Are we spamming users then,
or do these go unnoticed und ignored?)

Does this patch _force_ those who use GPIOs for SPI CS to adjust
their GPIO configuration in device trees?  Or make SPI
communication fail if the device tree won't get adjusted?  Should
you adjust users as well, and put a big fat remark into the
documentation?

A quick 'git grep spi-cs-high' reveals just one in-tree user, but
I'd expect quite a lot out-of-tree users which assume "regular,
straight semantics".


> @@ -1027,10 +1031,14 @@ static void of_register_spi_devices(struct spi_master *master)
>  			spi->mode |= SPI_CPHA;
>  		if (of_find_property(nc, "spi-cpol", NULL))
>  			spi->mode |= SPI_CPOL;
> -		if (of_find_property(nc, "spi-cs-high", NULL))
> -			spi->mode |= SPI_CS_HIGH;
>  		if (of_find_property(nc, "spi-3wire", NULL))
>  			spi->mode |= SPI_3WIRE;
> +		if (of_find_property(nc, "spi-cs-high", NULL)) {
> +			if (master->cs_gpios)
> +				dev_notice(&master->dev,
> +					"\"spi-cs-high\" property is obsolete.\n");
> +			spi->mode |= SPI_CS_HIGH;
> +		}
>  
>  		/* Device DUAL/QUAD mode */
>  		if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {

Here you keep the evaluation of the 'spi-cs-high' property.
Which is a good thing, and should not go away as Mark has
suggested, at least not without proper period of migration.

You introduce a diagnostic message if SPI CS is high-active (and
this spec comes from the SPI node) and the CS is backed by a GPIO
pin.

Considering that you cannot remove the "CS high-active" attribute
in general, and need to keep the 'spi-cs-high' property for those
cases where the signal is not driven by means of GPIO, and keep
backwards compatibility for GPIO backed signals -- is there
really any gain in the change?

I'm afraid that forcing users to specify the same "this SPI
slave's CS is high-active" information in different ways
depending on whether the signal is generated by GPIO or by other
means is not a good thing.  I'm even questioning the benefit of
introducing the optional second source of this information by
deriving it from the GPIO flags.  Am I missing something?

[ it's just a nit that hou move the 'spi-cs-high' evaluation in
the patch while I fail to see the motivation of the move, it's
neither logical grouping nor alphabetical sorting ]


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
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

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Alexander Shiyan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 4545 bytes --]




Понедельник,  2 декабря 2013, 15:59 +01:00 от Gerhard Sittig <gsi@denx.de>:
> On Sun, Dec 01, 2013 at 11:59 +0400, Alexander Shiyan wrote:
> > 
> > This patch takes the CS active level from the GPIO bindings,
> > so we remove the special property "spi-cs-high" for chipselects
> > that use GPIO.
> 
> I'm afraid this description does not reflect what the patch does.
> This is not saying that the patch is wrong, but suggests that the
> description may be in need of an update.
> 
> Whether there is some gain in the change is a different matter,
> see below.
> 
> 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
> >  drivers/spi/spi.c       | 28 ++++++++++++++++++----------
> >  include/linux/spi/spi.h |  2 ++
> >  2 files changed, 20 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 98f4b77..d2ba1ec 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -413,8 +413,12 @@ int spi_add_device(struct spi_device *spi)
> >  		goto done;
> >  	}
> >  
> > -	if (master->cs_gpios)
> > +	if (master->cs_gpios) {
> >  		spi->cs_gpio = master->cs_gpios[spi->chip_select];
> > +		if (!(master->cs_gpios_flags[spi->chip_select] &
> > +		      OF_GPIO_ACTIVE_LOW))
> > +			spi->mode |= SPI_CS_HIGH;
> > +	}
> >  
> >  	/* Drivers may modify this initial i/o setup, but will
> >  	 * normally rely on the device being setup.  Devices
> 
> OK, so you accept GPIO flags as an _additional_ source of
> information, to derive "CS high" attributes in the SPI subsystem
> in the (potential) absence of explicit specs in the device tree.
> 
> I'm not quite certain whether the GPIO pin's "not being inverted"
> (not being low-active, the de-facto default for a GPIO pin) and
> the SPI chip select's being high-active really are the same
> thing.  Unless I'm missing something, this is quite a change in
> semantics.  (Should this automatic deriving attributes emit a
> message as well, to raise awareness?  Are we spamming users then,
> or do these go unnoticed und ignored?)
> 
> Does this patch _force_ those who use GPIOs for SPI CS to adjust
> their GPIO configuration in device trees?  Or make SPI
> communication fail if the device tree won't get adjusted?  Should
> you adjust users as well, and put a big fat remark into the
> documentation?
> 
> A quick 'git grep spi-cs-high' reveals just one in-tree user, but
> I'd expect quite a lot out-of-tree users which assume "regular,
> straight semantics".
> 
> 
> > @@ -1027,10 +1031,14 @@ static void of_register_spi_devices(struct spi_master *master)
> >  			spi->mode |= SPI_CPHA;
> >  		if (of_find_property(nc, "spi-cpol", NULL))
> >  			spi->mode |= SPI_CPOL;
> > -		if (of_find_property(nc, "spi-cs-high", NULL))
> > -			spi->mode |= SPI_CS_HIGH;
> >  		if (of_find_property(nc, "spi-3wire", NULL))
> >  			spi->mode |= SPI_3WIRE;
> > +		if (of_find_property(nc, "spi-cs-high", NULL)) {
> > +			if (master->cs_gpios)
> > +				dev_notice(&master->dev,
> > +					"\"spi-cs-high\" property is obsolete.\n");
> > +			spi->mode |= SPI_CS_HIGH;
> > +		}
> >  
> >  		/* Device DUAL/QUAD mode */
> >  		if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
> 
> Here you keep the evaluation of the 'spi-cs-high' property.
> Which is a good thing, and should not go away as Mark has
> suggested, at least not without proper period of migration.
> 
> You introduce a diagnostic message if SPI CS is high-active (and
> this spec comes from the SPI node) and the CS is backed by a GPIO
> pin.
> 
> Considering that you cannot remove the "CS high-active" attribute
> in general, and need to keep the 'spi-cs-high' property for those
> cases where the signal is not driven by means of GPIO, and keep
> backwards compatibility for GPIO backed signals -- is there
> really any gain in the change?
> 
> I'm afraid that forcing users to specify the same "this SPI
> slave's CS is high-active" information in different ways
> depending on whether the signal is generated by GPIO or by other
> means is not a good thing.  I'm even questioning the benefit of
> introducing the optional second source of this information by
> deriving it from the GPIO flags.  Am I missing something?

Using GPIO flags is basically better. 
Imagine that tomorrow we may want the option to define open-drain
output on the CS pin. Specific pullup or pulldown.
Another separate properties?

---
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±²˜¢žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Gerhard Sittig]

On Mon, Dec 02, 2013 at 19:24 +0400, Alexander Shiyan wrote:
> 
> Using GPIO flags is basically better. 
> Imagine that tomorrow we may want the option to define open-drain
> output on the CS pin. Specific pullup or pulldown.
> Another separate properties?

This does not answer the actual concerns raised.  Do you change
the semantics without making users notice it?  Do you fragment
configuration by putting the same piece of information that is
specific to the SPI slave into totally different properties in
rather distant locations depending on how the CS signal gets
generated?  Shouldn't you update users when you change the
service?

Can you please address those concerns I raised, or clarify where
I'm wrong in my interpretation and why there is no issue?  A mere
"but it's better" is not convincing to me.  (Note that I trimmed
the above quotation because I feel that your response should be a
direct followup of the concerns raised, not buried here deep in
several response levels.)


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
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

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Alexander Shiyan]

Hello.

Понедельник,  2 декабря 2013, 17:31 +01:00 от Gerhard Sittig <gsi@denx.de>:
> On Mon, Dec 02, 2013 at 19:24 +0400, Alexander Shiyan wrote:
> > 
> > Using GPIO flags is basically better. 
> > Imagine that tomorrow we may want the option to define open-drain
> > output on the CS pin. Specific pullup or pulldown.
> > Another separate properties?
> 
> This does not answer the actual concerns raised.  Do you change
> the semantics without making users notice it?  Do you fragment
> configuration by putting the same piece of information that is
> specific to the SPI slave into totally different properties in
> rather distant locations depending on how the CS signal gets
> generated?  Shouldn't you update users when you change the
> service?
> 
> Can you please address those concerns I raised, or clarify where
> I'm wrong in my interpretation and why there is no issue?  A mere
> "but it's better" is not convincing to me.  (Note that I trimmed
> the above quotation because I feel that your response should be a
> direct followup of the concerns raised, not buried here deep in
> several response levels.)

SPI DT-bindings for GPIO CS and DTS users will be updated.
At this stage I present only basic concept of change.
Do not quite understand the subject of the dispute, but I hope
I answered correctly.
Thanks.

---

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Jonas Gorski]

On Mon, Dec 2, 2013 at 3:54 PM, Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org> wrote:
>> On Sun, Dec 1, 2013 at 8:59 AM, Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org> wrote:
>> > This patch takes the CS active level from the GPIO bindings,
>> > so we remove the special property "spi-cs-high" for chipselects
>> > that use GPIO.
> ...
>
>> > -       cs = devm_kzalloc(&master->dev,
>> > -                         sizeof(int) * master->num_chipselect,
>> > -                         GFP_KERNEL);
>> > -       master->cs_gpios = cs;
>> > +       master->cs_gpios = devm_kzalloc(&master->dev, master->num_chipselect *
>> > +               (sizeof(int) + sizeof(enum of_gpio_flags)), GFP_KERNEL);
>> >
>> >         if (!master->cs_gpios)
>> >                 return -ENOMEM;
>> >
>> >         for (i = 0; i < master->num_chipselect; i++)
>> > -               cs[i] = -ENOENT;
>> > +               master->cs_gpios[i] = -ENOENT;
>> >
>> >         for (i = 0; i < nb; i++)
>> > -               cs[i] = of_get_named_gpio(np, "cs-gpios", i);
>> > +               master->cs_gpios[i] =
>> > +                       of_get_named_gpio_flags(np, "cs-gpios", i,
>> > +                                               &master->cs_gpios_flags[i]);
>>
>> I don't see you initializing master->cs_gpio_flags anywhere, so won't
>> this cause a null/unitialized pointer access?
>
> A few lines above. Memory is being allocated together with cs_gpios.

I see the allocation, but I don't see the assignment of
master->cs_gpios_flags. Only master->cs_gpios gets assigned.


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

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Gerhard Sittig]

On Mon, Dec 02, 2013 at 19:03 +0100, Jonas Gorski wrote:
> 
> On Mon, Dec 2, 2013 at 3:54 PM, Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org> wrote:
> >> On Sun, Dec 1, 2013 at 8:59 AM, Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org> wrote:
> >> > This patch takes the CS active level from the GPIO bindings,
> >> > so we remove the special property "spi-cs-high" for chipselects
> >> > that use GPIO.
> > ...
> >
> >> > -       cs = devm_kzalloc(&master->dev,
> >> > -                         sizeof(int) * master->num_chipselect,
> >> > -                         GFP_KERNEL);
> >> > -       master->cs_gpios = cs;
> >> > +       master->cs_gpios = devm_kzalloc(&master->dev, master->num_chipselect *
> >> > +               (sizeof(int) + sizeof(enum of_gpio_flags)), GFP_KERNEL);
> >> >
> >> >         if (!master->cs_gpios)
> >> >                 return -ENOMEM;
> >> >
> >> >         for (i = 0; i < master->num_chipselect; i++)
> >> > -               cs[i] = -ENOENT;
> >> > +               master->cs_gpios[i] = -ENOENT;
> >> >
> >> >         for (i = 0; i < nb; i++)
> >> > -               cs[i] = of_get_named_gpio(np, "cs-gpios", i);
> >> > +               master->cs_gpios[i] =
> >> > +                       of_get_named_gpio_flags(np, "cs-gpios", i,
> >> > +                                               &master->cs_gpios_flags[i]);
> >>
> >> I don't see you initializing master->cs_gpio_flags anywhere, so won't
> >> this cause a null/unitialized pointer access?
> >
> > A few lines above. Memory is being allocated together with cs_gpios.
> 
> I see the allocation, but I don't see the assignment of
> master->cs_gpios_flags. Only master->cs_gpios gets assigned.

It's a side effect of the of_get_named_gpio_flags() call (passing
the variable by reference).

Don't worry, I felt the same when I read the patch.  Took me a
while to spot that everything was still in place. :)


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
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

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Jonas Gorski]

On Mon, Dec 2, 2013 at 9:17 PM, Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org> wrote:
> On Mon, Dec 02, 2013 at 19:03 +0100, Jonas Gorski wrote:
>>
>> On Mon, Dec 2, 2013 at 3:54 PM, Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org> wrote:
>> >> On Sun, Dec 1, 2013 at 8:59 AM, Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org> wrote:
>> >> > This patch takes the CS active level from the GPIO bindings,
>> >> > so we remove the special property "spi-cs-high" for chipselects
>> >> > that use GPIO.
>> > ...
>> >
>> >> > -       cs = devm_kzalloc(&master->dev,
>> >> > -                         sizeof(int) * master->num_chipselect,
>> >> > -                         GFP_KERNEL);
>> >> > -       master->cs_gpios = cs;
>> >> > +       master->cs_gpios = devm_kzalloc(&master->dev, master->num_chipselect *
>> >> > +               (sizeof(int) + sizeof(enum of_gpio_flags)), GFP_KERNEL);
>> >> >
>> >> >         if (!master->cs_gpios)
>> >> >                 return -ENOMEM;
>> >> >
>> >> >         for (i = 0; i < master->num_chipselect; i++)
>> >> > -               cs[i] = -ENOENT;
>> >> > +               master->cs_gpios[i] = -ENOENT;
>> >> >
>> >> >         for (i = 0; i < nb; i++)
>> >> > -               cs[i] = of_get_named_gpio(np, "cs-gpios", i);
>> >> > +               master->cs_gpios[i] =
>> >> > +                       of_get_named_gpio_flags(np, "cs-gpios", i,
>> >> > +                                               &master->cs_gpios_flags[i]);
>> >>
>> >> I don't see you initializing master->cs_gpio_flags anywhere, so won't
>> >> this cause a null/unitialized pointer access?
>> >
>> > A few lines above. Memory is being allocated together with cs_gpios.
>>
>> I see the allocation, but I don't see the assignment of
>> master->cs_gpios_flags. Only master->cs_gpios gets assigned.
>
> It's a side effect of the of_get_named_gpio_flags() call (passing
> the variable by reference).
>
> Don't worry, I felt the same when I read the patch.  Took me a
> while to spot that everything was still in place. :)

I still don't see it. The patch adds an additional pointer member:

> struct spi_master {
>        (...)
>        /* gpio chip select */
>        int                     *cs_gpios;
> +       /* GPIO flags */
> +       enum of_gpio_flags      *cs_gpios_flags;
 };

Then it changes the memory allocation to allocate additional memory for it:

> -       cs = devm_kzalloc(&master->dev,
> -                         sizeof(int) * master->num_chipselect,
> -                         GFP_KERNEL);
> -       master->cs_gpios = cs;
> +       master->cs_gpios = devm_kzalloc(&master->dev, master->num_chipselect *
> +               (sizeof(int) + sizeof(enum of_gpio_flags)), GFP_KERNEL);

But it only assigns the memory pointer to master->cs_gpios, but
master->cs_gpios_flags will still point to whatever it was initialized
to (likely 0), and is never actually set.

Then

> -               cs[i] = of_get_named_gpio(np, "cs-gpios", i);
> +               master->cs_gpios[i] =
> +                       of_get_named_gpio_flags(np, "cs-gpios", i,
> +                                               &master->cs_gpios_flags[i]);

will pass the address of the i-th element of the "array" pointed to by
master->cs_gpio_flags, so basically 0 + (i * sizeof(enum
of_gpio_flags), and not the intended value of (master->cs_gpios +
master->num_chipselect * sizeof(int)) + (i * sizeof(enum
of_gpio_flags).

Unless there is some serious black magic in of_get_named_gpio_flags(),
but I doubt that would be allowed in the kernel ;)

So as far as I can tell there's missing something along

master->cs_gpios_flags = (enum of_gpio_flags
*)&master->cs_gpios[master->num_chipselect];


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

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Gerhard Sittig]

On Mon, Dec 02, 2013 at 23:09 +0100, Jonas Gorski wrote:
> 
> On Mon, Dec 2, 2013 at 9:17 PM, Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org> wrote:
> > On Mon, Dec 02, 2013 at 19:03 +0100, Jonas Gorski wrote:
> >>
> >> I see the allocation, but I don't see the assignment of
> >> master->cs_gpios_flags. Only master->cs_gpios gets assigned.
> >
> > It's a side effect of the of_get_named_gpio_flags() call (passing
> > the variable by reference).
> >
> > Don't worry, I felt the same when I read the patch.  Took me a
> > while to spot that everything was still in place. :)
> 
> I still don't see it. The patch adds an additional pointer member:
> 
> > struct spi_master {
> >        (...)
> >        /* gpio chip select */
> >        int                     *cs_gpios;
> > +       /* GPIO flags */
> > +       enum of_gpio_flags      *cs_gpios_flags;
>  };
> 
> Then it changes the memory allocation to allocate additional memory for it:
> 
> > -       cs = devm_kzalloc(&master->dev,
> > -                         sizeof(int) * master->num_chipselect,
> > -                         GFP_KERNEL);
> > -       master->cs_gpios = cs;
> > +       master->cs_gpios = devm_kzalloc(&master->dev, master->num_chipselect *
> > +               (sizeof(int) + sizeof(enum of_gpio_flags)), GFP_KERNEL);
> 
> But it only assigns the memory pointer to master->cs_gpios, but
> master->cs_gpios_flags will still point to whatever it was initialized
> to (likely 0), and is never actually set.

You are right, I misread the code.

The assignment of "&cs_gpios[master->num_chipselect]" to the
cs_gpio_flags "start address" is missing.  Which involves dirty
casts that may be considered ugly.

Alternatively the two arrays could get allocated separately
(don't know what the overhead would be, and if that is considered
bad practise or wasteful although it's cleaner).


> Unless there is some serious black magic in of_get_named_gpio_flags(),
> but I doubt that would be allowed in the kernel ;)
> 
> So as far as I can tell there's missing something along
> 
> master->cs_gpios_flags = (enum of_gpio_flags *)&master->cs_gpios[master->num_chipselect];

Right!  This kind of assignment is "the standin" for the black
magic that is not expected here. :)


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
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

Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Alexander Shiyan]

> > On Mon, Dec 2, 2013 at 9:17 PM, Gerhard Sittig <gsi@denx.de> wrote:
> > > On Mon, Dec 02, 2013 at 19:03 +0100, Jonas Gorski wrote:
> > >>
> > >> I see the allocation, but I don't see the assignment of
> > >> master->cs_gpios_flags. Only master->cs_gpios gets assigned.
> > >
> > > It's a side effect of the of_get_named_gpio_flags() call (passing
> > > the variable by reference).
> > >
> > > Don't worry, I felt the same when I read the patch.  Took me a
> > > while to spot that everything was still in place. :)
> > 
> > I still don't see it. The patch adds an additional pointer member:
> > 
> > > struct spi_master {
> > >        (...)
> > >        /* gpio chip select */
> > >        int                     *cs_gpios;
> > > +       /* GPIO flags */
> > > +       enum of_gpio_flags      *cs_gpios_flags;
> >  };
> > 
> > Then it changes the memory allocation to allocate additional memory for it:
> > 
> > > -       cs = devm_kzalloc(&master->dev,
> > > -                         sizeof(int) * master->num_chipselect,
> > > -                         GFP_KERNEL);
> > > -       master->cs_gpios = cs;
> > > +       master->cs_gpios = devm_kzalloc(&master->dev, master->num_chipselect *
> > > +               (sizeof(int) + sizeof(enum of_gpio_flags)), GFP_KERNEL);
> > 
> > But it only assigns the memory pointer to master->cs_gpios, but
> > master->cs_gpios_flags will still point to whatever it was initialized
> > to (likely 0), and is never actually set.
> 
> You are right, I misread the code.
> 
> The assignment of "&cs_gpios[master->num_chipselect]" to the
> cs_gpio_flags "start address" is missing.  Which involves dirty
> casts that may be considered ugly.
> 
> Alternatively the two arrays could get allocated separately
> (don't know what the overhead would be, and if that is considered
> bad practise or wasteful although it's cleaner).
> 
> 
> > Unless there is some serious black magic in of_get_named_gpio_flags(),
> > but I doubt that would be allowed in the kernel ;)
> > 
> > So as far as I can tell there's missing something along
> > 
> > master->cs_gpios_flags = (enum of_gpio_flags *)&master->cs_gpios[master->num_chipselect];
> 
> Right!  This kind of assignment is "the standin" for the black
> magic that is not expected here. :)

Ahh, I see. The "cs_gpio_flags" member do not know anything about "cs_gpios" size.
Thanks.

---