Re: [PATCH 04/16] fpga: intel: pcie: parse feature list and create platform device for features.

[Wu Hao <hao.wu@intel.com>]

On Thu, May 04, 2017 at 10:13:41AM -0500, Li, Yi wrote:
> hi Hao
> 
> 
> On 3/30/2017 7:08 AM, Wu Hao wrote:
> >From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >
> >Device Featuer List structure creates a link list of feature headers
> >within the MMIO space to provide an extensiable way of adding features.
> >
> >The Intel FPGA PCIe driver walks through the feature headers to enumerate
> >feature devices, FPGA Management Engine (FME) and FPGA Port for Accelerated
> >Function Unit (AFU), and their private sub features. For feature devices,
> >it creates the platform devices and linked the private sub features into
> >their platform data.
> >
> >Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> >Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> >Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> >Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> >Signed-off-by: Kang Luwei <luwei.kang@intel.com>
> >Signed-off-by: Zhang Yi <yi.z.zhang@intel.com>
> >Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >Signed-off-by: Wu Hao <hao.wu@intel.com>
> >---
> >  drivers/fpga/intel/Makefile      |   2 +-
> >  drivers/fpga/intel/feature-dev.c | 139 +++++++
> >  drivers/fpga/intel/feature-dev.h | 342 ++++++++++++++++
> >  drivers/fpga/intel/pcie.c        | 841 ++++++++++++++++++++++++++++++++++++++-
> >  4 files changed, 1321 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/fpga/intel/feature-dev.c
> >  create mode 100644 drivers/fpga/intel/feature-dev.h
> >
> >.....
> >+
> >+static int
> >+build_info_create_dev(struct build_feature_devs_info *binfo,
> >+		      enum fpga_id_type type, int feature_nr, const char *name)
> >+{
> >+	struct platform_device *fdev;
> >+	struct resource *res;
> >+	struct feature_platform_data *pdata;
> >+	int ret;
> >+
> >+	/* we will create a new device, commit current device first */
> >+	ret = build_info_commit_dev(binfo);
> 
> Looks like the code create the platform device (prepared by previous
> feature) when prepare the current feature binfo, which I found is somewhat
> confusing. Is there a reason to do so?

Hi Yi,

Driver only creates new platform device when it switches to new feature
device parsing (new feature device found in the 'Device Feature List'),
this code just makes sure previous platform device for last feature device
is committed (platform_device_add). If there is no previous feature device
or previous feature device has been already committed, then this function
build_info_commit_dev will return 0 directly.

Thanks
Hao

> 
> >+	if (ret)
> >+		return ret;
> >+
> >+	/*
> >+	 * we use -ENODEV as the initialization indicator which indicates
> >+	 * whether the id need to be reclaimed
> >+	 */
> >+	fdev = binfo->feature_dev = platform_device_alloc(name, -ENODEV);
> >+	if (!fdev)
> >+		return -ENOMEM;
> >+
> >+	fdev->id = alloc_fpga_id(type, &fdev->dev);
> >+	if (fdev->id < 0)
> >+		return fdev->id;
> >+
> >+	fdev->dev.parent = &binfo->parent_dev->dev;
> >+
> >+	/*
> >+	 * we need not care the memory which is associated with the
> >+	 * platform device. After call platform_device_unregister(),
> >+	 * it will be automatically freed by device's
> >+	 * release() callback, platform_device_release().
> >+	 */
> >+	pdata = feature_platform_data_alloc_and_init(fdev, feature_nr);
> >+	if (!pdata)
> >+		return -ENOMEM;
> >+
> >+	/*
> >+	 * the count should be initialized to 0 to make sure
> >+	 *__fpga_port_enable() following __fpga_port_disable()
> >+	 * works properly for port device.
> >+	 * and it should always be 0 for fme device.
> >+	 */
> >+	WARN_ON(pdata->disable_count);
> >+
> >+	fdev->dev.platform_data = pdata;
> >+	fdev->num_resources = feature_nr;
> >+	fdev->resource = kcalloc(feature_nr, sizeof(*res), GFP_KERNEL);
> >+	if (!fdev->resource)
> >+		return -ENOMEM;
> >+
> >+	return 0;
> >+}
> >+
> >....