Re: [RESEND PATCH v10 0/3] fpga: Use standard class dev_release function

[Xu Yilun <yilun.xu@intel.com>]

On Tue, Jul 27, 2021 at 02:07:04PM -0700, Russ Weight wrote:
> 
> 
> On 7/27/21 10:44 AM, Tom Rix wrote:
> >
> > On 7/27/21 10:16 AM, Russ Weight wrote:
> >>
> >> On 7/27/21 9:55 AM, Tom Rix wrote:
> >>> On 7/27/21 9:38 AM, Russ Weight wrote:
> >>>> On 7/27/21 9:22 AM, Tom Rix wrote:
> >>>>> On 7/27/21 9:15 AM, Russ Weight wrote:
> >>>>>> On 7/27/21 6:24 AM, Tom Rix wrote:
> >>>>>>> On 7/26/21 2:33 PM, Russ Weight wrote:
> >>>>>>>> Hi Moritz,
> >>>>>>>>
> >>>>>>>> I sent this version of the patchset out on July 8th. The only discussion has
> >>>>>>>> been around the compat-id structure. I think there is agreement that this can
> >>>>>>>> be treated separately. What are your thoughts? Do you think this patchset is
> >>>>>>>> ready?
> >>>>>>> While I agree the compat_id can be treated separately, i still have a problem with the 2 register functions.
> >>>>>>>
> >>>>>>> In this discussion,
> >>>>>>>
> >>>>>>> https://lore.kernel.org/linux-fpga/DM6PR11MB38194D113950FA4B75C7F2C485E49@DM6PR11MB3819.namprd11.prod.outlook.com/
> >>>>>>>
> >>>>>>> *register_full(..., *info) becomes
> >>>>>>>
> >>>>>>> *register(..., *info)
> >>>>>>>
> >>>>>>> and
> >>>>>>>
> >>>>>>> *register_simple() is moved out of the public api and becomes private to the subsystem.
> >>>>>> Somehow I missed that part of the email. I'm not sure I understand the part about
> >>>>>> register_simple() "becomes private to the subsystem". Most users would call the simple
> >>>>>> version of the API. Is the proposal to replicate the register_simple() interface
> >>>>>> for each driver that calls it?
> >>>>> Still use *register_simple() internally, so most of your patchset is unchanged.
> >>>>>
> >>>>> Move it out the public api. include/linux/fpga-mgr.h into someplace appropriate in drivers/fpga/
> >>>> Ok - I'll do another spin of the patch set.
> >>>>
> >>>> Just to be clear, *register_full() gets renamed back to *register(). The current *register()
> >>>> goes back to *register_simple(). *register_simple() is an exported symbol, but the function
> >>>> prototype moves out of the public header file into private header file under drivers/fpga.
> >>> Mostly.
> >>>
> >>> If register_simple does not need to be exported, it shouldn't be.
> >> If *register_simple() is to reside in the fpga_mgr or fpga_region module and be
> >> called by other modules, then it has to be exported... unless it is defined as a
> >> static function in a header file. Shall I go that route instead? I could create
> >> a new drivers/fpga/fpga.h file and add the *register_simple() functions there.
> >> Or I could create separate drivers/fpga/fpga-mgr.h and drivers/fpga/fpga-region.h
> >> header files with one static function each.

If you should move the *register_simple() out of the public APIs, then
why you need the function?

> >
> > good point, exporting ok
> 
> Before I do this, can I get others to chime in? There was a previous patchset that
> used *register_simple() and *register(). It seemed that the consensus at that time
> was that it should be *register() and *register_full(). I'm starting to feel like
> I'm going in circles...
> 
> I personally don't have a problem with two public flavors of the register function
> (the way it is now), what do other's think? If it really is a problem to have two
> versions of the register function, maybe I should just change all of the callers
> to declare a structure and call the same register function?

I don't have strong perference of one register function or more. There
are some subsystems with register functions and others with more.

In our case, the *register_full/register(dev, info) is necessary. The
*register_simple(dev, ...) is the wrapper to make life easier for some
drivers, so optional. If there is strong concern about the multiple
register APIs, my suggestion is we start with the necessary one.

Thanks,
Yilun

