From: Greg KH <gregkh@linuxfoundation.org>
To: shubhrajyoti.datta@gmail.com
Cc: linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
devicetree@vger.kernel.org, arnd@arndb.de,
michal.simek@xilinx.com, robh+dt@kernel.org,
Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>,
Kedareswara rao Appana <appanad@xilinx.com>,
Srikanth Thokala <sthokal@xilinx.com>
Subject: Re: [PATCH v2 2/3] trafgen: xilinx: add axi traffic generator driver
Date: Mon, 9 Dec 2019 09:06:45 +0100 [thread overview]
Message-ID: <20191209080645.GA706232@kroah.com> (raw)
In-Reply-To: <d66da6c524d01414562d3c15853174eeafa0c9fa.1575871828.git.shubhrajyoti.datta@xilinx.com>
On Mon, Dec 09, 2019 at 11:41:27AM +0530, shubhrajyoti.datta@gmail.com wrote:
> +/* Macro */
We know it's a macro, no need to say it :)
> +#define to_xtg_dev_info(n) ((struct xtg_dev_info *)dev_get_drvdata(n))
No need for the cast, and really, no need for this macro at all. Please
drop it.
> + * FIXME: This structure is shared with the user application and
> + * hence need to be synchronized. We know these kind of structures
> + * should not be defined in the driver and this need to be fixed
> + * if found a proper placeholder (in uapi/).
Woah! This isn't ok to leave as a fixme. Please fix properly.
As it is, this is NOT a portable data structure by any means.
> + * FIXME: This structure is shared with the user application and
> + * hence need to be synchronized. We know these kind of structures
> + * should not be defined in the driver and this need to be fixed
> + * if found a proper placeholder (in uapi/).
Same here, this is just flat out not going to work.
> +static void xtg_access_rams(struct xtg_dev_info *tg, int where,
> + int count, int flags, u32 *data)
> +{
> + u32 index;
> +
> + switch (flags) {
> + case XTG_WRITE_RAM_ZERO:
> + memset_io(tg->regs + where, 0, count);
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
No #ifdef in .c code please, fix this correctly.
> + writel(0x0, tg->regs + where +
> + (XTG_COMMAND_RAM_MSB_OFFSET - XTG_COMMAND_RAM_OFFSET) +
> + XTG_EXTCMD_RAM_BLOCK_SIZE - XTG_CMD_RAM_BLOCK_SIZE);
> +#endif
> + break;
> + case XTG_WRITE_RAM:
> + for (index = 0; count > 0; index++, count -= 4)
> + writel(data[index], tg->regs + where + index * 4);
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
Same here, and everywhere else.
> +/**
> + * xtg_sysfs_ioctl - Implements sysfs operations
sysfs is not an ioctl. Please fix your naming.
> + * @dev: Device structure
> + * @buf: Value to write
> + * @opcode: Ioctl opcode
> + *
> + * Return: value read from the sysfs opcode.
> + */
Why are you creating kernel doc documentation for static functions?
That's not needed at all, right?
> +static ssize_t xtg_sysfs_ioctl(struct device *dev, const char *buf,
> + enum xtg_sysfs_ioctl_opcode opcode)
> +{
> + struct xtg_dev_info *tg = to_xtg_dev_info(dev);
> + unsigned long wrval;
> + ssize_t status, rdval = 0;
> +
> + if (opcode > XTG_GET_STREAM_TRANSFERCNT) {
> + status = kstrtoul(buf, 0, &wrval);
> + if (status < 0)
> + return status;
> + }
> +
> + switch (opcode) {
> + case XTG_GET_MASTER_CMP_STS:
> + rdval = (readl(tg->regs + XTG_MCNTL_OFFSET) &
> + XTG_MCNTL_MSTEN_MASK) ? 1 : 0;
> + break;
> +
> + case XTG_GET_MASTER_LOOP_EN:
> + rdval = (readl(tg->regs + XTG_MCNTL_OFFSET) &
> + XTG_MCNTL_LOOPEN_MASK) ? 1 : 0;
> + break;
> +
> + case XTG_GET_SLV_CTRL_REG:
> + rdval = readl(tg->regs + XTG_SCNTL_OFFSET);
> + break;
> +
> + case XTG_GET_ERR_STS:
> + rdval = readl(tg->regs + XTG_ERR_STS_OFFSET) &
> + XTG_ERR_ALL_ERRS_MASK;
> + break;
> +
> + case XTG_GET_CFG_STS:
> + rdval = readl(tg->regs + XTG_CFG_STS_OFFSET);
> + break;
> +
> + case XTG_GET_LAST_VALID_INDEX:
> + rdval = (((tg->last_wr_valid_idx << 16) & 0xffff0000) |
> + (tg->last_rd_valid_idx & 0xffff));
> + break;
> +
> + case XTG_GET_DEVICE_ID:
> + rdval = tg->id;
> + break;
> +
> + case XTG_GET_RESOURCE:
> + rdval = (unsigned long)tg->regs;
> + break;
> +
> + case XTG_GET_STATIC_ENABLE:
> + rdval = readl(tg->regs + XTG_STATIC_CNTL_OFFSET);
> + break;
> +
> + case XTG_GET_STATIC_BURSTLEN:
> + rdval = readl(tg->regs + XTG_STATIC_LEN_OFFSET);
> + break;
> +
> + case XTG_GET_STATIC_TRANSFERDONE:
> + rdval = (readl(tg->regs + XTG_STATIC_CNTL_OFFSET) &
> + XTG_STATIC_CNTL_TD_MASK);
> + break;
> +
> + case XTG_GET_STREAM_ENABLE:
> + rdval = readl(tg->regs + XTG_STREAM_CNTL_OFFSET);
> + break;
> +
> + case XTG_GET_STREAM_TRANSFERLEN:
> + rdval = (readl(tg->regs + XTG_STREAM_TL_OFFSET) &
> + XTG_STREAM_TL_TLEN_MASK);
> + break;
> +
> + case XTG_GET_STREAM_TRANSFERCNT:
> + rdval = ((readl(tg->regs + XTG_STREAM_TL_OFFSET) &
> + XTG_STREAM_TL_TCNT_MASK) >>
> + XTG_STREAM_TL_TCNT_SHIFT);
> + break;
> +
> + case XTG_GET_STREAM_TKTS1:
> + rdval = readl(tg->regs + XTG_STREAM_TKTS1_OFFSET);
> + break;
> + case XTG_GET_STREAM_TKTS2:
> + rdval = readl(tg->regs + XTG_STREAM_TKTS2_OFFSET);
> + break;
> + case XTG_GET_STREAM_TKTS3:
> + rdval = readl(tg->regs + XTG_STREAM_TKTS3_OFFSET);
> + break;
> + case XTG_GET_STREAM_TKTS4:
> + rdval = readl(tg->regs + XTG_STREAM_TKTS4_OFFSET);
> + break;
> +
> + case XTG_GET_STREAM_CFG:
> + rdval = (readl(tg->regs + XTG_STREAM_CFG_OFFSET));
> + break;
> +
> + case XTG_START_MASTER_LOGIC:
> + if (wrval)
> + writel(readl(tg->regs + XTG_MCNTL_OFFSET) |
> + XTG_MCNTL_MSTEN_MASK,
> + tg->regs + XTG_MCNTL_OFFSET);
> + break;
> +
> + case XTG_MASTER_LOOP_EN:
> + if (wrval)
> + writel(readl(tg->regs + XTG_MCNTL_OFFSET) |
> + XTG_MCNTL_LOOPEN_MASK,
> + tg->regs + XTG_MCNTL_OFFSET);
> + else
> + writel(readl(tg->regs + XTG_MCNTL_OFFSET) &
> + ~XTG_MCNTL_LOOPEN_MASK,
> + tg->regs + XTG_MCNTL_OFFSET);
> + break;
> +
> + case XTG_SET_SLV_CTRL_REG:
> + writel(wrval, tg->regs + XTG_SCNTL_OFFSET);
> + break;
> +
> + case XTG_ENABLE_ERRORS:
> + wrval &= XTG_ERR_ALL_ERRS_MASK;
> + writel(wrval, tg->regs + XTG_ERR_EN_OFFSET);
> + break;
> +
> + case XTG_CLEAR_ERRORS:
> + wrval &= XTG_ERR_ALL_ERRS_MASK;
> + writel(readl(tg->regs + XTG_ERR_STS_OFFSET) | wrval,
> + tg->regs + XTG_ERR_STS_OFFSET);
> + break;
> +
> + case XTG_ENABLE_INTRS:
> + if (wrval & XTG_MASTER_CMP_INTR) {
> + pr_info("Enabling Master Complete Interrupt\n");
> + writel(readl(tg->regs + XTG_ERR_EN_OFFSET) |
> + XTG_ERR_EN_MSTIRQEN_MASK,
> + tg->regs + XTG_ERR_EN_OFFSET);
> + }
> + if (wrval & XTG_MASTER_ERR_INTR) {
> + pr_info("Enabling Interrupt on Master Errors\n");
> + writel(readl(tg->regs + XTG_MSTERR_INTR_OFFSET) |
> + XTG_MSTERR_INTR_MINTREN_MASK,
> + tg->regs + XTG_MSTERR_INTR_OFFSET);
> + }
> + if (wrval & XTG_SLAVE_ERR_INTR) {
> + pr_info("Enabling Interrupt on Slave Errors\n");
> + writel(readl(tg->regs + XTG_SCNTL_OFFSET) |
> + XTG_SCNTL_ERREN_MASK,
> + tg->regs + XTG_SCNTL_OFFSET);
> + }
> + break;
> +
> + case XTG_CLEAR_MRAM:
> + xtg_access_rams(tg, tg->xtg_mram_offset,
> + XTG_MASTER_RAM_SIZE,
> + XTG_WRITE_RAM_ZERO, NULL);
> + break;
> +
> + case XTG_CLEAR_CRAM:
> + xtg_access_rams(tg, XTG_COMMAND_RAM_OFFSET,
> + XTG_COMMAND_RAM_SIZE,
> + XTG_WRITE_RAM_ZERO, NULL);
> + break;
> +
> + case XTG_CLEAR_PRAM:
> + xtg_access_rams(tg, XTG_PARAM_RAM_OFFSET,
> + XTG_PARAM_RAM_SIZE,
> + XTG_WRITE_RAM_ZERO, NULL);
> + break;
> +
> + case XTG_SET_STATIC_ENABLE:
> + if (wrval) {
> + wrval &= XTG_STATIC_CNTL_STEN_MASK;
> + writel(readl(tg->regs + XTG_STATIC_CNTL_OFFSET) | wrval,
> + tg->regs + XTG_STATIC_CNTL_OFFSET);
> + } else {
> + writel(readl(tg->regs + XTG_STATIC_CNTL_OFFSET) &
> + ~XTG_STATIC_CNTL_STEN_MASK,
> + tg->regs + XTG_STATIC_CNTL_OFFSET);
> + }
> + break;
> +
> + case XTG_SET_STATIC_BURSTLEN:
> + writel(wrval, tg->regs + XTG_STATIC_LEN_OFFSET);
> + break;
> +
> + case XTG_SET_STATIC_TRANSFERDONE:
> + wrval |= XTG_STATIC_CNTL_TD_MASK;
> + writel(readl(tg->regs + XTG_STATIC_CNTL_OFFSET) | wrval,
> + tg->regs + XTG_STATIC_CNTL_OFFSET);
> + break;
> +
> + case XTG_SET_STREAM_ENABLE:
> + if (wrval) {
> + rdval = readl(tg->regs + XTG_STREAM_CNTL_OFFSET);
> + rdval |= XTG_STREAM_CNTL_STEN_MASK,
> + writel(rdval,
> + tg->regs + XTG_STREAM_CNTL_OFFSET);
> + } else {
> + writel(readl(tg->regs + XTG_STREAM_CNTL_OFFSET) &
> + ~XTG_STREAM_CNTL_STEN_MASK,
> + tg->regs + XTG_STREAM_CNTL_OFFSET);
> + }
> + break;
> +
> + case XTG_SET_STREAM_TRANSFERLEN:
> + wrval &= XTG_STREAM_TL_TLEN_MASK;
> + rdval = readl(tg->regs + XTG_STREAM_TL_OFFSET);
> + rdval &= ~XTG_STREAM_TL_TLEN_MASK;
> + writel(rdval | wrval,
> + tg->regs + XTG_STREAM_TL_OFFSET);
> + break;
> +
> + case XTG_SET_STREAM_TRANSFERCNT:
> + wrval = ((wrval << XTG_STREAM_TL_TCNT_SHIFT) &
> + XTG_STREAM_TL_TCNT_MASK);
> + rdval = readl(tg->regs + XTG_STREAM_TL_OFFSET);
> + rdval = rdval & ~XTG_STREAM_TL_TCNT_MASK;
> + writel(rdval | wrval,
> + tg->regs + XTG_STREAM_TL_OFFSET);
> + break;
> +
> + case XTG_SET_STREAM_TKTS1:
> + writel(wrval, tg->regs + XTG_STREAM_TKTS1_OFFSET);
> + break;
> + case XTG_SET_STREAM_TKTS2:
> + writel(wrval, tg->regs + XTG_STREAM_TKTS2_OFFSET);
> + break;
> + case XTG_SET_STREAM_TKTS3:
> + writel(wrval, tg->regs + XTG_STREAM_TKTS3_OFFSET);
> + break;
> + case XTG_SET_STREAM_TKTS4:
> + writel(wrval, tg->regs + XTG_STREAM_TKTS4_OFFSET);
> + break;
> +
> + case XTG_SET_STREAM_CFG:
> + writel(wrval, tg->regs + XTG_STREAM_CFG_OFFSET);
> + break;
> +
> + default:
> + break;
> + }
> +
> + return rdval;
> +}
> +
> +/* Sysfs functions */
> +
> +static ssize_t id_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + ssize_t rdval = xtg_sysfs_ioctl(dev, buf, XTG_GET_DEVICE_ID);
> +
> + return snprintf(buf, PAGE_SIZE, "%zd\n", rdval);
You "know" the size of the data for sysfs, no need for snprintf() or
friends at all, just use sprintf() please.
> +static ssize_t xtg_pram_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr,
> + char *buf, loff_t off, size_t count)
> +{
> + pr_info("No read access to Parameter RAM\n");
> +
> + return 0;
> +}
This is pointless, why do you have/need this?
Looks like a good way to spam the kernel log :(
And never use pr_* calls in a driver, always use dev_* instead.
> + /*
> + * Create sysfs file entries for the device
> + */
> + err = sysfs_create_group(&dev->kobj, &xtg_attributes);
You just raced with userspace and lost. Set the attribute groups
pointer up correctly and the driver core will create/remove your
attributes for you. This code does it wrong.
> + dev_info(&pdev->dev, "Probing xilinx traffic generator success\n");
Do NOT be noisy when everything goes correctly. Drivers should not
print out anything if all is well, please drop this.
thanks,
greg k-h
next prev parent reply other threads:[~2019-12-09 8:06 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-09 6:11 [PATCH v2 1/3] dt-bindings: misc: Add dt bindings for traffic generator shubhrajyoti.datta
2019-12-09 6:11 ` [PATCH v2 2/3] trafgen: xilinx: add axi traffic generator driver shubhrajyoti.datta
2019-12-09 8:06 ` Greg KH [this message]
2019-12-09 6:11 ` [PATCH v2 3/3] trafgen: Document sysfs entries shubhrajyoti.datta
2019-12-09 8:07 ` [PATCH v2 1/3] dt-bindings: misc: Add dt bindings for traffic generator Greg KH
2019-12-18 23:55 ` Rob Herring
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=20191209080645.GA706232@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=appanad@xilinx.com \
--cc=arnd@arndb.de \
--cc=devicetree@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=michal.simek@xilinx.com \
--cc=robh+dt@kernel.org \
--cc=shubhrajyoti.datta@gmail.com \
--cc=shubhrajyoti.datta@xilinx.com \
--cc=sthokal@xilinx.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.