Re: [PATCH 0/1] Request to review progress decoupling vchiq platform code

On Tue, Jun 15, 2021 at 12:06:14AM +0200, Stefan Wahren wrote:
Hello,

Hi,

Am 14.06.21 um 21:32 schrieb Ojaswin Mujoo:

quoted

Greetings,

I'm working on addressing item 10 of the following TODO list:

    drivers/staging/vc04-services/interface/TODO

For reference, the task is:

    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.


This patch is the first step towards decoupling the platform and the cdev code.
It moves all the cdev related code from vchiq_arm.c to vchiq_dev.c. However, for
now, I have aimed to keep the functionality untouched, hence the platform code
still calls the cdev initialisation function, and isn't truly decoupled yet.

The summary of the changes is as follows:


 *  Definition of functions and variables shared by cdev and platform
    code are moved to vchiq_arm.h while declaration stays in vchiq_arm.c

 *  Declaration and definition of functions and variables only used by
    cdev code are moved to vchiq_dev.c file.

 *  Defined vchiq_deregister_chrdev() and vchiq_register_chrdev(..) in
    vchiq_dev.c which handle cdev creation and deletion. They are called by the
    platfrom code during probe().

looks like this should be 3 separate patches. So you have the pure move
at the end.

Got it, I'll split this into 3 commits:
1. Moving cdev code to a separate function
2. Moving to-be-shared declarations to vchiq_arm.h
3. Finally, moving cdev related code to vchiq_dev.c

quoted

I mainly wanted to put this patch out to see if I have the right idea of the
task at hand and to ensure I'm heading into the right direction. I would love to
hear your thoughts and suggestions on this. Once I have some feedback on this, I
can accordingly work towards a newer version to completely decouple the code. 

Lastly, I had some questions related to the the task: 

1. So regarding the following line in the TODO:

    "For instance to be able to use bcm2835-audio without having /dev/vchiq
    created." 

  I was wondering about the possible ways to achieve this. Specifically, I was
  thinking of the following 2 ways:

  1.1  Making a KConfig entry for Cdev creation, like CONFIG_VCHIQ_CDEV, and
       then do something like:

         vchiq_probe(..) 
         {
           /* platform init code */

           #if defined(CONFIG_VCHIQ_CDEV)

           /* Call cdev register function */

           #endif 
         }

A common pattern is to keep the calls, but have "empty" definitions of
the those functions in the header file in case CONFIG_VCHIQ_CDEV is not
defined.

Ahh okay, I'll try to do that.

quoted

  1.2  The second approach is creating an entirely separate module for the cdev,
       as suggested in the TODO. 

  So I'm just wondering what the right approach should be?

2. Second, I currently tested by installing my patches to a pi3 B+ and running
   `cat /dev/vchiq` to compare the output with the original kernel.  Also, to
   see if the platform code works without the cdev code, I commented out the
   call to vchiq_register_cdev() and made sure the platform device (and
   children) was registered but the char device was not present. However, I'm
   not sure if these tests are comprehensive enough. What would be the right way
   to test my changes?

Sounds okay, but a functional test is still necessary (tool is provided
by Raspberry Pi OS):

vchiq_test -f 10
vchiq_test -p 10

Perfect, this was what I was looking for, thank you! 

Regards
Stefan

I believe, after splitting the patch, the next logical steps would be 

1. Create a patch for adding CONFIG_VCHIQ_CDEV, but not splitting
   modules yet
2. After this, add a final patch to move cdev into it's own module
3. test test test

I can play around with this and see how it goes. Thanks again for the
help Stefan!

Regards,
Ojas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel