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

[Jagan Teki]

Hi Simon,

On Thu, Nov 24, 2016 at 7:51 AM, Simon Glass <sjg at chromium.org> wrote:
> 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.

Yeah, ie what if dts has a wrong value and do print that and continue
with default width, so-that the user will update this for next run.
Since it's not key a attribute to break or decide functionality better
to go with it.

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.