lkml.org 
[lkml]   [2018]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
On Tue, May 22, 2018 at 6:00 PM, Oleksandr Shamray
<oleksandrs@mellanox.com> 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

\
 
 \ /
  Last update: 2018-05-22 22:22    [W:0.043 / U:2.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site