Re: [V2 PATCH 03/10] added media agnostic (MA) data structures and handling

2014-11-11 Thread steph 
On Wed, Nov 12, 2014 at 10:14:38AM +0900, Greg KH wrote:
 On Tue, Nov 11, 2014 at 02:42:22PM -0800, Sean O. Stalley wrote:
  On Tue, Nov 11, 2014 at 01:38:21PM +0900, Greg KH wrote:
   On Mon, Nov 10, 2014 at 06:09:34PM -0800, Stephanie Wallick wrote:
   Yes, I am holding you to a higher standard than staging code normally
   is, and yes, it is purely because of the company you work for.  But I
   only do that because your company knows how to do this stuff right, and
   you have access to the resources and talent to help make this code
   right.  Other people and companies do not have the kind of advantage
   that you do.
  
  We know we are fortunate to work for a company with so much talent and
  resources and we don't mind being held to a higher standard. We have been
  receiving multiple requests for our host driver and wanted to make it
  available as soon as possible for others to use. We thought putting our
  host driver into staging would be a good way to release it, but realize now
  that it was premature. 
 
 Does the code even work?  The number of basic mistakes in it seems to
 imply that it doesn't, but I could be mistaken.
 

What is there works, but not everything in the MA USB Spec has been implemented
yet. 

  We won't resubmit the driver until a senior kernel developer has signed off 
  on it.
 
 Good, go kick some of them and get them to review the code, _after_ at
 least addressing the issues that the community has raised, you don't
 want to waste their time finding the same things we just did :)
 

Will do! We will most definitely fix the issues raised by the community before
having anyone else review the code. We appreciate everyone's time and feedback
:)


Thanks,
Stephanie
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/10] added media agnostic (MA) USB HCD driver

2014-11-03 Thread steph 
On Mon, Nov 03, 2014 at 01:18:16PM -0800, Greg KH wrote:
 On Mon, Nov 03, 2014 at 12:42:48PM -0800, Stephanie Wallick wrote:
  +#define DEBUG
 
 I doubt you want this in the driver enabled by default :(
 

Thank you for catching, will remove in the next patch version.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/10] added media agnostic (MA) USB HCD driver

2014-11-03 Thread steph 
On Mon, Nov 03, 2014 at 01:21:39PM -0800, Greg KH wrote:
 On Mon, Nov 03, 2014 at 12:42:48PM -0800, Stephanie Wallick wrote:
  +EXPORT_SYMBOL(mausb_register_ms_driver);
 
 EXPORT_SYMBOL_GPL()?  I have to ask...

The source is dual-licenced under BSD and GPL. It was our understanding
that dual-licensed should use EXPORT_SYMBOL() instead. Is that wrong?
 
  +static int mausb_hcd_init(void)
  +{
  +   int ret;
  +
  +   /* register HCD driver */
  +   ret = platform_driver_register(mausb_driver);
 
 Why is this a platform driver?  How does this relate to platform
 hardware?
 
The driver doesn't require platform resources. It looks like a host
controller driver but communicates over the network instead of to
a physical host controller. There is no MA USB-specific hardware.

Should we use a struct device instead of a struct platform_device?

  +   if (ret  0) {
  +   printk(KERN_DEBUG %s: failed to register HC driver: 
  +error number %d\n, __func__, ret);
 
 pr_err()?

Will change all printk() to pr_err() in next patch.
 
 return here, that way you don't need:
 
  +   } else {
 
 This indentation.
 
Will fix in next patch.

  +   /* register HCD device */
  +   ret = platform_device_register(mausb_pdev);
 
 But again, why is this a platform device?  What platform resources does
 it have / require?
 
See above.

  +
  +   if (ret  0) {
  +   printk(KERN_DEBUG %s: failed to register HC device:
  +   error number %d\n, __func__, ret);
 
 pr_err()?
 
See above.

  +   platform_driver_unregister(mausb_driver);
  +   } else {
  +   /* direct the release function (for exiting) */
  +   mausb_pdev.dev.release = mausb_dev_release;
 
 That seems like a serious hack, why do you need to do this in this
 manner?
 
This will go away when we get rid of the platform device.

  +
  +   if (ret  0) {
  +   printk(KERN_DEBUG failed to register HC
  +chardev: error number %d\n, ret);
 
 pr_err()?

See above.
 
 thanks,
 
 greg k-h

Thanks,
Stephanie
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/10] added media agnostic (MA) USB HCD driver

2014-11-03 Thread steph 
On Mon, Nov 03, 2014 at 04:13:55PM -0800, Greg KH wrote:
 On Mon, Nov 03, 2014 at 04:04:42PM -0800, steph wrote:
  On Mon, Nov 03, 2014 at 01:21:39PM -0800, Greg KH wrote:
   On Mon, Nov 03, 2014 at 12:42:48PM -0800, Stephanie Wallick wrote:
+EXPORT_SYMBOL(mausb_register_ms_driver);
   
   EXPORT_SYMBOL_GPL()?  I have to ask...
  
  The source is dual-licenced under BSD and GPL. It was our understanding
  that dual-licensed should use EXPORT_SYMBOL() instead. Is that wrong?
 
 Talk to your company lawyers please to get confirmation of what you want
 to do here, I can't answer this question, I just have to ask...

We have permission to go forward with the dual BSD/GPL license. I will leave
as is unless there is a future issue.
 
+static int mausb_hcd_init(void)
+{
+   int ret;
+
+   /* register HCD driver */
+   ret = platform_driver_register(mausb_driver);
   
   Why is this a platform driver?  How does this relate to platform
   hardware?
   
  The driver doesn't require platform resources. It looks like a host
  controller driver but communicates over the network instead of to
  a physical host controller. There is no MA USB-specific hardware.
  
  Should we use a struct device instead of a struct platform_device?
 
 Yes, please make it a virtual device.
 
Will do.

Thanks,
Stephanie

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel