| Message ID | 20240422065114.3185505-1-christoph.fritz@hexdev.de |
|---|---|
| Headers | show |
| Series | LIN Bus support for Linux | expand |
On 22/04/2024 08:51, Christoph Fritz wrote: > Add documentation of device tree bindings for serdev UART LIN-Bus > devices equipped with LIN transceivers. A nit, subject: drop second/last, redundant "dt bindings". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > --- > .../bindings/net/can/linux,lin-serdev.yaml | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/can/linux,lin-serdev.yaml > > diff --git a/Documentation/devicetree/bindings/net/can/linux,lin-serdev.yaml b/Documentation/devicetree/bindings/net/can/linux,lin-serdev.yaml > new file mode 100644 > index 0000000000000..cb4e932ff249c > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/can/linux,lin-serdev.yaml > @@ -0,0 +1,29 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/can/linux,lin-serdev.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Linux serdev LIN-Bus Support This looks like Linux binding, but we expect here description of hardware. > + > +description: | > + LIN-Bus support for UART devices equipped with LIN transceivers, > + utilizing the Serial Device Bus (serdev) interface. serdev is Linux thingy, AFAIR. Please describe the hardware. > + > + For more details on an adapter, visit: https://hexdev.de/hexlin#tty > + > +properties: > + compatible: > + const: linux,lin-serdev Feels confusing. Your link describes real hardware, but you wrote bindings for software construct. If you add this to DT, then it is hard-wired on the board, right? If so, how this could be a software construct? > + > +required: > + - compatible > + > +examples: > + - | > + &uart2 { & does not make much sense here. I think you wanted it to be serial bus, so serial. > + status = "okay"; Drop, it was not disabled anywhere. > + linbus { > + compatible = "linux,lin-serdev"; > + }; > + }; Best regards, Krzysztof
On Mon, 22 Apr 2024 08:51:08 +0200, Christoph Fritz wrote: > Add documentation of device tree bindings for serdev UART LIN-Bus > devices equipped with LIN transceivers. > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > --- > .../bindings/net/can/linux,lin-serdev.yaml | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/can/linux,lin-serdev.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/can/linux,lin-serdev.yaml: 'maintainers' is a required property hint: Metaschema for devicetree binding documentation from schema $id: http://devicetree.org/meta-schemas/base.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/can/linux,lin-serdev.yaml: 'oneOf' conditional failed, one must be fixed: 'unevaluatedProperties' is a required property 'additionalProperties' is a required property hint: Either unevaluatedProperties or additionalProperties must be present from schema $id: http://devicetree.org/meta-schemas/core.yaml# Error: Documentation/devicetree/bindings/net/can/linux,lin-serdev.example.dts:18.9-15 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/net/can/linux,lin-serdev.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2 make: *** [Makefile:240: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240422065114.3185505-6-christoph.fritz@hexdev.de The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 22/04/2024 08:51, Christoph Fritz wrote: > Add documentation of device tree bindings for serdev UART LIN-Bus > devices equipped with LIN transceivers. > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > --- Please test patches before sending... Best regards, Krzysztof
... > > --- /dev/null > > +++ b/drivers/net/can/lin.c > ...> +static int lin_create_sysfs_id_files(struct net_device *ndev) > > +{ > > + struct lin_device *ldev = netdev_priv(ndev); > > + struct kobj_attribute *attr; > > + int ret; > > + > > + for (int id = 0; id < LIN_NUM_IDS; id++) { > > + ldev->sysfs_entries[id].ldev = ldev; > > + attr = &ldev->sysfs_entries[id].attr; > > + attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id); > > + if (!attr->attr.name) > > + return -ENOMEM; > > + attr->attr.mode = 0644; > > + attr->show = lin_identifier_show; > > + attr->store = lin_identifier_store; > > + > > + sysfs_attr_init(&attr->attr); > > + ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr); > > + if (ret) { > > + kfree(attr->attr.name); > > + kfree(attr); > > Is the latter kfree() right? It appears not. Thanks for the catch, it's wrong and will be removed in v2. > > > + return -ENOMEM; > > + } > > + } > > + > > + return 0; > > +} > ... > > +static void lin_tx_work_handler(struct work_struct *ws) > > +{ > > + struct lin_device *ldev = container_of(ws, struct lin_device, > > + tx_work); > > + struct net_device *ndev = ldev->ndev; > > + struct canfd_frame *cfd; > > + struct lin_frame lf; > > + > > + ldev->tx_busy = true; > > How is this store protected against reordering/race conditions? Falsely it is not, like in mcp251x.c I'll add a mutex. > > > + > > + cfd = (struct canfd_frame *)ldev->tx_skb->data; > > + lf.checksum_mode = (cfd->can_id & LIN_ENHANCED_CKSUM_FLAG) ? > > + LINBUS_ENHANCED : LINBUS_CLASSIC; > > + lf.lin_id = (u8)(cfd->can_id & LIN_ID_MASK); > > Why is that cast necessary? It's not. > > > + lf.len = min(cfd->len, LIN_MAX_DLEN); > > + memcpy(lf.data, cfd->data, lf.len); > > + > > + ret = ldev->ldev_ops->ldo_tx(ldev, &lf); > > + if (ret) { > > + DEV_STATS_INC(ndev, tx_dropped); > > + netdev_err_once(ndev, "transmission failure %d\n", ret); > > + goto lin_tx_out; > > Where is this label? In a later patch, let me fix the patchset accordingly. > > > + } > > + > > + DEV_STATS_INC(ndev, tx_packets); > > + DEV_STATS_ADD(ndev, tx_bytes, lf.len); > > + ldev->tx_busy = false; > > The same as above. OK > > > + netif_wake_queue(ndev); > > +} > > + > > +static netdev_tx_t lin_start_xmit(struct sk_buff *skb, > > + struct net_device *ndev) > > +{ > > + struct lin_device *ldev = netdev_priv(ndev); > > + > > + if (ldev->tx_busy) > > + return NETDEV_TX_BUSY; > > And here too. OK > > > + > > + netif_stop_queue(ndev); > > + ldev->tx_skb = skb; > > + queue_work(ldev->wq, &ldev->tx_work); > > + > > + return NETDEV_TX_OK; > > +} > ... > > +u8 lin_get_checksum(u8 pid, u8 n_of_bytes, const u8 *bytes, > > + enum lin_checksum_mode cm) > > +{ > > + uint csum = 0; > > Is "uint" of the preffered types in the kernel? OK, no sysv 'uint', will be changed in another patch too > > > + int i; > > + > > + if (cm == LINBUS_ENHANCED) > > + csum += pid; > > + > > + for (i = 0; i < n_of_bytes; i++) { > > + csum += bytes[i]; > > + if (csum > 255) > > + csum -= 255; > > + } > > + > > + return (u8)(~csum & 0xff); > > Unnecessary cast? Yes > > > +} > > > > +int lin_rx(struct lin_device *ldev, const struct lin_frame *lf) > > +{ > > + struct net_device *ndev = ldev->ndev; > > + struct can_frame *cf; > > + struct sk_buff *skb; > > + int ret; > > + > > + if (!ndev) > > + return -ENODEV; > > Is this racy or is this only a sanity check? Just beeing cautious, I guess it can be removed > > > + netdev_dbg(ndev, "id:%02x, len:%u, data:%*ph, checksum:%02x (%s)\n", > > + lf->lin_id, lf->len, lf->len, lf->data, lf->checksum, > > + lf->checksum_mode ? "enhanced" : "classic"); > > + > > + ret = lin_bump_rx_err(ldev, lf); > > + if (ret) { > > + DEV_STATS_INC(ndev, rx_dropped); > > + return ret; > > + } > > + > > + skb = alloc_can_skb(ndev, &cf); > > + if (unlikely(!skb)) { > > + DEV_STATS_INC(ndev, rx_dropped); > > + return -ENOMEM; > > + } > > + > > + cf->can_id = lf->lin_id; > > + cf->len = min(lf->len, LIN_MAX_DLEN); > > + memcpy(cf->data, lf->data, cf->len); > > + > > + DEV_STATS_INC(ndev, rx_packets); > > + DEV_STATS_ADD(ndev, rx_bytes, cf->len); > > + > > + netif_receive_skb(skb); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(lin_rx); > > + > > +static int lin_set_bittiming(struct net_device *ndev) > > +{ > > + struct lin_device *ldev = netdev_priv(ndev); > > + unsigned int bitrate; > > + int ret; > > + > > + bitrate = ldev->can.bittiming.bitrate; > > + ret = ldev->ldev_ops->update_bitrate(ldev, bitrate); > > + > > + return ret; > > No need for ret then. Compact code, sure, will get adapted > > > +} > > + > > +static const u32 lin_bitrate[] = { 1200, 2400, 4800, 9600, 19200 }; > > + > > +struct lin_device *register_lin(struct device *dev, > > + const struct lin_device_ops *ldops) > > +{ > > + struct net_device *ndev; > > + struct lin_device *ldev; > > + int ret; > > + > > + if (!ldops || !ldops->ldo_tx || !ldops->update_bitrate) { > > + netdev_err(ndev, "missing mandatory lin_device_ops\n"); > > + return ERR_PTR(-EOPNOTSUPP); > > Would EINVAL fit better? no preference over the other, will use EINVAL > > > + } > > + > > + ndev = alloc_candev(sizeof(struct lin_device), 1); > > + if (!ndev) > > + return ERR_PTR(-ENOMEM); > > + > > + ldev = netdev_priv(ndev); > > + > > + ldev->ldev_ops = ldops; > > + ndev->netdev_ops = &lin_netdev_ops; > > + ndev->flags |= IFF_ECHO; > > + ndev->mtu = CANFD_MTU; > > + ldev->can.bittiming.bitrate = LIN_DEFAULT_BAUDRATE; > > + ldev->can.ctrlmode = CAN_CTRLMODE_LIN; > > + ldev->can.ctrlmode_supported = 0; > > + ldev->can.bitrate_const = lin_bitrate; > > + ldev->can.bitrate_const_cnt = ARRAY_SIZE(lin_bitrate); > > + ldev->can.do_set_bittiming = lin_set_bittiming; > > + ldev->ndev = ndev; > > + ldev->dev = dev; > > + > > + SET_NETDEV_DEV(ndev, dev); > > + > > + ret = lin_set_bittiming(ndev); > > + if (ret) { > > + netdev_err(ndev, "set bittiming failed\n"); > > + goto exit_candev; > > + } > > + > > + ret = register_candev(ndev); > > + if (ret) > > + goto exit_candev; > > + > > + ldev->lin_ids_kobj = kobject_create_and_add("lin_ids", &ndev->dev.kobj); > > + if (!ldev->lin_ids_kobj) { > > + netdev_err(ndev, "Failed to create sysfs directory\n"); > > + ret = -ENOMEM; > > + goto exit_unreg; > > + } > > + > > + ret = lin_create_sysfs_id_files(ndev); > > + if (ret) { > > + netdev_err(ndev, "Failed to create sysfs entry: %d\n", ret); > > + goto exit_kobj_put; > > + } > > + > > + ldev->wq = alloc_workqueue(dev_name(dev), WQ_FREEZABLE | WQ_MEM_RECLAIM, > > + 0); > > It would be worth noting why you need your own WQ. I'll add a comment stating: /* Use workqueue as tx over USB/SPI/... may sleep */ > > > + if (!ldev->wq) > > + goto exit_rm_files; > > + > > + INIT_WORK(&ldev->tx_work, lin_tx_work_handler); > > + > > + netdev_info(ndev, "LIN initialized.\n"); > > + > > + return ldev; > > + > > +exit_rm_files: > > + lin_remove_sysfs_id_files(ndev); > > +exit_kobj_put: > > + kobject_put(ldev->lin_ids_kobj); > > +exit_unreg: > > + unregister_candev(ndev); > > +exit_candev: > > + free_candev(ndev); > > + return ERR_PTR(ret); > > +} > > +EXPORT_SYMBOL_GPL(register_lin); > > + > > +void unregister_lin(struct lin_device *ldev) > > +{ > > + struct net_device *ndev = ldev->ndev; > > + > > + lin_remove_sysfs_id_files(ndev); > > + kobject_put(ldev->lin_ids_kobj); > > + unregister_candev(ndev); > > + destroy_workqueue(ldev->wq); > > + free_candev(ndev); > > +} > > +EXPORT_SYMBOL_GPL(unregister_lin); > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_AUTHOR("Christoph Fritz <christoph.fritz@hexdev.de>"); > > +MODULE_DESCRIPTION("LIN bus to CAN glue driver"); > > diff --git a/include/net/lin.h b/include/net/lin.h > > new file mode 100644 > > index 0000000000000..2fe16e142db96 > > --- /dev/null > > +++ b/include/net/lin.h > > @@ -0,0 +1,97 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */ > > + > > +#ifndef _NET_LIN_H_ > > +#define _NET_LIN_H_ > > + > > +#include <linux/can/dev.h> > > +#include <linux/device.h> > > + > > +#define LIN_NUM_IDS 64 > > +#define LIN_HEADER_SIZE 3 > > +#define LIN_MAX_DLEN 8 > > + > > +#define LIN_MAX_BAUDRATE 20000 > > +#define LIN_MIN_BAUDRATE 1000 > > +#define LIN_DEFAULT_BAUDRATE 9600 > > +#define LIN_SYNC_BYTE 0x55 > > + > > +#define LIN_ID_MASK 0x0000003FU > > GEN_MASK() ? In the upcoming v2 I'll use: #define LIN_ID_MASK GENMASK(5, 0) > > > +/* special ID descriptions for LIN */ > > +#define LIN_ENHANCED_CKSUM_FLAG 0x00000100U > > + > > +static const unsigned char lin_id_parity_tbl[] = { > > This ends up in every translation unit you include lin.h into. Bad. This is also being used by a serial lin driver. But I guess most of the drivers have no need for this. Mhm, ... any ideas? > And perhaps u8? OK > > > + 0x80, 0xc0, 0x40, 0x00, 0xc0, 0x80, 0x00, 0x40, > > + 0x00, 0x40, 0xc0, 0x80, 0x40, 0x00, 0x80, 0xc0, > > + 0x40, 0x00, 0x80, 0xc0, 0x00, 0x40, 0xc0, 0x80, > > + 0xc0, 0x80, 0x00, 0x40, 0x80, 0xc0, 0x40, 0x00, > > + 0x00, 0x40, 0xc0, 0x80, 0x40, 0x00, 0x80, 0xc0, > > + 0x80, 0xc0, 0x40, 0x00, 0xc0, 0x80, 0x00, 0x40, > > + 0xc0, 0x80, 0x00, 0x40, 0x80, 0xc0, 0x40, 0x00, > > + 0x40, 0x00, 0x80, 0xc0, 0x00, 0x40, 0xc0, 0x80, > > +}; > > + > > +#define LIN_GET_ID(PID) ((PID) & LIN_ID_MASK) > > FIELD_GET() ? In the upcoming v2 I'll use: #define LIN_GET_ID(PID) FIELD_GET(LIN_ID_MASK, PID) > > > +#define LIN_FORM_PID(ID) (LIN_GET_ID(ID) | \ > > + lin_id_parity_tbl[LIN_GET_ID(ID)]) > > +#define LIN_GET_PARITY(PID) ((PID) & ~LIN_ID_MASK) > > +#define LIN_CHECK_PID(PID) (LIN_GET_PARITY(PID) == \ > > + LIN_GET_PARITY(LIN_FORM_PID(PID))) > Thanks for the feedback -- Christoph
On 23. 04. 24, 11:33, Christoph Fritz wrote: >>> --- /dev/null >>> +++ b/include/net/lin.h >>> @@ -0,0 +1,97 @@ ... >>> +/* special ID descriptions for LIN */ >>> +#define LIN_ENHANCED_CKSUM_FLAG 0x00000100U >>> + >>> +static const unsigned char lin_id_parity_tbl[] = { >> >> This ends up in every translation unit you include lin.h into. Bad. > > This is also being used by a serial lin driver. But I guess most of the drivers have no need for this. Mhm, ... any ideas? If needs be, put it to a .c and keep an extern here. thanks,
Hello Krzysztof, thanks for your feedback, please see my answers below. On Mon, 2024-04-22 at 09:54 +0200, Krzysztof Kozlowski wrote: > On 22/04/2024 08:51, Christoph Fritz wrote: > > Add documentation of device tree bindings for serdev UART LIN-Bus > > devices equipped with LIN transceivers. > > A nit, subject: drop second/last, redundant "dt bindings". The > "dt-bindings" prefix is already stating that these are bindings. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 OK > > > > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > > --- > > .../bindings/net/can/linux,lin-serdev.yaml | 29 +++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/net/can/linux,lin-serdev.yaml > > > > diff --git a/Documentation/devicetree/bindings/net/can/linux,lin-serdev.yaml b/Documentation/devicetree/bindings/net/can/linux,lin-serdev.yaml > > new file mode 100644 > > index 0000000000000..cb4e932ff249c > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/can/linux,lin-serdev.yaml > > @@ -0,0 +1,29 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/net/can/linux,lin-serdev.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Linux serdev LIN-Bus Support > > This looks like Linux binding, but we expect here description of hardware. OK > > + > > +description: | > > + LIN-Bus support for UART devices equipped with LIN transceivers, > > + utilizing the Serial Device Bus (serdev) interface. > > serdev is Linux thingy, AFAIR. Please describe the hardware. OK, in v2 it will get changed to: """ LIN transceiver, mostly hard-wired to a serial device, used for communication on a LIN bus. """ > > > + > > + For more details on an adapter, visit: https://hexdev.de/hexlin#tty > > + > > +properties: > > + compatible: > > + const: linux,lin-serdev > > Feels confusing. Your link describes real hardware, but you wrote > bindings for software construct. > > If you add this to DT, then it is hard-wired on the board, right? Yes, it is hard-wired. > If so, how this could be a software construct? It's not, but fairly generic, that's why I used 'linux,lin-serdev' as compatible string. In v2 I'll change it to 'hexdev,lin-serdev'. > > + > > +required: > > + - compatible > > + > > +examples: > > + - | > > + &uart2 { > > & does not make much sense here. I think you wanted it to be serial bus, > so serial. OK > > > + status = "okay"; > > Drop, it was not disabled anywhere. OK > > > > + linbus { > > + compatible = "linux,lin-serdev"; > > + }; > > + }; Let me address these points, fix warnings from dt_binding_check and reroll in v2. Thanks -- Christoph