On Sun, May 28, 2017 at 10:26:59AM +0200, David Härdeman wrote:
> On Mon, May 22, 2017 at 09:09:42PM +0100, Sean Young wrote:
> >On Mon, May 01, 2017 at 06:04:42PM +0200, David Härdeman wrote:
> >> Using the kernel ida facilities, we can avoid a lot of unnecessary code 
> >> and at the same
> >> time get rid of lirc_dev_lock in favour of per-device locks (the irctls 
> >> array was used
> >> throughout lirc_dev so this patch necessarily touches a lot of code).
> >> 
> >> Signed-off-by: David Härdeman <da...@hardeman.nu>
> >> ---
> >>  drivers/media/rc/ir-lirc-codec.c        |    9 -
> >>  drivers/media/rc/lirc_dev.c             |  258 
> >> ++++++++++++-------------------
> >>  drivers/staging/media/lirc/lirc_zilog.c |   16 +-
> >>  include/media/lirc_dev.h                |   14 --
> >>  4 files changed, 115 insertions(+), 182 deletions(-)
> >> 
> >> diff --git a/drivers/media/rc/ir-lirc-codec.c 
> >> b/drivers/media/rc/ir-lirc-codec.c
> >> index a30af91710fe..2c1221a61ea1 100644
> >> --- a/drivers/media/rc/ir-lirc-codec.c
> >> +++ b/drivers/media/rc/ir-lirc-codec.c
> >> @@ -382,7 +382,6 @@ static int ir_lirc_register(struct rc_dev *dev)
> >>  
> >>    snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
> >>             dev->driver_name);
> >> -  drv->minor = -1;
> >>    drv->features = features;
> >>    drv->data = &dev->raw->lirc;
> >>    drv->rbuf = NULL;
> >> @@ -394,11 +393,9 @@ static int ir_lirc_register(struct rc_dev *dev)
> >>    drv->rdev = dev;
> >>    drv->owner = THIS_MODULE;
> >>  
> >> -  drv->minor = lirc_register_driver(drv);
> >> -  if (drv->minor < 0) {
> >> -          rc = -ENODEV;
> >> +  rc = lirc_register_driver(drv);
> >> +  if (rc < 0)
> >>            goto out;
> >> -  }
> >>  
> >>    dev->raw->lirc.drv = drv;
> >>    dev->raw->lirc.dev = dev;
> >> @@ -413,7 +410,7 @@ static int ir_lirc_unregister(struct rc_dev *dev)
> >>  {
> >>    struct lirc_codec *lirc = &dev->raw->lirc;
> >>  
> >> -  lirc_unregister_driver(lirc->drv->minor);
> >> +  lirc_unregister_driver(lirc->drv);
> >>    kfree(lirc->drv);
> >>    lirc->drv = NULL;
> >>  
> >> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> >> index e44e0b23b883..7db7d4c57991 100644
> >> --- a/drivers/media/rc/lirc_dev.c
> >> +++ b/drivers/media/rc/lirc_dev.c
> >> @@ -31,23 +31,35 @@
> >>  #include <linux/bitops.h>
> >>  #include <linux/device.h>
> >>  #include <linux/cdev.h>
> >> +#include <linux/idr.h>
> >>  
> >>  #include <media/rc-core.h>
> >>  #include <media/lirc.h>
> >>  #include <media/lirc_dev.h>
> >>  
> >>  #define IRCTL_DEV_NAME    "BaseRemoteCtl"
> >> -#define NOPLUG            -1
> >>  #define LOGHEAD           "lirc_dev (%s[%d]): "
> >>  
> >>  static dev_t lirc_base_dev;
> >>  
> >> +/**
> >> + * struct irctl - lirc per-device structure
> >> + * @d:                    internal copy of the &struct lirc_driver for 
> >> the device
> >> + * @dead:         if the device has gone away
> >> + * @users:                the number of users of the lirc chardev 
> >> (currently limited to 1)
> >> + * @mutex:                synchronizes open()/close()/ioctl()/etc calls
> >> + * @buf:          used to store lirc RX data
> >> + * @buf_internal: whether @buf was allocated internally or not
> >> + * @chunk_size:           @buf stores and read() returns data chunks of 
> >> this size
> >> + * @dev:          the &struct device for the lirc device
> >> + * @cdev:         the &struct device for the lirc chardev
> >> + */
> >>  struct irctl {
> >>    struct lirc_driver d;
> >> -  int attached;
> >> -  int open;
> >> +  bool dead;
> >> +  unsigned users;
> >>  
> >> -  struct mutex irctl_lock;
> >> +  struct mutex mutex;
> >>    struct lirc_buffer *buf;
> >>    bool buf_internal;
> >>    unsigned int chunk_size;
> >> @@ -56,9 +68,9 @@ struct irctl {
> >>    struct cdev cdev;
> >>  };
> >>  
> >> -static DEFINE_MUTEX(lirc_dev_lock);
> >> -
> >> -static struct irctl *irctls[MAX_IRCTL_DEVICES];
> >> +/* Used to keep track of allocated lirc devices */
> >> +#define LIRC_MAX_DEVICES 256
> >> +static DEFINE_IDA(lirc_ida);
> >>  
> >>  /* Only used for sysfs but defined to void otherwise */
> >>  static struct class *lirc_class;
> >> @@ -72,9 +84,6 @@ static void lirc_release(struct device *ld)
> >>            kfree(ir->buf);
> >>    }
> >>  
> >> -  mutex_lock(&lirc_dev_lock);
> >> -  irctls[ir->d.minor] = NULL;
> >> -  mutex_unlock(&lirc_dev_lock);
> >>    kfree(ir);
> >>  }
> >>  
> >> @@ -117,7 +126,18 @@ static int lirc_allocate_buffer(struct irctl *ir)
> >>    return err;
> >>  }
> >>  
> >> -int lirc_register_driver(struct lirc_driver *d)
> >> +/**
> >> + * lirc_register_driver() - create a new lirc device by registering a 
> >> driver
> >> + * @d: the &struct lirc_driver to register
> >> + *
> >> + * This function allocates and registers a lirc device for a given
> >> + * &lirc_driver. The &lirc_driver structure is updated to contain
> >> + * information about the allocated device (minor, buffer, etc).
> >> + *
> >> + * Return: zero on success or a negative error value.
> >> + */
> >> +int
> >> +lirc_register_driver(struct lirc_driver *d)
> >>  {
> >>    struct irctl *ir;
> >>    int minor;
> >> @@ -138,12 +158,6 @@ int lirc_register_driver(struct lirc_driver *d)
> >>            return -EINVAL;
> >>    }
> >>  
> >> -  if (d->minor >= MAX_IRCTL_DEVICES) {
> >> -          dev_err(d->dev, "minor must be between 0 and %d!\n",
> >> -                                          MAX_IRCTL_DEVICES - 1);
> >> -          return -EBADRQC;
> >> -  }
> >> -
> >>    if (d->code_length < 1 || d->code_length > (BUFLEN * 8)) {
> >>            dev_err(d->dev, "code length must be less than %d bits\n",
> >>                                                            BUFLEN * 8);
> >> @@ -156,49 +170,30 @@ int lirc_register_driver(struct lirc_driver *d)
> >>            return -EBADRQC;
> >>    }
> >>  
> >> -  mutex_lock(&lirc_dev_lock);
> >> -
> >> -  minor = d->minor;
> >> +  d->name[sizeof(d->name)-1] = '\0';
> >> +  if (d->features == 0)
> >> +          d->features = LIRC_CAN_REC_LIRCCODE;
> >>  
> >> -  if (minor < 0) {
> >> -          /* find first free slot for driver */
> >> -          for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++)
> >> -                  if (!irctls[minor])
> >> -                          break;
> >> -          if (minor == MAX_IRCTL_DEVICES) {
> >> -                  dev_err(d->dev, "no free slots for drivers!\n");
> >> -                  err = -ENOMEM;
> >> -                  goto out_lock;
> >> -          }
> >> -  } else if (irctls[minor]) {
> >> -          dev_err(d->dev, "minor (%d) just registered!\n", minor);
> >> -          err = -EBUSY;
> >> -          goto out_lock;
> >> -  }
> >> +  minor = ida_simple_get(&lirc_ida, 0, LIRC_MAX_DEVICES, GFP_KERNEL);
> >> +  if (minor < 0)
> >> +          return minor;
> >> +  d->minor = minor;
> >>  
> >>    ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
> >>    if (!ir) {
> >>            err = -ENOMEM;
> >> -          goto out_lock;
> >> +          goto out_minor;
> >>    }
> >>  
> >> -  mutex_init(&ir->irctl_lock);
> >> -  irctls[minor] = ir;
> >> -  d->minor = minor;
> >> -
> >> -  /* some safety check 8-) */
> >> -  d->name[sizeof(d->name)-1] = '\0';
> >> -
> >> -  if (d->features == 0)
> >> -          d->features = LIRC_CAN_REC_LIRCCODE;
> >> +  mutex_init(&ir->mutex);
> >>  
> >>    ir->d = *d;
> >
> >Here we copy lirc_driver.
> >
> >>  
> >>    if (LIRC_CAN_REC(d->features)) {
> >> -          err = lirc_allocate_buffer(irctls[minor]);
> >> +          err = lirc_allocate_buffer(ir);
> >>            if (err) {
> >>                    kfree(ir);
> >> -                  goto out_lock;
> >> +                  goto out_minor;
> >>            }
> >>            d->rbuf = ir->buf;
> >>    }
> >> @@ -218,107 +213,82 @@ int lirc_register_driver(struct lirc_driver *d)
> >>    if (err)
> >>            goto out_free_dev;
> >>  
> >> -  ir->attached = 1;
> >> -
> >>    err = device_add(&ir->dev);
> >>    if (err)
> >>            goto out_cdev;
> >>  
> >> -  mutex_unlock(&lirc_dev_lock);
> >> -
> >>    dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
> >>             ir->d.name, ir->d.minor);
> >>  
> >> -  return minor;
> >> +  d->lirc_internal = ir;
> >
> >And now a store a pointer to the copy in the original lirc_driver. 
> >
> >It would be much better to not copy lirc_driver and the lirc_internal
> >member would be unnecessary.
> 
> I know, but this is a more minimal fix. The struct copy is already in
> the current code and I'd prefer removing it in a separate patch once the
> use of lirc_driver has been vetted.

