Re: [PATCH] fpga fr br: separate freeze bridge driver code

[matthew . gerlach]

On Mon, 27 Feb 2017, Moritz Fischer wrote:

> Hi Matthew,
>
> small nit inline.
>
> On Mon, Feb 27, 2017 at 12:03 PM,  <matthew.gerlach@xxxxxxxxxxxxxxx> wrote:
> > From: Matthew Gerlach <matthew.gerlach@xxxxxxxxx>
> >
> > This patch separates the core Freeze Bridge
> > driver code from the platform driver code.
> > The intent is to allow the core driver code
> > to be used without requiring platform driver support.
> >
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/fpga/Kconfig                     |  7 ++++
> >  drivers/fpga/Makefile                    |  1 +
> >  drivers/fpga/altera-freeze-bridge-plat.c | 65 ++++++++++++++++++++++++++++++++
> >  drivers/fpga/altera-freeze-bridge.c      | 41 ++++----------------
> >  drivers/fpga/altera-freeze-bridge.h      | 26 +++++++++++++
> >  5 files changed, 107 insertions(+), 33 deletions(-)
> >  create mode 100644 drivers/fpga/altera-freeze-bridge-plat.c
> >  create mode 100644 drivers/fpga/altera-freeze-bridge.h
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index ce861a2..1a1fc47 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -63,6 +63,13 @@ config ALTERA_FREEZE_BRIDGE
> >           isolate one region of the FPGA from the busses while that
> >           region is being reprogrammed.
> >
> > +config ALTERA_FREEZE_BRIDGE_PLAT
> > +       tristate "Platform support of Altera FPGA Freeze Bridge"
> > +       depends on ALTERA_FREEZE_BRIDGE && OF && HAS_IOMEM
> > +       help
> > +         Say Y to enable platform driver support for Altera FPGA
> > +         Freeze bridges.
> > +
> >  endif # FPGA
> >
> >  endmenu
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 8df07bc..a270c00 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)      += zynq-fpga.o
> >  obj-$(CONFIG_FPGA_BRIDGE)              += fpga-bridge.o
> >  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)      += altera-hps2fpga.o altera-fpga2sdram.o
> >  obj-$(CONFIG_ALTERA_FREEZE_BRIDGE)     += altera-freeze-bridge.o
> > +obj-$(CONFIG_ALTERA_FREEZE_BRIDGE_PLAT)        += altera-freeze-bridge-plat.o
> >
> >  # High Level Interfaces
> >  obj-$(CONFIG_FPGA_REGION)              += fpga-region.o
> > diff --git a/drivers/fpga/altera-freeze-bridge-plat.c b/drivers/fpga/altera-freeze-bridge-plat.c
> > new file mode 100644
> > index 0000000..44ce539
> > --- /dev/null
> > +++ b/drivers/fpga/altera-freeze-bridge-plat.c
> > @@ -0,0 +1,65 @@
> > +/*
> > + * Platform Driver Support for FPGA Freeze Bridge Controller
> > + *
> > + *  Copyright (C) 2016 Altera Corporation. All rights reserved.
> > + *  Copyright (C) 2016-2017 Intel Corporation. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along with
> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +#include "altera-freeze-bridge.h"
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/of_device.h>
> > +#include <linux/module.h>
> > +
> > +static int altera_freeze_br_platform_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *np = pdev->dev.of_node;
> > +       struct resource *res;
> > +       void __iomem *reg_base;
> > +
> > +       if (!np)
> > +               return -ENODEV;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > +       reg_base = devm_ioremap_resource(dev, res);
> > +
> > +       if (IS_ERR(reg_base))
> > +               return PTR_ERR(reg_base);
> > +
> > +       return altera_freeze_br_probe(dev, reg_base);
>
> Maybe you can call this 'register' instead of 'probe' since the
> probing is basically
> done at this point. The same comment basically applies to the pr-core I 
> think.
Hi Moritz,

This is very good suggestion. Having a "probe" function and a 
"platform_probe" function was confusing to some people.  'register' makes 
a lot of sense.

Matthew Gerlach


> Thanks,
>
> Moritz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html