On Tue, Aug 27, 2019 at 04:40:39PM +0800, Charles Yeh wrote: > Johan Hovold <johan@xxxxxxxxxx> 於 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
