| Message ID | 20180322135338.60923-1-jakob.unterwurzacher@theobroma-systems.com |
|---|---|
| Headers | show |
| Series | can: ucan: add driver for Theobroma Systems UCAN devices | expand |
Hello Jacob, Am 22.03.2018 um 14:53 schrieb Jakob Unterwurzacher: > This is v3 of the Theobroma Systems CAN/USB adapter driver > upstreaming effort. > > Featured v2 -> v3 changes: > * count error frames as data packets > * use canid_t for all can ids > * use BIT(x) instead of (1 << x) > * use __le16 / __le32 for little-endian fields > * add spinlock around context allocation (fixes a possible race) > * fix comment style > * use WARN_ON return value > * fix state logic bug that did not allow return to ERROR_ACTIVE > * drop echo_index from context_array (not needed) > * rename "tx_contexts" -> "context_array" to prevent confusion > * add __func__ to all errors and warnings, and to info where it made sense The final output messages in the driver should especially be useful for the end user... and not the developer! This is also true for the function names. You already use more "__func__" than all other CAN drivers together. Just my opinion! > > Jakob Unterwurzacher (1): > can: ucan: add driver for Theobroma Systems UCAN devices > > Documentation/networking/can_ucan_protocol.rst | 315 +++++ > Documentation/networking/index.rst | 1 + > drivers/net/can/usb/Kconfig | 10 + > drivers/net/can/usb/Makefile | 1 + > drivers/net/can/usb/ucan.c | 1628 ++++++++++++++++++++++++ > 5 files changed, 1955 insertions(+) > create mode 100644 Documentation/networking/can_ucan_protocol.rst > create mode 100644 drivers/net/can/usb/ucan.c Wolfgang.
On 23.03.18 09:32, Wolfgang Grandegger wrote: >> * add __func__ to all errors and warnings, and to info where it made sense > > The final output messages in the driver should especially be useful for > the end user... and not the developer! This is also true for the > function names. You already use more "__func__" than all other CAN > drivers together. Just my opinion! The idea was to make it clear which driver printed the message. In my opinion, this is a problem: > drivers/net/can/usb$ git grep "No memory left for USB buffer" > ems_usb.c: netdev_err(netdev, "No memory left for USB buffer\n"); > ems_usb.c: netdev_err(netdev, "No memory left for USB buffer\n"); > esd_usb2.c: "No memory left for USB buffer\n"); > esd_usb2.c: netdev_err(netdev, "No memory left for USB buffer\n"); > gs_usb.c: netdev_err(netdev, "No memory left for USB buffer\n"); > gs_usb.c: "No memory left for USB buffer\n"); > kvaser_usb.c: "No memory left for USB buffer\n"); > mcba_usb.c: netdev_err(netdev, "No memory left for USB buffer\n"); > usb_8dev.c: netdev_err(netdev, "No memory left for USB buffer\n"); > usb_8dev.c: netdev_err(netdev, "No memory left for USB buffer\n"); But I'm open to other suggestions (use a fixed "ucan: " prefix?) or to drop it entirely if you think it is not worth it. Thanks for the feedback, Jakob
Am 23.03.2018 um 10:40 schrieb Jakob Unterwurzacher: > On 23.03.18 09:32, Wolfgang Grandegger wrote: >>> * add __func__ to all errors and warnings, and to info where it made >>> sense >> >> The final output messages in the driver should especially be useful for >> the end user... and not the developer! This is also true for the >> function names. You already use more "__func__" than all other CAN >> drivers together. Just my opinion! > > The idea was to make it clear which driver printed the message. In my > opinion, this is a problem: > >> drivers/net/can/usb$ git grep "No memory left for USB buffer" >> ems_usb.c: netdev_err(netdev, "No memory left for >> USB buffer\n"); >> ems_usb.c: netdev_err(netdev, "No memory left for USB >> buffer\n"); >> esd_usb2.c: "No memory left for USB >> buffer\n"); >> esd_usb2.c: netdev_err(netdev, "No memory left for USB >> buffer\n"); >> gs_usb.c: netdev_err(netdev, "No memory left for USB >> buffer\n"); >> gs_usb.c: "No memory left for >> USB buffer\n"); >> kvaser_usb.c: "No memory left for USB >> buffer\n"); >> mcba_usb.c: netdev_err(netdev, "No memory left for >> USB buffer\n"); >> usb_8dev.c: netdev_err(netdev, "No memory left for USB >> buffer\n"); >> usb_8dev.c: netdev_err(netdev, "No memory left for >> USB buffer\n"); > But I'm open to other suggestions (use a fixed "ucan: " prefix?) or to > drop it entirely if you think it is not worth it. But there is already a device prefix, e.g.: peak_usb 1-6:1.0: PEAK-System PCAN-USB adapter hwrev 28 serial FFFFFFFF (1 channel) peak_usb 1-6:1.0 can0: attached to PCAN-USB channel 0 (device 255) ^^^^^^^^ No need to add another one! Wolfgang.
On 23.03.18 11:04, Wolfgang Grandegger wrote: > >> But I'm open to other suggestions (use a fixed "ucan: " prefix?) or to >> drop it entirely if you think it is not worth it. > > But there is already a device prefix, e.g.: > > peak_usb 1-6:1.0: PEAK-System PCAN-USB adapter hwrev 28 serial FFFFFFFF (1 channel) > peak_usb 1-6:1.0 can0: attached to PCAN-USB channel 0 (device 255) > ^^^^^^^^ Interesting. Looks like the UCAN driver is missing something to make this work: [ 67.792947] usb 5-1.4: ucan_probe: registered UCAN device at can0 I'll take a closer look. Thanks, Jakob
Where possible we changed dev_XXX to netdev_XXX. and removed all __func__. netdev_XXX prints the necessary information on the device, bus and netif The remaining dev_XXX (within probe) are extended such that they print the string "ucan" (__func__ are also removed) Am 23.03.18 um 11:12 schrieb Jakob Unterwurzacher: > On 23.03.18 11:04, Wolfgang Grandegger wrote: >> >>> But I'm open to other suggestions (use a fixed "ucan: " prefix?) or to >>> drop it entirely if you think it is not worth it. >> >> But there is already a device prefix, e.g.: >> >> peak_usb 1-6:1.0: PEAK-System PCAN-USB adapter hwrev 28 serial >> FFFFFFFF (1 channel) >> peak_usb 1-6:1.0 can0: attached to PCAN-USB channel 0 (device 255) >> ^^^^^^^^ > > Interesting. Looks like the UCAN driver is missing something to make > this work: > > [ 67.792947] usb 5-1.4: ucan_probe: registered UCAN device at can0 > > I'll take a closer look. > > Thanks, > Jakob -- Martin Elshuber Theobroma Systems Design und Consulting GmbH Seestadtstraße 27 (Aspern IQ), 1220 Wien, Austria Phone: +43 1 236 98 93-405, Fax: +43 1 236 98 93-9 http://www.theobroma-systems.com