On Mon, 2020-05-11 at 18:33 +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 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? :)
The core module manages the buffers, routing, configuration,
sysfs/configfs and user space interface (via its component modules)
for common communication channels. The DCI interface is only available
when the hardware is connected via USB. Other connections do not
provide this.
That's why I want the module that actually introduces such an
interface (and has all the necessary information about it) to be in
charge. This keeps the core code simpler, as USB isn't always used.
Also, a new device is needed to create the desired sysfs layout,
in which the dci interface is represented with a new sub directory.
thanks,
Chris
