On Tue, May 22, 2018 at 6:00 PM, Oleksandr Shamray <oleksandrs@xxxxxxxxxxxx> wrote: > Ok. Changed to: > #define ASPEED_JTAG_IOUT_LEN(len) \ > (ASPEED_JTAG_CTL_ENG_EN | \ > ASPEED_JTAG_CTL_ENG_OUT_EN | \ > ASPEED_JTAG_CTL_INST_LEN(len)) > > #define ASPEED_JTAG_DOUT_LEN(len) \ > (ASPEED_JTAG_CTL_ENG_EN | \ > ASPEED_JTAG_CTL_ENG_OUT_EN | \ > ASPEED_JTAG_CTL_DATA_LEN(len)) What about #define _JTAG_OUT_ENABLE \ ( _ENG_EN | _ENG_OUT_EN) #define _IOUT_LEN(len) \ (_ENABLE | _INST_LEN(len)) #define _DOUT_LEN(len) \ ... ? >> > + apb_frq = clk_get_rate(aspeed_jtag->pclk); >> >> > + div = (apb_frq % freq == 0) ? (apb_frq / freq) - 1 : >> > + (apb_frq / freq); >> >> Isn't it the same as >> >> div = (apb_frq - 1) / freq; >> >> ? > Seems it is same. Thanks. Though be careful if apb_frq == 0. In either case the hw will be screwed, but differently. >> > + if (xfer->direction == JTAG_READ_XFER) >> > + tdi = UINT_MAX; >> > + else >> > + tdi = data[index]; >> >> > + if (xfer->direction == JTAG_READ_XFER) >> > + tdi = UINT_MAX; >> > + else >> > + tdi = data[index]; >> >> Take your time to think how the above duplication can be avoided. >> > > In both cases data[] is different, so I should check it twice, but I will > change it to, macro like: > > #define ASPEED_JTAG_GET_TDI(direction, data) \ > (direction == JTAG_READ_XFER) ? UNIT_MAX : data Perhaps choose better name for data, b/c in the above you are using data[index]. >> > + dev_err(aspeed_jtag->dev, "irq status:%x\n", >> > + status); >> Huh, really?! SPAM. > I will review and delete redundant debug messages. Just to be sure you got a point. This is interrupt context. Imagine what might go wrong. >> > + err = jtag_register(jtag); >> >> Perhaps we might have devm_ variant of this. Check how SPI framework >> deal with a such. >> > > Jtag driver uses miscdevice and related misc_register and misc_deregister > calls for creation and destruction. There is no device object prior > to call to misc_register, which could be used in devm_jtag_register. Same question as per previous patch. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
