[U-Boot,v4,3/3] usb:udc:samsung Add functions for storing private gadget data in UDC driver

This commit adds support for storing private data to Samsung's UDC
driver. This data is afterward used by usb gadget.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Marek Vasut <marex@denx.de>
---
 drivers/usb/gadget/s3c_udc_otg.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

Dear Lukasz Majewski,

> This commit adds support for storing private data to Samsung's UDC
> driver. This data is afterward used by usb gadget.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> ---
>  drivers/usb/gadget/s3c_udc_otg.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/gadget/s3c_udc_otg.c
> b/drivers/usb/gadget/s3c_udc_otg.c index f7f7b54..925d2f2 100644
> --- a/drivers/usb/gadget/s3c_udc_otg.c
> +++ b/drivers/usb/gadget/s3c_udc_otg.c
> @@ -133,6 +133,18 @@ static void nuke(struct s3c_ep *ep, int status);
>  static int s3c_udc_set_halt(struct usb_ep *_ep, int value);
>  static void s3c_udc_set_nak(struct s3c_ep *ep);
> 
> +void set_udc_gadget_private_data(void *p)
> +{
> +	DEBUG_SETUP("%s: the_controller: 0x%p, p: 0x%p\n", __func__,
> +	       the_controller, p);

debug() and fix this message, otherwise:

Acked-by: Marek Vasut <marex@denx.de>

Damn, I shouldn't have avoided reviewing them if I knew the remaining ones were 
so short. But after the first one, I was quite scared of what's coming next. 
Either way, here you go, fix these few issues and you're in mainline ;-)

> +	the_controller->gadget.dev.device_data = p;
> +}
> +
> +void *get_udc_gadget_private_data(struct usb_gadget *gadget)
> +{
> +	return gadget->dev.device_data;
> +}
> +
>  static struct usb_ep_ops s3c_ep_ops = {
>  	.enable = s3c_ep_enable,
>  	.disable = s3c_ep_disable,

Best regards,
Marek Vasut

Hi Marek,

> Dear Lukasz Majewski,
> 
> > This commit adds support for storing private data to Samsung's UDC
> > driver. This data is afterward used by usb gadget.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Marek Vasut <marex@denx.de>
> > ---
> >  drivers/usb/gadget/s3c_udc_otg.c |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/s3c_udc_otg.c
> > b/drivers/usb/gadget/s3c_udc_otg.c index f7f7b54..925d2f2 100644
> > --- a/drivers/usb/gadget/s3c_udc_otg.c
> > +++ b/drivers/usb/gadget/s3c_udc_otg.c
> > @@ -133,6 +133,18 @@ static void nuke(struct s3c_ep *ep, int
> > status); static int s3c_udc_set_halt(struct usb_ep *_ep, int value);
> >  static void s3c_udc_set_nak(struct s3c_ep *ep);
> > 
> > +void set_udc_gadget_private_data(void *p)
> > +{
> > +	DEBUG_SETUP("%s: the_controller: 0x%p, p: 0x%p\n",
> > __func__,
> > +	       the_controller, p);
> 
> debug() and fix this message, otherwise:

The DEBUG_SETUP macro has been used to be in sync with the already
available udc driver. This driver has different DEBUG_* macros, which
helps in debugging different parts of UDC driver. 

If this is MUST, then I will change it, otherwise I'd like to leave it
alone.

Is it OK with you?

> 
> Acked-by: Marek Vasut <marex@denx.de>

Thanks, please pull them to your u-boot-usb tree.
(and also the patch:
http://patchwork.ozlabs.org/patch/151983/
is also acked-by)


> 
> Damn, I shouldn't have avoided reviewing them if I knew the remaining
> ones were so short. But after the first one, I was quite scared of
> what's coming next. Either way, here you go, fix these few issues and
> you're in mainline ;-)
> 
> > +	the_controller->gadget.dev.device_data = p;
> > +}
> > +
> > +void *get_udc_gadget_private_data(struct usb_gadget *gadget)
> > +{
> > +	return gadget->dev.device_data;
> > +}
> > +
> >  static struct usb_ep_ops s3c_ep_ops = {
> >  	.enable = s3c_ep_enable,
> >  	.disable = s3c_ep_disable,

Dear Lukasz Majewski,

> Hi Marek,
> 
> > Dear Lukasz Majewski,
> > 
> > > This commit adds support for storing private data to Samsung's UDC
> > > driver. This data is afterward used by usb gadget.
> > > 
> > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > Cc: Marek Vasut <marex@denx.de>
> > > ---
> > > 
> > >  drivers/usb/gadget/s3c_udc_otg.c |   12 ++++++++++++
> > >  1 files changed, 12 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/usb/gadget/s3c_udc_otg.c
> > > b/drivers/usb/gadget/s3c_udc_otg.c index f7f7b54..925d2f2 100644
> > > --- a/drivers/usb/gadget/s3c_udc_otg.c
> > > +++ b/drivers/usb/gadget/s3c_udc_otg.c
> > > @@ -133,6 +133,18 @@ static void nuke(struct s3c_ep *ep, int
> > > status); static int s3c_udc_set_halt(struct usb_ep *_ep, int value);
> > > 
> > >  static void s3c_udc_set_nak(struct s3c_ep *ep);
> > > 
> > > +void set_udc_gadget_private_data(void *p)
> > > +{
> > > +	DEBUG_SETUP("%s: the_controller: 0x%p, p: 0x%p\n",
> > > __func__,
> > > +	       the_controller, p);
> > 
> > debug() and fix this message, otherwise:
> The DEBUG_SETUP macro has been used to be in sync with the already
> available udc driver. This driver has different DEBUG_* macros, which
> helps in debugging different parts of UDC driver.

Ok, so be it.

> If this is MUST, then I will change it, otherwise I'd like to leave it
> alone.
> 
> Is it OK with you?

It's fine then, thanks for clearing this.

> 
> > Acked-by: Marek Vasut <marex@denx.de>
> 
> Thanks, please pull them to your u-boot-usb tree.
> (and also the patch:
> http://patchwork.ozlabs.org/patch/151983/
> is also acked-by)

Will do, thanks for pointing out the other patch :) One more time, sorry for the 
delay.

> 
> > Damn, I shouldn't have avoided reviewing them if I knew the remaining
> > ones were so short. But after the first one, I was quite scared of
> > what's coming next. Either way, here you go, fix these few issues and
> > you're in mainline ;-)
> > 
> > > +	the_controller->gadget.dev.device_data = p;
> > > +}
> > > +
> > > +void *get_udc_gadget_private_data(struct usb_gadget *gadget)
> > > +{
> > > +	return gadget->dev.device_data;
> > > +}
> > > +
> > > 
> > >  static struct usb_ep_ops s3c_ep_ops = {
> > >  
> > >  	.enable = s3c_ep_enable,
> > >  	.disable = s3c_ep_disable,

Best regards,
Marek Vasut

Hi Marek,

> >   
> > > Acked-by: Marek Vasut <marex@denx.de>  
> > 
> > Thanks, please pull them to your u-boot-usb tree.
> > (and also the patch:
> > http://patchwork.ozlabs.org/patch/151983/
> > is also acked-by)  
> 
> Will do, thanks for pointing out the other patch :) One more time,
> sorry for the delay.

No problem :-)

