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

[Andy Shevchenko]

On Wed, Apr 7, 2021 at 6:21 AM Tran Van Pho <photranvan0712@xxxxxxxxx> wrote:
> On Wed, Apr 7, 2021 at 5:25 AM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>> On Tuesday, April 6, 2021, Pho Tran <photranvan0712@xxxxxxxxx> wrote:

...

>>> CP2108 has 16 GPIOs, So data types of several variables need to be is u16
>>> instead of u8(in struct cp210x_serial_private). This doesn't affect other
>>> CP210x devices.
>>
>> Here I am wondering if you should use 4 pins per interface. Is any pointer to data sheet?

> In datasheet, Using 4 pins per interface isn't specified. And when setting or getting Latch
> of GPIO, all of 16 gpios will be done on the same command. So there no difference between
> 4 interfaces of CP2108 in terms of GPIOs.

Okay, so we may have different configurations:
 1 interface + 1 GPIO (N or M pins)
 4 interfaces + 1 GPIO (N or M pins)

...

>>> Because CP2108 has 16 GPIO pins, the parameter passed by cp210x functions
>>> will be different from other CP210x devices. So need to check part number
>>> of the device to use correct data format  before sending commands to
>>> devices.
>>>
>>> Like CP2104, CP2108 have GPIO pins with configurable options. Therefore,
>>> should be mask all pins which are not in GPIO mode in cp2108_gpio_init()
>>> function.
>>
>> This I didn’t get. If you are talking about usage pin as GPIO, perhaps you should use valid_mask in GPIO chip structure. Otherwise you probably need to implement a proper pinmux ops for this (and register a pin controller which the code below also suggests).
>
>
> This message means 16 pins on  CP2108 can have many options like GPIO, TX toggle, RX toggle, clock output for example.
> If there are any pins set in the mode that aren't GPIO mode, we should mask these pins at gpio_altfunc in struct cp210x_serial_private.

Can those functions be changed at run time? Or is it simply one time setting?

> I am not clear  with what you said about pinmux, But I think "gpio_altfunc in struct cp210x_serial_private" has the same meaning as your idea.

Yeah, but it's custom grown (I believe due to historical reasons, i.e.
there was no pin controller framework at that time) approach.
If pin functions may be changed at run time, it's better to have them
described as pinmux.

...

>>> +       __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;
>>
>>
>> Sounds to me like pin controller functions rather than GPIO.
>
> Oh yes, This is related to the functions of pins. But we need to define it to get the state of the pins to
> check whether It's  GPIO mode or other function.

Yep, and this is pinmux functionality, no?

> It's also included in vendor specified USB packet structure.
> You can refer to https://www.silabs.com/documents/public/application-notes/an978-cp210x-usb-to-uart-api-specification.pdf
> at page34 to see struct _QUAD_PORT_CONFIG.
>
> I have  a question, Should I need to resolve the build warning on i386 reported by the kernel test robot?

If you get a warning, yes, it's better to resolve it.

>>> +       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;

-- 
With Best Regards,
Andy Shevchenko