Subject: Re: [PATCH V1 3/4] usb: serial: f81534: add output pin control
To: Johan Hovold <johan@kernel.org>
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@gmail.com>
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>
From: "Ji-Ze Hong (Peter Hong)" <hpeter@gmail.com>
Message-ID: <c7e9fd86-cd52-b72a-3b47-3d74323bd8a6@gmail.com>
Date: Thu, 21 Dec 2017 17:49:45 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <20171218160616.GC3374@localhost>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1517
Lines: 48

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?

Thanks
-- 
With Best Regards,
Peter Hong