Liam Girdwood wrote:
>
> 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@xxxxxx>
> > > > Signed-off-by: Margarita Olaya <x0080101@xxxxxx>
> > >
> > > 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()
Ok, I think I see your point. Since McPDM is only going to be used
for ALSA, it makes sense... But I still would think McPDM driver
should be independent of ALSA.
>
>
> > > > +/*
> > > > + * 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.
Ok, so at this point I see two approachs:
1. To leave as uplink/downlink in McPDM driver ops and add a comment on
how it relates to ALSA playback/capture data paths.
2. To change driver ops to playback/capture and leave a comment regarding
McPDM downlink/uplink data paths.
The only reason I see to use the first approach is to maintain McPDM
independent from ALSA side, but since it is a driver specifically for
audio, that shouldn't be a problem.
The idea would still be to show the relationship between McPDM and ALSA
data path names.
>
>
> > > > +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.
Yes, I should've set a comment here from the beginning.
>
> Thanks
>
> Liam
>
> ��.n��������+%������w��{.n�����{�������ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
