On Wed, 8 May 2024, Christoph Fritz wrote: > 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 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? I guess struct class would be correct direction but I'm not sure if it's viable in this case. It would avoid the need for custom sysfs setup code if it's workable. > > FWIW, this function doesn't do rollback when error occurs. > > OK, this issue can be fixed in revision v4. > > ... > > > 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? Either is fine for me. -- i.