Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver

From: Greg KH <gregkh@linuxfoundation.org>
To: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Even Xu <even.xu@intel.com>,
	jikos@kernel.org, benjamin.tissoires@redhat.com, arnd@arndb.de,
	andriy.shevchenko@intel.com, linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
Date: Wed, 4 Jan 2017 18:18:53 +0100	[thread overview]
Message-ID: <20170104171853.GA23715@kroah.com> (raw)
In-Reply-To: <1483549894.4497.55.camel@linux.intel.com>

On Wed, Jan 04, 2017 at 09:11:34AM -0800, Srinivas Pandruvada wrote:
> On Wed, 2017-01-04 at 14:03 +0100, Greg KH wrote:
> > On Fri, Dec 23, 2016 at 09:22:29AM +0800, Even Xu wrote:
> > > 
> > > Intel ISHFW supports many different clients, in
> > > hid/intel-ish-hid/ishtp bus driver, it creates following client
> > > devices:
> > > HID client:
> > > 	interface of sensor configure and sensor event report.
> > > SMHI client:
> > > 	interface of sensor calibration, ISHFW debug, ISHFW performance
> > > 	analysis and manufacture support.
> > > Trace client:
> > > 	interface of ISHFW debug log output.
> > > Trace configure client:
> > > 	interface of ISHFW debug log configuration, such as output
> > > port,
> > > 	log level, filter.
> > > ISHFW loader client:
> > > 	interface of customized ISHFW loader.
> > > HID client has been handle by hid/intel-ish-hid/intel-ishtp-hid
> > > client
> > > driver, and rest of the clients export interface using
> > > miscellaneous
> > > drivers. This interface is used by user space tools for debugging
> > > and
> > > calibration of sensors.
> > > 
> > > Signed-off-by: Even Xu <even.xu@intel.com>
> > > Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
> > > Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.c
> > > om>
> > > ---
> > >  drivers/misc/Kconfig                               |   1 +
> > >  drivers/misc/Makefile                              |   1 +
> > >  drivers/misc/intel-ish-client/Kconfig              |  15 +
> > >  drivers/misc/intel-ish-client/Makefile             |   8 +
> > >  .../misc/intel-ish-client/intel-ishtp-clients.c    | 884
> > > +++++++++++++++++++++
> > >  include/uapi/linux/intel-ishtp-clients.h           |  73 ++
> > 
> > 
> > Why create a whole new subdirectory for just one .c file?  Is that
> > really needed?
> The other option is to move this .c file to drivers/hid/intel-ish-hid/.
> I think the folders inside drivers/hid/ is mostly for just implementing
> transport layer for hid devices.
> 
> 
> > 
> > And I'm not quite sure why you need a misc driver, what exactly is
> > this
> > code doing?
> As described in the description, this driver is a companion driver for
> ISH user space tools for calibration, production and debug.

debug should not require a char device node, use debugfs, that is what
it is there for.

For "calibration", why not use configfs or even sysfs?

> Basically the ISH provided a standalone low power processor to
> developers and manufacturers  to do download some custom algorithms for
> sensors, which may not be compliant to USB HID sensor specifications
> (mostly for IOT space). In that case the user space for those can
> communicate using misc driver interface, without adding new kernel
> drivers.

So you hide it behind a char device node?  That's not very descriptive
or easy to understand :)

Again, use the interfaces that the kernel gives you for this type of
stuff, and don't create new one-off ones if at all possible.

thanks,

greg k-h