Re: [PATCH] rc-core: use an IDA rather than a bitmap

David Härdeman
2015-05-19T22:06:44Z

On Thu, May 14, 2015 at 05:29:29PM -0300, Mauro Carvalho Chehab wrote:
>Em Thu, 02 Apr 2015 12:18:55 +0200
>David Härdeman <david@hardeman.nu> escreveu:
>
>> This patch changes rc-core to use the kernel facilities that are already
>> available for handling unique numbers instead of rolling its own bitmap
>> stuff.
>>
>> Stefan, this should apply cleanly to the media git tree...could you test it?
>
>Patch looks good to me but...
>
>you forgot to add your SOB on it.

I didn't add it 'cause I wanted Stefan to test it first. Seems he's done
so...so I'll resubmit with my SOB and his Tested-by...

>
>> ---
>> drivers/media/rc/rc-ir-raw.c | 2 +-
>> drivers/media/rc/rc-main.c | 40 ++++++++++++++++++++--------------------
>> include/media/rc-core.h | 4 ++--
>> 3 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
>> index b732ac6..ad26052 100644
>> --- a/drivers/media/rc/rc-ir-raw.c
>> +++ b/drivers/media/rc/rc-ir-raw.c
>> @@ -271,7 +271,7 @@ int ir_raw_event_register(struct rc_dev *dev)
>>
>> spin_lock_init(&dev->raw->lock);
>> dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
>> - "rc%ld", dev->devno);
>> + "rc%u", dev->minor);
>>
>> if (IS_ERR(dev->raw->thread)) {
>> rc = PTR_ERR(dev->raw->thread);
>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>> index f8c5e47..d068c4e 100644
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -18,17 +18,15 @@
>> #include <linux/input.h>
>> #include <linux/leds.h>
>> #include <linux/slab.h>
>> +#include <linux/idr.h>
>> #include <linux/device.h>
>> #include <linux/module.h>
>> #include "rc-core-priv.h"
>>
>> -/* Bitmap to store allocated device numbers from 0 to IRRCV_NUM_DEVICES - 1 */
>> -#define IRRCV_NUM_DEVICES 256
>> -static DECLARE_BITMAP(ir_core_dev_number, IRRCV_NUM_DEVICES);
>> -
>> /* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */
>> #define IR_TAB_MIN_SIZE 256
>> #define IR_TAB_MAX_SIZE 8192
>> +#define RC_DEV_MAX 256
>>
>> /* FIXME: IR_KEYPRESS_TIMEOUT should be protocol specific */
>> #define IR_KEYPRESS_TIMEOUT 250
>> @@ -38,6 +36,9 @@ static LIST_HEAD(rc_map_list);
>> static DEFINE_SPINLOCK(rc_map_lock);
>> static struct led_trigger *led_feedback;
>>
>> +/* Used to keep track of rc devices */
>> +static DEFINE_IDA(rc_ida);
>> +
>> static struct rc_map_list *seek_rc_map(const char *name)
>> {
>> struct rc_map_list *map = NULL;
>> @@ -1312,7 +1313,9 @@ int rc_register_device(struct rc_dev *dev)
>> static bool raw_init = false; /* raw decoders loaded? */
>> struct rc_map *rc_map;
>> const char *path;
>> - int rc, devno, attr = 0;
>> + int attr = 0;
>> + int minor;
>> + int rc;
>>
>> if (!dev || !dev->map_name)
>> return -EINVAL;
>> @@ -1332,13 +1335,13 @@ int rc_register_device(struct rc_dev *dev)
>> if (dev->close)
>> dev->input_dev->close = ir_close;
>>
>> - do {
>> - devno = find_first_zero_bit(ir_core_dev_number,
>> - IRRCV_NUM_DEVICES);
>> - /* No free device slots */
>> - if (devno >= IRRCV_NUM_DEVICES)
>> - return -ENOMEM;
>> - } while (test_and_set_bit(devno, ir_core_dev_number));
>> + minor = ida_simple_get(&rc_ida, 0, RC_DEV_MAX, GFP_KERNEL);
>> + if (minor < 0)
>> + return minor;
>> +
>> + dev->minor = minor;
>> + dev_set_name(&dev->dev, "rc%u", dev->minor);
>> + dev_set_drvdata(&dev->dev, dev);
>>
>> dev->dev.groups = dev->sysfs_groups;
>> dev->sysfs_groups[attr++] = &rc_dev_protocol_attr_grp;
>> @@ -1358,9 +1361,6 @@ int rc_register_device(struct rc_dev *dev)
>> */
>> mutex_lock(&dev->lock);
>>
>> - dev->devno = devno;
>> - dev_set_name(&dev->dev, "rc%ld", dev->devno);
>> - dev_set_drvdata(&dev->dev, dev);
>> rc = device_add(&dev->dev);
>> if (rc)
>> goto out_unlock;
>> @@ -1433,8 +1433,8 @@ int rc_register_device(struct rc_dev *dev)
>>
>> mutex_unlock(&dev->lock);
>>
>> - IR_dprintk(1, "Registered rc%ld (driver: %s, remote: %s, mode %s)\n",
>> - dev->devno,
>> + IR_dprintk(1, "Registered rc%u (driver: %s, remote: %s, mode %s)\n",
>> + dev->minor,
>> dev->driver_name ? dev->driver_name : "unknown",
>> rc_map->name ? rc_map->name : "unknown",
>> dev->driver_type == RC_DRIVER_IR_RAW ? "raw" : "cooked");
>> @@ -1453,7 +1453,7 @@ out_dev:
>> device_del(&dev->dev);
>> out_unlock:
>> mutex_unlock(&dev->lock);
>> - clear_bit(dev->devno, ir_core_dev_number);
>> + ida_simple_remove(&rc_ida, minor);
>> return rc;
>> }
>> EXPORT_SYMBOL_GPL(rc_register_device);
>> @@ -1465,8 +1465,6 @@ void rc_unregister_device(struct rc_dev *dev)
>>
>> del_timer_sync(&dev->timer_keyup);
>>
>> - clear_bit(dev->devno, ir_core_dev_number);
>> -
>> if (dev->driver_type == RC_DRIVER_IR_RAW)
>> ir_raw_event_unregister(dev);
>>
>> @@ -1479,6 +1477,8 @@ void rc_unregister_device(struct rc_dev *dev)
>>
>> device_del(&dev->dev);
>>
>> + ida_simple_remove(&rc_ida, dev->minor);
>> +
>> rc_free_device(dev);
>> }
>>
>> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
>> index 2c7fbca..6b4400c 100644
>> --- a/include/media/rc-core.h
>> +++ b/include/media/rc-core.h
>> @@ -69,7 +69,7 @@ enum rc_filter_type {
>> * @rc_map: current scan/key table
>> * @lock: used to ensure we've filled in all protocol details before
>> * anyone can call show_protocols or store_protocols
>> - * @devno: unique remote control device number
>> + * @minor: unique minor remote control device number
>> * @raw: additional data for raw pulse/space devices
>> * @input_dev: the input child device used to communicate events to userspace
>> * @driver_type: specifies if protocol decoding is done in hardware or software
>> @@ -129,7 +129,7 @@ struct rc_dev {
>> const char *map_name;
>> struct rc_map rc_map;
>> struct mutex lock;
>> - unsigned long devno;
>> + unsigned int minor;
>> struct ir_raw_event_ctrl *raw;
>> struct input_dev *input_dev;
>> enum rc_driver_type driver_type;
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

--
David Härdeman