Skip to contentSkip to conversation

staging: vchiq: Minor fixups to CONFIG_VCHIQ_CDEV

PATCH In review Ojaswin Mujoo 3 participants 11 messages Jul 28, 2021 – Jul 29, 2021

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover staging: vchiq: Minor fixups to CONFIG_VCHIQ_CDEV

Hi everyone,

The patchset performs some minor fixups to the CONFIG_VCHIQ_CDEV config option defined in /staging/vc04_services/Kconfig. (Introduced in [1])

Summary:

Patch 1: Enable the config by default to be more consistent with how
cdev creation worked before this config was introduced
Patch 2: Add some more details to the config's help text

Please let me know if any changes are required.

Thank you!
Ojaswin

[1] https://lore.kernel.org/r/846c424dd4aae14d1cc28c8f30877a06e2b7dd10.1626882325.git.ojaswin98@gmail.com

Ojaswin Mujoo (2):
staging: vchiq: Set $CONFIG_VCHIQ_CDEV to be enabled by default
staging: vchiq: Add details to $CONFIG_VCHIQ_CDEV help text

 drivers/staging/vc04_services/Kconfig | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
Signature · 1 line
2.25.1
#1 lore raw

PATCH 1/2 staging: vchiq: Set $CONFIG_VCHIQ_CDEV to be enabled by default

Before this config was defined, the cdev used to be created unconditionally. When an earlier commit introduced this config, the default behavior was set to disabled, which might surprise some unsuspecting users. Hence, make this config default to 'Y' to be more backward consistent.

---
 drivers/staging/vc04_services/Kconfig | 1 +
 1 file changed, 1 insertion(+)
drivers/staging/vc04_services/Kconfig +1 -0
index 63caa6818d37..2b70c37cdd09 100644
@@ -23,6 +23,7 @@ if BCM2835_VCHIQ
 
 config VCHIQ_CDEV
 	bool "VCHIQ Character Driver"
+	default y
 	help
 		Enable the creation of VCHIQ character driver to help
 		communicate with the Videocore platform.
Signature · 1 line
2.25.1
maintainer
#2 lore raw

Re: [PATCH 1/2] staging: vchiq: Set $CONFIG_VCHIQ_CDEV to be enabled by default

Quoted 16 lines · Ojaswin Mujoo

Before this config was defined, the cdev used to be created unconditionally. When an earlier commit introduced this config, the default behavior was set to disabled, which might surprise some unsuspecting users. Hence, make this config default to 'Y' to be more backward consistent.

---
 drivers/staging/vc04_services/Kconfig | 1 +
 1 file changed, 1 insertion(+)
drivers/staging/vc04_services/Kconfig +0 -0
index 63caa6818d37..2b70c37cdd09 100644

drivers/staging/vc04_services/Kconfig if BCM2835_VCHIQ · line 26

Quoted 4 lines
drivers/staging/vc04_services/Kconfig +1 -0
@@ -23,6 +23,7 @@ if BCM2835_VCHIQ
 
 config VCHIQ_CDEV
 	bool "VCHIQ Character Driver"
+	default y

default y is only if the machine will not work without this option. Is that the case here? If not, then please do not have this as the default.

thanks,

greg k-h

#3 lore raw

Re: [PATCH 1/2] staging: vchiq: Set $CONFIG_VCHIQ_CDEV to be enabled by default

Quoted 18 lines · Greg KH
Quoted 17 lines · Ojaswin Mujoo

Before this config was defined, the cdev used to be created unconditionally. When an earlier commit introduced this config, the default behavior was set to disabled, which might surprise some unsuspecting users. Hence, make this config default to 'Y' to be more backward consistent.

---
 drivers/staging/vc04_services/Kconfig | 1 +
 1 file changed, 1 insertion(+)
drivers/staging/vc04_services/Kconfig +0 -0
index 63caa6818d37..2b70c37cdd09 100644
@@ -23,6 +23,7 @@ if BCM2835_VCHIQ
 
Quoted 8 lines

drivers/staging/vc04_services/Kconfig if BCM2835_VCHIQ · line 26

Quoted 3 lines
drivers/staging/vc04_services/Kconfig +1 -0
 config VCHIQ_CDEV
 	bool "VCHIQ Character Driver"
+	default y

default y is only if the machine will not work without this option. Is that the case here? If not, then please do not have this as the default.

Got it Greg.

From my testing, the Raspberry Pi does seem to boot correctly without this although some userspace libraries might not work.

