From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Shevchenko Subject: Re: [patch v21 1/4] drivers: jtag: Add JTAG core driver Date: Wed, 16 May 2018 00:21:35 +0300 Message-ID: References: <1526394095-5069-1-git-send-email-oleksandrs@mellanox.com> <1526394095-5069-2-git-send-email-oleksandrs@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <1526394095-5069-2-git-send-email-oleksandrs@mellanox.com> Sender: linux-kernel-owner@vger.kernel.org To: Oleksandr Shamray Cc: Greg Kroah-Hartman , Arnd Bergmann , Linux Kernel Mailing List , linux-arm Mailing List , devicetree , openbmc@lists.ozlabs.org, Joel Stanley , =?UTF-8?B?SmnFmcOtIFDDrXJrbw==?= , Tobias Klauser , "open list:SERIAL DRIVERS" , Vadim Pasternak , system-sw-low-level@mellanox.com, Rob Herring , openocd-devel-owner@lists.sourceforge.net, linux-api@vger.kernel.org, "David S. Miller" , Mauro Carvalho Chehab , Jiri Pirko List-Id: devicetree@vger.kernel.org On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray wrote: > Initial patch for JTAG driver > JTAG class driver provide infrastructure to support hardware/software > JTAG platform drivers. It provide user layer API interface for flashing > and debugging external devices which equipped with JTAG interface > using standard transactions. > > Driver exposes set of IOCTL to user space for: > - XFER: > - SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan); > - SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan); > - RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified > number of clocks). > - SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency. > > Driver core provides set of internal APIs for allocation and > registration: > - jtag_register; > - jtag_unregister; > - jtag_alloc; > - jtag_free; > > Platform driver on registration with jtag-core creates the next > entry in dev folder: > /dev/jtagX > 0xB0 all RATIO devices in development: > > 0xB1 00-1F PPPoX > +0xB2 00-0f linux/jtag.h JTAG driver > + Consider to preserve style (upper vs. lower). > + This provides basic core functionality support for JTAG class devices. > + Hardware that is equipped with a JTAG microcontroller can be > + supported by using this driver's interfaces. > + This driver exposes a set of IOCTLs to the user space for > + the following commands: > + SDR: (Scan Data Register) Performs an IEEE 1149.1 Data Register scan > + SIR: (Scan Instruction Register) Performs an IEEE 1149.1 Instruction > + Register scan. > + RUNTEST: Forces the IEEE 1149.1 bus to a run state for a specified > + number of clocks or a specified time period. Something feels wrong with formatting here. > +#define MAX_JTAG_NAME_LEN (sizeof("jtag") + 5) Interesting definition. Why not to set to 10 explicitly? And why 10? (16 sounds better) > +struct jtag { > + struct miscdevice miscdev; > + struct device *dev; Doesn't miscdev parent contain exactly this one? > + const struct jtag_ops *ops; > + int id; > + bool opened; > + struct mutex open_lock; > + unsigned long priv[0]; > +}; > + err = copy_to_user(u64_to_user_ptr(xfer.tdio), > + (void *)(xfer_data), data_size); Redundant parens in one case. Check the rest similar places. > +static int jtag_open(struct inode *inode, struct file *file) > +{ > + struct jtag *jtag = container_of(file->private_data, struct jtag, > + miscdev); I would don't care about length and put it on one line. > + if (jtag->opened) { > + jtag->opened = true; > + jtag->opened = false; Can it be opened non exclusively several times? If so, this needs to be a ref counter instead. > + if (!ops->idle || !ops->mode_set || !ops->status_get || !ops->xfer) > + return NULL; Are all of them mandatory? > +int jtag_register(struct jtag *jtag) Perhaps devm_ variant. > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg) Where is this used or supposed to be used? > +#define JTAG_MAX_XFER_DATA_LEN 65535 Is this limitation from some spec? Otherwise why not to allow 64K? > +/** > + * struct jtag_ops - callbacks for jtag control functions: > + * > + * @freq_get: get frequency function. Filled by device driver > + * @freq_set: set frequency function. Filled by device driver > + * @status_get: set status function. Filled by device driver > + * @idle: set JTAG to idle state function. Filled by device driver > + * @xfer: send JTAG xfer function. Filled by device driver > + */ Perhaps you need to describe which of them are _really_ mandatory and which are optional. -- With Best Regards, Andy Shevchenko