From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1754279AbdEDPOq (ORCPT <rfc822;w@1wt.eu>);
        Thu, 4 May 2017 11:14:46 -0400
Received: from mga02.intel.com ([134.134.136.20]:62976 "EHLO mga02.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752905AbdEDPOD (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 4 May 2017 11:14:03 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.38,287,1491289200"; 
   d="scan'208";a="1143625956"
Subject: Re: [PATCH 04/16] fpga: intel: pcie: parse feature list and create
 platform device for features.
To: Wu Hao <hao.wu@intel.com>, atull@kernel.org, moritz.fischer@ettus.com,
        linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org
References: <1490875696-15145-1-git-send-email-hao.wu@intel.com>
 <1490875696-15145-5-git-send-email-hao.wu@intel.com>
Cc: luwei.kang@intel.com, yi.z.zhang@intel.com,
        Xiao Guangrong <guangrong.xiao@linux.intel.com>,
        Tim Whisonant <tim.whisonant@intel.com>,
        Enno Luebbers <enno.luebbers@intel.com>,
        Shiva Rao <shiva.rao@intel.com>,
        Christopher Rauer <christopher.rauer@intel.com>
From: "Li, Yi" <yi1.li@linux.intel.com>
Message-ID: <fcd46099-4cc0-66ae-19f7-6c9bc719ff4c@linux.intel.com>
Date: Thu, 4 May 2017 10:13:41 -0500
User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <1490875696-15145-5-git-send-email-hao.wu@intel.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?

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