Since the machine itself works, I guess I'll drop this patch.

Thank you!
Ojaswin

Quoted 4 lines

thanks,

greg k-h

#4 lore raw

Re: [PATCH 1/2] staging: vchiq: Set $CONFIG_VCHIQ_CDEV to be enabled by default

Am 28.07.21 um 22:06 schrieb Ojaswin Mujoo:

Quoted 26 lines · Greg KH
Quoted 18 lines · Ojaswin Mujoo
Quoted 17 lines

Before this config was defined, the cdev used to be created unconditionally. When an earlier commit introduced this config, the default behavior was set to disabled, which might surprise some unsuspecting users. Hence, make this config default to 'Y' to be more backward consistent.

---
 drivers/staging/vc04_services/Kconfig | 1 +
 1 file changed, 1 insertion(+)
drivers/staging/vc04_services/Kconfig +0 -0
index 63caa6818d37..2b70c37cdd09 100644
@@ -23,6 +23,7 @@ if BCM2835_VCHIQ
 
Quoted 7 lines

drivers/staging/vc04_services/Kconfig if BCM2835_VCHIQ · line 26

Quoted 3 lines
drivers/staging/vc04_services/Kconfig +1 -0
 config VCHIQ_CDEV
 	bool "VCHIQ Character Driver"
+	default y

default y is only if the machine will not work without this option. Is that the case here? If not, then please do not have this as the default.

Quoted 6 lines

Got it Greg.

From my testing, the Raspberry Pi does seem to boot correctly without this although some userspace libraries might not work.

Since the machine itself works, I guess I'll drop this patch.

The idea is good, but the patch needs improvement :)

Acceptable solution might be that BCM2835_VCHIQ selects VCHIQ_CDEV.

Quoted 10 lines

Thank you!
Ojaswin

Quoted 3 lines

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#5 lore raw

Re: [PATCH 1/2] staging: vchiq: Set $CONFIG_VCHIQ_CDEV to be enabled by default

Quoted 29 lines · Stefan Wahren

Am 28.07.21 um 22:06 schrieb Ojaswin Mujoo:

Quoted 28 lines · Greg KH
Quoted 18 lines · Ojaswin Mujoo
Quoted 17 lines

Before this config was defined, the cdev used to be created unconditionally. When an earlier commit introduced this config, the default behavior was set to disabled, which might surprise some unsuspecting users. Hence, make this config default to 'Y' to be more backward consistent.

---
 drivers/staging/vc04_services/Kconfig | 1 +
 1 file changed, 1 insertion(+)
drivers/staging/vc04_services/Kconfig +0 -0
index 63caa6818d37..2b70c37cdd09 100644
@@ -23,6 +23,7 @@ if BCM2835_VCHIQ
 
Quoted 7 lines

drivers/staging/vc04_services/Kconfig if BCM2835_VCHIQ · line 26

Quoted 3 lines
drivers/staging/vc04_services/Kconfig +1 -0
 config VCHIQ_CDEV
 	bool "VCHIQ Character Driver"
+	default y

default y is only if the machine will not work without this option. Is that the case here? If not, then please do not have this as the default.

Got it Greg.

Quoted 8 lines
Quoted 4 lines

From my testing, the Raspberry Pi does seem to boot correctly without this although some userspace libraries might not work.

Since the machine itself works, I guess I'll drop this patch.

The idea is good, but the patch needs improvement :)

Acceptable solution might be that BCM2835_VCHIQ selects VCHIQ_CDEV.

Thanks for the suggestion, Stefan.

So on testing with select, it seems like select defines a hard reverse dependency i.e if BCM2835_VCHIQ=y then VCHIQ_CDEV=y and it can't be set to n.

Checking the Kconfig launguae doc, I found that we could use "imply" to define weak reverse dependencies, such that BCM2835_VCHIQ=y auto selects VCHIQ_CDEV=y but we can also change it back to n if required. I tested this out and it seems to work fine. I believe we can se imply here as it better suits the use case.

Thank you,
Ojaswin

Quoted 12 lines
Quoted 11 lines

Thank you!
Ojaswin

Quoted 3 lines

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#6 lore raw

PATCH 2/2 staging: vchiq: Add details to $CONFIG_VCHIQ_CDEV help text

In reply to Ojaswin Mujoo

Add some details to the Kconfig definition of $CONFIG_VCHIQ_CDEV to help make the motive behind it a bit more clear.

