Re: [PATCH 3/7] Asoc: sti: add CPU DAI driver for playback

[Arnaud Pouliquen <arnaud.pouliquen@st.com>]

On 04/24/2015 08:15 PM, Mark Brown wrote:
> On Tue, Apr 14, 2015 at 03:35:27PM +0200, Arnaud Pouliquen wrote:
>
>> +const struct snd_pcm_hardware uni_player_pcm_hw = {
>> +	.info		= (
>> +	SNDRV_PCM_INFO_INTERLEAVED |
>> +	SNDRV_PCM_INFO_BLOCK_TRANSFER |
>> +	SNDRV_PCM_INFO_PAUSE),
>> +	.formats	= (SNDRV_PCM_FMTBIT_S32_LE |
>> +	SNDRV_PCM_FMTBIT_S16_LE),
> Please use the normal kernel coding style, standard indentation and no
> brackets.
>
>> +	.rates		= SNDRV_PCM_RATE_CONTINUOUS,
>> +	.rate_min	= UNIPERIF_MIN_RATE,
>> +	.rate_max	= UNIPERIF_MAX_RATE,
>> +
>> +	.channels_min	= UNIPERIF_MIN_CHANNELS,
>> +	.channels_max	= UNIPERIF_MAX_CHANNELS,
>> +
>> +	.periods_min	= UNIPERIF_PERIODS_MIN,
>> +	.periods_max	= UNIPERIF_PERIODS_MAX,
>> +
>> +	.period_bytes_min = UNIPERIF_PERIODS_BYTES_MIN,
>> +	.period_bytes_max = UNIPERIF_PERIODS_BYTES_MAX,
>> +	.buffer_bytes_max = UNIPERIF_BUFFER_BYTES_MAX
> Just use the values directly, the indirection is just making it harder
> to find what's actually set.
>
>> +static inline int reset_player(struct uniperif *player)
>> +{
>> +	int count = 10;
>> +
>> +	if (player->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0)
>> +		while (GET_UNIPERIF_SOFT_RST_SOFT_RST(player) && count) {
>> +			udelay(5);
>> +			count--;
>> +		}
> Braces for the if too.  A cpu_relax() wouldn't go amiss in here either.
>
>> +static inline int get_property_hdl(struct device *dev, struct device_node *np,
>> +				   const char *prop, int idx)
>> +{
> This is very suspicous - why do you need this and if you need it why not
> add it to the generic DT code?
>
>> +static void uni_player_work(struct work_struct *work)
>> +{
>> +	struct uniperif *player =
>> +		container_of(work, struct uniperif, delayed_work.work);
>> +
>> +	spin_lock(&player->lock);
>> +	if (uni_player_suspend(player))
>> +		dev_err(player->dev, "%s: failed to suspend player", __func__);
>> +	spin_unlock(&player->lock);
>> +}
> What is this for, and why is there a spinlock which isn't IRQ safe?  If
> it's sensible to do this under a spinlock it seems like it might not
> need to be done in a workqueue...
>
> There's a lot of very coarse grained spinlock usage in this driver which
> I'm having a hard time understanding, at the very least the decisions
> about locking need to be documented much more clearly and I suspect that
> either things need to be finer grained, we should be using mutexes more
> or both.
>
This part is linked to the standby mode described in binding ( patch[1/7])
it manage a runtime suspend, because ASOC runtime suspend is dedicated 
to DAPM.
As you recommend i will try to change it by a DAPM linked to CPU_DAI. 
Just need to find a wait
to retrieve CPU_DAI context in DAPM..
Concerning spinlock
I use it to protect context ( called by IRQ, on suspend, by user...). As 
some code is called in atomic i can not use mutex.
I will review it and document it.
>> +static irqreturn_t uni_player_irq_handler(int irq, void *dev_id)
>> +{
>> +	irqreturn_t ret = IRQ_NONE;
>> +	struct uniperif *player = dev_id;
>> +	unsigned int status;
>> +	unsigned int tmp;
>> +
>> +	/* Get interrupt status & clear them immediately */
>> +	preempt_disable();
>> +	status = GET_UNIPERIF_ITS(player);
>> +	SET_UNIPERIF_ITS_BCLR(player, status);
>> +	preempt_enable();
> preempt_disable()?  Why is this being done, if you're doing unusual
> stuff like this the code needs to be very clear about what the locking
> rules are?
This is used to be sure to not miss an interrupt. If preempted between both
instruction i can clear an interruption flag before treat it. In this 
case i will receive
a second interrupt with all flag to 0.

>
>> +
>> +	if ((player->state == UNIPERIF_STATE_STANDBY) ||
>> +	    (player->state == UNIPERIF_STATE_STOPPED)) {
>> +		/* unexpected IRQ: do nothing */
>> +		dev_warn(player->dev, "unexpected IRQ: status flag: %#x",
>> +			 status);
>> +		return IRQ_HANDLED;
> We didn't handle it, we ignored it (after acknowledging it...).  I'd
> expect this check to be before we look at the hardware if it's needed.
>
>> +	};
> Extra semicolon here.
>
>> +	snd_pcm_stream_lock(player->substream);
> Again please explain the locking if we're doing something unusual.
This protect from a race condition, if application request a stop while 
we receive Error from IP.
I call it here to avoid toprotect all snd_pcm_stop calls... but i can  
protect only the snd_pcm_stop
calls if more clear
>
>> +	/* Check for fifo error (under run) */
>> +	if (unlikely(status & UNIPERIF_ITS_FIFO_ERROR_MASK(player))) {
>> +		dev_err(player->dev, "FIFO underflow error detected");
>> +
>> +		/* Interrupt is just for information when underflow recovery */
>> +		if (player->info->underflow_enabled) {
>> +			/* Update state to underflow */
>> +			player->state = UNIPERIF_STATE_UNDERFLOW;
> Why would underflow recovery be optional?
To propose 2 strategies:
- stop on underrun ( because plop will occurs)
- try to recover it:  time is recovered when new data is available ( 
sample dropping)

>
>> +	if (clk_set_rate(player->clk, rate_adjusted) < 0) {
>> +		dev_err(player->dev, "Failed to clk set rate %d !\n",
>> +			rate_adjusted);
>> +		return -EINVAL;
>> +	}
> Pass back the error code you got.
>
>> +	rate_achieved = clk_get_rate(player->clk);
>> +	if (!rate_achieved)
>> +		return -EINVAL;
> That check doesn't seem right - if the clock was set to 1Hz instead of
> 1MHz it'll report success.  Either trust that clk_set_rate() will give
> you an error if it fails or have some sort of reasonable check on the
> actual set value if that's important.

>
>> +static int snd_sti_clk_adjustment_get(struct snd_kcontrol *kcontrol,
>> +				      struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct uniperif *player = snd_kcontrol_chip(kcontrol);
>> +
>> +	spin_lock(&player->lock);
>> +	ucontrol->value.integer.value[0] = player->clk_adj;
>> +	spin_unlock(&player->lock);
>> +
>> +	return 0;
>> +}
> There was some discussion of interfaces for fine tuning clock rates with
> some other drivers recently, we need to think about this in some
> standard fashion.  Please split this out into a separate patch.  In
> general it's best to introduce unusual/new things like this and the IEC
> setting in separate patches to simplify review and allow the easy bits
> to get merged without being held up by complicated things like this.
Ok i will add Clk and IEC controls in separate patches
>> +	of_property_read_u32(pnode, "version", &info->ver);
>> +
>> +	of_property_read_u32(pnode, "uniperiph-id", &info->uni_num);
> We can't read this stuff from the hardware?
Unfortunately No...