| 1) Import drivers using VCHI. |
| |
| VCHI is just a tool to let drivers talk to the firmware. Here are |
| some of the ones we want: |
| |
| - vc_mem (https://github.com/raspberrypi/linux/blob/rpi-4.4.y/drivers/char/broadcom/vc_mem.c) |
| |
| This driver is what the vcdbg userspace program uses to set up its |
| requests to the firmware, which are transmitted across VCHIQ. vcdbg |
| is really useful for debugging firmware interactions. |
| |
| - VCSM (https://github.com/raspberrypi/linux/tree/rpi-4.4.y/drivers/char/broadcom/vc_sm) |
| |
| This driver is used for talking about regions of VC memory across |
| firmware protocols including VCHI. We'll want to extend this driver |
| to manage these buffers as dmabufs so that we can zero-copy import |
| camera images into vc4 for rendering/display. |
| |
| 2) Garbage-collect unused code |
| |
| One of the reasons this driver wasn't upstreamed previously was that |
| there's a lot code that got built that's probably unnecessary these |
| days. Once we have the set of VCHI-using drivers we want in tree, we |
| should be able to do a sweep of the code to see what's left that's |
| unused. |
| |
| 3) Make driver more portable |
| |
| Building this driver with arm/multi_v7_defconfig or arm64/defconfig |
| leads to data corruption during the following command: |
| |
| vchiq_test -f 1 |
| |
| This should be fixed. |
| |
| 4) Fix kernel module support |
| |
| Even the VPU firmware doesn't support a VCHI re-connect, the driver |
| should properly handle a module unload. This also includes that all |
| resouces must be freed (kthreads, debugfs entries, ...) and global |
| variables avoided. |
| |
| 5) Cleanup logging mechanism |
| |
| The driver should probably be using the standard kernel logging mechanisms |
| such as dev_info, dev_dbg, and friends. |
| |
| 6) Documentation |
| |
| A short top-down description of this driver's architecture (function of |
| kthreads, userspace, limitations) could be very helpful for reviewers. |
| |
| 7) Review and comment memory barriers |
| |
| There is a heavy use of memory barriers in this driver, it would be very |
| beneficial to go over all of them and, if correct, comment on their merits. |
| Extra points to whomever confidently reviews the remote_event_*() family of |
| functions. |
| |
| 8) Get rid of custom function return values |
| |
| Most functions use a custom set of return values, we should force proper Linux |
| error numbers. Special care is needed for VCHIQ_RETRY. |
| |
| 9) Reformat core code with more sane indentations |
| |
| The code follows the 80 characters limitation yet tends to go 3 or 4 levels of |
| indentation deep making it very unpleasant to read. This is specially relevant |
| in the character driver ioctl code and in the core thread functions. |
| |
| 10) Reorganize file structure: Move char driver to it's own file and join both |
| platform files |
| |
| The cdev is defined alongside with the platform code in vchiq_arm.c. It would |
| be nice to completely decouple it from the actual core code. For instance to be |
| able to use bcm2835-audio without having /dev/vchiq created. One could argue |
| it's better for security reasons or general cleanliness. It could even be |
| interesting to create two different kernel modules, something the likes of |
| vchiq-core.ko and vchiq-dev.ko. This would also ease the upstreaming process. |
| |
| The code in vchiq_bcm2835_arm.c should fit in the generic platform file. |
| |
| 12) Get rid of all the struct typedefs |
| |
| Most structs are typedefd, it's not encouraged in the kernel. |
| |
| 13) Get rid of all non essential global structures and create a proper per |
| device structure |
| |
| The first thing one generally sees in a probe function is a memory allocation |
| for all the device specific data. This structure is then passed all over the |
| driver. This is good practice since it makes the driver work regardless of the |
| number of devices probed. |