Date:   Mon, 1 Nov 2021 23:01:21 +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,
        linux-arm-kernel@lists.infradead.org,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] staging: vchiq: Replace global state with per device
 state
Message-ID: <20211101172843.GA17771@ojas>
References: <cover.1635764115.git.ojaswin98@gmail.com>
 <5b51970f601c4bf5165fc70174085a809b3c0316.1635764115.git.ojaswin98@gmail.com>
 <84300618-98c8-0f44-e5a5-8d7fd6c853b0@i2se.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <84300618-98c8-0f44-e5a5-8d7fd6c853b0@i2se.com>
Precedence: bulk

On Mon, Nov 01, 2021 at 01:19:17PM +0100, Stefan Wahren wrote:
> Hi Ojaswin,
> 
> Am 01.11.21 um 12:09 schrieb Ojaswin Mujoo:
> > Currently, the driver has a global g_state variable which is initialised
> > during probe and directly used all over the driver code. However, this
> > prevents the driver to support multiple VideoCore VPUs at the same time.
> >
> > Replace this global state with a per device state which is initialised
> > and allocated during probing.
> >
> > Signed-off-by: Ojaswin Mujoo <ojaswin98@gmail.com>
> ...
> >  
> >  /*
> > @@ -1763,6 +1795,7 @@ static int vchiq_probe(struct platform_device *pdev)
> >  	struct device_node *fw_node;
> >  	const struct of_device_id *of_id;
> >  	struct vchiq_drvdata *drvdata;
> > +	struct vchiq_device *vchiq_dev;
> >  	int err;
> >  
> >  	of_id = of_match_node(vchiq_of_match, pdev->dev.of_node);
> > @@ -1784,7 +1817,18 @@ static int vchiq_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, drvdata);
> >  
> > -	err = vchiq_platform_init(pdev, &g_state);
> > +	vchiq_dev = kzalloc(sizeof(struct vchiq_device), GFP_KERNEL);
> > +	vchiq_dev->state = kzalloc(sizeof(struct vchiq_state), GFP_KERNEL);
> > +	vchiq_dev->vchiq_pdev = *pdev;
> > +
> > +	g_state = vchiq_dev->state;
> > +
> 
> just a quick idea: how about storing the global state within vchiq_drvdata?
> 
> So there is no need to reinvent somekind of vchiq device which is the
> "same" as the platform device. After that you are able to access the
> private driver data via platform_get_drvdata().
Hi Stefan, 

Thank for looking into this. I agree that the vchiq_device is just a
sorta extension of the pdev. However, regarding using the drvdata, I had
some questions.

So I understand the purpose of this patch is to make sure our driver is
able to support multiple devices, that is it can work with say, an SoC
with 2 VideoCore VPUs. In this case, we would need to maintain 2 states
for each VPU instead of a global state, however if we use something like
the following:

 	drvdata = (struct vchiq_drvdata *)of_id->data;
  drvdata->state = kzalloc(sizeof(struct vchiq_state), GFP_KERNEL);
	platform_set_drvdata(pdev, drvdata);
		
Correct me if I'm wrong but I think the assignment to drvdata in line 1
above will return a pointer to the same structure. In which case, the
line 2 will always overwrite the older state that is present. 
That's why I was initialising a separate object (vchiq_device) everytime
the probe was called so that each device can have their own device
specific structs. 

Please let me know if my understanding of drvdata is wrong here.
> 
> Best regards
> 

Thank you!
Ojaswin