Re: [alsa-devel] [PATCHv2 5/6] ASoC: OMAP4: Add support for McPDM

26 Jan 2010

      On Mon, 2010-01-25 at 15:06 -0600, Candelaria Villareal, Jorge wrote:
...
Liam Girdwood wrote:
...
On Fri, 2010-01-22 at 17:15 -0600, Candelaria Villareal, Jorge wrote:
...
McPDM is the interface between Phoenix audio codec
and the OMAP4430 processor. It enables data to be transfered
to/from Phoenix at sample rates of 88.4 or 96 KHz.
Signed-off-by: Jorge Eduardo Candelaria x0107209@ti.com
Signed-off-by: Margarita Olaya x0080101@ti.com
It initially looks like a some of this can be called directly
as DAI ops
rather than by the machine driver.
Could you explain a little further?
I was meaning here that some of the functions below are only called by
higher level Digital Audio Interface (DAI) operations. e.g. the
following is called by capture stream close :-
...
...
...

Cleans McPDM uplink configuration.

This function should be called when the stream is closed.

*/

+int omap_mcpdm_clr_uplink(struct omap_mcpdm_link *uplink)
Would probably be more meaningful wrt ALSA as
omap_mcpdm_capture_close()
...
...
...
+/*

Takes the McPDM module in and out of reset state.

Uplink and downlink can be reset individually.

*/

+static void omap_mcpdm_reset(int links, int reset)
+{

  int ctrl = omap_mcpdm_read(MCPDM_CTRL);

  if (links & MCPDM_UPLINK) {

          if (reset)

                  ctrl |= SW_UP_RST;

          else

                  ctrl &= ~SW_UP_RST;

  }

  if (links & MCPDM_DOWNLINK) {

          if (reset)

                  ctrl |= SW_DN_RST;

          else

                  ctrl &= ~SW_DN_RST;

  }

  omap_mcpdm_write(MCPDM_CTRL, ctrl);

+}

Would it not be better to rename uplink/downlink as playback
and capture
for ALSA ? If so, you could have an inline playback and
capture version
of this function.
Data paths in McPDM module are named uplink/downlink, so these
names were chosen to be consistent. Is renaming it according
to ALSA the best approach?
Generally yes, as long as we can see the audio direction easily (a
comment would do here).
Fwiw, I would have written the above as :-
static inline void omap_mcpdm_reset_playback(int reset)
{
    int ctrl = omap_mcpdm_read(MCPDM_CTRL);
if (reset)
    	ctrl |= SW_UP_RST;
    else
    	ctrl &= ~SW_UP_RST;

    omap_mcpdm_write(MCPDM_CTRL, ctrl);
}
and one for capture reset too.
...
...
...
+int omap_mcpdm_set_offset(int offset1, int offset2)
+{

  int offset;

  if ((offset1 > DN_OFST_MAX) || (offset2 > DN_OFST_MAX))

          return -EINVAL;

  offset = (offset1 << DN_OFST_RX1) | (offset2 <<

DN_OFST_RX2);
...

  /* Enable/disable offset cancellation for downlink

channel 1 */
...

  if (offset1)

          offset |= DN_OFST_RX1_EN;

  else

          offset &= ~DN_OFST_RX1_EN;

  /* Enable/disable offset cancellation for downlink

channel 2 */
...

  if (offset2)

          offset |= DN_OFST_RX2_EN;

  else

          offset &= ~DN_OFST_RX2_EN;

  omap_mcpdm_write(MCPDM_DN_OFFSET, offset);

  return 0;

+}
What does this do and why is it not static ?
It enables and configures offset cancelation for the analog headset path. It is supposed to be called by the codec driver, so it should'nt be static. But, offset cancelation is probably not going to be used at first.
Ok, can we a comment to here to describe this.
Thanks
Liam