On Thu, Oct 22, 2015 at 08:21:03PM +0100, Damien Horsley wrote:
> On 19/10/15 19:07, Mark Brown wrote:
> > On Mon, Oct 12, 2015 at 01:40:33PM +0100, Damien Horsley wrote:
> >> + spin_lock_irqsave(&prl->lock, flags);
> >> + reg = img_prl_out_readl(prl, IMG_PRL_OUT_CTL);
> >> + ucontrol->value.integer.value[0] = !!(reg & IMG_PRL_OUT_CTL_EDGE_MASK);
> >> + spin_unlock_irqrestore(&prl->lock, flags);
> > Do you need to lock a single register read?
> Between the calls to reset_control_assert and reset_control_deassert,
> the block is held in reset. During this time, no register access will
> succeed. All register access that may occur concurrently with the reset
> needs to be locked
Add a comment so readers know this.
> >> +static struct snd_kcontrol_new img_prl_out_controls[] = {
> >> + {
> >> + .iface = SNDRV_CTL_ELEM_IFACE_PCM,
> >> + .name = "Parallel Out Edge Falling",
> >> + .info = img_prl_out_edge_info,
> >> + .get = img_prl_out_get_edge,
> >> + .put = img_prl_out_set_edge
> >> + }
> >> +};
> > If this is a boolean control (it looked like one) it should be called
> > Switch but it's not clear to me what exactly is being controlled here or
> > why it's something that should be exposed to userspace.
> This controls the edge (rising/falling) of the frame clock that the
> samples are generated on. Should I create a set_fmt function and use
> SND_SOC_DAIFMT_NB_NF / SND_SOC_DAIFMT_NB_IF to set this instead? I
> wasn't sure if those formats were just for I2S or not
Yes, that's part of the DAI format - and we definitely don't want users
randomly changing this at runtime, the CODEC and CPU need to be
configured together.
Attachment:
signature.asc
Description: PGP signature