---
 drivers/staging/vc04_services/Kconfig | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/vc04_services/Kconfig +6 -2
index 2b70c37cdd09..cb02d8a4cb74 100644
@@ -25,8 +25,12 @@ config VCHIQ_CDEV
 	bool "VCHIQ Character Driver"
 	default y
 	help
-		Enable the creation of VCHIQ character driver to help
-		communicate with the Videocore platform.
+		Enable the creation of VCHIQ character driver to help communicate
+		with the VideoCore platform. The cdev exposes ioctls used by
+		userspace libraries and testing tools to interact with VideoCore.
+		This can be set to 'N' if the VideoCore communication is not needed
+		by userspace but only by other kernel modules (like bcm2835-audio).
+		If not sure, set this to 'Y'.
 
 endif
 
Signature · 1 line
2.25.1
maintainer
#7 lore raw

Re: [PATCH 2/2] staging: vchiq: Add details to $CONFIG_VCHIQ_CDEV help text

Quoted 14 lines · Ojaswin Mujoo

Add some details to the Kconfig definition of $CONFIG_VCHIQ_CDEV to help make the motive behind it a bit more clear.

---
 drivers/staging/vc04_services/Kconfig | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/vc04_services/Kconfig +0 -0
index 2b70c37cdd09..cb02d8a4cb74 100644
@@ -25,8 +25,12 @@ config VCHIQ_CDEV
 	bool "VCHIQ Character Driver"

drivers/staging/vc04_services/Kconfig config VCHIQ_CDEV · line 33

Quoted 10 lines
drivers/staging/vc04_services/Kconfig +6 -2
 	default y
 	help
-		Enable the creation of VCHIQ character driver to help
-		communicate with the Videocore platform.
+		Enable the creation of VCHIQ character driver to help communicate
+		with the VideoCore platform. The cdev exposes ioctls used by
+		userspace libraries and testing tools to interact with VideoCore.
+		This can be set to 'N' if the VideoCore communication is not needed
+		by userspace but only by other kernel modules (like bcm2835-audio).
+		If not sure, set this to 'Y'.

I still do not understand if I need this driver or not, and I have this hardware! What functionality does this driver accomplish? What is VideoCore?

thanks,

greg k-h

#8 lore raw

Re: [PATCH 2/2] staging: vchiq: Add details to $CONFIG_VCHIQ_CDEV help text

Quoted 21 lines · Greg KH
Quoted 20 lines · Ojaswin Mujoo

Add some details to the Kconfig definition of $CONFIG_VCHIQ_CDEV to help make the motive behind it a bit more clear.

---
 drivers/staging/vc04_services/Kconfig | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/vc04_services/Kconfig +2 -2
index 2b70c37cdd09..cb02d8a4cb74 100644
@@ -25,8 +25,12 @@ config VCHIQ_CDEV
 	bool "VCHIQ Character Driver"
 	default y
 	help
-		Enable the creation of VCHIQ character driver to help
-		communicate with the Videocore platform.
+		Enable the creation of VCHIQ character driver to help communicate
+		with the VideoCore platform. The cdev exposes ioctls used by
Quoted 8 lines

drivers/staging/vc04_services/Kconfig config VCHIQ_CDEV · line 33

Quoted 4 lines
drivers/staging/vc04_services/Kconfig +4 -0
+		userspace libraries and testing tools to interact with VideoCore.
+		This can be set to 'N' if the VideoCore communication is not needed
+		by userspace but only by other kernel modules (like bcm2835-audio).
+		If not sure, set this to 'Y'.

I still do not understand if I need this driver or not, and I have this hardware! What functionality does this driver accomplish? What is VideoCore?

Hey Greg,

I believe I can add this under the CONFIG_BCM2835_VCHIQ config option, as that enables the core driver that implements the functions to communicate with VideoCore platform?

This config option merely adds a cdev which exposes the the core driver's functionality to userspace.

Regards,
Ojaswin

Quoted 4 lines

thanks,

greg k-h

#9 lore raw

Re: [PATCH 2/2] staging: vchiq: Add details to $CONFIG_VCHIQ_CDEV help text

Am 28.07.21 um 22:00 schrieb Ojaswin Mujoo:

Quoted 29 lines · Greg KH
Quoted 20 lines · Ojaswin Mujoo
Quoted 19 lines

Add some details to the Kconfig definition of $CONFIG_VCHIQ_CDEV to help make the motive behind it a bit more clear.

---
 drivers/staging/vc04_services/Kconfig | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/vc04_services/Kconfig +1 -2
