From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Oleksandr Shamray <oleksandrs@mellanox.com>
Cc: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Arnd Bergmann" <arnd@arndb.de>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>,
devicetree <devicetree@vger.kernel.org>,
"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
"Joel Stanley" <joel@jms.id.au>, "Jiří Pírko" <jiri@resnulli.us>,
"Tobias Klauser" <tklauser@distanz.ch>,
"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
"Vadim Pasternak" <vadimp@mellanox.com>,
system-sw-low-level <system-sw-low-level@mellanox.com>,
"Rob Herring" <robh+dt@kernel.org>,
"openocd-devel-owner@lists.sourceforge.net"
<openocd-devel-owner@lists.sourceforge.net>,
"linux-api@vger.kernel.org" <linux-api@vger.kernel.org>
Subject: Re: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
Date: Tue, 22 May 2018 23:21:06 +0300 [thread overview]
Message-ID: <CAHp75Vedcsbet3L0p0t5-=J0PmSe26VFXDbcRmk9943D9_FSqQ@mail.gmail.com> (raw)
In-Reply-To: <AM5PR0501MB244981D83DD33BD29FD64924B1940@AM5PR0501MB2449.eurprd05.prod.outlook.com>
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
next prev parent reply other threads:[~2018-05-22 20:21 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-15 14:21 [patch v21 0/4] JTAG driver introduction Oleksandr Shamray
2018-05-15 14:21 ` [patch v21 1/4] drivers: jtag: Add JTAG core driver Oleksandr Shamray
2018-05-15 21:21 ` Andy Shevchenko
2018-05-22 15:00 ` Oleksandr Shamray
2018-05-15 14:21 ` [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver Oleksandr Shamray
2018-05-15 21:00 ` Andy Shevchenko
2018-05-22 15:00 ` Oleksandr Shamray
2018-05-22 20:21 ` Andy Shevchenko [this message]
2018-05-15 14:21 ` [patch v21 3/4] Documentation: jtag: Add bindings for " Oleksandr Shamray
2018-05-15 14:21 ` [patch v21 4/4] Documentation: jtag: Add ABI documentation Oleksandr Shamray
2018-05-16 18:16 ` Randy Dunlap
2018-05-22 13:08 [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver Oleksandr Shamray
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHp75Vedcsbet3L0p0t5-=J0PmSe26VFXDbcRmk9943D9_FSqQ@mail.gmail.com' \
--to=andy.shevchenko@gmail.com \
--cc=arnd@arndb.de \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jiri@resnulli.us \
--cc=joel@jms.id.au \
--cc=linux-api@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=oleksandrs@mellanox.com \
--cc=openbmc@lists.ozlabs.org \
--cc=openocd-devel-owner@lists.sourceforge.net \
--cc=robh+dt@kernel.org \
--cc=system-sw-low-level@mellanox.com \
--cc=tklauser@distanz.ch \
--cc=vadimp@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).