Hi Daniel,
On 29 August 2014 12:52, Daniel Mack <daniel@xxxxxxxxxx> wrote:
> Hi,
>
> On 08/29/2014 08:13 AM, Jassi Brar wrote:
>> +/*
>> + * 5512.5Hz is going to need the maximum number of elements (80),
>> + * in the length-pattern loop, among standard ALSA supported rates.
>> + */
>> +#define MAX_LOOP_LEN 80
>> +
>> struct uac2_rtd_params {
>> struct snd_uac2_chip *uac2; /* parent chip */
>> bool ep_enabled; /* if the ep is enabled */
>> @@ -80,6 +87,9 @@ struct uac2_rtd_params {
>> unsigned max_psize;
>> struct uac2_req ureq[USB_XFERS];
>>
>> + unsigned pattern[MAX_LOOP_LEN];
>> + unsigned plen; /* valid entries in pattern[] */
>> +
>> spinlock_t lock;
>> };
>>
>
> You're doing this for both directions, while only the capture side needs
> such treatment.
>
Playback specific parameters placed in a common structure is a
tradeoff, I chose to avoid. Please also note, this makes code a bit
more symmetric in set_alt().
But I can move this to common structure if it pleases Gods.
>> +/*
>> + * Find optimal pattern of payloads for a given number
>> + * of samples and maximum sync period (in ms) over which
>> + * we have to distribute them uniformly.
>> + */
>> +static unsigned
>> +get_pattern(unsigned samples, unsigned sync, unsigned *pt)
>> +{
>> + unsigned n, x = 0, i = 0, p = samples % sync;
>> +
>> + do {
>> + x += p;
>> + n = samples / sync;
>> + if (x >= sync) {
>> + n += 1;
>> + x -= sync;
>> + }
>> + if (pt)
>> + pt[i] = n;
>> + i++;
>> + } while (x);
>> +
>> + return i;
>> +}
>>
>> static int
>> afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
>> {
>> @@ -1097,11 +1136,35 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
>> if (intf == agdev->as_out_intf) {
>> ep = agdev->out_ep;
>> prm = &uac2->c_prm;
>> + prm->plen = 1;
>> + prm->pattern[0] = prm->max_psize;
>> config_ep_by_speed(gadget, fn, ep);
>> agdev->as_out_alt = alt;
>> } else if (intf == agdev->as_in_intf) {
>> + struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
>> + unsigned intvl, rate;
>> +
>> + if (gadget->speed == USB_SPEED_FULL)
>> + intvl = (1 << (fs_epin_desc.bInterval - 1)) * 1000;
>> + else
>> + intvl = (1 << (hs_epin_desc.bInterval - 1)) * 125;
>> +
>> + rate = opts->p_srate;
>> + if (rate == 5512) { /* which implies 5512.5 practically */
>> + rate = 55125;
>> + intvl *= 10;
>> + }
>> +
>
> Well, I'd say that Alan's approach as implemented in my v6 is still more
> comprehensible for anyone who'll try to understand this later, and it
> doesn't make any assumption on run time values.
>
I hope you realize this 5512 check has nothing to do with the Alan's
approach .... I can very well remove the check and it will still work!
It is only an added feature to provide accurate data rate because
ALSA API can not define fractional rates.
> Plus, it only adds 5
> unsigned ints to struct snd_uac2_chip, while your version blows up
> struct uac2_rtd_params by 81 ints.
>
Sorry, that is bad comparison.
Real comparison is between having
+ unsigned int p_interval;
+ unsigned int p_residue;
+ unsigned int p_pktsize;
+ unsigned int p_pktsize_residue;
+ unsigned int p_framesize;
vs
+ unsigned idx;
+ unsigned pattern[MAX_LOOP_LEN];
+ unsigned plen;
That is, managing 5 vs 3 extra variables in code.
Please also look at the 'hotspot' iso_complete() function that
remains almost unchanged in my patch (as I have been suggesting you
all along).
> But it's also a matter of taste, so I guess it's ultimately up to Felipe
> now.
>
As a co-author of this driver, may I say its more than taste.
This implementation is cleaner (see your patch and this patch in
vertically split vim) and faster (see almost zero overhead in
iso_complete()).
Thanks
-Jassi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
