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

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 <[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...

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.

Thanks,
Michal