[U-Boot] [PATCH v2 10/22] spi: Add error checking for invalid bus widths

[Simon Glass]

Hi Fabio,

On 19 November 2016 at 13:49, Fabio Estevam <festevam at gmail.com> wrote:
> On Sat, Nov 19, 2016 at 6:04 PM, Simon Glass <sjg at chromium.org> wrote:
>
>>> EPROTONOSUPPORT means: /* Protocol not supported */, which does not
>>> seem to be very appropriate here.
>>
>> This is a protocol as far as I can see - you can either use one pin or
>> four pins.
>
> Actually they are SPI modes: one, two or four pins.
>
>>> Why not return -EINVAL instead?
>>
>> The value is valid but is not supported. If we just return -EINVAL for
>> anything we don't like, it makes it harder to root-cause the error. In
>> particular we use -EINVAL when decoding the device tree. But
>> EPROTONOSUPPORT is not widely used.
>
> I think the current behaviour of not returning an error code on an
> invalid mode is correct and it matches what the kernel does in
> drivers/spi/spi.c.
>
> If an invalid mode is passed we just ignore it and operate in single
> mode instead.
>
> Maybe we can make this clearer by printing a message like this:
>
> --- a/drivers/spi/spi-uclass.c
> +++ b/drivers/spi/spi-uclass.c
> @@ -408,7 +408,7 @@ int spi_slave_ofdata_to_platdata(const void *blob, int node,
>                 mode |= SPI_TX_QUAD;
>                 break;
>         default:
> -               error("spi-tx-bus-width %d not supported\n", value);
> +               printf("spi-tx-bus-width %d not supported, operating
> in single mode\n", value);
>                 break;
>         }
>
> @@ -423,7 +423,7 @@ int spi_slave_ofdata_to_platdata(const void *blob, int node,
>                 mode |= SPI_RX_QUAD;
>                 break;
>         default:
> -               error("spi-rx-bus-width %d not supported\n", value);
> +               printf("spi-rx-bus-width %d not supported, operating
> in single mode\n", value);
>                 break;
>         }

OK I took another look at the code around it and I see that I misread
it. The 'default' case really is an invalid value isn't it? So -EINVAL
is the right answer. Sorry about that.

Either it is an error, and we should return an error code, or it is
not and we should continue (and ideally not print a message since that
bloats the code).

In this case it looks wrong to me - someone has put an incorrect value
in the device tree, and they should fix it and retry.

Regards,
Simon