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

[U-Boot] [PATCH 1/2] 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>
---

 drivers/spi/spi-uclass.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

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 2/2] dm: spi: Check cs number before accessing slaves

[Bin Meng]

In spi_get_bus_and_cs() only bus number is checked before accessing
slaves. We should check cs number as well.

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

 drivers/spi/spi-uclass.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index 24de0b5..f633eb5 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -271,6 +271,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
 {
 	struct udevice *bus, *dev;
 	struct dm_spi_slave_platdata *plat;
+	struct spi_cs_info info;
 	bool created = false;
 	int ret;
 
@@ -283,6 +284,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
 		printf("Invalid bus %d (err=%d)\n", busnum, ret);
 		return ret;
 	}
+	ret = spi_cs_info(bus, cs, &info);
+	if (ret) {
+		printf("Invalid cs %d (err=%d)\n", cs, ret);
+		return ret;
+	}
 	ret = spi_find_chip_select(bus, cs, &dev);
 
 	/*
-- 
2.7.4

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

[Bin Meng]

On Thu, Aug 29, 2019 at 10:10 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>
> ---
>
>  drivers/spi/spi-uclass.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>

Ping?

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

[Bin Meng]

On Thu, Aug 29, 2019 at 10:10 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> In spi_get_bus_and_cs() only bus number is checked before accessing
> slaves. We should check cs number as well.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/spi/spi-uclass.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>

Ping?

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

[Jagan Teki]

On Thu, Aug 29, 2019 at 7:40 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>

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

[Jagan Teki]

On Thu, Aug 29, 2019 at 7:40 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> In spi_get_bus_and_cs() only bus number is checked before accessing
> slaves. We should check cs number as well.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/spi/spi-uclass.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> index 24de0b5..f633eb5 100644
> --- a/drivers/spi/spi-uclass.c
> +++ b/drivers/spi/spi-uclass.c
> @@ -271,6 +271,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>  {
>         struct udevice *bus, *dev;
>         struct dm_spi_slave_platdata *plat;
> +       struct spi_cs_info info;
>         bool created = false;
>         int ret;
>
> @@ -283,6 +284,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>                 printf("Invalid bus %d (err=%d)\n", busnum, ret);
>                 return ret;
>         }
> +       ret = spi_cs_info(bus, cs, &info);
> +       if (ret) {
> +               printf("Invalid cs %d (err=%d)\n", cs, ret);
> +               return ret;
> +       }
>         ret = spi_find_chip_select(bus, cs, &dev);

I think it would rather check the cs_info in spi_find_chip_select
function itself, make more sense.

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

[Bin Meng]

Hi Jagan,

On Sat, Sep 7, 2019 at 11:45 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Thu, Aug 29, 2019 at 7:40 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > In spi_get_bus_and_cs() only bus number is checked before accessing
> > slaves. We should check cs number as well.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  drivers/spi/spi-uclass.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> > index 24de0b5..f633eb5 100644
> > --- a/drivers/spi/spi-uclass.c
> > +++ b/drivers/spi/spi-uclass.c
> > @@ -271,6 +271,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
> >  {
> >         struct udevice *bus, *dev;
> >         struct dm_spi_slave_platdata *plat;
> > +       struct spi_cs_info info;
> >         bool created = false;
> >         int ret;
> >
> > @@ -283,6 +284,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
> >                 printf("Invalid bus %d (err=%d)\n", busnum, ret);
> >                 return ret;
> >         }
> > +       ret = spi_cs_info(bus, cs, &info);
> > +       if (ret) {
> > +               printf("Invalid cs %d (err=%d)\n", cs, ret);
> > +               return ret;
> > +       }
> >         ret = spi_find_chip_select(bus, cs, &dev);
>
> I think it would rather check the cs_info in spi_find_chip_select
> function itself, make more sense.

This routine spi_get_bus_and_cs() has both busnum and cs as
parameters, but only busnum is checked so far. It looks to me more
appropriate to check the cs in spi_get_bus_and_cs() directly, instead
of going into spi_find_chip_select().

Regards,
Bin

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

[Jagan Teki]

On Sat, Sep 7, 2019 at 9:19 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Jagan,
>
> On Sat, Sep 7, 2019 at 11:45 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Thu, Aug 29, 2019 at 7:40 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > In spi_get_bus_and_cs() only bus number is checked before accessing
> > > slaves. We should check cs number as well.
> > >
> > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > > ---
> > >
> > >  drivers/spi/spi-uclass.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> > > index 24de0b5..f633eb5 100644
> > > --- a/drivers/spi/spi-uclass.c
> > > +++ b/drivers/spi/spi-uclass.c
> > > @@ -271,6 +271,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
> > >  {
> > >         struct udevice *bus, *dev;
> > >         struct dm_spi_slave_platdata *plat;
> > > +       struct spi_cs_info info;
> > >         bool created = false;
> > >         int ret;
> > >
> > > @@ -283,6 +284,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
> > >                 printf("Invalid bus %d (err=%d)\n", busnum, ret);
> > >                 return ret;
> > >         }
> > > +       ret = spi_cs_info(bus, cs, &info);
> > > +       if (ret) {
> > > +               printf("Invalid cs %d (err=%d)\n", cs, ret);
> > > +               return ret;
> > > +       }
> > >         ret = spi_find_chip_select(bus, cs, &dev);
> >
> > I think it would rather check the cs_info in spi_find_chip_select
> > function itself, make more sense.
>
> This routine spi_get_bus_and_cs() has both busnum and cs as
> parameters, but only busnum is checked so far. It looks to me more
> appropriate to check the cs in spi_get_bus_and_cs() directly, instead
> of going into spi_find_chip_select().

True, but the spi_find_chip_select is also used in other calls (like
spi_find_bus_and_cs). Checking cs_info on the core function might help
to calling it on multiple places. what do you think?

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

[Bin Meng]

Hi Jagan,

On Sat, Sep 7, 2019 at 11:58 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Sat, Sep 7, 2019 at 9:19 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Jagan,
> >
> > On Sat, Sep 7, 2019 at 11:45 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > On Thu, Aug 29, 2019 at 7:40 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > In spi_get_bus_and_cs() only bus number is checked before accessing
> > > > slaves. We should check cs number as well.
> > > >
> > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > > > ---
> > > >
> > > >  drivers/spi/spi-uclass.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> > > > index 24de0b5..f633eb5 100644
> > > > --- a/drivers/spi/spi-uclass.c
> > > > +++ b/drivers/spi/spi-uclass.c
> > > > @@ -271,6 +271,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
> > > >  {
> > > >         struct udevice *bus, *dev;
> > > >         struct dm_spi_slave_platdata *plat;
> > > > +       struct spi_cs_info info;
> > > >         bool created = false;
> > > >         int ret;
> > > >
> > > > @@ -283,6 +284,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
> > > >                 printf("Invalid bus %d (err=%d)\n", busnum, ret);
> > > >                 return ret;
> > > >         }
> > > > +       ret = spi_cs_info(bus, cs, &info);
> > > > +       if (ret) {
> > > > +               printf("Invalid cs %d (err=%d)\n", cs, ret);
> > > > +               return ret;
> > > > +       }
> > > >         ret = spi_find_chip_select(bus, cs, &dev);
> > >
> > > I think it would rather check the cs_info in spi_find_chip_select
> > > function itself, make more sense.
> >
> > This routine spi_get_bus_and_cs() has both busnum and cs as
> > parameters, but only busnum is checked so far. It looks to me more
> > appropriate to check the cs in spi_get_bus_and_cs() directly, instead
> > of going into spi_find_chip_select().
>
> True, but the spi_find_chip_select is also used in other calls (like
> spi_find_bus_and_cs). Checking cs_info on the core function might help
> to calling it on multiple places. what do you think?

Ok, makes sense. I will do it in v2.

Regards,
Bin