Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932334AbbKDIl7 (ORCPT ); Wed, 4 Nov 2015 03:41:59 -0500 Received: from mail-pa0-f42.google.com ([209.85.220.42]:33182 "EHLO mail-pa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932223AbbKDIl6 (ORCPT ); Wed, 4 Nov 2015 03:41:58 -0500 Subject: Re: [PATCH V6 1/1] usb:serial: Add Fintek F81532/534 driver To: Andy Shevchenko References: <1446522669-18987-1-git-send-email-hpeter+linux_kernel@gmail.com> Cc: johan@kernel.org, Greg Kroah-Hartman , USB , "linux-kernel@vger.kernel.org" , tom_tsai@fintek.com.tw, peter_hong@fintek.com.tw, Peter Hung From: Peter Hung Message-ID: <5639C4D2.3060505@gmail.com> Date: Wed, 4 Nov 2015 16:41:54 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: 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 Content-Length: 3692 Lines: 98 Hi Andy Shevchenko 於 2015/11/3 下午 05:45 寫道: > On Tue, Nov 3, 2015 at 5:51 AM, Peter Hung wrote: >> + * Please reference https://bitbucket.org/hpeter/fintek-general/src/ >> + * with f81534/tools to get set_gpio.c & set_mode.c. Please use it >> + * carefully. > > Would it be good to have this under Documentation ? I had some documents in source code. The link is the demo program to implement switch UART/PIN funcion. I'll try to add for document with the repository. >> +static void f81534_dtr_rts(struct usb_serial_port *port, int on); >> + >> +static int f81534_set_port_mode(struct usb_serial_port *port, >> + enum uart_mode eMode); >> + >> +static int f81534_save_configure_data(struct usb_serial_port *port); >> + >> +static int f81534_switch_gpio_mode(struct usb_serial_port *port, u8 mode); >> + >> +static void f81534_wakeup_all_port(struct usb_serial *serial); >> + >> +static void f81534_compare_msr(struct usb_serial_port *port, u8 *msr, >> + bool is_port_open); >> + >> +static int f81534_prepare_write_buffer(struct usb_serial_port *port, >> + void *dest, size_t size); >> + >> +static int f81534_submit_writer(struct usb_serial_port *port, gfp_t mem_flags); > > Would it be possible to reshuffle code to reduce amount of forward declarations? > I'll try to reduce it. > >> + if ((size <= F81534_MAX_DATA_BLOCK) && >> + (read_size == (count + 1))) { > > No need to have internal parens. > In my opinion, It makes more readable. Should I remove it? >> + if (baudrate <= 115200) { >> + value = 0x01; /* 1.846m fixed */ >> + divisor = f81534_calc_baud_divisor(baudrate, 115200, NULL); >> + port_priv->current_baud_base = 115200; >> + } else { >> + for (count = 0; count < ARRAY_SIZE(baudrate_table) ; ++count) { >> + baud_base = baudrate_table[count]; >> + divisor = f81534_calc_baud_divisor(baudrate, baud_base, >> + &rem); >> + if (!rem) { >> + dev_dbg(&port->dev, "%s: found clockbase %d\n", >> + __func__, >> + baudrate_table[count]); >> + value = clock_table[count]; >> + port_priv->current_baud_base = baud_base; >> + break; >> + } >> + } > > Can you calculate baud rates more precisely? Any link to datasheet > where it's described? > I'll try to comment it within source code. >> +static int f81534_ioctl_get_rs485(struct usb_serial_port *port, >> + struct serial_rs485 __user *arg) >> +{ >> + int status; >> + struct serial_rs485 data; >> + struct f81534_port_private *port_priv = usb_get_serial_port_data(port); >> + struct f81534_serial_private *serial_priv = >> + usb_get_serial_data(port->serial); > > One line? > It's over 80 character with a tab, It cant be one line. I'll fix other issue that you mention. Thanks for your advices. -- With Best Regards, Peter Hung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/