index 2b70c37cdd09..cb02d8a4cb74 100644
@@ -25,8 +25,12 @@ config VCHIQ_CDEV
 	bool "VCHIQ Character Driver"
 	default y
 	help
-		Enable the creation of VCHIQ character driver to help
-		communicate with the Videocore platform.
+		Enable the creation of VCHIQ character driver to help communicate
Quoted 8 lines

drivers/staging/vc04_services/Kconfig config VCHIQ_CDEV · line 33

Quoted 5 lines
drivers/staging/vc04_services/Kconfig +5 -0
+		with the VideoCore platform. The cdev exposes ioctls used by
+		userspace libraries and testing tools to interact with VideoCore.
+		This can be set to 'N' if the VideoCore communication is not needed
+		by userspace but only by other kernel modules (like bcm2835-audio).
+		If not sure, set this to 'Y'.

I still do not understand if I need this driver or not, and I have this hardware! What functionality does this driver accomplish? What is VideoCore?

Quoted 5 lines

Hey Greg,

I believe I can add this under the CONFIG_BCM2835_VCHIQ config option, as that enables the core driver that implements the functions to communicate with VideoCore platform?

Sorry, today i'm too tired to give a good explanation. VideoCore is the VPU inside the BCM283x SoC. It runs a firmware and VCHIQ provides a way to communicate with this firmware / VPU. The VCHIQ driver is required to get access to the audio jack and camera interface (see depending drivers).

Unfortunately i don't have an application list by the hand which uses the CDEV interface for VCHIQ, please use this link [1] as a starting point.

[1] - https://github.com/raspberrypi/userland

Quoted 13 lines

This config option merely adds a cdev which exposes the the core driver's functionality to userspace.

Regards,
Ojaswin

Quoted 3 lines

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#10 lore raw

Re: [PATCH 2/2] staging: vchiq: Add details to $CONFIG_VCHIQ_CDEV help text

Quoted 41 lines · Stefan Wahren

Am 28.07.21 um 22:00 schrieb Ojaswin Mujoo:

Quoted 29 lines · Greg KH
Quoted 20 lines · Ojaswin Mujoo
Quoted 19 lines

Add some details to the Kconfig definition of $CONFIG_VCHIQ_CDEV to help make the motive behind it a bit more clear.

---
 drivers/staging/vc04_services/Kconfig | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/vc04_services/Kconfig +1 -2
index 2b70c37cdd09..cb02d8a4cb74 100644
@@ -25,8 +25,12 @@ config VCHIQ_CDEV
 	bool "VCHIQ Character Driver"
 	default y
 	help
-		Enable the creation of VCHIQ character driver to help
-		communicate with the Videocore platform.
+		Enable the creation of VCHIQ character driver to help communicate
Quoted 8 lines

drivers/staging/vc04_services/Kconfig config VCHIQ_CDEV · line 33

Quoted 5 lines
drivers/staging/vc04_services/Kconfig +5 -0
+		with the VideoCore platform. The cdev exposes ioctls used by
+		userspace libraries and testing tools to interact with VideoCore.
+		This can be set to 'N' if the VideoCore communication is not needed
+		by userspace but only by other kernel modules (like bcm2835-audio).
+		If not sure, set this to 'Y'.

I still do not understand if I need this driver or not, and I have this hardware! What functionality does this driver accomplish? What is VideoCore?

Quoted 5 lines

Hey Greg,

I believe I can add this under the CONFIG_BCM2835_VCHIQ config option, as that enables the core driver that implements the functions to communicate with VideoCore platform?

Sorry, today i'm too tired to give a good explanation. VideoCore is the VPU inside the BCM283x SoC. It runs a firmware and VCHIQ provides a way to communicate with this firmware / VPU. The VCHIQ driver is required to get access to the audio jack and camera interface (see depending drivers).

Quoted 5 lines

Unfortunately i don't have an application list by the hand which uses the CDEV interface for VCHIQ, please use this link [1] as a starting point.

[1] - https://github.com/raspberrypi/userland

Hi Stefan

Thanks for the brief, its good enough to help me get started. I've also been looking more closely into this driver in my free time lately and I can try to come up with a small summary to use as the help text. I'll add it in v2 of this patch.

Regards,
Ojaswin

Quoted 14 lines
Quoted 13 lines

This config option merely adds a cdev which exposes the the core driver's functionality to userspace.

Regards,
Ojaswin

Quoted 3 lines

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel