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

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 <[email protected]>
>> ---
>>
>>   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: [email protected]
Geschäftsführer : Stefano Babic