Patchwork [2/2] spi: dual and quad support(device tree)

login
register
mail settings
Submitter 王宇航
Date Aug. 26, 2013, 5:48 a.m.
Message ID <1377496080-14356-2-git-send-email-wangyuhang2014@gmail.com>
Download mbox | patch
Permalink /patch/269818/
State New
Headers show

Comments

王宇航 - Aug. 26, 2013, 5:48 a.m.
fix one thing in patch:
commit id:f477b7fb13df2b843997559ff34e87d054ba6538

change property spi-tx-nbits and spi-rx-nbits to optional.
If User don't set spi-tx-nbits or spi-rx-nbits, spi device mode
should be regarded as SINGLE, not return an error.

Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
---
 drivers/spi/spi.c |   68 +++++++++++++++++++++++++----------------------------
 1 file changed, 32 insertions(+), 36 deletions(-)
pekon gupta - Aug. 26, 2013, 7:18 a.m.
> 
> fix one thing in patch:
> commit id:f477b7fb13df2b843997559ff34e87d054ba6538
> 
> change property spi-tx-nbits and spi-rx-nbits to optional.
> If User don't set spi-tx-nbits or spi-rx-nbits, spi device mode
> should be regarded as SINGLE, not return an error.
> 
> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
> ---
>  drivers/spi/spi.c |   68 +++++++++++++++++++++++++--------------------------
> --
>  1 file changed, 32 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 50f7fc3..46a55f0 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -872,46 +872,42 @@ static void of_register_spi_devices(struct
> spi_master *master)
>  		/* Device DUAL/QUAD mode */
>  		prop = of_get_property(nc, "spi-tx-nbits", &len);
>  		if (!prop || len < sizeof(*prop)) {
> -			dev_err(&master->dev, "%s has no 'spi-tx-nbits'
> property\n",
> -				nc->full_name);
> -			spi_dev_put(spi);
> -			continue;
> -		}
> -		switch (be32_to_cpup(prop)) {
> -		case SPI_NBITS_SINGLE:
> -			break;
> -		case SPI_NBITS_DUAL:
> -			spi->mode |= SPI_TX_DUAL;
> -			break;
> -		case SPI_NBITS_QUAD:
> -			spi->mode |= SPI_TX_QUAD;
> -			break;
> -		default:
> -			dev_err(&master->dev, "spi-tx-nbits value is not
> supported\n");
> -			spi_dev_put(spi);
> -			continue;
> +			/* set tx mode in SINGLE as default */
[Pekon]: I think you should explicitly set, and not trust spi->mode defaults.
	spi->mode |= SPI_TX_SINGLE;

> +		} else {
> +			switch (be32_to_cpup(prop)) {
> +			case SPI_NBITS_SINGLE:
> +				break;
> +			case SPI_NBITS_DUAL:
> +				spi->mode |= SPI_TX_DUAL;
> +				break;
> +			case SPI_NBITS_QUAD:
> +				spi->mode |= SPI_TX_QUAD;
> +				break;

[Pekon]: This is incomplete definition, as indicated in [Patch 1/2] comments.
SPI_NBITS_QUAD does not clearly indicates whether SPI_TX_DUAL mode
is also supported or not. That is..
	spi->mode |= SPI_TX_QUAD;
or
	spi->mode |= SPI_TX_QUAD | SPI_TX_DUAL;


> +			default:
> +				dev_err(&master->dev,
> +					"spi-tx-nbits value is not
> supported\n");
> +				spi_dev_put(spi);
> +				continue;
>  		}
> 
>  		prop = of_get_property(nc, "spi-rx-nbits", &len);
>  		if (!prop || len < sizeof(*prop)) {
> -			dev_err(&master->dev, "%s has no 'spi-rx-nbits'
> property\n",
> -				nc->full_name);
> -			spi_dev_put(spi);
> -			continue;
> -		}
> -		switch (be32_to_cpup(prop)) {
> -		case SPI_NBITS_SINGLE:
> -			break;
> -		case SPI_NBITS_DUAL:
> -			spi->mode |= SPI_RX_DUAL;
> -			break;
> -		case SPI_NBITS_QUAD:
> -			spi->mode |= SPI_RX_QUAD;
> -			break;
> -		default:
> -			dev_err(&master->dev, "spi-rx-nbits value is not
> supported\n");
> -			spi_dev_put(spi);
> -			continue;
> +			/* set rx mode in SINGLE as default */
> +		} else {
> +			switch (be32_to_cpup(prop)) {
> +			case SPI_NBITS_SINGLE:
> +				break;
> +			case SPI_NBITS_DUAL:
> +				spi->mode |= SPI_RX_DUAL;
> +				break;
> +			case SPI_NBITS_QUAD:
> +				spi->mode |= SPI_RX_QUAD;
> +				break;
> +			default:
> +				dev_err(&master->dev,
> +					"spi-rx-nbits value is not
> supported\n");
> +				spi_dev_put(spi);
> +				continue;
>  		}
> 
>  		/* Device speed */
> --
> 1.7.9.5
pekon gupta - Aug. 26, 2013, 12:50 p.m.
> 
> >
> > fix one thing in patch:
> > commit id:f477b7fb13df2b843997559ff34e87d054ba6538
> >
> > change property spi-tx-nbits and spi-rx-nbits to optional.
> > If User don't set spi-tx-nbits or spi-rx-nbits, spi device mode
> > should be regarded as SINGLE, not return an error.
> >
> > Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
> > ---
> >  drivers/spi/spi.c |   68 +++++++++++++++++++++++++------------------------
> --
> > --
> >  1 file changed, 32 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 50f7fc3..46a55f0 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -872,46 +872,42 @@ static void of_register_spi_devices(struct
> > spi_master *master)
> >  		/* Device DUAL/QUAD mode */
> >  		prop = of_get_property(nc, "spi-tx-nbits", &len);
> >  		if (!prop || len < sizeof(*prop)) {
> > -			dev_err(&master->dev, "%s has no 'spi-tx-nbits'
> > property\n",
> > -				nc->full_name);
> > -			spi_dev_put(spi);
> > -			continue;
> > -		}
> > -		switch (be32_to_cpup(prop)) {
> > -		case SPI_NBITS_SINGLE:
> > -			break;
> > -		case SPI_NBITS_DUAL:
> > -			spi->mode |= SPI_TX_DUAL;
> > -			break;
> > -		case SPI_NBITS_QUAD:
> > -			spi->mode |= SPI_TX_QUAD;
> > -			break;
> > -		default:
> > -			dev_err(&master->dev, "spi-tx-nbits value is not
> > supported\n");
> > -			spi_dev_put(spi);
> > -			continue;
> > +			/* set tx mode in SINGLE as default */
[Pekon]: I think you should explicitly set, and not trust spi->mode defaults.
	spi->mode |= SPI_TX_SINGLE;
> 
> > +		} else {
> > +			switch (be32_to_cpup(prop)) {
> > +			case SPI_NBITS_SINGLE:
> > +				break;
> > +			case SPI_NBITS_DUAL:
> > +				spi->mode |= SPI_TX_DUAL;
> > +				break;
> > +			case SPI_NBITS_QUAD:
> > +				spi->mode |= SPI_TX_QUAD;
> > +				break;
> 
[Pekon]: And there please check for spi_master->mode_bits
before setting spi_device->mode.
spi_device->mode should be sub-set of spi_master->mode_bits.
Example: 
switch (be32_to_cpup(prop)) {
	case SPI_NBITS_SINGLE:
		if (spi_master->mode_bits & SPI_TX_SINGLE)
			spi-mode |= SPI_TX_SINGLE;
		else
			pr_err("SPI controller does not support SPI_TX_SINGLE\n");
	break;
	case SPI_NBITS_DUAL:
		if (spi_master->mode_bits & SPI_TX_DUAL)
			spi-mode |= SPI_TX_DUAL;
		else
			pr_err("SPI controller does not support SPI_TX_DUAL\n");
	break;
	case SPI_NBITS_QUAD:
		if (spi_master->mode_bits & SPI_TX_QUAD)
			spi-mode |= SPI_TX_QUAD;
		else
			pr_err("SPI controller does not support SPI_TX_QUAD\n");
	break;
}
This way you will do initial compatibility check between the
device and controller right at the probe, and you don't have
to worry about it later.


> 	spi->mode |= SPI_TX_QUAD;
> or
> 	spi->mode |= SPI_TX_QUAD | SPI_TX_DUAL;
> 
> 
> > +			default:
> > +				dev_err(&master->dev,
> > +					"spi-tx-nbits value is not
> > supported\n");
> > +				spi_dev_put(spi);
> > +				continue;
> >  		}
> >
> >  		prop = of_get_property(nc, "spi-rx-nbits", &len);
> >  		if (!prop || len < sizeof(*prop)) {
> > -			dev_err(&master->dev, "%s has no 'spi-rx-nbits'
> > property\n",
> > -				nc->full_name);
> > -			spi_dev_put(spi);
> > -			continue;
> > -		}
> > -		switch (be32_to_cpup(prop)) {
> > -		case SPI_NBITS_SINGLE:
> > -			break;
> > -		case SPI_NBITS_DUAL:
> > -			spi->mode |= SPI_RX_DUAL;
> > -			break;
> > -		case SPI_NBITS_QUAD:
> > -			spi->mode |= SPI_RX_QUAD;
> > -			break;
> > -		default:
> > -			dev_err(&master->dev, "spi-rx-nbits value is not
> > supported\n");
> > -			spi_dev_put(spi);
> > -			continue;
> > +			/* set rx mode in SINGLE as default */
> > +		} else {
> > +			switch (be32_to_cpup(prop)) {
> > +			case SPI_NBITS_SINGLE:
> > +				break;
> > +			case SPI_NBITS_DUAL:
> > +				spi->mode |= SPI_RX_DUAL;
> > +				break;
> > +			case SPI_NBITS_QUAD:
> > +				spi->mode |= SPI_RX_QUAD;
> > +				break;
> > +			default:
> > +				dev_err(&master->dev,
> > +					"spi-rx-nbits value is not
> > supported\n");
> > +				spi_dev_put(spi);
> > +				continue;
> >  		}
> >
> >  		/* Device speed */
> > --
> > 1.7.9.5
> 

with regards, pekon
王宇航 - Aug. 27, 2013, 6:11 a.m.
Hi, Pekon

2013/8/26 Gupta, Pekon <pekon@ti.com>:
>>
>> fix one thing in patch:
>> commit id:f477b7fb13df2b843997559ff34e87d054ba6538
>>
>> change property spi-tx-nbits and spi-rx-nbits to optional.
>> If User don't set spi-tx-nbits or spi-rx-nbits, spi device mode
>> should be regarded as SINGLE, not return an error.
>>
>> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
>> ---
>>  drivers/spi/spi.c |   68 +++++++++++++++++++++++++--------------------------
>> --
>>  1 file changed, 32 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 50f7fc3..46a55f0 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -872,46 +872,42 @@ static void of_register_spi_devices(struct
>> spi_master *master)
>>               /* Device DUAL/QUAD mode */
>>               prop = of_get_property(nc, "spi-tx-nbits", &len);
>>               if (!prop || len < sizeof(*prop)) {
>> -                     dev_err(&master->dev, "%s has no 'spi-tx-nbits'
>> property\n",
>> -                             nc->full_name);
>> -                     spi_dev_put(spi);
>> -                     continue;
>> -             }
>> -             switch (be32_to_cpup(prop)) {
>> -             case SPI_NBITS_SINGLE:
>> -                     break;
>> -             case SPI_NBITS_DUAL:
>> -                     spi->mode |= SPI_TX_DUAL;
>> -                     break;
>> -             case SPI_NBITS_QUAD:
>> -                     spi->mode |= SPI_TX_QUAD;
>> -                     break;
>> -             default:
>> -                     dev_err(&master->dev, "spi-tx-nbits value is not
>> supported\n");
>> -                     spi_dev_put(spi);
>> -                     continue;
>> +                     /* set tx mode in SINGLE as default */
> [Pekon]: I think you should explicitly set, and not trust spi->mode defaults.
>         spi->mode |= SPI_TX_SINGLE;
>
Now @spi_device->mode [bit8: SPI_TX_DUAL] [bit9: SPI_TX_QUAD] [bit10:
SPI_RX_DUAL] [bit11: SPI_RX_QUAD]. Because to every spi controller,
SPI SINGLE must be supported, so I did not add that as a mode. My
initial thought is if user don't select SPI_TX_DUAL and SPI_TX_QUAD,
then it is TX_SINGLE mode[bit8: 0 bit9: 0].
Now 2 approaches as follows:
1) #define SPI_TX_SINGLE ~(SPI_TX_DUAL | SPI_TX_QUAD)
    case SPI_NBITS_SINGLE:
            spi->mode & SPI_TX_SINGLE;
            break;
2)Adding a new bit in mode to specify SPI_SINGLE. But this bit also
should be reflected to @master->mode_bits. To the probe of each spi
controller driver, it is necessary to do "master->mode_bits |=
SPI_TX_SINGLE | SPI_RX_SINGLE;", in such case, it's not like a
selected mode, it's like a compulsary condition. Since all controller
do support, there's no need to make it a mode.

So I prefer the previous correction, what's your idea.

Best regards
pekon gupta - Aug. 27, 2013, 6:31 a.m.
> 
> Hi, Pekon
> 
> 2013/8/26 Gupta, Pekon <pekon@ti.com>:
> >>
> >> fix one thing in patch:
> >> commit id:f477b7fb13df2b843997559ff34e87d054ba6538
> >>
> >> change property spi-tx-nbits and spi-rx-nbits to optional.
> >> If User don't set spi-tx-nbits or spi-rx-nbits, spi device mode
> >> should be regarded as SINGLE, not return an error.
> >>
> >> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
> >> ---
> >>  drivers/spi/spi.c |   68 +++++++++++++++++++++++++----------------------
> ----
> >> --
> >>  1 file changed, 32 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> >> index 50f7fc3..46a55f0 100644
> >> --- a/drivers/spi/spi.c
> >> +++ b/drivers/spi/spi.c
> >> @@ -872,46 +872,42 @@ static void of_register_spi_devices(struct
> >> spi_master *master)
> >>               /* Device DUAL/QUAD mode */
> >>               prop = of_get_property(nc, "spi-tx-nbits", &len);
> >>               if (!prop || len < sizeof(*prop)) {
> >> -                     dev_err(&master->dev, "%s has no 'spi-tx-nbits'
> >> property\n",
> >> -                             nc->full_name);
> >> -                     spi_dev_put(spi);
> >> -                     continue;
> >> -             }
> >> -             switch (be32_to_cpup(prop)) {
> >> -             case SPI_NBITS_SINGLE:
> >> -                     break;
> >> -             case SPI_NBITS_DUAL:
> >> -                     spi->mode |= SPI_TX_DUAL;
> >> -                     break;
> >> -             case SPI_NBITS_QUAD:
> >> -                     spi->mode |= SPI_TX_QUAD;
> >> -                     break;
> >> -             default:
> >> -                     dev_err(&master->dev, "spi-tx-nbits value is not
> >> supported\n");
> >> -                     spi_dev_put(spi);
> >> -                     continue;
> >> +                     /* set tx mode in SINGLE as default */
> > [Pekon]: I think you should explicitly set, and not trust spi->mode defaults.
> >         spi->mode |= SPI_TX_SINGLE;
> >
> Now @spi_device->mode [bit8: SPI_TX_DUAL] [bit9: SPI_TX_QUAD] [bit10:
> SPI_RX_DUAL] [bit11: SPI_RX_QUAD]. Because to every spi controller,
> SPI SINGLE must be supported, so I did not add that as a mode. My
> initial thought is if user don't select SPI_TX_DUAL and SPI_TX_QUAD,
> then it is TX_SINGLE mode[bit8: 0 bit9: 0].
> Now 2 approaches as follows:
> 1) #define SPI_TX_SINGLE ~(SPI_TX_DUAL | SPI_TX_QUAD)
>     case SPI_NBITS_SINGLE:
>             spi->mode & SPI_TX_SINGLE;
>             break;
> 2)Adding a new bit in mode to specify SPI_SINGLE. But this bit also
> should be reflected to @master->mode_bits. To the probe of each spi
> controller driver, it is necessary to do "master->mode_bits |=
> SPI_TX_SINGLE | SPI_RX_SINGLE;", in such case, it's not like a
> selected mode, it's like a compulsary condition. Since all controller
> do support, there's no need to make it a mode.
> 
> So I prefer the previous correction, what's your idea.
> 
[Pekon]: Ok makes sense.. (1) is preferred, as it is compatible to existing layout

with regards, pekon

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 50f7fc3..46a55f0 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -872,46 +872,42 @@  static void of_register_spi_devices(struct spi_master *master)
 		/* Device DUAL/QUAD mode */
 		prop = of_get_property(nc, "spi-tx-nbits", &len);
 		if (!prop || len < sizeof(*prop)) {
-			dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n",
-				nc->full_name);
-			spi_dev_put(spi);
-			continue;
-		}
-		switch (be32_to_cpup(prop)) {
-		case SPI_NBITS_SINGLE:
-			break;
-		case SPI_NBITS_DUAL:
-			spi->mode |= SPI_TX_DUAL;
-			break;
-		case SPI_NBITS_QUAD:
-			spi->mode |= SPI_TX_QUAD;
-			break;
-		default:
-			dev_err(&master->dev, "spi-tx-nbits value is not supported\n");
-			spi_dev_put(spi);
-			continue;
+			/* set tx mode in SINGLE as default */
+		} else {
+			switch (be32_to_cpup(prop)) {
+			case SPI_NBITS_SINGLE:
+				break;
+			case SPI_NBITS_DUAL:
+				spi->mode |= SPI_TX_DUAL;
+				break;
+			case SPI_NBITS_QUAD:
+				spi->mode |= SPI_TX_QUAD;
+				break;
+			default:
+				dev_err(&master->dev,
+					"spi-tx-nbits value is not supported\n");
+				spi_dev_put(spi);
+				continue;
 		}
 
 		prop = of_get_property(nc, "spi-rx-nbits", &len);
 		if (!prop || len < sizeof(*prop)) {
-			dev_err(&master->dev, "%s has no 'spi-rx-nbits' property\n",
-				nc->full_name);
-			spi_dev_put(spi);
-			continue;
-		}
-		switch (be32_to_cpup(prop)) {
-		case SPI_NBITS_SINGLE:
-			break;
-		case SPI_NBITS_DUAL:
-			spi->mode |= SPI_RX_DUAL;
-			break;
-		case SPI_NBITS_QUAD:
-			spi->mode |= SPI_RX_QUAD;
-			break;
-		default:
-			dev_err(&master->dev, "spi-rx-nbits value is not supported\n");
-			spi_dev_put(spi);
-			continue;
+			/* set rx mode in SINGLE as default */
+		} else {
+			switch (be32_to_cpup(prop)) {
+			case SPI_NBITS_SINGLE:
+				break;
+			case SPI_NBITS_DUAL:
+				spi->mode |= SPI_RX_DUAL;
+				break;
+			case SPI_NBITS_QUAD:
+				spi->mode |= SPI_RX_QUAD;
+				break;
+			default:
+				dev_err(&master->dev,
+					"spi-rx-nbits value is not supported\n");
+				spi_dev_put(spi);
+				continue;
 		}
 
 		/* Device speed */