Re: [PATCH 3/3] Add handling CMP output connection

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

Clemens,

Thanks for your review. I arrange these issues to 5 items below.

cmp[PATCH 3/3]:
1. Calculating offset should be moved into a helper function because 
they are used twice.
OK. I push them into a helper function.

2. Comments should be start with capital letter in each sentenses.
OK. I rewrite it.

3. Use DIV_ROUND_UP macro instead of "for" loop.
OK. I rewrite with the macro.

4. This superfluous comment should say anything to be obvious about the 
code.
OK. I remove the comment.

5. In an oPCR, the payload field is changed only by the device.
OK. I misunderstand the specification. I check IEC 611883-1:2008 and I 
should follow the rules in "7.9 Plug control register modification 
rules". It menthions the fields which should be modified in the same 
compare_swap lock transaction and payload field is not included in it.

Then I have a question about handling payload field. In specification, 
"The payload field shall specify the maximum number of quadlets that may 
be transmitted in a single isochronous packet for this plug" but 
actually cheking this field seems to make no sense because there is no 
checking process in the procedure. I think I can do nothing for payload 
field.


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp

(Apr 28 2013 21:52), Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> To handle CMP output connection, this patch adds some macros, codes with
>> condition of direction and new functions. Once cmp_connection_init() is
>> executed with its direction, CMP input and output connection can be
>> handled by the same way.
>
>> +++ b/sound/firewire/cmp.c
>
>> +	if (c->direction == CMP_INPUT)
>> +		offset = CSR_REGISTER_BASE + CSR_IPCR(c->pcr_index);
>> +	else
>> +		offset = CSR_REGISTER_BASE + CSR_OPCR(c->pcr_index);
>
> This code is used twice and could be moved into a helper function.
>
>> +static int get_overhead_id(struct cmp_connection *c)
>
>> +	/*
>> +	 * apply "oPCR overhead ID encoding"
>> +	 * the encoding table can convert up to 512.
>> +	 * here the value over 512 is converted as the same way as 512.
>> +	 */
>
> 	/*
> 	 * Apply "oPCR overhead ID encoding":
> 	 * The encoding table can convert up to 512.
> 	 * Here any value over 512 is converted in the same way as 512.
> 	 */
>
>> +	for (id = 1; id < 16; id += 1) {
>> +		if (c->resources.bandwidth_overhead < (id << 5))
>> +			break;
>> +	}
>> +	if (id == 16)
>> +		id = 0;
>
> 	id = DIV_ROUND_UP(c->resources.bandwidth_overhead, 32);
> 	if (id >= 16)
> 		id = 0;
>
>> +static __be32 opcr_set_modify(struct cmp_connection *c, __be32 opcr)
>
>> +	/* generate speed and extended speed field value */
>
> This comment is superfluous; it does not tell anything non-obvious about
> the code.
>
>> +	/*
>> +	 * here zero is applied to payload field.
>> +	 * it means the maximum number of quadlets in an isochronous packet is
>> +	 * 1024 when spd is less than three, 1024 * 2 * xspd + 1 when spd is
>> +	 * equal to three. An arbitrary value can be set here but 0 is enough
>> +	 * for our purpose.
>> +	 */
>> +	opcr |= cpu_to_be32(0 << OPCR_PAYLOAD_SHIFT);
>
> In an oPCR, the payload field is changed only by the device.
>
>
> Regards,
> Clemens