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
Date: Tue, 15 Jun 2021 22:40:42 +0530 [thread overview]
Message-ID: <20210615171042.GA78787@ojas> (raw)
In-Reply-To: <2212368e-b597-b717-0d21-70b24322ca09@i2se.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
prev parent reply other threads:[~2021-06-15 17:11 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-14 19:32 [PATCH 0/1] Request to review progress decoupling vchiq platform code Ojaswin Mujoo
2021-06-14 19:33 ` [PATCH 1/1] staging: vchiq: Move vchiq char driver to its own file Ojaswin Mujoo
2021-06-14 22:06 ` [PATCH 0/1] Request to review progress decoupling vchiq platform code Stefan Wahren
2021-06-15 17:10 ` Ojaswin Mujoo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210615171042.GA78787@ojas \
--to=ojaswin98@gmail.com \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=dan.carpenter@oracle.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=nsaenz@kernel.org \
--cc=phil@raspberrypi.com \
--cc=stefan.wahren@i2se.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).