Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 15 Jun 2021 22:40:42 +0530
From:   Ojaswin Mujoo <ojaswin98@gmail.com>
To:     Stefan Wahren <stefan.wahren@i2se.com>
Cc:     nsaenz@kernel.org, gregkh@linuxfoundation.org,
        dan.carpenter@oracle.com, phil@raspberrypi.com,
        bcm-kernel-feedback-list@broadcom.com,
        linux-arm-kernel@lists.infradead.org,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/1] Request to review progress decoupling vchiq platform
 code
Message-ID: <20210615171042.GA78787@ojas>
References: <cover.1623698773.git.ojaswin98@gmail.com>
 <2212368e-b597-b717-0d21-70b24322ca09@i2se.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2212368e-b597-b717-0d21-70b24322ca09@i2se.com>
Precedence: bulk

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