Re: [PATCH 02/13] staging: greybus: Switch from strlcpy to strscpy
On 2/16/21 9:48 AM, Kumar Kartikeya Dwivedi wrote: On Tue, Feb 16, 2021 at 08:24:59PM IST, Alex Elder wrote: This is a good change. But while you're at it, I would appreciate if you would convert a few spots to use sizeof(dest) rather than a fixed constant. I will point them out below. If this is the *only* request for a change on your series, please tell me that and I can sign off on So far, this has been the only request. this without you implementing my suggestion. But if you post a version 2, please do what I describe. I will incorporate these if I end up sending a v2. Alternatively, would a separate patch with your suggestions applied on top of this be acceptable, if I don't? Yes. Assuming you do that, for this patch (as-is): Reviewed-by: Alex Elder ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: minor code style fix
On 2/12/21 4:50 PM, Manikantan Ravichandran wrote:
checkpatch warning fix for string split across lines
Signed-off-by: Manikantan Ravichandran
I think what you're doing here *looks* reasonable. But
the GB_AUDIO_MANAGER_MODULE_NAME_LEN_SSCANF symbol is
a (string) numeric value that is associated with the
"s" that immediately follows it.
So I don't think your change makes sense, given the
meaning of the line you're changing.
Thanks.
-Alex
---
drivers/staging/greybus/audio_manager_sysfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/audio_manager_sysfs.c
b/drivers/staging/greybus/audio_manager_sysfs.c
index ab882cc49b41..fcd518f9540c 100644
--- a/drivers/staging/greybus/audio_manager_sysfs.c
+++ b/drivers/staging/greybus/audio_manager_sysfs.c
@@ -18,8 +18,8 @@ static ssize_t manager_sysfs_add_store(struct kobject *kobj,
struct gb_audio_manager_module_descriptor desc = { {0} };
int num = sscanf(buf,
- "name=%" GB_AUDIO_MANAGER_MODULE_NAME_LEN_SSCANF "s "
- "vid=%d pid=%d intf_id=%d i/p devices=0x%X o/p
devices=0x%X",
+ "name=%" GB_AUDIO_MANAGER_MODULE_NAME_LEN_SSCANF
+ "s vid=%d pid=%d intf_id=%d i/p devices=0x%X o/p
devices=0x%X",
desc.name, &desc.vid, &desc.pid, &desc.intf_id,
&desc.ip_devices, &desc.op_devices);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/13] staging: greybus: Switch from strlcpy to strscpy
On 1/31/21 11:28 AM, Kumar Kartikeya Dwivedi wrote:
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
This is a good change. But while you're at it, I would
appreciate if you would convert a few spots to use
sizeof(dest) rather than a fixed constant. I will
point them out below.
If this is the *only* request for a change on your
series, please tell me that and I can sign off on
this without you implementing my suggestion. But
if you post a version 2, please do what I describe.
Thanks.
-Alex
---
drivers/staging/greybus/audio_helper.c | 2 +-
drivers/staging/greybus/audio_module.c | 2 +-
drivers/staging/greybus/audio_topology.c | 6 +++---
drivers/staging/greybus/power_supply.c | 2 +-
drivers/staging/greybus/spilib.c | 4 ++--
5 files changed, 8 insertions(+), 8 deletions(-)
. . .
diff --git a/drivers/staging/greybus/audio_module.c
b/drivers/staging/greybus/audio_module.c
index a243d60f0..0f9fdc077 100644
--- a/drivers/staging/greybus/audio_module.c
+++ b/drivers/staging/greybus/audio_module.c
@@ -342,7 +342,7 @@ static int gb_audio_probe(struct gb_bundle *bundle,
/* inform above layer for uevent */
dev_dbg(dev, "Inform set_event:%d to above layer\n", 1);
/* prepare for the audio manager */
- strlcpy(desc.name, gbmodule->name, GB_AUDIO_MANAGER_MODULE_NAME_LEN);
+ strscpy(desc.name, gbmodule->name, GB_AUDIO_MANAGER_MODULE_NAME_LEN);
Please use this here instead:
strscpy(desc.name, gbmodule->name, sizeof(desc.name));
desc.vid = 2; /* todo */
desc.pid = 3; /* todo */
desc.intf_id = gbmodule->dev_id;
diff --git a/drivers/staging/greybus/audio_topology.c
b/drivers/staging/greybus/audio_topology.c
index 662e3e8b4..e816e4db5 100644
--- a/drivers/staging/greybus/audio_topology.c
+++ b/drivers/staging/greybus/audio_topology.c
@@ -200,7 +200,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol
*kcontrol,
return -EINVAL;
name = gbaudio_map_controlid(module, data->ctl_id,
uinfo->value.enumerated.item);
- strlcpy(uinfo->value.enumerated.name, name, NAME_SIZE);
+ strscpy(uinfo->value.enumerated.name, name, NAME_SIZE);
Please use this here instead:
strscpy(uinfo->value.enumerated.name, name,
sizeof(uinfo->valiue.enumerated.name));
(I know NAME_SIZE is used throughout this file, and
could also be converted in this way, but we can save
that for another patch.)
break;
default:
dev_err(comp->dev, "Invalid type: %d for %s:kcontrol\n",
@@ -1047,7 +1047,7 @@ static int gbaudio_tplg_create_widget(struct
gbaudio_module_info *module,
}
/* Prefix dev_id to widget control_name */
- strlcpy(temp_name, w->name, NAME_SIZE);
+ strscpy(temp_name, w->name, NAME_SIZE);
Please use this here instead:
strscpy(temp_name, w->name, sizeof(temp_name));
snprintf(w->name, NAME_SIZE, "GB %d %s", module->dev_id, temp_name);
switch (w->type) {
@@ -1169,7 +1169,7 @@ static int gbaudio_tplg_process_kcontrols(struct
gbaudio_module_info *module,
}
control->id = curr->id;
/* Prefix dev_id to widget_name */
- strlcpy(temp_name, curr->name, NAME_SIZE);
+ strscpy(temp_name, curr->name, NAME_SIZE);
Please use this here instead:
strscpy(temp_name, curr->name, sizeof(temp_name));
snprintf(curr->name, NAME_SIZE, "GB %d %s", module->dev_id,
temp_name);
control->name = curr->name;
. . .
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: fix stack size warning with UBSAN
On 1/3/21 4:35 PM, Arnd Bergmann wrote:
From: Arnd Bergmann
clang warns about excessive stack usage in this driver when
UBSAN is enabled:
drivers/staging/greybus/audio_topology.c:977:12: error: stack frame size of
1836 bytes in function 'gbaudio_tplg_create_widget'
[-Werror,-Wframe-larger-than=]
Rework this code to no longer use compound literals for
initializing the structure in each case, but instead keep
the common bits in a preallocated constant array and copy
them as needed.
This is good, but I have a few comments.
You took out the default case, and it seems you are using
a w->type value bigger than the initialization array to
determine validity. But there are more values defined in
the snd_soc_dapm_type enumerated type than are explicitly
listed as cases in the switch statement. So the switch
statement no longer treats some types as invalid (such
as snd_soc_dapm_demux). Am I missing something?
You are setting explicit names, such as "spk", "hp",
"mic", etc. in the initialization array. But you
update the name after (struct) assigning from the
array. I have no real objection I guess, but why
bother? Why not just use null pointers in the
initialization array?
You change a semicolon to a comma in one spot, and you
should not have. I'll point that out below.
I like that you got rid of the type casts, which were
apparently unnecessary.
You dropped the break in the final case in the switch
statement, but in an earlier discussion I think we
concluded that wasn't a problem.
I guess the main thing is the first thing mentioned.
Thanks.
-Alex
Signed-off-by: Arnd Bergmann
---
drivers/staging/greybus/audio_topology.c | 106 ++-
1 file changed, 47 insertions(+), 59 deletions(-)
diff --git a/drivers/staging/greybus/audio_topology.c
b/drivers/staging/greybus/audio_topology.c
index 96b8b29fe899..c03873915c20 100644
--- a/drivers/staging/greybus/audio_topology.c
+++ b/drivers/staging/greybus/audio_topology.c
@@ -974,6 +974,44 @@ static int gbaudio_widget_event(struct snd_soc_dapm_widget
*w,
return ret;
}
+static const struct snd_soc_dapm_widget gbaudio_widgets[] = {
+ [snd_soc_dapm_spk] = SND_SOC_DAPM_SPK("spk", gbcodec_event_spk),
+ [snd_soc_dapm_hp] = SND_SOC_DAPM_HP("hp", gbcodec_event_hp),
+ [snd_soc_dapm_mic] = SND_SOC_DAPM_MIC("mic",
gbcodec_event_int_mic),
. . .
@@ -1050,78 +1088,28 @@ static int gbaudio_tplg_create_widget(struct
gbaudio_module_info *module,
strlcpy(temp_name, w->name, NAME_SIZE);
snprintf(w->name, NAME_SIZE, "GB %d %s", module->dev_id, temp_name);
+ if (w->type > ARRAY_SIZE(gbaudio_widgets)) {
+ ret = -EINVAL;
+ goto error;
+ }
+ *dw = gbaudio_widgets[w->type];
+ dw->name = w->name;
+
switch (w->type) {
case snd_soc_dapm_spk:
- *dw = (struct snd_soc_dapm_widget)
- SND_SOC_DAPM_SPK(w->name, gbcodec_event_spk);
module->op_devices |= GBAUDIO_DEVICE_OUT_SPEAKER;
break;
case snd_soc_dapm_hp:
- *dw = (struct snd_soc_dapm_widget)
- SND_SOC_DAPM_HP(w->name, gbcodec_event_hp);
module->op_devices |= (GBAUDIO_DEVICE_OUT_WIRED_HEADSET
- | GBAUDIO_DEVICE_OUT_WIRED_HEADPHONE);
+ | GBAUDIO_DEVICE_OUT_WIRED_HEADPHONE),
Please fix this (above) to preserve the original semicolon.
module->ip_devices |= GBAUDIO_DEVICE_IN_WIRED_HEADSET;
break;
case snd_soc_dapm_mic:
- *dw = (struct snd_soc_dapm_widget)
- SND_SOC_DAPM_MIC(w->name, gbcodec_event_int_mic);
module->ip_devices |= GBAUDIO_DEVICE_IN_BUILTIN_MIC;
break;
. . .
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 -next] staging: greybus: light: Use kzalloc for allocating only one thing
On 12/29/20 7:37 PM, Zheng Yongjun wrote: Use kzalloc rather than kcalloc(1,...) The semantic patch that makes this change is as follows: (http://coccinelle.lip6.fr/) // @@ @@ - kcalloc(1, + kzalloc( ...) // Signed-off-by: Zheng Yongjun Looks good. Reviewed-by: Alex Elder --- drivers/staging/greybus/light.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index d2672b65c3f4..87d36948c610 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -290,8 +290,7 @@ static int channel_attr_groups_set(struct gb_channel *channel, channel->attrs = kcalloc(size + 1, sizeof(*channel->attrs), GFP_KERNEL); if (!channel->attrs) return -ENOMEM; - channel->attr_group = kcalloc(1, sizeof(*channel->attr_group), - GFP_KERNEL); + channel->attr_group = kzalloc(sizeof(*channel->attr_group), GFP_KERNEL); if (!channel->attr_group) return -ENOMEM; channel->attr_groups = kcalloc(2, sizeof(*channel->attr_groups), ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] staging: greybus: light: Use kzalloc for allocating only one thing
On 12/29/20 7:50 AM, Zheng Yongjun wrote: Use kzalloc rather than kcalloc(1,...) The semantic patch that makes this change is as follows: (http://coccinelle.lip6.fr/) // @@ @@ - kcalloc(1, + kzalloc( ...) // Signed-off-by: Zheng Yongjun --- drivers/staging/greybus/light.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index d2672b65c3f4..d227382fca20 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -290,7 +290,7 @@ static int channel_attr_groups_set(struct gb_channel *channel, channel->attrs = kcalloc(size + 1, sizeof(*channel->attrs), GFP_KERNEL); if (!channel->attrs) return -ENOMEM; - channel->attr_group = kcalloc(1, sizeof(*channel->attr_group), + channel->attr_group = kzalloc(sizeof(*channel->attr_group), GFP_KERNEL); Looks good but that shortens the line enough to avoid a line wrap. Please send v2 with GFP_KERNEL on the same line as the kzalloc() call. -Alex if (!channel->attr_group) return -ENOMEM; ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
On 9/25/20 9:11 AM, Coiby Xu wrote:
On Thu, Sep 24, 2020 at 10:54:50AM +, 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-...@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/greybus-dev
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
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
+#include
/* 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 l
Re: [greybus-dev] [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t
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
> ---
> 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_LASTSNDRV_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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 2/3] staging: greybus: codecs: use SNDRV_PCM_FMTBIT_S16_LE for format bitmask
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
>
> $ 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
> ---
> 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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 1/3] [PATCH] staging: greybus: fix warnings about endianness detected by sparse
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
> $ 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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: issue with uninitialized value used in a comparison in gbcodec_mixer_dapm_ctl_put
On 7/30/20 11:02 AM, Colin Ian King wrote:
> Hi,
>
> Static analysis with Coverity has detected an uninitialized value being
> used in a comparison. The error was detected on a recent change to
> drivers/staging/greybus/audio_topology.c however the issue actually
> dates back to the original commit:
>
> commit 6339d2322c47f4b8ebabf9daf0130328ed72648b
> Author: Vaibhav Agarwal
> Date: Wed Jan 13 14:07:51 2016 -0700
>
> greybus: audio: Add topology parser for GB codec
>
> The analysis is as follows:
>
> 425 static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
> 426 struct snd_ctl_elem_value
> *ucontrol)
> 427 {
> 428int ret, wi, max, connect;
> 429unsigned int mask, val;
> 430struct gb_audio_ctl_elem_info *info;
> 431struct gbaudio_ctl_pvt *data;
>
>1. var_decl: Declaring variable gbvalue without initializer.
> 432struct gb_audio_ctl_elem_value gbvalue;
> 433struct gbaudio_module_info *module;
> 434struct snd_soc_dapm_widget_list *wlist =
> snd_kcontrol_chip(kcontrol);
> 435struct snd_soc_dapm_widget *widget = wlist->widgets[0];
> 436struct device *codec_dev = widget->dapm->dev;
> 437struct gbaudio_codec_info *gb = dev_get_drvdata(codec_dev);
> 438struct gb_bundle *bundle;
> 439
>
>2. Condition 0 /* __builtin_types_compatible_p() */, taking false branch.
>3. Condition 1 /* __builtin_types_compatible_p() */, taking true branch.
>4. Falling through to end of if statement.
>5. Condition !!branch, taking false branch.
>6. Condition ({...; !!branch;}), taking false branch.
>
> 440dev_dbg(codec_dev, "Entered %s:%s\n", __func__,
> kcontrol->id.name);
> 441module = find_gb_module(gb, kcontrol->id.name);
>
>7. Condition !module, taking false branch.
> 442if (!module)
> 443return -EINVAL;
> 444
> 445data = (struct gbaudio_ctl_pvt *)kcontrol->private_value;
> 446info = (struct gb_audio_ctl_elem_info *)data->info;
>
>8. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
> 447bundle = to_gb_bundle(module->dev);
> 448
>
>9. Condition data->vcount == 2, taking true branch.
> 449if (data->vcount == 2)
> 450dev_warn(widget->dapm->dev,
> 451 "GB: Control '%s' is stereo, which is not
> supported\n",
> 452 kcontrol->id.name);
> 453
> 454max = le32_to_cpu(info->value.integer.max);
> 455mask = (1 << fls(max)) - 1;
> 456val = ucontrol->value.integer.value[0] & mask;
>
>10. Condition !!val, taking true branch.
> 457connect = !!val;
> 458
> 459/* update ucontrol */
>
> Uninitialized scalar variable (UNINIT)
>11. uninit_use: Using uninitialized value gbvalue.value.integer_value[0].
> 460if (gbvalue.value.integer_value[0] != val) {
>
> The gbvalue.value.integer_value[0] read is bogus since gbvalue was
> declared on the stack but was not initialized. There seems to be no
> where that sets this data. I'm assuming most of the time that the
> comparison works because the garbage value is different from val and so
> the code in the if stanza is executed.
>
> Anyhow, I'm unsure what the original intent of the code was, so I've not
> attempted to fix this.
I think the fix is to add a call to this:
ret = gb_audio_gb_get_control(module->mgmt_connection, data->ctl_id,
GB_AUDIO_INVALID_INDEX, &gbvalue);
before the field within gbvalue is used.
Looking at gbcodec_mixer_dapm_ctl_get() defined just above that, it
seems that the call to gb_audio_gb_get_control() should be preceded
by a call to gb_pm_runtime_get_sync(). And given that duplication,
I suggest this call and the PM runtime wrapper functions should be
placed in a new helper function.
I know that Vaibhav said he would be fixing this, so I guess my
comments are directed at him. Thanks for sending the patch Colin.
-Alex
> Colin
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v4 1/7] staging: greybus: audio: Update snd_jack FW usage as per new APIs
On 7/9/20 5:27 AM, Vaibhav Agarwal wrote:
> snd_soc_jack APIs are modified in recent kernel versions. This patch
> updates the codec driver to resolve the compilation errors related to
> jack framework.
Greg has already accepted this series so I won't review this now. But
I still wanted to provide this comment.
It would be helpful in the future to provide a little more information
about the nature of the changes to these APIs. As a reviewer I had to
go track them down to get a little more context about what you are doing
here. So you could say something like:
Audio jacks are now registered at the card level rather than being
associated with a CODEC. The new card-based API allows a jack's pins
to be supplied when the jack is first registered. See: 970939964c26
("ASoC: Allow to register jacks at the card level")
In other words, don't just say "the APIs changed," say "here is how
the APIs have changed." This kind of introduction can be very helpful
and time saving for your reviewers.
-Alex
> Signed-off-by: Vaibhav Agarwal
> Reviewed-by: Dan Carpenter
> ---
> drivers/staging/greybus/audio_codec.c | 54 +--
> 1 file changed, 42 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_codec.c
> b/drivers/staging/greybus/audio_codec.c
> index 08746c85dea6..5d3a5e6a8fe6 100644
> --- a/drivers/staging/greybus/audio_codec.c
> +++ b/drivers/staging/greybus/audio_codec.c
> @@ -709,17 +709,26 @@ static struct snd_soc_dai_driver gbaudio_dai[] = {
> };
>
> static int gbaudio_init_jack(struct gbaudio_module_info *module,
> - struct snd_soc_codec *codec)
> + struct snd_soc_card *card)
> {
> int ret;
> + struct snd_soc_jack_pin *headset, *button;
>
> if (!module->jack_mask)
> return 0;
>
> snprintf(module->jack_name, NAME_SIZE, "GB %d Headset Jack",
>module->dev_id);
> - ret = snd_soc_jack_new(codec, module->jack_name, module->jack_mask,
> -&module->headset_jack);
> +
> + headset = devm_kzalloc(module->dev, sizeof(*headset), GFP_KERNEL);
> + if (!headset)
> + return -ENOMEM;
> +
> + headset->pin = module->jack_name;
> + headset->mask = module->jack_mask;
> +
> + ret = snd_soc_card_jack_new(card, module->jack_name, module->jack_mask,
> + &module->headset_jack, headset, 1);
> if (ret) {
> dev_err(module->dev, "Failed to create new jack\n");
> return ret;
> @@ -730,11 +739,21 @@ static int gbaudio_init_jack(struct gbaudio_module_info
> *module,
>
> snprintf(module->button_name, NAME_SIZE, "GB %d Button Jack",
>module->dev_id);
> - ret = snd_soc_jack_new(codec, module->button_name, module->button_mask,
> -&module->button_jack);
> + button = devm_kzalloc(module->dev, sizeof(*button), GFP_KERNEL);
> + if (!button) {
> + ret = -ENOMEM;
> + goto free_headset;
> + }
> +
> + button->pin = module->button_name;
> + button->mask = module->button_mask;
> +
> + ret = snd_soc_card_jack_new(card, module->button_name,
> + module->button_mask, &module->button_jack,
> + button, 1);
> if (ret) {
> dev_err(module->dev, "Failed to create button jack\n");
> - return ret;
> + goto free_headset;
> }
>
> /*
> @@ -750,7 +769,7 @@ static int gbaudio_init_jack(struct gbaudio_module_info
> *module,
> KEY_MEDIA);
> if (ret) {
> dev_err(module->dev, "Failed to set BTN_0\n");
> - return ret;
> + goto free_button;
> }
> }
>
> @@ -759,7 +778,7 @@ static int gbaudio_init_jack(struct gbaudio_module_info
> *module,
> KEY_VOICECOMMAND);
> if (ret) {
> dev_err(module->dev, "Failed to set BTN_1\n");
> - return ret;
> + goto free_button;
> }
> }
>
> @@ -768,7 +787,7 @@ static int gbaudio_init_jack(struct gbaudio_module_info
> *module,
> KEY_VOLUMEUP);
> if (ret) {
> dev_err(module->dev, "Failed to set BTN_2\n");
> - return ret;
> + goto free_button;
> }
> }
>
> @@ -777,7 +796,7 @@ static int gbaudio_init_jack(struct gbaudio_module_info
> *module,
> KEY_VOLUMEDOWN);
> if (ret) {
> dev_err(module->dev, "Failed to set BTN_0\n");
> - return ret;
> + goto free_button;
> }
>
Re: [greybus-dev] [PATCH] staging: greybus: audio: Uninitialized variable in gbaudio_remove_controls()
On 8/4/20 5:16 AM, Dan Carpenter wrote:
> The "err" variable is not meaningful so there is no need to print it.
> It's uninitialized on the first iteration through the loop.
>
> Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic
> audio modules")
> Signed-off-by: Dan Carpenter
This is a good fix, thanks.
Reviewed-by: Alex Elder
> ---
> drivers/staging/greybus/audio_helper.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_helper.c
> b/drivers/staging/greybus/audio_helper.c
> index 8b100a71f02e..237531ba60f3 100644
> --- a/drivers/staging/greybus/audio_helper.c
> +++ b/drivers/staging/greybus/audio_helper.c
> @@ -173,8 +173,7 @@ static int gbaudio_remove_controls(struct snd_card *card,
> struct device *dev,
> id.index = control->index;
> kctl = snd_ctl_find_id(card, &id);
> if (!kctl) {
> - dev_err(dev, "%d: Failed to find %s\n", err,
> - control->name);
> + dev_err(dev, "Failed to find %s\n", control->name);
> continue;
> }
> err = snd_ctl_remove(card, kctl);
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: loopback: fix a spelling error.
On 5/25/20 1:10 AM, Till Varoquaux wrote:
Successed -> succeeded.
Signed-off-by: Till Varoquaux
Looks good.
Reviewed-by: Alex Elder
---
drivers/staging/greybus/loopback.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/greybus/loopback.c
b/drivers/staging/greybus/loopback.c
index 583d9708a191..2471448ba42a 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -135,7 +135,7 @@ static ssize_t name##_##field##_show(struct device *dev,
\
char *buf) \
{ \
struct gb_loopback *gb = dev_get_drvdata(dev); \
- /* Report 0 for min and max if no transfer successed */ \
+ /* Report 0 for min and max if no transfer succeeded */ \
if (!gb->requests_completed) \
return sprintf(buf, "0\n"); \
return sprintf(buf, "%" #type "\n", gb->name.field); \
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] greybus: uart: fix uninitialized flow control variable
On 4/29/20 2:00 PM, Arnd Bergmann wrote:
> gcc-10 points out an uninitialized variable use:
Wow, nice, checking individual uninitialized fields within
the structure.
The structure should really be zero-initialized anyway; it's
passed as a structure in a message elsewhere. With your
change, all fields in the structure are written, but in
theory the structure could change and stack garbage could
be sent over the wire.
What do you think of doing this instead? Or in addition?
struct gb_tty_line_coding newline = { };
(Presumably that would also silence the warning.)
I endorse of your change, either way.
-Alex
> drivers/staging/greybus/uart.c: In function 'gb_tty_set_termios':
> drivers/staging/greybus/uart.c:540:24: error: 'newline.flow_control' is used
> uninitialized in this function [-Werror=uninitialized]
> 540 | newline.flow_control |= GB_SERIAL_AUTO_RTSCTS_EN;
>
> Instead of using |= and &= on the uninitialized variable, use a
> direct assignment.
>
> Fixes: e55c25206d5c ("greybus: uart: Handle CRTSCTS flag in termios")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/staging/greybus/uart.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
> index 55c51143bb09..4ffb334cd5cd 100644
> --- a/drivers/staging/greybus/uart.c
> +++ b/drivers/staging/greybus/uart.c
> @@ -537,9 +537,9 @@ static void gb_tty_set_termios(struct tty_struct *tty,
> }
>
> if (C_CRTSCTS(tty) && C_BAUD(tty) != B0)
> - newline.flow_control |= GB_SERIAL_AUTO_RTSCTS_EN;
> + newline.flow_control = GB_SERIAL_AUTO_RTSCTS_EN;
> else
> - newline.flow_control &= ~GB_SERIAL_AUTO_RTSCTS_EN;
> + newline.flow_control = 0;
>
> if (memcmp(&gb_tty->line_coding, &newline, sizeof(newline))) {
> memcpy(&gb_tty->line_coding, &newline, sizeof(newline));
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: tools: Fix braces {} style
On 3/22/20 12:30 PM, Simran Singhal wrote:
> This patch fixes the check reported by checkpatch.pl
> for braces {} should be used on all arms of this statement.
>
> Signed-off-by: Simran Singhal
Looks fine to me. And I saw no other instances of this in the
Greybus code. Thanks for the patch.
Reviewed-by: Alex Elder
> ---
> drivers/staging/greybus/tools/loopback_test.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/tools/loopback_test.c
> b/drivers/staging/greybus/tools/loopback_test.c
> index ba6f905f26fa..d46721502897 100644
> --- a/drivers/staging/greybus/tools/loopback_test.c
> +++ b/drivers/staging/greybus/tools/loopback_test.c
> @@ -801,8 +801,9 @@ static void prepare_devices(struct loopback_test *t)
> write_sysfs_val(t->devices[i].sysfs_entry,
> "outstanding_operations_max",
> t->async_outstanding_operations);
> - } else
> + } else {
> write_sysfs_val(t->devices[i].sysfs_entry, "async", 0);
> + }
> }
> }
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: Replace zero-length array with flexible-array member
On 2/11/20 4:47 PM, Gustavo A. R. Silva wrote:
>
>
> On 2/11/20 16:15, Alex Elder wrote:
>> On 2/11/20 3:12 PM, Gustavo A. R. Silva wrote:
>>> The current codebase makes use of the zero-length array language
>>> extension to the C90 standard, but the preferred mechanism to declare
>>> variable-length types such as these ones is a flexible array member[1][2],
>>> introduced in C99:
>>>
>>> struct foo {
>>> int stuff;
>>> struct boo array[];
>>> };
>>>
>>> By making use of the mechanism above, we will get a compiler warning
>>> in case the flexible array does not occur last in the structure, which
>>> will help us prevent some kind of undefined behavior bugs from being
>>> inadvertenly introduced[3] to the codebase from now on.
>>>
>>> This issue was found with the help of Coccinelle.
>>>
>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
>>> [2] https://github.com/KSPP/linux/issues/21
>>> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>>>
>>> Signed-off-by: Gustavo A. R. Silva
>>> ---
>>> drivers/staging/greybus/raw.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c
>>> index 838acbe84ca0..2b301b2aa107 100644
>>> --- a/drivers/staging/greybus/raw.c
>>> +++ b/drivers/staging/greybus/raw.c
>>> @@ -30,7 +30,7 @@ struct gb_raw {
>>> struct raw_data {
>>> struct list_head entry;
>>> u32 len;
>>> - u8 data[0];
>>> + u8 data[];
>>> };
>>>
>>> static struct class *raw_class;
>>>
>>
>> Does the kamlloc() call in receive_data() have any problems
>> with the sizeof(*raw_data) passed as its argument?
>>
>
> Not in this case. It'd be different with a one-element array (u8 data[1]),
> though.
>
>> I'm not entirely sure what sizeof(struct-with-flexible-array-member)
>> produces.
>>
>
> The same as sizeof(struct-with-zero-length-array):
>
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
I saw that, but I wondered what the standard says (and
whether Clang produces the same result).
I found this in a draft standard, and I guess we can
assume it applies here:
"...the size of the structure is as if the flexible
array member were omitted except that it may have
more trailing padding than the omission would imply."
Looks good to me.
Reviewed-by: Alex Elder
>
> --
> Gustavo
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: Replace zero-length array with flexible-array member
On 2/11/20 3:12 PM, Gustavo A. R. Silva wrote:
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertenly introduced[3] to the codebase from now on.
>
> This issue was found with the help of Coccinelle.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/staging/greybus/raw.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c
> index 838acbe84ca0..2b301b2aa107 100644
> --- a/drivers/staging/greybus/raw.c
> +++ b/drivers/staging/greybus/raw.c
> @@ -30,7 +30,7 @@ struct gb_raw {
> struct raw_data {
> struct list_head entry;
> u32 len;
> - u8 data[0];
> + u8 data[];
> };
>
> static struct class *raw_class;
>
Does the kamlloc() call in receive_data() have any problems
with the sizeof(*raw_data) passed as its argument?
I'm not entirely sure what sizeof(struct-with-flexible-array-member)
produces.
-Alex
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: bootrom: fix uninitialized variables
On 1/25/20 6:14 AM, SAURAV GIREPUNJE wrote: > On 25/01/20 11:00 +0100, Johan Hovold wrote: >> On Sat, Jan 25, 2020 at 02:14:03PM +0530, Saurav Girepunje wrote: >>> fix uninitialized variables issue found using static code analysis tool >> >> Which tool is that? >> >>> (error) Uninitialized variable: offset >>> (error) Uninitialized variable: size >>> >>> Signed-off-by: Saurav Girepunje >>> --- >>> drivers/staging/greybus/bootrom.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/staging/greybus/bootrom.c >>> b/drivers/staging/greybus/bootrom.c >>> index a8efb86..9eabeb3 100644 >>> --- a/drivers/staging/greybus/bootrom.c >>> +++ b/drivers/staging/greybus/bootrom.c >>> @@ -245,7 +245,7 @@ static int gb_bootrom_get_firmware(struct gb_operation >>> *op) >>> struct gb_bootrom_get_firmware_request *firmware_request; >>> struct gb_bootrom_get_firmware_response *firmware_response; >>> struct device *dev = &op->connection->bundle->dev; >>> - unsigned int offset, size; >>> + unsigned int offset = 0, size = 0; >>> enum next_request_type next_request; >>> int ret = 0; >> >> I think this has come up in the past, and while the code in question is >> overly complicated and confuses static checkers as well as humans, it >> looks correct to me. >> >> Please make sure to verify the output of any tools before posting >> patches based on them. >> >> Johan > I used cppcheck tool . Implied in Johan's question is a suggestion. When you propose a patch that addresses something flagged by a tool of some kind, it is good practice to identify the tool in the patch description, and even better, give an example of how the tool was invoked when reported the problem you're fixing. Sometimes people even include the output of the tool, though I think that can sometimes be a bit much. And as you have now heard several times, do not blindly trust the output of these tools. They're intended to call attention to things for you to examine; they are no match for a human, and things they tell you about are not guaranteed to be real problems. -Alex > ___ > greybus-dev mailing list > greybus-...@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/greybus-dev ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: add missing includes
On 8/27/19 10:53 AM, Rui Miguel Silva wrote: > Before moving greybus core out of staging and moving header files to > include/linux some greybus header files were missing the necessary > includes. This would trigger compilation faillures with some example > errors logged bellow for with CONFIG_KERNEL_HEADER_TEST=y. > > So, add the necessary headers to compile clean before relocating the > header files. This looks good to me; I trust you compiled it. There is one extra blank line you added in "operation.h" but that's not important. I don't think what I've done here serves as a real review, so: Acked-by: Alex Elder -Alex > > ./include/linux/greybus/hd.h:23:50: error: unknown type name 'u16' > int (*cport_disable)(struct gb_host_device *hd, u16 cport_id); ^~~ > ./include/linux/greybus/greybus_protocols.h:1314:2: error: unknown type name > '__u8' > __u8 data[0]; > ^~~~ > ./include/linux/greybus/hd.h:24:52: error: unknown type name 'u16' > int (*cport_connected)(struct gb_host_device *hd, u16 cport_id); ^~~ > ./include/linux/greybus/hd.h:25:48: error: unknown type name 'u16' > int (*cport_flush)(struct gb_host_device *hd, u16 cport_id); ^~~ > ./include/linux/greybus/hd.h:26:51: error: unknown type name 'u16' > int (*cport_shutdown)(struct gb_host_device *hd, u16 cport_id, ^~~ > ./include/linux/greybus/hd.h:27:5: error: unknown type name 'u8' > u8 phase, unsigned int timeout); > ^~ > ./include/linux/greybus/hd.h:28:50: error: unknown type name 'u16' > int (*cport_quiesce)(struct gb_host_device *hd, u16 cport_id, ^~~ > ./include/linux/greybus/hd.h:29:5: error: unknown type name 'size_t' > size_t peer_space, unsigned int timeout); > ^~ > ./include/linux/greybus/hd.h:29:5: note: 'size_t' is defined in header > ''; did you forget to '#include '? > ./include/linux/greybus/hd.h:1:1: > +#include > /* SPDX-License-Identifier: GPL-2.0 */ > ./include/linux/greybus/hd.h:29:5: > size_t peer_space, unsigned int timeout); > ^~ > ./include/linux/greybus/hd.h:30:48: error: unknown type name 'u16' > int (*cport_clear)(struct gb_host_device *hd, u16 cport_id); ^~~ > ./include/linux/greybus/hd.h:32:49: error: unknown type name 'u16' > int (*message_send)(struct gb_host_device *hd, u16 dest_cport_id, ^~~ > ./include/linux/greybus/hd.h:33:32: error: unknown type name 'gfp_t' > struct gb_message *message, gfp_t gfp_mask); ^ > ./include/linux/greybus/hd.h:35:55: error: unknown type name 'u16' > int (*latency_tag_enable)(struct gb_host_device *hd, u16 cport_id); > > Signed-off-by: Rui Miguel Silva > Signed-off-by: Rui Miguel Silva > --- > drivers/staging/greybus/bundle.h| 3 +++ > drivers/staging/greybus/connection.h| 3 +++ > drivers/staging/greybus/control.h | 3 +++ > drivers/staging/greybus/greybus_manifest.h | 3 +++ > drivers/staging/greybus/greybus_protocols.h | 2 ++ > drivers/staging/greybus/hd.h| 3 +++ > drivers/staging/greybus/interface.h | 3 +++ > drivers/staging/greybus/manifest.h | 2 ++ > drivers/staging/greybus/module.h| 3 +++ > drivers/staging/greybus/operation.h | 5 + > drivers/staging/greybus/svc.h | 3 +++ > 11 files changed, 33 insertions(+) > > diff --git a/drivers/staging/greybus/bundle.h > b/drivers/staging/greybus/bundle.h > index 8734d2055657..69fe5610bb42 100644 > --- a/drivers/staging/greybus/bundle.h > +++ b/drivers/staging/greybus/bundle.h > @@ -9,7 +9,10 @@ > #ifndef __BUNDLE_H > #define __BUNDLE_H > > +#include > #include > +#include > +#include > > #define BUNDLE_ID_NONE U8_MAX > > diff --git a/drivers/staging/greybus/connection.h > b/drivers/staging/greybus/connection.h > index 5ca3befc0636..d59b7fc1de3e 100644 > --- a/drivers/staging/greybus/connection.h > +++ b/drivers/staging/greybus/connection.h > @@ -9,8 +9,11 @@ > #ifndef __CONNECTION_H > #define __CONNECTION_H > > +#include > #include > #include > +#include > +#include > > #define GB_CONNECTION_FLAG_CSD BIT(0) > #define GB_CONNECTION_FLAG_NO_FLOWCTRL BIT(1) > diff --git a/drivers/staging/greybus/control.h > b/drivers/staging/greybus/control.h > index 3a29ec05f631..0d4e2ed20fe4 100644 > --- a/drivers/staging/greybus/control.h > +++ b/drivers/staging/greybus/control.h > @@ -9,6 +9,9 @@ > #ifndef __CONTROL_H > #define __CONTROL_H > > +#include > +#include > + > struct gb_contro
Re: [greybus-dev] [PATCH 0/9] staging: move greybus core out of staging
On 8/25/19 12:54 AM, Greg Kroah-Hartman wrote:
> The Greybus code has long been "stable" but was living in the
> drivers/staging/ directory to see if there really was going to be
> devices using this protocol over the long-term. With the success of
> millions of phones with this hardware and code in it, and the recent
> Google Summer of Code project, and a number of of new devices in the
> works from various companies, it is time to finally move this code out
> of staging into the "real" portion of the kernel so that people know
> they can rely on it.
>
> This series first does a little bit of checkpatch cleanups for some
> basic remaining issues in the greybus files, and then moves the include
> directory, the greybus core code, and the es2 greybus host controller
> driver into drivers/greybus.
>
> To come after this is the movement of the Documentation entries and a
> number of the module drivers that are stable.
They all look good to me. (I don't always agree with checkpatch,
but standardization is good.) Thanks Greg.
Acked-by: Alex Elder
> Greg Kroah-Hartman (9):
> staging: greybus: fix up SPDX comment in .h files
> staging: greybus: remove license "boilerplate"
> staging: greybus: hd: Fix up some alignment checkpatch issues
> staging: greybus: manifest: Fix up some alignment checkpatch issues
> staging: greybus: log: Fix up some alignment checkpatch issues
> staging: greybus: loopback: Fix up some alignment checkpatch issues
> staging: greybus: move core include files to include/linux/greybus/
> staging: greybus: move the greybus core to drivers/greybus
> staging: greybus: move es2 to drivers/greybus/
>
> MAINTAINERS | 3 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/greybus/Kconfig | 32 +
> drivers/greybus/Makefile | 26 +
> drivers/greybus/arpc.h| 63 ++
> drivers/{staging => }/greybus/bundle.c| 2 +-
> drivers/{staging => }/greybus/connection.c| 2 +-
> drivers/{staging => }/greybus/control.c | 2 +-
> drivers/{staging => }/greybus/core.c | 2 +-
> drivers/{staging => }/greybus/debugfs.c | 3 +-
> drivers/{staging => }/greybus/es2.c | 3 +-
> drivers/{staging => }/greybus/greybus_trace.h | 2 +-
> drivers/{staging => }/greybus/hd.c| 12 +-
> drivers/{staging => }/greybus/interface.c | 2 +-
> drivers/{staging => }/greybus/manifest.c | 41 ---
> drivers/{staging => }/greybus/module.c| 2 +-
> drivers/{staging => }/greybus/operation.c | 2 +-
> drivers/{staging => }/greybus/svc.c | 3 +-
> drivers/{staging => }/greybus/svc_watchdog.c | 2 +-
> .../Documentation/firmware/authenticate.c | 46
> .../greybus/Documentation/firmware/firmware.c | 46
> drivers/staging/greybus/Kconfig | 27 -
> drivers/staging/greybus/Makefile | 22
> drivers/staging/greybus/arche-platform.c | 2 +-
> drivers/staging/greybus/arpc.h| 109 --
> drivers/staging/greybus/audio_apbridgea.c | 3 +-
> drivers/staging/greybus/audio_apbridgea.h | 26 +
> drivers/staging/greybus/audio_codec.h | 4 +-
> drivers/staging/greybus/audio_gb.c| 4 +-
> drivers/staging/greybus/authentication.c | 3 +-
> drivers/staging/greybus/bootrom.c | 2 +-
> drivers/staging/greybus/camera.c | 2 +-
> drivers/staging/greybus/firmware.h| 4 +-
> drivers/staging/greybus/fw-core.c | 2 +-
> drivers/staging/greybus/fw-download.c | 2 +-
> drivers/staging/greybus/fw-management.c | 2 +-
> drivers/staging/greybus/gb-camera.h | 2 +-
> drivers/staging/greybus/gbphy.c | 2 +-
> drivers/staging/greybus/gbphy.h | 2 +-
> drivers/staging/greybus/gpio.c| 2 +-
> .../staging/greybus/greybus_authentication.h | 48 +---
> drivers/staging/greybus/greybus_firmware.h| 48 +---
> drivers/staging/greybus/hid.c | 3 +-
> drivers/staging/greybus/i2c.c | 2 +-
> drivers/staging/greybus/light.c | 4 +-
> drivers/staging/greybus/log.c | 9 +-
> drivers/staging/greybus/loopback.c| 9 +-
> drivers/staging/greybus/power_supply.c| 3 +-
> drivers/staging/greybus/pwm.c | 2 +-
> drivers/staging/greybus/raw.c | 3
Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
On 4/16/19 5:13 PM, Madhumitha Prabakaran wrote:
> Fix a blank line after structure declarations. Also, convert
> macros into inline functions in order to maintain Linux kernel
> coding style based on which the inline function is
> preferable over the macro.
Madhumitha, here is my explanation for why *not* to convert these
container_of() macros to inline functions. It's not just because
"we do this all over the kernel." The reason is that it actually
does not improve the code.
Inline functions are often better than macros because they are
explicit about types, allowing the compiler to tell you if you
are passing parameters of the right type, and possibly assigning
results to objects of the right type.
Here is the definition of the container_of() macro in :
#define container_of(ptr, type, member) ({ \
const typeof(((type *)0)->member) * __mptr = (ptr); \
(type *)((char *)__mptr - offsetof(type, member)); })
You see that ptr is explicitly assigned to a local variable
of the type of the member field, so the compiler is able to
check that assignment. And the return value is similarly
cast to the type of the containing structure, so the type
compatibility of the assignment can also be checked. It is
assumed that where this macro is used, the caller knows it
is passing an appropriate address.
There is another thing about this particular definition make
it just as good as an inline function. A macro expansion can
result in one of its parameters being used more than once, and
that allows those parameters to be *evaluated* more than once
in a single statement, which can produce incorrect code.
This macro is defined using the "statement expression"
extension to C--where a compound statement is enclosed in
parentheses--({ ... }). This allows a local variable to be
used in the macro expansion, which avoids any reuse of the
macro parameters which might cause side-effects.
So anyway, I don't think there is any benefit to replacing
macros like this that do container_of() with inline functions.
-Alex
> Blank line fixes are suggested by checkpatch.pl
>
> Signed-off-by: Madhumitha Prabakaran
>
> Changes in v2:
> - To maintain consistency in driver greybus, all the instances of macro
> with container_of are fixed in a single patch.
> ---
> drivers/staging/greybus/bundle.h| 6 +-
> drivers/staging/greybus/control.h | 6 +-
> drivers/staging/greybus/gbphy.h | 12 ++--
> drivers/staging/greybus/greybus.h | 6 +-
> drivers/staging/greybus/hd.h| 6 +-
> drivers/staging/greybus/interface.h | 6 +-
> drivers/staging/greybus/module.h| 6 +-
> drivers/staging/greybus/svc.h | 6 +-
> 8 files changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/greybus/bundle.h
> b/drivers/staging/greybus/bundle.h
> index 8734d2055657..84956eedb1c4 100644
> --- a/drivers/staging/greybus/bundle.h
> +++ b/drivers/staging/greybus/bundle.h
> @@ -31,7 +31,11 @@ struct gb_bundle {
>
> struct list_headlinks; /* interface->bundles */
> };
> -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
> +
> +static inline struct gb_bundle *to_gb_bundle(struct device *d)
> +{
> + return container_of(d, struct gb_bundle, dev);
> +}
>
> /* Greybus "private" definitions" */
> struct gb_bundle *gb_bundle_create(struct gb_interface *intf, u8 bundle_id,
> diff --git a/drivers/staging/greybus/control.h
> b/drivers/staging/greybus/control.h
> index 3a29ec05f631..a681ef74e7fe 100644
> --- a/drivers/staging/greybus/control.h
> +++ b/drivers/staging/greybus/control.h
> @@ -24,7 +24,11 @@ struct gb_control {
> char *vendor_string;
> char *product_string;
> };
> -#define to_gb_control(d) container_of(d, struct gb_control, dev)
> +
> +static inline struct gb_control *to_gb_control(struct device *d)
> +{
> + return container_of(d, struct gb_control, dev);
> +}
>
> struct gb_control *gb_control_create(struct gb_interface *intf);
> int gb_control_enable(struct gb_control *control);
> diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h
> index 99463489d7d6..20307f6dfcb9 100644
> --- a/drivers/staging/greybus/gbphy.h
> +++ b/drivers/staging/greybus/gbphy.h
> @@ -15,7 +15,11 @@ struct gbphy_device {
> struct list_head list;
> struct device dev;
> };
> -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> +
> +static inline struct gbphy_device *to_gbphy_dev(struct device *d)
> +{
> + return container_of(d, struct gbphy_device, dev);
> +}
>
> static inline void *gb_gbphy_get_data(struct gbphy_device *gdev)
> {
> @@ -43,7 +47,11 @@ struct gbphy_driver {
>
> struct device_driver driver;
> };
> -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver)
> +
> +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver *d)
> +{
> + return cont
Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
On 4/17/19 1:25 AM, Greg KH wrote:
> On Tue, Apr 16, 2019 at 05:13:18PM -0500, Madhumitha Prabakaran wrote:
>> Fix a blank line after structure declarations. Also, convert
>> macros into inline functions in order to maintain Linux kernel
>> coding style based on which the inline function is
>> preferable over the macro.
>>
>> Blank line fixes are suggested by checkpatch.pl
>>
>> Signed-off-by: Madhumitha Prabakaran
>>
>> Changes in v2:
>> - To maintain consistency in driver greybus, all the instances of macro
>> with container_of are fixed in a single patch.
>> ---
>> drivers/staging/greybus/bundle.h| 6 +-
>> drivers/staging/greybus/control.h | 6 +-
>> drivers/staging/greybus/gbphy.h | 12 ++--
>> drivers/staging/greybus/greybus.h | 6 +-
>> drivers/staging/greybus/hd.h| 6 +-
>> drivers/staging/greybus/interface.h | 6 +-
>> drivers/staging/greybus/module.h| 6 +-
>> drivers/staging/greybus/svc.h | 6 +-
>> 8 files changed, 45 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/bundle.h
>> b/drivers/staging/greybus/bundle.h
>> index 8734d2055657..84956eedb1c4 100644
>> --- a/drivers/staging/greybus/bundle.h
>> +++ b/drivers/staging/greybus/bundle.h
>> @@ -31,7 +31,11 @@ struct gb_bundle {
>>
>> struct list_headlinks; /* interface->bundles */
>> };
>> -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
>> +
>> +static inline struct gb_bundle *to_gb_bundle(struct device *d)
>> +{
>> +return container_of(d, struct gb_bundle, dev);
>> +}
>
> Why are we changing this to an inline function? The #define is fine,
> and how we "normally" do this type of container_of conversion.
I'm not completely sure about the inline function, but on the no blank
lines thing (and many other minor issues) "checkpatch.pl" is to blame.
There are lots of examples of issues that checkpatch points out that are
matters of opinion and not hardened kernel style rules.
We try to encourage people to get involved with kernel development by
fixing minor problems, and we tell them a good way to find them is
by running checkpatch and "fixing" what it reports. Unfortunately,
it is often things of this type, and reviewers balk and say "no,
please leave it," and the poor new person has a bad first experience.
I *like* "checkpatch.pl". And the fact that it can point out some
of these sorts of things is great. But it would be nice if certain
types of problems (like multiple blank lines, or lines that are 81
characters wide for example) would only be reported when a "--strict"
option or something were supplied.
-Alex
> I understand the objection of the "no blank line", but this was the
> "original" style that we used to create these #defines from the very
> beginning of the driver model work a decade ago. Changing that
> muscle-memory is going to be hard for some of us. Look at
> drivers/base/base.h for other examples of this.
>
> thanks,
>
> greg k-h
> ___
> greybus-dev mailing list
> greybus-...@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] Staging: greybus: Fix spinlock_t definition without comment
On 4/5/19 3:53 PM, Dan Carpenter wrote:
> On Fri, Apr 05, 2019 at 03:00:46PM -0500, Madhumitha Prabakaran
> wrote:
>> Fix spinlock_t definition without comment.
>>
>> Signed-off-by: Madhumitha Prabakaran
Madhumitha, the reason one would want a comment associated with
a lock field in a structure is to get some understanding of why
it's needed. Saying "protect structure fields" is not enough,
because that can pretty much be assumed, so a comment like that
adds no value.
Looking at the code, you can see the lock field protects the
connection's operations list. It also appears to be needed
for accessing the state field (reading or updating).
Given that, a better comment might be:
spinlock_t lock; /* operations list and state */
>> --- drivers/staging/greybus/connection.h | 2 +- 1 file changed, 1
>> insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/greybus/connection.h
>> b/drivers/staging/greybus/connection.h index
>> 5ca3befc0636..0aedd246e94a 100644 ---
>> a/drivers/staging/greybus/connection.h +++
>> b/drivers/staging/greybus/connection.h @@ -47,7 +47,7 @@ struct
>> gb_connection { unsigned longflags;
>>
>> struct mutex mutex; -spinlock_t
>> lock; + spinlock_t lock; /*
>> Protect structure fields */ enum gb_connection_state state;
>
> What does the mutex do then? Why can't we just use the spinlock for
> everything?
The mutex needs to be held during enable and disable of a connection.
Johan might be able to give you a more complete answer but these
operations (or at least the enable) need to block, so can't hold a
spinlock.
-Alex
> I did glance at the code and it wasn't immediately obvious to me.
>
> regards, dan carpenter
>
> ___ greybus-dev mailing
> list greybus-...@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] Staging: greybus: usb: Fixed a coding style error
On 4/1/19 9:22 AM, Will Cunningham wrote:
> Line was >80 characters.
This looks fine, but "tmp" is not a meaningful name.
That argument to gb_connection_create() is a cport id,
so "cport_id" would be a much better name for the variable.
It seems picky, but details like this make the code much
more understandable.
-Alex
> Signed-off-by: Will Cunningham
> ---
> Changes in v2:
> - Created a tmp variable to shorten line length.
> ---
> drivers/staging/greybus/usb.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
> index 1c246c73a085..c09793b48850 100644
> --- a/drivers/staging/greybus/usb.c
> +++ b/drivers/staging/greybus/usb.c
> @@ -163,14 +163,14 @@ static int gb_usb_probe(struct gbphy_device *gbphy_dev,
> struct gb_usb_device *gb_usb_dev;
> struct usb_hcd *hcd;
> int retval;
> + u16 tmp;
>
> hcd = usb_create_hcd(&usb_gb_hc_driver, dev, dev_name(dev));
> if (!hcd)
> return -ENOMEM;
>
> - connection = gb_connection_create(gbphy_dev->bundle,
> -
> le16_to_cpu(gbphy_dev->cport_desc->id),
> - NULL);
> + tmp = le16_to_cpu(gbphy_dev->cport_desc->id);
> + connection = gb_connection_create(gbphy_dev->bundle, tmp, NULL);
> if (IS_ERR(connection)) {
> retval = PTR_ERR(connection);
> goto exit_usb_put;
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] Staging: greybus: usb: Fixed a coding style error
On 3/31/19 1:40 AM, Joe Perches wrote: > On Sun, 2019-03-31 at 01:20 -0500, Alex Elder wrote: >> On 3/31/19 1:04 AM, Joe Perches wrote: >>> Blind adherence to 80 column limits leads to poor looking >>> code. Especially with longish identifier lengths. >> I agree. If it were me, I'd use a local variable. For example: >> >> struct greybus_descriptor_cport *cport_desc = gbphy_dev->cport_desc; >> ... >> connection = gb_connection_create(gbphy_dev->bundle, >>le16_to_cpu(cport_desc->id), NULL); >> >> Or maybe better: >> >> u16 cport_id = le16_to_cpu(gbphy_dev->cport_desc->id); >> ... >> connection = gb_connection_create(gbphy_dev->bundle, cport_id, NULL); > > True. > > A possible negative though: > > Temporaries that are only used once are sometimes > less readable as the declaration is supposed to be > done at an open brace and that could be relatively > far away from the set and use. Then assign it where it's used. The point is we're talking about a readability issue (long lines), and no matter how you try to fix it there are tradeoffs, and it's subjective. In any case, I prefer the use of the local variable to solve this readability problem over splitting the line. -Alex ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] Staging: greybus: usb: Fixed a coding style error
On 3/31/19 1:04 AM, Joe Perches wrote: > On Sun, 2019-03-31 at 01:30 -0400, Will Cunningham wrote: >> Line was >80 characters. > [] >> diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c > [] >> @@ -169,8 +169,8 @@ static int gb_usb_probe(struct gbphy_device *gbphy_dev, >> return -ENOMEM; >> >> connection = gb_connection_create(gbphy_dev->bundle, >> - >> le16_to_cpu(gbphy_dev->cport_desc->id), >> - NULL); >> +le16_to_cpu(gbphy_dev->cport_desc->id), >> +NULL); > > Blind adherence to 80 column limits leads to poor looking > code. Especially with longish identifier lengths. I agree. If it were me, I'd use a local variable. For example: struct greybus_descriptor_cport *cport_desc = gbphy_dev->cport_desc; ... connection = gb_connection_create(gbphy_dev->bundle, le16_to_cpu(cport_desc->id), NULL); Or maybe better: u16 cport_id = le16_to_cpu(gbphy_dev->cport_desc->id); ... connection = gb_connection_create(gbphy_dev->bundle, cport_id, NULL); -Alex > > Another way to do this, which is not necessarily actually > better is to position the left side of the assignment on a > separate line like: > > connection = >gb_connection_create(gbphy_dev->bundle, > le16_to_cpu(gbphy_dev->cport_desc->id), > > Is that better?I prefer the original. > It was better before as it was aligned to open parenthesis. > > > ___ > greybus-dev mailing list > greybus-...@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/greybus-dev > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: fix spelling mistake "entires" -> "entries"
On 09/14/2018 06:24 AM, Colin King wrote: > From: Colin Ian King > > Trivial fix to spelling mistake I hate to have two-character fixes to documentation like this. I.e., as long as you're touching the README file it might have been nice to review it and look for other things. I suspect you found this in the program output though, and also noticed it in the README file. So... Looks good to me. Reviewed-by: Alex Elder > > Signed-off-by: Colin Ian King > --- > drivers/staging/greybus/tools/README.loopback | 2 +- > drivers/staging/greybus/tools/loopback_test.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/greybus/tools/README.loopback > b/drivers/staging/greybus/tools/README.loopback > index 845b08dc4696..070a510cbe7c 100644 > --- a/drivers/staging/greybus/tools/README.loopback > +++ b/drivers/staging/greybus/tools/README.loopback > @@ -79,7 +79,7 @@ Here is the summary of the available options: > -t must be one of the test names - sink, transfer or ping > -i iteration count - the number of iterations to run the test over > Optional arguments > - -S sysfs location - location for greybus 'endo' entires default > /sys/bus/greybus/devices/ > + -S sysfs location - location for greybus 'endo' entries default > /sys/bus/greybus/devices/ > -D debugfs location - location for loopback debugfs entries default > /sys/kernel/debug/gb_loopback/ > -s size of data packet to send during test - defaults to zero > -m mask - a bit mask of connections to include example: -m 8 = 4th > connection -m 9 = 1st and 4th connection etc > diff --git a/drivers/staging/greybus/tools/loopback_test.c > b/drivers/staging/greybus/tools/loopback_test.c > index b82e2befe935..2fa88092514d 100644 > --- a/drivers/staging/greybus/tools/loopback_test.c > +++ b/drivers/staging/greybus/tools/loopback_test.c > @@ -192,7 +192,7 @@ void usage(void) > " -t must be one of the test names - sink, transfer or ping\n" > " -i iteration count - the number of iterations to run the test > over\n" > " Optional arguments\n" > - " -S sysfs location - location for greybus 'endo' entires default > /sys/bus/greybus/devices/\n" > + " -S sysfs location - location for greybus 'endo' entries default > /sys/bus/greybus/devices/\n" > " -D debugfs location - location for loopback debugfs entries > default /sys/kernel/debug/gb_loopback/\n" > " -s size of data packet to send during test - defaults to zero\n" > " -m mask - a bit mask of connections to include example: -m 8 = > 4th connection -m 9 = 1st and 4th connection etc\n" > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] Staging:greybus Fix comparison to NULL
On 06/05/2018 04:00 AM, Dan Carpenter wrote:
> On Tue, Jun 05, 2018 at 11:02:36AM +0530, Viresh Kumar wrote:
>> On 03-06-18, 08:52, Janani Sankara Babu wrote:
>>> This patch replaces comparison of var to NULL with !var
>>>
>>> Signed-off-by: Janani Sankara Babu
Wow, such deep discussion for such a trivial patch!
I say we take it, mostly because I personally prefer being
permissive if there's nothing technically wrong with a patch,
but also because I'm swayed by the other comments.
That being said, I agree with Johan--the patch would be
improved with a little better comment about what motivated
the submission. So I'll wait for v2.
-Alex
>>> ---
>>> drivers/staging/greybus/core.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/greybus/core.c b/drivers/staging/greybus/core.c
>>> index dafa430..5d14a4e 100644
>>> --- a/drivers/staging/greybus/core.c
>>> +++ b/drivers/staging/greybus/core.c
>>> @@ -48,7 +48,7 @@ static bool greybus_match_one_id(struct gb_bundle *bundle,
>>> static const struct greybus_bundle_id *
>>> greybus_match_id(struct gb_bundle *bundle, const struct greybus_bundle_id
>>> *id)
>>> {
>>> - if (id == NULL)
>>> + if (!id)
>>
>> It is pretty much personal preference and there is no good reason to accept
>> this
>> patch really.
>
> Checkpatch complains, so it's Official Kernel Style now.
>
> The != NULL comparison is a double negative which makes it not less
> annoying than official kernel style.
>
> regards,
> dan carpenter
>
> ___
> greybus-dev mailing list
> greybus-...@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v2] staging: greybus: Use gpio_is_valid()
On 04/27/2018 11:35 PM, Arvind Yadav wrote:
> Replace the manual validity checks for the GPIO with the
> gpio_is_valid().
>
> Signed-off-by: Arvind Yadav
Looks good.
Reviewed-by: Alex Elder
> ---
> chnage in v2 :
> Returning invalid gpio as error instead of -ENODEV.
>
> drivers/staging/greybus/arche-platform.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/greybus/arche-platform.c
> b/drivers/staging/greybus/arche-platform.c
> index 83254a7..c3a7da5 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -448,7 +448,7 @@ static int arche_platform_probe(struct platform_device
> *pdev)
> arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
> "svc,reset-gpio",
> 0);
> - if (arche_pdata->svc_reset_gpio < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
> dev_err(dev, "failed to get reset-gpio\n");
> return arche_pdata->svc_reset_gpio;
> }
> @@ -468,7 +468,7 @@ static int arche_platform_probe(struct platform_device
> *pdev)
> arche_pdata->svc_sysboot_gpio = of_get_named_gpio(np,
> "svc,sysboot-gpio",
> 0);
> - if (arche_pdata->svc_sysboot_gpio < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_sysboot_gpio)) {
> dev_err(dev, "failed to get sysboot gpio\n");
> return arche_pdata->svc_sysboot_gpio;
> }
> @@ -487,7 +487,7 @@ static int arche_platform_probe(struct platform_device
> *pdev)
> arche_pdata->svc_refclk_req = of_get_named_gpio(np,
> "svc,refclk-req-gpio",
> 0);
> - if (arche_pdata->svc_refclk_req < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_refclk_req)) {
> dev_err(dev, "failed to get svc clock-req gpio\n");
> return arche_pdata->svc_refclk_req;
> }
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: Use gpio_is_valid()
On 04/27/2018 07:50 AM, Arvind Yadav wrote:
>
>
> On Friday 27 April 2018 05:47 PM, Alex Elder wrote:
>> On 04/27/2018 05:52 AM, Arvind Yadav wrote:
>>> Replace the manual validity checks for the GPIO with the
>>> gpio_is_valid().
>> I haven't looked through the code paths very closely, but I
>> think that get_named_gpio() might return -EPROBE_DEFER, which
>> would be something we want to pass to the caller.
> Yes of_get_name_gpio() can return other error value apart from
> -EPROBE_DEFER.
>> So rather than returning -ENODEV and hiding the reason the
>> call to of_get_named_gpio() failed, you should continue
>> returning the errno it supplies (if not a valid gpio number).
>>
>> -Alex
> I have return -ENODEV because invalid gpio pin can be positive.
> static inline bool gpio_is_valid(int number)
> {
> return number >= 0 && number < ARCH_NR_GPIOS;
> }
> Here if number > ARCH_NR_GPIOS then it's invalid but return value
> will be positive.
Your reasoning is good. However in all three of these cases,
the GPIO number you're checking is the value returned from
of_get_named_gpio(). The return value is a "GPIO number to
use with Linux generic GPIO API, or one of the errno value."
So unless the API of of_get_named_gpio() changes, you can be
sure that if the value returned is invalid, it is a negative
errno. (And if the API did change, the person making that
change would be responsible for fixing all callers to ensure
the change didn't break them.)
This distinction may be why the code you're changing was only
testing for negative, rather than using gpio_is_valid() (you'll
see it's used elsewhere in the Greybus code--even in the same
source files.)
Anyway, changing the code to use gpio_is_valid() is fine. But
you should avoid obscuring the reason for the error that the
return value from of_get_named_gpio() provides.
-Alex
> We can return like this
> " return (gpio > 0) ? -ENODEV: gpio;"
>
> But not sure this is worth to handle this.
>
> ~arvind
>>
>>> Signed-off-by: Arvind Yadav
>>> ---
>>> drivers/staging/greybus/arche-platform.c | 12 ++--
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/greybus/arche-platform.c
>>> b/drivers/staging/greybus/arche-platform.c
>>> index 83254a7..fc6bf60 100644
>>> --- a/drivers/staging/greybus/arche-platform.c
>>> +++ b/drivers/staging/greybus/arche-platform.c
>>> @@ -448,9 +448,9 @@ static int arche_platform_probe(struct platform_device
>>> *pdev)
>>> arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
>>> "svc,reset-gpio",
>>> 0);
>>> - if (arche_pdata->svc_reset_gpio < 0) {
>>> + if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
>>> dev_err(dev, "failed to get reset-gpio\n");
>>> - return arche_pdata->svc_reset_gpio;
>>> + return -ENODEV;
>>> }
>>> ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio,
>>> "svc-reset");
>>> if (ret) {
>>> @@ -468,9 +468,9 @@ static int arche_platform_probe(struct platform_device
>>> *pdev)
>>> arche_pdata->svc_sysboot_gpio = of_get_named_gpio(np,
>>> "svc,sysboot-gpio",
>>> 0);
>>> - if (arche_pdata->svc_sysboot_gpio < 0) {
>>> + if (!gpio_is_valid(arche_pdata->svc_sysboot_gpio)) {
>>> dev_err(dev, "failed to get sysboot gpio\n");
>>> - return arche_pdata->svc_sysboot_gpio;
>>> + return -ENODEV;
>>> }
>>> ret = devm_gpio_request(dev, arche_pdata->svc_sysboot_gpio,
>>> "sysboot0");
>>> if (ret) {
>>> @@ -487,9 +487,9 @@ static int arche_platform_probe(struct platform_device
>>> *pdev)
>>> arche_pdata->svc_refclk_req = of_get_named_gpio(np,
>>> "svc,refclk-req-gpio",
>>> 0);
>>> - if (arche_pdata->svc_refclk_req < 0) {
>>> + if (!gpio_is_valid(arche_pdata->svc_refclk_req)) {
>>> dev_err(dev, "failed to get svc clock-req gpio\n");
>>> - return arche_pdata->svc_refclk_req;
>>> + return -ENODEV;
>>> }
>>> ret = devm_gpio_request(dev, arche_pdata->svc_refclk_req,
>>> "svc-clk-req");
>>>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH] staging: greybus: Use gpio_is_valid()
On 04/27/2018 05:52 AM, Arvind Yadav wrote:
> Replace the manual validity checks for the GPIO with the
> gpio_is_valid().
I haven't looked through the code paths very closely, but I
think that get_named_gpio() might return -EPROBE_DEFER, which
would be something we want to pass to the caller.
So rather than returning -ENODEV and hiding the reason the
call to of_get_named_gpio() failed, you should continue
returning the errno it supplies (if not a valid gpio number).
-Alex
> Signed-off-by: Arvind Yadav
> ---
> drivers/staging/greybus/arche-platform.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/greybus/arche-platform.c
> b/drivers/staging/greybus/arche-platform.c
> index 83254a7..fc6bf60 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -448,9 +448,9 @@ static int arche_platform_probe(struct platform_device
> *pdev)
> arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
> "svc,reset-gpio",
> 0);
> - if (arche_pdata->svc_reset_gpio < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
> dev_err(dev, "failed to get reset-gpio\n");
> - return arche_pdata->svc_reset_gpio;
> + return -ENODEV;
> }
> ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
> if (ret) {
> @@ -468,9 +468,9 @@ static int arche_platform_probe(struct platform_device
> *pdev)
> arche_pdata->svc_sysboot_gpio = of_get_named_gpio(np,
> "svc,sysboot-gpio",
> 0);
> - if (arche_pdata->svc_sysboot_gpio < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_sysboot_gpio)) {
> dev_err(dev, "failed to get sysboot gpio\n");
> - return arche_pdata->svc_sysboot_gpio;
> + return -ENODEV;
> }
> ret = devm_gpio_request(dev, arche_pdata->svc_sysboot_gpio, "sysboot0");
> if (ret) {
> @@ -487,9 +487,9 @@ static int arche_platform_probe(struct platform_device
> *pdev)
> arche_pdata->svc_refclk_req = of_get_named_gpio(np,
> "svc,refclk-req-gpio",
> 0);
> - if (arche_pdata->svc_refclk_req < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_refclk_req)) {
> dev_err(dev, "failed to get svc clock-req gpio\n");
> - return arche_pdata->svc_refclk_req;
> + return -ENODEV;
> }
> ret = devm_gpio_request(dev, arche_pdata->svc_refclk_req,
> "svc-clk-req");
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v2] Staging: greybus: camera: cleanup multiple checks for null pointers
On 01/08/2018 10:50 AM, Sumit Pundir wrote:
> Fixed coding style issue regarding null comparison at multiple lines.
> Issue reported by checkpatch.pl
>
> Signed-off-by: Sumit Pundir
Looks good. The subject should say "staging" rather than "Staging"
but that's probably not a big deal.
Reviewed-by: Alex Elder
> ---
> v2:
> Updated the patch title and description.
>
> drivers/staging/greybus/camera.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/greybus/camera.c
> b/drivers/staging/greybus/camera.c
> index f13f16b..07ebfb8 100644
> --- a/drivers/staging/greybus/camera.c
> +++ b/drivers/staging/greybus/camera.c
> @@ -918,7 +918,7 @@ static ssize_t gb_camera_debugfs_configure_streams(struct
> gb_camera *gcam,
>
> /* Retrieve number of streams to configure */
> token = strsep(&buf, ";");
> - if (token == NULL)
> + if (!token)
> return -EINVAL;
>
> ret = kstrtouint(token, 10, &nstreams);
> @@ -929,7 +929,7 @@ static ssize_t gb_camera_debugfs_configure_streams(struct
> gb_camera *gcam,
> return -EINVAL;
>
> token = strsep(&buf, ";");
> - if (token == NULL)
> + if (!token)
> return -EINVAL;
>
> ret = kstrtouint(token, 10, &flags);
> @@ -946,7 +946,7 @@ static ssize_t gb_camera_debugfs_configure_streams(struct
> gb_camera *gcam,
>
> /* width */
> token = strsep(&buf, ";");
> - if (token == NULL) {
> + if (!token) {
> ret = -EINVAL;
> goto done;
> }
> @@ -956,7 +956,7 @@ static ssize_t gb_camera_debugfs_configure_streams(struct
> gb_camera *gcam,
>
> /* height */
> token = strsep(&buf, ";");
> - if (token == NULL)
> + if (!token)
> goto done;
>
> ret = kstrtouint(token, 10, &stream->height);
> @@ -965,7 +965,7 @@ static ssize_t gb_camera_debugfs_configure_streams(struct
> gb_camera *gcam,
>
> /* Image format code */
> token = strsep(&buf, ";");
> - if (token == NULL)
> + if (!token)
> goto done;
>
> ret = kstrtouint(token, 16, &stream->format);
> @@ -1009,7 +1009,7 @@ static ssize_t gb_camera_debugfs_capture(struct
> gb_camera *gcam,
>
> /* Request id */
> token = strsep(&buf, ";");
> - if (token == NULL)
> + if (!token)
> return -EINVAL;
> ret = kstrtouint(token, 10, &request_id);
> if (ret < 0)
> @@ -1017,7 +1017,7 @@ static ssize_t gb_camera_debugfs_capture(struct
> gb_camera *gcam,
>
> /* Stream mask */
> token = strsep(&buf, ";");
> - if (token == NULL)
> + if (!token)
> return -EINVAL;
> ret = kstrtouint(token, 16, &streams_mask);
> if (ret < 0)
> @@ -1025,7 +1025,7 @@ static ssize_t gb_camera_debugfs_capture(struct
> gb_camera *gcam,
>
> /* number of frames */
> token = strsep(&buf, ";");
> - if (token == NULL)
> + if (!token)
> return -EINVAL;
> ret = kstrtouint(token, 10, &num_frames);
> if (ret < 0)
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/11] staging: greybus: add SPDX identifiers to all greybus driver files
On 11/07/2017 08:47 AM, Greg Kroah-Hartman wrote: > On Tue, Nov 07, 2017 at 08:42:07AM -0600, Alex Elder wrote: >> On 11/07/2017 07:58 AM, Greg Kroah-Hartman wrote: >>> It's good to have SPDX identifiers in all files to make it easier to >>> audit the kernel tree for correct licenses. >>> >>> Update the drivers/staging/greybus files files with the correct SPDX >>> license identifier based on the license text in the file itself. The >>> SPDX identifier is a legally binding shorthand, which can be used >>> instead of the full boiler plate text. >>> >>> This work is based on a script and data from Thomas Gleixner, Philippe >>> Ombredanne, and Kate Stewart. >> >> Looks good. Except... Why the C++ style comment? (Use /* */ not //?) > > That's the "style" that Linus wanted, it stands out, right? :) Yes it does. I'm very happy to see SPDX getting deployed. -Alex > > thanks for the review. > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/11] staging: greybus: Remove redundant license text
On 11/07/2017 07:58 AM, Greg Kroah-Hartman wrote: > Now that the SPDX tag is in all greybus files, that identifies the > license in a specific and legally-defined manner. So the extra GPL text > wording can be removed as it is no longer needed at all. > > This is done on a quest to remove the 700+ different ways that files in > the kernel describe the GPL license text. And there's unneeded stuff > like the address (sometimes incorrect) for the FSF which is never > needed. > > No copyright headers or other non-license-description text was removed. Awesome. Reviewed-by: Alex Elder > > Cc: Vaibhav Hiremath > Cc: Johan Hovold > Cc: Alex Elder > Cc: Greg Kroah-Hartman > Cc: Vaibhav Agarwal > Cc: Mark Greer > Cc: Viresh Kumar > Cc: Rui Miguel Silva > Cc: David Lin > Cc: "Bryan O'Donoghue" > Signed-off-by: Greg Kroah-Hartman > --- > drivers/staging/greybus/arche-apb-ctrl.c| 2 -- > drivers/staging/greybus/arche-platform.c| 2 -- > drivers/staging/greybus/arche_platform.h| 2 -- > drivers/staging/greybus/audio_apbridgea.c | 2 -- > drivers/staging/greybus/audio_codec.c | 2 -- > drivers/staging/greybus/audio_codec.h | 2 -- > drivers/staging/greybus/audio_gb.c | 2 -- > drivers/staging/greybus/audio_manager.c | 2 -- > drivers/staging/greybus/audio_manager.h | 2 -- > drivers/staging/greybus/audio_manager_module.c | 2 -- > drivers/staging/greybus/audio_manager_private.h | 2 -- > drivers/staging/greybus/audio_manager_sysfs.c | 2 -- > drivers/staging/greybus/audio_module.c | 2 -- > drivers/staging/greybus/audio_topology.c| 2 -- > drivers/staging/greybus/authentication.c| 2 -- > drivers/staging/greybus/bootrom.c | 2 -- > drivers/staging/greybus/bundle.c| 2 -- > drivers/staging/greybus/bundle.h| 2 -- > drivers/staging/greybus/camera.c| 2 -- > drivers/staging/greybus/connection.c| 2 -- > drivers/staging/greybus/connection.h| 2 -- > drivers/staging/greybus/control.c | 2 -- > drivers/staging/greybus/control.h | 2 -- > drivers/staging/greybus/core.c | 2 -- > drivers/staging/greybus/debugfs.c | 2 -- > drivers/staging/greybus/es2.c | 2 -- > drivers/staging/greybus/firmware.h | 2 -- > drivers/staging/greybus/fw-core.c | 2 -- > drivers/staging/greybus/fw-download.c | 2 -- > drivers/staging/greybus/fw-management.c | 2 -- > drivers/staging/greybus/gb-camera.h | 2 -- > drivers/staging/greybus/gbphy.c | 2 -- > drivers/staging/greybus/gbphy.h | 2 -- > drivers/staging/greybus/gpio.c | 2 -- > drivers/staging/greybus/greybus.h | 2 -- > drivers/staging/greybus/greybus_trace.h | 2 -- > drivers/staging/greybus/hd.c| 2 -- > drivers/staging/greybus/hd.h| 2 -- > drivers/staging/greybus/hid.c | 2 -- > drivers/staging/greybus/i2c.c | 2 -- > drivers/staging/greybus/interface.c | 2 -- > drivers/staging/greybus/interface.h | 2 -- > drivers/staging/greybus/light.c | 2 -- > drivers/staging/greybus/log.c | 2 -- > drivers/staging/greybus/loopback.c | 2 -- > drivers/staging/greybus/manifest.c | 2 -- > drivers/staging/greybus/manifest.h | 2 -- > drivers/staging/greybus/module.c| 2 -- > drivers/staging/greybus/module.h| 2 -- > drivers/staging/greybus/operation.c | 2 -- > drivers/staging/greybus/operation.h | 2 -- > drivers/staging/greybus/power_supply.c | 2 -- > drivers/staging/greybus/pwm.c | 2 -- > drivers/staging/greybus/raw.c | 2 -- > drivers/staging/greybus/sdio.c | 2 -- > drivers/staging/greybus/spi.c | 2 -- > drivers/staging/greybus/spilib.c| 2 -- > drivers/staging/greybus/svc.c | 2 -- > drivers/staging/greybus/svc.h | 2 -- > drivers/staging/greybus/svc_watchdog.c | 2 -- > drivers/staging/greybus/uart.c | 2 -- > drivers/staging/greybus/usb.c | 3 --- > drivers/staging/greybus/vibrator.c | 2 -- > 63 files changed, 127 deletions(-) > > diff --git a/drivers/staging/greybus/arche-apb-ctrl.c > b/drivers/staging/greybus/arche-apb-ctrl.c > index c6c3d01395ed..b0c66112eb18 100644 &g
Re: [PATCH 01/11] staging: greybus: add SPDX identifiers to all greybus driver files
On 11/07/2017 07:58 AM, Greg Kroah-Hartman wrote: > It's good to have SPDX identifiers in all files to make it easier to > audit the kernel tree for correct licenses. > > Update the drivers/staging/greybus files files with the correct SPDX > license identifier based on the license text in the file itself. The > SPDX identifier is a legally binding shorthand, which can be used > instead of the full boiler plate text. > > This work is based on a script and data from Thomas Gleixner, Philippe > Ombredanne, and Kate Stewart. Looks good. Except... Why the C++ style comment? (Use /* */ not //?) Otherwise: Reviewed-by: Alex Elder > > Cc: Johan Hovold > Cc: Alex Elder > Cc: Greg Kroah-Hartman > Cc: Vaibhav Hiremath > Cc: Vaibhav Agarwal > Cc: Mark Greer > Cc: Viresh Kumar > Cc: Rui Miguel Silva > Cc: David Lin > Cc: "Bryan O'Donoghue" > Cc: Thomas Gleixner > Cc: Kate Stewart > Cc: Philippe Ombredanne > Signed-off-by: Greg Kroah-Hartman > --- > drivers/staging/greybus/Documentation/firmware/authenticate.c | 1 + > drivers/staging/greybus/Documentation/firmware/firmware.c | 1 + > drivers/staging/greybus/arche-apb-ctrl.c | 1 + > drivers/staging/greybus/arche-platform.c | 1 + > drivers/staging/greybus/arche_platform.h | 1 + > drivers/staging/greybus/arpc.h| 1 + > drivers/staging/greybus/audio_apbridgea.c | 1 + > drivers/staging/greybus/audio_apbridgea.h | 1 + > drivers/staging/greybus/audio_codec.c | 1 + > drivers/staging/greybus/audio_codec.h | 1 + > drivers/staging/greybus/audio_gb.c| 1 + > drivers/staging/greybus/audio_manager.c | 1 + > drivers/staging/greybus/audio_manager.h | 1 + > drivers/staging/greybus/audio_manager_module.c| 1 + > drivers/staging/greybus/audio_manager_private.h | 1 + > drivers/staging/greybus/audio_manager_sysfs.c | 1 + > drivers/staging/greybus/audio_module.c| 1 + > drivers/staging/greybus/audio_topology.c | 1 + > drivers/staging/greybus/authentication.c | 1 + > drivers/staging/greybus/bootrom.c | 1 + > drivers/staging/greybus/bundle.c | 1 + > drivers/staging/greybus/bundle.h | 1 + > drivers/staging/greybus/camera.c | 1 + > drivers/staging/greybus/connection.c | 1 + > drivers/staging/greybus/connection.h | 1 + > drivers/staging/greybus/control.c | 1 + > drivers/staging/greybus/control.h | 1 + > drivers/staging/greybus/core.c| 1 + > drivers/staging/greybus/debugfs.c | 1 + > drivers/staging/greybus/es2.c | 1 + > drivers/staging/greybus/firmware.h| 1 + > drivers/staging/greybus/fw-core.c | 1 + > drivers/staging/greybus/fw-download.c | 1 + > drivers/staging/greybus/fw-management.c | 1 + > drivers/staging/greybus/gb-camera.h | 1 + > drivers/staging/greybus/gbphy.c | 1 + > drivers/staging/greybus/gbphy.h | 1 + > drivers/staging/greybus/gpio.c| 1 + > drivers/staging/greybus/greybus.h | 1 + > drivers/staging/greybus/greybus_authentication.h | 1 + > drivers/staging/greybus/greybus_firmware.h| 1 + > drivers/staging/greybus/greybus_manifest.h| 1 + > drivers/staging/greybus/greybus_protocols.h | 1 + > drivers/staging/greybus/greybus_trace.h | 1 + > drivers/staging/greybus/hd.c | 1 + > drivers/staging/greybus/hd.h | 1 + > drivers/staging/greybus/hid.c | 1 + > drivers/staging/greybus/i2c.c | 1 + > drivers/staging/greybus/interface.c | 1 + > drivers/staging/greybus/interface.h | 1 + > drivers/staging/greybus/light.c | 1 + > drivers/staging/greybus/log.c | 1 + > drivers/staging/greybus/loopback.c
Re: [greybus-dev] [PATCH 2/2] staging: greybus: uart.c: Remove include linux/serial.h
On 04/12/2017 08:36 PM, Darryl T. Agostinelli wrote: > $ make includecheck | grep staging > ./drivers/staging/greybus/uart.c: linux/serial.h is included more than once. > > Signed-off-by: Darryl T. Agostinelli Looks good. Reviewed-by: Alex Elder > --- > drivers/staging/greybus/uart.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c > index b72693e..c6d01b8 100644 > --- a/drivers/staging/greybus/uart.c > +++ b/drivers/staging/greybus/uart.c > @@ -23,7 +23,6 @@ > #include > #include > #include > -#include > #include > #include > #include > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH 1/2] staging: greybus: light.c: Remove include linux/version.h
On 04/12/2017 08:36 PM, Darryl T. Agostinelli wrote: > Fixes: > $ make versioncheck | grep staging > ./drivers/staging/greybus/light.c: 15 linux/version.h not needed. > > Signed-off-by: Darryl T. Agostinelli Looks good. Reviewed-by: Alex Elder > --- > drivers/staging/greybus/light.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c > index 8dffd8a..1681362 100644 > --- a/drivers/staging/greybus/light.c > +++ b/drivers/staging/greybus/light.c > @@ -12,7 +12,6 @@ > #include > #include > #include > -#include > #include > > #include "greybus.h" > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [Outreachy kernel] [PATCH v2 3/6] staging: greybus: Remove unnecessary braces
On 02/21/2017 07:44 AM, Johan Hovold wrote:
> On Tue, Feb 21, 2017 at 02:20:50PM +0100, Julia Lawall wrote:
>>
>>
>> On Tue, 21 Feb 2017, simran singhal wrote:
>>
>>> This patch removes braces for single statement blocks. The warning
>>> was detected using checkpatch.pl.
>>> Coccinelle was used to make the change.
>>>
>>> @@
>>> expression e,e1;
>>> @@
>>> - if (e) {
>>> + if (e)
>>> e1;
>>> - }
>>>
>>> Signed-off-by: simran singhal
>>
>> Acked-by: Julia Lawall
>
> I do not think this patch should be merged.
>
> Note that all of the braces you are removing are for single statements
> that span multiple lines, and that is precisely why the braces were
> added in the first place. Having braces for such statements often
> increase readability even if they are not mandated by the coding
> standard.
>
> So I suggest this patch is dropped.
I concur. I think the change was well-intended but I find the
practice Johan describes to improve readability of the code.
-Alex
>
> Thanks,
> Johan
> ___
> greybus-dev mailing list
> greybus-...@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybus: remove timesync protocol support
On 01/05/2017 11:39 AM, Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman
>
> While the timesync protocol was a great idea, it never ended up getting
> implemented by any known hardware devices. It's also a bit
> "interesting" in how it ties into the platform controller.
>
> So, just remove it for now. It's not needed, no one uses it, and it's a
> stumbling block in getting the greybus core code merged out of the
> staging tree. If anyone wants it in the future, reverting this patch is
> a great place to start from.
>
> Signed-off-by: Greg Kroah-Hartman
This consists entirely of deletions. I haven't applied the
patch to test it, so I trust it compiles as it should. And if
that is true, I'm sure it causes no harm, since as you said,
nobody uses the code right now.
Reviewed-by: Alex Elder
> ---
>
> Sorry Bryan, I know it was a lot of work, and it's a great protocol and
> implementation, but it never made it into a device :(
>
> Note, there are some remants in the arche-platform driver, but that's
> probably next on the chopping block, so I didn't unwind it from there as
> it's a bit more complex, and it never gets built so no one will see
> it...
>
> drivers/staging/greybus/Makefile|4 +-
> drivers/staging/greybus/arche_platform.h|2 -
> drivers/staging/greybus/control.c | 50 -
> drivers/staging/greybus/control.h |7 -
> drivers/staging/greybus/core.c | 11 -
> drivers/staging/greybus/es2.c | 132 ---
> drivers/staging/greybus/greybus.h |1 -
> drivers/staging/greybus/greybus_protocols.h | 47 -
> drivers/staging/greybus/greybus_trace.h | 28 -
> drivers/staging/greybus/hd.h|7 -
> drivers/staging/greybus/interface.c | 56 +-
> drivers/staging/greybus/interface.h |5 -
> drivers/staging/greybus/svc.c | 87 --
> drivers/staging/greybus/svc.h |7 -
> drivers/staging/greybus/timesync.c | 1357
> ---
> drivers/staging/greybus/timesync.h | 45 -
> drivers/staging/greybus/timesync_platform.c | 82 --
> 17 files changed, 3 insertions(+), 1925 deletions(-)
> delete mode 100644 drivers/staging/greybus/timesync.c
> delete mode 100644 drivers/staging/greybus/timesync.h
> delete mode 100644 drivers/staging/greybus/timesync_platform.c
>
> diff --git a/drivers/staging/greybus/Makefile
> b/drivers/staging/greybus/Makefile
> index f337b7b70782..b26b9a35bdd5 100644
> --- a/drivers/staging/greybus/Makefile
> +++ b/drivers/staging/greybus/Makefile
> @@ -10,9 +10,7 @@ greybus-y :=core.o \
> control.o \
> svc.o \
> svc_watchdog.o \
> - operation.o \
> - timesync.o \
> - timesync_platform.o
> + operation.o
>
> obj-$(CONFIG_GREYBUS)+= greybus.o
>
> diff --git a/drivers/staging/greybus/arche_platform.h
> b/drivers/staging/greybus/arche_platform.h
> index bd12345b82a2..c0591df9b9d6 100644
> --- a/drivers/staging/greybus/arche_platform.h
> +++ b/drivers/staging/greybus/arche_platform.h
> @@ -10,8 +10,6 @@
> #ifndef __ARCHE_PLATFORM_H
> #define __ARCHE_PLATFORM_H
>
> -#include "timesync.h"
> -
> enum arche_platform_state {
> ARCHE_PLATFORM_STATE_OFF,
> ARCHE_PLATFORM_STATE_ACTIVE,
> diff --git a/drivers/staging/greybus/control.c
> b/drivers/staging/greybus/control.c
> index 4716190e740a..5b30be30a3a4 100644
> --- a/drivers/staging/greybus/control.c
> +++ b/drivers/staging/greybus/control.c
> @@ -198,56 +198,6 @@ int gb_control_mode_switch_operation(struct gb_control
> *control)
> return ret;
> }
>
> -int gb_control_timesync_enable(struct gb_control *control, u8 count,
> -u64 frame_time, u32 strobe_delay, u32 refclk)
> -{
> - struct gb_control_timesync_enable_request request;
> -
> - request.count = count;
> - request.frame_time = cpu_to_le64(frame_time);
> - request.strobe_delay = cpu_to_le32(strobe_delay);
> - request.refclk = cpu_to_le32(refclk);
> - return gb_operation_sync(control->connection,
> - GB_CONTROL_TYPE_TIMESYNC_ENABLE, &request,
> - sizeof(request), NULL, 0);
> -}
> -
> -int gb_control_timesync_disable(struct gb_control *control)
> -{
> - return gb_operation_sync(control->connection,
> - GB_CONTROL_TYPE_TIMESYNC_DISABLE, NULL, 0,
> -
Re: [PATCH] staging: greybus: remove timesync protocol support
On 01/05/2017 12:25 PM, Bryan O'Donoghue wrote: > On 05/01/17 17:39, Greg Kroah-Hartman wrote: >> From: Greg Kroah-Hartman >> >> While the timesync protocol was a great idea, it never ended up getting >> implemented by any known hardware devices. It's also a bit >> "interesting" in how it ties into the platform controller. >> >> So, just remove it for now. It's not needed, no one uses it, and it's a >> stumbling block in getting the greybus core code merged out of the >> staging tree. If anyone wants it in the future, reverting this patch is >> a great place to start from. >> >> Signed-off-by: Greg Kroah-Hartman >> --- >> >> Sorry Bryan, I know it was a lot of work, and it's a great protocol and >> implementation, but it never made it into a device :( > > Kill it. > > One less turd to polish. I think the core algorithm has some value that we might use at some future date. But I concur, at this point it interferes with things getting reorganized cleanly. -Alex > > Acked-by: Bryan O'Donoghue > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybuis: fix permission style warnings
On 11/30/2016 12:32 PM, Dan Carpenter wrote: > On Wed, Nov 30, 2016 at 05:21:40PM +0100, Andrea Ghittino wrote: >> Honestly, for a kernel newbie it is difficult to manage. I received 2 >> email that suggest >> to use S_IRUGO. My original patch was with octal, based on checkpatch.pl >> advice. Go ahead with the octal version. If Linus (and checkpatch) prefers it that way, so be it. -Alex > You've going to have to get used to conflicting advice and annoying > people if you want to be a kernel dev. Let it wash off you like water > off a duck. > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybuis: fix permission style warnings
On 11/26/2016 03:50 PM, Andrea Ghittino wrote:
> Fixes greybus user/groups permission style warnings
> found by checkpatch.pl tool
>
> Signed-off-by: Andrea Ghittino
I don't understand why using 0444 would be preferred
over S_IRUGO (for example). Do you know? Maybe the
checkpatch.pl output says something about it. There
may well be a good reason, but otherwise I prefer
using the symbolic constants (and therefore *not*
applying this patch).
-Alex
> diff --git a/drivers/staging/greybus/camera.c
> b/drivers/staging/greybus/camera.c
> index 1c5b41a..4424f63 100644
> --- a/drivers/staging/greybus/camera.c
> +++ b/drivers/staging/greybus/camera.c
> @@ -1067,22 +1067,22 @@ struct gb_camera_debugfs_entry {
> static const struct gb_camera_debugfs_entry gb_camera_debugfs_entries[] = {
> {
> .name = "capabilities",
> - .mask = S_IFREG | S_IRUGO,
> + .mask = S_IFREG | 0444,
> .buffer = GB_CAMERA_DEBUGFS_BUFFER_CAPABILITIES,
> .execute = gb_camera_debugfs_capabilities,
> }, {
> .name = "configure_streams",
> - .mask = S_IFREG | S_IRUGO | S_IWUGO,
> + .mask = S_IFREG | 0666,
> .buffer = GB_CAMERA_DEBUGFS_BUFFER_STREAMS,
> .execute = gb_camera_debugfs_configure_streams,
> }, {
> .name = "capture",
> - .mask = S_IFREG | S_IRUGO | S_IWUGO,
> + .mask = S_IFREG | 0666,
> .buffer = GB_CAMERA_DEBUGFS_BUFFER_CAPTURE,
> .execute = gb_camera_debugfs_capture,
> }, {
> .name = "flush",
> - .mask = S_IFREG | S_IRUGO | S_IWUGO,
> + .mask = S_IFREG | 0666,
> .buffer = GB_CAMERA_DEBUGFS_BUFFER_FLUSH,
> .execute = gb_camera_debugfs_flush,
> },
> @@ -1097,7 +1097,7 @@ static ssize_t gb_camera_debugfs_read(struct file
> *file, char __user *buf,
> ssize_t ret;
>
> /* For read-only entries the operation is triggered by a read. */
> - if (!(op->mask & S_IWUGO)) {
> + if (!(op->mask & 0222)) {
> ret = op->execute(gcam, NULL, 0);
> if (ret < 0)
> return ret;
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: greybus: arche-platform: fix line over 80 characters style warnings
On 11/29/2016 03:11 PM, Andrea Ghittino wrote:
> Fixes greybus "line over 80 characters" style warnings
> found by checkpatch.pl tool
I have a few comments. Please update your patch and send
a new version.
> Signed-off-by: Andrea Ghittino
You state that this is v2; how does this version differ
from what you posted last time?
You should normally summarize a history of changes between
patch versions, in the space below the "---" line:
> ---
Here.
So you might do something like:
v2: - Rebased against the latest version of the code
- Reworded the description of the patch
- Cleaned up a white space problem reported by John Doe
> diff --git a/drivers/staging/greybus/arche-apb-ctrl.c
> b/drivers/staging/greybus/arche-apb-ctrl.c
> index 3fda0cd..755120a 100644
> --- a/drivers/staging/greybus/arche-apb-ctrl.c
> +++ b/drivers/staging/greybus/arche-apb-ctrl.c
> @@ -168,7 +168,10 @@ static int standby_boot_seq(struct platform_device *pdev)
> if (apb->init_disabled)
> return 0;
>
> - /* Even if it is in OFF state, then we do not want to change the state
> */
> + /*
> + * Even if it is in OFF state,
> + * then we do not want to change the state
> + */
The 80 characters per line rule is not all that critical. That
being said, I don't object to fixing these issues.
However in this case I find the way you formatted it looks
strange to my eyes--comments with very short lines look a
little odd to me. In my environment, I set up my editor to
wrap at about column 70, but you can go beyond that. Doing
multi-line comments that are much shorter than that is unusual
though.
Two suggestions:
- Shorten the comment line by just moving a word or two to the
next line, e.g.:
/*
* Even if it is in OFF state, then we do not want to change
* the state
*/
- Sometimes (and this particular example is one of those times)
the comment can be slightly reworded, resulting in something
just as good but fitting the 80 column limit. E.g.:
/* Don't change state if it is in standby, or off */
(Note, I haven't looked closely at the code to verify that
my wording here actually matches what it does.)
These general suggestions apply to a few other of your fixes,
below.
> if (apb->state == ARCHE_PLATFORM_STATE_STANDBY ||
> apb->state == ARCHE_PLATFORM_STATE_OFF)
> return 0;
> diff --git a/drivers/staging/greybus/arche-platform.c
> b/drivers/staging/greybus/arche-platform.c
> index 338c2d3..18ab2b4 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -295,7 +295,10 @@ static irqreturn_t arche_platform_wd_irq(int irq, void
> *devid)
>
> arche_platform_set_wake_detect_state(arche_pdata,
>
> WD_STATE_IDLE);
> } else {
> - /* Check we are not in middle of irq thread
> already */
> + /*
> + * Check we are not in middle
> + * of irq thread already
> + */
> if (arche_pdata->wake_detect_state !=
> WD_STATE_COLDBOOT_START) {
>
> arche_platform_set_wake_detect_state(arche_pdata,
> @@ -312,12 +315,14 @@ static irqreturn_t arche_platform_wd_irq(int irq, void
> *devid)
> if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> arche_pdata->wake_detect_start = jiffies;
> /*
> - * In the begining, when wake/detect goes low (first
> time), we assume
> - * it is meant for coldboot and set the flag. If
> wake/detect line stays low
> - * beyond 30msec, then it is coldboot else fallback to
> standby boot.
> + * In the begining, when wake/detect goes low
I think it would be nice to fix this spelling error (s/begin/beginn/)
too. Some people might argue that belongs in another patch. Either
way, since you're fixing this comment, consider fixing everything
that's wrong with it while you're at it.
> + * (first time), we assume it is meant for coldboot
> + * and set the flag. If wake/detect line stays low
> + * beyond 30msec, then it is coldboot else
> + * fallback to standby boot.
>*/
> arche_platform_set_wake_detect_state(arche_pdata,
> -
> WD_STATE_BOOT_INIT);
> +
> WD_STATE_BOOT_INIT);
I'm not sure what changed in the line above--it must be in the white
space or something. Either mention tha
Re: Greybus Future
On 10/31/2016 08:50 AM, Alex Elder wrote: > The Greybus kernel code, developed as part of Google's Project Ara, > is in the upstream Linux kernel tree (under drivers/staging). The > cancellation of that project makes the future for Greybus a bit less > certain. There is interest among the core developers of Greybus > (and others) to do what we can to preserve the value of the Greybus > implementation for use beyond its original target platform. For > that to have any chance of success, we need to establish some > organization, and basically form a community of developers and > interested parties. At long last I can tell you I've arranged to get a mailing list and IRC channel set up for Greybus development. The e-mail address is: greybus-...@lists.linaro.org This list is intended to handle all development discussion for Greybus and related topics (like gbsim and manifesto), and this includes patch traffic. Please add yourself to the mailing list here: https://lists.linaro.org/mailman/listinfo/greybus-dev Archives of traffic on the list will be maintained here: https://lists.linaro.org/pipermail/greybus-dev The IRC channel is on Freenode, named #greybus. I believe it too is logged (and if it is not now, we'll arrange for it to be). We will continue to use the git repositories on Github: https://github.com/projectara There is no Wiki or other web page, nor other resources like bug tracking or a patchwork instance. In time we may add these. For anyone previously associated with Project Ara, please note that the old e-mail lists and IRC channel will be shut down at some future date (perhaps at the end of this year). Any questions, please contact me. Thanks. -Alex > I'm proposing below a few things, which I'll implement this week > unless I hear people express strong opposition (or clearly better > suggestions). If you have comments, please say something. > > Git repositories. Public git repositories related to Project Ara > are all hosted here: > https://github.com/projectara > At this time I see no reason to move away from this, but it would > not surprise me if we decided to host the code and documentation > somewhere else at some future date. > > Mailing List. Because the Greybus kernel code sits in the staging > tree, mail about it should go to the driver-devel mailing list > (de...@driverdev.osuosl.org). I know that some people prefer better > mail filters rather than more mailing lists, but I think Greybus > discussion warrants a separate list. Its code spans multiple code > bases, and I think we're going to need some dedicated and ongoing > strategy and design discussions. So I'd like to create a Greybus > mailing list as a home for all Greybus-related discussion, including > patch traffic. My suggestion is to create a majordomo list: > grey...@kernel.org > > Patchwork. In the past I have found Patchwork to be a very useful > tool in managing incoming patches. I'd like to set up a patchwork > instance for Greybus, monitoring the greybus mailing list, again > at kernel.org. > > Wiki page. If we want a Wiki page, we could set one up at > kernel.org. Thoughts? > > Web home page. I have secured the "greybus.org" domain, so if there > is any value in populating that page it could serve as the home page > for the project. It currently has no content. > > Are there any other important resources I've missed? Once we get > a list going (or not) I've got a few other things to talk about. > > Thanks. > > -Alex > > PS I have addressed this message to a set of interested people > whose e-mail addresses I knew; the set is definitely incomplete. > This will form the initial membership of the mailing list. If > you do not want to be on that list, or if you know others I > should include, please send me contact information by private > message. > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Greybus Future
On 11/02/2016 11:29 AM, Alexandre Bailon wrote: > On 10/31/2016 02:50 PM, Alex Elder wrote: >> Git repositories. Public git repositories related to Project Ara >> are all hosted here: >> https://github.com/projectara >> At this time I see no reason to move away from this, but it would >> not surprise me if we decided to host the code and documentation >> somewhere else at some future date. > Do we have the rights to add new repos to https://github.com/projectara? Yes. I just created a test repository there. If you find you have trouble let me know and I'll create one for you. -Alex > > Thanks, > Alexandre > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Greybus Future
The Greybus kernel code, developed as part of Google's Project Ara, is in the upstream Linux kernel tree (under drivers/staging). The cancellation of that project makes the future for Greybus a bit less certain. There is interest among the core developers of Greybus (and others) to do what we can to preserve the value of the Greybus implementation for use beyond its original target platform. For that to have any chance of success, we need to establish some organization, and basically form a community of developers and interested parties. I'm proposing below a few things, which I'll implement this week unless I hear people express strong opposition (or clearly better suggestions). If you have comments, please say something. Git repositories. Public git repositories related to Project Ara are all hosted here: https://github.com/projectara At this time I see no reason to move away from this, but it would not surprise me if we decided to host the code and documentation somewhere else at some future date. Mailing List. Because the Greybus kernel code sits in the staging tree, mail about it should go to the driver-devel mailing list (de...@driverdev.osuosl.org). I know that some people prefer better mail filters rather than more mailing lists, but I think Greybus discussion warrants a separate list. Its code spans multiple code bases, and I think we're going to need some dedicated and ongoing strategy and design discussions. So I'd like to create a Greybus mailing list as a home for all Greybus-related discussion, including patch traffic. My suggestion is to create a majordomo list: grey...@kernel.org Patchwork. In the past I have found Patchwork to be a very useful tool in managing incoming patches. I'd like to set up a patchwork instance for Greybus, monitoring the greybus mailing list, again at kernel.org. Wiki page. If we want a Wiki page, we could set one up at kernel.org. Thoughts? Web home page. I have secured the "greybus.org" domain, so if there is any value in populating that page it could serve as the home page for the project. It currently has no content. Are there any other important resources I've missed? Once we get a list going (or not) I've got a few other things to talk about. Thanks. -Alex PS I have addressed this message to a set of interested people whose e-mail addresses I knew; the set is definitely incomplete. This will form the initial membership of the mailing list. If you do not want to be on that list, or if you know others I should include, please send me contact information by private message. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] staging: greybus: audio_codec.c: code indent should use tabs where possible
On 09/21/2016 12:05 PM, Richard Groux wrote: > Minor error spotted by checkpatch.pl in greybus > code indent should use tabs where possible > > Signed-off-by: Richard Groux Looks good. Reviewed-by: Alex Elder > --- > drivers/staging/greybus/audio_codec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/greybus/audio_codec.c > b/drivers/staging/greybus/audio_codec.c > index f347743..a46ca42 100644 > --- a/drivers/staging/greybus/audio_codec.c > +++ b/drivers/staging/greybus/audio_codec.c > @@ -1008,7 +1008,7 @@ static int gbcodec_probe(struct snd_soc_codec *codec) > snd_soc_codec_set_drvdata(codec, info); > gbcodec = info; > > -device_init_wakeup(codec->dev, 1); > + device_init_wakeup(codec->dev, 1); > return 0; > } > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: greybus: audio_codec.c: space required before the open brace
On 09/21/2016 12:05 PM, Richard Groux wrote:
> Minor error spotted by checkpatch.pl in greybus
> space required before the open brace '{'
>
> Signed-off-by: Richard Groux
Sure, looks good. Greg will have to apply these patches.
Reviewed-by: Alex Elder
> ---
> drivers/staging/greybus/audio_codec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/audio_codec.c
> b/drivers/staging/greybus/audio_codec.c
> index 2f70295..f347743 100644
> --- a/drivers/staging/greybus/audio_codec.c
> +++ b/drivers/staging/greybus/audio_codec.c
> @@ -322,7 +322,7 @@ int gbaudio_module_update(struct gbaudio_codec_info
> *codec,
> dev_dbg(module->dev, "%s:Module update %s sequence\n", w->name,
> enable ? "Enable":"Disable");
>
> - if ((w->id != snd_soc_dapm_aif_in) && (w->id != snd_soc_dapm_aif_out)){
> + if ((w->id != snd_soc_dapm_aif_in) && (w->id != snd_soc_dapm_aif_out)) {
> dev_dbg(codec->dev, "No action required for %s\n", w->name);
> return 0;
> }
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