Your patch adds the lirc_internal pointer to work around this, which makes
the code even worse than it is.


Sean

> 
> >> +  return 0;
> >>  
> >>  out_cdev:
> >>    cdev_del(&ir->cdev);
> >>  out_free_dev:
> >>    put_device(&ir->dev);
> >> -out_lock:
> >> -  mutex_unlock(&lirc_dev_lock);
> >> +out_minor:
> >> +  ida_simple_remove(&lirc_ida, minor);
> >>  
> >>    return err;
> >>  }
> >>  EXPORT_SYMBOL(lirc_register_driver);
> >>  
> >> -int lirc_unregister_driver(int minor)
> >> +/**
> >> + * lirc_unregister_driver() - remove the lirc device for a given driver
> >> + * @d: the &struct lirc_driver to unregister
> >> + *
> >> + * This function unregisters the lirc device for a given &lirc_driver.
> >> + */
> >> +void
> >> +lirc_unregister_driver(struct lirc_driver *d)
> >>  {
> >>    struct irctl *ir;
> >>  
> >> -  if (minor < 0 || minor >= MAX_IRCTL_DEVICES) {
> >> -          pr_err("minor (%d) must be between 0 and %d!\n",
> >> -                                  minor, MAX_IRCTL_DEVICES - 1);
> >> -          return -EBADRQC;
> >> -  }
> >> +  if (!d->lirc_internal)
> >> +          return;
> >>  
> >> -  ir = irctls[minor];
> >> -  if (!ir) {
> >> -          pr_err("failed to get irctl\n");
> >> -          return -ENOENT;
> >> -  }
> >> +  ir = d->lirc_internal;
> >
> >This is done to find a our copy again.
> >
> >> -  mutex_lock(&lirc_dev_lock);
> >> -
> >> -  if (ir->d.minor != minor) {
> >> -          dev_err(ir->d.dev, "lirc_dev: minor %d device not registered\n",
> >> -                                                                  minor);
> >> -          mutex_unlock(&lirc_dev_lock);
> >> -          return -ENOENT;
> >> -  }
> >> +  mutex_lock(&ir->mutex);
> >>  
> >>    dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
> >>            ir->d.name, ir->d.minor);
> >>  
> >> -  ir->attached = 0;
> >> -  if (ir->open) {
> >> +  ir->dead = true;
> >> +  if (ir->users) {
> >>            dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
> >>                    ir->d.name, ir->d.minor);
> >>            wake_up_interruptible(&ir->buf->wait_poll);
> >>    }
> >>  
> >> -  mutex_unlock(&lirc_dev_lock);
> >> +  mutex_unlock(&ir->mutex);
> >>  
> >>    device_del(&ir->dev);
> >>    cdev_del(&ir->cdev);
> >> -  put_device(&ir->dev);
> >> -
> >> -  return 0;
> >> +  ida_simple_remove(&lirc_ida, d->minor);
> >> +  d->minor = -1;
> >> +  d->lirc_internal = NULL;
> >>  }
> >>  EXPORT_SYMBOL(lirc_unregister_driver);
> >>  
> >>  int lirc_dev_fop_open(struct inode *inode, struct file *file)
> >>  {
> >> -  struct irctl *ir;
> >> +  struct irctl *ir = container_of(inode->i_cdev, struct irctl, cdev);
> >>    int retval;
> >>  
> >> -  if (iminor(inode) >= MAX_IRCTL_DEVICES) {
> >> -          pr_err("open result for %d is -ENODEV\n", iminor(inode));
> >> -          return -ENODEV;
> >> -  }
> >> -
> >> -  if (mutex_lock_interruptible(&lirc_dev_lock))
> >> -          return -ERESTARTSYS;
> >> +  mutex_lock(&ir->mutex);
> >>  
> >> -  ir = irctls[iminor(inode)];
> >> -  mutex_unlock(&lirc_dev_lock);
> >> -
> >> -  if (!ir) {
> >> -          retval = -ENODEV;
> >> +  if (ir->users) {
> >> +          retval = -EBUSY;
> >> +          mutex_unlock(&ir->mutex);
> >>            goto error;
> >>    }
> >> +  ir->users++;
> >>  
> >> -  dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
> >> -
> >> -  if (ir->d.minor == NOPLUG) {
> >> -          retval = -ENODEV;
> >> -          goto error;
> >> -  }
> >> +  mutex_unlock(&ir->mutex);
> >>  
> >> -  if (ir->open) {
> >> -          retval = -EBUSY;
> >> -          goto error;
> >> -  }
> >> +  dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
> >>  
> >>    if (ir->d.rdev) {
> >>            retval = rc_open(ir->d.rdev);
> >> @@ -329,8 +299,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file 
> >> *file)
> >>    if (ir->buf)
> >>            lirc_buffer_clear(ir->buf);
> >>  
> >> -  ir->open++;
> >> -
> >> +  file->private_data = ir;
> >>    nonseekable_open(inode, file);
> >>  
> >>    return 0;
> >> @@ -342,22 +311,14 @@ EXPORT_SYMBOL(lirc_dev_fop_open);
> >>  
> >>  int lirc_dev_fop_close(struct inode *inode, struct file *file)
> >>  {
> >> -  struct irctl *ir = irctls[iminor(inode)];
> >> -  int ret;
> >> -
> >> -  if (!ir) {
> >> -          pr_err("called with invalid irctl\n");
> >> -          return -EINVAL;
> >> -  }
> >> +  struct irctl *ir = file->private_data;
> >>  
> >> -  ret = mutex_lock_killable(&lirc_dev_lock);
> >> -  WARN_ON(ret);
> >> +  mutex_lock(&ir->mutex);
> >>  
> >>    rc_close(ir->d.rdev);
> >> +  ir->users--;
> >>  
> >> -  ir->open--;
> >> -  if (!ret)
> >> -          mutex_unlock(&lirc_dev_lock);
> >> +  mutex_unlock(&ir->mutex);
> >>  
> >>    return 0;
> >>  }
> >> @@ -365,26 +326,21 @@ EXPORT_SYMBOL(lirc_dev_fop_close);
> >>  
> >>  unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
> >>  {
> >> -  struct irctl *ir = irctls[iminor(file_inode(file))];
> >> +  struct irctl *ir = file->private_data;
> >>    unsigned int ret;
> >>  
> >> -  if (!ir) {
> >> -          pr_err("called with invalid irctl\n");
> >> -          return POLLERR;
> >> -  }
> >> -
> >> -  if (!ir->attached)
> >> +  if (ir->dead)
> >>            return POLLHUP | POLLERR;
> >>  
> >> -  if (ir->buf) {
> >> -          poll_wait(file, &ir->buf->wait_poll, wait);
> >> +  if (!ir->buf)
> >> +          return POLLERR;
> >> +
> >> +  poll_wait(file, &ir->buf->wait_poll, wait);
> >>  
> >> -          if (lirc_buffer_empty(ir->buf))
> >> -                  ret = 0;
> >> -          else
> >> -                  ret = POLLIN | POLLRDNORM;
> >> -  } else
> >> -          ret = POLLERR;
> >> +  if (lirc_buffer_empty(ir->buf))
> >> +          ret = 0;
> >> +  else
> >> +          ret = POLLIN | POLLRDNORM;
> >>  
> >>    dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n",
> >>            ir->d.name, ir->d.minor, ret);
> >> @@ -395,25 +351,20 @@ EXPORT_SYMBOL(lirc_dev_fop_poll);
> >>  
> >>  long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned 
> >> long arg)
> >>  {
> >> +  struct irctl *ir = file->private_data;
> >>    __u32 mode;
> >>    int result = 0;
> >> -  struct irctl *ir = irctls[iminor(file_inode(file))];
> >> -
> >> -  if (!ir) {
> >> -          pr_err("no irctl found!\n");
> >> -          return -ENODEV;
> >> -  }
> >>  
> >>    dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
> >>            ir->d.name, ir->d.minor, cmd);
> >>  
> >> -  if (ir->d.minor == NOPLUG || !ir->attached) {
> >> +  if (ir->dead) {
> >>            dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
> >>                    ir->d.name, ir->d.minor);
> >>            return -ENODEV;
> >>    }
> >>  
> >> -  mutex_lock(&ir->irctl_lock);
> >> +  mutex_lock(&ir->mutex);
> >>  
> >>    switch (cmd) {
> >>    case LIRC_GET_FEATURES:
> >> @@ -468,7 +419,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned 
> >> int cmd, unsigned long arg)
> >>            result = -ENOTTY;
> >>    }
> >>  
> >> -  mutex_unlock(&ir->irctl_lock);
> >> +  mutex_unlock(&ir->mutex);
> >>  
> >>    return result;
> >>  }
> >> @@ -479,16 +430,11 @@ ssize_t lirc_dev_fop_read(struct file *file,
> >>                      size_t length,
> >>                      loff_t *ppos)
> >>  {
> >> -  struct irctl *ir = irctls[iminor(file_inode(file))];
> >> +  struct irctl *ir = file->private_data;
> >>    unsigned char *buf;
> >>    int ret = 0, written = 0;
> >>    DECLARE_WAITQUEUE(wait, current);
> >>  
> >> -  if (!ir) {
> >> -          pr_err("called with invalid irctl\n");
> >> -          return -ENODEV;
> >> -  }
> >> -
> >>    if (!LIRC_CAN_REC(ir->d.features))
> >>            return -EINVAL;
> >>  
> >> @@ -498,11 +444,12 @@ ssize_t lirc_dev_fop_read(struct file *file,
> >>    if (!buf)
> >>            return -ENOMEM;
> >>  
> >> -  if (mutex_lock_interruptible(&ir->irctl_lock)) {
> >> +  if (mutex_lock_interruptible(&ir->mutex)) {
> >>            ret = -ERESTARTSYS;
> >>            goto out_unlocked;
> >>    }
> >> -  if (!ir->attached) {
> >> +
> >> +  if (ir->dead) {
> >>            ret = -ENODEV;
> >>            goto out_locked;
> >>    }
> >> @@ -541,18 +488,18 @@ ssize_t lirc_dev_fop_read(struct file *file,
> >>                            break;
> >>                    }
> >>  
> >> -                  mutex_unlock(&ir->irctl_lock);
> >> +                  mutex_unlock(&ir->mutex);
> >>                    set_current_state(TASK_INTERRUPTIBLE);
> >>                    schedule();
> >>                    set_current_state(TASK_RUNNING);
> >>  
> >> -                  if (mutex_lock_interruptible(&ir->irctl_lock)) {
> >> +                  if (mutex_lock_interruptible(&ir->mutex)) {
> >>                            ret = -ERESTARTSYS;
> >>                            remove_wait_queue(&ir->buf->wait_poll, &wait);
> >>                            goto out_unlocked;
> >>                    }
> >>  
> >> -                  if (!ir->attached) {
> >> +                  if (ir->dead) {
> >>                            ret = -ENODEV;
> >>                            goto out_locked;
> >>                    }
> >> @@ -570,7 +517,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
> >>    remove_wait_queue(&ir->buf->wait_poll, &wait);
> >>  
> >>  out_locked:
> >> -  mutex_unlock(&ir->irctl_lock);
> >> +  mutex_unlock(&ir->mutex);
> >>  
> >>  out_unlocked:
> >>    kfree(buf);
> >> @@ -581,7 +528,8 @@ EXPORT_SYMBOL(lirc_dev_fop_read);
> >>  
> >>  void *lirc_get_pdata(struct file *file)
> >>  {
> >> -  return irctls[iminor(file_inode(file))]->d.data;
> >> +  struct irctl *ir = file->private_data;
> >> +  return ir->d.data;
> >>  }
> >>  EXPORT_SYMBOL(lirc_get_pdata);
> >>  
> >> @@ -596,7 +544,7 @@ static int __init lirc_dev_init(void)
> >>            return PTR_ERR(lirc_class);
> >>    }
> >>  
> >> -  retval = alloc_chrdev_region(&lirc_base_dev, 0, MAX_IRCTL_DEVICES,
> >> +  retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
> >>                                 IRCTL_DEV_NAME);
> >>    if (retval) {
> >>            class_destroy(lirc_class);
> >> @@ -613,7 +561,7 @@ static int __init lirc_dev_init(void)
> >>  static void __exit lirc_dev_exit(void)
> >>  {
> >>    class_destroy(lirc_class);
> >> -  unregister_chrdev_region(lirc_base_dev, MAX_IRCTL_DEVICES);
> >> +  unregister_chrdev_region(lirc_base_dev, LIRC_MAX_DEVICES);
> >>    pr_info("module unloaded\n");
> >>  }
> >>  
> >> diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
> >> b/drivers/staging/media/lirc/lirc_zilog.c
> >> index 59e05aa1bc55..ffb70dee4547 100644
> >> --- a/drivers/staging/media/lirc/lirc_zilog.c
> >> +++ b/drivers/staging/media/lirc/lirc_zilog.c
> >> @@ -183,11 +183,7 @@ static void release_ir_device(struct kref *ref)
> >>     * ir->open_count ==  0 - happens on final close()
> >>     * ir_lock, tx_ref_lock, rx_ref_lock, all released
> >>     */
> >> -  if (ir->l.minor >= 0) {
> >> -          lirc_unregister_driver(ir->l.minor);
> >> -          ir->l.minor = -1;
> >> -  }
> >> -
> >> +  lirc_unregister_driver(&ir->l);
> >>    if (kfifo_initialized(&ir->rbuf.fifo))
> >>            lirc_buffer_free(&ir->rbuf);
> >>    list_del(&ir->list);
> >> @@ -1382,7 +1378,6 @@ static const struct file_operations lirc_fops = {
> >>  
> >>  static struct lirc_driver lirc_template = {
> >>    .name           = "lirc_zilog",
> >> -  .minor          = -1,
> >>    .code_length    = 13,
> >>    .buffer_size    = BUFLEN / 2,
> >>    .chunk_size     = 2,
> >> @@ -1597,14 +1592,13 @@ static int ir_probe(struct i2c_client *client, 
> >> const struct i2c_device_id *id)
> >>    }
> >>  
> >>    /* register with lirc */
> >> -  ir->l.minor = lirc_register_driver(&ir->l);
> >> -  if (ir->l.minor < 0) {
> >> +  ret = lirc_register_driver(&ir->l);
> >> +  if (ret < 0) {
> >>            dev_err(tx->ir->l.dev,
> >> -                  "%s: lirc_register_driver() failed: %i\n",
> >> -                  __func__, ir->l.minor);
> >> -          ret = -EBADRQC;
> >> +                  "%s: lirc_register_driver failed: %i\n", __func__, ret);
> >>            goto out_put_xx;
> >>    }
> >> +
> >>    dev_info(ir->l.dev,
> >>             "IR unit on %s (i2c-%d) registered as lirc%d and ready\n",
> >>             adap->name, adap->nr, ir->l.minor);
> >> diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
> >> index 1f327e25a9be..f7629ff116a9 100644
> >> --- a/include/media/lirc_dev.h
> >> +++ b/include/media/lirc_dev.h
> >> @@ -9,7 +9,6 @@
> >>  #ifndef _LINUX_LIRC_DEV_H
> >>  #define _LINUX_LIRC_DEV_H
> >>  
> >> -#define MAX_IRCTL_DEVICES 8
> >>  #define BUFLEN            16
> >>  
> >>  #define mod(n, div) ((n) % (div))
> >> @@ -164,6 +163,8 @@ static inline unsigned int lirc_buffer_write(struct 
> >> lirc_buffer *buf,
> >>   *                        device.
> >>   *
> >>   * @owner:                the module owning this struct
> >> + *
> >> + * @lirc_internal:        lirc_dev bookkeeping data, don't touch.
> >
> >It's not a great name or comment :)
> >
> >Otherwise, it's a good idea.
> >
> >
> >Sean
> >
> >>   */
> >>  struct lirc_driver {
> >>    char name[40];
> >> @@ -182,19 +183,12 @@ struct lirc_driver {
> >>    const struct file_operations *fops;
> >>    struct device *dev;
> >>    struct module *owner;
> >> +  void *lirc_internal;
> >>  };
> >>  
> >> -/* following functions can be called ONLY from user context
> >> - *
> >> - * returns negative value on error or minor number
> >> - * of the registered device if success
> >> - * contents of the structure pointed by p is copied
> >> - */
> >>  extern int lirc_register_driver(struct lirc_driver *d);
> >>  
> >> -/* returns negative value on error or 0 if success
> >> -*/
> >> -extern int lirc_unregister_driver(int minor);
> >> +extern void lirc_unregister_driver(struct lirc_driver *d);
> >>  
> >>  /* Returns the private data stored in the lirc_driver
> >>   * associated with the given device file pointer.
> 
> -- 
> David Härdeman

Reply via email to