Re: [PATCH v9 26/34] ASoC: qcom: qdsp6: q6afe: Split USB AFE dev_token param into separate API

[Wesley Cheng]

Hi Pierre,

On 10/17/2023 3:39 PM, Pierre-Louis Bossart wrote:
> On 10/17/23 15:01, Wesley Cheng wrote:
> > The Q6USB backend can carry information about the available USB SND cards
> > and PCM devices discovered on the USB bus.  The dev_token field is used by
> > the audio DSP to notify the USB offload driver of which card and PCM index
> > to enable playback on.  Separate this into a dedicated API, so the USB
> > backend can set the dev_token accordingly.  The audio DSP does not utilize
> > this information until the AFE port start command is sent, which is done
> > during the PCM prepare phase.
> >
> > Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> > ---
> >   sound/soc/qcom/qdsp6/q6afe.c | 49 +++++++++++++++++++++++++-----------
> >   sound/soc/qcom/qdsp6/q6afe.h |  1 +
> >   2 files changed, 36 insertions(+), 14 deletions(-)
> >
> > diff --git a/sound/soc/qcom/qdsp6/q6afe.c b/sound/soc/qcom/qdsp6/q6afe.c
> > index 72c4e6fe20c4..f09a756246f8 100644
> > --- a/sound/soc/qcom/qdsp6/q6afe.c
> > +++ b/sound/soc/qcom/qdsp6/q6afe.c
> > @@ -1394,10 +1394,42 @@ void q6afe_tdm_port_prepare(struct q6afe_port *port,
> >   }
> >   EXPORT_SYMBOL_GPL(q6afe_tdm_port_prepare);
> >   
> > -static int afe_port_send_usb_dev_param(struct q6afe_port *port, struct q6afe_usb_cfg *cfg)
> > +/**
> > + * afe_port_send_usb_dev_param() - Send USB dev token
> > + *
> > + * @port: Instance of afe port
> > + * @cardidx: USB SND card index to reference
> > + * @pcmidx: USB SND PCM device index to reference
> > + *
> > + * The USB dev token carries information about which USB SND card instance and
> > + * PCM device to execute the offload on.  This information is carried through
> > + * to the stream enable QMI request, which is handled by the offload class
> > + * driver.  The information is parsed to determine which USB device to query
> > + * the required resources for.
> > + */
> > +int afe_port_send_usb_dev_param(struct q6afe_port *port, int cardidx, int pcmidx)
> >   {
> > -	union afe_port_config *pcfg = &port->port_cfg;
> >   	struct afe_param_id_usb_audio_dev_params usb_dev;
> > +	int ret;
> > +
> > +	memset(&usb_dev, 0, sizeof(usb_dev));
> > +
> > +	usb_dev.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG;
> > +	usb_dev.dev_token = (cardidx << 16) | (pcmidx << 8);
> > +	ret = q6afe_port_set_param_v2(port, &usb_dev,
> > +				AFE_PARAM_ID_USB_AUDIO_DEV_PARAMS,
> > +				AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(usb_dev));
> > +	if (ret)
> > +		dev_err(port->afe->dev, "%s: AFE device param cmd failed %d\n",
> > +			__func__, ret);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(afe_port_send_usb_dev_param);
> > +
> > +static int afe_port_send_usb_params(struct q6afe_port *port, struct q6afe_usb_cfg *cfg)
> > +{
> > +	union afe_port_config *pcfg = &port->port_cfg;
> >   	struct afe_param_id_usb_audio_dev_lpcm_fmt lpcm_fmt;
> >   	struct afe_param_id_usb_audio_svc_interval svc_int;
> >   	int ret = 0;
> > @@ -1408,20 +1440,9 @@ static int afe_port_send_usb_dev_param(struct q6afe_port *port, struct q6afe_usb
> >   		goto exit;
> >   	}
> >   
> > -	memset(&usb_dev, 0, sizeof(usb_dev));
> >   	memset(&lpcm_fmt, 0, sizeof(lpcm_fmt));
> >   	memset(&svc_int, 0, sizeof(svc_int));
> >   
> > -	usb_dev.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG;
> > -	ret = q6afe_port_set_param_v2(port, &usb_dev,
> > -				      AFE_PARAM_ID_USB_AUDIO_DEV_PARAMS,
> > -				      AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(usb_dev));
> > -	if (ret) {
> > -		dev_err(port->afe->dev, "%s: AFE device param cmd failed %d\n",
> > -			__func__, ret);
> > -		goto exit;
> > -	}
> > -
>
> this feels like a questionable patch split. Why not introduce the new
> helper earlier and avoid adding code then modifying the same code?

Let me see if I can squash this with the change that adds the USB AFE port.

Thanks
Wesley Cheng