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
