Re: [patch v21 1/4] drivers: jtag: Add JTAG core driver

From: Andy Shevchenko
Date: Tue May 15 2018 - 17:21:45 EST


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