On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray <oleksandrs@xxxxxxxxxxxx> 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: > <mailto:vgo@xxxxxxxx> > 0xB1 00-1F PPPoX <mailto:mostrows@xxxxxxxxxxxxxxxxx> > +0xB2 00-0f linux/jtag.h JTAG driver > + <mailto:oleksandrs@xxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
