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@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()
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;elsectrl &= ~SW_UP_RST;}if (links & MCPDM_DOWNLINK) {if (reset)ctrl |= SW_DN_RST;elsectrl &= ~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 downlinkchannel 1 */
if (offset1)offset |= DN_OFST_RX1_EN;elseoffset &= ~DN_OFST_RX1_EN;/* Enable/disable offset cancellation for downlinkchannel 2 */
if (offset2)offset |= DN_OFST_RX2_EN;elseoffset &= ~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