Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 25 Mar 2023 15:27:36 +0800
From:   Xu Yilun <yilun.xu@intel.com>
To:     Marco Pagani <marpagan@redhat.com>
Cc:     Moritz Fischer <mdf@kernel.org>, Wu Hao <hao.wu@intel.com>,
        Tom Rix <trix@redhat.com>, linux-kernel@vger.kernel.org,
        linux-fpga@vger.kernel.org
Subject: Re: [RFC PATCH v2 2/4] fpga: add fake FPGA bridge
Message-ID: <ZB6iaKEVnVfV3Atl@yilunxu-OptiPlex-7050>
References: <20230310170412.708363-1-marpagan@redhat.com>
 <20230310170412.708363-3-marpagan@redhat.com>
 <ZBQlnUQWZHJ+ZBu5@yilunxu-OptiPlex-7050>
 <988e8e8e-d514-4c69-a384-7d0a70c514c4@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <988e8e8e-d514-4c69-a384-7d0a70c514c4@redhat.com>
Precedence: bulk

On 2023-03-21 at 18:33:05 +0100, Marco Pagani wrote:
> 
> 
> On 2023-03-17 09:32, Xu Yilun wrote:
> > On 2023-03-10 at 18:04:10 +0100, Marco Pagani wrote:
> >> Add fake FPGA bridge driver with support functions. The driver includes
> >> a counter for the number of switching cycles. This module is part of
> >> the KUnit tests for the FPGA subsystem.
> >>
> >> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> >> ---
> >>  drivers/fpga/tests/fake-fpga-bridge.c | 228 ++++++++++++++++++++++++++
> >>  drivers/fpga/tests/fake-fpga-bridge.h |  36 ++++
> >>  2 files changed, 264 insertions(+)
> >>  create mode 100644 drivers/fpga/tests/fake-fpga-bridge.c
> >>  create mode 100644 drivers/fpga/tests/fake-fpga-bridge.h
> >>
> >> diff --git a/drivers/fpga/tests/fake-fpga-bridge.c b/drivers/fpga/tests/fake-fpga-bridge.c
> >> new file mode 100644
> >> index 000000000000..8a2f64fc1bbb
> >> --- /dev/null
> >> +++ b/drivers/fpga/tests/fake-fpga-bridge.c
> >> @@ -0,0 +1,228 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Driver for the fake FPGA bridge
> >> + *
> >> + * Copyright (C) 2023 Red Hat, Inc.
> >> + *
> >> + * Author: Marco Pagani <marpagan@redhat.com>
> >> + */
> >> +
> >> +#include <linux/types.h>
> >> +#include <linux/device.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/fpga/fpga-bridge.h>
> >> +#include <kunit/test.h>
> >> +
> >> +#include "fake-fpga-bridge.h"
> >> +
> >> +#define FAKE_FPGA_BRIDGE_DEV_NAME	"fake_fpga_bridge"
> >> +
> >> +struct fake_bridge_priv {
> >> +	int id;
> >> +	bool enable;
> >> +	int cycles_count;
> >> +	struct kunit *test;
> >> +};
> >> +
> >> +struct fake_bridge_data {
> >> +	struct kunit *test;
> >> +};
> >> +
> >> +static int op_enable_show(struct fpga_bridge *bridge)
> >> +{
> >> +	struct fake_bridge_priv *priv;
> >> +
> >> +	priv = bridge->priv;
> >> +
> >> +	if (priv->test)
> >> +		kunit_info(priv->test, "Fake FPGA bridge %d: enable_show\n",
> >> +			   priv->id);
> > 
> > Why check the kunit pointer every time? I remember you mentioned that
> > the fake fpga modules are expected to be used out of Kunit test, so the
> > priv->test may be NULL? I suggest you work on these usecases in separate
> > patchsets. For now just check priv->test on probe is fine.
> > 
> 
> The idea was to provide additional info messages, tied with the test, if the
> fake bridge is registered with a test instance. If you believe these prints
> are unnecessary, I can remove them or replace them with generic dev_info().

OK, on second thought, it's good to me.

Thanks,
Yilun