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

[Johan Hovold <johan@kernel.org>]

On Tue, Aug 27, 2019 at 04:40:39PM +0800, Charles Yeh wrote:
> Johan Hovold <johan@kernel.org> 於 2019年7月16日 週二 下午4:49寫道：
> > >  #define PL2303_FLOWCTRL_MASK         0xf0
> > > +#define PL2303_HXN_FLOWCTRL_MASK     0x1C
> > > +#define PL2303_HXN_FLOWCTRL          0x0A
> >
> > I asked you to keep related defines together (and to move the mask where
> > the register define was, not the other way round). Please move these to
> > the other HXN defines below, and keep the register address defines
> > before the corresponding bit defines.
> 
> Charles Ans:

[ You don't need to prefix your replies like this, I can tell from the
  number of > signs. ]

> I am not 100% sure what you mean, please see if it is defined below
> 
> #define PL2303_FLOWCTRL_MASK        0xf0
> 
> #define PL2303_READ_TYPE_HX_STATUS    0x8080
> 
> #define PL2303_HXN_CTRL_XON_XOFF    0x0C
> #define PL2303_HXN_CTRL_RTS_CTS        0x18
> #define PL2303_HXN_CTRL_NONE        0x1C
> #define PL2303_HXN_FLOWCTRL_MASK    0x1C
> #define PL2303_HXN_FLOWCTRL        0x0A

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).

> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE_MASK    0x03
> #define PL2303_HXN_RESET_CONTROL    0x07
> 
> > > +
> > > +#define PL2303_HXN_RESET_CONTROL_MASK        0x03
> > This makes no sense. The whole register is used for reset. If you want a
> > define that can be used for resetting both pipes then add two separate
> > defines for up and down respectively, and add a third define for
> > resetting both buffer as a bitwise OR of the two.
> 
> Charles Ans:
> Yes,The whole register is used for reset.
> Bit 0 and bit 1 are used for up & downstream data pipe,
> Bit 2 for interface reset
> Bit 4 for chip reset.
> 
> But I only reset bit 0 & bit 1.

Yes, but you need to reflect that in how you name your defines. Add two
separate defines for up and downstream data pipe reset. If you want you
add a third as the bitwise-OR of the two as well (perhaps with a _BOTH
suffix in the name).

> > Also move this one after the corresponding register address define
> > below.
> >
> > > +#define PL2303_HXN_RESET_CONTROL     0x07
> > > +#define PL2303_HXN_CTRL_XON_XOFF     0x0C
> > > +#define PL2303_HXN_CTRL_RTS_CTS              0x18
> > > +#define PL2303_HXN_CTRL_NONE         0x1C
> 
> Charles Ans:
> I am not 100% sure what you mean, please see if it is defined below
> 
> #define PL2303_FLOWCTRL_MASK        0xf0
> 
> #define PL2303_READ_TYPE_HX_STATUS    0x8080
> 
> #define PL2303_HXN_CTRL_XON_XOFF    0x0C
> #define PL2303_HXN_CTRL_RTS_CTS        0x18
> #define PL2303_HXN_CTRL_NONE        0x1C
> #define PL2303_HXN_FLOWCTRL_MASK    0x1C
> #define PL2303_HXN_FLOWCTRL        0x0A
> 
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE_MASK    0x03
> #define PL2303_HXN_RESET_CONTROL    0x07

I meant that you should move the bit values (masks) after the register
address that they apply to (as also mentioned above). For example,

	#define PL2303_HXN_RESET_REG	 			0x07
	#define PL2303_HXN_RESET_UPSTREAM_PIPE			0x02
	#define PL2303_HXN_RESET_DOWNSTREAM_PIPE		0x01

> > > +             pl2303_update_reg(serial, PL2303_HXN_RESET_CONTROL,
> > > +                             PL2303_HXN_RESET_CONTROL_MASK, 0x03);
> >
> > So two things; first, do you really need to read back the current value?
> > I would assume that it always reads back as 0 and that writing 0x03 in
> > this case would be sufficient to reset both buffers.
> >
> 
> Charles Ans:
>  Yes, I want to read back the current value.
> because the whole register is used for reset.
> Bit 0 and bit 1 are used for up & downstream data pipe,
> Bit 2 for interface reset
> Bit 4 for chip reset.
> 
> But I only reset bit 0 & bit 1.

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.

> > Second, please use a define for 0x03; no magic constants, as we have
> > discussed before. You don't need a separate mask define if you're always
> > resetting both buffers together (just use the same value define twice).
> 
> Charles Ans:
> OK, I will define for 0x03.
> 
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03

As mentioned above, add separate defines for each pipe. You can also add
a third as their bitwise OR.

Johan