On Tue, 20 Nov 2018 00:56:13 +0000
Trent Piepho tpiepho@xxxxxxxxxx wrote:
>On Tue, 2018-11-20 at 01:28 +0100, Anatolij Gustschin wrote:
>> Add USB interface driver for ARRI FPGA configuration devices based on
>> FTDI FT232H chip. Depending on USB PID the driver registers different
>> platform devices describing an FPGA configuration interface.
>
>Is ARRI different than Arria?
yes, ARRI is a company name.
>> +/* Use baudrate calculation borrowed from libftdi */
>> +static int ftdi_to_clkbits(int baudrate, unsigned int clk, int clk_div,
>
>Linux uses unsigned values for clocks. Does it make any sense to mix
>the unsigned clk with signed values? Seems like baudrate and clk_div
>should also be unsigned.
okay, will fix to unsigned.
>> + unsigned long *encoded_divisor)
>
>unsigned long is an odd choice here. Is there any to reason to use an
>unsigned long to store the result of right shifting a signed int
>(best_div)? It can't be longer than a int, but it can be negative.
okay, I'll change that to unsigned int.
>> +{
>> + static const char frac_code[8] = { 0, 3, 2, 4, 1, 5, 6, 7 };
>> + int best_baud = 0;
>> + int div, best_div;
>> +
>> + if (baudrate >= clk / clk_div) {
>> + *encoded_divisor = 0;
>> + best_baud = clk / clk_div;
>> + } else if (baudrate >= clk / (clk_div + clk_div / 2)) {
>> + *encoded_divisor = 1;
>> + best_baud = clk / (clk_div + clk_div / 2);
>> + } else if (baudrate >= clk / (2 * clk_div)) {
>> + *encoded_divisor = 2;
>> + best_baud = clk / (2 * clk_div);
>> + } else {
>> + /*
>> + * Divide by 16 to have 3 fractional bits and
>> + * one bit for rounding
>> + */
>> + div = clk * 16 / clk_div / baudrate;
>>
>> + if (div & 1) /* Decide if to round up or down */
>> + best_div = div / 2 + 1;
>> + else
>> + best_div = div / 2;
>
>In Linux we would write:
>
>best_div = DIV_ROUND_UP(div, 2);
>
>Though I think you can combine that with the above to get:
>
>best_div = DIV_ROUND_CLOSEST(clk * 8 / clk_div, baudrate);
>
>That what the above is trying to accomplish in a round about way
will rework this, too. Thanks for suggestions.
>> + if (best_div > 0x20000)
>> + best_div = 0x1ffff;
>Looks like the above was probably supposed to be >=
I'll check it.
>> + best_baud = clk * 16 / clk_div / best_div;
>> + if (best_baud & 1) /* Decide if to round up or down */
>> + best_baud = best_baud / 2 + 1;
>> + else
>> + best_baud = best_baud / 2;
>
>Again, looks like a complicated way to round to the nearest.
will change this, too.
Thanks,
Anatolij