> 
> - Russ
> 
> 
> >
> > Tom
> >
> >>
> >> - Russ
> >>
> >>> Tom
> >>>
> >>>> - Russ
> >>>>
> >>>>> Tom
> >>>>>
> >>>>>>> Tom
> >>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> - Russ
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 7/26/21 2:27 PM, Russ Weight wrote:
> >>>>>>>>> The FPGA framework has a convention of using managed resource functions
> >>>>>>>>> to allow parent drivers to manage the data structures allocated by the
> >>>>>>>>> class drivers. They use an empty *_dev_release() function to satisfy the
> >>>>>>>>> class driver.
> >>>>>>>>>
> >>>>>>>>> This is inconsistent with linux driver model.
> >>>>>>>>>
> >>>>>>>>> These changes remove the managed resource functions and populate the class
> >>>>>>>>> dev_release callback functions. They also merge the create() and register()
> >>>>>>>>> functions into a single register() or register_full() function for each of
> >>>>>>>>> the fpga-mgr, fpga-region, and fpga-bridge class drivers.
> >>>>>>>>>
> >>>>>>>>> The new *register_full() functions accept an info data structure to provide
> >>>>>>>>> flexibility in passing optional parameters. The *register() functions
> >>>>>>>>> support the legacy parameter list for users that don't require the use of
> >>>>>>>>> optional parameters.
> >>>>>>>>>
> >>>>>>>>> For more context, refer to this email thread:
> >>>>>>>>>
> >>>>>>>>> https://marc.info/?l=linux-fpga&m=162127412218557&w=2
> >>>>>>>>>
> >>>>>>>>> I turned on the configs assocated with each of the modified files, but I
> >>>>>>>>> must have been missing some dependencies, because not all of them compiled.
> >>>>>>>>> I did a run-time test specifically with the dfl-fme infrastructure. This
> >>>>>>>>> would have exercised the region, bridge, and fpga-mgr frameworks.
> >>>>>>>>>
> >>>>>>>>> Changelog v9 -> v10:
> >>>>>>>>>       - Fixed commit messages to reference register_full() instead of
> >>>>>>>>>         register_simple().
> >>>>>>>>>       - Removed the fpga_bridge_register_full() function, because there is
> >>>>>>>>>         not need for it yet. Updated the documentation and commit message
> >>>>>>>>>         accordingly.
> >>>>>>>>>       - Updated documentation to reference the fpga_manager_info and
> >>>>>>>>>         fpga_region_info structures.
> >>>>>>>>>
> >>>>>>>>> Changelog v8 -> v9:
> >>>>>>>>>       - Cleaned up documentation for the FPGA Manager, Bridge, and Region
> >>>>>>>>>         register functions
> >>>>>>>>>       - Renamed fpga_*_register() to fpga_*_register_full()
> >>>>>>>>>       - Renamed fpga_*_register_simple() to fpga_*_register()
> >>>>>>>>>       - Renamed devm_fpga_mgr_register() to devm_fpga_mgr_register_full()
> >>>>>>>>>       - Renamed devm_fpga_mgr_register_simple() to devm_fpga_mgr_register()
> >>>>>>>>>
> >>>>>>>>> Changelog v7 -> v8:
> >>>>>>>>>       - Added reviewed-by tags.
> >>>>>>>>>       - Updated Documentation/driver-api/fpga/ files: fpga-mgr.rst,
> >>>>>>>>>         fpga-bridge.rst, and fpga-region.rst.
> >>>>>>>>>
> >>>>>>>>> Changelog v6 -> v7:
> >>>>>>>>>       - Update the commit messages to describe the new parameters for the
> >>>>>>>>>         *register() functions and to mention the *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 definitions/prototypes to apply const to the new info
> >>>>>>>>>         parameter.
> >>>>>>>>>       - Verify that info->br_ops is non-null in the fpga_bridge_register()
> >>>>>>>>>         function.
> >>>>>>>>>       - Verify a non-null info pointer in the fpga_region_register() function.
> >>>>>>>>>
> >>>>>>>>> Changelog v5 -> v6:
> >>>>>>>>>       - Moved FPGA manager/bridge/region optional parameters out of the ops
> >>>>>>>>>         structure and back into the FPGA class driver structure.
> >>>>>>>>>       - Changed fpga_*_register() function parameters to accept an info data
> >>>>>>>>>         structure to provide flexibility in passing optional parameters.
> >>>>>>>>>       - Added fpga_*_register_simple() functions to support current parameters
> >>>>>>>>>         for users that don't require use of optional parameters.
> >>>>>>>>>
> >>>>>>>>> Changelog v4 -> 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.
> >>>>>>>>>       - Created the fpga_region_ops data structure which is optionally passed
> >>>>>>>>>         to fpga_region_register(). compat_id, the get_bridges() pointer, and
> >>>>>>>>>         the priv pointer are included in the fpga_region_ops structure.
> >>>>>>>>>
> >>>>>>>>> Changelog v3 -> 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.
> >>>>>>>>>       - Added the compat_id parameter to fpga_region_register() to ensure
> >>>>>>>>>         that the compat_id is set before the device_register() call.
> >>>>>>>>>       - Modified the dfl_fpga_feature_devs_enumerate() function to restore
> >>>>>>>>>         the fpga_region_register() call to the correct location.
> >>>>>>>>>
> >>>>>>>>> Changelog v2 -> v3:
> >>>>>>>>>       - Cleaned up comment headers for fpga_mgr_register(), fpga_bridge_register(),
> >>>>>>>>>         and fpga_region_register().
> >>>>>>>>>       - Fixed error return on ida_simple_get() failure for fpga_mgr_register(),
> >>>>>>>>>         fpga_bridge_register(), and fpga_region_register().
> >>>>>>>>>       - Fixed error return value for fpga_bridge_register(): ERR_PTR(ret) instead
> >>>>>>>>>         of NULL.
> >>>>>>>>>
> >>>>>>>>> Changelog v1 -> v2:
> >>>>>>>>>       - Restored devm_fpga_mgr_register() functionality to the fpga-mgr
> >>>>>>>>>         class driver, 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.
> >>>>>>>>>
> >>>>>>>>> Russ Weight (3):
> >>>>>>>>>       fpga: mgr: Use standard dev_release for class driver
> >>>>>>>>>       fpga: bridge: Use standard dev_release for class driver
> >>>>>>>>>       fpga: region: Use standard dev_release for class driver
> >>>>>>>>>
> >>>>>>>>>      Documentation/driver-api/fpga/fpga-bridge.rst |   6 +-
> >>>>>>>>>      Documentation/driver-api/fpga/fpga-mgr.rst    |  38 +++-
> >>>>>>>>>      Documentation/driver-api/fpga/fpga-region.rst |  12 +-
> >>>>>>>>>      drivers/fpga/altera-cvp.c                     |  12 +-
> >>>>>>>>>      drivers/fpga/altera-fpga2sdram.c              |  12 +-
> >>>>>>>>>      drivers/fpga/altera-freeze-bridge.c           |  10 +-
> >>>>>>>>>      drivers/fpga/altera-hps2fpga.c                |  12 +-
> >>>>>>>>>      drivers/fpga/altera-pr-ip-core.c              |   7 +-
> >>>>>>>>>      drivers/fpga/altera-ps-spi.c                  |   9 +-
> >>>>>>>>>      drivers/fpga/dfl-fme-br.c                     |  10 +-
> >>>>>>>>>      drivers/fpga/dfl-fme-mgr.c                    |  22 +-
> >>>>>>>>>      drivers/fpga/dfl-fme-region.c                 |  17 +-
> >>>>>>>>>      drivers/fpga/dfl.c                            |  12 +-
> >>>>>>>>>      drivers/fpga/fpga-bridge.c                    | 122 +++-------
> >>>>>>>>>      drivers/fpga/fpga-mgr.c                       | 215 ++++++++----------
> >>>>>>>>>      drivers/fpga/fpga-region.c                    | 119 ++++------
> >>>>>>>>>      drivers/fpga/ice40-spi.c                      |   9 +-
> >>>>>>>>>      drivers/fpga/machxo2-spi.c                    |   9 +-
> >>>>>>>>>      drivers/fpga/of-fpga-region.c                 |  10 +-
> >>>>>>>>>      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-pr-decoupler.c            |  17 +-
> >>>>>>>>>      drivers/fpga/xilinx-spi.c                     |  11 +-
> >>>>>>>>>      drivers/fpga/zynq-fpga.c                      |  16 +-
> >>>>>>>>>      drivers/fpga/zynqmp-fpga.c                    |   9 +-
> >>>>>>>>>      include/linux/fpga/fpga-bridge.h              |  30 ++-
> >>>>>>>>>      include/linux/fpga/fpga-mgr.h                 |  62 +++--
> >>>>>>>>>      include/linux/fpga/fpga-region.h              |  36 ++-
> >>>>>>>>>      30 files changed, 383 insertions(+), 511 deletions(-)
> >>>>>>>>>
> >