RE: [PATCH] ANDROID: sound: usb: Add vendor's hooking interface

[정재훈 <jh0801.jung@samsung.com>]

> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
> owner@vger.kernel.org] On Behalf Of Greg KH
> Sent: Wednesday, June 17, 2020 4:53 PM
> To: JaeHun Jung
> Cc: linux-usb@vger.kernel.org
> Subject: Re: [PATCH] ANDROID: sound: usb: Add vendor's hooking interface
> 
> On Wed, Jun 17, 2020 at 07:17:38AM +0200, Greg KH wrote:
> > On Wed, Jun 17, 2020 at 10:55:30AM +0900, JaeHun Jung wrote:
> > > In mobile, a co-processor is used when using USB audio to improve
> > > power consumption.
> > > hooking is required for sync-up when operating the co-processor. So
> > > register call-back function.
> > > The main operation of the call-back function is as follows:
> > > - Initialize the co-processor by transmitting data
> > >   when initializing.
> > > - Change the co-processor setting value through
> > >   the interface function.
> > > - Configure sampling rate
> > > - pcm open/close
> > >
> > > Bug: 156315379
> > >
> > > Change-Id: I32e1dd408e64aaef68ee06c480c4b4d4c95546dc
> >
> > No need for Bug or Change-Id on patches submitted to us, same for the
> > odd "ANDROID:" in the subject.
> >

Ok, I will delete it.

> > > Signed-off-by: JaeHun Jung <jh0801.jung@samsung.com>
> > > ---
> > >  sound/usb/card.c     | 16 ++++++++++++++++
> > >  sound/usb/card.h     |  1 +
> > >  sound/usb/clock.c    |  5 +++++
> > >  sound/usb/pcm.c      | 33 +++++++++++++++++++++++++++++++++
> > >  sound/usb/usbaudio.h | 30 ++++++++++++++++++++++++++++++
> > >  5 files changed, 85 insertions(+)
> >
> > Did you run scripts/get_maintainer.pl on this patch to determine that
> > maybe the alsa-devel list should also be needed?
> >

Yes, it was sent looking for maintainer of sound/usb.
This callbacks is for sync with Audio Core.
So, I was implement on sound/usb.

> >
> >
> > >
> > > diff --git a/sound/usb/card.c b/sound/usb/card.c index
> > > fd6fd17..2f3fa14 100644
> > > --- a/sound/usb/card.c
> > > +++ b/sound/usb/card.c
> > > @@ -111,6 +111,7 @@ MODULE_PARM_DESC(skip_validation, "Skip unit
> > > descriptor validation (default: no)  static
> > > DEFINE_MUTEX(register_mutex);  static struct snd_usb_audio
> > > *usb_chip[SNDRV_CARDS];  static struct usb_driver usb_audio_driver;
> > > +struct snd_usb_audio_vendor_ops *usb_audio_ops;
> > >
> > >  /*
> > >   * disconnect streams
> > > @@ -210,6 +211,12 @@ static int snd_usb_create_stream(struct
> snd_usb_audio *chip, int ctrlif, int int
> > >  	return 0;
> > >  }
> > >
> > > +void snd_set_vender_interface(struct snd_usb_audio_vendor_ops
> > > +*vendor_ops) {
> > > +	usb_audio_ops = vendor_ops;
> > > +}
> > > +EXPORT_SYMBOL_GPL(snd_set_vender_interface);
> >
> > You are exporting a lot of new symbols, but you have no user of these
> > symbols, which is not allowed, as you know.  Please also post your
> > user of them so we can see if you are doing things correctly or not.
> >

Yes, I know.
This is called from Audio core module.
Audio related drivers associated with this module cannot disclose because
of security.
I think this is true of other vendors as well.

> > Also, only one set of "vendor ops" does not make any sense at all,
> > this needs to be on a per-host-controller basis, right?  If so, why is
> > this all in the sound driver?
> 

Currently, this interface is only for USB audio. USB information is that is
has in the xhci host driver.
When USB audio is connected, F/W of audio core performs the control of USB
host for low power.

> Also, your api is making a lot of assumptions about the running system,
> there seems to not be any way to always "know" what bus/device the
> callbacks are being used for in many places.

This is only used in limited scenarios. And the information of USB host get
through from exynos_usb_audio driver.

> 
> Why not just add the needed functionality to the sound driver itself
> instead of trying to rely on these odd "callbacks"?

Audio core operates in F/W and is module.
Because there are many connected modules, it is cannot on built-in and
module to implement the non-callbacks

> 
> thanks,
> 
> greg k-h