Date: Mon, 18 Dec 2017 17:06:16 +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,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH V1 3/4] usb: serial: f81534: add output pin control
Message-ID: <20171218160616.GC3374@localhost>
References: <1510818369-10323-1-git-send-email-hpeter+linux_kernel@gmail.com>
 <1510818369-10323-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: <1510818369-10323-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
Content-Length: 4004
Lines: 128

On Thu, Nov 16, 2017 at 03:46:08PM +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.
> 
> Register mapping for output value:
> 	Port 0:
> 		M2: 0x2ae8 bit7, M1: 0x2a90 bit5, M0/SD: 0x2a90 bit4
> 	Port 1:
> 		M2: 0x2ae8 bit6, M1: 0x2ae8 bit0, M0/SD: 0x2ae8 bit3
> 	Port 2:
> 		M2: 0x2a90 bit0, M1: 0x2ae8 bit2, M0/SD: 0x2a80 bit6
> 	Port 3:
> 		M2: 0x2a90 bit3, M1: 0x2a90 bit2, M0/SD: 0x2a90 bit1
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
>  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 b2d10309c335..30b966d71ae8 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -56,6 +56,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
> @@ -166,6 +167,23 @@ struct f81534_port_private {
>  	u8 phy_num;
>  };
>  
> +struct f81534_pin_data {
> +	const u16 reg_addr;
> +	const u16 reg_mask;

Should the mask not 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) } } },

Please use a space after { and before } consistently.

> +};
> +
>  static int f81534_logic_to_phy_port(struct usb_serial *serial,
>  					struct usb_serial_port *port)
>  {
> @@ -271,6 +289,22 @@ static int f81534_get_register(struct usb_serial *serial, u16 reg, u8 *data)
>  	return status;
>  }
>  
> +static int f81534_set_mask_register(struct usb_serial *serial, u16 reg,
> +					u8 mask, u8 data)
> +{
> +	int status;
> +	u8 tmp;
> +
> +	status = f81534_get_register(serial, reg, &tmp);
> +	if (status)
> +		return status;
> +
> +	tmp &= ~mask;
> +	tmp |= (mask & data);
> +
> +	return f81534_set_register(serial, reg, tmp);
> +}
> +
>  static int f81534_set_port_register(struct usb_serial_port *port, u16 reg,
>  					u8 data)
>  {
> @@ -1299,6 +1333,37 @@ static void f81534_lsr_worker(struct work_struct *work)
>  		dev_warn(&port->dev, "read LSR failed: %d\n", status);
>  }
>  
> +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?

> +
> +	dev_info(&port->dev, "Output pin (M0/M1/M2): %d\n", value);

Use dev_dbg here.

Johan