From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-18.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6A62FC11F68 for ; Wed, 7 Jul 2021 13:29:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5293361C99 for ; Wed, 7 Jul 2021 13:29:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231749AbhGGNbz (ORCPT ); Wed, 7 Jul 2021 09:31:55 -0400 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:29229 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231755AbhGGNbx (ORCPT ); Wed, 7 Jul 2021 09:31:53 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1625664552; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WSsEqV2F4JkcUmtGAVUHu99SvV2WB5pZ0tRvLyx0oOU=; b=TPFqso+DJjN8IcYMRtonikNgnVlJOUuRsiS2tbvii4XyM0OvUqbP8b9Up5Camnunih9C37 tWpG0jeFOJeHqDtOTc2bWsUU41tLn6oISYNKKdPeBtygIiUDXT7JeFwITsLaZpjDyp4lbH yeBN5bbC8TM/hr1J9HygOfwMisGDs3E= Received: from mail-ot1-f70.google.com (mail-ot1-f70.google.com [209.85.210.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-273-_PF08uhNNf6vFC3Vwazsdg-1; Wed, 07 Jul 2021 09:29:11 -0400 X-MC-Unique: _PF08uhNNf6vFC3Vwazsdg-1 Received: by mail-ot1-f70.google.com with SMTP id e7-20020a0568302007b029047bf63db9fbso1629018otp.0 for ; Wed, 07 Jul 2021 06:29:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=WSsEqV2F4JkcUmtGAVUHu99SvV2WB5pZ0tRvLyx0oOU=; b=so0t3Crjn6kWMDzGROIwEUBuhHCwn12JjeCLhXmPy7kXeawBFbOEgJ7IoF1AIzZqB2 lAr9HSwgovyM7R7tcfDXj1YtTzAhm+F16Vmtt16biZoaY+NOk1hb45HriSgyDUMqktB9 REHLn39Hd1dfuNmE38SGls+GZ8gXAIi7pd9Sh0enMyIDTCOdl9AQ/N6UKkGDBoYU4dKg MNTdcgntlg/kdeeL6hE29ics4MaBZa+mhgPHtZcz4qc9hCzG58QkObVP6nKhIf6mMcf3 Ul4+qgi6oPhFzfRVFSHzI69wrtiPYk5+yUE4sU26sN5yOTn1YxmlEKc8Jy0OgabyPkBD vwJg== X-Gm-Message-State: AOAM530PBxnBgOvKkOkL2nMBxv1vHW2kmlabblEAzHNuFHINWH5e+9ua 59BdienrdXklxV183QUFGNyxwYtpNQcCh11qmhWA+3d0AWSxTjuH8EX9Ni4W2aeiytMxcfZcAFT jtm1kmr6jAhr1xuFR5EIPdg== X-Received: by 2002:aca:d4c9:: with SMTP id l192mr8889845oig.5.1625664549751; Wed, 07 Jul 2021 06:29:09 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz8jQityOePLAqjX+csX/y6U9hjKAOwpu9vrvQxkp+ED/+cPi2AjHIMLfkeeaVvPpd+Fi7tfw== X-Received: by 2002:aca:d4c9:: with SMTP id l192mr8889817oig.5.1625664549338; Wed, 07 Jul 2021 06:29:09 -0700 (PDT) Received: from localhost.localdomain (075-142-250-213.res.spectrum.com. [75.142.250.213]) by smtp.gmail.com with ESMTPSA id o20sm3429554ook.40.2021.07.07.06.29.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 07 Jul 2021 06:29:08 -0700 (PDT) Subject: Re: [PATCH v9 1/3] fpga: mgr: Use standard dev_release for class driver To: Russ Weight , mdf@kernel.org, linux-fpga@vger.kernel.org Cc: lgoncalv@redhat.com, yilun.xu@intel.com, hao.wu@intel.com, matthew.gerlach@intel.com, richard.gong@intel.com References: <20210701013733.75483-1-russell.h.weight@intel.com> <20210701013733.75483-2-russell.h.weight@intel.com> From: Tom Rix Message-ID: Date: Wed, 7 Jul 2021 06:29:06 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Precedence: bulk List-ID: X-Mailing-List: linux-fpga@vger.kernel.org On 7/6/21 4:03 PM, Russ Weight wrote: > > On 7/6/21 2:44 PM, Tom Rix wrote: >> On 7/6/21 2:02 PM, Russ Weight wrote: >>> On 7/6/21 12:04 PM, Tom Rix wrote: >>>> Russ, >>>> >>>> My two big concerns are. >>>> >>>> compat_id does not belong in fpga_mgr_info, it belongs in fme_mgr_priv. >>> fpga_mgr_info is a temporary structure on the stack that contains the >>> parameters only long enough to pass them in and have them added to the >>> fpga_manager structure. Do you have a concern about it being on the >>> fpga_manager structure. >> My concern is compat_id should never have been part of fpga-mgr in the first place. >> >> It is dfl specific, so instead of going in a common structure, it should go into a dfl structure >> >> The likely place it should go is fme_mgr_priv. >> >> The need for 2 register functions here and in fpga-region is caused by carrying this dfl data around in the wrong place. > This issue was raised for the previous patch version, and Hao pointed out that > its placement in the fpga_manager structure was a result of community discussion. Everything is a community discussion, I think we got it wrong in the past. > > These changes aren't affecting the location of compat_id. Where it is > stored is not really within the scope of what I am trying to accomplish. > If it needs to be changed, could it be handled in a separate patch set? The location is why you need the *info structs and have a *register_full() that dfl uses and a *register() that everyone else uses. If you only had the *info variant of the register(), the problem would be abstracted somewhat away. Since the public api is changing, I think moving the compat_id first is necessary. Or once it moves the info structs will change/go away and one of the registers()'s will go away. I'll have a move patchset out later today. Tom > > - Russ > >> Tom >> >>>> there are two register functions, there should be one. >>> I get what you are saying. I don't really like the idea of forcing the >>> complexity of the temporary structure on all callers, since only a >>> couple require it, but we can do that if that is the concensus. >>> >>> What do others think? >>> >>>> On 6/30/21 6:37 PM, Russ Weight wrote: >>>>> The FPGA manager class driver data structure is being treated as a >>>>> managed resource instead of using the standard dev_release call-back >>>>> function to release the class data structure. This change removes >>>>> the managed resource code for the freeing of the class data structure >>>>> and combines the create() and register() functions into a single >>>>> register() or register_simple() function. >>>>> >>>>> The register() function accepts an info data structure to provide >>>>> flexibility in passing optional parameters. The register_simple() >>>>> function supports the current parameter list for users that don't >>>>> require the use of optional parameters. >>>>> >>>>> The devm_fpga_mgr_register() function is retained, and the >>>>> devm_fpga_mgr_register_simple() function is added. >>>>> >>>>> Signed-off-by: Russ Weight >>>>> Reviewed-by: Xu Yilun >>>>> --- >>>>> v9: >>>>>     - Cleaned up documentation for the FPGA Manager register functions >>>>>     - Renamed fpga_mgr_register() to fpga_mgr_register_full() >>>>>     - Renamed fpga_mgr_register_simple() to fpga_mgr_register() >>>>>     - Renamed devm_fpga_mgr_register() to devm_fpga_mgr_register_full() >>>>>     - Renamed devm_fpga_mgr_register_simple() to devm_fpga_mgr_register() >>>>> v8: >>>>>     - Added reviewed-by tag. >>>>>     - Updated Documentation/driver-api/fpga/fpga-mgr.rst documentation. >>>>> v7: >>>>>     - Update the commit message to describe the new parameters for the >>>>>       *fpga_mgr_register() functions and to mention the >>>>>       *fpga_mgr_register_simple() functions. >>>>>     - Fix function prototypes in header file to rename dev to parent. >>>>>     - Make use of the PTR_ERR_OR_ZERO() macro when possible. >>>>>     - Some cleanup of comments. >>>>>     - Update function defintions/prototypes to apply const to the new info >>>>>       parameter. >>>>> v6: >>>>>     - Moved FPGA manager optional parameters out of the ops structure and >>>>>       back into the FPGA manager structure. >>>>>     - Changed fpga_mgr_register()/devm_fpga_mgr_register() parameters to >>>>>       accept an info data structure to provide flexibility in passing optional >>>>>       parameters. >>>>>     - Added fpga_mgr_register_simple()/devm_fpga_mgr_register_simple() >>>>>       functions to support current parameters for users that don't require >>>>>       the use of optional parameters. >>>>> v5: >>>>>     - Rebased on top of recently accepted patches. >>>>>     - Removed compat_id from the fpga_mgr_register() parameter list >>>>>       and added it to the fpga_manager_ops structure. This also required >>>>>       dynamically allocating the dfl-fme-ops structure in order to add >>>>>       the appropriate compat_id. >>>>> v4: >>>>>     - Added the compat_id parameter to fpga_mgr_register() and >>>>>       devm_fpga_mgr_register() to ensure that the compat_id is set before >>>>>       the device_register() call. >>>>> v3: >>>>>     - Cleaned up comment header for fpga_mgr_register() >>>>>     - Fix error return on ida_simple_get() failure >>>>> v2: >>>>>     - Restored devm_fpga_mgr_register() functionality, adapted for the combined >>>>>       create/register functionality. >>>>>     - All previous callers of devm_fpga_mgr_register() will continue to call >>>>>       devm_fpga_mgr_register(). >>>>>     - replaced unnecessary ternary operators in return statements with standard >>>>>       if conditions. >>>>> --- >>>>>    Documentation/driver-api/fpga/fpga-mgr.rst |  34 +++- >>>>>    drivers/fpga/altera-cvp.c                  |  12 +- >>>>>    drivers/fpga/altera-pr-ip-core.c           |   7 +- >>>>>    drivers/fpga/altera-ps-spi.c               |   9 +- >>>>>    drivers/fpga/dfl-fme-mgr.c                 |  22 +-- >>>>>    drivers/fpga/fpga-mgr.c                    | 215 +++++++++------------ >>>>>    drivers/fpga/ice40-spi.c                   |   9 +- >>>>>    drivers/fpga/machxo2-spi.c                 |   9 +- >>>>>    drivers/fpga/socfpga-a10.c                 |  16 +- >>>>>    drivers/fpga/socfpga.c                     |   9 +- >>>>>    drivers/fpga/stratix10-soc.c               |  16 +- >>>>>    drivers/fpga/ts73xx-fpga.c                 |   9 +- >>>>>    drivers/fpga/xilinx-spi.c                  |  11 +- >>>>>    drivers/fpga/zynq-fpga.c                   |  16 +- >>>>>    drivers/fpga/zynqmp-fpga.c                 |   9 +- >>>>>    include/linux/fpga/fpga-mgr.h              |  62 ++++-- >>>>>    16 files changed, 210 insertions(+), 255 deletions(-) >>>>> >>>>> diff --git a/Documentation/driver-api/fpga/fpga-mgr.rst b/Documentation/driver-api/fpga/fpga-mgr.rst >>>>> index 4d926b452cb3..826ed8dc9210 100644 >>>>> --- a/Documentation/driver-api/fpga/fpga-mgr.rst >>>>> +++ b/Documentation/driver-api/fpga/fpga-mgr.rst >>>>> @@ -24,7 +24,7 @@ How to support a new FPGA device >>>>>    -------------------------------- >>>>>      To add another FPGA manager, write a driver that implements a set of ops.  The >>>>> -probe function calls fpga_mgr_register(), such as:: >>>>> +probe function calls fpga_mgr_register() or fpga_mgr_register_full(), such as:: >>>>>          static const struct fpga_manager_ops socfpga_fpga_ops = { >>>>>            .write_init = socfpga_fpga_ops_configure_init, >>>>> @@ -49,14 +49,14 @@ probe function calls fpga_mgr_register(), such as:: >>>>>             * them in priv >>>>>             */ >>>>>    -        mgr = devm_fpga_mgr_create(dev, "Altera SOCFPGA FPGA Manager", >>>>> -                       &socfpga_fpga_ops, priv); >>>>> -        if (!mgr) >>>>> -            return -ENOMEM; >>>>> +        mgr = fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager", >>>>> +                    &socfpga_fpga_ops, priv); >>>>> +        if (IS_ERR(mgr)) >>>>> +            return PTR_ERR(mgr); >>>>>              platform_set_drvdata(pdev, mgr); >>>>>    -        return fpga_mgr_register(mgr); >>>>> +        return 0; >>>>>        } >>>>>          static int socfpga_fpga_remove(struct platform_device *pdev) >>>>> @@ -68,6 +68,11 @@ probe function calls fpga_mgr_register(), such as:: >>>>>            return 0; >>>>>        } >>>>>    +Alternatively, the probe function could call one of the resource managed >>>>> +register functions, devm_fpga_mgr_register() or devm_fpga_mgr_register_full(). >>>>> +When these functions are used, the parameter syntax is the same, but the call >>>>> +to fpga_mgr_unregister() should be removed. In the above example, the >>>>> +socfpga_fpga_remove() function would not be required. >>>>>      The ops will implement whatever device specific register writes are needed to >>>>>    do the programming sequence for this particular FPGA.  These ops return 0 for >>>>> @@ -104,8 +109,13 @@ API for implementing a new FPGA Manager driver >>>>>    * ``fpga_mgr_states`` -  Values for :c:expr:`fpga_manager->state`. >>>>>    * struct fpga_manager -  the FPGA manager struct >>>>>    * struct fpga_manager_ops -  Low level FPGA manager driver ops >>>>> -* devm_fpga_mgr_create() -  Allocate and init a manager struct >>>>> -* fpga_mgr_register() -  Register an FPGA manager >>>>> +* fpga_mgr_register_full() -  Create and register an FPGA manager using the >>>>> +  fpga_mgr_info structure to provide the full flexibility of options >>>>> +* fpga_mgr_register() -  Create and register an FPGA manager using standard >>>>> +  arguments >>>>> +* devm_fpga_mgr_register_full() -  Resource managed version of >>>>> +  fpga_mgr_register_full() >>>>> +* devm_fpga_mgr_register() -  Resource managed version of fpga_mgr_register() >>>>>    * fpga_mgr_unregister() -  Unregister an FPGA manager >>>>>      .. kernel-doc:: include/linux/fpga/fpga-mgr.h >>>>> @@ -118,10 +128,16 @@ API for implementing a new FPGA Manager driver >>>>>       :functions: fpga_manager_ops >>>>>      .. kernel-doc:: drivers/fpga/fpga-mgr.c >>>>> -   :functions: devm_fpga_mgr_create >>>>> +   :functions: fpga_mgr_register_full >>>>>      .. kernel-doc:: drivers/fpga/fpga-mgr.c >>>>>       :functions: fpga_mgr_register >>>>>    +.. kernel-doc:: drivers/fpga/fpga-mgr.c >>>>> +   :functions: devm_fpga_mgr_register_full >>>>> + >>>>> +.. kernel-doc:: drivers/fpga/fpga-mgr.c >>>>> +   :functions: devm_fpga_mgr_register >>>>> + >>>>>    .. kernel-doc:: drivers/fpga/fpga-mgr.c >>>>>       :functions: fpga_mgr_unregister >>>>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c >>>>> index ccf4546eff29..4ffb9da537d8 100644 >>>>> --- a/drivers/fpga/altera-cvp.c >>>>> +++ b/drivers/fpga/altera-cvp.c >>>>> @@ -652,19 +652,15 @@ static int altera_cvp_probe(struct pci_dev *pdev, >>>>>        snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%s", >>>>>             ALTERA_CVP_MGR_NAME, pci_name(pdev)); >>>>>    -    mgr = devm_fpga_mgr_create(&pdev->dev, conf->mgr_name, >>>>> -                   &altera_cvp_ops, conf); >>>>> -    if (!mgr) { >>>>> -        ret = -ENOMEM; >>>>> +    mgr = fpga_mgr_register(&pdev->dev, conf->mgr_name, >>>>> +                &altera_cvp_ops, conf); >>>>> +    if (IS_ERR(mgr)) { >>>>> +        ret = PTR_ERR(mgr); >>>>>            goto err_unmap; >>>>>        } >>>>>          pci_set_drvdata(pdev, mgr); >>>>>    -    ret = fpga_mgr_register(mgr); >>>>> -    if (ret) >>>>> -        goto err_unmap; >>>>> - >>>>>        return 0; >>>>>      err_unmap: >>>>> diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c >>>>> index dfdf21ed34c4..be0667968d33 100644 >>>>> --- a/drivers/fpga/altera-pr-ip-core.c >>>>> +++ b/drivers/fpga/altera-pr-ip-core.c >>>>> @@ -191,11 +191,8 @@ int alt_pr_register(struct device *dev, void __iomem *reg_base) >>>>>            (val & ALT_PR_CSR_STATUS_MSK) >> ALT_PR_CSR_STATUS_SFT, >>>>>            (int)(val & ALT_PR_CSR_PR_START)); >>>>>    -    mgr = devm_fpga_mgr_create(dev, dev_name(dev), &alt_pr_ops, priv); >>>>> -    if (!mgr) >>>>> -        return -ENOMEM; >>>>> - >>>>> -    return devm_fpga_mgr_register(dev, mgr); >>>>> +    mgr = devm_fpga_mgr_register(dev, dev_name(dev), &alt_pr_ops, priv); >>>>> +    return PTR_ERR_OR_ZERO(mgr); >>>>>    } >>>>>    EXPORT_SYMBOL_GPL(alt_pr_register); >>>>>    diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c >>>>> index 23bfd4d1ad0f..5e1e009dba89 100644 >>>>> --- a/drivers/fpga/altera-ps-spi.c >>>>> +++ b/drivers/fpga/altera-ps-spi.c >>>>> @@ -302,12 +302,9 @@ static int altera_ps_probe(struct spi_device *spi) >>>>>        snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s %s", >>>>>             dev_driver_string(&spi->dev), dev_name(&spi->dev)); >>>>>    -    mgr = devm_fpga_mgr_create(&spi->dev, conf->mgr_name, >>>>> -                   &altera_ps_ops, conf); >>>>> -    if (!mgr) >>>>> -        return -ENOMEM; >>>>> - >>>>> -    return devm_fpga_mgr_register(&spi->dev, mgr); >>>>> +    mgr = devm_fpga_mgr_register(&spi->dev, conf->mgr_name, >>>>> +                     &altera_ps_ops, conf); >>>>> +    return PTR_ERR_OR_ZERO(mgr); >>>>>    } >>>>>      static const struct spi_device_id altera_ps_spi_ids[] = { >>>>> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c >>>>> index d5861d13b306..620e7874ab53 100644 >>>>> --- a/drivers/fpga/dfl-fme-mgr.c >>>>> +++ b/drivers/fpga/dfl-fme-mgr.c >>>>> @@ -282,7 +282,7 @@ static void fme_mgr_get_compat_id(void __iomem *fme_pr, >>>>>    static int fme_mgr_probe(struct platform_device *pdev) >>>>>    { >>>>>        struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev); >>>>> -    struct fpga_compat_id *compat_id; >>>>> +    struct fpga_manager_info info = { 0 }; >>>>>        struct device *dev = &pdev->dev; >>>>>        struct fme_mgr_priv *priv; >>>>>        struct fpga_manager *mgr; >>>>> @@ -302,20 +302,16 @@ static int fme_mgr_probe(struct platform_device *pdev) >>>>>                return PTR_ERR(priv->ioaddr); >>>>>        } >>>>>    -    compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL); >>>>> -    if (!compat_id) >>>>> +    info.name = "DFL FME FPGA Manager"; >>>>> +    info.mops = &fme_mgr_ops; >>>>> +    info.priv = priv; >>>>> +    info.compat_id = devm_kzalloc(dev, sizeof(*info.compat_id), GFP_KERNEL); >>>>> +    if (!info.compat_id) >>>>>            return -ENOMEM; >>>> could we limp along instead of failing ? >>> The compat_id is used to verify compatibility between the static region and >>> the partial reconfiguration. If there is insufficient memory for the small >>> data structure, I think it is best to fail here. This is also consistent >>> current implementation. >>> >>>>>    -    fme_mgr_get_compat_id(priv->ioaddr, compat_id); >>>>> - >>>>> -    mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager", >>>>> -                   &fme_mgr_ops, priv); >>>>> -    if (!mgr) >>>>> -        return -ENOMEM; >>>>> - >>>>> -    mgr->compat_id = compat_id; >>>>> - >>>>> -    return devm_fpga_mgr_register(dev, mgr); >>>>> +    fme_mgr_get_compat_id(priv->ioaddr, info.compat_id); >>>>> +    mgr = devm_fpga_mgr_register_full(dev, &info); >>>>> +    return PTR_ERR_OR_ZERO(mgr); >>>>>    } >>>>>      static struct platform_driver fme_mgr_driver = { >>>>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c >>>>> index ecb4c3c795fa..5a913aa649fd 100644 >>>>> --- a/drivers/fpga/fpga-mgr.c >>>>> +++ b/drivers/fpga/fpga-mgr.c >>>>> @@ -550,21 +550,19 @@ void fpga_mgr_unlock(struct fpga_manager *mgr) >>>>>    EXPORT_SYMBOL_GPL(fpga_mgr_unlock); >>>>>      /** >>>>> - * fpga_mgr_create - create and initialize an FPGA manager struct >>>>> + * fpga_mgr_register_full - create and register an FPGA Manager device >>>>>     * @parent:    fpga manager device from pdev >>>>> - * @name:    fpga manager name >>>>> - * @mops:    pointer to structure of fpga manager ops >>>>> - * @priv:    fpga manager private data >>>>> + * @info:    parameters for fpga manager >>>>>     * >>>>> - * The caller of this function is responsible for freeing the struct with >>>>> - * fpga_mgr_free().  Using devm_fpga_mgr_create() instead is recommended. >>>>> + * The caller of this function is responsible for calling fpga_mgr_unregister(). >>>>> + * Using devm_fpga_mgr_register_full() instead is recommended. >>>>>     * >>>>> - * Return: pointer to struct fpga_manager or NULL >>>>> + * Return: pointer to struct fpga_manager pointer or ERR_PTR() >>>>>     */ >>>>> -struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name, >>>>> -                     const struct fpga_manager_ops *mops, >>>>> -                     void *priv) >>>>> +struct fpga_manager * >>>>> +fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info) >>>>>    { >>>>> +    const struct fpga_manager_ops *mops = info->mops; >>>>>        struct fpga_manager *mgr; >>>>>        int id, ret; >>>>>    @@ -572,29 +570,31 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name, >>>>>            !mops->write_init || (!mops->write && !mops->write_sg) || >>>>>            (mops->write && mops->write_sg)) { >>>>>            dev_err(parent, "Attempt to register without fpga_manager_ops\n"); >>>>> -        return NULL; >>>>> +        return ERR_PTR(-EINVAL); >>>>>        } >>>>>    -    if (!name || !strlen(name)) { >>>>> +    if (!info->name || !strlen(info->name)) { >>>>>            dev_err(parent, "Attempt to register with no name!\n"); >>>>> -        return NULL; >>>>> +        return ERR_PTR(-EINVAL); >>>>>        } >>>>>          mgr = kzalloc(sizeof(*mgr), GFP_KERNEL); >>>>>        if (!mgr) >>>>> -        return NULL; >>>>> +        return ERR_PTR(-ENOMEM); >>>>>          id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL); >>>>> -    if (id < 0) >>>>> +    if (id < 0) { >>>>> +        ret = id; >>>>>            goto error_kfree; >>>>> +    } >>>>>          mutex_init(&mgr->ref_mutex); >>>>>    -    mgr->name = name; >>>>> -    mgr->mops = mops; >>>>> -    mgr->priv = priv; >>>>> +    mgr->name = info->name; >>>>> +    mgr->mops = info->mops; >>>>> +    mgr->priv = info->priv; >>>>> +    mgr->compat_id = info->compat_id; >>>>>    -    device_initialize(&mgr->dev); >>>>>        mgr->dev.class = fpga_mgr_class; >>>>>        mgr->dev.groups = mops->groups; >>>>>        mgr->dev.parent = parent; >>>>> @@ -605,6 +605,19 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name, >>>>>        if (ret) >>>>>            goto error_device; >>>>>    +    /* >>>>> +     * Initialize framework state by requesting low level driver read state >>>>> +     * from device.  FPGA may be in reset mode or may have been programmed >>>>> +     * by bootloader or EEPROM. >>>>> +     */ >>>>> +    mgr->state = mgr->mops->state(mgr); >>>> This is existing code, but it seems wrong. >>>> >>>> A side effect of state() should not be to initialize. >>>> >>>> An init() should be added to mops >>> I think this is just capturing the state of the underlying device an >>> reflecting it in the fpga-manager state? Maybe someone else can comment >>> on this. It seems outside the scope of these patches. >>> >>>>> + >>>>> +    ret = device_register(&mgr->dev); >>>>> +    if (ret) { >>>>> +        put_device(&mgr->dev); >>>>> +        return ERR_PTR(ret); >>>>> +    } >>>>> + >>>>>        return mgr; >>>>>      error_device: >>>>> @@ -612,96 +625,36 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name, >>>>>    error_kfree: >>>>>        kfree(mgr); >>>>>    -    return NULL; >>>>> +    return ERR_PTR(ret); >>>>>    } >>>>> -EXPORT_SYMBOL_GPL(fpga_mgr_create); >>>>> +EXPORT_SYMBOL_GPL(fpga_mgr_register_full); >>>>>      /** >>>>> - * fpga_mgr_free - free an FPGA manager created with fpga_mgr_create() >>>>> - * @mgr:    fpga manager struct >>>>> - */ >>>>> -void fpga_mgr_free(struct fpga_manager *mgr) >>>>> -{ >>>>> -    ida_simple_remove(&fpga_mgr_ida, mgr->dev.id); >>>>> -    kfree(mgr); >>>>> -} >>>>> -EXPORT_SYMBOL_GPL(fpga_mgr_free); >>>>> - >>>>> -static void devm_fpga_mgr_release(struct device *dev, void *res) >>>>> -{ >>>>> -    struct fpga_mgr_devres *dr = res; >>>>> - >>>>> -    fpga_mgr_free(dr->mgr); >>>>> -} >>>>> - >>>>> -/** >>>>> - * devm_fpga_mgr_create - create and initialize a managed FPGA manager struct >>>>> + * fpga_mgr_register - create and register an FPGA Manager device >>>>>     * @parent:    fpga manager device from pdev >>>>>     * @name:    fpga manager name >>>>>     * @mops:    pointer to structure of fpga manager ops >>>>>     * @priv:    fpga manager private data >>>>>     * >>>>> - * This function is intended for use in an FPGA manager driver's probe function. >>>>> - * After the manager driver creates the manager struct with >>>>> - * devm_fpga_mgr_create(), it should register it with fpga_mgr_register().  The >>>>> - * manager driver's remove function should call fpga_mgr_unregister().  The >>>>> - * manager struct allocated with this function will be freed automatically on >>>>> - * driver detach.  This includes the case of a probe function returning error >>>>> - * before calling fpga_mgr_register(), the struct will still get cleaned up. >>>>> + * The caller of this function is responsible for calling fpga_mgr_unregister(). >>>>> + * Using devm_fpga_mgr_register() instead is recommended. This simple >>>>> + * version of the register function should be sufficient for most users. The >>>>> + * fpga_mgr_register_full() function is available for users that need to pass >>>>> + * additional, optional parameters. >>>>>     * >>>>> - * Return: pointer to struct fpga_manager or NULL >>>>> + * Return: pointer to struct fpga_manager pointer or ERR_PTR() >>>>>     */ >>>>> -struct fpga_manager *devm_fpga_mgr_create(struct device *parent, const char *name, >>>>> -                      const struct fpga_manager_ops *mops, >>>>> -                      void *priv) >>>>> +struct fpga_manager * >>>>> +fpga_mgr_register(struct device *parent, const char *name, >>>>> +          const struct fpga_manager_ops *mops, void *priv) >>>>>    { >>>>> -    struct fpga_mgr_devres *dr; >>>>> +    struct fpga_manager_info info = { 0 }; >>>>>    -    dr = devres_alloc(devm_fpga_mgr_release, sizeof(*dr), GFP_KERNEL); >>>>> -    if (!dr) >>>>> -        return NULL; >>>>> +    info.name = name; >>>>> +    info.mops = mops; >>>>> +    info.priv = priv; >>>>>    -    dr->mgr = fpga_mgr_create(parent, name, mops, priv); >>>>> -    if (!dr->mgr) { >>>>> -        devres_free(dr); >>>>> -        return NULL; >>>>> -    } >>>>> - >>>>> -    devres_add(parent, dr); >>>>> - >>>>> -    return dr->mgr; >>>>> -} >>>>> -EXPORT_SYMBOL_GPL(devm_fpga_mgr_create); >>>>> - >>>>> -/** >>>>> - * fpga_mgr_register - register an FPGA manager >>>>> - * @mgr: fpga manager struct >>>>> - * >>>>> - * Return: 0 on success, negative error code otherwise. >>>>> - */ >>>>> -int fpga_mgr_register(struct fpga_manager *mgr) >>>>> -{ >>>>> -    int ret; >>>>> - >>>>> -    /* >>>>> -     * Initialize framework state by requesting low level driver read state >>>>> -     * from device.  FPGA may be in reset mode or may have been programmed >>>>> -     * by bootloader or EEPROM. >>>>> -     */ >>>>> -    mgr->state = mgr->mops->state(mgr); >>>>> - >>>>> -    ret = device_add(&mgr->dev); >>>>> -    if (ret) >>>>> -        goto error_device; >>>>> - >>>>> -    dev_info(&mgr->dev, "%s registered\n", mgr->name); >>>>> - >>>>> -    return 0; >>>>> - >>>>> -error_device: >>>>> -    ida_simple_remove(&fpga_mgr_ida, mgr->dev.id); >>>>> - >>>>> -    return ret; >>>>> +    return fpga_mgr_register_full(parent, &info); >>>>>    } >>>>>    EXPORT_SYMBOL_GPL(fpga_mgr_register); >>>>>    @@ -726,14 +679,6 @@ void fpga_mgr_unregister(struct fpga_manager *mgr) >>>>>    } >>>>>    EXPORT_SYMBOL_GPL(fpga_mgr_unregister); >>>>>    -static int fpga_mgr_devres_match(struct device *dev, void *res, >>>>> -                 void *match_data) >>>>> -{ >>>>> -    struct fpga_mgr_devres *dr = res; >>>>> - >>>>> -    return match_data == dr->mgr; >>>>> -} >>>>> - >>>>>    static void devm_fpga_mgr_unregister(struct device *dev, void *res) >>>>>    { >>>>>        struct fpga_mgr_devres *dr = res; >>>>> @@ -742,45 +687,67 @@ static void devm_fpga_mgr_unregister(struct device *dev, void *res) >>>>>    } >>>>>      /** >>>>> - * devm_fpga_mgr_register - resource managed variant of fpga_mgr_register() >>>>> - * @dev: managing device for this FPGA manager >>>>> - * @mgr: fpga manager struct >>>>> + * devm_fpga_mgr_register_full - resource managed variant of fpga_mgr_register() >>>>> + * @parent:    fpga manager device from pdev >>>>> + * @info:    parameters for fpga manager >>>>>     * >>>>> - * This is the devres variant of fpga_mgr_register() for which the unregister >>>>> + * This is the devres variant of fpga_mgr_register_full() for which the unregister >>>>>     * function will be called automatically when the managing device is detached. >>>>>     */ >>>>> -int devm_fpga_mgr_register(struct device *dev, struct fpga_manager *mgr) >>>>> +struct fpga_manager * >>>>> +devm_fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info) >>>>>    { >>>>>        struct fpga_mgr_devres *dr; >>>>> -    int ret; >>>>> - >>>>> -    /* >>>>> -     * Make sure that the struct fpga_manager * that is passed in is >>>>> -     * managed itself. >>>>> -     */ >>>>> -    if (WARN_ON(!devres_find(dev, devm_fpga_mgr_release, >>>>> -                 fpga_mgr_devres_match, mgr))) >>>>> -        return -EINVAL; >>>>> +    struct fpga_manager *mgr; >>>>>          dr = devres_alloc(devm_fpga_mgr_unregister, sizeof(*dr), GFP_KERNEL); >>>>>        if (!dr) >>>>> -        return -ENOMEM; >>>>> +        return ERR_PTR(-ENOMEM); >>>>>    -    ret = fpga_mgr_register(mgr); >>>>> -    if (ret) { >>>>> +    mgr = fpga_mgr_register_full(parent, info); >>>>> +    if (IS_ERR(mgr)) { >>>>>            devres_free(dr); >>>>> -        return ret; >>>>> +        return mgr; >>>>>        } >>>>>          dr->mgr = mgr; >>>>> -    devres_add(dev, dr); >>>>> +    devres_add(parent, dr); >>>>>    -    return 0; >>>>> +    return mgr; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(devm_fpga_mgr_register_full); >>>>> + >>>>> +/** >>>>> + * devm_fpga_mgr_register - resource managed variant of fpga_mgr_register() >>>>> + * @parent:    fpga manager device from pdev >>>>> + * @name:    fpga manager name >>>>> + * @mops:    pointer to structure of fpga manager ops >>>>> + * @priv:    fpga manager private data >>>>> + * >>>>> + * This is the devres variant of fpga_mgr_register() for which the >>>>> + * unregister function will be called automatically when the managing >>>>> + * device is detached. >>>>> + */ >>>>> +struct fpga_manager * >>>>> +devm_fpga_mgr_register(struct device *parent, const char *name, >>>>> +               const struct fpga_manager_ops *mops, void *priv) >>>>> +{ >>>>> +    struct fpga_manager_info info = { 0 }; >>>>> + >>>>> +    info.name = name; >>>>> +    info.mops = mops; >>>>> +    info.priv = priv; >>>>> + >>>>> +    return devm_fpga_mgr_register_full(parent, &info); >>>>>    } >>>>>    EXPORT_SYMBOL_GPL(devm_fpga_mgr_register); >>>>>      static void fpga_mgr_dev_release(struct device *dev) >>>>>    { >>>>> +    struct fpga_manager *mgr = to_fpga_manager(dev); >>>>> + >>>>> +    ida_simple_remove(&fpga_mgr_ida, mgr->dev.id); >>>>> +    kfree(mgr); >>>>>    } >>>>>      static int __init fpga_mgr_class_init(void) >>>>> diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c >>>>> index 69dec5af23c3..b7591912248f 100644 >>>>> --- a/drivers/fpga/ice40-spi.c >>>>> +++ b/drivers/fpga/ice40-spi.c >>>>> @@ -178,12 +178,9 @@ static int ice40_fpga_probe(struct spi_device *spi) >>>>>            return ret; >>>>>        } >>>>>    -    mgr = devm_fpga_mgr_create(dev, "Lattice iCE40 FPGA Manager", >>>>> -                   &ice40_fpga_ops, priv); >>>>> -    if (!mgr) >>>>> -        return -ENOMEM; >>>>> - >>>>> -    return devm_fpga_mgr_register(dev, mgr); >>>>> +    mgr = devm_fpga_mgr_register(dev, "Lattice iCE40 FPGA Manager", >>>>> +                     &ice40_fpga_ops, priv); >>>>> +    return PTR_ERR_OR_ZERO(mgr); >>>>>    } >>>>>      static const struct of_device_id ice40_fpga_of_match[] = { >>>>> diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c >>>>> index 1afb41aa20d7..8469dfb5dd21 100644 >>>>> --- a/drivers/fpga/machxo2-spi.c >>>>> +++ b/drivers/fpga/machxo2-spi.c >>>>> @@ -366,12 +366,9 @@ static int machxo2_spi_probe(struct spi_device *spi) >>>>>            return -EINVAL; >>>>>        } >>>>>    -    mgr = devm_fpga_mgr_create(dev, "Lattice MachXO2 SPI FPGA Manager", >>>>> -                   &machxo2_ops, spi); >>>>> -    if (!mgr) >>>>> -        return -ENOMEM; >>>>> - >>>>> -    return devm_fpga_mgr_register(dev, mgr); >>>>> +    mgr = devm_fpga_mgr_register(dev, "Lattice MachXO2 SPI FPGA Manager", >>>>> +                     &machxo2_ops, spi); >>>>> +    return PTR_ERR_OR_ZERO(mgr); >>>>>    } >>>>>      #ifdef CONFIG_OF >>>>> diff --git a/drivers/fpga/socfpga-a10.c b/drivers/fpga/socfpga-a10.c >>>>> index 573d88bdf730..ac8e89b8a5cc 100644 >>>>> --- a/drivers/fpga/socfpga-a10.c >>>>> +++ b/drivers/fpga/socfpga-a10.c >>>>> @@ -508,19 +508,15 @@ static int socfpga_a10_fpga_probe(struct platform_device *pdev) >>>>>            return -EBUSY; >>>>>        } >>>>>    -    mgr = devm_fpga_mgr_create(dev, "SoCFPGA Arria10 FPGA Manager", >>>>> -                   &socfpga_a10_fpga_mgr_ops, priv); >>>>> -    if (!mgr) >>>>> -        return -ENOMEM; >>>>> - >>>>> -    platform_set_drvdata(pdev, mgr); >>>>> - >>>>> -    ret = fpga_mgr_register(mgr); >>>>> -    if (ret) { >>>>> +    mgr = fpga_mgr_register(dev, "SoCFPGA Arria10 FPGA Manager", >>>>> +                &socfpga_a10_fpga_mgr_ops, priv); >>>>> +    if (IS_ERR(mgr)) { >>>>>            clk_disable_unprepare(priv->clk); >>>>> -        return ret; >>>>> +        return PTR_ERR(mgr); >>>>>        } >>>>>    +    platform_set_drvdata(pdev, mgr); >>>>> + >>>>>        return 0; >>>>>    } >>>>>    diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c >>>>> index 1f467173fc1f..7e0741f99696 100644 >>>>> --- a/drivers/fpga/socfpga.c >>>>> +++ b/drivers/fpga/socfpga.c >>>>> @@ -571,12 +571,9 @@ static int socfpga_fpga_probe(struct platform_device *pdev) >>>>>        if (ret) >>>>>            return ret; >>>>>    -    mgr = devm_fpga_mgr_create(dev, "Altera SOCFPGA FPGA Manager", >>>>> -                   &socfpga_fpga_ops, priv); >>>>> -    if (!mgr) >>>>> -        return -ENOMEM; >>>>> - >>>>> -    return devm_fpga_mgr_register(dev, mgr); >>>>> +    mgr = devm_fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager", >>>>> +                     &socfpga_fpga_ops, priv); >>>>> +    return PTR_ERR_OR_ZERO(mgr); >>>>>    } >>>>>      #ifdef CONFIG_OF >>>>> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c >>>>> index a2cea500f7cc..9155e888a133 100644 >>>>> --- a/drivers/fpga/stratix10-soc.c >>>>> +++ b/drivers/fpga/stratix10-soc.c >>>>> @@ -425,18 +425,11 @@ static int s10_probe(struct platform_device *pdev) >>>>>          init_completion(&priv->status_return_completion); >>>>>    -    mgr = fpga_mgr_create(dev, "Stratix10 SOC FPGA Manager", >>>>> -                  &s10_ops, priv); >>>>> -    if (!mgr) { >>>>> -        dev_err(dev, "unable to create FPGA manager\n"); >>>>> -        ret = -ENOMEM; >>>>> -        goto probe_err; >>>>> -    } >>>>> - >>>>> -    ret = fpga_mgr_register(mgr); >>>>> -    if (ret) { >>>>> +    mgr = fpga_mgr_register(dev, "Stratix10 SOC FPGA Manager", >>>>> +                &s10_ops, priv); >>>>> +    if (IS_ERR(mgr)) { >>>>>            dev_err(dev, "unable to register FPGA manager\n"); >>>>> -        fpga_mgr_free(mgr); >>>>> +        ret = PTR_ERR(mgr); >>>>>            goto probe_err; >>>>>        } >>>>>    @@ -454,7 +447,6 @@ static int s10_remove(struct platform_device *pdev) >>>>>        struct s10_priv *priv = mgr->priv; >>>>>          fpga_mgr_unregister(mgr); >>>>> -    fpga_mgr_free(mgr); >>>>>        stratix10_svc_free_channel(priv->chan); >>>>>          return 0; >>>>> diff --git a/drivers/fpga/ts73xx-fpga.c b/drivers/fpga/ts73xx-fpga.c >>>>> index 101f016c6ed8..fd52de20ddb2 100644 >>>>> --- a/drivers/fpga/ts73xx-fpga.c >>>>> +++ b/drivers/fpga/ts73xx-fpga.c >>>>> @@ -122,12 +122,9 @@ static int ts73xx_fpga_probe(struct platform_device *pdev) >>>>>        if (IS_ERR(priv->io_base)) >>>>>            return PTR_ERR(priv->io_base); >>>>>    -    mgr = devm_fpga_mgr_create(kdev, "TS-73xx FPGA Manager", >>>>> -                   &ts73xx_fpga_ops, priv); >>>>> -    if (!mgr) >>>>> -        return -ENOMEM; >>>>> - >>>>> -    return devm_fpga_mgr_register(kdev, mgr); >>>>> +    mgr = devm_fpga_mgr_register(kdev, "TS-73xx FPGA Manager", >>>>> +                     &ts73xx_fpga_ops, priv); >>>>> +    return PTR_ERR_OR_ZERO(mgr); >>>>>    } >>>>>      static struct platform_driver ts73xx_fpga_driver = { >>>>> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c >>>>> index fee4d0abf6bf..db23626f9ab2 100644 >>>>> --- a/drivers/fpga/xilinx-spi.c >>>>> +++ b/drivers/fpga/xilinx-spi.c >>>>> @@ -247,13 +247,10 @@ static int xilinx_spi_probe(struct spi_device *spi) >>>>>            return dev_err_probe(&spi->dev, PTR_ERR(conf->done), >>>>>                         "Failed to get DONE gpio\n"); >>>>>    -    mgr = devm_fpga_mgr_create(&spi->dev, >>>>> -                   "Xilinx Slave Serial FPGA Manager", >>>>> -                   &xilinx_spi_ops, conf); >>>>> -    if (!mgr) >>>>> -        return -ENOMEM; >>>>> - >>>>> -    return devm_fpga_mgr_register(&spi->dev, mgr); >>>>> +    mgr = devm_fpga_mgr_register(&spi->dev, >>>>> +                     "Xilinx Slave Serial FPGA Manager", >>>>> +                     &xilinx_spi_ops, conf); >>>>> +    return PTR_ERR_OR_ZERO(mgr); >>>>>    } >>>>>      static const struct of_device_id xlnx_spi_of_match[] = { >>>>> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c >>>>> index 9b75bd4f93d8..426aa34c6a0d 100644 >>>>> --- a/drivers/fpga/zynq-fpga.c >>>>> +++ b/drivers/fpga/zynq-fpga.c >>>>> @@ -609,20 +609,16 @@ static int zynq_fpga_probe(struct platform_device *pdev) >>>>>          clk_disable(priv->clk); >>>>>    -    mgr = devm_fpga_mgr_create(dev, "Xilinx Zynq FPGA Manager", >>>>> -                   &zynq_fpga_ops, priv); >>>>> -    if (!mgr) >>>>> -        return -ENOMEM; >>>>> - >>>>> -    platform_set_drvdata(pdev, mgr); >>>>> - >>>>> -    err = fpga_mgr_register(mgr); >>>>> -    if (err) { >>>>> +    mgr = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager", >>>>> +                &zynq_fpga_ops, priv); >>>>> +    if (IS_ERR(mgr)) { >>>>>            dev_err(dev, "unable to register FPGA manager\n"); >>>>>            clk_unprepare(priv->clk); >>>>> -        return err; >>>>> +        return PTR_ERR(mgr); >>>>>        } >>>>>    +    platform_set_drvdata(pdev, mgr); >>>>> + >>>>>        return 0; >>>>>    } >>>>>    diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c >>>>> index 125743c9797f..975bdc4d0a65 100644 >>>>> --- a/drivers/fpga/zynqmp-fpga.c >>>>> +++ b/drivers/fpga/zynqmp-fpga.c >>>>> @@ -102,12 +102,9 @@ static int zynqmp_fpga_probe(struct platform_device *pdev) >>>>>          priv->dev = dev; >>>>>    -    mgr = devm_fpga_mgr_create(dev, "Xilinx ZynqMP FPGA Manager", >>>>> -                   &zynqmp_fpga_ops, priv); >>>>> -    if (!mgr) >>>>> -        return -ENOMEM; >>>>> - >>>>> -    return devm_fpga_mgr_register(dev, mgr); >>>>> +    mgr = devm_fpga_mgr_register(dev, "Xilinx ZynqMP FPGA Manager", >>>>> +                     &zynqmp_fpga_ops, priv); >>>>> +    return PTR_ERR_OR_ZERO(mgr); >>>>>    } >>>>>      static const struct of_device_id zynqmp_fpga_of_match[] = { >>>>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h >>>>> index 474c1f506307..ac3ddaf2903c 100644 >>>>> --- a/include/linux/fpga/fpga-mgr.h >>>>> +++ b/include/linux/fpga/fpga-mgr.h >>>>> @@ -105,6 +105,36 @@ struct fpga_image_info { >>>>>    #endif >>>>>    }; >>>>>    +/** >>>>> + * struct fpga_compat_id - id for compatibility check >>>>> + * >>>>> + * @id_h: high 64bit of the compat_id >>>>> + * @id_l: low 64bit of the compat_id >>>>> + */ >>>>> +struct fpga_compat_id { >>>>> +    u64 id_h; >>>>> +    u64 id_l; >>>>> +}; >>>>> + >>>> There are two register functions are to handle this dfl only structure. >>>> >>>> This really needs to be moved fme_mgr_priv. >>>> >>>>> +/** >>>>> + * struct fpga_manager_info - collection of parameters for an FPGA Manager >>>>> + * @name: fpga manager name >>>>> + * @compat_id: FPGA manager id for compatibility check. >>>>> + * @mops: pointer to structure of fpga manager ops >>>>> + * @priv: fpga manager private data >>>>> + * >>>>> + * fpga_manager_info contains parameters for the register function. These >>>>> + * are separated into an info structure because they some are optional >>>>> + * others could be added to in the future. The info structure facilitates >>>>> + * maintaining a stable API. >>>>> + */ >>>>> +struct fpga_manager_info { >>>>> +    const char *name; >>>>> +    struct fpga_compat_id *compat_id; >>>>> +    const struct fpga_manager_ops *mops; >>>>> +    void *priv; >>>>> +}; >>>>> + >>>>>    /** >>>>>     * struct fpga_manager_ops - ops for low level fpga manager drivers >>>>>     * @initial_header_size: Maximum number of bytes that should be passed into write_init >>>>> @@ -143,17 +173,6 @@ struct fpga_manager_ops { >>>>>    #define FPGA_MGR_STATUS_IP_PROTOCOL_ERR        BIT(3) >>>>>    #define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR    BIT(4) >>>>>    -/** >>>>> - * struct fpga_compat_id - id for compatibility check >>>>> - * >>>>> - * @id_h: high 64bit of the compat_id >>>>> - * @id_l: low 64bit of the compat_id >>>>> - */ >>>>> -struct fpga_compat_id { >>>>> -    u64 id_h; >>>>> -    u64 id_l; >>>>> -}; >>>>> - >>>>>    /** >>>>>     * struct fpga_manager - fpga manager structure >>>>>     * @name: name of low level fpga manager >>>>> @@ -191,17 +210,18 @@ struct fpga_manager *fpga_mgr_get(struct device *dev); >>>>>      void fpga_mgr_put(struct fpga_manager *mgr); >>>>>    -struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name, >>>>> -                     const struct fpga_manager_ops *mops, >>>>> -                     void *priv); >>>>> -void fpga_mgr_free(struct fpga_manager *mgr); >>>> removing these should cause any oot driver to fail to load. >>>> >>>> so an oops is unlikely. >>>> >>>> I'm ok with with api change. >>>> >>>>> -int fpga_mgr_register(struct fpga_manager *mgr); >>>>> -void fpga_mgr_unregister(struct fpga_manager *mgr); >>>>> +struct fpga_manager * >>>>> +fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info); >>>>>    -int devm_fpga_mgr_register(struct device *dev, struct fpga_manager *mgr); >>>>> +struct fpga_manager * >>>>> +fpga_mgr_register(struct device *parent, const char *name, >>>>> +          const struct fpga_manager_ops *mops, void *priv); >>>> having 2 ways to do the same thing is not great. >>>> >>>> this should be reduced down to 1. >>>> >>>> My preference is for the *info one as it's api will be durable. >>> I would like to see if other's agree. >>> >>>> Tom >>>> >>>>> +void fpga_mgr_unregister(struct fpga_manager *mgr); >>>>>    -struct fpga_manager *devm_fpga_mgr_create(struct device *dev, const char *name, >>>>> -                      const struct fpga_manager_ops *mops, >>>>> -                      void *priv); >>>>> +struct fpga_manager * >>>>> +devm_fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info); >>>>> +struct fpga_manager * >>>>> +devm_fpga_mgr_register(struct device *parent, const char *name, >>>>> +               const struct fpga_manager_ops *mops, void *priv); >>>>>      #endif /*_LINUX_FPGA_MGR_H */