From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752215AbcAKGGN (ORCPT ); Mon, 11 Jan 2016 01:06:13 -0500 Received: from mail-pa0-f66.google.com ([209.85.220.66]:35396 "EHLO mail-pa0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750710AbcAKGGK (ORCPT ); Mon, 11 Jan 2016 01:06:10 -0500 Subject: Re: [PATCH V7 1/1] usb:serial: Add Fintek F81532/534 driver To: Johan Hovold References: <1449040739-29352-1-git-send-email-hpeter+linux_kernel@gmail.com> <20160103185840.GG25304@localhost> Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, tom_tsai@fintek.com.tw, peter_hong@fintek.com.tw, Peter Hung From: Peter Hung Message-ID: <56934652.1030309@gmail.com> Date: Mon, 11 Jan 2016 14:06:10 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <20160103185840.GG25304@localhost> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Johan, Johan Hovold 於 2016/1/4 上午 02:58 寫道: > On Wed, Dec 02, 2015 at 03:18:59PM +0800, Peter Hung wrote: >> This driver is for Fintek F81532/F81534 USB to Serial Ports IC. > > Do you have a pointer to datasheets and/or information about these > devices? Can't seem to find anything on Fintek's homepage. F81532 https://drive.google.com/file/d/0B8vRwwYO7aMFOTRRMmhWQVNvajQ/view?usp=sharing F81534 https://drive.google.com/file/d/0B8vRwwYO7aMFV29pQWJqbVBNc00/view?usp=sharing > I noticed that you have not considered all my comments on earlier > revisions of this driver. Please make sure to address all issues raised. I'll recheck it again. > We may need to come up with a generic interface for switching tranceiver > modes as there are more drivers that could benefit from this. I think we > should prevent mode switching for a port that is open as does not seem > to make much sense. > > How about you drop mode-switching support completely for now (e.g. > gpiolib and rs485-ioctl) and focus on getting the basic functionality > right first? > You could also consider extending you user-space tool and use that to > switch modes (e.g. reprogram the device eeprom) through libudev, at > least until a generic interface is in place. Ok, we'll remove gpiolib & rs485-ioctl with next version and only remain the initialization of gpio/mode control on attach() once. Also we'll provide tools via libusb to modify the configuration data on bitbucket. > As for the basic design, you should use per-interface read and write > urbs. Submit the read urb(s) when the first port is opened, and kill > them when the last device is closed. Similarly for the write urbs, > submit one when there's at least one port with data in its fifo that it > non-busy (TX_EMPTY is set). The method you mentioned is more make sense, but it'll changes the driver tx flow and makes it more difficultly to maintain. Could I preserve current flow? The current is 1 read-urb submits on attach() & resume() and per-port write-urb submits on write() or read-urb callback TX_EMPTY. The style is the same with "mxuport.c". >> +#define F81534_CUSTOM_ADDRESS_START 0x2f00 >> +#define F81534_CUSTOM_TOTAL_SIZE 0x10 >> +#define F81534_CUSTOM_DATA_SIZE 0x10 >> +#define F81534_CUSTOM_MAX_IDX \ >> + (F81534_CUSTOM_TOTAL_SIZE/F81534_CUSTOM_DATA_SIZE) > > Do you really expect F81534_CUSTOM_MAX_IDX to be 1? Yes, we'll remove the TOTAL_SIZE and set MAX_IDX = 1 with next version. >> +#ifndef C_CMSPAR >> +#define C_CMSPAR(tty) _C_FLAG((tty), CMSPAR) >> +#endif > > Drop this. OK >> +/* >> + * The following magic numbers is F81532/534 output pin >> + * register maps >> + */ >> +static const struct io_map_value f81534_rs232_control = { >> + FINTEK_DEVICE_ID, F81534_NUM_PORT, uart_mode_rs232, >> + { >> + /* please reference f81439 io port */ > > I can't seem to find anything about a f81439 io port. Do you have a > link? > > I believe I already asked you to describe the following function in a > comment here. On next version we'll add more documents with this section and change it to "f81534_pin_control". It controls F81532/534 output pin. The pin default value can be changed with user-space tools. >> +static int f81534_read_data(struct usb_serial *usbserial, u32 address, >> + u32 size, unsigned char *buf) >> +{ >> + u32 read_size, count; >> + u32 block = 0; > > Please use size_t for the size, count and block variables (throughout). OK. >> + /* someone is changing setting, pause TX */ >> + updating_data = mutex_is_locked(&serial_priv->change_mode_mutex); >> + if (updating_data) >> + return 0; > > This cannot be done in a race-free way. You should just prevent mode > changes while the port is open. OK, we'll remove it next version. >> + bool reConfigure = false; > > Again, please remove all CamelCase. OK. >> + if (status) { >> + dev_err(&port->dev, >> + "%s: f81534_write_data failed!! status:%d\n", > > Please remove all exclamation marks (!) from all error messages. > > Please also try to spell out what went wrong instead of relying on the > function name, for example in this case: > > "failed to save configuration data: %d\n" > > Also add a space between the final colon (:) and the error code. OK. >> + memcpy(&port_priv->f81534_gpio_chip, &f81534_gpio_chip_templete, >> + sizeof(f81534_gpio_chip_templete)); >> + >> + snprintf(name, max_name - 1, "%s-%d", IC_NAME, idx); > > The size includes the trailing NUL so use max_name only here. OK, but this section will be removed next version. >> +static int f81534_find_config_idx(struct usb_serial *serial, uintptr_t *index) >> +{ >> + int idx, status; >> + u8 custom_data; >> + int offset; >> + >> + for (idx = F81534_CUSTOM_MAX_IDX - 1; idx >= 0; --idx) { > > Why go through all this trouble when F81534_CUSTOM_MAX_IDX is a constant > defined as 1? The define is for the historical, we original designed for a section to store the data (about 0x100 / 0x10 = 0x10 set of data), but It makes more difficulty for H/W design. So we reduce from 0x100 to 0x10, only 1 set of data remained with MP IC. I'll reduce the code for next version. >> + /* Save the configuration area idx as private data for attach() */ >> + usb_set_serial_data(serial, (void *) setting_idx); > > No space after casts. OK. >> + dev_info(&serial->dev->dev, >> + "%s: read configure from block:%d\n", __func__, >> + (int) setting_idx); >> + } else { >> + dev_info(&serial->dev->dev, "%s: read configure default\n", >> + __func__); >> + } > > Demote these info messages to debug level. OK. >> + >> + for (i = 0; i < F81534_NUM_PORT; ++i) { >> + /* >> + * For older configuration use. We'll transform it to newer >> + * setting after load it with final port probed. >> + */ >> + switch (setting[i]) { >> + case F81534_OLD_CONFIG_37: >> + case F81534_OLD_CONFIG_38: >> + case F81534_OLD_CONFIG_39: >> + ++num_port; >> + break; >> + } >> + } > > Please explain what this old and new configuration style is and why you > need it. > > Please also document the format of your configuration data. We had 3 generation of F81532/534. All gen has an internal storage. 1st is pure USB-to-TTL RS232 IC and designed for 4 ports only, no any internal data will used. All mode and gpio control should manually set by AP or Driver and all storage space value are 0xff. The f81534_calc_num_ports() will run to final we marked as "oldest version" for this IC. 2nd is designed to match our transceivers(F81437/438/439). We'll save data in F81534_DEF_CONF_ADDRESS_START(0x3000) with 8bytes. The first 4bytes is transceiver type, value is only 0x37/0x38/0x39 to represent F81437/438/439, and the following 4bytes are save mode & gpio state, the last 4bytes will be limited by the first 4bytes transceiver type. The f81534_calc_num_ports() will run to "older configuration" with checking F81534_OLD_CONFIG_37/F81534_OLD_CONFIG_38 /F81534_OLD_CONFIG_39 section. 3rd is designed to more generic to use any transceiver and this is our mass production type. We'll save data in F81534_CUSTOM_ADDRESS_START (0x2f00) with 9bytes. The 1st byte is a indicater. If the token is not F81534_CUSTOM_VALID_TOKEN(0xf0), the IC is 2nd gen type, the following 4bytes save port mode (0:RS232/1:RS485 Invert/2:RS485), and the last 4bytes save GPIO state(value from 0~7 to represent 3 GPIO output pin). The f81534_calc_num_ports() will run to "new style" with checking F81534_PORT_UNAVAILABLE section. I'll add this document in code with next version. >> + if (num_port) >> + return num_port; >> + >> + dev_err(&serial->dev->dev, "Read Failed!!, default 4 ports\n"); > > If this is really an error you should bail out and return 0, otherwise > use dev_dbg or possible _warn here. OK, we'll change it as dev_warn(). >> +static void f81534_release(struct usb_serial *serial) >> +{ >> + struct f81534_serial_private *serial_priv = >> + usb_get_serial_data(serial); >> + >> + kfree(serial_priv); >> +} > > Please try to keep related function together. Place the release callback > after f81534_attach(). OK. >> +static int f81534_set_port_mode(struct usb_serial_port *port, >> + enum uart_mode eMode) > > CamelCase OK. >> +static int f81534_setup_urbs(struct usb_serial *serial) > > Rename f81534_setup_ports. OK. >> +static int f81534_write(struct tty_struct *tty, >> + struct usb_serial_port *port, >> + const unsigned char *buf, int count) >> +{ >> + int bytes_out, status; >> + >> + if (!count) >> + return 0; >> + >> + bytes_out = kfifo_in_locked(&port->write_fifo, buf, count, >> + &port->lock); >> + >> + status = f81534_submit_writer(port, GFP_KERNEL); > > You must use GFP_ATOMIC here. OK. > Please use a concise description here (e.g. "Fintek F81532/F81534") > here. > OK. Thanks for your advices. -- With Best Regards, Peter Hung