This version is much much better.
On Sun, Jul 15, 2018 at 12:34:28PM -0400, Jacob Feder wrote:
> +static ssize_t sysfs_write(struct device *dev, const char *buf,
> + size_t count, unsigned int addr_offset)
> +{
> + struct axis_fifo *fifo = dev_get_drvdata(dev);
> + unsigned long tmp;
> +
> + if (kstrtoul(buf, 0, &tmp))
> + return -EINVAL;
It's better to preserve the error code from kstrtoul(). I wouldn't
comment on this except there are a bunch of places which need to
preserve the error code.
> +
> + iowrite32(tmp, fifo->base_addr + addr_offset);
> +
> + return count;
> +}
> +
> +static ssize_t sysfs_read(struct device *dev, char *buf,
> + unsigned int addr_offset)
> +{
> + struct axis_fifo *fifo = dev_get_drvdata(dev);
> + unsigned int read_val;
> + unsigned int strlen;
> + char tmp[32];
> +
> + read_val = ioread32(fifo->base_addr + addr_offset);
> +
> + strlen = snprintf(tmp, 32, "0x%08x\n", read_val);
> + if (strlen < 0)
> + return -EINVAL;
Kernel snprintf() won't ever return error codes, so this check is not
required... Just delete it. Or at least preserve the error code.
> +
> + memcpy(buf, tmp, strlen);
> +
> + return strlen;
> +}
> +
> +static void reset_ip_core(struct axis_fifo *fifo)
> +{
> + iowrite32(XLLF_SRR_RESET_MASK,
> + fifo->base_addr + XLLF_SRR_OFFSET);
> + iowrite32(XLLF_TDFR_RESET_MASK,
> + fifo->base_addr + XLLF_TDFR_OFFSET);
> + iowrite32(XLLF_RDFR_RESET_MASK,
> + fifo->base_addr + XLLF_RDFR_OFFSET);
These can fit in 80 characters now.
iowrite32(XLLF_RDFR_RESET_MASK, fifo->base_addr + XLLF_RDFR_OFFSET);
> + iowrite32(XLLF_INT_TC_MASK | XLLF_INT_RC_MASK | XLLF_INT_RPURE_MASK |
> + XLLF_INT_RPORE_MASK | XLLF_INT_RPUE_MASK |
> + XLLF_INT_TPOE_MASK | XLLF_INT_TSE_MASK,
> + fifo->base_addr + XLLF_IER_OFFSET);
> + iowrite32(XLLF_INT_ALL_MASK,
> + fifo->base_addr + XLLF_ISR_OFFSET);
> +}
> +
> +/* reads a single packet from the fifo as dictated by the tlast signal */
> +static ssize_t axis_fifo_read(struct file *f, char __user *buf,
> + size_t len, loff_t *off)
> +{
> + struct axis_fifo *fifo = (struct axis_fifo *)f->private_data;
> + unsigned int bytes_available;
> + unsigned int words_available;
> + unsigned int copied;
> + unsigned int copy;
> + unsigned int i;
> + int ret;
> + u32 tmp_buf[READ_BUF_SIZE];
> +
> + if (fifo->read_flags & O_NONBLOCK) {
> + /* opened in non-blocking mode
> + * return if there are no packets available
> + */
> + if (!ioread32(fifo->base_addr + XLLF_RDFO_OFFSET))
> + return -EAGAIN;
> + } else {
> + /* opened in blocking mode
> + * wait for a packet available interrupt (or timeout)
> + * if nothing is currently available
> + */
> + spin_lock_irq(&fifo->read_queue_lock);
> + if (read_timeout < 0) {
> + ret = wait_event_interruptible_lock_irq_timeout(
> + fifo->read_queue,
> + ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
> + fifo->read_queue_lock, MAX_SCHEDULE_TIMEOUT);
> + } else {
> + ret = wait_event_interruptible_lock_irq_timeout(
> + fifo->read_queue,
> + ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
> + fifo->read_queue_lock,
> + msecs_to_jiffies(read_timeout));
> + }
I'm not a huge fan of ternaries but they would help here:
ret = wait_event_interruptible_lock_irq_timeout(
fifo->read_queue,
ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
fifo->read_queue_lock,
(read_timeout >= 0) ? msecs_to_jiffies(read_timeout) :
MAX_SCHEDULE_TIMEOUT);
> + spin_unlock_irq(&fifo->read_queue_lock);
> +
> + if (ret == 0) {
> + /* timeout occurred */
> + dev_dbg(fifo->dt_device, "read timeout");
> + return -EAGAIN;
> + } else if (ret == -ERESTARTSYS) {
> + /* signal received */
> + return -ERESTARTSYS;
> + } else if (ret < 0) {
> + dev_err(fifo->dt_device, "wait_event_interruptible_timeout() error in read (ret=%i)\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + bytes_available = ioread32(fifo->base_addr + XLLF_RLR_OFFSET);
> + if (!bytes_available) {
> + dev_err(fifo->dt_device, "received a packet of length 0 - fifo core will be reset\n");
> + reset_ip_core(fifo);
> + return -EIO;
> + }
> +
> + if (bytes_available > len) {
> + dev_err(fifo->dt_device, "user read buffer too small (available bytes=%u user buffer bytes=%u) - fifo core will be reset\n",
> + bytes_available, len);
> + reset_ip_core(fifo);
> + return -EINVAL;
> + }
> +
> + if (bytes_available % sizeof(u32)) {
> + /* this probably can't happen unless IP
> + * registers were previously mishandled
> + */
> + dev_err(fifo->dt_device, "received a packet that isn't word-aligned - fifo core will be reset\n");
> + reset_ip_core(fifo);
> + return -EIO;
> + }
> +
> + words_available = bytes_available / sizeof(u32);
> +
> + /* read data into an intermediate buffer, copying the contents
> + * to userspace when the buffer is full
> + */
> + copied = 0;
> + while (words_available > 0) {
> + copy = min(words_available, READ_BUF_SIZE);
> +
> + for (i = 0; i < copy; i++) {
> + tmp_buf[i] = ioread32(fifo->base_addr +
> + XLLF_RDFD_OFFSET);
> + }
> +
> + if (copy_to_user(buf + copied * sizeof(u32), tmp_buf,
> + copy * sizeof(u32))) {
> + reset_ip_core(fifo);
> + return -EFAULT;
> + }
> +
> + copied += copy;
> + words_available -= copy;
> + }
> +
> + return bytes_available;
> +}
> +
> +static ssize_t axis_fifo_write(struct file *f, const char __user *buf,
> + size_t len, loff_t *off)
> +{
> + struct axis_fifo *fifo = (struct axis_fifo *)f->private_data;
> + unsigned int words_to_write;
> + unsigned int copied;
> + unsigned int copy;
> + unsigned int i;
> + int ret;
> + u32 tmp_buf[WRITE_BUF_SIZE];
> +
> + if (len % sizeof(u32)) {
> + dev_err(fifo->dt_device,
> + "tried to send a packet that isn't word-aligned\n");
> + return -EINVAL;
> + }
> +
> + words_to_write = len / sizeof(u32);
> +
> + if (!words_to_write) {
> + dev_err(fifo->dt_device,
> + "tried to send a packet of length 0\n");
> + return -EINVAL;
> + }
> +
> + if (words_to_write > fifo->tx_fifo_depth) {
> + dev_err(fifo->dt_device, "tried to write more words [%u] than slots in the fifo buffer [%u]\n",
> + words_to_write, fifo->tx_fifo_depth);
> + return -EINVAL;
> + }
> +
> + if (fifo->write_flags & O_NONBLOCK) {
> + /* opened in non-blocking mode
> + * return if there is not enough room available in the fifo
> + */
> + if (words_to_write > ioread32(fifo->base_addr +
> + XLLF_TDFV_OFFSET)) {
> + return -EAGAIN;
> + }
> + } else {
> + /* opened in blocking mode */
> +
> + /* wait for an interrupt (or timeout) if there isn't
> + * currently enough room in the fifo
> + */
> + spin_lock_irq(&fifo->write_queue_lock);
> + if (write_timeout < 0) {
> + ret = wait_event_interruptible_lock_irq_timeout(
> + fifo->write_queue,
> + ioread32(fifo->base_addr +
> + XLLF_TDFV_OFFSET) >=
> + words_to_write,
> + fifo->write_queue_lock,
> + MAX_SCHEDULE_TIMEOUT);
> + } else {
> + ret = wait_event_interruptible_lock_irq_timeout(
> + fifo->write_queue,
> + ioread32(fifo->base_addr +
> + XLLF_TDFV_OFFSET) >=
> + words_to_write,
> + fifo->write_queue_lock,
> + msecs_to_jiffies(write_timeout));
> + }
ret = wait_event_interruptible_lock_irq_timeout(
fifo->write_queue,
ioread32(fifo->base_addr + XLLF_TDFV_OFFSET)
>= words_to_write,
fifo->write_queue_lock,
(write_timeout < 0) ? MAX_SCHEDULE_TIMEOUT :
msecs_to_jiffies(write_timeout));
> + spin_unlock_irq(&fifo->write_queue_lock);
> +
> + if (ret == 0) {
> + /* timeout occurred */
> + dev_dbg(fifo->dt_device, "write timeout\n");
> + return -EAGAIN;
> + } else if (ret == -ERESTARTSYS) {
> + /* signal received */
> + return -ERESTARTSYS;
> + } else if (ret < 0) {
> + /* unknown error */
> + dev_err(fifo->dt_device,
> + "wait_event_interruptible_timeout() error in write (ret=%i)\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + /* write data from an intermediate buffer into the fifo IP, refilling
> + * the buffer with userspace data as needed
> + */
> + copied = 0;
> + while (words_to_write > 0) {
> + copy = min(words_to_write, WRITE_BUF_SIZE);
> +
> + if (copy_from_user(tmp_buf, buf + copied * sizeof(u32),
> + copy * sizeof(u32))) {
> + reset_ip_core(fifo);
> + return -EFAULT;
> + }
> +
> + for (i = 0; i < copy; i++)
> + iowrite32(tmp_buf[i], fifo->base_addr +
> + XLLF_TDFD_OFFSET);
> +
> + copied += copy;
> + words_to_write -= copy;
> + }
> +
> + /* write packet size to fifo */
> + iowrite32(copied * sizeof(u32), fifo->base_addr + XLLF_TLR_OFFSET);
> +
> + return (ssize_t)copied * sizeof(u32);
> +}
> +
> +/* read named property from the device tree */
> +static int get_dts_property(struct axis_fifo *fifo,
> + char *name, unsigned int *var)
> +{
> + if (of_property_read_u32(fifo->dt_device->of_node,
> + name, var) < 0) {
> + dev_err(fifo->dt_device,
> + "couldn't read IP dts property '%s'", name);
> + return -1;
Preserve the error code. Plus it looks nicer:
rc = of_property_read_u32(fifo->dt_device->of_node, name, var);
if (rc) {
dev_err(fifo->dt_device, "couldn't read IP dts property '%s'",
name);
return rc;
}
> + }
> + dev_dbg(fifo->dt_device, "dts property '%s' = %u\n",
> + name, *var);
> +
> + return 0;
> +}
> +
> +static int axis_fifo_probe(struct platform_device *pdev)
> +{
> + struct resource *r_irq; /* interrupt resources */
> + struct resource *r_mem; /* IO mem resources */
> + struct device *dev = &pdev->dev; /* OS device (from device tree) */
> + struct axis_fifo *fifo = NULL;
> +
> + char device_name[32];
> +
> + int rc = 0; /* error return value */
> +
> + /* IP properties from device tree */
> + unsigned int rxd_tdata_width;
> + unsigned int txc_tdata_width;
> + unsigned int txd_tdata_width;
> + unsigned int tdest_width;
> + unsigned int tid_width;
> + unsigned int tuser_width;
> + unsigned int data_interface_type;
> + unsigned int has_tdest;
> + unsigned int has_tid;
> + unsigned int has_tkeep;
> + unsigned int has_tstrb;
> + unsigned int has_tuser;
> + unsigned int rx_fifo_depth;
> + unsigned int rx_programmable_empty_threshold;
> + unsigned int rx_programmable_full_threshold;
> + unsigned int axi_id_width;
> + unsigned int axi4_data_width;
> + unsigned int select_xpm;
> + unsigned int tx_fifo_depth;
> + unsigned int tx_programmable_empty_threshold;
> + unsigned int tx_programmable_full_threshold;
> + unsigned int use_rx_cut_through;
> + unsigned int use_rx_data;
> + unsigned int use_tx_control;
> + unsigned int use_tx_cut_through;
> + unsigned int use_tx_data;
> +
> + /* ----------------------------
> + * init wrapper device
> + * ----------------------------
> + */
> +
> + /* allocate device wrapper memory */
> + fifo = devm_kmalloc(dev, sizeof(*fifo), GFP_KERNEL);
> + if (!fifo)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, fifo);
> + fifo->dt_device = dev;
> +
> + init_waitqueue_head(&fifo->read_queue);
> + init_waitqueue_head(&fifo->write_queue);
> +
> + dev_dbg(fifo->dt_device, "initialized queues\n");
This print doesn't tell you anything except that the function was
called. Just remove it and use ftrace.
> +
> + spin_lock_init(&fifo->read_queue_lock);
> + spin_lock_init(&fifo->write_queue_lock);
> +
> + dev_dbg(fifo->dt_device, "initialized spinlocks\n");
Delete.
> +
> + /* ----------------------------
> + * init device memory space
> + * ----------------------------
> + */
> +
> + /* get iospace for the device */
> + r_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!r_mem) {
> + dev_err(fifo->dt_device, "invalid address\n");
> + rc = -ENODEV;
> + goto err_initial;
> + }
> +
> + fifo->mem_start = r_mem->start;
> + fifo->mem_end = r_mem->end;
Why not just save r_mem?
> +
> + /* request physical memory */
> + if (!request_mem_region(fifo->mem_start,
> + fifo->mem_end -
> + fifo->mem_start + 1,
resource_size(r_mem),
> + DRIVER_NAME)) {
> + dev_err(fifo->dt_device,
> + "couldn't lock memory region at %p\n",
> + (void *)fifo->mem_start);
> + rc = -EBUSY;
> + goto err_initial;
> + }
> + dev_dbg(fifo->dt_device,
> + "got memory location [0x%x - 0x%x]\n",
> + fifo->mem_start, fifo->mem_end);
> +
> + /* map physical memory to kernel virtual address space */
> + fifo->base_addr = ioremap(fifo->mem_start, fifo->mem_end -
> + fifo->mem_start + 1);
fifo->base_addr = ioremap(fifo->mem_start, resource_size(r_mem));
Fits on one line.
> +
> + if (!fifo->base_addr) {
> + dev_err(fifo->dt_device,
> + "couldn't map physical memory\n");
> + rc = -EIO;
For ioremap() failure the "rc = -ENOMEM;".
> + goto err_mem;
> + }
> + dev_dbg(fifo->dt_device, "remapped memory to 0x%x\n",
> + (unsigned int)fifo->base_addr);
Use %p. Casting a pointer to (unsigned int) will generate a GCC
warning. Plus casting is ugly.
> +
> + /* create unique device name */
> + snprintf(device_name, 32, DRIVER_NAME "_%x",
> + (unsigned int)fifo->mem_start);
Get rid of the magic 32 and the cast.
snprintf(device_name, sizeof(device_name) "%s_%x",
DRIVER_NAME, fifo->mem_start);
> +
> + dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
> +
> + /* ----------------------------
> + * init IP
> + * ----------------------------
> + */
> +
> + /* retrieve device tree properties */
> + if (get_dts_property(fifo, "xlnx,axi-str-rxd-tdata-width",
> + &rxd_tdata_width)) {
> + rc = -EIO;
> + goto err_mem;
After the ioremap() succeeds all the gotos should be goto unmap; until
we get to the next allocation. Also preserve the error code:
rc = get_dts_property(fifo, "xlnx,axi-str-rxd-tdata-width",
&rxd_tdata_width);
if (rc)
goto unmap;
> + }
> + if (get_dts_property(fifo, "xlnx,axi-str-txc-tdata-width",
> + &txc_tdata_width)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,axi-str-txd-tdata-width",
> + &txd_tdata_width)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,axis-tdest-width",
> + &tdest_width)) {
This will fit on one line:
rc = get_dts_property(fifo, "xlnx,axis-tdest-width", &tdest_width);
if (rc)
goto unmap;
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,axis-tid-width",
> + &tid_width)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,axis-tuser-width",
> + &tuser_width)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,data-interface-type",
> + &data_interface_type)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,has-axis-tdest",
> + &has_tdest)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,has-axis-tid",
> + &has_tid)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,has-axis-tkeep",
> + &has_tkeep)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,has-axis-tstrb",
> + &has_tstrb)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,has-axis-tuser",
> + &has_tuser)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,rx-fifo-depth",
> + &rx_fifo_depth)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,rx-fifo-pe-threshold",
> + &rx_programmable_empty_threshold)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,rx-fifo-pf-threshold",
> + &rx_programmable_full_threshold)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,s-axi-id-width",
> + &axi_id_width)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,s-axi4-data-width",
> + &axi4_data_width)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,select-xpm",
> + &select_xpm)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,tx-fifo-depth",
> + &tx_fifo_depth)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,tx-fifo-pe-threshold",
> + &tx_programmable_empty_threshold)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,tx-fifo-pf-threshold",
> + &tx_programmable_full_threshold)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,use-rx-cut-through",
> + &use_rx_cut_through)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,use-rx-data",
> + &use_rx_data)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,use-tx-ctrl",
> + &use_tx_control)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,use-tx-cut-through",
> + &use_tx_cut_through)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (get_dts_property(fifo, "xlnx,use-tx-data",
> + &use_tx_data)) {
> + rc = -EIO;
> + goto err_mem;
> + }
> +
> + /* check validity of device tree properties */
> + if (rxd_tdata_width != 32) {
> + dev_err(fifo->dt_device,
> + "rxd_tdata_width width [%u] unsupported\n",
> + rxd_tdata_width);
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (txd_tdata_width != 32) {
> + dev_err(fifo->dt_device,
> + "txd_tdata_width width [%u] unsupported\n",
> + txd_tdata_width);
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (has_tdest) {
> + dev_err(fifo->dt_device, "tdest not supported\n");
> + rc = -EIO;
> + goto err_mem;
> + }
Just out of curiosity, what would happen if we remove this check?
> + if (has_tid) {
> + dev_err(fifo->dt_device, "tid not supported\n");
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (has_tkeep) {
> + dev_err(fifo->dt_device, "tkeep not supported\n");
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (has_tstrb) {
> + dev_err(fifo->dt_device, "tstrb not supported\n");
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (has_tuser) {
> + dev_err(fifo->dt_device, "tuser not supported\n");
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (use_rx_cut_through) {
> + dev_err(fifo->dt_device,
> + "rx cut-through not supported\n");
Fits on one line:
dev_err(fifo->dt_device, "rx cut-through not supported\n");
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (use_tx_cut_through) {
> + dev_err(fifo->dt_device,
> + "tx cut-through not supported\n");
> + rc = -EIO;
> + goto err_mem;
> + }
> + if (use_tx_control) {
> + dev_err(fifo->dt_device,
> + "tx control not supported\n");
> + rc = -EIO;
> + goto err_mem;
> + }
> +
> + /* TODO
> + * these exist in the device tree but it's unclear what they do
> + * - select-xpm
> + * - data-interface-type
> + */
> +
> + /* set device wrapper properties based on IP config */
> + fifo->rx_fifo_depth = rx_fifo_depth;
> + /* IP sets TDFV to fifo depth - 4 so we will do the same */
> + fifo->tx_fifo_depth = tx_fifo_depth - 4;
> + fifo->has_rx_fifo = use_rx_data;
> + fifo->has_tx_fifo = use_tx_data;
> +
> + reset_ip_core(fifo);
> +
> + /* ----------------------------
> + * init device interrupts
> + * ----------------------------
> + */
> +
> + /* get IRQ resource */
> + r_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!r_irq) {
> + dev_err(fifo->dt_device,
> + "no IRQ found at 0x%08x mapped to 0x%08x\n",
> + (unsigned int __force)fifo->mem_start,
> + (unsigned int __force)fifo->base_addr);
No need for __force. Just get rid to the casts actually.
> + rc = -EIO;
> + goto err_mem;
> + }
> + dev_dbg(fifo->dt_device, "found IRQ\n");
> +
> + /* request IRQ */
> + fifo->irq = r_irq->start;
> + rc = request_irq(fifo->irq, &axis_fifo_irq, 0,
> + DRIVER_NAME, fifo);
> + if (rc) {
> + dev_err(fifo->dt_device,
> + "couldn't allocate interrupt %i\n",
> + fifo->irq);
> + rc = -EIO;
Just leave rc as is.
> + goto err_mem;
> + }
> + dev_dbg(fifo->dt_device,
> + "initialized IRQ %i\n",
> + fifo->irq);
Fits on one line.
> +
> + /* ----------------------------
> + * init char device
> + * ----------------------------
> + */
> +
> + /* allocate device number */
> + if (alloc_chrdev_region(&fifo->devt, 0, 1, DRIVER_NAME) < 0) {
> + dev_err(fifo->dt_device, "couldn't allocate dev_t\n");
> + rc = -EIO;
> + goto err_irq;
> + }
rc = alloc_chrdev_region(&fifo->devt, 0, 1, DRIVER_NAME);
if (rc)
goto err_irq;
Most of these functions have their own printks built in so there isn't
really a need to have a printk here.
> + dev_dbg(fifo->dt_device,
> + "allocated device number major %i minor %i\n",
> + MAJOR(fifo->devt), MINOR(fifo->devt));
> +
> + /* create driver file */
> + fifo->device = NULL;
Delete this line.
> + fifo->device = device_create(axis_fifo_driver_class, NULL, fifo->devt,
> + NULL, device_name);
> + if (!fifo->device) {
> + dev_err(fifo->dt_device,
> + "couldn't create driver file\n");
> + rc = -EIO;
rc = -ENOMEM;
> + goto err_chrdev_region;
> + }
> + dev_set_drvdata(fifo->device, fifo);
> + dev_dbg(fifo->dt_device, "created device file\n");
Just delete these debugging lines. It's obviously to the point where
it's working and you can test it now.
> +
> + /* create character device */
> + cdev_init(&fifo->char_device, &fops);
> + if (cdev_add(&fifo->char_device,
> + fifo->devt, 1) < 0) {
> + dev_err(fifo->dt_device,
> + "couldn't create character device\n");
> + rc = -EIO;
> + goto err_dev;
> + }
Preserve the code.
> + dev_dbg(fifo->dt_device, "created character device\n");
> +
> + /* create sysfs entries */
> + if (sysfs_create_group(&fifo->device->kobj,
> + &axis_fifo_attrs_group) < 0) {
> + dev_err(fifo->dt_device,
> + "couldn't register sysfs group\n");
> + rc = -EIO;
> + goto err_cdev;
> + }
> +
> + dev_info(fifo->dt_device,
> + "axis-fifo created at 0x%08x mapped to 0x%08x, irq=%i, major=%i, minor=%i\n",
> + (unsigned int __force)fifo->mem_start,
> + (unsigned int __force)fifo->base_addr,
> + fifo->irq, MAJOR(fifo->devt),
> + MINOR(fifo->devt));
> +
> + return 0;
> +
> +err_cdev:
> + cdev_del(&fifo->char_device);
> +err_dev:
> + dev_set_drvdata(fifo->device, NULL);
This hopefully isn't required since we delete fifo->device on the
next line.
> + device_destroy(axis_fifo_driver_class, fifo->devt);
> +err_chrdev_region:
> + unregister_chrdev_region(fifo->devt, 1);
> +err_irq:
> + free_irq(fifo->irq, fifo);
> +err_mem:
> + release_mem_region(fifo->mem_start,
> + fifo->mem_end -
> + fifo->mem_start + 1);
> +err_initial:
> + dev_set_drvdata(dev, NULL);
> + return rc;
> +}
> +
> +static int axis_fifo_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct axis_fifo *fifo = dev_get_drvdata(dev);
> +
> + dev_info(dev, "removing\n");
Delete. ftrace.
> +
> + sysfs_remove_group(&fifo->device->kobj,
> + &axis_fifo_attrs_group);
One line:
sysfs_remove_group(&fifo->device->kobj, &axis_fifo_attrs_group);
> + cdev_del(&fifo->char_device);
> + dev_set_drvdata(fifo->device, NULL);
> + device_destroy(axis_fifo_driver_class, fifo->devt);
> + unregister_chrdev_region(fifo->devt, 1);
> + free_irq(fifo->irq, fifo);
Don't forget to do an iounmap() here as well.
> + release_mem_region(fifo->mem_start,
> + fifo->mem_end -
> + fifo->mem_start + 1);
> + dev_set_drvdata(dev, NULL);
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id axis_fifo_of_match[] = {
> + { .compatible = "xlnx,axi-fifo-mm-s-4.1", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, axis_fifo_of_match);
> +#else
> +# define axis_fifo_of_match
> +#endif
> +
> +static struct platform_driver axis_fifo_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = axis_fifo_of_match,
> + },
> + .probe = axis_fifo_probe,
> + .remove = axis_fifo_remove,
> +};
> +
> +static int __init axis_fifo_init(void)
> +{
> + printk(KERN_INFO "axis-fifo driver loaded with parameters read_timeout = %i, write_timeout = %i\n",
> + read_timeout, write_timeout);
Delete. Or update to use pr_info() at least.
> + axis_fifo_driver_class = class_create(THIS_MODULE, DRIVER_NAME);
> + return platform_driver_register(&axis_fifo_driver);
> +}
> +
> +module_init(axis_fifo_init);
> +
> +static void __exit axis_fifo_exit(void)
> +{
> + platform_driver_unregister(&axis_fifo_driver);
> + class_destroy(axis_fifo_driver_class);
> + printk(KERN_INFO "axis-fifo driver exit\n");
Delete.
> +}
> +
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
