[alsa-devel] [PATCH 1/3] OMAP: McBSP: Use textual values in DMA operating mode sysfs files

[Eduardo Valentin]

Hello Jarkko,

On Sun, Aug 23, 2009 at 11:24:25AM +0200, Jarkko Nikula wrote:
> Use more descriptive than numerical value when showing and storing the
> McBSP DMA operating mode. Show function is using similar syntax than e.g.
> the led triggers so that all possible values for store function are
> printed but with current value surrounded with square brackets.

I liked the idea as I said before. But let's try to improve it a bit.

> 
> Signed-off-by: Jarkko Nikula <jhnikula at gmail.com>
> Cc: Peter Ujfalusi <peter.ujfalusi at nokia.com>
> Cc: Eduardo Valentin <eduardo.valentin at nokia.com>
> ---
>  arch/arm/plat-omap/mcbsp.c |   52 ++++++++++++++++++++++++-------------------
>  1 files changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> index b63a720..28e29b9 100644
> --- a/arch/arm/plat-omap/mcbsp.c
> +++ b/arch/arm/plat-omap/mcbsp.c
> @@ -1161,25 +1161,31 @@ static DEVICE_ATTR(prop, 0644, prop##_show, prop##_store);
>  THRESHOLD_PROP_BUILDER(max_tx_thres);
>  THRESHOLD_PROP_BUILDER(max_rx_thres);
>  
> +static const char *dma_op_modes[] = {
> +	"element", "threshold", "frame",
> +};
> +
>  static ssize_t dma_op_mode_show(struct device *dev,
>  			struct device_attribute *attr, char *buf)
>  {
>  	struct omap_mcbsp *mcbsp = dev_get_drvdata(dev);
> -	int dma_op_mode;
> +	int dma_op_mode, i = 0;
> +	ssize_t len = 0;
> +	const char * const *s;
>  
>  	spin_lock_irq(&mcbsp->lock);
>  	dma_op_mode = mcbsp->dma_op_mode;
>  	spin_unlock_irq(&mcbsp->lock);
>  
> -	return sprintf(buf, "current mode: %d\n"
> -			"possible mode values are:\n"
> -			"%d - %s\n"
> -			"%d - %s\n"
> -			"%d - %s\n",
> -			dma_op_mode,
> -			MCBSP_DMA_MODE_ELEMENT, "element mode",
> -			MCBSP_DMA_MODE_THRESHOLD, "threshold mode",
> -			MCBSP_DMA_MODE_FRAME, "frame mode");
> +	for (s = &dma_op_modes[i]; i < ARRAY_SIZE(dma_op_modes); s++, i++) {
> +		if (dma_op_mode == i)
> +			len += sprintf(buf + len, "[%s] ", *s);
> +		else
> +			len += sprintf(buf + len, "%s ", *s);
> +	}
> +	len += sprintf(len+buf, "\n");

Just a tiny thing, add spaces before and after + operator:
+	len += sprintf(len + buf, "\n");

> +
> +	return len;
>  }
>  
>  static ssize_t dma_op_mode_store(struct device *dev,
> @@ -1187,26 +1193,26 @@ static ssize_t dma_op_mode_store(struct device *dev,
>  				const char *buf, size_t size)
>  {
>  	struct omap_mcbsp *mcbsp = dev_get_drvdata(dev);
> -	unsigned long val;
> -	int status;
> +	const char * const *s;
> +	char *p;
> +	int len, i = 0;
>  
> -	status = strict_strtoul(buf, 0, &val);
> -	if (status)
> -		return status;
> +	p = memchr(buf, '\n', size);
> +	len = p ? p - buf : size;

I guess here we have two better options, please use one of these:
* strstrip
* sysfs_streq

>  
> -	spin_lock_irq(&mcbsp->lock);
> +	for (s = &dma_op_modes[i]; i < ARRAY_SIZE(dma_op_modes); s++, i++)
> +		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))

how about using strnicmp ??

> +			break;
>  
> +	if (i == ARRAY_SIZE(dma_op_modes))
> +		return -EINVAL;
> +
> +	spin_lock_irq(&mcbsp->lock);
>  	if (!mcbsp->free) {
>  		size = -EBUSY;
>  		goto unlock;
>  	}
> -
> -	if (val > MCBSP_DMA_MODE_FRAME || val < MCBSP_DMA_MODE_ELEMENT) {
> -		size = -EINVAL;
> -		goto unlock;
> -	}
> -
> -	mcbsp->dma_op_mode = val;
> +	mcbsp->dma_op_mode = i;
>  
>  unlock:
>  	spin_unlock_irq(&mcbsp->lock);
> -- 
> 1.6.3.3

-- 
Eduardo Valentin