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.
+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?
- 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.
- /* 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?
- 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.
- 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?