[RFC,18/18] staging: vchiq: add more tasks to the TODO list
diff mbox series

Message ID 20181026134813.7775-19-nsaenzjulienne@suse.de
State New, archived
Headers show
Series
  • staging: vchiq: remove dead code & misc fixes
Related show

Commit Message

Nicolas Saenz Julienne Oct. 26, 2018, 1:48 p.m. UTC
The more the better.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 .../staging/vc04_services/interface/vchi/TODO | 46 ++++++++++++++++++-
 1 file changed, 44 insertions(+), 2 deletions(-)

Comments

Stefan Wahren Oct. 28, 2018, 9:11 p.m. UTC | #1
> Nicolas Saenz Julienne <nsaenzjulienne@suse.de> hat am 26. Oktober 2018 um 15:48 geschrieben:
> 
> 
> The more the better.

Please try to find a better commit log ;-)

> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  .../staging/vc04_services/interface/vchi/TODO | 46 ++++++++++++++++++-
>  1 file changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchi/TODO b/drivers/staging/vc04_services/interface/vchi/TODO
> index 0b3ec75ff627..bf2135826431 100644
> --- a/drivers/staging/vc04_services/interface/vchi/TODO
> +++ b/drivers/staging/vc04_services/interface/vchi/TODO
> @@ -27,8 +27,8 @@ 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: 
> -  
> +leads to data corruption during the following command:
> +

I assume this wasn't intended.

Btw after applying Phil's patch series. This point can be dropped.

Beside that i'm fine with the additions.

Patch
diff mbox series

diff --git a/drivers/staging/vc04_services/interface/vchi/TODO b/drivers/staging/vc04_services/interface/vchi/TODO
index 0b3ec75ff627..bf2135826431 100644
--- a/drivers/staging/vc04_services/interface/vchi/TODO
+++ b/drivers/staging/vc04_services/interface/vchi/TODO
@@ -27,8 +27,8 @@  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: 
-  
+leads to data corruption during the following command:
+
   vchiq_test -f 1
 
 This should be fixed.
@@ -49,3 +49,45 @@  such as dev_info, dev_dbg, and friends.
 
 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.