Dear Lukasz Majewski,

In message <20120430085801.4fe5af09@lmajewski.digital.local> you wrote:
> 
> > > +void set_udc_gadget_private_data(void *p)
> > > +{
> > > +	DEBUG_SETUP("%s: the_controller: 0x%p, p: 0x%p\n",
> > > __func__,
> > > +	       the_controller, p);
> > 
> > debug() and fix this message, otherwise:
> 
> The DEBUG_SETUP macro has been used to be in sync with the already
> available udc driver. This driver has different DEBUG_* macros, which
> helps in debugging different parts of UDC driver. 

I think Marek has a good point here.  It was an oversight that this
"private" DEBUG_ stuff slipped into mainline.  This should never have
happened.  We tried hard to get rid of such conditionally compiled
code for debug() with the rest of the code, so we should not start
re-adding all this again.

> If this is MUST, then I will change it, otherwise I'd like to leave it
> alone.
> 
> Is it OK with you?

Sorry, but I object.

At the moment, only include/usb/s3c_udc.h defines this, i. e. it is
not a generally usable feature anyway.  In anyu case, this
implementation needs to get fixed.  See the code for the debug()
implementation for an example.

Instead of defining your own set of private macros, you can use
debug_cond() instead - this works without #ifdef's.

Thanks.

Best regards,

Wolfgang Denk

On Mon, 30 Apr 2012 15:38:31 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Lukasz Majewski,
> 
> In message <20120430085801.4fe5af09@lmajewski.digital.local> you
> wrote:
> > 
> > > > +void set_udc_gadget_private_data(void *p)
> > > > +{
> > > > +	DEBUG_SETUP("%s: the_controller: 0x%p, p: 0x%p\n",
> > > > __func__,
> > > > +	       the_controller, p);
> > > 
> > > debug() and fix this message, otherwise:
> > 
> > The DEBUG_SETUP macro has been used to be in sync with the already
> > available udc driver. This driver has different DEBUG_* macros,
> > which helps in debugging different parts of UDC driver. 
> 
> I think Marek has a good point here.  It was an oversight that this
> "private" DEBUG_ stuff slipped into mainline.  This should never have
> happened.  We tried hard to get rid of such conditionally compiled
> code for debug() with the rest of the code, so we should not start
> re-adding all this again.
> 
> > If this is MUST, then I will change it, otherwise I'd like to leave
> > it alone.
> > 
> > Is it OK with you?
> 
> Sorry, but I object.

So I will change this patch accordingly and replace DEBUG_SETUP with
debug macro.

> 
> At the moment, only include/usb/s3c_udc.h defines this, i. e. it is
> not a generally usable feature anyway.  In anyu case, this
> implementation needs to get fixed.  See the code for the debug()
> implementation for an example.
> 
> Instead of defining your own set of private macros, you can use
> debug_cond() instead - this works without #ifdef's.
>