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

[Heiko Schocher <hs@nabladev.com>]

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@nabladev.com>
>>>> ---
>>>>
>>>> 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@nabladev.com
Geschäftsführer : Stefano Babic