The Greybus specification defines the Vendor Specific bundle
class[1]. Using this class allows vendors to avoid defining a
custom generic class, simplifying driver development. Currently,
drivers can register to either match on a bundle class or a
Vendor & Product ID with `GREYBUS_CLASS` and `GREYBUS_DEVICE`
respectively.
However, when matching to a vendor specific driver, the current
implementation does not check the bundle's class. This has the
effect of matching the vendor specific driver to ANY bundle found
in the device's manifest, regardless of its class. This is
incorrect as only vendor class bundles should be considered.
For instance, a driver registered for `GREYBUS_DEVICE(0xCAFE, 0xBABE)`
would be matched to a Camera class bundle found on that device.
Instead, only a `GREYBUS_CLASS_VENDOR` class bundle should get
matched to the driver.
[1] https://github.com/projectara/greybus-spec/blob/149aa4a8f4422533475e0193ece…
Signed-off-by: Yacin Belmihoub-Martel <yacin.belmihoubmartel(a)gmail.com>
---
drivers/greybus/core.c | 12 ++++--------
include/linux/greybus.h | 10 +++-------
include/linux/greybus/greybus_id.h | 6 ------
3 files changed, 7 insertions(+), 21 deletions(-)
diff --git a/drivers/greybus/core.c b/drivers/greybus/core.c
index 313eb65cf..4cd218d29 100644
--- a/drivers/greybus/core.c
+++ b/drivers/greybus/core.c
@@ -60,16 +60,12 @@ static int is_gb_svc(const struct device *dev)
static bool greybus_match_one_id(struct gb_bundle *bundle,
const struct greybus_bundle_id *id)
{
- if ((id->match_flags & GREYBUS_ID_MATCH_VENDOR) &&
- (id->vendor != bundle->intf->vendor_id))
+ if (id->class != bundle->class)
return false;
- if ((id->match_flags & GREYBUS_ID_MATCH_PRODUCT) &&
- (id->product != bundle->intf->product_id))
- return false;
-
- if ((id->match_flags & GREYBUS_ID_MATCH_CLASS) &&
- (id->class != bundle->class))
+ if ((id->class == GREYBUS_CLASS_VENDOR) &&
+ (id->vendor != bundle->intf->vendor_id ||
+ id->product != bundle->intf->product_id))
return false;
return true;
diff --git a/include/linux/greybus.h b/include/linux/greybus.h
index 4d58e27ce..1f1b3cb5c 100644
--- a/include/linux/greybus.h
+++ b/include/linux/greybus.h
@@ -37,18 +37,14 @@
#define GREYBUS_VERSION_MAJOR 0x00
#define GREYBUS_VERSION_MINOR 0x01
-#define GREYBUS_ID_MATCH_DEVICE \
- (GREYBUS_ID_MATCH_VENDOR | GREYBUS_ID_MATCH_PRODUCT)
+#define GREYBUS_DEVICE_CLASS(c) \
+ .class = (c),
#define GREYBUS_DEVICE(v, p) \
- .match_flags = GREYBUS_ID_MATCH_DEVICE, \
+ GREYBUS_DEVICE_CLASS(GREYBUS_CLASS_VENDOR)\
.vendor = (v), \
.product = (p),
-#define GREYBUS_DEVICE_CLASS(c) \
- .match_flags = GREYBUS_ID_MATCH_CLASS, \
- .class = (c),
-
/* Maximum number of CPorts */
#define CPORT_ID_MAX 4095 /* UniPro max id is 4095 */
#define CPORT_ID_BAD U16_MAX
diff --git a/include/linux/greybus/greybus_id.h b/include/linux/greybus/greybus_id.h
index f4c844009..e33ed0061 100644
--- a/include/linux/greybus/greybus_id.h
+++ b/include/linux/greybus/greybus_id.h
@@ -11,7 +11,6 @@
struct greybus_bundle_id {
- __u16 match_flags;
__u32 vendor;
__u32 product;
__u8 class;
@@ -19,9 +18,4 @@ struct greybus_bundle_id {
kernel_ulong_t driver_info __aligned(sizeof(kernel_ulong_t));
};
-/* Used to match the greybus_bundle_id */
-#define GREYBUS_ID_MATCH_VENDOR BIT(0)
-#define GREYBUS_ID_MATCH_PRODUCT BIT(1)
-#define GREYBUS_ID_MATCH_CLASS BIT(2)
-
#endif /* __LINUX_GREYBUS_ID_H */
--
2.52.0
Driver core holds a reference to the USB interface and its parent USB
device while the interface is bound to a driver and there is no need to
take additional references unless the structures are needed after
disconnect.
Drop the redundant device reference to reduce cargo culting, make it
easier to spot drivers where an extra reference is needed, and reduce
the risk of memory leaks when drivers fail to release it.
Signed-off-by: Johan Hovold <johan(a)kernel.org>
---
Changes in v2
- drop temporary udev variable as reported by the kernel test robot
(W=1 warning)
drivers/greybus/es2.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/greybus/es2.c b/drivers/greybus/es2.c
index 6ae0ac828afa..a97868009191 100644
--- a/drivers/greybus/es2.c
+++ b/drivers/greybus/es2.c
@@ -772,7 +772,6 @@ static int check_urb_status(struct urb *urb)
static void es2_destroy(struct es2_ap_dev *es2)
{
- struct usb_device *udev;
struct urb *urb;
int i;
@@ -804,10 +803,7 @@ static void es2_destroy(struct es2_ap_dev *es2)
gb_hd_cport_release_reserved(es2->hd, ES2_CPORT_CDSI1);
gb_hd_cport_release_reserved(es2->hd, ES2_CPORT_CDSI0);
- udev = es2->usb_dev;
gb_hd_put(es2->hd);
-
- usb_put_dev(udev);
}
static void cport_in_callback(struct urb *urb)
@@ -1257,7 +1253,7 @@ static int ap_probe(struct usb_interface *interface,
bool bulk_in_found = false;
bool arpc_in_found = false;
- udev = usb_get_dev(interface_to_usbdev(interface));
+ udev = interface_to_usbdev(interface);
num_cports = apb_get_cport_count(udev);
if (num_cports < 0) {
--
2.52.0
Driver core holds a reference to the USB interface and its parent USB
device while the interface is bound to a driver and there is no need to
take additional references unless the structures are needed after
disconnect.
Drop the redundant device reference to reduce cargo culting, make it
easier to spot drivers where an extra reference is needed, and reduce
the risk of memory leaks when drivers fail to release it.
Signed-off-by: Johan Hovold <johan(a)kernel.org>
---
drivers/greybus/es2.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/greybus/es2.c b/drivers/greybus/es2.c
index 6ae0ac828afa..bb513ef7f7ff 100644
--- a/drivers/greybus/es2.c
+++ b/drivers/greybus/es2.c
@@ -806,8 +806,6 @@ static void es2_destroy(struct es2_ap_dev *es2)
udev = es2->usb_dev;
gb_hd_put(es2->hd);
-
- usb_put_dev(udev);
}
static void cport_in_callback(struct urb *urb)
@@ -1257,7 +1255,7 @@ static int ap_probe(struct usb_interface *interface,
bool bulk_in_found = false;
bool arpc_in_found = false;
- udev = usb_get_dev(interface_to_usbdev(interface));
+ udev = interface_to_usbdev(interface);
num_cports = apb_get_cport_count(udev);
if (num_cports < 0) {
--
2.52.0
Replace 'unsigned long long int' with 'unsigned long long' as the
'int' suffix is unnecessary and not preferred by kernel coding style.
Signed-off-by: Oskar Ray-Frayssinet <rayfraytech(a)gmail.com>
---
drivers/staging/greybus/Documentation/firmware/authenticate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/greybus/Documentation/firmware/authenticate.c b/drivers/staging/greybus/Documentation/firmware/authenticate.c
index 3d2c6f88a138..0ef88b7d24de 100644
--- a/drivers/staging/greybus/Documentation/firmware/authenticate.c
+++ b/drivers/staging/greybus/Documentation/firmware/authenticate.c
@@ -58,7 +58,7 @@ int main(int argc, char *argv[])
goto close_fd;
}
- printf("UID received: 0x%llx\n", *(unsigned long long int *)(uid.uid));
+ printf("UID received: 0x%llx\n", *(unsigned long long *)(uid.uid));
/* Get certificate */
printf("Get IMS certificate\n");
--
2.43.0
Hello everyone,
I’ve recently been experimenting with chaining multiple Greybus nodes to
a single host over I²C (QWIIC port [0]). This general idea is not new
(see MicroMod Qwiic Pro Kit [1]), although those setups do not use Greybus.
Since I²C does not allow a slave to initiate transfers, it is not well
suited for node-initiated events (e.g. interrupts, SVC-initiated
control). However, for my current use case I am primarily interested in
polling-based functionality, so this limitation is acceptable.
In a typical Greybus topology, an Application Processor (AP), an SVC,
and one or more modules are connected via UniPro. In practice, because
most application processors lack a native UniPro interface, they connect
through a bridge device that also implements the SVC.
For the I²C-based setup described above, I have considered the following
topologies:
1. Separate co-processor (SVC/Bridge)
This approach is reasonable on SoCs such as TI AM6254, which include
an M4F core that can serve as the SVC/bridge and control the I²C bus.
However, on devices like TI AM62L, which lack such a core, introducing
an additional processor solely for Greybus does not seem justified.
Also, this would make the above much less portable, since it expects a
hardware component, not all BeagleBoard.org boards have.
```
+----+ +---------------+ +----------+
| AP | <--- bus ----> | SVC / Bridge | <--- I2C ----> |
Module |
+----+ +---------------+ +----------+
|
| +----------+
`----------- I2C ----> | Module |
+----------+
```
2. SVC per node
Each node implements its own SVC. Since an I²C slave cannot initiate
communication, the AP must already know the address of each SVC/module.
This also seems inefficient when chaining multiple nodes.
```
+----+ +----------------+
| AP | <--- I2C ---> | SVC / Module |
+----+ +----------------+
|
| +----------------+
`---- I2C ----> | SVC / Module |
+----------------+
```
3. SVC/Bridge functionality inside the AP
For this use case, this seems to be the most practical option.
To clarify, I am not proposing any new data paths in the Greybus
pipeline. The idea is to have a reusable an SVC/bridge implementation
similar to what exists in greybus-zephyr [2][3], but hosted within the AP.
```
+-------------+ +-----------+
| AP / SVC | <--- I2C ---> | Module |
+-------------+ +-----------+
|
| +----------+
`-- I2C ---> | Module |
+----------+
```
Before proceeding further, I would appreciate feedback on this
approach—particularly whether there are concerns with option 3, or if
there are alternative designs I should consider.
Best regards,
Ayush Singh
[0]: https://www.sparkfun.com/qwiic
[1]:
https://www.digikey.in/en/maker/projects/micromod-qwiic-pro-kit-project-gui…
[2]:
https://github.com/beagleboard/greybus-zephyr/blob/main/subsys/greybus/svc.…
[3]:
https://github.com/beagleboard/greybus-zephyr/blob/main/subsys/greybus/apbr…