On Mon, 2024-05-06 at 19:24 +0300, Ilpo Järvinen wrote:
> On Thu, 2 May 2024, Christoph Fritz wrote:
>
> > This patch adds a LIN (local interconnect network) bus abstraction on
> > top of CAN. It is a glue driver adapting CAN on one side while offering
> > LIN abstraction on the other side. So that upcoming LIN device drivers
> > can make use of it.
...
> > +static ssize_t lin_identifier_show(struct kobject *kobj,
> > + struct kobj_attribute *attr, char *buf)
> > +{
> > + struct lin_attr *lin_attr = container_of(attr, struct lin_attr, attr);
> > + struct lin_device *ldev = lin_attr->ldev;
> > + ssize_t count = 0;
> > + struct lin_responder_answer answ;
> > + int k, ret;
> > + long id;
> > +
> > + if (!ldev->ldev_ops->get_responder_answer)
> > + return -EOPNOTSUPP;
> > +
> > + ret = kstrtol(attr->attr.name, 16, &id);
> > + if (ret)
> > + return ret;
> > + if (id < 0 || id >= LIN_NUM_IDS)
> > + return -EINVAL;
> > +
> > + count += scnprintf(buf + count, PAGE_SIZE - count,
> > + "%-6s %-11s %-9s %-9s %-2s %-24s %-6s\n",
> > + "state", "cksum-mode", "is_event", "event_id",
> > + "n", "data", "cksum");
>
> Onl use sysfs_emit() and sysfs_emit_at() in *_show functions.
OK
> > +
> > + ret = ldev->ldev_ops->get_responder_answer(ldev, id, &answ);
> > + if (ret)
> > + return ret;
> > +
> > + count += scnprintf(buf + count, PAGE_SIZE - count,
> > + "%-6s %-11s %-9s %-9u %-2u ",
> > + answ.is_active ? "active" : "off",
> > + answ.lf.checksum_mode ? "enhanced" : "classic",
> > + answ.is_event_frame ? "yes" : "no",
> > + answ.event_associated_id,
> > + answ.lf.len);
> > +
> > + for (k = 0; k < answ.lf.len; k++)
> > + count += scnprintf(buf + count, PAGE_SIZE - count,
> > + "%02x ", answ.lf.data[k]);
> > + for (; k < 8; k++)
> > + count += scnprintf(buf + count, PAGE_SIZE - count,
> > + " ");
> > + if (answ.lf.len)
> > + count += scnprintf(buf + count, PAGE_SIZE - count,
> > + " %02x", answ.lf.checksum);
> > +
> > + count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
> > +
> > + return count;
> > +}
...
> > +
> > +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);
> > + return -ENOMEM;
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> Can you use .dev_groups instead ?
I'm not sure where to attach this in this glue code here. Should I do a
class_register() and add the .dev_groups there?
> FWIW, this function doesn't do rollback when error occurs.
OK, this issue can be fixed in revision v4.
...
> > diff --git a/include/net/lin.h b/include/net/lin.h
> > new file mode 100644
> > index 0000000000000..e7c7c820a6e18
> > --- /dev/null
> > +++ b/include/net/lin.h
> > @@ -0,0 +1,92 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */
> > +
> > +#ifndef _NET_LIN_H_
> > +#define _NET_LIN_H_
> > +
> > +#include <linux/bitfield.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 GENMASK(5, 0)
> > +/* special ID descriptions for LIN */
> > +#define LIN_RXOFFLOAD_DATA_FLAG 0x00000200U
> > +#define LIN_ENHANCED_CKSUM_FLAG 0x00000100U
>
> BIT(x) x 2
OK
>
> > +
> > +extern u8 lin_get_id_parity(u8 id);
> > +
> > +#define LIN_GET_ID(PID) FIELD_GET(LIN_ID_MASK, PID)
> > +#define LIN_FORM_PID(ID) (LIN_GET_ID(ID) | \
> > + lin_get_id_parity(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)))
> > +
> > +struct lin_attr {
> > + struct kobj_attribute attr;
> > + struct lin_device *ldev;
> > +};
> > +
> > +struct lin_device {
> > + struct can_priv can; /* must be the first member */
> > + struct net_device *ndev;
> > + struct device *dev;
> > + const struct lin_device_ops *ldev_ops;
> > + struct workqueue_struct *wq;
> > + struct work_struct tx_work;
> > + bool tx_busy;
> > + struct sk_buff *tx_skb;
> > + struct kobject *lin_ids_kobj;
> > + struct lin_attr sysfs_entries[LIN_NUM_IDS];
> > +};
> > +
> > +enum lin_checksum_mode {
> > + LINBUS_CLASSIC = 0,
> > + LINBUS_ENHANCED,
> > +};
> > +
> > +struct lin_frame {
> > + u8 lin_id;
> > + u8 len;
> > + u8 data[LIN_MAX_DLEN];
> > + u8 checksum;
> > + enum lin_checksum_mode checksum_mode;
> > +};
> > +
> > +struct lin_responder_answer {
> > + bool is_active;
> > + bool is_event_frame;
> > + u8 event_associated_id;
> > + struct lin_frame lf;
> > +};
> > +
> > +struct lin_device_ops {
> > + int (*ldo_open)(struct lin_device *ldev);
> > + int (*ldo_stop)(struct lin_device *ldev);
> > + int (*ldo_tx)(struct lin_device *ldev, const struct lin_frame *frame);
> > + int (*update_bitrate)(struct lin_device *ldev, u16 bitrate);
> > + int (*update_responder_answer)(struct lin_device *ldev,
> > + const struct lin_responder_answer *answ);
> > + int (*get_responder_answer)(struct lin_device *ldev, u8 id,
> > + struct lin_responder_answer *answ);
> > +};
> > +
> > +int lin_rx(struct lin_device *ldev, const struct lin_frame *lf);
> > +
> > +u8 lin_get_checksum(u8 pid, u8 n_of_bytes, const u8 *bytes,
> > + enum lin_checksum_mode cm);
> > +
> > +struct lin_device *register_lin(struct device *dev,
> > + const struct lin_device_ops *ldops);
> > +void unregister_lin(struct lin_device *ldev);
> > +
> > +#endif /* _NET_LIN_H_ */
> > diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> > index 02ec32d694742..51b0e2a7624e4 100644
> > --- a/include/uapi/linux/can/netlink.h
> > +++ b/include/uapi/linux/can/netlink.h
> > @@ -103,6 +103,7 @@ struct can_ctrlmode {
> > #define CAN_CTRLMODE_CC_LEN8_DLC 0x100 /* Classic CAN DLC option */
> > #define CAN_CTRLMODE_TDC_AUTO 0x200 /* CAN transiver automatically calculates TDCV */
> > #define CAN_CTRLMODE_TDC_MANUAL 0x400 /* TDCV is manually set up by user */
>
> BIT(x) is these days available also for uapi I think.
>
> > +#define CAN_CTRLMODE_LIN 0x800 /* LIN bus mode */
So, should I use just BIT(11) for the new define, or should I also
refactor the whole list while at it?
Thanks
-- Christoph
