Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller
On Thu, Oct 29, 2015 at 03:42:59PM +, Damien Horsley wrote: > For the I2S In, there is another issue with flushing on stream close. If > the stream is stopped, then reconfigured to use a larger number of > channels (without the stream being closed), then the per-channel fifos > will become inconsistent with each other. The new channels will have no > samples in their FIFOs, while the others may contain samples from the > previous stream. > Would hw_params be the correct place to flush instead? Yes, you could flush there (or in both places for that matter). signature.asc Description: PGP signature
Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller
On 28/10/15 23:43, Mark Brown wrote: > On Wed, Oct 28, 2015 at 09:18:20PM +, Damien Horsley wrote: >> On 28/10/15 01:04, Mark Brown wrote: > I think it also makes sense to keep the blocks consistent with each other. The spdif (out and in), and parallel out, all flush automatically when stopped, and the fifo for the i2s out block is cleared when the reset is asserted. > >>> This seems like an issue that got missed in the other drivers then. I'd >>> expect the trigger operation to be a minimal operation which starts and >>> stops the data transfer, not doing anything else. > >> The spdif out, spdif in, and parallel out blocks auto-flush whenever >> they are stopped. It is not possible for software to prevent this behavior. > > Oh, so this isn't the drivers doing this? In that case it's fine for > them to do that, if it's what the hardware does it's what the hardware > does. It sounded like you were saying that there was similar code in > the other drivers. > For the I2S In, there is another issue with flushing on stream close. If the stream is stopped, then reconfigured to use a larger number of channels (without the stream being closed), then the per-channel fifos will become inconsistent with each other. The new channels will have no samples in their FIFOs, while the others may contain samples from the previous stream. Would hw_params be the correct place to flush instead? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller
On Wed, Oct 28, 2015 at 09:18:20PM +, Damien Horsley wrote: > On 28/10/15 01:04, Mark Brown wrote: > >> I think it also makes sense to keep the blocks consistent with each > >> other. The spdif (out and in), and parallel out, all flush automatically > >> when stopped, and the fifo for the i2s out block is cleared when the > >> reset is asserted. > > This seems like an issue that got missed in the other drivers then. I'd > > expect the trigger operation to be a minimal operation which starts and > > stops the data transfer, not doing anything else. > The spdif out, spdif in, and parallel out blocks auto-flush whenever > they are stopped. It is not possible for software to prevent this behavior. Oh, so this isn't the drivers doing this? In that case it's fine for them to do that, if it's what the hardware does it's what the hardware does. It sounded like you were saying that there was similar code in the other drivers. signature.asc Description: PGP signature
Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller
On 28/10/15 01:04, Mark Brown wrote: > On Tue, Oct 27, 2015 at 01:55:27PM +, Damien Horsley wrote: >> On 23/10/15 23:57, Mark Brown wrote: > >>> Shouldn't we be doing that flush on stream close instead? If nothing >>> else the flush is going to discard a bit of data if the stream is just >>> paused. > >> The FIFOs are only 8 frames in size, so I am not sure there is an >> issue with these frames being lost. > >> I think it also makes sense to keep the blocks consistent with each >> other. The spdif (out and in), and parallel out, all flush automatically >> when stopped, and the fifo for the i2s out block is cleared when the >> reset is asserted. > > This seems like an issue that got missed in the other drivers then. I'd > expect the trigger operation to be a minimal operation which starts and > stops the data transfer, not doing anything else. > The spdif out, spdif in, and parallel out blocks auto-flush whenever they are stopped. It is not possible for software to prevent this behavior. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller
On Tue, Oct 27, 2015 at 01:55:27PM +, Damien Horsley wrote: > On 23/10/15 23:57, Mark Brown wrote: > > Shouldn't we be doing that flush on stream close instead? If nothing > > else the flush is going to discard a bit of data if the stream is just > > paused. > The FIFOs are only 8 frames in size, so I am not sure there is an > issue with these frames being lost. > I think it also makes sense to keep the blocks consistent with each > other. The spdif (out and in), and parallel out, all flush automatically > when stopped, and the fifo for the i2s out block is cleared when the > reset is asserted. This seems like an issue that got missed in the other drivers then. I'd expect the trigger operation to be a minimal operation which starts and stops the data transfer, not doing anything else. signature.asc Description: PGP signature
Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller
On 23/10/15 23:57, Mark Brown wrote:
> On Thu, Oct 22, 2015 at 08:09:38PM +0100, Damien Horsley wrote:
>> On 19/10/15 18:47, Mark Brown wrote:
>>> On Mon, Oct 12, 2015 at 01:40:29PM +0100, Damien Horsley wrote:
>
>>> The APIs here all seem a bit odd - for example the enable API taking a
>>> register value as an argument (normally reg is a register address BTW)
>>> and returning a value but the disable API doing a read/modify/write
>>> cycle.
>
>> Sure. It reduces the number of register accesses this way, but the
>> difference in execution time is not significant. Would you prefer these
>> to both do read-modify-writes?
>
> I would prefer that the functions look consistent with each other and
> ideally resemble common register acceess idioms in the kernel.
>
Ok.
+static inline void img_i2s_in_flush(struct img_i2s_in *i2s)
+{
+ int i;
+ u32 reg;
+
+ for (i = 0; i < i2s->active_channels; i++) {
+ reg = img_i2s_in_ch_disable(i2s, i);
+ reg |= IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
+ img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
+ reg &= ~IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
+ img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
+ img_i2s_in_ch_enable(i2s, i, reg);
+ }
+}
>
>>> This all seems to be connected to this, which is itself slightly funky
>>> especially in the context of the only user...
>
>> They are also used during hw_params and set_format.
>
> My point is that the flush function has only one user.
>
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ reg = img_i2s_in_readl(i2s, IMG_I2S_IN_CTL);
+ reg &= ~IMG_I2S_IN_CTL_ME_MASK;
+ img_i2s_in_writel(i2s, reg, IMG_I2S_IN_CTL);
+ img_i2s_in_flush(i2s);
+ break;
>
>>> ...which looks like it'll enable everything, then disable and reenable.
>>> Plus needing to do a flush on trigger seems weird.
>
>> If the FIFOs are not flushed, some samples from the previous stream will
>> be transferred to the user application when the block is started again
>
> Shouldn't we be doing that flush on stream close instead? If nothing
> else the flush is going to discard a bit of data if the stream is just
> paused.
>
The FIFOs are only 8 frames in size, so I am not sure there is an
issue with these frames being lost.
I think it also makes sense to keep the blocks consistent with each
other. The spdif (out and in), and parallel out, all flush automatically
when stopped, and the fifo for the i2s out block is cleared when the
reset is asserted.
+ if ((channels < 2) ||
+ (channels > (i2s->max_i2s_chan * 2)) ||
+ (channels % 2))
+ return -EINVAL;
>
>>> This indentation is very weird.
>
>> Ok. What is the correct indentation for this?
>
> Align the continuation lines of the if condition with the first line.
>
Ok
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller
On Thu, Oct 22, 2015 at 08:09:38PM +0100, Damien Horsley wrote:
> On 19/10/15 18:47, Mark Brown wrote:
> > On Mon, Oct 12, 2015 at 01:40:29PM +0100, Damien Horsley wrote:
> > The APIs here all seem a bit odd - for example the enable API taking a
> > register value as an argument (normally reg is a register address BTW)
> > and returning a value but the disable API doing a read/modify/write
> > cycle.
> Sure. It reduces the number of register accesses this way, but the
> difference in execution time is not significant. Would you prefer these
> to both do read-modify-writes?
I would prefer that the functions look consistent with each other and
ideally resemble common register acceess idioms in the kernel.
> >> +static inline void img_i2s_in_flush(struct img_i2s_in *i2s)
> >> +{
> >> + int i;
> >> + u32 reg;
> >> +
> >> + for (i = 0; i < i2s->active_channels; i++) {
> >> + reg = img_i2s_in_ch_disable(i2s, i);
> >> + reg |= IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
> >> + img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
> >> + reg &= ~IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
> >> + img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
> >> + img_i2s_in_ch_enable(i2s, i, reg);
> >> + }
> >> +}
> > This all seems to be connected to this, which is itself slightly funky
> > especially in the context of the only user...
> They are also used during hw_params and set_format.
My point is that the flush function has only one user.
> >> + case SNDRV_PCM_TRIGGER_STOP:
> >> + case SNDRV_PCM_TRIGGER_SUSPEND:
> >> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> >> + reg = img_i2s_in_readl(i2s, IMG_I2S_IN_CTL);
> >> + reg &= ~IMG_I2S_IN_CTL_ME_MASK;
> >> + img_i2s_in_writel(i2s, reg, IMG_I2S_IN_CTL);
> >> + img_i2s_in_flush(i2s);
> >> + break;
> > ...which looks like it'll enable everything, then disable and reenable.
> > Plus needing to do a flush on trigger seems weird.
> If the FIFOs are not flushed, some samples from the previous stream will
> be transferred to the user application when the block is started again
Shouldn't we be doing that flush on stream close instead? If nothing
else the flush is going to discard a bit of data if the stream is just
paused.
> >> + if ((channels < 2) ||
> >> + (channels > (i2s->max_i2s_chan * 2)) ||
> >> + (channels % 2))
> >> + return -EINVAL;
> > This indentation is very weird.
> Ok. What is the correct indentation for this?
Align the continuation lines of the if condition with the first line.
signature.asc
Description: PGP signature
Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller
On 19/10/15 18:47, Mark Brown wrote:
> On Mon, Oct 12, 2015 at 01:40:29PM +0100, Damien Horsley wrote:
>
>> +static inline u32 img_i2s_in_ch_disable(struct img_i2s_in *i2s, u32 chan)
>> +{
>> +u32 reg;
>> +
>> +reg = img_i2s_in_ch_readl(i2s, chan, IMG_I2S_IN_CH_CTL);
>> +reg &= ~IMG_I2S_IN_CH_CTL_ME_MASK;
>> +img_i2s_in_ch_writel(i2s, chan, reg, IMG_I2S_IN_CH_CTL);
>> +
>> +return reg;
>> +}
>> +
>> +static inline void img_i2s_in_ch_enable(struct img_i2s_in *i2s, u32 chan,
>> +u32 reg)
>> +{
>> +reg |= IMG_I2S_IN_CH_CTL_ME_MASK;
>> +img_i2s_in_ch_writel(i2s, chan, reg, IMG_I2S_IN_CH_CTL);
>> +}
>
> The APIs here all seem a bit odd - for example the enable API taking a
> register value as an argument (normally reg is a register address BTW)
> and returning a value but the disable API doing a read/modify/write
> cycle.
>
Sure. It reduces the number of register accesses this way, but the
difference in execution time is not significant. Would you prefer these
to both do read-modify-writes?
>> +static inline void img_i2s_in_flush(struct img_i2s_in *i2s)
>> +{
>> +int i;
>> +u32 reg;
>> +
>> +for (i = 0; i < i2s->active_channels; i++) {
>> +reg = img_i2s_in_ch_disable(i2s, i);
>> +reg |= IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
>> +img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
>> +reg &= ~IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
>> +img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
>> +img_i2s_in_ch_enable(i2s, i, reg);
>> +}
>> +}
>
> This all seems to be connected to this, which is itself slightly funky
> especially in the context of the only user...
>
They are also used during hw_params and set_format.
>> +case SNDRV_PCM_TRIGGER_STOP:
>> +case SNDRV_PCM_TRIGGER_SUSPEND:
>> +case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>> +reg = img_i2s_in_readl(i2s, IMG_I2S_IN_CTL);
>> +reg &= ~IMG_I2S_IN_CTL_ME_MASK;
>> +img_i2s_in_writel(i2s, reg, IMG_I2S_IN_CTL);
>> +img_i2s_in_flush(i2s);
>> +break;
>
> ...which looks like it'll enable everything, then disable and reenable.
> Plus needing to do a flush on trigger seems weird.
>
If the FIFOs are not flushed, some samples from the previous stream will
be transferred to the user application when the block is started again
>> +if ((channels < 2) ||
>> +(channels > (i2s->max_i2s_chan * 2)) ||
>> +(channels % 2))
>> +return -EINVAL;
>
> This indentation is very weird.
>
Ok. What is the correct indentation for this?
>> +control_mask = (u32)(~IMG_I2S_IN_CTL_16PACK_MASK &
>> +~IMG_I2S_IN_CTL_ACTIVE_CHAN_MASK);
>
>> +chan_control_mask = (u32)(~IMG_I2S_IN_CH_CTL_16PACK_MASK &
>> +~IMG_I2S_IN_CH_CTL_FEN_MASK &
>> +~IMG_I2S_IN_CH_CTL_FMODE_MASK &
>> +~IMG_I2S_IN_CH_CTL_SW_MASK &
>> +~IMG_I2S_IN_CH_CTL_FW_MASK &
>> +~IMG_I2S_IN_CH_CTL_PACKH_MASK);
>
> This also looks very odd. Normally we'd write masks as being the valid
> bits and or them together.
>
Ok
>> +i2s->clk_sys = devm_clk_get(dev, "sys");
>> +if (IS_ERR(i2s->clk_sys))
>> +return PTR_ERR(i2s->clk_sys);
>
> Please print an error message so people can tell why things failed.
>
Ok
>> +rst = devm_reset_control_get(dev, "rst");
>> +if (IS_ERR(rst)) {
>> +dev_dbg(dev, "No top level reset found\n");
>
> You should check for -EPROBE_DEFER here and just return the error here
> if you get it (on the basis that the reset framework ought to be using a
> different error if there's nothing bound in DT).
>
Ok
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller
On Mon, Oct 12, 2015 at 01:40:29PM +0100, Damien Horsley wrote:
> +static inline u32 img_i2s_in_ch_disable(struct img_i2s_in *i2s, u32 chan)
> +{
> + u32 reg;
> +
> + reg = img_i2s_in_ch_readl(i2s, chan, IMG_I2S_IN_CH_CTL);
> + reg &= ~IMG_I2S_IN_CH_CTL_ME_MASK;
> + img_i2s_in_ch_writel(i2s, chan, reg, IMG_I2S_IN_CH_CTL);
> +
> + return reg;
> +}
> +
> +static inline void img_i2s_in_ch_enable(struct img_i2s_in *i2s, u32 chan,
> + u32 reg)
> +{
> + reg |= IMG_I2S_IN_CH_CTL_ME_MASK;
> + img_i2s_in_ch_writel(i2s, chan, reg, IMG_I2S_IN_CH_CTL);
> +}
The APIs here all seem a bit odd - for example the enable API taking a
register value as an argument (normally reg is a register address BTW)
and returning a value but the disable API doing a read/modify/write
cycle.
> +static inline void img_i2s_in_flush(struct img_i2s_in *i2s)
> +{
> + int i;
> + u32 reg;
> +
> + for (i = 0; i < i2s->active_channels; i++) {
> + reg = img_i2s_in_ch_disable(i2s, i);
> + reg |= IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
> + img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
> + reg &= ~IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
> + img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
> + img_i2s_in_ch_enable(i2s, i, reg);
> + }
> +}
This all seems to be connected to this, which is itself slightly funky
especially in the context of the only user...
> + case SNDRV_PCM_TRIGGER_STOP:
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + reg = img_i2s_in_readl(i2s, IMG_I2S_IN_CTL);
> + reg &= ~IMG_I2S_IN_CTL_ME_MASK;
> + img_i2s_in_writel(i2s, reg, IMG_I2S_IN_CTL);
> + img_i2s_in_flush(i2s);
> + break;
...which looks like it'll enable everything, then disable and reenable.
Plus needing to do a flush on trigger seems weird.
> + if ((channels < 2) ||
> + (channels > (i2s->max_i2s_chan * 2)) ||
> + (channels % 2))
> + return -EINVAL;
This indentation is very weird.
> + control_mask = (u32)(~IMG_I2S_IN_CTL_16PACK_MASK &
> + ~IMG_I2S_IN_CTL_ACTIVE_CHAN_MASK);
> + chan_control_mask = (u32)(~IMG_I2S_IN_CH_CTL_16PACK_MASK &
> + ~IMG_I2S_IN_CH_CTL_FEN_MASK &
> + ~IMG_I2S_IN_CH_CTL_FMODE_MASK &
> + ~IMG_I2S_IN_CH_CTL_SW_MASK &
> + ~IMG_I2S_IN_CH_CTL_FW_MASK &
> + ~IMG_I2S_IN_CH_CTL_PACKH_MASK);
This also looks very odd. Normally we'd write masks as being the valid
bits and or them together.
> + i2s->clk_sys = devm_clk_get(dev, "sys");
> + if (IS_ERR(i2s->clk_sys))
> + return PTR_ERR(i2s->clk_sys);
Please print an error message so people can tell why things failed.
> + rst = devm_reset_control_get(dev, "rst");
> + if (IS_ERR(rst)) {
> + dev_dbg(dev, "No top level reset found\n");
You should check for -EPROBE_DEFER here and just return the error here
if you get it (on the basis that the reset framework ought to be using a
different error if there's nothing bound in DT).
signature.asc
Description: PGP signature
