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

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 <[email protected]>
>> ---
>>
>> 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...

Doesn't it make more sense to use here

gpiod_set_raw_value()

instead, which sets the physical levels?

Thanks!

bye,
Heiko
> 
> Thanks,
> Michal

-- 
Nabla Software Engineering
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: [email protected]
Geschäftsführer : Stefano Babic