| Message ID | CAOMZO5B0N8yXtM=6pxZ4CfkuE=YmyGpBbEiszs0vQiKhzC-OsQ@mail.gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
Hi Fabio, On 19 November 2016 at 13:49, Fabio Estevam <festevam@gmail.com> wrote: > On Sat, Nov 19, 2016 at 6:04 PM, Simon Glass <sjg@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
On Sun, Nov 20, 2016 at 2:19 AM, Fabio Estevam <festevam@gmail.com> wrote: > On Sat, Nov 19, 2016 at 6:04 PM, Simon Glass <sjg@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. thanks!
Hi Jagan, On 21 November 2016 at 10:57, Jagan Teki <jagan@openedev.com> wrote: > On Sun, Nov 20, 2016 at 2:19 AM, Fabio Estevam <festevam@gmail.com> wrote: >> On Sat, Nov 19, 2016 at 6:04 PM, Simon Glass <sjg@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
Hi Simon, On Thu, Nov 24, 2016 at 7:51 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Jagan, > > On 21 November 2016 at 10:57, Jagan Teki <jagan@openedev.com> wrote: >> On Sun, Nov 20, 2016 at 2:19 AM, Fabio Estevam <festevam@gmail.com> wrote: >>> On Sat, Nov 19, 2016 at 6:04 PM, Simon Glass <sjg@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!
On Fri, Nov 25, 2016 at 2:57 PM, Jagan Teki <jagan@openedev.com> wrote: > 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. Agreed. This also matches with the kernel behaviour.
Hi, On 25 November 2016 at 09:59, Fabio Estevam <festevam@gmail.com> wrote: > On Fri, Nov 25, 2016 at 2:57 PM, Jagan Teki <jagan@openedev.com> wrote: > >> 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. > > Agreed. This also matches with the kernel behaviour. So it is correct to print an error, and then continue? This error will almost never occur and thus it wastes code space. SPI is sensitive because it can be used in SPL. Linux doesn't care about code size as much. So how about either: 1. debug() and return an error 2. debug() and skip the error Regards, Simon
On Fri, Nov 25, 2016 at 5:38 PM, Simon Glass <sjg@chromium.org> wrote: > So it is correct to print an error, and then continue? This error will > almost never occur and thus it wastes code space. SPI is sensitive > because it can be used in SPL. Linux doesn't care about code size as > much. > > So how about either: > 1. debug() and return an error > 2. debug() and skip the error I prefer option 2, thanks.
On Sat, Nov 26, 2016 at 1:08 AM, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On 25 November 2016 at 09:59, Fabio Estevam <festevam@gmail.com> wrote: >> On Fri, Nov 25, 2016 at 2:57 PM, Jagan Teki <jagan@openedev.com> wrote: >> >>> 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. >> >> Agreed. This also matches with the kernel behaviour. > > So it is correct to print an error, and then continue? This error will > almost never occur and thus it wastes code space. SPI is sensitive > because it can be used in SPL. Linux doesn't care about code size as > much. > > So how about either: > 1. debug() and return an error > 2. debug() and skip the error I prefer 2. for SPL and replace debug with printf for U-Boot. thanks!
Hi, On 25 November 2016 at 20:28, Jagan Teki <jagan@openedev.com> wrote: > On Sat, Nov 26, 2016 at 1:08 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi, >> >> On 25 November 2016 at 09:59, Fabio Estevam <festevam@gmail.com> wrote: >>> On Fri, Nov 25, 2016 at 2:57 PM, Jagan Teki <jagan@openedev.com> wrote: >>> >>>> 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. >>> >>> Agreed. This also matches with the kernel behaviour. >> >> So it is correct to print an error, and then continue? This error will >> almost never occur and thus it wastes code space. SPI is sensitive >> because it can be used in SPL. Linux doesn't care about code size as >> much. >> >> So how about either: >> 1. debug() and return an error >> 2. debug() and skip the error > > I prefer 2. for SPL and replace debug with printf for U-Boot. OK I have sent v3. Regards, Simon
--- 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; }