* [PATCH 1/3] [PATCH] staging: greybus: fix warnings about endianness detected by sparse
@ 2020-09-24 10:20 ` Coiby Xu
0 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-24 10:20 UTC (permalink / raw)
To: devel
Cc: Alex Elder, open list, Vaibhav Agarwal, Greg Kroah-Hartman,
Johan Hovold, Mark Greer, moderated list:GREYBUS SUBSYSTEM,
Dan Carpenter
This patch fix the following warnings from sparse,
$ make C=2 drivers/staging/greybus/
drivers/staging/greybus/audio_module.c:222:25: warning: incorrect type in assignment (different base types)
drivers/staging/greybus/audio_module.c:222:25: expected restricted __le16 [usertype] data_cport
drivers/staging/greybus/audio_module.c:222:25: got unsigned short [usertype] intf_cport_id
drivers/staging/greybus/audio_topology.c:460:40: warning: restricted __le32 degrades to integer
drivers/staging/greybus/audio_topology.c:691:41: warning: incorrect type in assignment (different base types)
drivers/staging/greybus/audio_topology.c:691:41: expected unsigned int access
drivers/staging/greybus/audio_topology.c:691:41: got restricted __le32 [usertype] access
drivers/staging/greybus/audio_topology.c:746:44: warning: incorrect type in assignment (different base types)
drivers/staging/greybus/audio_topology.c:746:44: expected unsigned int
drivers/staging/greybus/audio_topology.c:746:44: got restricted __le32
drivers/staging/greybus/audio_topology.c:748:52: warning: incorrect type in assignment (different base types)
drivers/staging/greybus/audio_topology.c:748:52: expected unsigned int
drivers/staging/greybus/audio_topology.c:748:52: got restricted __le32
drivers/staging/greybus/audio_topology.c:802:42: warning: restricted __le32 degrades to integer
drivers/staging/greybus/audio_topology.c:805:50: warning: incorrect type in assignment (different base types)
drivers/staging/greybus/audio_topology.c:805:50: expected restricted __le32
drivers/staging/greybus/audio_topology.c:805:50: got unsigned int
drivers/staging/greybus/audio_topology.c:814:50: warning: restricted __le32 degrades to integer
drivers/staging/greybus/audio_topology.c:817:58: warning: incorrect type in assignment (different base types)
drivers/staging/greybus/audio_topology.c:817:58: expected restricted __le32
drivers/staging/greybus/audio_topology.c:817:58: got unsigned int
drivers/staging/greybus/audio_topology.c:889:25: warning: incorrect type in assignment (different base types)
drivers/staging/greybus/audio_topology.c:889:25: expected unsigned int access
drivers/staging/greybus/audio_topology.c:889:25: got restricted __le32 [usertype] access
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
drivers/staging/greybus/audio_module.c | 6 +++---
drivers/staging/greybus/audio_topology.c | 18 +++++++++---------
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/greybus/audio_module.c b/drivers/staging/greybus/audio_module.c
index 16f60256adb2..c52c4f361b90 100644
--- a/drivers/staging/greybus/audio_module.c
+++ b/drivers/staging/greybus/audio_module.c
@@ -219,7 +219,7 @@ static int gb_audio_add_data_connection(struct gbaudio_module_info *gbmodule,
greybus_set_drvdata(bundle, gbmodule);
dai->id = 0;
- dai->data_cport = connection->intf_cport_id;
+ dai->data_cport = cpu_to_le16(connection->intf_cport_id);
dai->connection = connection;
list_add(&dai->list, &gbmodule->data_list);
@@ -329,7 +329,7 @@ static int gb_audio_probe(struct gb_bundle *bundle,
if (ret) {
dev_err(dev,
"%d:Error while enabling %d:data connection\n",
- ret, dai->data_cport);
+ ret, le16_to_cpu(dai->data_cport));
goto disable_data_connection;
}
}
@@ -451,7 +451,7 @@ static int gb_audio_resume(struct device *dev)
if (ret) {
dev_err(dev,
"%d:Error while enabling %d:data connection\n",
- ret, dai->data_cport);
+ ret, le16_to_cpu(dai->data_cport));
return ret;
}
}
diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
index 83b38ae8908c..56bf1a4f95ad 100644
--- a/drivers/staging/greybus/audio_topology.c
+++ b/drivers/staging/greybus/audio_topology.c
@@ -466,7 +466,7 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
goto exit;
/* update ucontrol */
- if (gbvalue.value.integer_value[0] != val) {
+ if (gbvalue.value.integer_value[0] != cpu_to_le32(val)) {
for (wi = 0; wi < wlist->num_widgets; wi++) {
widget = wlist->widgets[wi];
snd_soc_dapm_mixer_update_power(widget->dapm, kcontrol,
@@ -689,7 +689,7 @@ static int gbaudio_tplg_create_kcontrol(struct gbaudio_module_info *gb,
return -ENOMEM;
ctldata->ctl_id = ctl->id;
ctldata->data_cport = le16_to_cpu(ctl->data_cport);
- ctldata->access = ctl->access;
+ ctldata->access = le32_to_cpu(ctl->access);
ctldata->vcount = ctl->count_values;
ctldata->info = &ctl->info;
*kctl = (struct snd_kcontrol_new)
@@ -744,10 +744,10 @@ static int gbcodec_enum_dapm_ctl_get(struct snd_kcontrol *kcontrol,
return ret;
}
- ucontrol->value.enumerated.item[0] = gbvalue.value.enumerated_item[0];
+ ucontrol->value.enumerated.item[0] = le32_to_cpu(gbvalue.value.enumerated_item[0]);
if (e->shift_l != e->shift_r)
ucontrol->value.enumerated.item[1] =
- gbvalue.value.enumerated_item[1];
+ le32_to_cpu(gbvalue.value.enumerated_item[1]);
return 0;
}
@@ -801,10 +801,10 @@ static int gbcodec_enum_dapm_ctl_put(struct snd_kcontrol *kcontrol,
mask = e->mask << e->shift_l;
if (gbvalue.value.enumerated_item[0] !=
- ucontrol->value.enumerated.item[0]) {
+ cpu_to_le32(ucontrol->value.enumerated.item[0])) {
change = 1;
gbvalue.value.enumerated_item[0] =
- ucontrol->value.enumerated.item[0];
+ cpu_to_le32(ucontrol->value.enumerated.item[0]);
}
if (e->shift_l != e->shift_r) {
@@ -813,10 +813,10 @@ static int gbcodec_enum_dapm_ctl_put(struct snd_kcontrol *kcontrol,
val |= ucontrol->value.enumerated.item[1] << e->shift_r;
mask |= e->mask << e->shift_r;
if (gbvalue.value.enumerated_item[1] !=
- ucontrol->value.enumerated.item[1]) {
+ cpu_to_le32(ucontrol->value.enumerated.item[1])) {
change = 1;
gbvalue.value.enumerated_item[1] =
- ucontrol->value.enumerated.item[1];
+ cpu_to_le32(ucontrol->value.enumerated.item[1]);
}
}
@@ -887,7 +887,7 @@ static int gbaudio_tplg_create_mixer_ctl(struct gbaudio_module_info *gb,
return -ENOMEM;
ctldata->ctl_id = ctl->id;
ctldata->data_cport = le16_to_cpu(ctl->data_cport);
- ctldata->access = ctl->access;
+ ctldata->access = le32_to_cpu(ctl->access);
ctldata->vcount = ctl->count_values;
ctldata->info = &ctl->info;
*kctl = (struct snd_kcontrol_new)
--
2.28.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 2/3] staging: greybus: codecs: use SNDRV_PCM_FMTBIT_S16_LE for format bitmask
2020-09-24 10:20 ` Coiby Xu
@ 2020-09-24 10:20 ` Coiby Xu
-1 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-24 10:20 UTC (permalink / raw)
To: devel
Cc: Vaibhav Agarwal, Mark Greer, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, moderated list:GREYBUS SUBSYSTEM, open list
snd_soc_pcm_stream.formats should use the bitmask SNDRV_PCM_FMTBIT_*
instead of the sequential integers SNDRV_PCM_FORMAT_* as explained by
commit e712bfca1ac1f63f622f87c2f33b57608f2a4d19
("ASoC: codecs: use SNDRV_PCM_FMTBIT_* for format bitmask").
Found by sparse,
$ make C=2 drivers/staging/greybus/
drivers/staging/greybus/audio_codec.c:691:36: warning: incorrect type in initializer (different base types)
drivers/staging/greybus/audio_codec.c:691:36: expected unsigned long long [usertype] formats
drivers/staging/greybus/audio_codec.c:691:36: got restricted snd_pcm_format_t [usertype]
drivers/staging/greybus/audio_codec.c:701:36: warning: incorrect type in initializer (different base types)
drivers/staging/greybus/audio_codec.c:701:36: expected unsigned long long [usertype] formats
drivers/staging/greybus/audio_codec.c:701:36: got restricted snd_pcm_format_t [usertype]
Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
drivers/staging/greybus/audio_codec.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index 74538f8c5fa4..494aa823e998 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -688,7 +688,7 @@ static struct snd_soc_dai_driver gbaudio_dai[] = {
.playback = {
.stream_name = "I2S 0 Playback",
.rates = SNDRV_PCM_RATE_48000,
- .formats = SNDRV_PCM_FORMAT_S16_LE,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE,
.rate_max = 48000,
.rate_min = 48000,
.channels_min = 1,
@@ -698,7 +698,7 @@ static struct snd_soc_dai_driver gbaudio_dai[] = {
.capture = {
.stream_name = "I2S 0 Capture",
.rates = SNDRV_PCM_RATE_48000,
- .formats = SNDRV_PCM_FORMAT_S16_LE,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE,
.rate_max = 48000,
.rate_min = 48000,
.channels_min = 1,
--
2.28.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 2/3] staging: greybus: codecs: use SNDRV_PCM_FMTBIT_S16_LE for format bitmask
@ 2020-09-24 10:20 ` Coiby Xu
0 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-24 10:20 UTC (permalink / raw)
To: devel
Cc: Alex Elder, Vaibhav Agarwal, Greg Kroah-Hartman, Johan Hovold,
Mark Greer, moderated list:GREYBUS SUBSYSTEM, open list
snd_soc_pcm_stream.formats should use the bitmask SNDRV_PCM_FMTBIT_*
instead of the sequential integers SNDRV_PCM_FORMAT_* as explained by
commit e712bfca1ac1f63f622f87c2f33b57608f2a4d19
("ASoC: codecs: use SNDRV_PCM_FMTBIT_* for format bitmask").
Found by sparse,
$ make C=2 drivers/staging/greybus/
drivers/staging/greybus/audio_codec.c:691:36: warning: incorrect type in initializer (different base types)
drivers/staging/greybus/audio_codec.c:691:36: expected unsigned long long [usertype] formats
drivers/staging/greybus/audio_codec.c:691:36: got restricted snd_pcm_format_t [usertype]
drivers/staging/greybus/audio_codec.c:701:36: warning: incorrect type in initializer (different base types)
drivers/staging/greybus/audio_codec.c:701:36: expected unsigned long long [usertype] formats
drivers/staging/greybus/audio_codec.c:701:36: got restricted snd_pcm_format_t [usertype]
Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
drivers/staging/greybus/audio_codec.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index 74538f8c5fa4..494aa823e998 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -688,7 +688,7 @@ static struct snd_soc_dai_driver gbaudio_dai[] = {
.playback = {
.stream_name = "I2S 0 Playback",
.rates = SNDRV_PCM_RATE_48000,
- .formats = SNDRV_PCM_FORMAT_S16_LE,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE,
.rate_max = 48000,
.rate_min = 48000,
.channels_min = 1,
@@ -698,7 +698,7 @@ static struct snd_soc_dai_driver gbaudio_dai[] = {
.capture = {
.stream_name = "I2S 0 Capture",
.rates = SNDRV_PCM_RATE_48000,
- .formats = SNDRV_PCM_FORMAT_S16_LE,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE,
.rate_max = 48000,
.rate_min = 48000,
.channels_min = 1,
--
2.28.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 2/3] staging: greybus: codecs: use SNDRV_PCM_FMTBIT_S16_LE for format bitmask
2020-09-24 10:20 ` Coiby Xu
@ 2020-09-24 12:54 ` Alex Elder
-1 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-24 12:54 UTC (permalink / raw)
To: Coiby Xu, devel
Cc: Alex Elder, Johan Hovold, moderated list:GREYBUS SUBSYSTEM, open list
On 9/24/20 5:20 AM, Coiby Xu wrote:
> snd_soc_pcm_stream.formats should use the bitmask SNDRV_PCM_FMTBIT_*
> instead of the sequential integers SNDRV_PCM_FORMAT_* as explained by
> commit e712bfca1ac1f63f622f87c2f33b57608f2a4d19
> ("ASoC: codecs: use SNDRV_PCM_FMTBIT_* for format bitmask").
>
> Found by sparse,
Looks good.
Reviewed-by: Alex Elder <elder@linaro.org>
>
> $ make C=2 drivers/staging/greybus/
> drivers/staging/greybus/audio_codec.c:691:36: warning: incorrect type in initializer (different base types)
> drivers/staging/greybus/audio_codec.c:691:36: expected unsigned long long [usertype] formats
> drivers/staging/greybus/audio_codec.c:691:36: got restricted snd_pcm_format_t [usertype]
> drivers/staging/greybus/audio_codec.c:701:36: warning: incorrect type in initializer (different base types)
> drivers/staging/greybus/audio_codec.c:701:36: expected unsigned long long [usertype] formats
> drivers/staging/greybus/audio_codec.c:701:36: got restricted snd_pcm_format_t [usertype]
>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
> drivers/staging/greybus/audio_codec.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
> index 74538f8c5fa4..494aa823e998 100644
> --- a/drivers/staging/greybus/audio_codec.c
> +++ b/drivers/staging/greybus/audio_codec.c
> @@ -688,7 +688,7 @@ static struct snd_soc_dai_driver gbaudio_dai[] = {
> .playback = {
> .stream_name = "I2S 0 Playback",
> .rates = SNDRV_PCM_RATE_48000,
> - .formats = SNDRV_PCM_FORMAT_S16_LE,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE,
> .rate_max = 48000,
> .rate_min = 48000,
> .channels_min = 1,
> @@ -698,7 +698,7 @@ static struct snd_soc_dai_driver gbaudio_dai[] = {
> .capture = {
> .stream_name = "I2S 0 Capture",
> .rates = SNDRV_PCM_RATE_48000,
> - .formats = SNDRV_PCM_FORMAT_S16_LE,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE,
> .rate_max = 48000,
> .rate_min = 48000,
> .channels_min = 1,
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 2/3] staging: greybus: codecs: use SNDRV_PCM_FMTBIT_S16_LE for format bitmask
@ 2020-09-24 12:54 ` Alex Elder
0 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-24 12:54 UTC (permalink / raw)
To: Coiby Xu, devel
Cc: moderated list:GREYBUS SUBSYSTEM, Alex Elder, Johan Hovold, open list
On 9/24/20 5:20 AM, Coiby Xu wrote:
> snd_soc_pcm_stream.formats should use the bitmask SNDRV_PCM_FMTBIT_*
> instead of the sequential integers SNDRV_PCM_FORMAT_* as explained by
> commit e712bfca1ac1f63f622f87c2f33b57608f2a4d19
> ("ASoC: codecs: use SNDRV_PCM_FMTBIT_* for format bitmask").
>
> Found by sparse,
Looks good.
Reviewed-by: Alex Elder <elder@linaro.org>
>
> $ make C=2 drivers/staging/greybus/
> drivers/staging/greybus/audio_codec.c:691:36: warning: incorrect type in initializer (different base types)
> drivers/staging/greybus/audio_codec.c:691:36: expected unsigned long long [usertype] formats
> drivers/staging/greybus/audio_codec.c:691:36: got restricted snd_pcm_format_t [usertype]
> drivers/staging/greybus/audio_codec.c:701:36: warning: incorrect type in initializer (different base types)
> drivers/staging/greybus/audio_codec.c:701:36: expected unsigned long long [usertype] formats
> drivers/staging/greybus/audio_codec.c:701:36: got restricted snd_pcm_format_t [usertype]
>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
> drivers/staging/greybus/audio_codec.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
> index 74538f8c5fa4..494aa823e998 100644
> --- a/drivers/staging/greybus/audio_codec.c
> +++ b/drivers/staging/greybus/audio_codec.c
> @@ -688,7 +688,7 @@ static struct snd_soc_dai_driver gbaudio_dai[] = {
> .playback = {
> .stream_name = "I2S 0 Playback",
> .rates = SNDRV_PCM_RATE_48000,
> - .formats = SNDRV_PCM_FORMAT_S16_LE,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE,
> .rate_max = 48000,
> .rate_min = 48000,
> .channels_min = 1,
> @@ -698,7 +698,7 @@ static struct snd_soc_dai_driver gbaudio_dai[] = {
> .capture = {
> .stream_name = "I2S 0 Capture",
> .rates = SNDRV_PCM_RATE_48000,
> - .formats = SNDRV_PCM_FORMAT_S16_LE,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE,
> .rate_max = 48000,
> .rate_min = 48000,
> .channels_min = 1,
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
2020-09-24 10:20 ` Coiby Xu
@ 2020-09-24 10:20 ` Coiby Xu
-1 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-24 10:20 UTC (permalink / raw)
To: devel
Cc: Vaibhav Agarwal, Mark Greer, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Jaroslav Kysela, Takashi Iwai,
moderated list:GREYBUS SUBSYSTEM, open list,
moderated list:SOUND
Use __8 to replace int and remove the unnecessary __bitwise type attribute.
Found by sparse,
$ make C=2 drivers/staging/greybus/
drivers/staging/greybus/audio_topology.c:185:24: warning: cast to restricted snd_ctl_elem_type_t
drivers/staging/greybus/audio_topology.c:679:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
drivers/staging/greybus/audio_topology.c:906:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
drivers/staging/greybus/audio_topology.c | 2 +-
include/uapi/sound/asound.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
index 56bf1a4f95ad..f6023ff390c2 100644
--- a/drivers/staging/greybus/audio_topology.c
+++ b/drivers/staging/greybus/audio_topology.c
@@ -182,7 +182,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
/* update uinfo */
uinfo->access = data->access;
uinfo->count = data->vcount;
- uinfo->type = (snd_ctl_elem_type_t)info->type;
+ uinfo->type = info->type;
switch (info->type) {
case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN:
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 535a7229e1d9..8e71a95644ab 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -950,7 +950,7 @@ struct snd_ctl_card_info {
unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */
};
-typedef int __bitwise snd_ctl_elem_type_t;
+typedef __u8 snd_ctl_elem_type_t;
#define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
#define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
#define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
@@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
#define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
#define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
-typedef int __bitwise snd_ctl_elem_iface_t;
+typedef __u8 snd_ctl_elem_iface_t;
#define SNDRV_CTL_ELEM_IFACE_CARD ((__force snd_ctl_elem_iface_t) 0) /* global control */
#define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force snd_ctl_elem_iface_t) 1) /* hardware dependent device */
#define SNDRV_CTL_ELEM_IFACE_MIXER ((__force snd_ctl_elem_iface_t) 2) /* virtual mixer device */
--
2.28.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-24 10:20 ` Coiby Xu
0 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-24 10:20 UTC (permalink / raw)
To: devel
Cc: moderated list:SOUND, Alex Elder, Vaibhav Agarwal, Mark Greer,
Greg Kroah-Hartman, Takashi Iwai, Johan Hovold, Jaroslav Kysela,
moderated list:GREYBUS SUBSYSTEM, open list
Use __8 to replace int and remove the unnecessary __bitwise type attribute.
Found by sparse,
$ make C=2 drivers/staging/greybus/
drivers/staging/greybus/audio_topology.c:185:24: warning: cast to restricted snd_ctl_elem_type_t
drivers/staging/greybus/audio_topology.c:679:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
drivers/staging/greybus/audio_topology.c:906:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
drivers/staging/greybus/audio_topology.c | 2 +-
include/uapi/sound/asound.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
index 56bf1a4f95ad..f6023ff390c2 100644
--- a/drivers/staging/greybus/audio_topology.c
+++ b/drivers/staging/greybus/audio_topology.c
@@ -182,7 +182,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
/* update uinfo */
uinfo->access = data->access;
uinfo->count = data->vcount;
- uinfo->type = (snd_ctl_elem_type_t)info->type;
+ uinfo->type = info->type;
switch (info->type) {
case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN:
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 535a7229e1d9..8e71a95644ab 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -950,7 +950,7 @@ struct snd_ctl_card_info {
unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */
};
-typedef int __bitwise snd_ctl_elem_type_t;
+typedef __u8 snd_ctl_elem_type_t;
#define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
#define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
#define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
@@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
#define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
#define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
-typedef int __bitwise snd_ctl_elem_iface_t;
+typedef __u8 snd_ctl_elem_iface_t;
#define SNDRV_CTL_ELEM_IFACE_CARD ((__force snd_ctl_elem_iface_t) 0) /* global control */
#define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force snd_ctl_elem_iface_t) 1) /* hardware dependent device */
#define SNDRV_CTL_ELEM_IFACE_MIXER ((__force snd_ctl_elem_iface_t) 2) /* virtual mixer device */
--
2.28.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* RE: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
2020-09-24 10:20 ` Coiby Xu
(?)
@ 2020-09-24 10:54 ` David Laight
-1 siblings, 0 replies; 38+ messages in thread
From: David Laight @ 2020-09-24 10:54 UTC (permalink / raw)
To: 'Coiby Xu', devel
Cc: Vaibhav Agarwal, Mark Greer, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Jaroslav Kysela, Takashi Iwai,
moderated list:GREYBUS SUBSYSTEM, open list,
moderated list:SOUND
From: Coiby Xu
> Sent: 24 September 2020 11:21
> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>
> Found by sparse,
...
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 535a7229e1d9..8e71a95644ab 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> unsigned char components[128]; /* card components / fine identification, delimited with one
> space (AC97 etc..) */
> };
>
> -typedef int __bitwise snd_ctl_elem_type_t;
> +typedef __u8 snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
WTF is all that about anyway??
What is wrong with:
#define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 38+ messages in thread* RE: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-24 10:54 ` David Laight
0 siblings, 0 replies; 38+ messages in thread
From: David Laight @ 2020-09-24 10:54 UTC (permalink / raw)
To: 'Coiby Xu', devel
Cc: moderated list:SOUND, Alex Elder, Vaibhav Agarwal, Mark Greer,
Greg Kroah-Hartman, Takashi Iwai, Johan Hovold,
moderated list:GREYBUS SUBSYSTEM, open list
From: Coiby Xu
> Sent: 24 September 2020 11:21
> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>
> Found by sparse,
...
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 535a7229e1d9..8e71a95644ab 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> unsigned char components[128]; /* card components / fine identification, delimited with one
> space (AC97 etc..) */
> };
>
> -typedef int __bitwise snd_ctl_elem_type_t;
> +typedef __u8 snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
WTF is all that about anyway??
What is wrong with:
#define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 38+ messages in thread* RE: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-24 10:54 ` David Laight
0 siblings, 0 replies; 38+ messages in thread
From: David Laight @ 2020-09-24 10:54 UTC (permalink / raw)
To: 'Coiby Xu', devel
Cc: moderated list:SOUND, Alex Elder, Vaibhav Agarwal, Mark Greer,
Greg Kroah-Hartman, Takashi Iwai, Johan Hovold, Jaroslav Kysela,
moderated list:GREYBUS SUBSYSTEM, open list
From: Coiby Xu
> Sent: 24 September 2020 11:21
> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>
> Found by sparse,
...
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 535a7229e1d9..8e71a95644ab 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> unsigned char components[128]; /* card components / fine identification, delimited with one
> space (AC97 etc..) */
> };
>
> -typedef int __bitwise snd_ctl_elem_type_t;
> +typedef __u8 snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
WTF is all that about anyway??
What is wrong with:
#define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
2020-09-24 10:54 ` David Laight
@ 2020-09-25 14:11 ` Coiby Xu
-1 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-25 14:11 UTC (permalink / raw)
To: David Laight
Cc: devel, Vaibhav Agarwal, Mark Greer, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Jaroslav Kysela, Takashi Iwai,
moderated list:GREYBUS SUBSYSTEM, open list,
moderated list:SOUND
On Thu, Sep 24, 2020 at 10:54:50AM +0000, David Laight wrote:
>From: Coiby Xu
>> Sent: 24 September 2020 11:21
>> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>>
>> Found by sparse,
>...
>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>> index 535a7229e1d9..8e71a95644ab 100644
>> --- a/include/uapi/sound/asound.h
>> +++ b/include/uapi/sound/asound.h
>> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>> unsigned char components[128]; /* card components / fine identification, delimited with one
>> space (AC97 etc..) */
>> };
>>
>> -typedef int __bitwise snd_ctl_elem_type_t;
>> +typedef __u8 snd_ctl_elem_type_t;
>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
>
>WTF is all that about anyway??
>What is wrong with:
>#define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
I'm sorry I don't quite understand you. Are you suggesting SNDRV_CTL_ELEM_TYPE_NONE
isn't needed in the first place?
>
> David
>
>-
>Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
>Registration No: 1397386 (Wales)
>
--
Best regards,
Coiby
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-25 14:11 ` Coiby Xu
0 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-25 14:11 UTC (permalink / raw)
To: David Laight
Cc: devel, moderated list:SOUND, Alex Elder, Vaibhav Agarwal,
Greg Kroah-Hartman, Takashi Iwai, Johan Hovold, Mark Greer,
moderated list:GREYBUS SUBSYSTEM, Jaroslav Kysela, open list
On Thu, Sep 24, 2020 at 10:54:50AM +0000, David Laight wrote:
>From: Coiby Xu
>> Sent: 24 September 2020 11:21
>> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>>
>> Found by sparse,
>...
>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>> index 535a7229e1d9..8e71a95644ab 100644
>> --- a/include/uapi/sound/asound.h
>> +++ b/include/uapi/sound/asound.h
>> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>> unsigned char components[128]; /* card components / fine identification, delimited with one
>> space (AC97 etc..) */
>> };
>>
>> -typedef int __bitwise snd_ctl_elem_type_t;
>> +typedef __u8 snd_ctl_elem_type_t;
>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
>
>WTF is all that about anyway??
>What is wrong with:
>#define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
I'm sorry I don't quite understand you. Are you suggesting SNDRV_CTL_ELEM_TYPE_NONE
isn't needed in the first place?
>
> David
>
>-
>Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
>Registration No: 1397386 (Wales)
>
--
Best regards,
Coiby
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
2020-09-25 14:11 ` Coiby Xu
(?)
@ 2020-09-25 16:02 ` Alex Elder
-1 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-25 16:02 UTC (permalink / raw)
To: Coiby Xu, David Laight
Cc: devel, moderated list:SOUND, Alex Elder, Takashi Iwai,
Johan Hovold, moderated list:GREYBUS SUBSYSTEM, Jaroslav Kysela,
open list
On 9/25/20 9:11 AM, Coiby Xu wrote:
> On Thu, Sep 24, 2020 at 10:54:50AM +0000, David Laight wrote:
>> From: Coiby Xu
>>> Sent: 24 September 2020 11:21
>>> Use __8 to replace int and remove the unnecessary __bitwise type
>>> attribute.
>>>
>>> Found by sparse,
>> ...
>>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>> index 535a7229e1d9..8e71a95644ab 100644
>>> --- a/include/uapi/sound/asound.h
>>> +++ b/include/uapi/sound/asound.h
>>> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>>> unsigned char components[128]; /* card components / fine
>>> identification, delimited with one
>>> space (AC97 etc..) */
>>> };
>>>
>>> -typedef int __bitwise snd_ctl_elem_type_t;
>>> +typedef __u8 snd_ctl_elem_type_t;
>>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force
>>> snd_ctl_elem_type_t) 0) /* invalid */
>>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force
>>> snd_ctl_elem_type_t) 1) /* boolean type */
>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force
>>> snd_ctl_elem_type_t) 2) /* integer type */
>>
>> WTF is all that about anyway??
>> What is wrong with:
>> #define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
>
> I'm sorry I don't quite understand you. Are you suggesting
> SNDRV_CTL_ELEM_TYPE_NONE
> isn't needed in the first place?
I think David is asking why it's defined the way it is,
and I'd guess it's to have the compiler issue an error
if you attempt to assign one of these values to a variable
or field of the wrong type.
No, you should not attempt to change this.
-Alex
>> David
>>
>> -
>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
>> MK1 1PT, UK
>> Registration No: 1397386 (Wales)
>>
>
> --
> Best regards,
> Coiby
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-25 16:02 ` Alex Elder
0 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-25 16:02 UTC (permalink / raw)
To: Coiby Xu, David Laight
Cc: devel, moderated list:SOUND, Alex Elder, open list, Takashi Iwai,
Johan Hovold, moderated list:GREYBUS SUBSYSTEM
On 9/25/20 9:11 AM, Coiby Xu wrote:
> On Thu, Sep 24, 2020 at 10:54:50AM +0000, David Laight wrote:
>> From: Coiby Xu
>>> Sent: 24 September 2020 11:21
>>> Use __8 to replace int and remove the unnecessary __bitwise type
>>> attribute.
>>>
>>> Found by sparse,
>> ...
>>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>> index 535a7229e1d9..8e71a95644ab 100644
>>> --- a/include/uapi/sound/asound.h
>>> +++ b/include/uapi/sound/asound.h
>>> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>>> unsigned char components[128]; /* card components / fine
>>> identification, delimited with one
>>> space (AC97 etc..) */
>>> };
>>>
>>> -typedef int __bitwise snd_ctl_elem_type_t;
>>> +typedef __u8 snd_ctl_elem_type_t;
>>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force
>>> snd_ctl_elem_type_t) 0) /* invalid */
>>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force
>>> snd_ctl_elem_type_t) 1) /* boolean type */
>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force
>>> snd_ctl_elem_type_t) 2) /* integer type */
>>
>> WTF is all that about anyway??
>> What is wrong with:
>> #define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
>
> I'm sorry I don't quite understand you. Are you suggesting
> SNDRV_CTL_ELEM_TYPE_NONE
> isn't needed in the first place?
I think David is asking why it's defined the way it is,
and I'd guess it's to have the compiler issue an error
if you attempt to assign one of these values to a variable
or field of the wrong type.
No, you should not attempt to change this.
-Alex
>> David
>>
>> -
>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
>> MK1 1PT, UK
>> Registration No: 1397386 (Wales)
>>
>
> --
> Best regards,
> Coiby
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-25 16:02 ` Alex Elder
0 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-25 16:02 UTC (permalink / raw)
To: Coiby Xu, David Laight
Cc: devel, moderated list:SOUND, Alex Elder, open list, Takashi Iwai,
Johan Hovold, moderated list:GREYBUS SUBSYSTEM, Jaroslav Kysela
On 9/25/20 9:11 AM, Coiby Xu wrote:
> On Thu, Sep 24, 2020 at 10:54:50AM +0000, David Laight wrote:
>> From: Coiby Xu
>>> Sent: 24 September 2020 11:21
>>> Use __8 to replace int and remove the unnecessary __bitwise type
>>> attribute.
>>>
>>> Found by sparse,
>> ...
>>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>> index 535a7229e1d9..8e71a95644ab 100644
>>> --- a/include/uapi/sound/asound.h
>>> +++ b/include/uapi/sound/asound.h
>>> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>>> unsigned char components[128]; /* card components / fine
>>> identification, delimited with one
>>> space (AC97 etc..) */
>>> };
>>>
>>> -typedef int __bitwise snd_ctl_elem_type_t;
>>> +typedef __u8 snd_ctl_elem_type_t;
>>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force
>>> snd_ctl_elem_type_t) 0) /* invalid */
>>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force
>>> snd_ctl_elem_type_t) 1) /* boolean type */
>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force
>>> snd_ctl_elem_type_t) 2) /* integer type */
>>
>> WTF is all that about anyway??
>> What is wrong with:
>> #define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
>
> I'm sorry I don't quite understand you. Are you suggesting
> SNDRV_CTL_ELEM_TYPE_NONE
> isn't needed in the first place?
I think David is asking why it's defined the way it is,
and I'd guess it's to have the compiler issue an error
if you attempt to assign one of these values to a variable
or field of the wrong type.
No, you should not attempt to change this.
-Alex
>> David
>>
>> -
>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
>> MK1 1PT, UK
>> Registration No: 1397386 (Wales)
>>
>
> --
> Best regards,
> Coiby
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
2020-09-25 16:02 ` Alex Elder
@ 2020-09-26 4:01 ` Coiby Xu
-1 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-26 4:01 UTC (permalink / raw)
To: Alex Elder
Cc: David Laight, devel, moderated list:SOUND, Alex Elder,
Takashi Iwai, Johan Hovold, moderated list:GREYBUS SUBSYSTEM,
Jaroslav Kysela, open list
On Fri, Sep 25, 2020 at 11:02:23AM -0500, Alex Elder wrote:
>On 9/25/20 9:11 AM, Coiby Xu wrote:
>>On Thu, Sep 24, 2020 at 10:54:50AM +0000, David Laight wrote:
>>>From: Coiby Xu
>>>>Sent: 24 September 2020 11:21
>>>>Use __8 to replace int and remove the unnecessary __bitwise type
>>>>attribute.
>>>>
>>>>Found by sparse,
>>>...
>>>>diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>>>index 535a7229e1d9..8e71a95644ab 100644
>>>>--- a/include/uapi/sound/asound.h
>>>>+++ b/include/uapi/sound/asound.h
>>>>@@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>>>> unsigned char components[128]; /* card components / fine
>>>>identification, delimited with one
>>>>space (AC97 etc..) */
>>>> };
>>>>
>>>>-typedef int __bitwise snd_ctl_elem_type_t;
>>>>+typedef __u8 snd_ctl_elem_type_t;
>>>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force
>>>>snd_ctl_elem_type_t) 0) /* invalid */
>>>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force
>>>>snd_ctl_elem_type_t) 1) /* boolean type */
>>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force
>>>>snd_ctl_elem_type_t) 2) /* integer type */
>>>
>>>WTF is all that about anyway??
>>>What is wrong with:
>>>#define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
>>
>>I'm sorry I don't quite understand you. Are you suggesting
>>SNDRV_CTL_ELEM_TYPE_NONE
>>isn't needed in the first place?
>
>I think David is asking why it's defined the way it is,
>and I'd guess it's to have the compiler issue an error
>if you attempt to assign one of these values to a variable
>or field of the wrong type.
>
>No, you should not attempt to change this.
Thank you for the explanation!
>
> -Alex
>>> David
>>>
>>>-
>>>Registered Address Lakeside, Bramley Road, Mount Farm, Milton
>>>Keynes, MK1 1PT, UK
>>>Registration No: 1397386 (Wales)
>>>
>>
>>--
>>Best regards,
>>Coiby
>>_______________________________________________
>>greybus-dev mailing list
>>greybus-dev@lists.linaro.org
>>https://lists.linaro.org/mailman/listinfo/greybus-dev
>
--
Best regards,
Coiby
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-26 4:01 ` Coiby Xu
0 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-26 4:01 UTC (permalink / raw)
To: Alex Elder
Cc: devel, moderated list:SOUND, Alex Elder, open list, Takashi Iwai,
Johan Hovold, moderated list:GREYBUS SUBSYSTEM, David Laight,
Jaroslav Kysela
On Fri, Sep 25, 2020 at 11:02:23AM -0500, Alex Elder wrote:
>On 9/25/20 9:11 AM, Coiby Xu wrote:
>>On Thu, Sep 24, 2020 at 10:54:50AM +0000, David Laight wrote:
>>>From: Coiby Xu
>>>>Sent: 24 September 2020 11:21
>>>>Use __8 to replace int and remove the unnecessary __bitwise type
>>>>attribute.
>>>>
>>>>Found by sparse,
>>>...
>>>>diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>>>index 535a7229e1d9..8e71a95644ab 100644
>>>>--- a/include/uapi/sound/asound.h
>>>>+++ b/include/uapi/sound/asound.h
>>>>@@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>>>> unsigned char components[128]; /* card components / fine
>>>>identification, delimited with one
>>>>space (AC97 etc..) */
>>>> };
>>>>
>>>>-typedef int __bitwise snd_ctl_elem_type_t;
>>>>+typedef __u8 snd_ctl_elem_type_t;
>>>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force
>>>>snd_ctl_elem_type_t) 0) /* invalid */
>>>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force
>>>>snd_ctl_elem_type_t) 1) /* boolean type */
>>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force
>>>>snd_ctl_elem_type_t) 2) /* integer type */
>>>
>>>WTF is all that about anyway??
>>>What is wrong with:
>>>#define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
>>
>>I'm sorry I don't quite understand you. Are you suggesting
>>SNDRV_CTL_ELEM_TYPE_NONE
>>isn't needed in the first place?
>
>I think David is asking why it's defined the way it is,
>and I'd guess it's to have the compiler issue an error
>if you attempt to assign one of these values to a variable
>or field of the wrong type.
>
>No, you should not attempt to change this.
Thank you for the explanation!
>
> -Alex
>>> David
>>>
>>>-
>>>Registered Address Lakeside, Bramley Road, Mount Farm, Milton
>>>Keynes, MK1 1PT, UK
>>>Registration No: 1397386 (Wales)
>>>
>>
>>--
>>Best regards,
>>Coiby
>>_______________________________________________
>>greybus-dev mailing list
>>greybus-dev@lists.linaro.org
>>https://lists.linaro.org/mailman/listinfo/greybus-dev
>
--
Best regards,
Coiby
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
2020-09-25 14:11 ` Coiby Xu
(?)
@ 2020-09-26 10:11 ` David Laight
-1 siblings, 0 replies; 38+ messages in thread
From: David Laight @ 2020-09-26 10:11 UTC (permalink / raw)
To: 'Coiby Xu'
Cc: devel, Vaibhav Agarwal, Mark Greer, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Jaroslav Kysela, Takashi Iwai,
moderated list:GREYBUS SUBSYSTEM, open list,
moderated list:SOUND
From: Coiby Xu
> Sent: 25 September 2020 15:11
>
> On Thu, Sep 24, 2020 at 10:54:50AM +0000, David Laight wrote:
> >From: Coiby Xu
> >> Sent: 24 September 2020 11:21
> >> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
> >>
> >> Found by sparse,
> >...
> >> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> >> index 535a7229e1d9..8e71a95644ab 100644
> >> --- a/include/uapi/sound/asound.h
> >> +++ b/include/uapi/sound/asound.h
> >> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> >> unsigned char components[128]; /* card components / fine identification, delimited with one
> >> space (AC97 etc..) */
> >> };
> >>
> >> -typedef int __bitwise snd_ctl_elem_type_t;
> >> +typedef __u8 snd_ctl_elem_type_t;
> >> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> >> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> >> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
> >
> >WTF is all that about anyway??
> >What is wrong with:
> >#define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
>
> I'm sorry I don't quite understand you. Are you suggesting SNDRV_CTL_ELEM_TYPE_NONE
> isn't needed in the first place?
No, just remove all the casts from the constants.
Are the types even used anywhere else?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 38+ messages in thread* RE: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-26 10:11 ` David Laight
0 siblings, 0 replies; 38+ messages in thread
From: David Laight @ 2020-09-26 10:11 UTC (permalink / raw)
To: 'Coiby Xu'
Cc: devel, moderated list:SOUND, Alex Elder, Vaibhav Agarwal,
Greg Kroah-Hartman, Takashi Iwai, Johan Hovold, Mark Greer,
moderated list:GREYBUS SUBSYSTEM, open list
From: Coiby Xu
> Sent: 25 September 2020 15:11
>
> On Thu, Sep 24, 2020 at 10:54:50AM +0000, David Laight wrote:
> >From: Coiby Xu
> >> Sent: 24 September 2020 11:21
> >> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
> >>
> >> Found by sparse,
> >...
> >> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> >> index 535a7229e1d9..8e71a95644ab 100644
> >> --- a/include/uapi/sound/asound.h
> >> +++ b/include/uapi/sound/asound.h
> >> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> >> unsigned char components[128]; /* card components / fine identification, delimited with one
> >> space (AC97 etc..) */
> >> };
> >>
> >> -typedef int __bitwise snd_ctl_elem_type_t;
> >> +typedef __u8 snd_ctl_elem_type_t;
> >> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> >> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> >> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
> >
> >WTF is all that about anyway??
> >What is wrong with:
> >#define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
>
> I'm sorry I don't quite understand you. Are you suggesting SNDRV_CTL_ELEM_TYPE_NONE
> isn't needed in the first place?
No, just remove all the casts from the constants.
Are the types even used anywhere else?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 38+ messages in thread* RE: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-26 10:11 ` David Laight
0 siblings, 0 replies; 38+ messages in thread
From: David Laight @ 2020-09-26 10:11 UTC (permalink / raw)
To: 'Coiby Xu'
Cc: devel, moderated list:SOUND, Alex Elder, Vaibhav Agarwal,
Greg Kroah-Hartman, Takashi Iwai, Johan Hovold, Mark Greer,
moderated list:GREYBUS SUBSYSTEM, Jaroslav Kysela, open list
From: Coiby Xu
> Sent: 25 September 2020 15:11
>
> On Thu, Sep 24, 2020 at 10:54:50AM +0000, David Laight wrote:
> >From: Coiby Xu
> >> Sent: 24 September 2020 11:21
> >> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
> >>
> >> Found by sparse,
> >...
> >> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> >> index 535a7229e1d9..8e71a95644ab 100644
> >> --- a/include/uapi/sound/asound.h
> >> +++ b/include/uapi/sound/asound.h
> >> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> >> unsigned char components[128]; /* card components / fine identification, delimited with one
> >> space (AC97 etc..) */
> >> };
> >>
> >> -typedef int __bitwise snd_ctl_elem_type_t;
> >> +typedef __u8 snd_ctl_elem_type_t;
> >> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> >> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> >> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
> >
> >WTF is all that about anyway??
> >What is wrong with:
> >#define SNDRV_CTL_ELEM_TYPE_NONE 0u /* invalid */
>
> I'm sorry I don't quite understand you. Are you suggesting SNDRV_CTL_ELEM_TYPE_NONE
> isn't needed in the first place?
No, just remove all the casts from the constants.
Are the types even used anywhere else?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
2020-09-24 10:20 ` Coiby Xu
(?)
@ 2020-09-24 11:00 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 38+ messages in thread
From: Greg Kroah-Hartman @ 2020-09-24 11:00 UTC (permalink / raw)
To: Coiby Xu
Cc: devel, moderated list:SOUND, Alex Elder, Vaibhav Agarwal,
Mark Greer, Takashi Iwai, Johan Hovold, Jaroslav Kysela,
moderated list:GREYBUS SUBSYSTEM, open list
On Thu, Sep 24, 2020 at 06:20:39PM +0800, Coiby Xu wrote:
> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>
> Found by sparse,
>
> $ make C=2 drivers/staging/greybus/
> drivers/staging/greybus/audio_topology.c:185:24: warning: cast to restricted snd_ctl_elem_type_t
> drivers/staging/greybus/audio_topology.c:679:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
> drivers/staging/greybus/audio_topology.c:906:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
> drivers/staging/greybus/audio_topology.c | 2 +-
> include/uapi/sound/asound.h | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 56bf1a4f95ad..f6023ff390c2 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -182,7 +182,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
> /* update uinfo */
> uinfo->access = data->access;
> uinfo->count = data->vcount;
> - uinfo->type = (snd_ctl_elem_type_t)info->type;
> + uinfo->type = info->type;
>
> switch (info->type) {
> case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN:
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 535a7229e1d9..8e71a95644ab 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */
> };
>
> -typedef int __bitwise snd_ctl_elem_type_t;
> +typedef __u8 snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
> @@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>
> -typedef int __bitwise snd_ctl_elem_iface_t;
> +typedef __u8 snd_ctl_elem_iface_t;
> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force snd_ctl_elem_iface_t) 0) /* global control */
> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force snd_ctl_elem_iface_t) 1) /* hardware dependent device */
> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force snd_ctl_elem_iface_t) 2) /* virtual mixer device */
I can't take a uapi sound header file patch along with a driver change,
these need to be independant.
And this is a userspace api, you just changed the size of an externally
facing variable, are you _SURE_ that is ok to do?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-24 11:00 ` Greg Kroah-Hartman
0 siblings, 0 replies; 38+ messages in thread
From: Greg Kroah-Hartman @ 2020-09-24 11:00 UTC (permalink / raw)
To: Coiby Xu
Cc: devel, moderated list:SOUND, Alex Elder, Vaibhav Agarwal,
open list, Takashi Iwai, Mark Greer,
moderated list:GREYBUS SUBSYSTEM, Johan Hovold
On Thu, Sep 24, 2020 at 06:20:39PM +0800, Coiby Xu wrote:
> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>
> Found by sparse,
>
> $ make C=2 drivers/staging/greybus/
> drivers/staging/greybus/audio_topology.c:185:24: warning: cast to restricted snd_ctl_elem_type_t
> drivers/staging/greybus/audio_topology.c:679:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
> drivers/staging/greybus/audio_topology.c:906:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
> drivers/staging/greybus/audio_topology.c | 2 +-
> include/uapi/sound/asound.h | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 56bf1a4f95ad..f6023ff390c2 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -182,7 +182,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
> /* update uinfo */
> uinfo->access = data->access;
> uinfo->count = data->vcount;
> - uinfo->type = (snd_ctl_elem_type_t)info->type;
> + uinfo->type = info->type;
>
> switch (info->type) {
> case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN:
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 535a7229e1d9..8e71a95644ab 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */
> };
>
> -typedef int __bitwise snd_ctl_elem_type_t;
> +typedef __u8 snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
> @@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>
> -typedef int __bitwise snd_ctl_elem_iface_t;
> +typedef __u8 snd_ctl_elem_iface_t;
> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force snd_ctl_elem_iface_t) 0) /* global control */
> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force snd_ctl_elem_iface_t) 1) /* hardware dependent device */
> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force snd_ctl_elem_iface_t) 2) /* virtual mixer device */
I can't take a uapi sound header file patch along with a driver change,
these need to be independant.
And this is a userspace api, you just changed the size of an externally
facing variable, are you _SURE_ that is ok to do?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-24 11:00 ` Greg Kroah-Hartman
0 siblings, 0 replies; 38+ messages in thread
From: Greg Kroah-Hartman @ 2020-09-24 11:00 UTC (permalink / raw)
To: Coiby Xu
Cc: devel, moderated list:SOUND, Alex Elder, Vaibhav Agarwal,
open list, Takashi Iwai, Mark Greer,
moderated list:GREYBUS SUBSYSTEM, Johan Hovold, Jaroslav Kysela
On Thu, Sep 24, 2020 at 06:20:39PM +0800, Coiby Xu wrote:
> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>
> Found by sparse,
>
> $ make C=2 drivers/staging/greybus/
> drivers/staging/greybus/audio_topology.c:185:24: warning: cast to restricted snd_ctl_elem_type_t
> drivers/staging/greybus/audio_topology.c:679:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
> drivers/staging/greybus/audio_topology.c:906:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
> drivers/staging/greybus/audio_topology.c | 2 +-
> include/uapi/sound/asound.h | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 56bf1a4f95ad..f6023ff390c2 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -182,7 +182,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
> /* update uinfo */
> uinfo->access = data->access;
> uinfo->count = data->vcount;
> - uinfo->type = (snd_ctl_elem_type_t)info->type;
> + uinfo->type = info->type;
>
> switch (info->type) {
> case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN:
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 535a7229e1d9..8e71a95644ab 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */
> };
>
> -typedef int __bitwise snd_ctl_elem_type_t;
> +typedef __u8 snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
> @@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>
> -typedef int __bitwise snd_ctl_elem_iface_t;
> +typedef __u8 snd_ctl_elem_iface_t;
> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force snd_ctl_elem_iface_t) 0) /* global control */
> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force snd_ctl_elem_iface_t) 1) /* hardware dependent device */
> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force snd_ctl_elem_iface_t) 2) /* virtual mixer device */
I can't take a uapi sound header file patch along with a driver change,
these need to be independant.
And this is a userspace api, you just changed the size of an externally
facing variable, are you _SURE_ that is ok to do?
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
2020-09-24 11:00 ` Greg Kroah-Hartman
@ 2020-09-25 14:07 ` Coiby Xu
-1 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-25 14:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Alex Elder
Cc: devel, moderated list:SOUND, Vaibhav Agarwal, Mark Greer,
Takashi Iwai, Johan Hovold, Jaroslav Kysela,
moderated list:GREYBUS SUBSYSTEM, open list
On Thu, Sep 24, 2020 at 01:00:57PM +0200, Greg Kroah-Hartman wrote:
>On Thu, Sep 24, 2020 at 06:20:39PM +0800, Coiby Xu wrote:
>> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>>
>> Found by sparse,
>>
>> $ make C=2 drivers/staging/greybus/
>> drivers/staging/greybus/audio_topology.c:185:24: warning: cast to restricted snd_ctl_elem_type_t
>> drivers/staging/greybus/audio_topology.c:679:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
>> drivers/staging/greybus/audio_topology.c:906:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>> drivers/staging/greybus/audio_topology.c | 2 +-
>> include/uapi/sound/asound.h | 4 ++--
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
>> index 56bf1a4f95ad..f6023ff390c2 100644
>> --- a/drivers/staging/greybus/audio_topology.c
>> +++ b/drivers/staging/greybus/audio_topology.c
>> @@ -182,7 +182,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
>> /* update uinfo */
>> uinfo->access = data->access;
>> uinfo->count = data->vcount;
>> - uinfo->type = (snd_ctl_elem_type_t)info->type;
>> + uinfo->type = info->type;
>>
>> switch (info->type) {
>> case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN:
>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>> index 535a7229e1d9..8e71a95644ab 100644
>> --- a/include/uapi/sound/asound.h
>> +++ b/include/uapi/sound/asound.h
>> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>> unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */
>> };
>>
>> -typedef int __bitwise snd_ctl_elem_type_t;
>> +typedef __u8 snd_ctl_elem_type_t;
>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
>> @@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
>> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
>> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>>
>> -typedef int __bitwise snd_ctl_elem_iface_t;
>> +typedef __u8 snd_ctl_elem_iface_t;
>> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force snd_ctl_elem_iface_t) 0) /* global control */
>> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force snd_ctl_elem_iface_t) 1) /* hardware dependent device */
>> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force snd_ctl_elem_iface_t) 2) /* virtual mixer device */
>
>I can't take a uapi sound header file patch along with a driver change,
>these need to be independant.
Thank you and Alex for reminding me this is a change of public header!
>
>And this is a userspace api, you just changed the size of an externally
>facing variable, are you _SURE_ that is ok to do?
The reasons I make this change are, 1) using int to represent 7 enumeration
values seems to be overkill; 2) using one type could avoid worries
about byte order.
I examine one userspace example (libalsa-intf/alsa_mixer.c [1]). This
change won't cause compiling breakage. But I don't the experience to
imagine any other bad consequence.
Another way to avoid userspace API change is to let
"struct gb_audio_ctl_elem_info" use snd_ctl_elem_iface_t type which may
be more elegant than using "__force" as suggested by Alex,
$ git diff
diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
index aeb8f9243545..7f6753ec7ef7 100644
--- a/include/linux/greybus/greybus_protocols.h
+++ b/include/linux/greybus/greybus_protocols.h
@@ -8,6 +8,7 @@
#define __GREYBUS_PROTOCOLS_H
#include <linux/types.h>
+#include <sound/asound.h>
/* Fixed IDs for control/svc protocols */
@@ -1997,7 +1998,7 @@ struct gb_audio_enumerated {
} __packed;
struct gb_audio_ctl_elem_info { /* See snd_ctl_elem_info in Linux source */
- __u8 type; /* GB_AUDIO_CTL_ELEM_TYPE_* */
+ snd_ctl_elem_type_t type; /* GB_AUDIO_CTL_ELEM_TYPE_* */
__le16 dimen[4];
union {
struct gb_audio_integer integer
My only concern is if greybus authors have any special reason to not include
sound/asound.h in the first place and re-define things like SNDRV_CTL_ELEM_TYPE_*,
/* See SNDRV_CTL_ELEM_TYPE_* in Linux source */
#define GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN 0x01
#define GB_AUDIO_CTL_ELEM_TYPE_INTEGER 0x02
/* See SNDRV_CTL_ELEM_IFACE_* in Linux source */
#define GB_AUDIO_CTL_ELEM_IFACE_CARD 0x00
...
/* SNDRV_CTL_ELEM_ACCESS_* in Linux source */
#define GB_AUDIO_ACCESS_READ BIT(0)
...
[1] https://gitlab.com/Codeaurora/platform_hardware_qcom_audio/-/blob/aee6032826ec7c8b6edda404422fda0ef40ec2db/libalsa-intf/alsa_mixer.c
alsa_mixer.c
>
>thanks,
>
>greg k-h
--
Best regards,
Coiby
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-25 14:07 ` Coiby Xu
0 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-25 14:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Alex Elder
Cc: devel, moderated list:SOUND, Vaibhav Agarwal, open list,
Takashi Iwai, Mark Greer, moderated list:GREYBUS SUBSYSTEM,
Johan Hovold, Jaroslav Kysela
On Thu, Sep 24, 2020 at 01:00:57PM +0200, Greg Kroah-Hartman wrote:
>On Thu, Sep 24, 2020 at 06:20:39PM +0800, Coiby Xu wrote:
>> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>>
>> Found by sparse,
>>
>> $ make C=2 drivers/staging/greybus/
>> drivers/staging/greybus/audio_topology.c:185:24: warning: cast to restricted snd_ctl_elem_type_t
>> drivers/staging/greybus/audio_topology.c:679:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
>> drivers/staging/greybus/audio_topology.c:906:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>> drivers/staging/greybus/audio_topology.c | 2 +-
>> include/uapi/sound/asound.h | 4 ++--
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
>> index 56bf1a4f95ad..f6023ff390c2 100644
>> --- a/drivers/staging/greybus/audio_topology.c
>> +++ b/drivers/staging/greybus/audio_topology.c
>> @@ -182,7 +182,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
>> /* update uinfo */
>> uinfo->access = data->access;
>> uinfo->count = data->vcount;
>> - uinfo->type = (snd_ctl_elem_type_t)info->type;
>> + uinfo->type = info->type;
>>
>> switch (info->type) {
>> case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN:
>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>> index 535a7229e1d9..8e71a95644ab 100644
>> --- a/include/uapi/sound/asound.h
>> +++ b/include/uapi/sound/asound.h
>> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>> unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */
>> };
>>
>> -typedef int __bitwise snd_ctl_elem_type_t;
>> +typedef __u8 snd_ctl_elem_type_t;
>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
>> @@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
>> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
>> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>>
>> -typedef int __bitwise snd_ctl_elem_iface_t;
>> +typedef __u8 snd_ctl_elem_iface_t;
>> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force snd_ctl_elem_iface_t) 0) /* global control */
>> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force snd_ctl_elem_iface_t) 1) /* hardware dependent device */
>> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force snd_ctl_elem_iface_t) 2) /* virtual mixer device */
>
>I can't take a uapi sound header file patch along with a driver change,
>these need to be independant.
Thank you and Alex for reminding me this is a change of public header!
>
>And this is a userspace api, you just changed the size of an externally
>facing variable, are you _SURE_ that is ok to do?
The reasons I make this change are, 1) using int to represent 7 enumeration
values seems to be overkill; 2) using one type could avoid worries
about byte order.
I examine one userspace example (libalsa-intf/alsa_mixer.c [1]). This
change won't cause compiling breakage. But I don't the experience to
imagine any other bad consequence.
Another way to avoid userspace API change is to let
"struct gb_audio_ctl_elem_info" use snd_ctl_elem_iface_t type which may
be more elegant than using "__force" as suggested by Alex,
$ git diff
diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
index aeb8f9243545..7f6753ec7ef7 100644
--- a/include/linux/greybus/greybus_protocols.h
+++ b/include/linux/greybus/greybus_protocols.h
@@ -8,6 +8,7 @@
#define __GREYBUS_PROTOCOLS_H
#include <linux/types.h>
+#include <sound/asound.h>
/* Fixed IDs for control/svc protocols */
@@ -1997,7 +1998,7 @@ struct gb_audio_enumerated {
} __packed;
struct gb_audio_ctl_elem_info { /* See snd_ctl_elem_info in Linux source */
- __u8 type; /* GB_AUDIO_CTL_ELEM_TYPE_* */
+ snd_ctl_elem_type_t type; /* GB_AUDIO_CTL_ELEM_TYPE_* */
__le16 dimen[4];
union {
struct gb_audio_integer integer
My only concern is if greybus authors have any special reason to not include
sound/asound.h in the first place and re-define things like SNDRV_CTL_ELEM_TYPE_*,
/* See SNDRV_CTL_ELEM_TYPE_* in Linux source */
#define GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN 0x01
#define GB_AUDIO_CTL_ELEM_TYPE_INTEGER 0x02
/* See SNDRV_CTL_ELEM_IFACE_* in Linux source */
#define GB_AUDIO_CTL_ELEM_IFACE_CARD 0x00
...
/* SNDRV_CTL_ELEM_ACCESS_* in Linux source */
#define GB_AUDIO_ACCESS_READ BIT(0)
...
[1] https://gitlab.com/Codeaurora/platform_hardware_qcom_audio/-/blob/aee6032826ec7c8b6edda404422fda0ef40ec2db/libalsa-intf/alsa_mixer.c
alsa_mixer.c
>
>thanks,
>
>greg k-h
--
Best regards,
Coiby
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
2020-09-25 14:07 ` Coiby Xu
(?)
@ 2020-09-25 15:57 ` Alex Elder
-1 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-25 15:57 UTC (permalink / raw)
To: Coiby Xu, Greg Kroah-Hartman
Cc: devel, moderated list:SOUND, Vaibhav Agarwal, Mark Greer,
Takashi Iwai, Johan Hovold, Jaroslav Kysela,
moderated list:GREYBUS SUBSYSTEM, open list
On 9/25/20 9:07 AM, Coiby Xu wrote:
> On Thu, Sep 24, 2020 at 01:00:57PM +0200, Greg Kroah-Hartman wrote:
>> On Thu, Sep 24, 2020 at 06:20:39PM +0800, Coiby Xu wrote:
>>> Use __8 to replace int and remove the unnecessary __bitwise type
>>> attribute.
>>>
>>> Found by sparse,
. . .
>>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>> index 535a7229e1d9..8e71a95644ab 100644
>>> --- a/include/uapi/sound/asound.h
>>> +++ b/include/uapi/sound/asound.h
>>> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>>> unsigned char components[128]; /* card components / fine
>>> identification, delimited with one space (AC97 etc..) */
>>> };
>>>
>>> -typedef int __bitwise snd_ctl_elem_type_t;
>>> +typedef __u8 snd_ctl_elem_type_t;
>>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force
>>> snd_ctl_elem_type_t) 0) /* invalid */
>>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force
>>> snd_ctl_elem_type_t) 1) /* boolean type */
>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force
>>> snd_ctl_elem_type_t) 2) /* integer type */
>>> @@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force
>>> snd_ctl_elem_type_t) 6) /* 64-bit integer type */
>>> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>>>
>>> -typedef int __bitwise snd_ctl_elem_iface_t;
>>> +typedef __u8 snd_ctl_elem_iface_t;
>>> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force
>>> snd_ctl_elem_iface_t) 0) /* global control */
>>> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force
>>> snd_ctl_elem_iface_t) 1) /* hardware dependent device */
>>> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force
>>> snd_ctl_elem_iface_t) 2) /* virtual mixer device */
>>
>> I can't take a uapi sound header file patch along with a driver change,
>> these need to be independant.
>
> Thank you and Alex for reminding me this is a change of public header!
>>
>> And this is a userspace api, you just changed the size of an externally
>> facing variable, are you _SURE_ that is ok to do?
>
> The reasons I make this change are, 1) using int to represent 7 enumeration
> values seems to be overkill; 2) using one type could avoid worries
> about byte order.
(1) might be a valid suggestion, but the file you suggest
changing is part of the Linux user space API, which you
basically can't change.
I'm fairly certain the user space API is defined to have
CPU-local byte order (unless specified explicitly otherwise)
so that is not a concern.
> I examine one userspace example (libalsa-intf/alsa_mixer.c [1]). This
> change won't cause compiling breakage. But I don't the experience to
> imagine any other bad consequence.
As a rule, once the user space API has been established, it
stays with us forever. You can add to it, but modifying
(or removing) an existing interface is essentially forbidden.
> Another way to avoid userspace API change is to let
> "struct gb_audio_ctl_elem_info" use snd_ctl_elem_iface_t type which may
> be more elegant than using "__force" as suggested by Alex,
The Greybus definitions were explicitly defined to be
operating system independent. For that reason there are
(admittedly cumbersome) definitions of certain types and
values that essentially mimic those defined by Linux
exactly That way Linux (or another system using Greybus)
can change its internal values without changing the
Greybus API definition. (This addresses the concern you
mention below.)
What you are suggesting is not consistent with that
principle.
-Alex
> $ git diff
> diff --git a/include/linux/greybus/greybus_protocols.h
> b/include/linux/greybus/greybus_protocols.h
> index aeb8f9243545..7f6753ec7ef7 100644
> --- a/include/linux/greybus/greybus_protocols.h
> +++ b/include/linux/greybus/greybus_protocols.h
> @@ -8,6 +8,7 @@
> #define __GREYBUS_PROTOCOLS_H
>
> #include <linux/types.h>
> +#include <sound/asound.h>
>
> /* Fixed IDs for control/svc protocols */
>
> @@ -1997,7 +1998,7 @@ struct gb_audio_enumerated {
> } __packed;
>
> struct gb_audio_ctl_elem_info { /* See snd_ctl_elem_info in Linux
> source */
> - __u8 type; /* GB_AUDIO_CTL_ELEM_TYPE_* */
> + snd_ctl_elem_type_t type; /*
> GB_AUDIO_CTL_ELEM_TYPE_* */
> __le16 dimen[4];
> union {
> struct gb_audio_integer integer
>
> My only concern is if greybus authors have any special reason to not
> include
> sound/asound.h in the first place and re-define things like
> SNDRV_CTL_ELEM_TYPE_*,
>
> /* See SNDRV_CTL_ELEM_TYPE_* in Linux source */
> #define GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN 0x01
> #define GB_AUDIO_CTL_ELEM_TYPE_INTEGER 0x02
>
> /* See SNDRV_CTL_ELEM_IFACE_* in Linux source */
> #define GB_AUDIO_CTL_ELEM_IFACE_CARD 0x00
> ...
>
> /* SNDRV_CTL_ELEM_ACCESS_* in Linux source */
> #define GB_AUDIO_ACCESS_READ BIT(0)
> ...
>
> [1]
> https://gitlab.com/Codeaurora/platform_hardware_qcom_audio/-/blob/aee6032826ec7c8b6edda404422fda0ef40ec2db/libalsa-intf/alsa_mixer.c
>
> alsa_mixer.c
>>
>> thanks,
>>
>> greg k-h
>
> --
> Best regards,
> Coiby
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-25 15:57 ` Alex Elder
0 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-25 15:57 UTC (permalink / raw)
To: Coiby Xu, Greg Kroah-Hartman
Cc: devel, moderated list:SOUND, Vaibhav Agarwal, open list,
Takashi Iwai, Mark Greer, moderated list:GREYBUS SUBSYSTEM,
Johan Hovold
On 9/25/20 9:07 AM, Coiby Xu wrote:
> On Thu, Sep 24, 2020 at 01:00:57PM +0200, Greg Kroah-Hartman wrote:
>> On Thu, Sep 24, 2020 at 06:20:39PM +0800, Coiby Xu wrote:
>>> Use __8 to replace int and remove the unnecessary __bitwise type
>>> attribute.
>>>
>>> Found by sparse,
. . .
>>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>> index 535a7229e1d9..8e71a95644ab 100644
>>> --- a/include/uapi/sound/asound.h
>>> +++ b/include/uapi/sound/asound.h
>>> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>>> unsigned char components[128]; /* card components / fine
>>> identification, delimited with one space (AC97 etc..) */
>>> };
>>>
>>> -typedef int __bitwise snd_ctl_elem_type_t;
>>> +typedef __u8 snd_ctl_elem_type_t;
>>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force
>>> snd_ctl_elem_type_t) 0) /* invalid */
>>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force
>>> snd_ctl_elem_type_t) 1) /* boolean type */
>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force
>>> snd_ctl_elem_type_t) 2) /* integer type */
>>> @@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force
>>> snd_ctl_elem_type_t) 6) /* 64-bit integer type */
>>> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>>>
>>> -typedef int __bitwise snd_ctl_elem_iface_t;
>>> +typedef __u8 snd_ctl_elem_iface_t;
>>> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force
>>> snd_ctl_elem_iface_t) 0) /* global control */
>>> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force
>>> snd_ctl_elem_iface_t) 1) /* hardware dependent device */
>>> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force
>>> snd_ctl_elem_iface_t) 2) /* virtual mixer device */
>>
>> I can't take a uapi sound header file patch along with a driver change,
>> these need to be independant.
>
> Thank you and Alex for reminding me this is a change of public header!
>>
>> And this is a userspace api, you just changed the size of an externally
>> facing variable, are you _SURE_ that is ok to do?
>
> The reasons I make this change are, 1) using int to represent 7 enumeration
> values seems to be overkill; 2) using one type could avoid worries
> about byte order.
(1) might be a valid suggestion, but the file you suggest
changing is part of the Linux user space API, which you
basically can't change.
I'm fairly certain the user space API is defined to have
CPU-local byte order (unless specified explicitly otherwise)
so that is not a concern.
> I examine one userspace example (libalsa-intf/alsa_mixer.c [1]). This
> change won't cause compiling breakage. But I don't the experience to
> imagine any other bad consequence.
As a rule, once the user space API has been established, it
stays with us forever. You can add to it, but modifying
(or removing) an existing interface is essentially forbidden.
> Another way to avoid userspace API change is to let
> "struct gb_audio_ctl_elem_info" use snd_ctl_elem_iface_t type which may
> be more elegant than using "__force" as suggested by Alex,
The Greybus definitions were explicitly defined to be
operating system independent. For that reason there are
(admittedly cumbersome) definitions of certain types and
values that essentially mimic those defined by Linux
exactly That way Linux (or another system using Greybus)
can change its internal values without changing the
Greybus API definition. (This addresses the concern you
mention below.)
What you are suggesting is not consistent with that
principle.
-Alex
> $ git diff
> diff --git a/include/linux/greybus/greybus_protocols.h
> b/include/linux/greybus/greybus_protocols.h
> index aeb8f9243545..7f6753ec7ef7 100644
> --- a/include/linux/greybus/greybus_protocols.h
> +++ b/include/linux/greybus/greybus_protocols.h
> @@ -8,6 +8,7 @@
> #define __GREYBUS_PROTOCOLS_H
>
> #include <linux/types.h>
> +#include <sound/asound.h>
>
> /* Fixed IDs for control/svc protocols */
>
> @@ -1997,7 +1998,7 @@ struct gb_audio_enumerated {
> } __packed;
>
> struct gb_audio_ctl_elem_info { /* See snd_ctl_elem_info in Linux
> source */
> - __u8 type; /* GB_AUDIO_CTL_ELEM_TYPE_* */
> + snd_ctl_elem_type_t type; /*
> GB_AUDIO_CTL_ELEM_TYPE_* */
> __le16 dimen[4];
> union {
> struct gb_audio_integer integer
>
> My only concern is if greybus authors have any special reason to not
> include
> sound/asound.h in the first place and re-define things like
> SNDRV_CTL_ELEM_TYPE_*,
>
> /* See SNDRV_CTL_ELEM_TYPE_* in Linux source */
> #define GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN 0x01
> #define GB_AUDIO_CTL_ELEM_TYPE_INTEGER 0x02
>
> /* See SNDRV_CTL_ELEM_IFACE_* in Linux source */
> #define GB_AUDIO_CTL_ELEM_IFACE_CARD 0x00
> ...
>
> /* SNDRV_CTL_ELEM_ACCESS_* in Linux source */
> #define GB_AUDIO_ACCESS_READ BIT(0)
> ...
>
> [1]
> https://gitlab.com/Codeaurora/platform_hardware_qcom_audio/-/blob/aee6032826ec7c8b6edda404422fda0ef40ec2db/libalsa-intf/alsa_mixer.c
>
> alsa_mixer.c
>>
>> thanks,
>>
>> greg k-h
>
> --
> Best regards,
> Coiby
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-25 15:57 ` Alex Elder
0 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-25 15:57 UTC (permalink / raw)
To: Coiby Xu, Greg Kroah-Hartman
Cc: devel, moderated list:SOUND, Vaibhav Agarwal, open list,
Takashi Iwai, Mark Greer, moderated list:GREYBUS SUBSYSTEM,
Johan Hovold, Jaroslav Kysela
On 9/25/20 9:07 AM, Coiby Xu wrote:
> On Thu, Sep 24, 2020 at 01:00:57PM +0200, Greg Kroah-Hartman wrote:
>> On Thu, Sep 24, 2020 at 06:20:39PM +0800, Coiby Xu wrote:
>>> Use __8 to replace int and remove the unnecessary __bitwise type
>>> attribute.
>>>
>>> Found by sparse,
. . .
>>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>> index 535a7229e1d9..8e71a95644ab 100644
>>> --- a/include/uapi/sound/asound.h
>>> +++ b/include/uapi/sound/asound.h
>>> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>>> unsigned char components[128]; /* card components / fine
>>> identification, delimited with one space (AC97 etc..) */
>>> };
>>>
>>> -typedef int __bitwise snd_ctl_elem_type_t;
>>> +typedef __u8 snd_ctl_elem_type_t;
>>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force
>>> snd_ctl_elem_type_t) 0) /* invalid */
>>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force
>>> snd_ctl_elem_type_t) 1) /* boolean type */
>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force
>>> snd_ctl_elem_type_t) 2) /* integer type */
>>> @@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force
>>> snd_ctl_elem_type_t) 6) /* 64-bit integer type */
>>> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>>>
>>> -typedef int __bitwise snd_ctl_elem_iface_t;
>>> +typedef __u8 snd_ctl_elem_iface_t;
>>> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force
>>> snd_ctl_elem_iface_t) 0) /* global control */
>>> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force
>>> snd_ctl_elem_iface_t) 1) /* hardware dependent device */
>>> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force
>>> snd_ctl_elem_iface_t) 2) /* virtual mixer device */
>>
>> I can't take a uapi sound header file patch along with a driver change,
>> these need to be independant.
>
> Thank you and Alex for reminding me this is a change of public header!
>>
>> And this is a userspace api, you just changed the size of an externally
>> facing variable, are you _SURE_ that is ok to do?
>
> The reasons I make this change are, 1) using int to represent 7 enumeration
> values seems to be overkill; 2) using one type could avoid worries
> about byte order.
(1) might be a valid suggestion, but the file you suggest
changing is part of the Linux user space API, which you
basically can't change.
I'm fairly certain the user space API is defined to have
CPU-local byte order (unless specified explicitly otherwise)
so that is not a concern.
> I examine one userspace example (libalsa-intf/alsa_mixer.c [1]). This
> change won't cause compiling breakage. But I don't the experience to
> imagine any other bad consequence.
As a rule, once the user space API has been established, it
stays with us forever. You can add to it, but modifying
(or removing) an existing interface is essentially forbidden.
> Another way to avoid userspace API change is to let
> "struct gb_audio_ctl_elem_info" use snd_ctl_elem_iface_t type which may
> be more elegant than using "__force" as suggested by Alex,
The Greybus definitions were explicitly defined to be
operating system independent. For that reason there are
(admittedly cumbersome) definitions of certain types and
values that essentially mimic those defined by Linux
exactly That way Linux (or another system using Greybus)
can change its internal values without changing the
Greybus API definition. (This addresses the concern you
mention below.)
What you are suggesting is not consistent with that
principle.
-Alex
> $ git diff
> diff --git a/include/linux/greybus/greybus_protocols.h
> b/include/linux/greybus/greybus_protocols.h
> index aeb8f9243545..7f6753ec7ef7 100644
> --- a/include/linux/greybus/greybus_protocols.h
> +++ b/include/linux/greybus/greybus_protocols.h
> @@ -8,6 +8,7 @@
> #define __GREYBUS_PROTOCOLS_H
>
> #include <linux/types.h>
> +#include <sound/asound.h>
>
> /* Fixed IDs for control/svc protocols */
>
> @@ -1997,7 +1998,7 @@ struct gb_audio_enumerated {
> } __packed;
>
> struct gb_audio_ctl_elem_info { /* See snd_ctl_elem_info in Linux
> source */
> - __u8 type; /* GB_AUDIO_CTL_ELEM_TYPE_* */
> + snd_ctl_elem_type_t type; /*
> GB_AUDIO_CTL_ELEM_TYPE_* */
> __le16 dimen[4];
> union {
> struct gb_audio_integer integer
>
> My only concern is if greybus authors have any special reason to not
> include
> sound/asound.h in the first place and re-define things like
> SNDRV_CTL_ELEM_TYPE_*,
>
> /* See SNDRV_CTL_ELEM_TYPE_* in Linux source */
> #define GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN 0x01
> #define GB_AUDIO_CTL_ELEM_TYPE_INTEGER 0x02
>
> /* See SNDRV_CTL_ELEM_IFACE_* in Linux source */
> #define GB_AUDIO_CTL_ELEM_IFACE_CARD 0x00
> ...
>
> /* SNDRV_CTL_ELEM_ACCESS_* in Linux source */
> #define GB_AUDIO_ACCESS_READ BIT(0)
> ...
>
> [1]
> https://gitlab.com/Codeaurora/platform_hardware_qcom_audio/-/blob/aee6032826ec7c8b6edda404422fda0ef40ec2db/libalsa-intf/alsa_mixer.c
>
> alsa_mixer.c
>>
>> thanks,
>>
>> greg k-h
>
> --
> Best regards,
> Coiby
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
2020-09-25 15:57 ` Alex Elder
@ 2020-09-26 5:09 ` Coiby Xu
-1 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-26 5:09 UTC (permalink / raw)
To: Alex Elder
Cc: Greg Kroah-Hartman, devel, moderated list:SOUND, Vaibhav Agarwal,
Mark Greer, Takashi Iwai, Johan Hovold, Jaroslav Kysela,
moderated list:GREYBUS SUBSYSTEM, open list
On Fri, Sep 25, 2020 at 10:57:27AM -0500, Alex Elder wrote:
>On 9/25/20 9:07 AM, Coiby Xu wrote:
>>On Thu, Sep 24, 2020 at 01:00:57PM +0200, Greg Kroah-Hartman wrote:
>>>On Thu, Sep 24, 2020 at 06:20:39PM +0800, Coiby Xu wrote:
>>>>Use __8 to replace int and remove the unnecessary __bitwise type
>>>>attribute.
>>>>
>>>>Found by sparse,
>
>. . .
>
>>>>diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>>>index 535a7229e1d9..8e71a95644ab 100644
>>>>--- a/include/uapi/sound/asound.h
>>>>+++ b/include/uapi/sound/asound.h
>>>>@@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>>>> unsigned char components[128]; /* card components / fine
>>>>identification, delimited with one space (AC97 etc..) */
>>>> };
>>>>
>>>>-typedef int __bitwise snd_ctl_elem_type_t;
>>>>+typedef __u8 snd_ctl_elem_type_t;
>>>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force
>>>>snd_ctl_elem_type_t) 0) /* invalid */
>>>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force
>>>>snd_ctl_elem_type_t) 1) /* boolean type */
>>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force
>>>>snd_ctl_elem_type_t) 2) /* integer type */
>>>>@@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
>>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force
>>>>snd_ctl_elem_type_t) 6) /* 64-bit integer type */
>>>> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>>>>
>>>>-typedef int __bitwise snd_ctl_elem_iface_t;
>>>>+typedef __u8 snd_ctl_elem_iface_t;
>>>> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force
>>>>snd_ctl_elem_iface_t) 0) /* global control */
>>>> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force
>>>>snd_ctl_elem_iface_t) 1) /* hardware dependent device */
>>>> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force
>>>>snd_ctl_elem_iface_t) 2) /* virtual mixer device */
>>>
>>>I can't take a uapi sound header file patch along with a driver change,
>>>these need to be independant.
>>
>>Thank you and Alex for reminding me this is a change of public header!
>>>
>>>And this is a userspace api, you just changed the size of an externally
>>>facing variable, are you _SURE_ that is ok to do?
>>
>>The reasons I make this change are, 1) using int to represent 7 enumeration
>>values seems to be overkill; 2) using one type could avoid worries
>>about byte order.
>
>(1) might be a valid suggestion, but the file you suggest
>changing is part of the Linux user space API, which you
>basically can't change.
>
>I'm fairly certain the user space API is defined to have
>CPU-local byte order (unless specified explicitly otherwise)
>so that is not a concern.
Thank you for sharing me about the byte order of user space API which
prompts me to examine where "info->type" comes from,
uinfo->type = (snd_ctl_elem_type_t)info->type;
Eventually it comes from the parsed topology data which is obtained via
gb_audio_gb_get_topology(gbmodule->mgmt_connection, &topology). But
since (struct gb_audio_ctl_elem_info*)->type has __u8 type, there is no
endianness concern. Then based on the principles of userspace API
shouldn't be modified and greybus is operating system independent, your
previous suggestion to use __force which means "I know what I'm doing"
is actually a good idea,
uinfo->type = (__force snd_ctl_elem_type_t)info->type;
>>I examine one userspace example (libalsa-intf/alsa_mixer.c [1]). This
>>change won't cause compiling breakage. But I don't the experience to
>>imagine any other bad consequence.
>
>As a rule, once the user space API has been established, it
>stays with us forever. You can add to it, but modifying
>(or removing) an existing interface is essentially forbidden.
>
>>Another way to avoid userspace API change is to let
>>"struct gb_audio_ctl_elem_info" use snd_ctl_elem_iface_t type which may
>>be more elegant than using "__force" as suggested by Alex,
>
>The Greybus definitions were explicitly defined to be
>operating system independent. For that reason there are
>(admittedly cumbersome) definitions of certain types and
>values that essentially mimic those defined by Linux
>exactly That way Linux (or another system using Greybus)
>can change its internal values without changing the
>Greybus API definition. (This addresses the concern you
>mention below.)
>
>What you are suggesting is not consistent with that
>principle.
>
> -Alex
>
>
>>$ git diff
>>diff --git a/include/linux/greybus/greybus_protocols.h
>>b/include/linux/greybus/greybus_protocols.h
>>index aeb8f9243545..7f6753ec7ef7 100644
>>--- a/include/linux/greybus/greybus_protocols.h
>>+++ b/include/linux/greybus/greybus_protocols.h
>>@@ -8,6 +8,7 @@
>> #define __GREYBUS_PROTOCOLS_H
>>
>> #include <linux/types.h>
>>+#include <sound/asound.h>
>>
>> /* Fixed IDs for control/svc protocols */
>>
>>@@ -1997,7 +1998,7 @@ struct gb_audio_enumerated {
>> } __packed;
>>
>> struct gb_audio_ctl_elem_info { /* See snd_ctl_elem_info in Linux
>>source */
>>- __u8 type; /* GB_AUDIO_CTL_ELEM_TYPE_* */
>>+ snd_ctl_elem_type_t type; /*
>>GB_AUDIO_CTL_ELEM_TYPE_* */
>> __le16 dimen[4];
>> union {
>> struct gb_audio_integer integer
>>
>>My only concern is if greybus authors have any special reason to not
>>include
>>sound/asound.h in the first place and re-define things like
>>SNDRV_CTL_ELEM_TYPE_*,
>>
>>/* See SNDRV_CTL_ELEM_TYPE_* in Linux source */
>>#define GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN 0x01
>>#define GB_AUDIO_CTL_ELEM_TYPE_INTEGER 0x02
>>
>>/* See SNDRV_CTL_ELEM_IFACE_* in Linux source */
>>#define GB_AUDIO_CTL_ELEM_IFACE_CARD 0x00
>>...
>>
>>/* SNDRV_CTL_ELEM_ACCESS_* in Linux source */
>>#define GB_AUDIO_ACCESS_READ BIT(0)
>>...
>>
>>[1] https://gitlab.com/Codeaurora/platform_hardware_qcom_audio/-/blob/aee6032826ec7c8b6edda404422fda0ef40ec2db/libalsa-intf/alsa_mixer.c
>>
>> alsa_mixer.c
>>>
>>>thanks,
>>>
>>>greg k-h
>>
>>--
>>Best regards,
>>Coiby
>
--
Best regards,
Coiby
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-26 5:09 ` Coiby Xu
0 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-26 5:09 UTC (permalink / raw)
To: Alex Elder
Cc: devel, moderated list:SOUND, Vaibhav Agarwal, Greg Kroah-Hartman,
Takashi Iwai, Mark Greer, moderated list:GREYBUS SUBSYSTEM,
Johan Hovold, Jaroslav Kysela, open list
On Fri, Sep 25, 2020 at 10:57:27AM -0500, Alex Elder wrote:
>On 9/25/20 9:07 AM, Coiby Xu wrote:
>>On Thu, Sep 24, 2020 at 01:00:57PM +0200, Greg Kroah-Hartman wrote:
>>>On Thu, Sep 24, 2020 at 06:20:39PM +0800, Coiby Xu wrote:
>>>>Use __8 to replace int and remove the unnecessary __bitwise type
>>>>attribute.
>>>>
>>>>Found by sparse,
>
>. . .
>
>>>>diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>>>index 535a7229e1d9..8e71a95644ab 100644
>>>>--- a/include/uapi/sound/asound.h
>>>>+++ b/include/uapi/sound/asound.h
>>>>@@ -950,7 +950,7 @@ struct snd_ctl_card_info {
>>>> unsigned char components[128]; /* card components / fine
>>>>identification, delimited with one space (AC97 etc..) */
>>>> };
>>>>
>>>>-typedef int __bitwise snd_ctl_elem_type_t;
>>>>+typedef __u8 snd_ctl_elem_type_t;
>>>> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force
>>>>snd_ctl_elem_type_t) 0) /* invalid */
>>>> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force
>>>>snd_ctl_elem_type_t) 1) /* boolean type */
>>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force
>>>>snd_ctl_elem_type_t) 2) /* integer type */
>>>>@@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
>>>> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force
>>>>snd_ctl_elem_type_t) 6) /* 64-bit integer type */
>>>> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>>>>
>>>>-typedef int __bitwise snd_ctl_elem_iface_t;
>>>>+typedef __u8 snd_ctl_elem_iface_t;
>>>> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force
>>>>snd_ctl_elem_iface_t) 0) /* global control */
>>>> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force
>>>>snd_ctl_elem_iface_t) 1) /* hardware dependent device */
>>>> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force
>>>>snd_ctl_elem_iface_t) 2) /* virtual mixer device */
>>>
>>>I can't take a uapi sound header file patch along with a driver change,
>>>these need to be independant.
>>
>>Thank you and Alex for reminding me this is a change of public header!
>>>
>>>And this is a userspace api, you just changed the size of an externally
>>>facing variable, are you _SURE_ that is ok to do?
>>
>>The reasons I make this change are, 1) using int to represent 7 enumeration
>>values seems to be overkill; 2) using one type could avoid worries
>>about byte order.
>
>(1) might be a valid suggestion, but the file you suggest
>changing is part of the Linux user space API, which you
>basically can't change.
>
>I'm fairly certain the user space API is defined to have
>CPU-local byte order (unless specified explicitly otherwise)
>so that is not a concern.
Thank you for sharing me about the byte order of user space API which
prompts me to examine where "info->type" comes from,
uinfo->type = (snd_ctl_elem_type_t)info->type;
Eventually it comes from the parsed topology data which is obtained via
gb_audio_gb_get_topology(gbmodule->mgmt_connection, &topology). But
since (struct gb_audio_ctl_elem_info*)->type has __u8 type, there is no
endianness concern. Then based on the principles of userspace API
shouldn't be modified and greybus is operating system independent, your
previous suggestion to use __force which means "I know what I'm doing"
is actually a good idea,
uinfo->type = (__force snd_ctl_elem_type_t)info->type;
>>I examine one userspace example (libalsa-intf/alsa_mixer.c [1]). This
>>change won't cause compiling breakage. But I don't the experience to
>>imagine any other bad consequence.
>
>As a rule, once the user space API has been established, it
>stays with us forever. You can add to it, but modifying
>(or removing) an existing interface is essentially forbidden.
>
>>Another way to avoid userspace API change is to let
>>"struct gb_audio_ctl_elem_info" use snd_ctl_elem_iface_t type which may
>>be more elegant than using "__force" as suggested by Alex,
>
>The Greybus definitions were explicitly defined to be
>operating system independent. For that reason there are
>(admittedly cumbersome) definitions of certain types and
>values that essentially mimic those defined by Linux
>exactly That way Linux (or another system using Greybus)
>can change its internal values without changing the
>Greybus API definition. (This addresses the concern you
>mention below.)
>
>What you are suggesting is not consistent with that
>principle.
>
> -Alex
>
>
>>$ git diff
>>diff --git a/include/linux/greybus/greybus_protocols.h
>>b/include/linux/greybus/greybus_protocols.h
>>index aeb8f9243545..7f6753ec7ef7 100644
>>--- a/include/linux/greybus/greybus_protocols.h
>>+++ b/include/linux/greybus/greybus_protocols.h
>>@@ -8,6 +8,7 @@
>> #define __GREYBUS_PROTOCOLS_H
>>
>> #include <linux/types.h>
>>+#include <sound/asound.h>
>>
>> /* Fixed IDs for control/svc protocols */
>>
>>@@ -1997,7 +1998,7 @@ struct gb_audio_enumerated {
>> } __packed;
>>
>> struct gb_audio_ctl_elem_info { /* See snd_ctl_elem_info in Linux
>>source */
>>- __u8 type; /* GB_AUDIO_CTL_ELEM_TYPE_* */
>>+ snd_ctl_elem_type_t type; /*
>>GB_AUDIO_CTL_ELEM_TYPE_* */
>> __le16 dimen[4];
>> union {
>> struct gb_audio_integer integer
>>
>>My only concern is if greybus authors have any special reason to not
>>include
>>sound/asound.h in the first place and re-define things like
>>SNDRV_CTL_ELEM_TYPE_*,
>>
>>/* See SNDRV_CTL_ELEM_TYPE_* in Linux source */
>>#define GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN 0x01
>>#define GB_AUDIO_CTL_ELEM_TYPE_INTEGER 0x02
>>
>>/* See SNDRV_CTL_ELEM_IFACE_* in Linux source */
>>#define GB_AUDIO_CTL_ELEM_IFACE_CARD 0x00
>>...
>>
>>/* SNDRV_CTL_ELEM_ACCESS_* in Linux source */
>>#define GB_AUDIO_ACCESS_READ BIT(0)
>>...
>>
>>[1] https://gitlab.com/Codeaurora/platform_hardware_qcom_audio/-/blob/aee6032826ec7c8b6edda404422fda0ef40ec2db/libalsa-intf/alsa_mixer.c
>>
>> alsa_mixer.c
>>>
>>>thanks,
>>>
>>>greg k-h
>>
>>--
>>Best regards,
>>Coiby
>
--
Best regards,
Coiby
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [greybus-dev] [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
2020-09-24 10:20 ` Coiby Xu
(?)
@ 2020-09-24 13:01 ` Alex Elder
-1 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-24 13:01 UTC (permalink / raw)
To: Coiby Xu, devel
Cc: moderated list:SOUND, Alex Elder, Takashi Iwai, Johan Hovold,
Jaroslav Kysela, moderated list:GREYBUS SUBSYSTEM, open list
On 9/24/20 5:20 AM, Coiby Xu wrote:
> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>
> Found by sparse,
Greg's right, don't change the public header.
You could try this in the Greybus code to eliminate the warning,
but I haven't tried it (and for all I know it's not a good idea):
uinfo->type = (__force snd_ctl_elem_type_t)info->type;
-Alex
> $ make C=2 drivers/staging/greybus/
> drivers/staging/greybus/audio_topology.c:185:24: warning: cast to restricted snd_ctl_elem_type_t
> drivers/staging/greybus/audio_topology.c:679:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
> drivers/staging/greybus/audio_topology.c:906:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
> drivers/staging/greybus/audio_topology.c | 2 +-
> include/uapi/sound/asound.h | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 56bf1a4f95ad..f6023ff390c2 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -182,7 +182,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
> /* update uinfo */
> uinfo->access = data->access;
> uinfo->count = data->vcount;
> - uinfo->type = (snd_ctl_elem_type_t)info->type;
> + uinfo->type = info->type;
>
> switch (info->type) {
> case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN:
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 535a7229e1d9..8e71a95644ab 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */
> };
>
> -typedef int __bitwise snd_ctl_elem_type_t;
> +typedef __u8 snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
> @@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>
> -typedef int __bitwise snd_ctl_elem_iface_t;
> +typedef __u8 snd_ctl_elem_iface_t;
> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force snd_ctl_elem_iface_t) 0) /* global control */
> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force snd_ctl_elem_iface_t) 1) /* hardware dependent device */
> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force snd_ctl_elem_iface_t) 2) /* virtual mixer device */
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-24 13:01 ` Alex Elder
0 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-24 13:01 UTC (permalink / raw)
To: Coiby Xu, devel
Cc: moderated list:SOUND, Alex Elder, open list, Takashi Iwai,
moderated list:GREYBUS SUBSYSTEM, Johan Hovold
On 9/24/20 5:20 AM, Coiby Xu wrote:
> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>
> Found by sparse,
Greg's right, don't change the public header.
You could try this in the Greybus code to eliminate the warning,
but I haven't tried it (and for all I know it's not a good idea):
uinfo->type = (__force snd_ctl_elem_type_t)info->type;
-Alex
> $ make C=2 drivers/staging/greybus/
> drivers/staging/greybus/audio_topology.c:185:24: warning: cast to restricted snd_ctl_elem_type_t
> drivers/staging/greybus/audio_topology.c:679:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
> drivers/staging/greybus/audio_topology.c:906:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
> drivers/staging/greybus/audio_topology.c | 2 +-
> include/uapi/sound/asound.h | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 56bf1a4f95ad..f6023ff390c2 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -182,7 +182,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
> /* update uinfo */
> uinfo->access = data->access;
> uinfo->count = data->vcount;
> - uinfo->type = (snd_ctl_elem_type_t)info->type;
> + uinfo->type = info->type;
>
> switch (info->type) {
> case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN:
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 535a7229e1d9..8e71a95644ab 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */
> };
>
> -typedef int __bitwise snd_ctl_elem_type_t;
> +typedef __u8 snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
> @@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>
> -typedef int __bitwise snd_ctl_elem_iface_t;
> +typedef __u8 snd_ctl_elem_iface_t;
> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force snd_ctl_elem_iface_t) 0) /* global control */
> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force snd_ctl_elem_iface_t) 1) /* hardware dependent device */
> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force snd_ctl_elem_iface_t) 2) /* virtual mixer device */
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
@ 2020-09-24 13:01 ` Alex Elder
0 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-24 13:01 UTC (permalink / raw)
To: Coiby Xu, devel
Cc: moderated list:SOUND, Alex Elder, open list, Takashi Iwai,
Jaroslav Kysela, moderated list:GREYBUS SUBSYSTEM, Johan Hovold
On 9/24/20 5:20 AM, Coiby Xu wrote:
> Use __8 to replace int and remove the unnecessary __bitwise type attribute.
>
> Found by sparse,
Greg's right, don't change the public header.
You could try this in the Greybus code to eliminate the warning,
but I haven't tried it (and for all I know it's not a good idea):
uinfo->type = (__force snd_ctl_elem_type_t)info->type;
-Alex
> $ make C=2 drivers/staging/greybus/
> drivers/staging/greybus/audio_topology.c:185:24: warning: cast to restricted snd_ctl_elem_type_t
> drivers/staging/greybus/audio_topology.c:679:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
> drivers/staging/greybus/audio_topology.c:906:14: warning: restricted snd_ctl_elem_iface_t degrades to integer
>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
> drivers/staging/greybus/audio_topology.c | 2 +-
> include/uapi/sound/asound.h | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 56bf1a4f95ad..f6023ff390c2 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -182,7 +182,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
> /* update uinfo */
> uinfo->access = data->access;
> uinfo->count = data->vcount;
> - uinfo->type = (snd_ctl_elem_type_t)info->type;
> + uinfo->type = info->type;
>
> switch (info->type) {
> case GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN:
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 535a7229e1d9..8e71a95644ab 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -950,7 +950,7 @@ struct snd_ctl_card_info {
> unsigned char components[128]; /* card components / fine identification, delimited with one space (AC97 etc..) */
> };
>
> -typedef int __bitwise snd_ctl_elem_type_t;
> +typedef __u8 snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_NONE ((__force snd_ctl_elem_type_t) 0) /* invalid */
> #define SNDRV_CTL_ELEM_TYPE_BOOLEAN ((__force snd_ctl_elem_type_t) 1) /* boolean type */
> #define SNDRV_CTL_ELEM_TYPE_INTEGER ((__force snd_ctl_elem_type_t) 2) /* integer type */
> @@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
> #define SNDRV_CTL_ELEM_TYPE_INTEGER64 ((__force snd_ctl_elem_type_t) 6) /* 64-bit integer type */
> #define SNDRV_CTL_ELEM_TYPE_LAST SNDRV_CTL_ELEM_TYPE_INTEGER64
>
> -typedef int __bitwise snd_ctl_elem_iface_t;
> +typedef __u8 snd_ctl_elem_iface_t;
> #define SNDRV_CTL_ELEM_IFACE_CARD ((__force snd_ctl_elem_iface_t) 0) /* global control */
> #define SNDRV_CTL_ELEM_IFACE_HWDEP ((__force snd_ctl_elem_iface_t) 1) /* hardware dependent device */
> #define SNDRV_CTL_ELEM_IFACE_MIXER ((__force snd_ctl_elem_iface_t) 2) /* virtual mixer device */
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [greybus-dev] [PATCH 1/3] [PATCH] staging: greybus: fix warnings about endianness detected by sparse
2020-09-24 10:20 ` Coiby Xu
@ 2020-09-24 12:50 ` Alex Elder
-1 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-24 12:50 UTC (permalink / raw)
To: Coiby Xu, devel
Cc: Alex Elder, open list, Johan Hovold,
moderated list:GREYBUS SUBSYSTEM, Dan Carpenter
On 9/24/20 5:20 AM, Coiby Xu wrote:
> This patch fix the following warnings from sparse,
You need to address Greg's comment.
But in general this looks good. I have one comment below, which
you can address in v2. If you (or others) disagree with it, I'm
fine with your code as-is. Either way, you can add this:
Reviewed-by: Alex Elder <elder@linaro.org>
> $ make C=2 drivers/staging/greybus/
> drivers/staging/greybus/audio_module.c:222:25: warning: incorrect type in assignment (different base types)
> drivers/staging/greybus/audio_module.c:222:25: expected restricted __le16 [usertype] data_cport
> drivers/staging/greybus/audio_module.c:222:25: got unsigned short [usertype] intf_cport_id
> drivers/staging/greybus/audio_topology.c:460:40: warning: restricted __le32 degrades to integer
> drivers/staging/greybus/audio_topology.c:691:41: warning: incorrect type in assignment (different base types)
. . .
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 83b38ae8908c..56bf1a4f95ad 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -466,7 +466,7 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
> goto exit;
>
> /* update ucontrol */
> - if (gbvalue.value.integer_value[0] != val) {
> + if (gbvalue.value.integer_value[0] != cpu_to_le32(val)) {
It's equivalent, but I have a small preference to convert
the value from gbvalue into CPU byte order rather than
what you have here.
> for (wi = 0; wi < wlist->num_widgets; wi++) {
> widget = wlist->widgets[wi];
> snd_soc_dapm_mixer_update_power(widget->dapm, kcontrol,
> @@ -689,7 +689,7 @@ static int gbaudio_tplg_create_kcontrol(struct gbaudio_module_info *gb,
> return -ENOMEM;
> ctldata->ctl_id = ctl->id;
> ctldata->data_cport = le16_to_cpu(ctl->data_cport);
> - ctldata->access = ctl->access;
> + ctldata->access = le32_to_cpu(ctl->access);
> ctldata->vcount = ctl->count_values;
> ctldata->info = &ctl->info;
> *kctl = (struct snd_kcontrol_new)
> @@ -744,10 +744,10 @@ static int gbcodec_enum_dapm_ctl_get(struct snd_kcontrol *kcontrol,
> return ret;
> }
>
> - ucontrol->value.enumerated.item[0] = gbvalue.value.enumerated_item[0];
> + ucontrol->value.enumerated.item[0] = le32_to_cpu(gbvalue.value.enumerated_item[0]);
> if (e->shift_l != e->shift_r)
> ucontrol->value.enumerated.item[1] =
> - gbvalue.value.enumerated_item[1];
> + le32_to_cpu(gbvalue.value.enumerated_item[1]);
>
> return 0;
> }
> @@ -801,10 +801,10 @@ static int gbcodec_enum_dapm_ctl_put(struct snd_kcontrol *kcontrol,
> mask = e->mask << e->shift_l;
>
> if (gbvalue.value.enumerated_item[0] !=
> - ucontrol->value.enumerated.item[0]) {
> + cpu_to_le32(ucontrol->value.enumerated.item[0])) {
> change = 1;
> gbvalue.value.enumerated_item[0] =
> - ucontrol->value.enumerated.item[0];
> + cpu_to_le32(ucontrol->value.enumerated.item[0]);
> }
>
> if (e->shift_l != e->shift_r) {
> @@ -813,10 +813,10 @@ static int gbcodec_enum_dapm_ctl_put(struct snd_kcontrol *kcontrol,
> val |= ucontrol->value.enumerated.item[1] << e->shift_r;
> mask |= e->mask << e->shift_r;
> if (gbvalue.value.enumerated_item[1] !=
> - ucontrol->value.enumerated.item[1]) {
> + cpu_to_le32(ucontrol->value.enumerated.item[1])) {
> change = 1;
> gbvalue.value.enumerated_item[1] =
> - ucontrol->value.enumerated.item[1];
> + cpu_to_le32(ucontrol->value.enumerated.item[1]);
> }
> }
>
> @@ -887,7 +887,7 @@ static int gbaudio_tplg_create_mixer_ctl(struct gbaudio_module_info *gb,
> return -ENOMEM;
> ctldata->ctl_id = ctl->id;
> ctldata->data_cport = le16_to_cpu(ctl->data_cport);
> - ctldata->access = ctl->access;
> + ctldata->access = le32_to_cpu(ctl->access);
> ctldata->vcount = ctl->count_values;
> ctldata->info = &ctl->info;
> *kctl = (struct snd_kcontrol_new)
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 1/3] [PATCH] staging: greybus: fix warnings about endianness detected by sparse
@ 2020-09-24 12:50 ` Alex Elder
0 siblings, 0 replies; 38+ messages in thread
From: Alex Elder @ 2020-09-24 12:50 UTC (permalink / raw)
To: Coiby Xu, devel
Cc: moderated list:GREYBUS SUBSYSTEM, Alex Elder, open list,
Dan Carpenter, Johan Hovold
On 9/24/20 5:20 AM, Coiby Xu wrote:
> This patch fix the following warnings from sparse,
You need to address Greg's comment.
But in general this looks good. I have one comment below, which
you can address in v2. If you (or others) disagree with it, I'm
fine with your code as-is. Either way, you can add this:
Reviewed-by: Alex Elder <elder@linaro.org>
> $ make C=2 drivers/staging/greybus/
> drivers/staging/greybus/audio_module.c:222:25: warning: incorrect type in assignment (different base types)
> drivers/staging/greybus/audio_module.c:222:25: expected restricted __le16 [usertype] data_cport
> drivers/staging/greybus/audio_module.c:222:25: got unsigned short [usertype] intf_cport_id
> drivers/staging/greybus/audio_topology.c:460:40: warning: restricted __le32 degrades to integer
> drivers/staging/greybus/audio_topology.c:691:41: warning: incorrect type in assignment (different base types)
. . .
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 83b38ae8908c..56bf1a4f95ad 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -466,7 +466,7 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
> goto exit;
>
> /* update ucontrol */
> - if (gbvalue.value.integer_value[0] != val) {
> + if (gbvalue.value.integer_value[0] != cpu_to_le32(val)) {
It's equivalent, but I have a small preference to convert
the value from gbvalue into CPU byte order rather than
what you have here.
> for (wi = 0; wi < wlist->num_widgets; wi++) {
> widget = wlist->widgets[wi];
> snd_soc_dapm_mixer_update_power(widget->dapm, kcontrol,
> @@ -689,7 +689,7 @@ static int gbaudio_tplg_create_kcontrol(struct gbaudio_module_info *gb,
> return -ENOMEM;
> ctldata->ctl_id = ctl->id;
> ctldata->data_cport = le16_to_cpu(ctl->data_cport);
> - ctldata->access = ctl->access;
> + ctldata->access = le32_to_cpu(ctl->access);
> ctldata->vcount = ctl->count_values;
> ctldata->info = &ctl->info;
> *kctl = (struct snd_kcontrol_new)
> @@ -744,10 +744,10 @@ static int gbcodec_enum_dapm_ctl_get(struct snd_kcontrol *kcontrol,
> return ret;
> }
>
> - ucontrol->value.enumerated.item[0] = gbvalue.value.enumerated_item[0];
> + ucontrol->value.enumerated.item[0] = le32_to_cpu(gbvalue.value.enumerated_item[0]);
> if (e->shift_l != e->shift_r)
> ucontrol->value.enumerated.item[1] =
> - gbvalue.value.enumerated_item[1];
> + le32_to_cpu(gbvalue.value.enumerated_item[1]);
>
> return 0;
> }
> @@ -801,10 +801,10 @@ static int gbcodec_enum_dapm_ctl_put(struct snd_kcontrol *kcontrol,
> mask = e->mask << e->shift_l;
>
> if (gbvalue.value.enumerated_item[0] !=
> - ucontrol->value.enumerated.item[0]) {
> + cpu_to_le32(ucontrol->value.enumerated.item[0])) {
> change = 1;
> gbvalue.value.enumerated_item[0] =
> - ucontrol->value.enumerated.item[0];
> + cpu_to_le32(ucontrol->value.enumerated.item[0]);
> }
>
> if (e->shift_l != e->shift_r) {
> @@ -813,10 +813,10 @@ static int gbcodec_enum_dapm_ctl_put(struct snd_kcontrol *kcontrol,
> val |= ucontrol->value.enumerated.item[1] << e->shift_r;
> mask |= e->mask << e->shift_r;
> if (gbvalue.value.enumerated_item[1] !=
> - ucontrol->value.enumerated.item[1]) {
> + cpu_to_le32(ucontrol->value.enumerated.item[1])) {
> change = 1;
> gbvalue.value.enumerated_item[1] =
> - ucontrol->value.enumerated.item[1];
> + cpu_to_le32(ucontrol->value.enumerated.item[1]);
> }
> }
>
> @@ -887,7 +887,7 @@ static int gbaudio_tplg_create_mixer_ctl(struct gbaudio_module_info *gb,
> return -ENOMEM;
> ctldata->ctl_id = ctl->id;
> ctldata->data_cport = le16_to_cpu(ctl->data_cport);
> - ctldata->access = ctl->access;
> + ctldata->access = le32_to_cpu(ctl->access);
> ctldata->vcount = ctl->count_values;
> ctldata->info = &ctl->info;
> *kctl = (struct snd_kcontrol_new)
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 1/3] [PATCH] staging: greybus: fix warnings about endianness detected by sparse
2020-09-24 12:50 ` Alex Elder
@ 2020-09-25 14:09 ` Coiby Xu
-1 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-25 14:09 UTC (permalink / raw)
To: Alex Elder
Cc: devel, Alex Elder, open list, Johan Hovold,
moderated list:GREYBUS SUBSYSTEM, Dan Carpenter
On Thu, Sep 24, 2020 at 07:50:57AM -0500, Alex Elder wrote:
>On 9/24/20 5:20 AM, Coiby Xu wrote:
>> This patch fix the following warnings from sparse,
>
>You need to address Greg's comment.
>
>But in general this looks good. I have one comment below, which
>you can address in v2. If you (or others) disagree with it, I'm
>fine with your code as-is. Either way, you can add this:
>
>Reviewed-by: Alex Elder <elder@linaro.org>
Thank you fore reviewing this patch!
>
>> $ make C=2 drivers/staging/greybus/
>> drivers/staging/greybus/audio_module.c:222:25: warning: incorrect type in assignment (different base types)
>> drivers/staging/greybus/audio_module.c:222:25: expected restricted __le16 [usertype] data_cport
>> drivers/staging/greybus/audio_module.c:222:25: got unsigned short [usertype] intf_cport_id
>> drivers/staging/greybus/audio_topology.c:460:40: warning: restricted __le32 degrades to integer
>> drivers/staging/greybus/audio_topology.c:691:41: warning: incorrect type in assignment (different base types)
>
>. . .
>
>> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
>> index 83b38ae8908c..56bf1a4f95ad 100644
>> --- a/drivers/staging/greybus/audio_topology.c
>> +++ b/drivers/staging/greybus/audio_topology.c
>> @@ -466,7 +466,7 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
>> goto exit;
>>
>> /* update ucontrol */
>> - if (gbvalue.value.integer_value[0] != val) {
>> + if (gbvalue.value.integer_value[0] != cpu_to_le32(val)) {
>
>It's equivalent, but I have a small preference to convert
>the value from gbvalue into CPU byte order rather than
>what you have here.
Thank you for the suggestion! I'll use CPU byte order when submitting
next version.
>
>> for (wi = 0; wi < wlist->num_widgets; wi++) {
>> widget = wlist->widgets[wi];
>> snd_soc_dapm_mixer_update_power(widget->dapm, kcontrol,
>> @@ -689,7 +689,7 @@ static int gbaudio_tplg_create_kcontrol(struct gbaudio_module_info *gb,
>> return -ENOMEM;
>> ctldata->ctl_id = ctl->id;
>> ctldata->data_cport = le16_to_cpu(ctl->data_cport);
>> - ctldata->access = ctl->access;
>> + ctldata->access = le32_to_cpu(ctl->access);
>> ctldata->vcount = ctl->count_values;
>> ctldata->info = &ctl->info;
>> *kctl = (struct snd_kcontrol_new)
>> @@ -744,10 +744,10 @@ static int gbcodec_enum_dapm_ctl_get(struct snd_kcontrol *kcontrol,
>> return ret;
>> }
>>
>> - ucontrol->value.enumerated.item[0] = gbvalue.value.enumerated_item[0];
>> + ucontrol->value.enumerated.item[0] = le32_to_cpu(gbvalue.value.enumerated_item[0]);
>> if (e->shift_l != e->shift_r)
>> ucontrol->value.enumerated.item[1] =
>> - gbvalue.value.enumerated_item[1];
>> + le32_to_cpu(gbvalue.value.enumerated_item[1]);
>>
>> return 0;
>> }
>> @@ -801,10 +801,10 @@ static int gbcodec_enum_dapm_ctl_put(struct snd_kcontrol *kcontrol,
>> mask = e->mask << e->shift_l;
>>
>> if (gbvalue.value.enumerated_item[0] !=
>> - ucontrol->value.enumerated.item[0]) {
>> + cpu_to_le32(ucontrol->value.enumerated.item[0])) {
>> change = 1;
>> gbvalue.value.enumerated_item[0] =
>> - ucontrol->value.enumerated.item[0];
>> + cpu_to_le32(ucontrol->value.enumerated.item[0]);
>> }
>>
>> if (e->shift_l != e->shift_r) {
>> @@ -813,10 +813,10 @@ static int gbcodec_enum_dapm_ctl_put(struct snd_kcontrol *kcontrol,
>> val |= ucontrol->value.enumerated.item[1] << e->shift_r;
>> mask |= e->mask << e->shift_r;
>> if (gbvalue.value.enumerated_item[1] !=
>> - ucontrol->value.enumerated.item[1]) {
>> + cpu_to_le32(ucontrol->value.enumerated.item[1])) {
>> change = 1;
>> gbvalue.value.enumerated_item[1] =
>> - ucontrol->value.enumerated.item[1];
>> + cpu_to_le32(ucontrol->value.enumerated.item[1]);
>> }
>> }
>>
>> @@ -887,7 +887,7 @@ static int gbaudio_tplg_create_mixer_ctl(struct gbaudio_module_info *gb,
>> return -ENOMEM;
>> ctldata->ctl_id = ctl->id;
>> ctldata->data_cport = le16_to_cpu(ctl->data_cport);
>> - ctldata->access = ctl->access;
>> + ctldata->access = le32_to_cpu(ctl->access);
>> ctldata->vcount = ctl->count_values;
>> ctldata->info = &ctl->info;
>> *kctl = (struct snd_kcontrol_new)
>>
>
--
Best regards,
Coiby
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [greybus-dev] [PATCH 1/3] [PATCH] staging: greybus: fix warnings about endianness detected by sparse
@ 2020-09-25 14:09 ` Coiby Xu
0 siblings, 0 replies; 38+ messages in thread
From: Coiby Xu @ 2020-09-25 14:09 UTC (permalink / raw)
To: Alex Elder
Cc: devel, Alex Elder, open list, Johan Hovold,
moderated list:GREYBUS SUBSYSTEM, Dan Carpenter
On Thu, Sep 24, 2020 at 07:50:57AM -0500, Alex Elder wrote:
>On 9/24/20 5:20 AM, Coiby Xu wrote:
>> This patch fix the following warnings from sparse,
>
>You need to address Greg's comment.
>
>But in general this looks good. I have one comment below, which
>you can address in v2. If you (or others) disagree with it, I'm
>fine with your code as-is. Either way, you can add this:
>
>Reviewed-by: Alex Elder <elder@linaro.org>
Thank you fore reviewing this patch!
>
>> $ make C=2 drivers/staging/greybus/
>> drivers/staging/greybus/audio_module.c:222:25: warning: incorrect type in assignment (different base types)
>> drivers/staging/greybus/audio_module.c:222:25: expected restricted __le16 [usertype] data_cport
>> drivers/staging/greybus/audio_module.c:222:25: got unsigned short [usertype] intf_cport_id
>> drivers/staging/greybus/audio_topology.c:460:40: warning: restricted __le32 degrades to integer
>> drivers/staging/greybus/audio_topology.c:691:41: warning: incorrect type in assignment (different base types)
>
>. . .
>
>> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
>> index 83b38ae8908c..56bf1a4f95ad 100644
>> --- a/drivers/staging/greybus/audio_topology.c
>> +++ b/drivers/staging/greybus/audio_topology.c
>> @@ -466,7 +466,7 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
>> goto exit;
>>
>> /* update ucontrol */
>> - if (gbvalue.value.integer_value[0] != val) {
>> + if (gbvalue.value.integer_value[0] != cpu_to_le32(val)) {
>
>It's equivalent, but I have a small preference to convert
>the value from gbvalue into CPU byte order rather than
>what you have here.
Thank you for the suggestion! I'll use CPU byte order when submitting
next version.
>
>> for (wi = 0; wi < wlist->num_widgets; wi++) {
>> widget = wlist->widgets[wi];
>> snd_soc_dapm_mixer_update_power(widget->dapm, kcontrol,
>> @@ -689,7 +689,7 @@ static int gbaudio_tplg_create_kcontrol(struct gbaudio_module_info *gb,
>> return -ENOMEM;
>> ctldata->ctl_id = ctl->id;
>> ctldata->data_cport = le16_to_cpu(ctl->data_cport);
>> - ctldata->access = ctl->access;
>> + ctldata->access = le32_to_cpu(ctl->access);
>> ctldata->vcount = ctl->count_values;
>> ctldata->info = &ctl->info;
>> *kctl = (struct snd_kcontrol_new)
>> @@ -744,10 +744,10 @@ static int gbcodec_enum_dapm_ctl_get(struct snd_kcontrol *kcontrol,
>> return ret;
>> }
>>
>> - ucontrol->value.enumerated.item[0] = gbvalue.value.enumerated_item[0];
>> + ucontrol->value.enumerated.item[0] = le32_to_cpu(gbvalue.value.enumerated_item[0]);
>> if (e->shift_l != e->shift_r)
>> ucontrol->value.enumerated.item[1] =
>> - gbvalue.value.enumerated_item[1];
>> + le32_to_cpu(gbvalue.value.enumerated_item[1]);
>>
>> return 0;
>> }
>> @@ -801,10 +801,10 @@ static int gbcodec_enum_dapm_ctl_put(struct snd_kcontrol *kcontrol,
>> mask = e->mask << e->shift_l;
>>
>> if (gbvalue.value.enumerated_item[0] !=
>> - ucontrol->value.enumerated.item[0]) {
>> + cpu_to_le32(ucontrol->value.enumerated.item[0])) {
>> change = 1;
>> gbvalue.value.enumerated_item[0] =
>> - ucontrol->value.enumerated.item[0];
>> + cpu_to_le32(ucontrol->value.enumerated.item[0]);
>> }
>>
>> if (e->shift_l != e->shift_r) {
>> @@ -813,10 +813,10 @@ static int gbcodec_enum_dapm_ctl_put(struct snd_kcontrol *kcontrol,
>> val |= ucontrol->value.enumerated.item[1] << e->shift_r;
>> mask |= e->mask << e->shift_r;
>> if (gbvalue.value.enumerated_item[1] !=
>> - ucontrol->value.enumerated.item[1]) {
>> + cpu_to_le32(ucontrol->value.enumerated.item[1])) {
>> change = 1;
>> gbvalue.value.enumerated_item[1] =
>> - ucontrol->value.enumerated.item[1];
>> + cpu_to_le32(ucontrol->value.enumerated.item[1]);
>> }
>> }
>>
>> @@ -887,7 +887,7 @@ static int gbaudio_tplg_create_mixer_ctl(struct gbaudio_module_info *gb,
>> return -ENOMEM;
>> ctldata->ctl_id = ctl->id;
>> ctldata->data_cport = le16_to_cpu(ctl->data_cport);
>> - ctldata->access = ctl->access;
>> + ctldata->access = le32_to_cpu(ctl->access);
>> ctldata->vcount = ctl->count_values;
>> ctldata->info = &ctl->info;
>> *kctl = (struct snd_kcontrol_new)
>>
>
--
Best regards,
Coiby
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 38+ messages in thread