Re: [PATCH v2 1/4] staging: greybus: audio: Avoid less than zero check for le32 variable

[Vaibhav Agarwal]

On Tue, Jan 17, 2017 at 10:56 PM, Mark Greer  wrote:
> Hi Vaibhav.
>
> On Tue, Jan 17, 2017 at 08:19:27PM +0530, Vaibhav Agarwal wrote:
>> mixer control->info call back function checks for -ve values to rebase
>> min and max values. However, le32 variable is used to fetch values from
>> GB module FW. Thus -ve value checking is not required. Fix this!!
>
> nit: Please spell out "negative" so it is as easy as possible for the
>  casual observer to understand what you're saying.

Will modify in v3.

>
>> Signed-off-by: Vaibhav Agarwal 
>> ---
>>  drivers/staging/greybus/audio_topology.c | 12 
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/audio_topology.c 
>> b/drivers/staging/greybus/audio_topology.c
>> index 3001a4971c06..a8b63a8b2bb0 100644
>> --- a/drivers/staging/greybus/audio_topology.c
>> +++ b/drivers/staging/greybus/audio_topology.c
>> @@ -361,8 +361,8 @@ static int gbcodec_mixer_dapm_ctl_info(struct 
>> snd_kcontrol *kcontrol,
>>   info = (struct gb_audio_ctl_elem_info *)data->info;
>>
>>   /* update uinfo */
>> - platform_max = info->value.integer.max;
>> - platform_min = info->value.integer.min;
>> + platform_max = le32_to_cpu(info->value.integer.max);
>> + platform_min = le32_to_cpu(info->value.integer.min);
>
> Should this piece be in patch 4/4?  It seems out of place in this patch.

Yes. It should be part of 4/4 only. Will update in v3

--
vaibhav
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/4] staging: greybus: audio: Avoid less than zero check for le32 variable

[Mark Greer]

Hi Vaibhav.

On Tue, Jan 17, 2017 at 08:19:27PM +0530, Vaibhav Agarwal wrote:
> mixer control->info call back function checks for -ve values to rebase
> min and max values. However, le32 variable is used to fetch values from
> GB module FW. Thus -ve value checking is not required. Fix this!!

nit: Please spell out "negative" so it is as easy as possible for the
 casual observer to understand what you're saying.

> Signed-off-by: Vaibhav Agarwal 
> ---
>  drivers/staging/greybus/audio_topology.c | 12 
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/greybus/audio_topology.c 
> b/drivers/staging/greybus/audio_topology.c
> index 3001a4971c06..a8b63a8b2bb0 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -361,8 +361,8 @@ static int gbcodec_mixer_dapm_ctl_info(struct 
> snd_kcontrol *kcontrol,
>   info = (struct gb_audio_ctl_elem_info *)data->info;
>  
>   /* update uinfo */
> - platform_max = info->value.integer.max;
> - platform_min = info->value.integer.min;
> + platform_max = le32_to_cpu(info->value.integer.max);
> + platform_min = le32_to_cpu(info->value.integer.min);

Should this piece be in patch 4/4?  It seems out of place in this patch.

>   if (platform_max == 1 &&
>   !strnstr(kcontrol->id.name, " Volume", NAME_SIZE))
> @@ -371,12 +371,8 @@ static int gbcodec_mixer_dapm_ctl_info(struct 
> snd_kcontrol *kcontrol,
>   uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
>  
>   uinfo->count = data->vcount;
> - uinfo->value.integer.min = 0;
> - if (info->value.integer.min < 0 &&
> - (uinfo->type == SNDRV_CTL_ELEM_TYPE_INTEGER))
> - uinfo->value.integer.max = platform_max - platform_min;
> - else
> - uinfo->value.integer.max = platform_max;
> + uinfo->value.integer.min = platform_min;
> + uinfo->value.integer.max = platform_max;

This part looks good.

Thanks,

Mark
--
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 1/4] staging: greybus: audio: Avoid less than zero check for le32 variable

[Vaibhav Agarwal]

mixer control->info call back function checks for -ve values to rebase
min and max values. However, le32 variable is used to fetch values from
GB module FW. Thus -ve value checking is not required. Fix this!!

Signed-off-by: Vaibhav Agarwal 
---
 drivers/staging/greybus/audio_topology.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/greybus/audio_topology.c 
b/drivers/staging/greybus/audio_topology.c
index 3001a4971c06..a8b63a8b2bb0 100644
--- a/drivers/staging/greybus/audio_topology.c
+++ b/drivers/staging/greybus/audio_topology.c
@@ -361,8 +361,8 @@ static int gbcodec_mixer_dapm_ctl_info(struct snd_kcontrol 
*kcontrol,
info = (struct gb_audio_ctl_elem_info *)data->info;
 
/* update uinfo */
-   platform_max = info->value.integer.max;
-   platform_min = info->value.integer.min;
+   platform_max = le32_to_cpu(info->value.integer.max);
+   platform_min = le32_to_cpu(info->value.integer.min);
 
if (platform_max == 1 &&
!strnstr(kcontrol->id.name, " Volume", NAME_SIZE))
@@ -371,12 +371,8 @@ static int gbcodec_mixer_dapm_ctl_info(struct snd_kcontrol 
*kcontrol,
uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
 
uinfo->count = data->vcount;
-   uinfo->value.integer.min = 0;
-   if (info->value.integer.min < 0 &&
-   (uinfo->type == SNDRV_CTL_ELEM_TYPE_INTEGER))
-   uinfo->value.integer.max = platform_max - platform_min;
-   else
-   uinfo->value.integer.max = platform_max;
+   uinfo->value.integer.min = platform_min;
+   uinfo->value.integer.max = platform_max;
 
return 0;
 }
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel