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

[Ojaswin Mujoo <ojaswin98@gmail.com>]

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:
> > 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
> >
> >
> > 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.
> >
> >   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