Re: [PATCH 3/7] drivers: soc: xilinx: add check for platform

[Michal Simek]

On 9/30/23 09:17, Greg KH wrote:
> On Fri, Sep 29, 2023 at 03:55:54AM -0700, Jay Buddhabhatti wrote:
> > Some error event IDs for Versal and Versal NET are different.
> > Both the platforms should access their respective error event
> > IDs so use sub_family_code to check for platform and check
> > error IDs for respective platforms. The family code is passed
> > via platform data to avoid platform detection again.
> > Platform data is setup when even driver is registered.
> >
> > Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@xxxxxxx>
> > ---
> >   drivers/firmware/xilinx/zynqmp.c        |  2 +-
> >   drivers/soc/xilinx/xlnx_event_manager.c | 25 ++++++++++++++++++++-----
> >   include/linux/firmware/xlnx-zynqmp.h    | 16 ++++++++++++----
> >   3 files changed, 33 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> > index 5f40288f69a9..6583efa9ac48 100644
> > --- a/drivers/firmware/xilinx/zynqmp.c
> > +++ b/drivers/firmware/xilinx/zynqmp.c
> > @@ -2019,7 +2019,7 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
> >   	np = of_find_compatible_node(NULL, NULL, "xlnx,versal");
> >   	if (np) {
> >   		em_dev = platform_device_register_data(&pdev->dev, "xlnx_event_manager",
> > -						       -1, NULL, 0);
> > +						       -1, &pm_sub_family_code, 4);
> >   		if (IS_ERR(em_dev))
> >   			dev_err_probe(&pdev->dev, PTR_ERR(em_dev), "EM register fail with error\n");
> >   	}
> > diff --git a/drivers/soc/xilinx/xlnx_event_manager.c b/drivers/soc/xilinx/xlnx_event_manager.c
> > index 38cfc161a713..8074ded7b39c 100644
> > --- a/drivers/soc/xilinx/xlnx_event_manager.c
> > +++ b/drivers/soc/xilinx/xlnx_event_manager.c
> > @@ -23,6 +23,7 @@ static DEFINE_PER_CPU_READ_MOSTLY(int, cpu_number1);
> >   
> >   static int virq_sgi;
> >   static int event_manager_availability = -EACCES;
> > +static u32 pm_sub_family_code;
>
> Why is this not a per-device variable?  Global variables like this for
> drivers are almost always wrong.

Not sure I get what you mean by per-device. Can you please elaborate on it?
This is the part of firmware infrastructure and there is only one instance of 
the driver in the system registered from firmware driver itself.

If this should be done in a generic way then there are much more variables which 
should be moved to driver data not just this one.

Thanks,
Michal