Date: Tue, 9 Jan 2018 12:19:14 +0100
From: Johan Hovold <johan@kernel.org>
To: "Ji-Ze Hong (Peter Hong)" <hpeter@gmail.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        peter_hong@fintek.com.tw,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH V2 3/5] usb: serial: f81534: add output pin control
Message-ID: <20180109111914.GP11344@localhost>
References: <1515032961-29131-1-git-send-email-hpeter+linux_kernel@gmail.com>
 <1515032961-29131-3-git-send-email-hpeter+linux_kernel@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1515032961-29131-3-git-send-email-hpeter+linux_kernel@gmail.com>
User-Agent: Mutt/1.7.2 (2016-11-26)
Sender: linux-kernel-owner@vger.kernel.org

On Thu, Jan 04, 2018 at 10:29:19AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had 3 output pin (M0/SD, M1, M2) with open-drain mode to
> control transceiver. We'll read it from internal Flash with address
> 0x2f05~0x2f08 for 4 ports. The value is range from 0 to 7. The M0/SD is
> MSB of this value. For a examples, If read value is 6, we'll write M0/SD,
> M1, M2 as 1, 1, 0.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> V2:
> 	1: Fix for space between brace.
> 	2: Remain the old pin control method.
> 
>  drivers/usb/serial/f81534.c | 67 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 8a778bc1d492..7f175f39a171 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -52,6 +52,7 @@
>  #define F81534_CUSTOM_NO_CUSTOM_DATA	0xff
>  #define F81534_CUSTOM_VALID_TOKEN	0xf0
>  #define F81534_CONF_OFFSET		1
> +#define F81534_CONF_GPIO_OFFSET		4
>  
>  #define F81534_MAX_DATA_BLOCK		64
>  #define F81534_MAX_BUS_RETRY		20
> @@ -164,6 +165,23 @@ struct f81534_port_private {
>  	u8 phy_num;
>  };
>  
> +struct f81534_pin_data {
> +	const u16 reg_addr;
> +	const u16 reg_mask;

I asked in my previous review comments whether this mask should really
be u8?

> +};
> +
> +struct f81534_port_out_pin {
> +	struct f81534_pin_data pin[3];
> +};
> +
> +/* Pin output value for M2/M1/M0(SD) */
> +static const struct f81534_port_out_pin f81534_port_out_pins[] = {
> +	 { { {0x2ae8, BIT(7)}, {0x2a90, BIT(5)}, {0x2a90, BIT(4) } } },
> +	 { { {0x2ae8, BIT(6)}, {0x2ae8, BIT(0)}, {0x2ae8, BIT(3) } } },
> +	 { { {0x2a90, BIT(0)}, {0x2ae8, BIT(2)}, {0x2a80, BIT(6) } } },
> +	 { { {0x2a90, BIT(3)}, {0x2a90, BIT(2)}, {0x2a90, BIT(1) } } },

Nit picking, but you still don't use space consistently around { and }
above.

Johan