Re: [PATCH v2] driver: fpga: xilinx-selectmap: add csi and rdwr support

[Heiko Schocher]

Hello Michal,

On 23.07.26 07:44, Michal Simek wrote:
> On 7/22/26 16:45, Heiko Schocher wrote:
> > Hello Michal,
> >
> > On 22.07.26 08:55, Michal Simek wrote:
> > > On 7/21/26 13:42, Heiko Schocher wrote:
> > > > Add csi_b and rdwr pin support for the xilinx,selectmap driver.
> > > >
> > > > In current driver the pins are configured from DTS and set to
> > > > GPIOD_OUT_HIGH. This works in case you have one FPGA.
> > > >
> > > > Extend this to really implement csi_b and rdwr pin function in
> > > > driver, so it works with more than one FPGA.
> > > >
> > > > Tested on AM625 based board with 2 FPGAs connected to GPMC.
> > > >
> > > > Signed-off-by: Heiko Schocher <hs@xxxxxxxxxxxx>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - add comments from Michal
> > > >    - skip check if gpio descriptor variables csi_b/rdwr_b are valid,
> > > >      as validate_desc() checks this in gpiod_set_value() call.
> > > >    - initialize the gpio variables csi_b/rdwr_b immediately with
> > > >      the return value from devm_gpiod_get_optional(), so we can
> > > >      drop local gpio variable at all
> > > >
> > > >   drivers/fpga/xilinx-selectmap.c | 23 ++++++++++++++++-------
> > > >   1 file changed, 16 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/fpga/xilinx-selectmap.c b/drivers/fpga/xilinx-selectmap.c
> > > > index d0cbb5fdfe3a..d5175f9430e4 100644
> > > > --- a/drivers/fpga/xilinx-selectmap.c
> > > > +++ b/drivers/fpga/xilinx-selectmap.c
> > > > @@ -19,6 +19,8 @@
> > > >   struct xilinx_selectmap_conf {
> > > >       struct xilinx_fpga_core core;
> > > >       void __iomem *base;
> > > > +    struct gpio_desc *csi_b;
> > > > +    struct gpio_desc *rdwr_b;
> > > >   };
> > > >   #define to_xilinx_selectmap_conf(obj) \
> > > > @@ -30,16 +32,21 @@ static int xilinx_selectmap_write(struct xilinx_fpga_core *core,
> > > >       struct xilinx_selectmap_conf *conf = to_xilinx_selectmap_conf(core);
> > > >       size_t i;
> > > > +    gpiod_set_value(conf->csi_b, GPIOD_OUT_HIGH);
> > > > +    gpiod_set_value(conf->rdwr_b, GPIOD_OUT_HIGH);
> > >
> > > I think GPIOD_OUT_HIGH/LOW is wrong in this context.
> > > If you do "git grep gpiod_set_value" you will see that values are 0/1
> > I will change it!
> >
> > Hmm.. while thinking about it... CSI_B and RDWR_B signals
> > are defined as active low...
>
> I have never worked with this driver but values here should be 0 or 1. And if that means active 
> low/high is said via DT.
>
> GPIOD_OUT_LOW below should mean that you are starting in deassert state (if it is low/high is coming 
> from DT). And before write you assert that line and deassert after it.

Fine for me, I rework it, thanks!

bye,
Heiko
> Thanks,
> Michal

--
Nabla Software Engineering
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: office@xxxxxxxxxxxx
Geschäftsführer : Stefano Babic