Re: [PATCH RFC] drivers: most: add USB adapter driver

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, May 11, 2020 at 02:46:58PM +0000, Christian.Gromm@microchip.com 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@microchip.com>
> > > ---
> > >  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@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel