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 <[email protected]>
...
>
> /*
> @@ -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().
Best regards
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 <[email protected]>
> ...
> >
> > /*
> > @@ -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