On Mon, May 11, 2020 at 02:46:58PM +0000, Christian.Gromm@xxxxxxxxxxxxx wrote: > On Mon, 2020-05-11 at 13:47 +0200, Greg KH wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > On Mon, May 11, 2020 at 11:51:15AM +0200, Christian Gromm wrote: > > > This patch adds the MOST USB adapter driver to the stable branch. > > > This is > > > a follow-up to commit <b276527>. > > > > I do not understand the "a follow-up..." sentance. Always use the > > format of: > > b27652753918 ("staging: most: move core files out of the > > staging area") > > when writing kernel commits in changelogs. > > > > Also, that commit doesn't really mean anything here, this is a > > stand-alone driver for the most subsystem. This changelog needs > > work. > > Purpose was sharing the information that this is patch is > only one part of moving the complete driver stack. That a > first step has alread been done and others are to follow. > But you're probably right and nobody realy needs to know. > > I'll skip this. > > > > > > Signed-off-by: Christian Gromm <christian.gromm@xxxxxxxxxxxxx> > > > --- > > > drivers/most/Kconfig | 6 + > > > drivers/most/Makefile | 2 + > > > drivers/most/usb/Kconfig | 14 + > > > drivers/most/usb/Makefile | 4 + > > > drivers/most/usb/usb.c | 1262 > > > +++++++++++++++++++++++++++++++++++++++++ > > > > Why not just call this file most-usb.c so you don't have to do the > > 2-step Makefile work. Also, why a whole subdir for a single .c file? > > To keep the staging layout. No need to do that, this is a new layout :) > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > You shouldn't need any pr_*() calls because this is a driver and you > > always have access to the struct device * it controls. So drop this > > and > > fix up the remaining pr_*() calls to be dev_*() instead. > > There are helper functions that actually don't have access to the > struct device and it felt like an overhead to pass the device > pointer just for logging purposes. pr_* calls show almost nothing when it comes to the actual device/driver being affected. That's why the dev_*() functions are there, please use them. > > > +/** > > > + * struct most_dci_obj - Direct Communication Interface > > > + * @kobj:position in sysfs > > > + * @usb_device: pointer to the usb device > > > + * @reg_addr: register address for arbitrary DCI access > > > + */ > > > +struct most_dci_obj { > > > + struct device dev; > > > > Wait, why is a USB driver creating something with a separate struct > > device embedded in it? Shouldn't the most core handle stuff like > > this? > > The driver adds an ABI interface that belongs to USB only. This keeps > the core generic. So this same type of thing is also needed in the other bus controllers (serial, i2c, etc.)? Creating a new device implies it lives on a bus, and almost always the bus code for creating/managing that code lives in a single place, not in the individual drivers. Why doesn't the most core handle this? What does the most core do? :) > > > +static DEVICE_ATTR(arb_address, 0644, value_show, value_store); > > > +static DEVICE_ATTR(arb_value, 0644, value_show, value_store); > > > > Loads of sysfs files with no documentation for them? > > > > see driver/staging/most/Documentation Add it as part of this patch series, as you are moving these sysfs files into the "real" part of the kernel and belong out of drivers/staging/ thanks, greg k-h