Re: [alsa-devel] [PATCH v2] staging: bcm2835-audio: interpolate audio delay
Hi Kirill. Thanks for your comments.
> On 22 Oct 2018, at 23:25, Kirill Marinushkin wrote:
>
> AFAIU, this patch is wrong. Please correct me, maybe I misunderstand
> something.
>
>> The problem that this patch seeks to resolve is that when userland asks for
>> the delay
>
> The userspace asks not for delay, but for the pointer.
The call in question is snd_pcm_delay. I presume this delay is calculated from
knowledge of the stream position “pos", the period (buffer?) number (and
period/buffer size) and the snd_pcm_runtime structure’s “delay" field
(“runtime->delay”).
> You modify the function, which is called `snd_bcm2835_pcm_pointer`. Here you
> are
> supposed to increase `alsa_stream->pos` with the proper offset. Instead, you
> imitate a delay, but in fact the delay is not increased.
>
> So, the proper solution should be to fix the reported pointer.
I think there is a difficulty with this. The “pos” pointer looks to have to be
modulo the buffer size. This causes a problem, as I see it, in that if the
calculated (pos + interpolated delay in bytes) is longer than the buffer size,
it must be wrapped, but AFAIK we are unable to increment a buffer index used in
the snd_pcm_delay calculation. Hence the calculation of the actual position
would be wrong. This is why the snd_pcm_runtime delay field is used. On
reflection, BTW, the patch assumes that the field's original value was zero —
that can be rectified.
> As a result,
> userspace will recieve the correct delay, instead of these crazy 10 ms.
Just to point out that with the proposed patch, it appears that the correct
delay is being reported, (apart, possibly, from any delay originally set in the
snd_pcm_delay field, as mentioned above).
All the best,
Mike
> FYI, there is
>> a discussion of the effects of a downstream equivalent of this suggested
>> patch
>> at:
>> https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016.
>
> Thank you for the link, it clarified for me what you try to achieve.
>
> On 10/22/18 21:17, Mike Brady wrote:
>> When the BCM2835 audio output is used, userspace sees a jitter up to 10ms
>> in the audio position, aka "delay" -- the number of frames that must
>> be output before a new frame would be played.
>> Make this a bit nicer for userspace by interpolating the position
>> using the CPU clock.
>> The overhead is small -- an extra ktime_get() every time a GPU message
>> is sent -- and another call and a few calculations whenever the delay
>> is sought from userland.
>> At 48,000 frames per second, i.e. approximately 20 microseconds per
>> frame, it would take a clock inaccuracy of
>> 20 microseconds in 10 milliseconds -- 2,000 parts per million --
>> to result in an inaccurate estimate, whereas
>> crystal- or resonator-based clocks typically have an
>> inaccuracy of 10s to 100s of parts per million.
>>
>> Signed-off-by: Mike Brady
>> ---
>> Changes in v2 -- remove inappropriate addition of SNDRV_PCM_INFO_BATCH flag
>>
>> .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 20 +++
>> .../vc04_services/bcm2835-audio/bcm2835.h | 1 +
>> 2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>> b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>> index e66da11af5cf..9053b996cada 100644
>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>> @@ -74,6 +74,7 @@ void bcm2835_playback_fifo(struct bcm2835_alsa_stream
>> *alsa_stream,
>> atomic_set(&alsa_stream->pos, pos);
>>
>> alsa_stream->period_offset += bytes;
>> +alsa_stream->interpolate_start = ktime_get();
>> if (alsa_stream->period_offset >= alsa_stream->period_size) {
>> alsa_stream->period_offset %= alsa_stream->period_size;
>> snd_pcm_period_elapsed(substream);
>> @@ -243,6 +244,7 @@ static int snd_bcm2835_pcm_prepare(struct
>> snd_pcm_substream *substream)
>> atomic_set(&alsa_stream->pos, 0);
>> alsa_stream->period_offset = 0;
>> alsa_stream->draining = false;
>> +alsa_stream->interpolate_start = ktime_get();
>>
>> return 0;
>> }
>> @@ -292,6 +294,24 @@ snd_bcm2835_pcm_pointer(struct snd_pcm_substream
>> *substream)
>> {
>> struct snd_pcm_runtime *runtime = substream->runtime;
>> struct bcm2835_alsa_stream *alsa_stream = runtime->private_data;
>> +ktime_t now = ktime_get();
>> +
>> +/* Give userspace better delay reporting by interpolating between GPU
>> + * notifications, assuming audio speed is close enough to the clock
>> + * used for ktime
>> + */
>> +
>> +if ((ktime_to_ns(alsa_stream->interpolate_start)) &&
>> +(ktime_compare(alsa_stream->interpolate_start, now) < 0)) {
>> +u64 interval =
>> +(ktime_to_ns(ktime_sub(now,
>> +alsa_stream->interpolate_start)));
>> +u64 frames_output_in_in
Re: [PATCH v2] staging: mt7621-dma: Add braces around else branches
Hi Kiberly,
Thanks for adding all the emails in CC.
I would encourage you for your next patch to distinguish between CC and TO.
You should send your patch TO important maintainers in the get_maintainers.pl
list (as default, to all of them). If there is someone you really want to look
into the patch, then add him/her in TO as well.
Put the rest (people and mailing lists) in CC. Why? Some people filter their
mails so that they can concentrate on the mails they got send directly and look
on mails they are in CC with lower priority (maybe not at all, because there are
too much?). So it is important to have the maintainers in the TO list and not
in CC.
Keep up the good work :)
Matthias
On 24/10/2018 06:56, Kimberly Brown wrote:
> Add braces around else branches in conditional statements that include
> branches with multiple statements. This style complies with the Linux
> kernel coding style and improves consistency and readability. Issues found by
> checkpatch.
>
> Signed-off-by: Kimberly Brown
> Reviewed-by: Matthias Brugger
> ---
> Changes in v2:
> - Added people and mailing lists from get_maintainer.pl to the CC list
> - Added Reviewed-by line
>
> drivers/staging/mt7621-dma/mtk-hsdma.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/mt7621-dma/mtk-hsdma.c
> b/drivers/staging/mt7621-dma/mtk-hsdma.c
> index df6ebf41bdea..35556f024aa1 100644
> --- a/drivers/staging/mt7621-dma/mtk-hsdma.c
> +++ b/drivers/staging/mt7621-dma/mtk-hsdma.c
> @@ -418,8 +418,9 @@ static void mtk_hsdma_chan_done(struct mtk_hsdam_engine
> *hsdma,
> vchan_cookie_complete(&desc->vdesc);
> chan_issued = gdma_next_desc(chan);
> }
> - } else
> + } else {
> dev_dbg(hsdma->ddev.dev, "no desc to complete\n");
> + }
>
> if (chan_issued)
> set_bit(chan->id, &hsdma->chan_issued);
> @@ -456,8 +457,9 @@ static void mtk_hsdma_issue_pending(struct dma_chan *c)
> if (gdma_next_desc(chan)) {
> set_bit(chan->id, &hsdma->chan_issued);
> tasklet_schedule(&hsdma->task);
> - } else
> + } else {
> dev_dbg(hsdma->ddev.dev, "no desc to issue\n");
> + }
> }
> spin_unlock_bh(&chan->vchan.lock);
> }
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: greybus: loopback.c: remove unused lists
gb_loopback_device::list_op_async is never used except for the
LIST_INIT. The ::list field appears to have a few more uses, but on
closer inspection the linked list of struct gb_loopbacks that it heads
is never used for anything, so there's no reason to maintain it, much
less to keep it sorted.
Reviewed-by: Bryan O'Donoghue
Signed-off-by: Rasmus Villemoes
---
Sending as a proper patch. Marked v2 since this replaces earlier 2/3
and 3/3 patches. Applies on top of
b4fc4e8340784e30c5a59bf0791f9c3ce15e (staging: greybus:
loopback.c: remove unused gb_loopback::lbid).
drivers/staging/greybus/loopback.c | 38 --
1 file changed, 38 deletions(-)
diff --git a/drivers/staging/greybus/loopback.c
b/drivers/staging/greybus/loopback.c
index 7080294f705c..e4d42c1dc284 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -47,8 +47,6 @@ struct gb_loopback_device {
/* We need to take a lock in atomic context */
spinlock_t lock;
- struct list_head list;
- struct list_head list_op_async;
wait_queue_head_t wq;
};
@@ -68,7 +66,6 @@ struct gb_loopback {
struct kfifo kfifo_lat;
struct mutex mutex;
struct task_struct *task;
- struct list_head entry;
struct device *dev;
wait_queue_head_t wq;
wait_queue_head_t wq_completion;
@@ -987,37 +984,6 @@ static const struct file_operations
gb_loopback_debugfs_latency_ops = {
.release= single_release,
};
-static int gb_loopback_bus_id_compare(void *priv, struct list_head *lha,
- struct list_head *lhb)
-{
- struct gb_loopback *a = list_entry(lha, struct gb_loopback, entry);
- struct gb_loopback *b = list_entry(lhb, struct gb_loopback, entry);
- struct gb_connection *ca = a->connection;
- struct gb_connection *cb = b->connection;
-
- if (ca->bundle->intf->interface_id < cb->bundle->intf->interface_id)
- return -1;
- if (cb->bundle->intf->interface_id < ca->bundle->intf->interface_id)
- return 1;
- if (ca->bundle->id < cb->bundle->id)
- return -1;
- if (cb->bundle->id < ca->bundle->id)
- return 1;
- if (ca->intf_cport_id < cb->intf_cport_id)
- return -1;
- else if (cb->intf_cport_id < ca->intf_cport_id)
- return 1;
-
- return 0;
-}
-
-static void gb_loopback_insert_id(struct gb_loopback *gb)
-{
- /* perform an insertion sort */
- list_add_tail(&gb->entry, &gb_dev.list);
- list_sort(NULL, &gb_dev.list, gb_loopback_bus_id_compare);
-}
-
#define DEBUGFS_NAMELEN 32
static int gb_loopback_probe(struct gb_bundle *bundle,
@@ -1113,7 +1079,6 @@ static int gb_loopback_probe(struct gb_bundle *bundle,
}
spin_lock_irqsave(&gb_dev.lock, flags);
- gb_loopback_insert_id(gb);
gb_dev.count++;
spin_unlock_irqrestore(&gb_dev.lock, flags);
@@ -1169,7 +1134,6 @@ static void gb_loopback_disconnect(struct gb_bundle
*bundle)
spin_lock_irqsave(&gb_dev.lock, flags);
gb_dev.count--;
- list_del(&gb->entry);
spin_unlock_irqrestore(&gb_dev.lock, flags);
device_unregister(gb->dev);
@@ -1196,8 +1160,6 @@ static int loopback_init(void)
{
int retval;
- INIT_LIST_HEAD(&gb_dev.list);
- INIT_LIST_HEAD(&gb_dev.list_op_async);
spin_lock_init(&gb_dev.lock);
gb_dev.root = debugfs_create_dir("gb_loopback", NULL);
--
2.19.1.6.gbde171bbf5
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: mt7621-dma: Add braces around else branches
On Wed, Oct 24, 2018 at 12:45:44PM +0200, Matthias Brugger wrote: > Hi Kiberly, > > Thanks for adding all the emails in CC. > I would encourage you for your next patch to distinguish between CC and TO. > You should send your patch TO important maintainers in the get_maintainers.pl > list (as default, to all of them). If there is someone you really want to look > into the patch, then add him/her in TO as well. > > Put the rest (people and mailing lists) in CC. Why? Some people filter their > mails so that they can concentrate on the mails they got send directly and > look > on mails they are in CC with lower priority (maybe not at all, because there > are > too much?). So it is important to have the maintainers in the TO list and not > in CC. +1 I'm glad that there's someone else in the Linux community that agrees with me on this point, and is willing to speak out about it. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: mt7621-dma: Add braces around else branches
On Wed, Oct 24, 2018 at 12:45:44PM +0200, Matthias Brugger wrote: > Hi Kiberly, > > Thanks for adding all the emails in CC. > I would encourage you for your next patch to distinguish between CC and TO. > You should send your patch TO important maintainers in the get_maintainers.pl > list (as default, to all of them). If there is someone you really want to look > into the patch, then add him/her in TO as well. > I normally just add the first person from get_maintainer.pl to the To:. It's basically random I think. The rest I put in the CC. This is a staging patch so no one is very picky. There isn't a Fixes tag for this but if there were I add the person who wrote the original patch to the To header. Hopefully, they review and Ack my patch. I feel like that's very important. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] Re: [PATCH v2] staging: mt7621-dma: Add braces around else branches
On Wed, 24 Oct 2018, Russell King - ARM Linux wrote: > On Wed, Oct 24, 2018 at 12:45:44PM +0200, Matthias Brugger wrote: > > Hi Kiberly, > > > > Thanks for adding all the emails in CC. > > I would encourage you for your next patch to distinguish between CC and TO. > > You should send your patch TO important maintainers in the > > get_maintainers.pl > > list (as default, to all of them). If there is someone you really want to > > look > > into the patch, then add him/her in TO as well. > > > > Put the rest (people and mailing lists) in CC. Why? Some people filter their > > mails so that they can concentrate on the mails they got send directly and > > look > > on mails they are in CC with lower priority (maybe not at all, because > > there are > > too much?). So it is important to have the maintainers in the TO list and > > not in CC. > > +1 > > I'm glad that there's someone else in the Linux community that agrees > with me on this point, and is willing to speak out about it. If it's an important point, perhaps it should be mentioned in submitting-patches.rst? There is a mention of the Cc tag, but no indication of who to put in CC. julia ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] Re: [PATCH v2] staging: mt7621-dma: Add braces around else branches
On Wed, Oct 24, 2018 at 02:06:18PM +0100, Julia Lawall wrote: > > > On Wed, 24 Oct 2018, Russell King - ARM Linux wrote: > > > On Wed, Oct 24, 2018 at 12:45:44PM +0200, Matthias Brugger wrote: > > > Hi Kiberly, > > > > > > Thanks for adding all the emails in CC. > > > I would encourage you for your next patch to distinguish between CC and > > > TO. > > > You should send your patch TO important maintainers in the > > > get_maintainers.pl > > > list (as default, to all of them). If there is someone you really want to > > > look > > > into the patch, then add him/her in TO as well. > > > > > > Put the rest (people and mailing lists) in CC. Why? Some people filter > > > their > > > mails so that they can concentrate on the mails they got send directly > > > and look > > > on mails they are in CC with lower priority (maybe not at all, because > > > there are > > > too much?). So it is important to have the maintainers in the TO list and > > > not in CC. > > > > +1 > > > > I'm glad that there's someone else in the Linux community that agrees > > with me on this point, and is willing to speak out about it. > > If it's an important point, perhaps it should be mentioned in > submitting-patches.rst? There is a mention of the Cc tag, but no > indication of who to put in CC. submitting-patches.rst talks about the Cc tag in the commit, not the To or Cc in the email client. In any case, there's a lot of personal issues here: most kernel developers don't care whether they're in the To or Cc header of an email, but there are some who do use it as Matthias says - which is actually the long-standing definition of these headers. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: ni_mio_common: scale ao INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS
Changes implementation of INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS for ni_mio devices to scale the result by the number of channels being used. The user is already required to indicate which channels (and how many obviously) are intended to be used. There is no point of not using this information--the analog input cards already similarly scale the timing results based on the number of channels. Signed-off-by: Spencer E. Olson --- This patch is made in reference to the last set of patches adding the timing constraint facility in pci_mio_common (51fd3673838396844f15de0e906be5333bfbbc8d). drivers/staging/comedi/drivers/ni_mio_common.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c index 2d1e0325d04d..5edf59ac6706 100644 --- a/drivers/staging/comedi/drivers/ni_mio_common.c +++ b/drivers/staging/comedi/drivers/ni_mio_common.c @@ -2843,7 +2843,8 @@ static int ni_ao_insn_config(struct comedi_device *dev, return ni_ao_arm(dev, s); case INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS: /* we don't care about actual channels */ - data[1] = board->ao_speed; + /* data[3] : chanlist_len */ + data[1] = board->ao_speed * data[3]; data[2] = 0; return 0; default: -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: iio: ad2s1210: Switch to the gpio descriptor interface
On Wed, Oct 24, 2018 at 08:52:49AM +0200, Slawomir Stepien wrote:
> On paź 24, 2018 00:34, Nishad Kamdar wrote:
> > Use the gpiod interface instead of the deprecated old non-descriptor
> > interface.
>
> Hi Nishad
>
> Few more comments from me below.
>
> > Signed-off-by: Nishad Kamdar
> > ---
> > Changes in v3:
> > - Use a pointer to pointer for gpio_desc in
> >struct ad2s1210_gpio as it will be used to
> >modify a pointer.
> > - Use dot notation to initialize the structure.
> > - Use a pointer variable to avoid writing gpios[i].
> > Changes in v2:
> > - Use the spi_device struct embedded in st instead
> >of passing it as an argument to ad2s1210_setup_gpios().
> > - Use an array of structs to reduce redundant code in
> >in ad2s1210_setup_gpios().
> > - Remove ad2s1210_free_gpios() as devm API is being used.
> > ---
> > drivers/staging/iio/resolver/ad2s1210.c | 92 ++---
> > drivers/staging/iio/resolver/ad2s1210.h | 3 -
> > 2 files changed, 50 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c
> > b/drivers/staging/iio/resolver/ad2s1210.c
> > index ac13b99bd9cb..fbf0cc722979 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -15,7 +15,7 @@
> > #include
> > #include
> > #include
> > -#include
> > +#include
> > #include
> >
> > #include
> > @@ -69,10 +69,21 @@ enum ad2s1210_mode {
> >
> > static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
> >
> > +struct ad2s1210_gpio {
> > + struct gpio_desc **ptr;
> > + const char *name;
> > + unsigned long flags;
> > +};
> > +
> > struct ad2s1210_state {
> > const struct ad2s1210_platform_data *pdata;
> > struct mutex lock;
> > struct spi_device *sdev;
> > + struct gpio_desc *sample;
> > + struct gpio_desc *a0;
> > + struct gpio_desc *a1;
> > + struct gpio_desc *res0;
> > + struct gpio_desc *res1;
> > unsigned int fclkin;
> > unsigned int fexcit;
> > bool hysteresis;
> > @@ -91,8 +102,8 @@ static const int ad2s1210_mode_vals[4][2] = {
> > static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
> > struct ad2s1210_state *st)
> > {
> > - gpio_set_value(st->pdata->a[0], ad2s1210_mode_vals[mode][0]);
> > - gpio_set_value(st->pdata->a[1], ad2s1210_mode_vals[mode][1]);
> > + gpiod_set_value(st->a0, ad2s1210_mode_vals[mode][0]);
> > + gpiod_set_value(st->a1, ad2s1210_mode_vals[mode][1]);
> > st->mode = mode;
> > }
> >
> > @@ -152,8 +163,8 @@ int ad2s1210_update_frequency_control_word(struct
> > ad2s1210_state *st)
> >
> > static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state
> > *st)
> > {
> > - int resolution = (gpio_get_value(st->pdata->res[0]) << 1) |
> > - gpio_get_value(st->pdata->res[1]);
> > + int resolution = (gpiod_get_value(st->res0) << 1) |
> > + gpiod_get_value(st->res1);
> >
> > return ad2s1210_resolution_value[resolution];
> > }
> > @@ -164,10 +175,10 @@ static const int ad2s1210_res_pins[4][2] = {
> >
> > static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
> > {
> > - gpio_set_value(st->pdata->res[0],
> > - ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> > - gpio_set_value(st->pdata->res[1],
> > - ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
> > + gpiod_set_value(st->res0,
> > + ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> > + gpiod_set_value(st->res1,
> > + ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
> > }
> >
> > static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
> > @@ -401,15 +412,15 @@ static ssize_t ad2s1210_clear_fault(struct device
> > *dev,
> > int ret;
> >
> > mutex_lock(&st->lock);
> > - gpio_set_value(st->pdata->sample, 0);
> > + gpiod_set_value(st->sample, 0);
> > /* delay (2 * tck + 20) nano seconds */
> > udelay(1);
> > - gpio_set_value(st->pdata->sample, 1);
> > + gpiod_set_value(st->sample, 1);
> > ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
> > if (ret < 0)
> > goto error_ret;
> > - gpio_set_value(st->pdata->sample, 0);
> > - gpio_set_value(st->pdata->sample, 1);
> > + gpiod_set_value(st->sample, 0);
> > + gpiod_set_value(st->sample, 1);
> > error_ret:
> > mutex_unlock(&st->lock);
> >
> > @@ -466,7 +477,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> > s16 vel;
> >
> > mutex_lock(&st->lock);
> > - gpio_set_value(st->pdata->sample, 0);
> > + gpiod_set_value(st->sample, 0);
> > /* delay (6 * tck + 20) nano seconds */
> > udelay(1);
> >
> > @@ -512,7 +523,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> > }
> >
> > error_ret:
> > - gpio_set_value(st->pdata->sample, 1);
> > + gpiod_set_value(st->sample, 1);
> >
Re: [PATCH] staging: comedi: ni_mio_common: scale ao INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS
On Wed, Oct 24, 2018 at 07:59:45AM -0600, Spencer E. Olson wrote:
> Changes implementation of INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS for
> ni_mio devices to scale the result by the number of channels being used.
I really can't understand this statement at all. What changes the
implementation?
> The user is already required to indicate which channels (and how many
> obviously) are intended to be used. There is no point of not using this
> information--the analog input cards already similarly scale the timing
> results based on the number of channels.
This sounds like it's just an optimization but I think this patch is a
behavior change or a bug fix, right? It's not totally clear to me from
the patch description what the user visible effect of this patch is.
Could you spell that out a little bit more clearly and resend the
patch? (It's also possible that I just don't understand Comedi well
enough to understand the patch description).
>
> Signed-off-by: Spencer E. Olson
> ---
> This patch is made in reference to the last set of patches adding the timing
> constraint facility in pci_mio_common
> (51fd3673838396844f15de0e906be5333bfbbc8d).
I feel like we should be using the Fixes tag for this. Like so:
Fixes: 51fd36738383 ("staging: comedi: ni_mio_common: implement
INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS")
Signed-off-by: Your
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: ni_mio_common: scale ao INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS
On Wed, Oct 24, 2018 at 8:18 AM Dan Carpenter wrote:
>
> On Wed, Oct 24, 2018 at 07:59:45AM -0600, Spencer E. Olson wrote:
> > Changes implementation of INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS for
> > ni_mio devices to scale the result by the number of channels being used.
>
> I really can't understand this statement at all. What changes the
> implementation?
>
> > The user is already required to indicate which channels (and how many
> > obviously) are intended to be used. There is no point of not using this
> > information--the analog input cards already similarly scale the timing
> > results based on the number of channels.
>
> This sounds like it's just an optimization but I think this patch is a
> behavior change or a bug fix, right? It's not totally clear to me from
> the patch description what the user visible effect of this patch is.
> Could you spell that out a little bit more clearly and resend the
> patch? (It's also possible that I just don't understand Comedi well
> enough to understand the patch description).
>
> >
> > Signed-off-by: Spencer E. Olson
> > ---
> > This patch is made in reference to the last set of patches adding the
> > timing
> > constraint facility in pci_mio_common
> > (51fd3673838396844f15de0e906be5333bfbbc8d).
>
>
> I feel like we should be using the Fixes tag for this. Like so:
>
> Fixes: 51fd36738383 ("staging: comedi: ni_mio_common: implement
> INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS")
> Signed-off-by: Your
I did consider submitting it as a fix instead, but I couldn't actually
remember what my intentions were when I wrote the original code from
the earlier patch. On the other hand, after reading more on the
interface documentation and user code that I have written (not yet
submitted to the comedi community), I think you are right and that
this should be submitted as a fix. Thanks
>
> regards,
> dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: clarify/unify macros for NI macro-defined terminals
Uses a single macro to define multiple macros that represent a series of
terminals for NI devices. This patch also redefines NI_MAX_COUNTERS as the
maximum number of counters possible on NI devices (instead of the maximum
index of the counters). This was a little confusing and caused a bug in
commit 347e244884c3b ("staging: comedi: tio: implement global tio/ctr routing")
when setting/reading registers for counter terminals.
Fixes: 347e244884c3b ("staging: comedi: tio: implement global tio/ctr routing")
Signed-off-by: Spencer E. Olson
---
drivers/staging/comedi/comedi.h | 39 ++---
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/comedi/comedi.h b/drivers/staging/comedi/comedi.h
index e90b17775284..09a940066c0e 100644
--- a/drivers/staging/comedi/comedi.h
+++ b/drivers/staging/comedi/comedi.h
@@ -1005,35 +1005,38 @@ enum i8254_mode {
* and INSN_DEVICE_CONFIG_GET_ROUTES.
*/
#define NI_NAMES_BASE 0x8000u
+
+#define _TERM_N(base, n, x)((base) + ((x) & ((n) - 1)))
+
/*
* not necessarily all allowed 64 PFIs are valid--certainly not for all devices
*/
-#define NI_PFI(x) (NI_NAMES_BASE+ ((x) & 0x3f))
+#define NI_PFI(x) _TERM_N(NI_NAMES_BASE, 64, x)
/* 8 trigger lines by standard, Some devices cannot talk to all eight. */
-#define TRIGGER_LINE(x)(NI_PFI(-1) + 1 + ((x) & 0x7))
+#define TRIGGER_LINE(x)_TERM_N(NI_PFI(-1) + 1, 8, x)
/* 4 RTSI shared MUXes to route signals to/from TRIGGER_LINES on NI hardware */
-#define NI_RTSI_BRD(x) (TRIGGER_LINE(-1) + 1 + ((x) & 0x3))
+#define NI_RTSI_BRD(x) _TERM_N(TRIGGER_LINE(-1) + 1, 4, x)
/* *** Counter/timer names : 8 counters max *** */
-#define NI_COUNTER_NAMES_BASE (NI_RTSI_BRD(-1) + 1)
-#define NI_MAX_COUNTERS 7
-#define NI_CtrSource(x) (NI_COUNTER_NAMES_BASE + ((x) &
NI_MAX_COUNTERS))
+#define NI_MAX_COUNTERS8
+#define NI_COUNTER_NAMES_BASE (NI_RTSI_BRD(-1) + 1)
+#define NI_CtrSource(x) _TERM_N(NI_COUNTER_NAMES_BASE,
NI_MAX_COUNTERS, x)
/* Gate, Aux, A,B,Z are all treated, at times as gates */
-#define NI_GATES_NAMES_BASE(NI_CtrSource(-1) + 1)
-#define NI_CtrGate(x) (NI_GATES_NAMES_BASE + ((x) & NI_MAX_COUNTERS))
-#define NI_CtrAux(x) (NI_CtrGate(-1) + 1 + ((x) & NI_MAX_COUNTERS))
-#define NI_CtrA(x)(NI_CtrAux(-1)+ 1 + ((x) & NI_MAX_COUNTERS))
-#define NI_CtrB(x)(NI_CtrA(-1) + 1 + ((x) & NI_MAX_COUNTERS))
-#define NI_CtrZ(x)(NI_CtrB(-1) + 1 + ((x) & NI_MAX_COUNTERS))
-#define NI_GATES_NAMES_MAX NI_CtrZ(-1)
-#define NI_CtrArmStartTrigger(x) (NI_CtrZ(-1)+ 1 + ((x) &
NI_MAX_COUNTERS))
+#define NI_GATES_NAMES_BASE(NI_CtrSource(-1) + 1)
+#define NI_CtrGate(x) _TERM_N(NI_GATES_NAMES_BASE, NI_MAX_COUNTERS, x)
+#define NI_CtrAux(x) _TERM_N(NI_CtrGate(-1) + 1, NI_MAX_COUNTERS, x)
+#define NI_CtrA(x) _TERM_N(NI_CtrAux(-1) + 1, NI_MAX_COUNTERS, x)
+#define NI_CtrB(x) _TERM_N(NI_CtrA(-1) + 1, NI_MAX_COUNTERS, x)
+#define NI_CtrZ(x) _TERM_N(NI_CtrB(-1) + 1, NI_MAX_COUNTERS, x)
+#define NI_GATES_NAMES_MAX NI_CtrZ(-1)
+#define NI_CtrArmStartTrigger(x) _TERM_N(NI_CtrZ(-1)+ 1, NI_MAX_COUNTERS,
x)
#define NI_CtrInternalOutput(x) \
-(NI_CtrArmStartTrigger(-1) + 1 + ((x) & NI_MAX_COUNTERS))
+ _TERM_N(NI_CtrArmStartTrigger(-1) + 1, NI_MAX_COUNTERS, x)
/** external pin(s) labeled conveniently as CtrOut. */
-#define NI_CtrOut(x) (NI_CtrInternalOutput(-1) + 1 + ((x) &
NI_MAX_COUNTERS))
+#define NI_CtrOut(x) _TERM_N(NI_CtrInternalOutput(-1) + 1, NI_MAX_COUNTERS,
x)
/** For Buffered sampling of ctr -- x series capability. */
-#define NI_CtrSampleClock(x) (NI_CtrOut(-1) + 1 + ((x) & NI_MAX_COUNTERS))
-#define NI_COUNTER_NAMES_MAX NI_CtrSampleClock(-1)
+#define NI_CtrSampleClock(x) _TERM_N(NI_CtrOut(-1) + 1, NI_MAX_COUNTERS, x)
+#define NI_COUNTER_NAMES_MAX NI_CtrSampleClock(-1)
enum ni_common_signal_names {
/* PXI_Star: this is a non-NI-specific signal */
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: comedi: ni_mio_common: scale ao INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS
Fixes implementation of INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS for
ni_mio devices. The previous patch should have used the channel
information passed in to scale the result by the number of channels being
used.
Fixes: 51fd36738383 ("staging: comedi: ni_mio_common: implement
INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS")
Signed-off-by: Spencer E. Olson
---
drivers/staging/comedi/drivers/ni_mio_common.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
b/drivers/staging/comedi/drivers/ni_mio_common.c
index 2d1e0325d04d..5edf59ac6706 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -2843,7 +2843,8 @@ static int ni_ao_insn_config(struct comedi_device *dev,
return ni_ao_arm(dev, s);
case INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS:
/* we don't care about actual channels */
- data[1] = board->ao_speed;
+ /* data[3] : chanlist_len */
+ data[1] = board->ao_speed * data[3];
data[2] = 0;
return 0;
default:
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: ni_mio_common: scale ao INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS
On Wed, Oct 24, 2018 at 08:26:51AM -0600, Spencer Olson wrote:
> On Wed, Oct 24, 2018 at 8:18 AM Dan Carpenter
> wrote:
> >
> > On Wed, Oct 24, 2018 at 07:59:45AM -0600, Spencer E. Olson wrote:
> > > Changes implementation of INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS for
> > > ni_mio devices to scale the result by the number of channels being used.
> >
> > I really can't understand this statement at all. What changes the
> > implementation?
> >
> > > The user is already required to indicate which channels (and how many
> > > obviously) are intended to be used. There is no point of not using this
> > > information--the analog input cards already similarly scale the timing
> > > results based on the number of channels.
> >
> > This sounds like it's just an optimization but I think this patch is a
> > behavior change or a bug fix, right? It's not totally clear to me from
> > the patch description what the user visible effect of this patch is.
> > Could you spell that out a little bit more clearly and resend the
> > patch? (It's also possible that I just don't understand Comedi well
> > enough to understand the patch description).
> >
> > >
> > > Signed-off-by: Spencer E. Olson
> > > ---
> > > This patch is made in reference to the last set of patches adding the
> > > timing
> > > constraint facility in pci_mio_common
> > > (51fd3673838396844f15de0e906be5333bfbbc8d).
> >
> >
> > I feel like we should be using the Fixes tag for this. Like so:
> >
> > Fixes: 51fd36738383 ("staging: comedi: ni_mio_common: implement
> > INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS")
> > Signed-off-by: Your
>
> I did consider submitting it as a fix instead, but I couldn't actually
> remember what my intentions were when I wrote the original code from
> the earlier patch. On the other hand, after reading more on the
> interface documentation and user code that I have written (not yet
> submitted to the comedi community), I think you are right and that
> this should be submitted as a fix. Thanks
>
That kind of information is actually really interesting to me when I'm
reviewing the code. The changelog could say something like this:
I noticed a bug in the API . I haven't quite
published the user space code for this so this patch doesn't cause
any regressions.
Then I would feel very good about a patch like that.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4] staging: iio: ad2s1210: Switch to the gpio descriptor interface
Use the gpiod interface instead of the deprecated old non-descriptor
interface.
Signed-off-by: Nishad Kamdar
---
Changes in v4:
- Add spaces after { and before } in gpios[]
initialization.
- Check the correct pointer for error.
- Align the dev_err msg to existing format in the code.
Changes in v3:
- Use a pointer to pointer for gpio_desc in
struct ad2s1210_gpio as it will be used to
modify a pointer.
- Use dot notation to initialize the structure.
- Use a pointer variable to avoid writing gpios[i].
Changes in v2:
- Use the spi_device struct embedded in st instead
of passing it as an argument to ad2s1210_setup_gpios().
- Use an array of structs to reduce redundant code in
in ad2s1210_setup_gpios().
- Remove ad2s1210_free_gpios() as devm API is being used.
---
drivers/staging/iio/resolver/ad2s1210.c | 92 ++---
drivers/staging/iio/resolver/ad2s1210.h | 3 -
2 files changed, 50 insertions(+), 45 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c
b/drivers/staging/iio/resolver/ad2s1210.c
index ac13b99bd9cb..93c3c70ce62e 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -15,7 +15,7 @@
#include
#include
#include
-#include
+#include
#include
#include
@@ -69,10 +69,21 @@ enum ad2s1210_mode {
static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
+struct ad2s1210_gpio {
+ struct gpio_desc **ptr;
+ const char *name;
+ unsigned long flags;
+};
+
struct ad2s1210_state {
const struct ad2s1210_platform_data *pdata;
struct mutex lock;
struct spi_device *sdev;
+ struct gpio_desc *sample;
+ struct gpio_desc *a0;
+ struct gpio_desc *a1;
+ struct gpio_desc *res0;
+ struct gpio_desc *res1;
unsigned int fclkin;
unsigned int fexcit;
bool hysteresis;
@@ -91,8 +102,8 @@ static const int ad2s1210_mode_vals[4][2] = {
static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
struct ad2s1210_state *st)
{
- gpio_set_value(st->pdata->a[0], ad2s1210_mode_vals[mode][0]);
- gpio_set_value(st->pdata->a[1], ad2s1210_mode_vals[mode][1]);
+ gpiod_set_value(st->a0, ad2s1210_mode_vals[mode][0]);
+ gpiod_set_value(st->a1, ad2s1210_mode_vals[mode][1]);
st->mode = mode;
}
@@ -152,8 +163,8 @@ int ad2s1210_update_frequency_control_word(struct
ad2s1210_state *st)
static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)
{
- int resolution = (gpio_get_value(st->pdata->res[0]) << 1) |
- gpio_get_value(st->pdata->res[1]);
+ int resolution = (gpiod_get_value(st->res0) << 1) |
+ gpiod_get_value(st->res1);
return ad2s1210_resolution_value[resolution];
}
@@ -164,10 +175,10 @@ static const int ad2s1210_res_pins[4][2] = {
static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
{
- gpio_set_value(st->pdata->res[0],
- ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
- gpio_set_value(st->pdata->res[1],
- ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
+ gpiod_set_value(st->res0,
+ ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
+ gpiod_set_value(st->res1,
+ ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
}
static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
@@ -401,15 +412,15 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
int ret;
mutex_lock(&st->lock);
- gpio_set_value(st->pdata->sample, 0);
+ gpiod_set_value(st->sample, 0);
/* delay (2 * tck + 20) nano seconds */
udelay(1);
- gpio_set_value(st->pdata->sample, 1);
+ gpiod_set_value(st->sample, 1);
ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
if (ret < 0)
goto error_ret;
- gpio_set_value(st->pdata->sample, 0);
- gpio_set_value(st->pdata->sample, 1);
+ gpiod_set_value(st->sample, 0);
+ gpiod_set_value(st->sample, 1);
error_ret:
mutex_unlock(&st->lock);
@@ -466,7 +477,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
s16 vel;
mutex_lock(&st->lock);
- gpio_set_value(st->pdata->sample, 0);
+ gpiod_set_value(st->sample, 0);
/* delay (6 * tck + 20) nano seconds */
udelay(1);
@@ -512,7 +523,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
}
error_ret:
- gpio_set_value(st->pdata->sample, 1);
+ gpiod_set_value(st->sample, 1);
/* delay (2 * tck + 20) nano seconds */
udelay(1);
mutex_unlock(&st->lock);
@@ -630,30 +641,32 @@ static const struct iio_info ad2s1210_info = {
static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
{
- unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_I
Re: [PATCH v2] staging: comedi: ni_mio_common: scale ao INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS
That works. Thanks! regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: axis-fifo: Fix line over 80 characters error
This patch fixes the checkpatch.pl warning: WARNING: line over 80 characters +(write_timeout >= 0) ? msecs_to_jiffies(write_timeout) : Signed-off-by: Aleksa Zdravkovic --- drivers/staging/axis-fifo/axis-fifo.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c index c18bf31f55b6..2f609712f507 100644 --- a/drivers/staging/axis-fifo/axis-fifo.c +++ b/drivers/staging/axis-fifo/axis-fifo.c @@ -482,10 +482,10 @@ static ssize_t axis_fifo_write(struct file *f, const char __user *buf, spin_lock_irq(&fifo->write_queue_lock); ret = wait_event_interruptible_lock_irq_timeout (fifo->write_queue, -ioread32(fifo->base_addr + XLLF_TDFV_OFFSET) + ioread32(fifo->base_addr + XLLF_TDFV_OFFSET) >= words_to_write, -fifo->write_queue_lock, -(write_timeout >= 0) ? msecs_to_jiffies(write_timeout) : + fifo->write_queue_lock, + (write_timeout >= 0) ? msecs_to_jiffies(write_timeout) : MAX_SCHEDULE_TIMEOUT); spin_unlock_irq(&fifo->write_queue_lock); -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH V2 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context
From: k...@linuxonhyperv.com Sent: Wednesday, October 17, 2018 10:09 PM > > Currently we are replicating state in struct hv_context that is unnecessary - > this state can be retrieved from the hypervisor. Furthermore, this is a > per-cpu > state that is being maintained as a global state in struct hv_context. > Get rid of this state in struct hv_context. > > Signed-off-by: K. Y. Srinivasan > --- > drivers/hv/hv.c | 10 +++--- > drivers/hv/hyperv_vmbus.h | 2 -- > 2 files changed, 3 insertions(+), 9 deletions(-) > Reviewed-by: Michael Kelley ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up
From: k...@linuxonhyperv.com Sent: Wednesday, October
17, 2018 10:10 PM
> From: Dexuan Cui
>
> In kvp_send_key(), we do need call process_ib_ipinfo() if
> message->kvp_hdr.operation is KVP_OP_GET_IP_INFO, because it turns out
> the userland hv_kvp_daemon needs the info of operation, adapter_id and
> addr_family. With the incorrect fc62c3b1977d, the host can't get the
> VM's IP via KVP.
>
> And, fc62c3b1977d added a "break;", but actually forgot to initialize
> the key_size/value in the case of KVP_OP_SET, so the default key_size of
> 0 is passed to the kvp daemon, and the pool files
> /var/lib/hyperv/.kvp_pool_* can't be updated.
>
> This patch effectively rolls back the previous fc62c3b1977d, and
> correctly fixes the "this statement may fall through" warnings.
>
> This patch is tested on WS 2012 R2 and 2016.
>
> Fixes: fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may fall
> through" warnings")
> Signed-off-by: Dexuan Cui
> Cc: K. Y. Srinivasan
> Cc: Haiyang Zhang
> Cc: Stephen Hemminger
> Cc:
> Signed-off-by: K. Y. Srinivasan
> ---
> drivers/hv/hv_kvp.c | 26 ++
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
Reviewed-by: Michael Kelley
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: adt7316: Switch to the gpio descriptor interface
On Mon, Oct 22, 2018 at 11:22:15PM +0530, Shreeya Patel wrote:
> On Mon, 2018-10-22 at 22:44 +0530, Nishad Kamdar wrote:
> > Use the gpiod interface instead of the deprecated old non-descriptor
> > interface for ldac_pin.
> >
> > Signed-off-by: Nishad Kamdar
> > ---
>
> Hi Nishad,
>
> I have been working on implementing device tree bindings for this
> driver and removing platform data from it.
>
> I've sent a series of patches for it to Jonathan. I didn't send it
> on the mailing list as I wanted to confirm from Jonathan if we wants
> something more to be done in the series. That series also includes
> the change which you are proposing here.
> I am really sorry that you didn't know about this.
> If you are planning to send any other patches for this driver then
> let me know before you start implementing it so that we can
> co-ordinate before doing any work.
>
> Thanks
>
>
> > drivers/staging/iio/addac/adt7316.c | 17 -
> > 1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/iio/addac/adt7316.c
> > b/drivers/staging/iio/addac/adt7316.c
> > index 3f22d1088713..94f945ba0097 100644
> > --- a/drivers/staging/iio/addac/adt7316.c
> > +++ b/drivers/staging/iio/addac/adt7316.c
> > @@ -8,7 +8,7 @@
> > */
> >
> > #include
> > -#include
> > +#include
> > #include
> > #include
> > #include
> > @@ -177,7 +177,7 @@
> >
> > struct adt7316_chip_info {
> > struct adt7316_bus bus;
> > - u16 ldac_pin;
> > + struct gpio_desc*ldac_pin;
> > u16 int_mask; /* 0x2f */
> > u8 config1;
> > u8 config2;
> > @@ -950,8 +950,8 @@ static ssize_t adt7316_store_update_DAC(struct
> > device *dev,
> > if (ret)
> > return -EIO;
> > } else {
> > - gpio_set_value(chip->ldac_pin, 0);
> > - gpio_set_value(chip->ldac_pin, 1);
> > + gpiod_set_value(chip->ldac_pin, 0);
> > + gpiod_set_value(chip->ldac_pin, 1);
> > }
> >
> > return len;
> > @@ -2120,7 +2120,14 @@ int adt7316_probe(struct device *dev, struct
> > adt7316_bus *bus,
> > else
> > return -ENODEV;
> >
> > - chip->ldac_pin = adt7316_platform_data[1];
> > + chip->ldac_pin = devm_gpiod_get(dev, "ldac",
> > GPIOD_OUT_HIGH);
> > + if (IS_ERR(chip->ldac_pin)) {
> > + ret = PTR_ERR(chip->ldac_pin);
> > + dev_err(dev, "Failed to request ldac GPIO: %d\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > if (chip->ldac_pin) {
> > chip->config3 |= ADT7316_DA_EN_VIA_DAC_LDCA;
> > if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
Hello Shreeya,
Sure no problem.
Thanks and regards,
Nishad
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: clarify/unify macros for NI macro-defined terminals
On 24/10/18 15:33, Spencer E. Olson wrote:
Uses a single macro to define multiple macros that represent a series of
terminals for NI devices. This patch also redefines NI_MAX_COUNTERS as the
maximum number of counters possible on NI devices (instead of the maximum
index of the counters). This was a little confusing and caused a bug in
commit 347e244884c3b ("staging: comedi: tio: implement global tio/ctr routing")
when setting/reading registers for counter terminals.
Fixes: 347e244884c3b ("staging: comedi: tio: implement global tio/ctr routing")
Signed-off-by: Spencer E. Olson
---
drivers/staging/comedi/comedi.h | 39 ++---
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/comedi/comedi.h b/drivers/staging/comedi/comedi.h
index e90b17775284..09a940066c0e 100644
--- a/drivers/staging/comedi/comedi.h
+++ b/drivers/staging/comedi/comedi.h
@@ -1005,35 +1005,38 @@ enum i8254_mode {
* and INSN_DEVICE_CONFIG_GET_ROUTES.
*/
#define NI_NAMES_BASE 0x8000u
+
+#define _TERM_N(base, n, x)((base) + ((x) & ((n) - 1)))
+
/*
* not necessarily all allowed 64 PFIs are valid--certainly not for all
devices
*/
-#define NI_PFI(x) (NI_NAMES_BASE+ ((x) & 0x3f))
+#define NI_PFI(x) _TERM_N(NI_NAMES_BASE, 64, x)
/* 8 trigger lines by standard, Some devices cannot talk to all eight. */
-#define TRIGGER_LINE(x)(NI_PFI(-1) + 1 + ((x) & 0x7))
+#define TRIGGER_LINE(x)_TERM_N(NI_PFI(-1) + 1, 8, x)
/* 4 RTSI shared MUXes to route signals to/from TRIGGER_LINES on NI hardware
*/
-#define NI_RTSI_BRD(x) (TRIGGER_LINE(-1) + 1 + ((x) & 0x3))
+#define NI_RTSI_BRD(x) _TERM_N(TRIGGER_LINE(-1) + 1, 4, x)
/* *** Counter/timer names : 8 counters max *** */
-#define NI_COUNTER_NAMES_BASE (NI_RTSI_BRD(-1) + 1)
-#define NI_MAX_COUNTERS 7
-#define NI_CtrSource(x) (NI_COUNTER_NAMES_BASE + ((x) &
NI_MAX_COUNTERS))
+#define NI_MAX_COUNTERS8
+#define NI_COUNTER_NAMES_BASE (NI_RTSI_BRD(-1) + 1)
+#define NI_CtrSource(x) _TERM_N(NI_COUNTER_NAMES_BASE,
NI_MAX_COUNTERS, x)
/* Gate, Aux, A,B,Z are all treated, at times as gates */
-#define NI_GATES_NAMES_BASE(NI_CtrSource(-1) + 1)
-#define NI_CtrGate(x) (NI_GATES_NAMES_BASE + ((x) & NI_MAX_COUNTERS))
-#define NI_CtrAux(x) (NI_CtrGate(-1) + 1 + ((x) & NI_MAX_COUNTERS))
-#define NI_CtrA(x)(NI_CtrAux(-1)+ 1 + ((x) & NI_MAX_COUNTERS))
-#define NI_CtrB(x)(NI_CtrA(-1) + 1 + ((x) & NI_MAX_COUNTERS))
-#define NI_CtrZ(x)(NI_CtrB(-1) + 1 + ((x) & NI_MAX_COUNTERS))
-#define NI_GATES_NAMES_MAX NI_CtrZ(-1)
-#define NI_CtrArmStartTrigger(x) (NI_CtrZ(-1)+ 1 + ((x) &
NI_MAX_COUNTERS))
+#define NI_GATES_NAMES_BASE(NI_CtrSource(-1) + 1)
+#define NI_CtrGate(x) _TERM_N(NI_GATES_NAMES_BASE, NI_MAX_COUNTERS, x)
+#define NI_CtrAux(x) _TERM_N(NI_CtrGate(-1) + 1, NI_MAX_COUNTERS, x)
+#define NI_CtrA(x) _TERM_N(NI_CtrAux(-1) + 1, NI_MAX_COUNTERS, x)
+#define NI_CtrB(x) _TERM_N(NI_CtrA(-1) + 1, NI_MAX_COUNTERS, x)
+#define NI_CtrZ(x) _TERM_N(NI_CtrB(-1) + 1, NI_MAX_COUNTERS, x)
+#define NI_GATES_NAMES_MAX NI_CtrZ(-1)
+#define NI_CtrArmStartTrigger(x) _TERM_N(NI_CtrZ(-1)+ 1, NI_MAX_COUNTERS,
x)
#define NI_CtrInternalOutput(x) \
-(NI_CtrArmStartTrigger(-1) + 1 + ((x) & NI_MAX_COUNTERS))
+ _TERM_N(NI_CtrArmStartTrigger(-1) + 1, NI_MAX_COUNTERS, x)
/** external pin(s) labeled conveniently as CtrOut. */
-#define NI_CtrOut(x) (NI_CtrInternalOutput(-1) + 1 + ((x) &
NI_MAX_COUNTERS))
+#define NI_CtrOut(x) _TERM_N(NI_CtrInternalOutput(-1) + 1, NI_MAX_COUNTERS,
x)
/** For Buffered sampling of ctr -- x series capability. */
-#define NI_CtrSampleClock(x) (NI_CtrOut(-1) + 1 + ((x) & NI_MAX_COUNTERS))
-#define NI_COUNTER_NAMES_MAX NI_CtrSampleClock(-1)
+#define NI_CtrSampleClock(x) _TERM_N(NI_CtrOut(-1) + 1, NI_MAX_COUNTERS, x)
+#define NI_COUNTER_NAMES_MAX NI_CtrSampleClock(-1)
enum ni_common_signal_names {
/* PXI_Star: this is a non-NI-specific signal */
It's no more ugly than the original, although I pity the fool who has to
make sense of the preprocessor output!
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: clarify/unify macros for NI macro-defined terminals
On Wed, Oct 24, 2018 at 10:39 AM Ian Abbott wrote:
>
> On 24/10/18 15:33, Spencer E. Olson wrote:
> > Uses a single macro to define multiple macros that represent a series of
> > terminals for NI devices. This patch also redefines NI_MAX_COUNTERS as the
> > maximum number of counters possible on NI devices (instead of the maximum
> > index of the counters). This was a little confusing and caused a bug in
> > commit 347e244884c3b ("staging: comedi: tio: implement global tio/ctr
> > routing")
> > when setting/reading registers for counter terminals.
> >
> > Fixes: 347e244884c3b ("staging: comedi: tio: implement global tio/ctr
> > routing")
> > Signed-off-by: Spencer E. Olson
> > ---
> > drivers/staging/comedi/comedi.h | 39 ++---
> > 1 file changed, 21 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/comedi.h
> > b/drivers/staging/comedi/comedi.h
> > index e90b17775284..09a940066c0e 100644
> > --- a/drivers/staging/comedi/comedi.h
> > +++ b/drivers/staging/comedi/comedi.h
> > @@ -1005,35 +1005,38 @@ enum i8254_mode {
> >* and INSN_DEVICE_CONFIG_GET_ROUTES.
> >*/
> > #define NI_NAMES_BASE 0x8000u
> > +
> > +#define _TERM_N(base, n, x) ((base) + ((x) & ((n) - 1)))
> > +
> > /*
> >* not necessarily all allowed 64 PFIs are valid--certainly not for all
> > devices
> >*/
> > -#define NI_PFI(x)(NI_NAMES_BASE+ ((x) & 0x3f))
> > +#define NI_PFI(x)_TERM_N(NI_NAMES_BASE, 64, x)
> > /* 8 trigger lines by standard, Some devices cannot talk to all eight. */
> > -#define TRIGGER_LINE(x) (NI_PFI(-1) + 1 + ((x) & 0x7))
> > +#define TRIGGER_LINE(x) _TERM_N(NI_PFI(-1) + 1, 8, x)
> > /* 4 RTSI shared MUXes to route signals to/from TRIGGER_LINES on NI
> > hardware */
> > -#define NI_RTSI_BRD(x) (TRIGGER_LINE(-1) + 1 + ((x) & 0x3))
> > +#define NI_RTSI_BRD(x) _TERM_N(TRIGGER_LINE(-1) + 1, 4, x)
> >
> > /* *** Counter/timer names : 8 counters max *** */
> > -#define NI_COUNTER_NAMES_BASE (NI_RTSI_BRD(-1) + 1)
> > -#define NI_MAX_COUNTERS 7
> > -#define NI_CtrSource(x) (NI_COUNTER_NAMES_BASE + ((x) &
> > NI_MAX_COUNTERS))
> > +#define NI_MAX_COUNTERS 8
> > +#define NI_COUNTER_NAMES_BASE(NI_RTSI_BRD(-1) + 1)
> > +#define NI_CtrSource(x)_TERM_N(NI_COUNTER_NAMES_BASE,
> > NI_MAX_COUNTERS, x)
> > /* Gate, Aux, A,B,Z are all treated, at times as gates */
> > -#define NI_GATES_NAMES_BASE(NI_CtrSource(-1) + 1)
> > -#define NI_CtrGate(x) (NI_GATES_NAMES_BASE + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_CtrAux(x)(NI_CtrGate(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_CtrA(x) (NI_CtrAux(-1)+ 1 + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_CtrB(x) (NI_CtrA(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_CtrZ(x) (NI_CtrB(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_GATES_NAMES_MAX NI_CtrZ(-1)
> > -#define NI_CtrArmStartTrigger(x) (NI_CtrZ(-1)+ 1 + ((x) &
> > NI_MAX_COUNTERS))
> > +#define NI_GATES_NAMES_BASE (NI_CtrSource(-1) + 1)
> > +#define NI_CtrGate(x)_TERM_N(NI_GATES_NAMES_BASE,
> > NI_MAX_COUNTERS, x)
> > +#define NI_CtrAux(x) _TERM_N(NI_CtrGate(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > +#define NI_CtrA(x) _TERM_N(NI_CtrAux(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > +#define NI_CtrB(x) _TERM_N(NI_CtrA(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > +#define NI_CtrZ(x) _TERM_N(NI_CtrB(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > +#define NI_GATES_NAMES_MAX NI_CtrZ(-1)
> > +#define NI_CtrArmStartTrigger(x) _TERM_N(NI_CtrZ(-1)+ 1,
> > NI_MAX_COUNTERS, x)
> > #define NI_CtrInternalOutput(x) \
> > - (NI_CtrArmStartTrigger(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > + _TERM_N(NI_CtrArmStartTrigger(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > /** external pin(s) labeled conveniently as CtrOut. */
> > -#define NI_CtrOut(x) (NI_CtrInternalOutput(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > +#define NI_CtrOut(x) _TERM_N(NI_CtrInternalOutput(-1) + 1,
> > NI_MAX_COUNTERS, x)
> > /** For Buffered sampling of ctr -- x series capability. */
> > -#define NI_CtrSampleClock(x) (NI_CtrOut(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_COUNTER_NAMES_MAX NI_CtrSampleClock(-1)
> > +#define NI_CtrSampleClock(x) _TERM_N(NI_CtrOut(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > +#define NI_COUNTER_NAMES_MAX NI_CtrSampleClock(-1)
> >
> > enum ni_common_signal_names {
> > /* PXI_Star: this is a non-NI-specific signal */
> >
>
> It's no more ugly than the original, although I pity the fool who has to
> make sense of the preprocessor output!
Unfortunately, I do agree. Oh well.
>
> Reviewed-by: Ian Abbott
>
> --
> -=( Ian Abbott || Web: www.mev.co.uk )=-
> -=( MEV Ltd. is a company registered in England & Wales. )=
Re: [PATCH v2] staging: comedi: ni_mio_common: scale ao INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS
On 24/10/18 15:46, Spencer E. Olson wrote:
Fixes implementation of INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS for
ni_mio devices. The previous patch should have used the channel
information passed in to scale the result by the number of channels being
used.
Fixes: 51fd36738383 ("staging: comedi: ni_mio_common: implement
INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS")
Signed-off-by: Spencer E. Olson
---
drivers/staging/comedi/drivers/ni_mio_common.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
b/drivers/staging/comedi/drivers/ni_mio_common.c
index 2d1e0325d04d..5edf59ac6706 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -2843,7 +2843,8 @@ static int ni_ao_insn_config(struct comedi_device *dev,
return ni_ao_arm(dev, s);
case INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS:
/* we don't care about actual channels */
- data[1] = board->ao_speed;
+ /* data[3] : chanlist_len */
+ data[1] = board->ao_speed * data[3];
data[2] = 0;
return 0;
default:
Thanks. Hopefully this and your other patch will make it into the next
version of the kernel (4.20? 5.0?). If not, they should be backported.
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [alsa-devel] [PATCH v2] staging: bcm2835-audio: interpolate audio delay
Hello Mike,
On 10/24/18 10:20, Mike Brady wrote:
> Hi Kirill. Thanks for your comments.
>
>> On 22 Oct 2018, at 23:25, Kirill Marinushkin wrote:
>>
>> AFAIU, this patch is wrong. Please correct me, maybe I misunderstand
>> something.
>>
>>> The problem that this patch seeks to resolve is that when userland asks for
>>> the delay
>>
>> The userspace asks not for delay, but for the pointer.
>
> The call in question is snd_pcm_delay. I presume this delay is calculated
> from knowledge of the stream position “pos", the period (buffer?) number (and
> period/buffer size) and the snd_pcm_runtime structure’s “delay" field
> (“runtime->delay”).
>
In kernel, this delay is calculated in `snd_pcm_calc_delay()`, defined at
`sound/core/pcm_native.c:889`. If you analyze how this calculation is done, you
will see that the returned value is a summary of the following fields:
* runtime->status->hw_ptr
* runtime->buffer_size
* runtime->control->appl_ptr
* runtime->boundary
* runtime->delay
>> You modify the function, which is called `snd_bcm2835_pcm_pointer`. Here you
>> are
>> supposed to increase `alsa_stream->pos` with the proper offset. Instead, you
>> imitate a delay, but in fact the delay is not increased.
>>
>> So, the proper solution should be to fix the reported pointer.
>
> I think there is a difficulty with this. The “pos” pointer looks to have to
> be modulo the buffer size. This causes a problem, as I see it, in that if the
> calculated (pos + interpolated delay in bytes) is longer than the buffer size
There is no "interpolated delay". The concept of "interpolated delay" is
incorrect. When you play sound - the pointer increments. But in this commit you
increment the delay, as if sound doesn't play.
>> As a result,
>> userspace will recieve the correct delay, instead of these crazy 10 ms.
>
> Just to point out that with the proposed patch, it appears that the correct
> delay is being reported, (apart, possibly, from any delay originally set in
> the snd_pcm_delay field, as mentioned above).
Then I would like to point out the alsa-lib function `snd_pcm_avail()` - it will
return the wrong value.
>
> All the best,
> Mike
>
>> FYI, there is
>>> a discussion of the effects of a downstream equivalent of this suggested
>>> patch
>>> at:
>>> https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016.
>>
>> Thank you for the link, it clarified for me what you try to achieve.
>>
>> On 10/22/18 21:17, Mike Brady wrote:
>>> When the BCM2835 audio output is used, userspace sees a jitter up to 10ms
>>> in the audio position, aka "delay" -- the number of frames that must
>>> be output before a new frame would be played.
>>> Make this a bit nicer for userspace by interpolating the position
>>> using the CPU clock.
>>> The overhead is small -- an extra ktime_get() every time a GPU message
>>> is sent -- and another call and a few calculations whenever the delay
>>> is sought from userland.
>>> At 48,000 frames per second, i.e. approximately 20 microseconds per
>>> frame, it would take a clock inaccuracy of
>>> 20 microseconds in 10 milliseconds -- 2,000 parts per million --
>>> to result in an inaccurate estimate, whereas
>>> crystal- or resonator-based clocks typically have an
>>> inaccuracy of 10s to 100s of parts per million.
>>>
>>> Signed-off-by: Mike Brady
>>> ---
>>> Changes in v2 -- remove inappropriate addition of SNDRV_PCM_INFO_BATCH flag
>>>
>>> .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 20 +++
>>> .../vc04_services/bcm2835-audio/bcm2835.h | 1 +
>>> 2 files changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>> b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>> index e66da11af5cf..9053b996cada 100644
>>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>> @@ -74,6 +74,7 @@ void bcm2835_playback_fifo(struct bcm2835_alsa_stream
>>> *alsa_stream,
>>> atomic_set(&alsa_stream->pos, pos);
>>>
>>> alsa_stream->period_offset += bytes;
>>> + alsa_stream->interpolate_start = ktime_get();
>>> if (alsa_stream->period_offset >= alsa_stream->period_size) {
>>> alsa_stream->period_offset %= alsa_stream->period_size;
>>> snd_pcm_period_elapsed(substream);
>>> @@ -243,6 +244,7 @@ static int snd_bcm2835_pcm_prepare(struct
>>> snd_pcm_substream *substream)
>>> atomic_set(&alsa_stream->pos, 0);
>>> alsa_stream->period_offset = 0;
>>> alsa_stream->draining = false;
>>> + alsa_stream->interpolate_start = ktime_get();
>>>
>>> return 0;
>>> }
>>> @@ -292,6 +294,24 @@ snd_bcm2835_pcm_pointer(struct snd_pcm_substream
>>> *substream)
>>> {
>>> struct snd_pcm_runtime *runtime = substream->runtime;
>>> struct bcm2835_alsa_stream *alsa_stream = runtime->private_data;
>>> + ktime_t now = ktime_get();
>>> +
>>> + /* Give userspace better delay reporting by i
Re: [alsa-devel] [PATCH v2] staging: bcm2835-audio: interpolate audio delay
Hi Kirill. Thanks again for your comments. > On 24 Oct 2018, at 19:06, Kirill Marinushkin wrote: > > Hello Mike, > > On 10/24/18 10:20, Mike Brady wrote: >> Hi Kirill. Thanks for your comments. >> >>> On 22 Oct 2018, at 23:25, Kirill Marinushkin >>> wrote: >>> >>> AFAIU, this patch is wrong. Please correct me, maybe I misunderstand >>> something. >>> The problem that this patch seeks to resolve is that when userland asks for the delay >>> >>> The userspace asks not for delay, but for the pointer. >> >> The call in question is snd_pcm_delay. I presume this delay is calculated >> from knowledge of the stream position “pos", the period (buffer?) number >> (and period/buffer size) and the snd_pcm_runtime structure’s “delay" field >> (“runtime->delay”). >> > > > In kernel, this delay is calculated in `snd_pcm_calc_delay()`, defined at > `sound/core/pcm_native.c:889`. If you analyze how this calculation is done, > you > will see that the returned value is a summary of the following fields: > > * runtime->status->hw_ptr > * runtime->buffer_size > * runtime->control->appl_ptr > * runtime->boundary > * runtime->delay That’s very useful, thanks. >>> You modify the function, which is called `snd_bcm2835_pcm_pointer`. Here >>> you are >>> supposed to increase `alsa_stream->pos` with the proper offset. Instead, you >>> imitate a delay, but in fact the delay is not increased. >>> >>> So, the proper solution should be to fix the reported pointer. >> >> I think there is a difficulty with this. The “pos” pointer looks to have to >> be modulo the buffer size. This causes a problem, as I see it, in that if >> the calculated (pos + interpolated delay in bytes) is longer than the buffer >> size > > > There is no "interpolated delay". The concept of "interpolated delay" is > incorrect. Yes, my language here is wrong. What I mean is the estimated number of frames output since the pointer was last updated — let’s call it the `interpolated frame count`. > When you play sound - the pointer increments. Unfortunately, when you play sound, the pointer does not actually increment, for up to about 10 milliseconds. I know of no way to actually access the true “live” position of the frame that is being played at any instant; hence the desire to estimate it. What actually seems to be happening is that when `bcm2835_playback_fifo` is called, the pointer is updated, but as frames are individually output to the DAC, this pointer does not increment. It is not updated until the next time `bcm2835_playback_fifo` is called. > But in this commit you increment the delay, as if sound doesn't play. It is true that the patch does make use of the snd_pcm_runtime structure’s “delay" field (aka "runtime->delay” here). That field is defined for: “/* extra delay; typically FIFO size */”. Clearly it is not being used for that here — it is being used simply because it is part of the calculation done in snd_pcm_calc_delay(), as you point out. At present, it looks like that field isn’t being used –– it’s set to zero –– and not modified anywhere else in the driver, AFAICS. If it was necessary, it would be a simple matter to preserve whatever value it was given. >>> As a result, >>> userspace will recieve the correct delay, instead of these crazy 10 ms. >> >> Just to point out that with the proposed patch, it appears that the correct >> delay is being reported, (apart, possibly, from any delay originally set in >> the snd_pcm_delay field, as mentioned above). > > > Then I would like to point out the alsa-lib function `snd_pcm_avail()` - it > will return the wrong value. It is already the case that snd_pcm_avail() does not return the true delay. The ALSA documentation states: "The value returned by that call [i.e. the snd_pcm_avail*() functions] is not directly related to the delay…” Kind regards Mike >> >>> FYI, there is a discussion of the effects of a downstream equivalent of this suggested patch at: https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016. >>> >>> Thank you for the link, it clarified for me what you try to achieve. >>> >>> On 10/22/18 21:17, Mike Brady wrote: When the BCM2835 audio output is used, userspace sees a jitter up to 10ms in the audio position, aka "delay" -- the number of frames that must be output before a new frame would be played. Make this a bit nicer for userspace by interpolating the position using the CPU clock. The overhead is small -- an extra ktime_get() every time a GPU message is sent -- and another call and a few calculations whenever the delay is sought from userland. At 48,000 frames per second, i.e. approximately 20 microseconds per frame, it would take a clock inaccuracy of 20 microseconds in 10 milliseconds -- 2,000 parts per million -- to result in an inaccurate estimate, whereas crystal- or resonator-based clocks typically have an >>
Re: [alsa-devel] [PATCH v2] staging: bcm2835-audio: interpolate audio delay
Hello Mike, We are not on the same page. What you hear is not what I tell you. Either you don't understand what happens in your commit, or I don't understand what happens in the driver. Hopefully somebody in the community can comment here. On 10/24/18 21:54, Mike Brady wrote: You modify the function, which is called `snd_bcm2835_pcm_pointer`. Here you are supposed to increase `alsa_stream->pos` with the proper offset. Instead, you imitate a delay, but in fact the delay is not increased. So, the proper solution should be to fix the reported pointer. >>> >>> I think there is a difficulty with this. The “pos” pointer looks to have to >>> be modulo the buffer size. This causes a problem, as I see it, in that if >>> the calculated (pos + interpolated delay in bytes) is longer than the >>> buffer size >> >> >> There is no "interpolated delay". The concept of "interpolated delay" is >> incorrect. > > Yes, my language here is wrong. What I mean is the estimated number of frames > output since the pointer was last updated — let’s call it the `interpolated > frame count`. > That's not what I mean. From my perspective, the problem is not in language, but in the concept which you introduce here. >> When you play sound - the pointer increments. > > Unfortunately, when you play sound, the pointer does not actually increment, > for up to about 10 milliseconds. I know of no way to actually access the true > “live” position of the frame that is being played at any instant; hence the > desire to estimate it. > Your vision of situation in the opposite from my vision. What you see as a symptom - I see as a root cause. As I see, you should fix the pointer-not-incrementing. Why do you think that it's okay that the pointer is not updating during sound play? Why do you insist that there is a delay? I don't understand why we are so stuck here. > What actually seems to be happening is that when `bcm2835_playback_fifo` is > called, the pointer is updated, but as frames are individually output to the > DAC, this pointer does not increment. It is not updated until the next time > `bcm2835_playback_fifo` is called. > >> But in this commit you increment the delay, as if sound doesn't play. > > It is true that the patch does make use of the snd_pcm_runtime structure’s > “delay" field (aka "runtime->delay” here). That field is defined for: “/* > extra delay; typically FIFO size */”. Clearly it is not being used for that > here — it is being used simply because it is part of the calculation done in > snd_pcm_calc_delay(), as you point out. At present, it looks like that field > isn’t being used –– it’s set to zero –– and not modified anywhere else in the > driver, AFAICS. If it was necessary, it would be a simple matter to preserve > whatever value it was given. > That's not what I am talking about. Somehow we don't understand each other. As a result, userspace will recieve the correct delay, instead of these crazy 10 ms. >>> >>> Just to point out that with the proposed patch, it appears that the correct >>> delay is being reported, (apart, possibly, from any delay originally set in >>> the snd_pcm_delay field, as mentioned above). >> >> >> Then I would like to point out the alsa-lib function `snd_pcm_avail()` - it >> will return the wrong value. > > It is already the case that snd_pcm_avail() does not return the true delay. > The ALSA documentation states: "The value returned by that call [i.e. the > snd_pcm_avail*() functions] is not directly related to the delay…” > Do you mean, that you are submitting the patch into the upstream kernel without reading the code? snd_pcm_avail() is calculated based on: * hw_ptr * buffer_size * appl_ptr * boundary If you fix hw_ptr - it will fix both snd_pcm_delay() and snd_pcm_avail(). Instead, you invent the "interpolated delay", which in fact only compensates the wrong hw_ptr instead of fixing it. Best Regards, Kirill ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: change do_insn*_ioctl to allow more samples
Changes do_insn*_ioctl functions to allow for data lengths for each
comedi_insn of up to 2^16. This patch also changes these functions to only
allocate as much memory as is necessary for each comedi_insn, rather than
allocating a fixed-sized scratch space.
In testing some user-space code for the new INSN_DEVICE_CONFIG_GET_ROUTES
facility with some newer hardware, I discovered that do_insn_ioctl and
do_insnlist_ioctl limited the amount of data that can be passed into the
kernel for insn's to a length of 256. For some newer hardware, the number
of routes can be greater than 1000. Working around the old limits (256)
would complicate the user-space/kernel interaction.
The new upper limit is reasonable with current memory available and does
not otherwise impact the memory footprint for any current or otherwise
typical configuration.
Signed-off-by: Spencer E. Olson
---
drivers/staging/comedi/comedi_fops.c | 37 +---
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c
b/drivers/staging/comedi/comedi_fops.c
index c1c6b2b4ab91..a163ec2df872 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1500,7 +1500,7 @@ static int parse_insn(struct comedi_device *dev, struct
comedi_insn *insn,
* data (for reads) to insns[].data pointers
*/
/* arbitrary limits */
-#define MAX_SAMPLES 256
+#define MAX_SAMPLES 65536
static int do_insnlist_ioctl(struct comedi_device *dev,
struct comedi_insnlist __user *arg, void *file)
{
@@ -1513,12 +1513,6 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
if (copy_from_user(&insnlist, arg, sizeof(insnlist)))
return -EFAULT;
- data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
- if (!data) {
- ret = -ENOMEM;
- goto error;
- }
-
insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL);
if (!insns) {
ret = -ENOMEM;
@@ -1539,6 +1533,14 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
ret = -EINVAL;
goto error;
}
+
+ data = kmalloc_array(insns[i].n, sizeof(unsigned int),
+GFP_KERNEL);
+ if (!data) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
if (insns[i].insn & INSN_MASK_WRITE) {
if (copy_from_user(data, insns[i].data,
insns[i].n * sizeof(unsigned int))) {
@@ -1560,6 +1562,11 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
goto error;
}
}
+
+ /* free up the single-instruction data memory */
+ kfree(data);
+ data = NULL;
+
if (need_resched())
schedule();
}
@@ -1594,20 +1601,20 @@ static int do_insn_ioctl(struct comedi_device *dev,
unsigned int *data = NULL;
int ret = 0;
- data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
- if (!data) {
- ret = -ENOMEM;
- goto error;
- }
-
if (copy_from_user(&insn, arg, sizeof(insn))) {
- ret = -EFAULT;
- goto error;
+ return -EFAULT;
}
/* This is where the behavior of insn and insnlist deviate. */
if (insn.n > MAX_SAMPLES)
insn.n = MAX_SAMPLES;
+
+ data = kmalloc_array(insn.n, sizeof(unsigned int), GFP_KERNEL);
+ if (!data) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
if (insn.insn & INSN_MASK_WRITE) {
if (copy_from_user(data,
insn.data,
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: axis-fifo: Fix line over 80 characters error
On Wed, Oct 24, 2018 at 05:05:53PM +0200, Aleksa Zdravkovic wrote: > This patch fixes the checkpatch.pl warning: > > WARNING: line over 80 characters > + (write_timeout >= 0) ? msecs_to_jiffies(write_timeout) > : > > Signed-off-by: Aleksa Zdravkovic > --- > drivers/staging/axis-fifo/axis-fifo.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/axis-fifo/axis-fifo.c > b/drivers/staging/axis-fifo/axis-fifo.c > index c18bf31f55b6..2f609712f507 100644 > --- a/drivers/staging/axis-fifo/axis-fifo.c > +++ b/drivers/staging/axis-fifo/axis-fifo.c > @@ -482,10 +482,10 @@ static ssize_t axis_fifo_write(struct file *f, const > char __user *buf, > spin_lock_irq(&fifo->write_queue_lock); > ret = wait_event_interruptible_lock_irq_timeout > (fifo->write_queue, > - ioread32(fifo->base_addr + XLLF_TDFV_OFFSET) > + ioread32(fifo->base_addr + XLLF_TDFV_OFFSET) > >= words_to_write, > - fifo->write_queue_lock, > - (write_timeout >= 0) ? msecs_to_jiffies(write_timeout) > : > + fifo->write_queue_lock, > + (write_timeout >= 0) ? msecs_to_jiffies(write_timeout) : > MAX_SCHEDULE_TIMEOUT); The original was fine. Just leave it. Checkpatch.pl is only useful if it improves the readability for humans. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: iio: ad2s1210: Switch to the gpio descriptor interface
On paź 24, 2018 20:20, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated old non-descriptor
> interface.
>
> Signed-off-by: Nishad Kamdar
> ---
> Changes in v4:
> - Add spaces after { and before } in gpios[]
>initialization.
> - Check the correct pointer for error.
> - Align the dev_err msg to existing format in the code.
> Changes in v3:
> - Use a pointer to pointer for gpio_desc in
>struct ad2s1210_gpio as it will be used to
>modify a pointer.
> - Use dot notation to initialize the structure.
> - Use a pointer variable to avoid writing gpios[i].
> Changes in v2:
> - Use the spi_device struct embedded in st instead
>of passing it as an argument to ad2s1210_setup_gpios().
> - Use an array of structs to reduce redundant code in
>in ad2s1210_setup_gpios().
> - Remove ad2s1210_free_gpios() as devm API is being used.
> ---
> drivers/staging/iio/resolver/ad2s1210.c | 92 ++---
> drivers/staging/iio/resolver/ad2s1210.h | 3 -
> 2 files changed, 50 insertions(+), 45 deletions(-)
Looks good to me.
Reviewed-by: Slawomir Stepien
--
Slawomir Stepien
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
