Re: [PATCH v9] USB: serial: cp210x: Add support for GPIOs on CP2108

[Johan Hovold]

On Mon, Apr 26, 2021 at 09:49:37AM +0000, Tung Pham wrote:
> Dear Johan Hovold.
> Thanks for your review.
> I read you comment and answer you as following:
> 
> On Thu, Apr 08, 2021 at 05:36:07PM +0700, Pho Tran wrote:
> > From: Pho Tran <pho.tran@xxxxxxxxxx>
> >
> > Similar to other CP210x devices, GPIO interfaces (gpiochip) should be 
> > supported for CP2108.
> 
> > +/*
> > + * Quad Port Config definitions
> > + * Refer to 
> > +https://www.silabs.com/documents/public/application-notes/an978-cp210
> > +x-usb-to-uart-api-specification.pdf
> > + * for more information.
> > + * CP210X_VENDOR_SPECIFIC, CP210X_GET_PORTCONFIG call reads these 
> > +0x49 bytes
> > + * on a CP2108 chip.
> > + * CP2108 Quad Port State structure(used in Quad Port Config 
> > +structure)  */ struct cp210x_quad_port_state {
> > +     __le16 gpio_mode_PB0;
> > +     __le16 gpio_mode_PB1;
> > +     __le16 gpio_mode_PB2;
> > +     __le16 gpio_mode_PB3;
> > +     __le16 gpio_mode_PB4;
> > +
> > +
> > +     __le16 gpio_lowpower_PB0;
> > +     __le16 gpio_lowpower_PB1;
> > +     __le16 gpio_lowpower_PB2;
> > +     __le16 gpio_lowpower_PB3;
> > +     __le16 gpio_lowpower_PB4;
> > +
> > +     __le16 gpio_latch_PB0;
> > +     __le16 gpio_latch_PB1;
> > +     __le16 gpio_latch_PB2;
> > +     __le16 gpio_latch_PB3;
> > +     __le16 gpio_latch_PB4;
> > +};
> > +
> > +// Cp2108 Quad Port Config structure
> > +struct cp210x_quad_port_config {
> > +     struct cp210x_quad_port_state reset_state;
> > +     struct cp210x_quad_port_state suspend_state;
> > +     u8 ipdelay_IFC[4];
> > +     u8 enhancedfxn_IFC[4];
> > +     u8 enhancedfxn_device;
> > +     u8 extclkfreq[4];
> > +} __packed;
> 
> One more thing; I noticed that the layout of the other port-config
> structures do not match the ones used by your library API, which is
> what the above pdf documents (e.g. they have additional padding).
> 
> Tung Pham: the layout is correct, the document add padding bit to
> align data to 8 or 16 bit, we already use      __le16, so the data is
> aligned to 16 bit.

Not sure I understand what you're saying here.

My point was simply that the layout of the other structures as expected
by the device doesn't match the layout described in you library API
documentation.

It doesn't appear to have anything to do with member alignment, it
just looks like random unused bits in the structures. Take the
single-port config, for example:

	struct cp210x_single_port_config {
		__le16	gpio_mode;
		u8	__pad0[2];
		__le16	reset_state;
		u8	__pad1[4];
		__le16	suspend_state;
		u8	device_cfg;
	} __packed;

You library API has this as:

	typedef struct _PORT_CONFIG
	{
		uint16_t Mode;
		uint16_t Reset_Latch;
		uint16_t Suspend_Latch;
		unsigned char EnhancedFxn;
	} PORT_CONFIG, *PPORT_CONFIG;

which simply doesn't match up (and there's no implicit padding between
members, only after EnhancedFxn).

So my questions again are:

 1) Have you verified that the struct cp210x_quad_port_config above
    actually matches what the device uses?

 2) Do you have any documentation of the structures as expected by the
    device firmware (not your library)?

> Did you verify that the above layout is actually correct? And did you
> try changing the pin functions in EEPROM and make sure that your code
> handles it as expected?
> 
> Tung Pham: we have tested to toggle GPIO pin in normal case, we will
> test the case that the gpio have alternative function in the future.

Great, this could be one way of verifying the above. Please do that and
let me know the result.

That part of the code has already been found broken twice during review
so you really should have tested this before submitting.

> Is there any corresponding document for the actual device protocol?
> Tung Pham:
> You can refer to 
> https://www.silabs.com/documents/public/data-sheets/cp2108-datasheet.pdf
> for understanding the functionality of cp2108.
> And 
> https://www.silabs.com/documents/public/application-notes/AN721.pdf
> for use simplicity software to configure the GPIO pin alternative function of cp2108.

Ok, but those do not document the layout of these structures either
(e.g. cp210x_single_port_config with the __pad0 and __pad1 members).

Johan