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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
