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

[Heiko Schocher]

Hello Michal,

On 20.07.26 14:40, Michal Simek wrote:
> On 7/16/26 09:00, 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>
> > ---
> >
> >   drivers/fpga/xilinx-selectmap.c | 20 ++++++++++++++++++--
> >   1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/fpga/xilinx-selectmap.c b/drivers/fpga/xilinx-selectmap.c
> > index d0cbb5fdfe3a..b5fbdedf8aec 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,9 +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;
> > +    if (conf->csi_b)
>
> Is this really needed?
>
> 4023 int gpiod_set_value(struct gpio_desc *desc, int value)
> 4024 {
> 4025         VALIDATE_DESC(desc);
> 4026         /* Should be using gpiod_set_value_cansleep() */
> 4027         WARN_ON(desc->gdev->can_sleep);
> 4028         return gpiod_set_value_nocheck(desc, value);
> 4029 }
> 4030 EXPORT_SYMBOL_GPL(gpiod_set_value);
>
>   398 #define VALIDATE_DESC(desc) do { \
>   399         int __valid = validate_desc(desc, __func__); \
>   400         if (__valid <= 0) \
>   401                 return __valid; \
>   402         } while (0)
>
>   385 static int validate_desc(const struct gpio_desc *desc, const char *func)
>   386 {
>   387         if (!desc)
>   388                 return 0;
>   389
>   390         if (IS_ERR(desc)) {
>   391                 pr_warn("%s: invalid GPIO (errorpointer: %pe)\n", func, desc);
>   392                 return PTR_ERR(desc);
>   393         }
>   394
>   395         return 1;
>   396 }
>   397
>
> You should be able to just skip it because validate_desc is checking it for you.

It seems this is possible yes, so I drop it!

> > +        gpiod_set_value(conf->csi_b, GPIOD_OUT_HIGH);
> > +
> > +    if (conf->rdwr_b)
> > +        gpiod_set_value(conf->rdwr_b, GPIOD_OUT_HIGH);
> > +
> >       for (i = 0; i < count; ++i)
> >           writeb(buf[i], conf->base);
> > +    if (conf->rdwr_b)
> > +        gpiod_set_value(conf->rdwr_b, GPIOD_OUT_LOW);
> > +
> > +    if (conf->csi_b)
> > +        gpiod_set_value(conf->csi_b, GPIOD_OUT_LOW);
> > +
> >       return 0;
> >   }
> > @@ -56,16 +70,18 @@ static int xilinx_selectmap_probe(struct platform_device *pdev)
> >       conf->base = base;
> >       /* CSI_B is active low */
> > -    gpio = devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_HIGH);
> > +    gpio = devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_LOW);
> >       if (IS_ERR(gpio))
> >           return dev_err_probe(&pdev->dev, PTR_ERR(gpio),
> >                        "Failed to get CSI_B gpio\n");
> > +    conf->csi_b = gpio;
>
> Isn't it easier to simply call to avoid it?
>
> conf->csi_b = devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_LOW);

Of course, will send a v2 and drop the local gpio variable as not
longer needed.

Thanks for the review!

bye,
Heiko
> Thanks,
> Michal

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