Re: [PATCH] [PATCH v7] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Johan Hovold <johan@kernel.org>]

On Mon, Sep 23, 2019 at 05:53:34PM +0800, Charles Yeh wrote:
> Johan Hovold <johan@kernel.org> 於 2019年9月20日 週五 下午3:56寫道：
> > Yes, that's better, but you're mixing register addresses, bit values and
> > masks above. Perhaps things would be more clear if you but a _REG suffix
> > on the register defines and order things as follows:
> >
> >         #define PL2303_HXN_<name1>_REG                  0xX1
> >         #define PL2303_HXN_<name1>_<field>_MASK         0xY1
> >         #define PL2303_HXN_<name1>_<field>_<value>      0xZ1
> >
> >         #define PL2303_HXN_<name2>_REG                  0xX2
> >         #define PL2303_HXN_<name2>_<field>_MASK         0xY2
> >         #define PL2303_HXN_<name2>_<field>_<value>      0xZ2
> >
> > The idea is simply to keep related defines together and not mix
> > register address, masks and value defines.
> >
> > Keep registers sorted by address, and bit masks and values by bit order
> > (e.g. MSB first).
> 
> Thank you for your reply
> 
> Charles Ans:
> The new define is follows
> 
> #define PL2303_READ_TYPE_HX_STATUS    0x8080
> 
> #define PL2303_HXN_FLOWCTRL_REG        0x0A
> #define PL2303_HXN_FLOWCTRL_MASK    0x1C
> #define PL2303_HXN_FLOWCTRL_NONE        0x1C
> #define PL2303_HXN_FLOWCTRL_RTS_CTS        0x18
> #define PL2303_HXN_FLOWCTRL_XON_XOFF    0x0C
> 
> #define PL2303_HXN_RESET_REG    0x07
> #define PL2303_HXN_RESET_UPSTREAM_PIPE    0x02
> #define PL2303_HXN_RESET_DOWNSTREAM_PIPE    0x01

That looks much better. But please move the reset defines above the
flow control ones to keep the registers sorted by address (0x7 < 0xa).

> > Yes, but that doesn't imply that you need to read back the old value.
> >
> > I'm assuming it would either always read back as 0, or you would read
> > back the previous value written, which means you could end up resetting
> > something you did not intend.
> >
> > Either way, you should not read back the current value when resetting
> > the data pipes.
> >
> 
> Thank you for your reply
> 
> Charles Ans:
> The new code is follows
> 
>     pl2303_vendor_write(serial,
>                 PL2303_HXN_RESET_REG,
>                 PL2303_HXN_RESET_UPSTREAM_PIPE |
> PL2303_HXN_RESET_DOWNSTREAM_PIPE);
> 
> 
> Please confirm the above new define & code..
> If there is no problem.. I will write a new Patch file.

Also looks good, thanks. Just move the reset define block as mentioned
above.

Johan