Date: Thu, 24 Nov 2016 16:04:25 +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,
        tom_tsai@fintek.com.tw, peter_hong@fintek.com.tw,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH V12 1/1] usb:serial: Add Fintek F81532/534 driver
Message-ID: <20161124150425.GA30963@localhost>
References: <1479101879-26128-1-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: <1479101879-26128-1-git-send-email-hpeter+linux_kernel@gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2542
Lines: 91

On Mon, Nov 14, 2016 at 01:37:59PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> This driver is for Fintek F81532/F81534 USB to Serial Ports IC.
> 
> F81532 spec:
> https://drive.google.com/file/d/0B8vRwwYO7aMFOTRRMmhWQVNvajQ/view?usp=
> sharing
> 
> F81534 spec:
> https://drive.google.com/file/d/0B8vRwwYO7aMFV29pQWJqbVBNc00/view?usp=
> sharing
> 
> Features:
> 1. F81532 is 1-to-2 & F81534 is 1-to-4 serial ports IC
> 2. Support Baudrate from B50 to B115200.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> Changelog:
> V12
>     1. Max TX change from 100 to 124 bytes.
>     2. Add probe() to verify endpoints & packet size.
>     3. Rename function names.
>          set/get_normal_register() -> set/get_register()
>          set/get_register() -> set/get_port_register()
>          set/get_register_delay() -> set/get_spi_register()
>          read_data() -> read_flash()
>          command_delay() -> wait_for_spi()

> +static int f81534_probe(struct usb_serial *serial,
> +					const struct usb_device_id *id)
> +{
> +	struct usb_endpoint_descriptor *endpoint;
> +	struct usb_host_interface *iface_desc;
> +	struct device *dev;
> +	int num_bulk_in = 0;
> +	int num_bulk_out = 0;
> +	int size_bulk_in = 0;
> +	int size_bulk_out = 0;
> +	int i;
> +
> +	dev = &serial->interface->dev;
> +	iface_desc = serial->interface->cur_altsetting;
> +
> +	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> +		endpoint = &iface_desc->endpoint[i].desc;
> +
> +		if (usb_endpoint_is_bulk_in(endpoint)) {
> +			++num_bulk_in;
> +			size_bulk_in = usb_endpoint_maxp(endpoint);
> +		}
> +
> +		if (usb_endpoint_is_bulk_out(endpoint)) {
> +			++num_bulk_out;
> +			size_bulk_out = usb_endpoint_maxp(endpoint);
> +		}
> +	}
> +
> +	if (num_bulk_in != 1 || num_bulk_out != 1) {
> +		dev_err(dev, "%s: endpoints not matched\n", __func__);

Better to spell out: "expected endpoints not found\n".

> +		return -EINVAL;

And this should be -ENODEV;

> +	}
> +
> +	if (size_bulk_out != F81534_WRITE_BUFFER_SIZE ||
> +			size_bulk_in != F81534_MAX_RECEIVE_BLOCK_SIZE) {
> +		dev_err(dev, "%s: endpoints packet size not matched\n",
> +				__func__);

Similarly: "unsupported endpoint max packet size\n".

But just to be clear: You do want to bail out if connected at full
speed? You could also ask usb-serial core to allocate large enough
buffers (e.g. by setting the bulk_out_size driver field) and the host
controller will handle partitioning.

> +		return -EINVAL;

And -ENODEV.

> +	}
> +
> +	return 0;
> +}

Thanks,
Johan