Message-ID: <CO1PR11MB4882A90461E033F8EA301C07815F9@CO1PR11MB4882.namprd11.prod.outlook.com>
Date:   Thu, 29 Apr 2021 02:54:09 +0000
From:   Tung Pham <Tung.Pham@...abs.com>
To:     Johan Hovold <johan@...nel.org>
CC:     Pho Tran <photranvan0712@...il.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Hung Nguyen <Hung.Nguyen@...abs.com>,
        Pho Tran <Pho.Tran@...abs.com>
Subject: RE: [PATCH v9] USB: serial: cp210x: Add support for GPIOs on CP2108

Dear Johan.
Thanks for your review of my code.
You can find my answer below in Tung Pham mark:

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@...abs.com>
> >
> > 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-cp2
> > +10 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)?


Tung Pham: the device return some unused bytes, and manufacturing library already discard these byte to assign value to PORT_CONFIG, so you don't see padding byte on PORT_CONFIG structure.   you can find the structure  of port setting in this code: 

https://www.silabs.com/documents/public/software/USBXpressHostSDK-Linux.tar


\USBXpressHostSDK-Linux\USBXpressHostSDK\CP210x\srcpkg\cp210xmanufacturing_1.0.tar\cp210xmanufacturing_1.0\cp210xmanufacturing\cp210xmanufacturing\src\CP2104Device.cpp

CP210x_STATUS CCP2104Device::GetPortConfig(PORT_CONFIG* PortConfig)


> 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.p
> df 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

Tung Pham

Powered by blists - more mailing lists