[U-Boot] [PATCH v2 1/4] dm: spi: Return 0 if driver does not implement ops->cs_info

[U-Boot] [PATCH v2 1/4] dm: spi: Return 0 if driver does not implement ops->cs_info

[Bin Meng]

If an SPI controller driver does not implement ops->cs_info, that
probably means any chip select number could be valid, hence let's
return 0 for spi_cs_info().

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>

---

Changes in v2:
- update spi-howto.rst to reflect the code changes

 doc/driver-model/spi-howto.rst | 4 ++--
 drivers/spi/spi-uclass.c       | 7 +++----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/doc/driver-model/spi-howto.rst b/doc/driver-model/spi-howto.rst
index a538fdc..7e64fae 100644
--- a/doc/driver-model/spi-howto.rst
+++ b/doc/driver-model/spi-howto.rst
@@ -634,8 +634,8 @@ method for cs_info() to deal with this. If you don't provide it, then the
 device tree will be used to determine what chip selects are valid.
 
 Return -ENODEV if the supplied chip select is invalid, or 0 if it is valid.
-If you don't provide the cs_info() method, -ENODEV is assumed for all
-chip selects that do not appear in the device tree.
+If you don't provide the cs_info() method, 0 is assumed for all chip selects
+that do not appear in the device tree.
 
 
 Test it
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index 88cb2a1..24de0b5 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -237,11 +237,10 @@ int spi_cs_info(struct udevice *bus, uint cs, struct spi_cs_info *info)
 		return ops->cs_info(bus, cs, info);
 
 	/*
-	 * We could assume there is at least one valid chip select, but best
-	 * to be sure and return an error in this case. The driver didn't
-	 * care enough to tell us.
+	 * We could assume there is at least one valid chip select.
+	 * The driver didn't care enough to tell us.
 	 */
-	return -ENODEV;
+	return 0;
 }
 
 int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp,
-- 
2.7.4

[U-Boot] [PATCH v2 2/4] dm: spi: Change cs_info op to return -EINVAL for invalid cs num

[Bin Meng]

We need distinguish the following two situations in various SPI APIs:

- given chip select num is invalid
- given chip select num is valid, but no device is attached

Currently -ENODEV is returned for both cases.

For the first case, it's more reasonable to return -EINVAL instead of
-ENODEV for invalid chip select numbers.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- new patch to change cs_info op to return -EINVAL for invalid cs num

 doc/driver-model/spi-howto.rst | 4 ++--
 drivers/spi/ath79_spi.c        | 2 +-
 drivers/spi/bcm63xx_hsspi.c    | 2 +-
 drivers/spi/bcm63xx_spi.c      | 2 +-
 drivers/spi/sandbox_spi.c      | 2 +-
 drivers/spi/tegra20_sflash.c   | 2 +-
 include/spi.h                  | 2 +-
 7 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/doc/driver-model/spi-howto.rst b/doc/driver-model/spi-howto.rst
