From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Date: Tue, 20 Nov 2018 15:49:47 +0100 From: Anatolij Gustschin Subject: Re: [PATCH v2 1/3] usb: misc: add driver for FT232H based FPGA configuration devices Message-ID: <20181120154947.4dbc90d6@crub> In-Reply-To: <1542675372.30311.573.camel@impinj.com> References: <20181120002821.12794-1-agust@denx.de> <20181120002821.12794-2-agust@denx.de> <1542675372.30311.573.camel@impinj.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit To: Trent Piepho Cc: "linux-spi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-fpga@vger.kernel.org" , "linux-usb@vger.kernel.org" , "gregkh@linuxfoundation.org" , "broonie@kernel.org" , "atull@kernel.org" , "mdf@kernel.org" List-ID: On Tue, 20 Nov 2018 00:56:13 +0000 Trent Piepho tpiepho@impinj.com 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v2,1/3] usb: misc: add driver for FT232H based FPGA configuration devices From: Anatolij Gustschin Message-Id: <20181120154947.4dbc90d6@crub> Date: Tue, 20 Nov 2018 15:49:47 +0100 To: Trent Piepho Cc: "linux-spi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-fpga@vger.kernel.org" , "linux-usb@vger.kernel.org" , "gregkh@linuxfoundation.org" , "broonie@kernel.org" , "atull@kernel.org" , "mdf@kernel.org" List-ID: T24gVHVlLCAyMCBOb3YgMjAxOCAwMDo1NjoxMyArMDAwMApUcmVudCBQaWVwaG8gdHBpZXBob0Bp bXBpbmouY29tIHdyb3RlOgoKPk9uIFR1ZSwgMjAxOC0xMS0yMCBhdCAwMToyOCArMDEwMCwgQW5h dG9saWogR3VzdHNjaGluIHdyb3RlOgo+PiBBZGQgVVNCIGludGVyZmFjZSBkcml2ZXIgZm9yIEFS UkkgRlBHQSBjb25maWd1cmF0aW9uIGRldmljZXMgYmFzZWQgb24KPj4gRlRESSBGVDIzMkggY2hp cC4gRGVwZW5kaW5nIG9uIFVTQiBQSUQgdGhlIGRyaXZlciByZWdpc3RlcnMgZGlmZmVyZW50Cj4+ IHBsYXRmb3JtIGRldmljZXMgZGVzY3JpYmluZyBhbiBGUEdBIGNvbmZpZ3VyYXRpb24gaW50ZXJm YWNlLiAgCj4KPklzIEFSUkkgZGlmZmVyZW50IHRoYW4gQXJyaWE/Cgp5ZXMsIEFSUkkgaXMgYSBj b21wYW55IG5hbWUuCgo+PiArLyogVXNlIGJhdWRyYXRlIGNhbGN1bGF0aW9uIGJvcnJvd2VkIGZy b20gbGliZnRkaSAqLwo+PiArc3RhdGljIGludCBmdGRpX3RvX2Nsa2JpdHMoaW50IGJhdWRyYXRl LCB1bnNpZ25lZCBpbnQgY2xrLCBpbnQgY2xrX2RpdiwgIAo+Cj5MaW51eCB1c2VzIHVuc2lnbmVk IHZhbHVlcyBmb3IgY2xvY2tzLiAgRG9lcyBpdCBtYWtlIGFueSBzZW5zZSB0byBtaXgKPnRoZSB1 bnNpZ25lZCBjbGsgd2l0aCBzaWduZWQgdmFsdWVzPyAgU2VlbXMgbGlrZSBiYXVkcmF0ZSBhbmQg Y2xrX2Rpdgo+c2hvdWxkIGFsc28gYmUgdW5zaWduZWQuCgpva2F5LCB3aWxsIGZpeCB0byB1bnNp Z25lZC4KCj4+ICsJCQkgICB1bnNpZ25lZCBsb25nICplbmNvZGVkX2Rpdmlzb3IpICAKPgo+dW5z aWduZWQgbG9uZyBpcyBhbiBvZGQgY2hvaWNlIGhlcmUuICBJcyB0aGVyZSBhbnkgdG8gcmVhc29u IHRvIHVzZSBhbgo+dW5zaWduZWQgbG9uZyB0byBzdG9yZSB0aGUgcmVzdWx0IG9mIHJpZ2h0IHNo aWZ0aW5nIGEgc2lnbmVkIGludAo+KGJlc3RfZGl2KT8gIEl0IGNhbid0IGJlIGxvbmdlciB0aGFu IGEgaW50LCBidXQgaXQgY2FuIGJlIG5lZ2F0aXZlLgoKb2theSwgSSdsbCBjaGFuZ2UgdGhhdCB0 byB1bnNpZ25lZCBpbnQuCgo+PiArewo+PiArCXN0YXRpYyBjb25zdCBjaGFyIGZyYWNfY29kZVs4 XSA9IHsgMCwgMywgMiwgNCwgMSwgNSwgNiwgNyB9Owo+PiArCWludCBiZXN0X2JhdWQgPSAwOwo+ PiArCWludCBkaXYsIGJlc3RfZGl2Owo+PiArCj4+ICsJaWYgKGJhdWRyYXRlID49IGNsayAvIGNs a19kaXYpIHsKPj4gKwkJKmVuY29kZWRfZGl2aXNvciA9IDA7Cj4+ICsJCWJlc3RfYmF1ZCA9IGNs ayAvIGNsa19kaXY7Cj4+ICsJfSBlbHNlIGlmIChiYXVkcmF0ZSA+PSBjbGsgLyAoY2xrX2RpdiAr IGNsa19kaXYgLyAyKSkgewo+PiArCQkqZW5jb2RlZF9kaXZpc29yID0gMTsKPj4gKwkJYmVzdF9i YXVkID0gY2xrIC8gKGNsa19kaXYgKyBjbGtfZGl2IC8gMik7Cj4+ICsJfSBlbHNlIGlmIChiYXVk cmF0ZSA+PSBjbGsgLyAoMiAqIGNsa19kaXYpKSB7Cj4+ICsJCSplbmNvZGVkX2Rpdmlzb3IgPSAy Owo+PiArCQliZXN0X2JhdWQgPSBjbGsgLyAoMiAqIGNsa19kaXYpOwo+PiArCX0gZWxzZSB7Cj4+ ICsJCS8qCj4+ICsJCSAqIERpdmlkZSBieSAxNiB0byBoYXZlIDMgZnJhY3Rpb25hbCBiaXRzIGFu ZAo+PiArCQkgKiBvbmUgYml0IGZvciByb3VuZGluZwo+PiArCQkgKi8KPj4gKwkJZGl2ID0gY2xr ICogMTYgLyBjbGtfZGl2IC8gYmF1ZHJhdGU7Cj4+IAo+PiArCQlpZiAoZGl2ICYgMSkJLyogRGVj aWRlIGlmIHRvIHJvdW5kIHVwIG9yIGRvd24gKi8KPj4gKwkJCWJlc3RfZGl2ID0gZGl2IC8gMiAr IDE7Cj4+ICsJCWVsc2UKPj4gKwkJCWJlc3RfZGl2ID0gZGl2IC8gMjsgIAo+Cj5JbiBMaW51eCB3 ZSB3b3VsZCB3cml0ZToKPgo+YmVzdF9kaXYgPSBESVZfUk9VTkRfVVAoZGl2LCAyKTsKPgo+VGhv dWdoIEkgdGhpbmsgeW91IGNhbiBjb21iaW5lIHRoYXQgd2l0aCB0aGUgYWJvdmUgdG8gZ2V0Ogo+ Cj5iZXN0X2RpdiA9IERJVl9ST1VORF9DTE9TRVNUKGNsayAqIDggLyBjbGtfZGl2LCBiYXVkcmF0 ZSk7Cj4KPlRoYXQgd2hhdCB0aGUgYWJvdmUgaXMgdHJ5aW5nIHRvIGFjY29tcGxpc2ggaW4gYSBy b3VuZCBhYm91dCB3YXkKCndpbGwgcmV3b3JrIHRoaXMsIHRvby4gVGhhbmtzIGZvciBzdWdnZXN0 aW9ucy4KCj4+ICsJCWlmIChiZXN0X2RpdiA+IDB4MjAwMDApCj4+ICsJCQliZXN0X2RpdiA9IDB4 MWZmZmY7Cgo+TG9va3MgbGlrZSB0aGUgYWJvdmUgd2FzIHByb2JhYmx5IHN1cHBvc2VkIHRvIGJl ID49CgpJJ2xsIGNoZWNrIGl0LgoKPj4gKwkJYmVzdF9iYXVkID0gY2xrICogMTYgLyBjbGtfZGl2 IC8gYmVzdF9kaXY7Cj4+ICsJCWlmIChiZXN0X2JhdWQgJiAxKQkvKiBEZWNpZGUgaWYgdG8gcm91 bmQgdXAgb3IgZG93biAqLwo+PiArCQkJYmVzdF9iYXVkID0gYmVzdF9iYXVkIC8gMiArIDE7Cj4+ ICsJCWVsc2UKPj4gKwkJCWJlc3RfYmF1ZCA9IGJlc3RfYmF1ZCAvIDI7ICAKPgo+QWdhaW4sIGxv b2tzIGxpa2UgYSBjb21wbGljYXRlZCB3YXkgdG8gcm91bmQgdG8gdGhlIG5lYXJlc3QuCgp3aWxs IGNoYW5nZSB0aGlzLCB0b28uCgpUaGFua3MsCkFuYXRvbGlqCg==