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

[Xu, Even]

Hi, Greg,

Thanks for your review and suggestion, I will rework my patch based on your suggestion and then submit again.

Best Regards,
Even Xu

-----Original Message-----
From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] 
Sent: Thursday, January 5, 2017 3:41 AM
To: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
Cc: Xu, Even <even.xu@xxxxxxxxx>; jikos@xxxxxxxxxx; benjamin.tissoires@xxxxxxxxxx; arnd@xxxxxxxx; Shevchenko, Andriy <andriy.shevchenko@xxxxxxxxx>; linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver

On Wed, Jan 04, 2017 at 10:41:26AM -0800, Srinivas Pandruvada wrote:
> On Wed, 2017-01-04 at 18:18 +0100, Greg KH wrote:
> > 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
> > > > > 
> [...]
> 
> > 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?
> 
> We will check on this. There is some legacy with the deployed user 
> space tools.

Um, you do know that's not a good reason/excuse at all to take incorrect kernel code, right?  Please don't use that as any kind of excuse.

> > > 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 :)
> We added several new sensors to IIO and in process of adding new 
> sensors to standardize ABI for sensors defined in HID sensor spec.
> 
> Customers can develop and download some algorithm which uses output of 
> several sensors and come up with some fusion sensor to detect some 
> activity. Either some kernel driver needs to read this and pass this 
> event to user space or directly let the user space communicate with 
> the firmware using character device.
> Is there any better way to handle this?
> 
> We want customers to use upstream kernel without out of tree kernel 
> drivers.

Why are you somehow claiming this is an either/or kind of situation?
What out-of-tree kernel modules are there?  Why can't they just be merged if they are somewhere?

Having an interface to add new types of "functionality" is great, and fine, but why you think a char device node is that type of api is confusing to me when I already pointed out an number of other potential solutions.  Have you tried them out and found they do not work?  If so, great, please explain what is lacking and we can go from there.

If not, please do some basic research first before trying to claim that a char device is the only possible solution.

You have run this code through the internal Intel kernel developer review process on their mailing list, correct?  What did they say about your current design?  If not, why have you not taken advantage of this resource?

thanks,

greg k-h