index 7e64fae..451dc08 100644
--- a/doc/driver-model/spi-howto.rst
+++ b/doc/driver-model/spi-howto.rst
@@ -116,7 +116,7 @@ Put this code at the bottom of your existing driver file:
 	static int exynos_cs_info(struct udevice *bus, uint cs,
 				  struct spi_cs_info *info)
 	{
-		return -ENODEV;
+		return -EINVAL;
 	}
 
 	static const struct dm_spi_ops exynos_spi_ops = {
@@ -633,7 +633,7 @@ is not obvious from outside the driver. In this case you can provide a
 method for cs_info() to deal with this. If you don't provide it, then the
 device tree will be used to determine what chip selects are valid.
 
-Return -ENODEV if the supplied chip select is invalid, or 0 if it is valid.
+Return -EINVAL if the supplied chip select is invalid, or 0 if it is valid.
 If you don't provide the cs_info() method, 0 is assumed for all chip selects
 that do not appear in the device tree.
 
diff --git a/drivers/spi/ath79_spi.c b/drivers/spi/ath79_spi.c
index 4fd3c05..2070692 100644
--- a/drivers/spi/ath79_spi.c
+++ b/drivers/spi/ath79_spi.c
@@ -198,7 +198,7 @@ static int ath79_cs_info(struct udevice *bus, uint cs,
 {
 	/* Always allow activity on CS 0/1/2 */
 	if (cs >= 3)
-		return -ENODEV;
+		return -EINVAL;
 
 	return 0;
 }
diff --git a/drivers/spi/bcm63xx_hsspi.c b/drivers/spi/bcm63xx_hsspi.c
index 4f527fa7..f1e246e 100644
--- a/drivers/spi/bcm63xx_hsspi.c
+++ b/drivers/spi/bcm63xx_hsspi.c
@@ -108,7 +108,7 @@ static int bcm63xx_hsspi_cs_info(struct udevice *bus, uint cs,
 
 	if (cs >= priv->num_cs) {
 		printf("no cs %u\n", cs);
-		return -ENODEV;
+		return -EINVAL;
 	}
 
 	return 0;
diff --git a/drivers/spi/bcm63xx_spi.c b/drivers/spi/bcm63xx_spi.c
index 4d19e03..69f88c9 100644
--- a/drivers/spi/bcm63xx_spi.c
+++ b/drivers/spi/bcm63xx_spi.c
@@ -130,7 +130,7 @@ static int bcm63xx_spi_cs_info(struct udevice *bus, uint cs,
 
 	if (cs >= priv->num_cs) {
 		printf("no cs %u\n", cs);
-		return -ENODEV;
+		return -EINVAL;
 	}
 
 	return 0;
diff --git a/drivers/spi/sandbox_spi.c b/drivers/spi/sandbox_spi.c
index 906401e..16473ec 100644
--- a/drivers/spi/sandbox_spi.c
+++ b/drivers/spi/sandbox_spi.c
@@ -117,7 +117,7 @@ static int sandbox_cs_info(struct udevice *bus, uint cs,
 {
 	/* Always allow activity on CS 0 */
 	if (cs >= 1)
-		return -ENODEV;
+		return -EINVAL;
 
 	return 0;
 }
diff --git a/drivers/spi/tegra20_sflash.c b/drivers/spi/tegra20_sflash.c
index a54b10f..567e33f 100644
--- a/drivers/spi/tegra20_sflash.c
+++ b/drivers/spi/tegra20_sflash.c
@@ -78,7 +78,7 @@ int tegra20_sflash_cs_info(struct udevice *bus, unsigned int cs,
 {
 	/* Tegra20 SPI-Flash - only 1 device ('bus/cs') */
 	if (cs != 0)
-		return -ENODEV;
+		return -EINVAL;
 	else
 		return 0;
 }
diff --git a/include/spi.h b/include/spi.h
index 3785941..cc344de 100644
--- a/include/spi.h
+++ b/include/spi.h
@@ -438,7 +438,7 @@ struct dm_spi_ops {
 	 * @cs:		The chip select (0..n-1)
 	 * @info:	Returns information about the chip select, if valid.
 	 *		On entry info->dev is NULL
-	 * @return 0 if OK (and @info is set up), -ENODEV if the chip select
+	 * @return 0 if OK (and @info is set up), -EINVAL if the chip select
 	 *	   is invalid, other -ve value on error
 	 */
 	int (*cs_info)(struct udevice *bus, uint cs, struct spi_cs_info *info);
-- 
2.7.4

[U-Boot] [PATCH v2 3/4] dm: spi: Check cs number before accessing slaves

[Bin Meng]

Add chip select number check in spi_find_chip_select().

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- move the chip select number check to spi_find_chip_select()

 drivers/spi/spi-uclass.c | 45 ++++++++++++++++++++++++++-------------------
 include/spi.h            |  3 ++-
 2 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index 24de0b5..cdeceb5 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -179,7 +179,32 @@ int spi_chip_select(struct udevice *dev)
 
 int spi_find_chip_select(struct udevice *bus, int cs, struct udevice **devp)
 {
+	struct dm_spi_ops *ops;
+	struct spi_cs_info info;
 	struct udevice *dev;
+	int ret;
+
+	/*
+	 * Ask the driver. For the moment we don't have CS info.
+	 * When we do we could provide the driver with a helper function
+	 * to figure out what chip selects are valid, or just handle the
+	 * request.
+	 */
+	ops = spi_get_ops(bus);
+	if (ops->cs_info) {
+		ret = ops->cs_info(bus, cs, &info);
+	} else {
+		/*
+		 * We could assume there is at least one valid chip select.
+		 * The driver didn't care enough to tell us.
+		 */
+		ret = 0;
+	}
+
+	if (ret) {
+		printf("Invalid cs %d (err=%d)\n", cs, ret);
+		return ret;
+	}
 
 	for (device_find_first_child(bus, &dev); dev;
 	     device_find_next_child(&dev)) {
@@ -214,7 +239,6 @@ int spi_cs_is_valid(unsigned int busnum, unsigned int cs)
 int spi_cs_info(struct udevice *bus, uint cs, struct spi_cs_info *info)
 {
 	struct spi_cs_info local_info;
-	struct dm_spi_ops *ops;
 	int ret;
 
 	if (!info)
@@ -223,24 +247,7 @@ int spi_cs_info(struct udevice *bus, uint cs, struct spi_cs_info *info)
 	/* If there is a device attached, return it */
 	info->dev = NULL;
 	ret = spi_find_chip_select(bus, cs, &info->dev);
-	if (!ret)
-		return 0;
-
-	/*
-	 * Otherwise ask the driver. For the moment we don't have CS info.
-	 * When we do we could provide the driver with a helper function
-	 * to figure out what chip selects are valid, or just handle the
-	 * request.
-	 */
-	ops = spi_get_ops(bus);
-	if (ops->cs_info)
-		return ops->cs_info(bus, cs, info);
-
-	/*
-	 * We could assume there is at least one valid chip select.
-	 * The driver didn't care enough to tell us.
-	 */
-	return 0;
+	return ret == -ENODEV ? 0 : ret;
 }
 
 int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp,
diff --git a/include/spi.h b/include/spi.h
index cc344de..6b144bc 100644
--- a/include/spi.h
+++ b/include/spi.h
@@ -528,7 +528,8 @@ int spi_chip_select(struct udevice *slave);
  * @bus:	SPI bus to search
  * @cs:		Chip select to look for
  * @devp:	Returns the slave device if found
- * @return 0 if found, -ENODEV on error
+ * @return 0 if found, -EINVAL if cs is invalid, -ENODEV if no device attached,
+ *	   other -ve value on error
  */
 int spi_find_chip_select(struct udevice *bus, int cs, struct udevice **devp);
 
-- 
2.7.4

[U-Boot] [PATCH v2 4/4] test: dm: spi: Fix sandbox dm_test_spi_find()

[Bin Meng]

Per sandbox_cs_info(), sandbox spi controller only supports chip
select 0. Current test case tries to locate devices on chip select
1, and any call to spi_get_bus_and_cs() or spi_cs_info() with cs
number 1 should not return 0.

This updates the test case to handle it correctly.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- new patch to fix sandbox dm_test_spi_find()

 test/dm/spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/test/dm/spi.c b/test/dm/spi.c
index ffd789c..8e417ac 100644
--- a/test/dm/spi.c
+++ b/test/dm/spi.c
@@ -77,10 +77,10 @@ static int dm_test_spi_find(struct unit_test_state *uts)
 	/* We should be able to add something to another chip select */
 	ut_assertok(sandbox_sf_bind_emul(state, busnum, cs_b, bus, node,
 					 "name"));
-	ut_assertok(spi_get_bus_and_cs(busnum, cs_b, speed, mode,
+	ut_asserteq(-EINVAL, spi_get_bus_and_cs(busnum, cs_b, speed, mode,
 				       "spi_flash_std", "name", &bus, &slave));
-	ut_assertok(spi_cs_info(bus, cs_b, &info));
-	ut_asserteq_ptr(info.dev, slave->dev);
+	ut_asserteq(-EINVAL, spi_cs_info(bus, cs_b, &info));
+	ut_asserteq_ptr(NULL, info.dev);
 
 	/*
 	 * Since we are about to destroy all devices, we must tell sandbox
-- 
2.7.4

[U-Boot] [PATCH v2 1/4] dm: spi: Return 0 if driver does not implement ops->cs_info

[Bin Meng]

Hi Jagan,

On Mon, Sep 9, 2019 at 9:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> If an SPI controller driver does not implement ops->cs_info, that
> probably means any chip select number could be valid, hence let's
> return 0 for spi_cs_info().
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
>
> ---
>
> Changes in v2:
> - update spi-howto.rst to reflect the code changes
>
>  doc/driver-model/spi-howto.rst | 4 ++--
>  drivers/spi/spi-uclass.c       | 7 +++----
>  2 files changed, 5 insertions(+), 6 deletions(-)
>

Ping for this series?

Regards,
Bin

[U-Boot] [PATCH v2 1/4] dm: spi: Return 0 if driver does not implement ops->cs_info

[Bin Meng]

On Sun, Sep 29, 2019 at 4:04 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Jagan,
>
> On Mon, Sep 9, 2019 at 9:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > If an SPI controller driver does not implement ops->cs_info, that
> > probably means any chip select number could be valid, hence let's
> > return 0 for spi_cs_info().
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> >
> > ---
> >
> > Changes in v2:
> > - update spi-howto.rst to reflect the code changes
> >
> >  doc/driver-model/spi-howto.rst | 4 ++--
> >  drivers/spi/spi-uclass.c       | 7 +++----
> >  2 files changed, 5 insertions(+), 6 deletions(-)
> >
>
> Ping for this series?
>

Another ping?

[U-Boot] [PATCH v2 1/4] dm: spi: Return 0 if driver does not implement ops->cs_info

[Bin Meng]

On Tue, Oct 8, 2019 at 8:59 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sun, Sep 29, 2019 at 4:04 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Jagan,
> >
> > On Mon, Sep 9, 2019 at 9:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > If an SPI controller driver does not implement ops->cs_info, that
> > > probably means any chip select number could be valid, hence let's
> > > return 0 for spi_cs_info().
> > >
> > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - update spi-howto.rst to reflect the code changes
> > >
> > >  doc/driver-model/spi-howto.rst | 4 ++--
> > >  drivers/spi/spi-uclass.c       | 7 +++----
> > >  2 files changed, 5 insertions(+), 6 deletions(-)
> > >
> >
> > Ping for this series?
> >
>
> Another ping?

Ping!

[U-Boot] [PATCH v2 1/4] dm: spi: Return 0 if driver does not implement ops->cs_info

[Jagan Teki]

On Sun, Sep 29, 2019 at 1:34 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Jagan,
>
> On Mon, Sep 9, 2019 at 9:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > If an SPI controller driver does not implement ops->cs_info, that
> > probably means any chip select number could be valid, hence let's
> > return 0 for spi_cs_info().
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> >
> > ---
> >
> > Changes in v2:
> > - update spi-howto.rst to reflect the code changes
> >
> >  doc/driver-model/spi-howto.rst | 4 ++--
> >  drivers/spi/spi-uclass.c       | 7 +++----
> >  2 files changed, 5 insertions(+), 6 deletions(-)
> >
>
> Ping for this series?

Sorry, been in vacations. will try to give an update on this.

[U-Boot] [PATCH v2 1/4] dm: spi: Return 0 if driver does not implement ops->cs_info

[Jagan Teki]

On Mon, Sep 9, 2019 at 6:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> If an SPI controller driver does not implement ops->cs_info, that
> probably means any chip select number could be valid, hence let's
> return 0 for spi_cs_info().
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
>
> ---

Applied to u-boot-spi/master

[U-Boot] [PATCH v2 3/4] dm: spi: Check cs number before accessing slaves

[Jagan Teki]

On Mon, Sep 9, 2019 at 6:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Add chip select number check in spi_find_chip_select().

Since we discussed the cs check to common call in
spi_find_chip_select. Would you please add some more commit
description.


>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---

Tested-by: Jagan Teki <jagan@amarulasolutions.com> # SoPine

[U-Boot] [PATCH v2 4/4] test: dm: spi: Fix sandbox dm_test_spi_find()

[Jagan Teki]

On Mon, Sep 9, 2019 at 6:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Per sandbox_cs_info(), sandbox spi controller only supports chip
> select 0. Current test case tries to locate devices on chip select
> 1, and any call to spi_get_bus_and_cs() or spi_cs_info() with cs
> number 1 should not return 0.
>
> This updates the test case to handle it correctly.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
> - new patch to fix sandbox dm_test_spi_find()
>
>  test/dm/spi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/test/dm/spi.c b/test/dm/spi.c
> index ffd789c..8e417ac 100644
> --- a/test/dm/spi.c
> +++ b/test/dm/spi.c
> @@ -77,10 +77,10 @@ static int dm_test_spi_find(struct unit_test_state *uts)
>         /* We should be able to add something to another chip select */
>         ut_assertok(sandbox_sf_bind_emul(state, busnum, cs_b, bus, node,
>                                          "name"));
> -       ut_assertok(spi_get_bus_and_cs(busnum, cs_b, speed, mode,
> +       ut_asserteq(-EINVAL, spi_get_bus_and_cs(busnum, cs_b, speed, mode,
>                                        "spi_flash_std", "name", &bus, &slave));
> -       ut_assertok(spi_cs_info(bus, cs_b, &info));
> -       ut_asserteq_ptr(info.dev, slave->dev);
> +       ut_asserteq(-EINVAL, spi_cs_info(bus, cs_b, &info));
> +       ut_asserteq_ptr(NULL, info.dev);

I assume you ran the sandbox test for this, if yes

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>

[U-Boot] [PATCH v2 3/4] dm: spi: Check cs number before accessing slaves

[Jagan Teki]

Hi Bin,

On Mon, Sep 9, 2019 at 6:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Add chip select number check in spi_find_chip_select().
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
> - move the chip select number check to spi_find_chip_select()
>
>  drivers/spi/spi-uclass.c | 45 ++++++++++++++++++++++++++-------------------
>  include/spi.h            |  3 ++-
>  2 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> index 24de0b5..cdeceb5 100644
> --- a/drivers/spi/spi-uclass.c
> +++ b/drivers/spi/spi-uclass.c
> @@ -179,7 +179,32 @@ int spi_chip_select(struct udevice *dev)
>
>  int spi_find_chip_select(struct udevice *bus, int cs, struct udevice **devp)
>  {
> +       struct dm_spi_ops *ops;
> +       struct spi_cs_info info;
>         struct udevice *dev;
> +       int ret;
> +
> +       /*
> +        * Ask the driver. For the moment we don't have CS info.
> +        * When we do we could provide the driver with a helper function
> +        * to figure out what chip selects are valid, or just handle the
> +        * request.
> +        */
> +       ops = spi_get_ops(bus);
> +       if (ops->cs_info) {
> +               ret = ops->cs_info(bus, cs, &info);
> +       } else {
> +               /*
> +                * We could assume there is at least one valid chip select.
> +                * The driver didn't care enough to tell us.
> +                */
> +               ret = 0;
> +       }
> +
> +       if (ret) {
> +               printf("Invalid cs %d (err=%d)\n", cs, ret);
> +               return ret;
> +       }
>

This is breaking 'sf probe' with associated bus and cs on SiFive.

[U-Boot] [PATCH v2 3/4] dm: spi: Check cs number before accessing slaves

[Bin Meng]

Hi Jagan,

On Wed, Oct 16, 2019 at 11:21 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> Hi Bin,
>
> On Mon, Sep 9, 2019 at 6:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Add chip select number check in spi_find_chip_select().
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >
> > ---
> >
> > Changes in v2:
> > - move the chip select number check to spi_find_chip_select()
> >
> >  drivers/spi/spi-uclass.c | 45 ++++++++++++++++++++++++++-------------------
> >  include/spi.h            |  3 ++-
> >  2 files changed, 28 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> > index 24de0b5..cdeceb5 100644
> > --- a/drivers/spi/spi-uclass.c
> > +++ b/drivers/spi/spi-uclass.c
> > @@ -179,7 +179,32 @@ int spi_chip_select(struct udevice *dev)
> >
> >  int spi_find_chip_select(struct udevice *bus, int cs, struct udevice **devp)
> >  {
> > +       struct dm_spi_ops *ops;
> > +       struct spi_cs_info info;
> >         struct udevice *dev;
> > +       int ret;
> > +
> > +       /*
> > +        * Ask the driver. For the moment we don't have CS info.
> > +        * When we do we could provide the driver with a helper function
> > +        * to figure out what chip selects are valid, or just handle the
> > +        * request.
> > +        */
> > +       ops = spi_get_ops(bus);
> > +       if (ops->cs_info) {
> > +               ret = ops->cs_info(bus, cs, &info);
> > +       } else {
> > +               /*
> > +                * We could assume there is at least one valid chip select.
> > +                * The driver didn't care enough to tell us.
> > +                */
> > +               ret = 0;
> > +       }
> > +
> > +       if (ret) {
> > +               printf("Invalid cs %d (err=%d)\n", cs, ret);
> > +               return ret;
> > +       }
> >
>
> This is breaking 'sf probe' with associated bus and cs on SiFive.

No this does not break anything on SiFive. It enforces the 'sf probe'
to use the correct CS number on SiFive. And something is wrong when CS
is 0 on SiFive.

Regards,
Bin

[U-Boot] [PATCH v2 3/4] dm: spi: Check cs number before accessing slaves

[Bin Meng]

Hi Jagan,

On Tue, Oct 29, 2019 at 6:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Jagan,
>
> On Wed, Oct 16, 2019 at 11:21 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > Hi Bin,
> >
> > On Mon, Sep 9, 2019 at 6:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Add chip select number check in spi_find_chip_select().
> > >
> > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - move the chip select number check to spi_find_chip_select()
> > >
> > >  drivers/spi/spi-uclass.c | 45 ++++++++++++++++++++++++++-------------------
> > >  include/spi.h            |  3 ++-
> > >  2 files changed, 28 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> > > index 24de0b5..cdeceb5 100644
> > > --- a/drivers/spi/spi-uclass.c
> > > +++ b/drivers/spi/spi-uclass.c
> > > @@ -179,7 +179,32 @@ int spi_chip_select(struct udevice *dev)
> > >
> > >  int spi_find_chip_select(struct udevice *bus, int cs, struct udevice **devp)
> > >  {
> > > +       struct dm_spi_ops *ops;
> > > +       struct spi_cs_info info;
> > >         struct udevice *dev;
> > > +       int ret;
> > > +
> > > +       /*
> > > +        * Ask the driver. For the moment we don't have CS info.
> > > +        * When we do we could provide the driver with a helper function
> > > +        * to figure out what chip selects are valid, or just handle the
> > > +        * request.
> > > +        */
> > > +       ops = spi_get_ops(bus);
> > > +       if (ops->cs_info) {
> > > +               ret = ops->cs_info(bus, cs, &info);
> > > +       } else {
> > > +               /*
> > > +                * We could assume there is at least one valid chip select.
> > > +                * The driver didn't care enough to tell us.
> > > +                */
> > > +               ret = 0;
> > > +       }
> > > +
> > > +       if (ret) {
> > > +               printf("Invalid cs %d (err=%d)\n", cs, ret);
> > > +               return ret;
> > > +       }
> > >
> >
> > This is breaking 'sf probe' with associated bus and cs on SiFive.
>
> No this does not break anything on SiFive. It enforces the 'sf probe'
> to use the correct CS number on SiFive. And something is wrong when CS
> is 0 on SiFive.

As discussed in another thread, this patch does not break SiFive.
Could you please merge the remaining 2 patches in this series? thanks!

Regards,
Bin

[PATCH v2 3/4] dm: spi: Check cs number before accessing slaves

[Bin Meng]

Hi Jagan,

On Mon, Nov 18, 2019 at 3:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Jagan,
>
> On Tue, Oct 29, 2019 at 6:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Jagan,
> >
> > On Wed, Oct 16, 2019 at 11:21 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Mon, Sep 9, 2019 at 6:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Add chip select number check in spi_find_chip_select().
> > > >
> > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > > >
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - move the chip select number check to spi_find_chip_select()
> > > >
> > > >  drivers/spi/spi-uclass.c | 45 ++++++++++++++++++++++++++-------------------
> > > >  include/spi.h            |  3 ++-
> > > >  2 files changed, 28 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> > > > index 24de0b5..cdeceb5 100644
> > > > --- a/drivers/spi/spi-uclass.c
> > > > +++ b/drivers/spi/spi-uclass.c
> > > > @@ -179,7 +179,32 @@ int spi_chip_select(struct udevice *dev)
> > > >
> > > >  int spi_find_chip_select(struct udevice *bus, int cs, struct udevice **devp)
> > > >  {
> > > > +       struct dm_spi_ops *ops;
> > > > +       struct spi_cs_info info;
> > > >         struct udevice *dev;
> > > > +       int ret;
> > > > +
> > > > +       /*
> > > > +        * Ask the driver. For the moment we don't have CS info.
> > > > +        * When we do we could provide the driver with a helper function
> > > > +        * to figure out what chip selects are valid, or just handle the
> > > > +        * request.
> > > > +        */
> > > > +       ops = spi_get_ops(bus);
> > > > +       if (ops->cs_info) {
> > > > +               ret = ops->cs_info(bus, cs, &info);
> > > > +       } else {
> > > > +               /*
> > > > +                * We could assume there is at least one valid chip select.
> > > > +                * The driver didn't care enough to tell us.
> > > > +                */
> > > > +               ret = 0;
> > > > +       }
> > > > +
> > > > +       if (ret) {
> > > > +               printf("Invalid cs %d (err=%d)\n", cs, ret);
> > > > +               return ret;
> > > > +       }
> > > >
> > >
> > > This is breaking 'sf probe' with associated bus and cs on SiFive.
> >
> > No this does not break anything on SiFive. It enforces the 'sf probe'
> > to use the correct CS number on SiFive. And something is wrong when CS
> > is 0 on SiFive.
>
> As discussed in another thread, this patch does not break SiFive.
> Could you please merge the remaining 2 patches in this series? thanks!

Ping?

Regards,
Bin

[PATCH v2 3/4] dm: spi: Check cs number before accessing slaves

[Jagan Teki]

On Thu, Jan 9, 2020 at 7:17 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Jagan,
>
> On Mon, Nov 18, 2019 at 3:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Jagan,
> >
> > On Tue, Oct 29, 2019 at 6:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Jagan,
> > >
> > > On Wed, Oct 16, 2019 at 11:21 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Mon, Sep 9, 2019 at 6:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Add chip select number check in spi_find_chip_select().
> > > > >
> > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > > > >
> > > > > ---
> > > > >
> > > > > Changes in v2:
> > > > > - move the chip select number check to spi_find_chip_select()
> > > > >
> > > > >  drivers/spi/spi-uclass.c | 45 ++++++++++++++++++++++++++-------------------
> > > > >  include/spi.h            |  3 ++-
> > > > >  2 files changed, 28 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> > > > > index 24de0b5..cdeceb5 100644
> > > > > --- a/drivers/spi/spi-uclass.c
> > > > > +++ b/drivers/spi/spi-uclass.c
> > > > > @@ -179,7 +179,32 @@ int spi_chip_select(struct udevice *dev)
> > > > >
> > > > >  int spi_find_chip_select(struct udevice *bus, int cs, struct udevice **devp)
> > > > >  {
> > > > > +       struct dm_spi_ops *ops;
> > > > > +       struct spi_cs_info info;
> > > > >         struct udevice *dev;
> > > > > +       int ret;
> > > > > +
> > > > > +       /*
> > > > > +        * Ask the driver. For the moment we don't have CS info.
> > > > > +        * When we do we could provide the driver with a helper function
> > > > > +        * to figure out what chip selects are valid, or just handle the
> > > > > +        * request.
> > > > > +        */
> > > > > +       ops = spi_get_ops(bus);
> > > > > +       if (ops->cs_info) {
> > > > > +               ret = ops->cs_info(bus, cs, &info);
> > > > > +       } else {
> > > > > +               /*
> > > > > +                * We could assume there is at least one valid chip select.
> > > > > +                * The driver didn't care enough to tell us.
> > > > > +                */
> > > > > +               ret = 0;
> > > > > +       }
> > > > > +
> > > > > +       if (ret) {
> > > > > +               printf("Invalid cs %d (err=%d)\n", cs, ret);
> > > > > +               return ret;
> > > > > +       }
> > > > >
> > > >
> > > > This is breaking 'sf probe' with associated bus and cs on SiFive.
> > >
> > > No this does not break anything on SiFive. It enforces the 'sf probe'
> > > to use the correct CS number on SiFive. And something is wrong when CS
> > > is 0 on SiFive.
> >
> > As discussed in another thread, this patch does not break SiFive.
> > Could you please merge the remaining 2 patches in this series? thanks!
>
> Ping?

I took it already will send it PR soon.

Jagan.