Re: [PATCH V12 1/1] usb:serial: Add Fintek F81532/534 driver

[Johan Hovold <johan@kernel.org>]

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