Re: [PATCH] rc-core: improve the lirc protocol reporting

[Mauro Carvalho Chehab <mchehab@osg.samsung.com>]

Hi David,

Em Wed, 22 Jul 2015 22:55:24 +0200
David Härdeman <david@hardeman.nu> escreveu:

> Commit 275ddb40bcf686d210d86c6718e42425a6a0bc76 removed the lirc
> "protocol" but kept backwards compatibility by always listing
> the protocol as present and enabled. This patch further improves
> the logic by only listing the protocol if the lirc module is loaded
> (or if lirc is builtin).

Makes sense, but see below.

> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/rc-main.c |   19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index ecaee02..3f0f71a 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -828,6 +828,23 @@ struct rc_filter_attribute {
>  		.mask = (_mask),					\
>  	}
>  
> +static bool lirc_is_present(void)
> +{
> +#if defined(CONFIG_LIRC_MODULE)
> +	struct module *lirc;
> +
> +	mutex_lock(&module_mutex);
> +	lirc = find_module("lirc_dev");
> +	mutex_unlock(&module_mutex);

I don't think it would be a good idea to play with the module mutex
lock here or calling find_module(). This is something that no other driver
does (well, except for FB DRM driver, that dynamically loads a module
if not found).

Perhaps we could use some simpler logic, like storing some value if lirc
got loaded or not (worse case, we might use a static atomic var at rc core).

> +
> +	return lirc ? true : false;
> +#elif defined(CONFIG_LIRC)
> +	return true;
> +#else
> +	return false;
> +#endif
> +}
> +
>  /**
>   * show_protocols() - shows the current/wakeup IR protocol(s)
>   * @device:	the device descriptor
> @@ -882,7 +899,7 @@ static ssize_t show_protocols(struct device *device,
>  			allowed &= ~proto_names[i].type;
>  	}
>  
> -	if (dev->driver_type == RC_DRIVER_IR_RAW)
> +	if (dev->driver_type == RC_DRIVER_IR_RAW && lirc_is_present())
>  		tmp += sprintf(tmp, "[lirc] ");
>  
>  	if (tmp != buf)
> 
> --
> 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