All of lore.kernel.org
 help / color / mirror / Atom feed
From: Moritz Fischer <mdf@kernel.org>
To: Tom Rix <trix@redhat.com>
Cc: Moritz Fischer <mdf@kernel.org>,
	linux-fpga@vger.kernel.org, hao.wu@intel.com,
	michal.simek@xilinx.com, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, russell.h.weight@intel.com,
	matthew.gerlach@intel.com
Subject: Re: [PATCH 10/10] fpga: fpga-mgr: altera-pr-ip: Simplify registration
Date: Sun, 4 Oct 2020 16:39:14 -0700	[thread overview]
Message-ID: <20201004233914.GA111357@epycbox.lan> (raw)
In-Reply-To: <a49b1d7c-9756-1059-f7a1-25dae460d659@redhat.com>

On Sun, Oct 04, 2020 at 11:47:26AM -0700, Tom Rix wrote:
> 
> On 10/3/20 10:14 PM, Moritz Fischer wrote:
> > Simplify registration using new devm_fpga_mgr_register() API.
> > Remove the now obsolete altera_pr_unregister() function.
> >
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> >
> > We should take another look at this, IIRC correctly the point of
> > splitting this up into a separate driver was to make it useable by a
> > different (pci?) driver later on.
> >
> > It doesn't seem like this happened, and I think we should just make this
> > a platform driver?
> >
> > ---
> >  drivers/fpga/altera-pr-ip-core-plat.c  | 10 ----------
> >  drivers/fpga/altera-pr-ip-core.c       | 14 +-------------
> >  include/linux/fpga/altera-pr-ip-core.h |  1 -
> >  3 files changed, 1 insertion(+), 24 deletions(-)
> >
> > diff --git a/drivers/fpga/altera-pr-ip-core-plat.c b/drivers/fpga/altera-pr-ip-core-plat.c
> > index 99b9cc0e70f0..b008a6b8d2d3 100644
> > --- a/drivers/fpga/altera-pr-ip-core-plat.c
> > +++ b/drivers/fpga/altera-pr-ip-core-plat.c
> > @@ -28,15 +28,6 @@ static int alt_pr_platform_probe(struct platform_device *pdev)
> >  	return alt_pr_register(dev, reg_base);
> >  }
> >  
> > -static int alt_pr_platform_remove(struct platform_device *pdev)
> > -{
> > -	struct device *dev = &pdev->dev;
> > -
> > -	alt_pr_unregister(dev);
> > -
> > -	return 0;
> > -}
> > -
> >  static const struct of_device_id alt_pr_of_match[] = {
> >  	{ .compatible = "altr,a10-pr-ip", },
> >  	{},
> > @@ -46,7 +37,6 @@ MODULE_DEVICE_TABLE(of, alt_pr_of_match);
> >  
> >  static struct platform_driver alt_pr_platform_driver = {
> >  	.probe = alt_pr_platform_probe,
> > -	.remove = alt_pr_platform_remove,
> >  	.driver = {
> >  		.name	= "alt_a10_pr_ip",
> >  		.of_match_table = alt_pr_of_match,
> > diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c
> > index 2cf25fd5e897..dfdf21ed34c4 100644
> > --- a/drivers/fpga/altera-pr-ip-core.c
> > +++ b/drivers/fpga/altera-pr-ip-core.c
> > @@ -195,22 +195,10 @@ int alt_pr_register(struct device *dev, void __iomem *reg_base)
> >  	if (!mgr)
> >  		return -ENOMEM;
> >  
> > -	dev_set_drvdata(dev, mgr);
> > -
> > -	return fpga_mgr_register(mgr);
> > +	return devm_fpga_mgr_register(dev, mgr);
> >  }
> >  EXPORT_SYMBOL_GPL(alt_pr_register);
> >  
> > -void alt_pr_unregister(struct device *dev)
> > -{
> > -	struct fpga_manager *mgr = dev_get_drvdata(dev);
> > -
> > -	dev_dbg(dev, "%s\n", __func__);
> > -
> > -	fpga_mgr_unregister(mgr);
> > -}
> > -EXPORT_SYMBOL_GPL(alt_pr_unregister);
> 
> Similar to the others, except for removing this symbol.
> 
> A patch should do one logical thing.

I was on the fence with this. Tbh, this driver should be a platform
driver. I'll create a separate series for that.

> 
> I'd rather this be split out of the patchset.
> 
> Tom
> 
> > -
> >  MODULE_AUTHOR("Matthew Gerlach <matthew.gerlach@linux.intel.com>");
> >  MODULE_DESCRIPTION("Altera Partial Reconfiguration IP Core");
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/fpga/altera-pr-ip-core.h b/include/linux/fpga/altera-pr-ip-core.h
> > index 0b08ac20ab16..a6b4c07858cc 100644
> > --- a/include/linux/fpga/altera-pr-ip-core.h
> > +++ b/include/linux/fpga/altera-pr-ip-core.h
> > @@ -13,6 +13,5 @@
> >  #include <linux/io.h>
> >  
> >  int alt_pr_register(struct device *dev, void __iomem *reg_base);
> > -void alt_pr_unregister(struct device *dev);
> >  
> >  #endif /* _ALT_PR_IP_CORE_H */
> 

Cheers,
Moritz

WARNING: multiple messages have this Message-ID
From: Moritz Fischer <mdf@kernel.org>
To: Tom Rix <trix@redhat.com>
Cc: russell.h.weight@intel.com, linux-fpga@vger.kernel.org,
	linux-kernel@vger.kernel.org, matthew.gerlach@intel.com,
	michal.simek@xilinx.com, Moritz Fischer <mdf@kernel.org>,
	linux-arm-kernel@lists.infradead.org, hao.wu@intel.com
Subject: Re: [PATCH 10/10] fpga: fpga-mgr: altera-pr-ip: Simplify registration
Date: Sun, 4 Oct 2020 16:39:14 -0700	[thread overview]
Message-ID: <20201004233914.GA111357@epycbox.lan> (raw)
In-Reply-To: <a49b1d7c-9756-1059-f7a1-25dae460d659@redhat.com>

On Sun, Oct 04, 2020 at 11:47:26AM -0700, Tom Rix wrote:
> 
> On 10/3/20 10:14 PM, Moritz Fischer wrote:
> > Simplify registration using new devm_fpga_mgr_register() API.
> > Remove the now obsolete altera_pr_unregister() function.
> >
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> >
> > We should take another look at this, IIRC correctly the point of
> > splitting this up into a separate driver was to make it useable by a
> > different (pci?) driver later on.
> >
> > It doesn't seem like this happened, and I think we should just make this
> > a platform driver?
> >
> > ---
> >  drivers/fpga/altera-pr-ip-core-plat.c  | 10 ----------
> >  drivers/fpga/altera-pr-ip-core.c       | 14 +-------------
> >  include/linux/fpga/altera-pr-ip-core.h |  1 -
> >  3 files changed, 1 insertion(+), 24 deletions(-)
> >
> > diff --git a/drivers/fpga/altera-pr-ip-core-plat.c b/drivers/fpga/altera-pr-ip-core-plat.c
> > index 99b9cc0e70f0..b008a6b8d2d3 100644
> > --- a/drivers/fpga/altera-pr-ip-core-plat.c
> > +++ b/drivers/fpga/altera-pr-ip-core-plat.c
> > @@ -28,15 +28,6 @@ static int alt_pr_platform_probe(struct platform_device *pdev)
> >  	return alt_pr_register(dev, reg_base);
> >  }
> >  
> > -static int alt_pr_platform_remove(struct platform_device *pdev)
> > -{
> > -	struct device *dev = &pdev->dev;
> > -
> > -	alt_pr_unregister(dev);
> > -
> > -	return 0;
> > -}
> > -
> >  static const struct of_device_id alt_pr_of_match[] = {
> >  	{ .compatible = "altr,a10-pr-ip", },
> >  	{},
> > @@ -46,7 +37,6 @@ MODULE_DEVICE_TABLE(of, alt_pr_of_match);
> >  
> >  static struct platform_driver alt_pr_platform_driver = {
> >  	.probe = alt_pr_platform_probe,
> > -	.remove = alt_pr_platform_remove,
> >  	.driver = {
> >  		.name	= "alt_a10_pr_ip",
> >  		.of_match_table = alt_pr_of_match,
> > diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c
> > index 2cf25fd5e897..dfdf21ed34c4 100644
> > --- a/drivers/fpga/altera-pr-ip-core.c
> > +++ b/drivers/fpga/altera-pr-ip-core.c
> > @@ -195,22 +195,10 @@ int alt_pr_register(struct device *dev, void __iomem *reg_base)
> >  	if (!mgr)
> >  		return -ENOMEM;
> >  
> > -	dev_set_drvdata(dev, mgr);
> > -
> > -	return fpga_mgr_register(mgr);
> > +	return devm_fpga_mgr_register(dev, mgr);
> >  }
> >  EXPORT_SYMBOL_GPL(alt_pr_register);
> >  
> > -void alt_pr_unregister(struct device *dev)
> > -{
> > -	struct fpga_manager *mgr = dev_get_drvdata(dev);
> > -
> > -	dev_dbg(dev, "%s\n", __func__);
> > -
> > -	fpga_mgr_unregister(mgr);
> > -}
> > -EXPORT_SYMBOL_GPL(alt_pr_unregister);
> 
> Similar to the others, except for removing this symbol.
> 
> A patch should do one logical thing.

I was on the fence with this. Tbh, this driver should be a platform
driver. I'll create a separate series for that.

> 
> I'd rather this be split out of the patchset.
> 
> Tom
> 
> > -
> >  MODULE_AUTHOR("Matthew Gerlach <matthew.gerlach@linux.intel.com>");
> >  MODULE_DESCRIPTION("Altera Partial Reconfiguration IP Core");
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/fpga/altera-pr-ip-core.h b/include/linux/fpga/altera-pr-ip-core.h
> > index 0b08ac20ab16..a6b4c07858cc 100644
> > --- a/include/linux/fpga/altera-pr-ip-core.h
> > +++ b/include/linux/fpga/altera-pr-ip-core.h
> > @@ -13,6 +13,5 @@
> >  #include <linux/io.h>
> >  
> >  int alt_pr_register(struct device *dev, void __iomem *reg_base);
> > -void alt_pr_unregister(struct device *dev);
> >  
> >  #endif /* _ALT_PR_IP_CORE_H */
> 

Cheers,
Moritz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2020-10-04 23:39 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-04  5:14 [PATCH 00/10] Introduce devm_fpga_mgr_register() Moritz Fischer
2020-10-04  5:14 ` Moritz Fischer
2020-10-04  5:14 ` [PATCH 01/10] fpga: fpga-mgr: Add devm_fpga_mgr_register() API Moritz Fischer
2020-10-04  5:14   ` Moritz Fischer
2020-10-04 18:08   ` Tom Rix
2020-10-04 18:08     ` Tom Rix
2020-10-05  5:18   ` Wu, Hao
2020-10-05  5:18     ` Wu, Hao
2020-10-05 16:45     ` Moritz Fischer
2020-10-05 16:45       ` Moritz Fischer
2020-10-04  5:14 ` [PATCH 02/10] fpga: fpga-mgr: altera-ps-spi: Simplify registration Moritz Fischer
2020-10-04  5:14   ` Moritz Fischer
2020-10-04 18:15   ` Tom Rix
2020-10-04 18:15     ` Tom Rix
2020-10-04  5:14 ` [PATCH 03/10] fpga: fpga-mgr: dfl-fme-mgr: " Moritz Fischer
2020-10-04  5:14   ` Moritz Fischer
2020-10-04 18:22   ` Tom Rix
2020-10-04 18:22     ` Tom Rix
2020-10-04 23:40     ` Moritz Fischer
2020-10-04 23:40       ` Moritz Fischer
2020-10-04  5:14 ` [PATCH 04/10] fpga: fpga-mgr: ice40-spi: " Moritz Fischer
2020-10-04  5:14   ` Moritz Fischer
2020-10-04 18:24   ` Tom Rix
2020-10-04 18:24     ` Tom Rix
2020-10-04  5:14 ` [PATCH 05/10] fpga: fpga-mgr: machxo2-spi: " Moritz Fischer
2020-10-04  5:14   ` Moritz Fischer
2020-10-04 18:25   ` Tom Rix
2020-10-04 18:25     ` Tom Rix
2020-10-04  5:14 ` [PATCH 06/10] fpga: fpga-mgr: socfpga: " Moritz Fischer
2020-10-04  5:14   ` Moritz Fischer
2020-10-04 18:27   ` Tom Rix
2020-10-04 18:27     ` Tom Rix
2020-10-04  5:14 ` [PATCH 07/10] fpga: fpga-mgr: ts73xx: " Moritz Fischer
2020-10-04  5:14   ` Moritz Fischer
2020-10-04 18:27   ` Tom Rix
2020-10-04 18:27     ` Tom Rix
2020-10-04  5:14 ` [PATCH 08/10] fpga: fpga-mgr: xilinx-spi: " Moritz Fischer
2020-10-04  5:14   ` Moritz Fischer
2020-10-04 18:28   ` Tom Rix
2020-10-04 18:28     ` Tom Rix
2020-10-04  5:14 ` [PATCH 09/10] fpga: fpga-mgr: zynqmp: " Moritz Fischer
2020-10-04  5:14   ` Moritz Fischer
2020-10-04 18:33   ` Tom Rix
2020-10-04 18:33     ` Tom Rix
2020-10-04  5:14 ` [PATCH 10/10] fpga: fpga-mgr: altera-pr-ip: " Moritz Fischer
2020-10-04  5:14   ` Moritz Fischer
2020-10-04 18:47   ` Tom Rix
2020-10-04 18:47     ` Tom Rix
2020-10-04 23:39     ` Moritz Fischer [this message]
2020-10-04 23:39       ` Moritz Fischer
2020-11-03  7:14 [PATCH 00/10] Introduce devm_fpga_mgr_register() API Moritz Fischer
2020-11-03  7:14 ` [PATCH 10/10] fpga: fpga-mgr: altera-pr-ip: Simplify registration Moritz Fischer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201004233914.GA111357@epycbox.lan \
    --to=mdf@kernel.org \
    --cc=hao.wu@intel.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-fpga@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matthew.gerlach@intel.com \
    --cc=michal.simek@xilinx.com \
    --cc=russell.h.weight@intel.com \
    --cc=trix@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.