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

[Simon Glass]

Hi Jagan,

On 21 November 2016 at 10:57, Jagan Teki <jagan at openedev.com> wrote:
> On Sun, Nov 20, 2016 at 2:19 AM, 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;
>
> Yes, this is what I am commenting about.
>
> -EINVAL not needed, we can print "%d is not supporting and operating
> in normal/single mode and move on", So-that the dts will fix if
> something went wrong.

Well if you add printf() values you will bloat the code for little
benefit. If the device tree is invalid it really should be fixed.

Regards,
Simon