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

[Li, Yi]

hi Hao


On 3/30/2017 7:08 AM, Wu Hao wrote:
> From: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Enno Luebbers <enno.luebbers@xxxxxxxxx>
> Signed-off-by: Shiva Rao <shiva.rao@xxxxxxxxx>
> Signed-off-by: Christopher Rauer <christopher.rauer@xxxxxxxxx>
> Signed-off-by: Kang Luwei <luwei.kang@xxxxxxxxx>
> Signed-off-by: Zhang Yi <yi.z.zhang@xxxxxxxxx>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
> Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
> ---
>   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?

> +	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;
> +}
> +
> ....