Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751933AbdL0KbF (ORCPT ); Wed, 27 Dec 2017 05:31:05 -0500 Received: from mail-lf0-f54.google.com ([209.85.215.54]:36680 "EHLO mail-lf0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751002AbdL0KbD (ORCPT ); Wed, 27 Dec 2017 05:31:03 -0500 X-Google-Smtp-Source: ACJfBovPx3ZOTffAetKAZ8MhXUSM7ohqzPsAgo/pVUh6C7T5xLwqyKObRBFB2gVPnpHKNYVbdvqhAQ== Date: Wed, 27 Dec 2017 11:30:55 +0100 From: Johan Hovold To: "Ji-Ze Hong (Peter Hong)" Cc: Johan Hovold , gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, "Ji-Ze Hong (Peter Hong)" Subject: Re: [PATCH V1 3/4] usb: serial: f81534: add output pin control Message-ID: <20171227103055.GN3374@localhost> References: <1510818369-10323-1-git-send-email-hpeter+linux_kernel@gmail.com> <1510818369-10323-3-git-send-email-hpeter+linux_kernel@gmail.com> <20171218160616.GC3374@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2093 Lines: 57 On Thu, Dec 21, 2017 at 05:49:45PM +0800, Ji-Ze Hong (Peter Hong) wrote: > Hi Johan, > > Johan Hovold 於 2017/12/19 上午 12:06 寫道: > > On Thu, Nov 16, 2017 at 03:46:08PM +0800, Ji-Ze Hong (Peter Hong) wrote: > >> +static int f81534_set_port_output_pin(struct usb_serial_port *port) > >> +{ > >> + struct f81534_serial_private *serial_priv; > >> + struct f81534_port_private *port_priv; > >> + struct usb_serial *serial; > >> + const struct f81534_port_out_pin *pins; > >> + int status; > >> + int i; > >> + u8 value; > >> + u8 idx; > >> + > >> + serial = port->serial; > >> + serial_priv = usb_get_serial_data(serial); > >> + port_priv = usb_get_serial_port_data(port); > >> + > >> + idx = F81534_CONF_GPIO_OFFSET + port_priv->phy_num; > >> + value = serial_priv->conf_data[idx]; > >> + pins = &f81534_port_out_pins[port_priv->phy_num]; > >> + > >> + for (i = 0; i < ARRAY_SIZE(pins->pin); ++i) { > >> + status = f81534_set_mask_register(serial, > >> + pins->pin[i].reg_addr, pins->pin[i].reg_mask, > >> + value & BIT(i) ? pins->pin[i].reg_mask : 0); > >> + if (status) > >> + return status; > >> + } > > > > You're using 24 (get or set) accesses to update these three registers > > here. Why not read them out (if necessary), determine their new values > > and then write them back when done instead? > > > > In this code, I'm only read/write 3 registers of 0x2ae8, 0x2a90, 0x2a80, > but some register will read/write more than once. Should I change the > code from port_probe() to attach() and re-write it as: > 1: read the 3 register > 2: change them will 12 pin desire value > 3: write it back > Is it ok? Do you expect these pins to ever be changed after probe? If not, then perhaps it can be moved to attach(), but otherwise I guess they should be set at port_probe(). By using shadow registers, you should be able to reduce the number of device accesses, but perhaps it's not worth the complexity. Do you have a rough idea about how long these register updates take? I was just worried that these changes will add up to really long probe times. Thanks, Johan