Re: [PATCH v2 2/8] pinctrl: stm32: Introduce HDP driver

[Clement LE GOFFIC <clement.legoffic@foss.st.com>]

On 5/21/25 00:34, Linus Walleij wrote:
> Hi Clément,
> 
> thanks for your patch!
> 
> On Tue, May 20, 2025 at 5:04 PM Clément Le Goffic
> <clement.legoffic@foss.st.com> wrote:
> 
>> This patch introduce the driver for the Hardware Debug Port available on
>> STM32MP platforms. The HDP allows the observation of internal SoC
>> signals by using multiplexers. Each HDP port can provide up to 16
>> internal signals (one of them can be software controlled as a GPO).
>>
>> Signed-off-by: Clément Le Goffic <clement.legoffic@foss.st.com>
> 
> (...)
>> +static int stm32_hdp_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +       return GPIO_LINE_DIRECTION_OUT;
>> +}
> 
> That's reasonable.
> 
>> +static int stm32_hdp_gpio_get(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +       struct stm32_hdp *hdp = gpiochip_get_data(gc);
>> +
>> +       if (((hdp->mux_conf & HDP_MUX_MASK(offset))) == HDP_MUX_GPOVAL(offset))
>> +               return !!(readl_relaxed(hdp->base + HDP_GPOVAL) & BIT(offset));
>> +       else
>> +               return !!(readl_relaxed(hdp->base + HDP_VAL) & BIT(offset));
>> +}
> 
> ...but you still make it possible to read the value of the line
> if it's not muxed as GPO?
> 
> Should it not stm32_hdp_gpio_get_direction() return
> GPIO_LINE_DIRECTION_IN if HDP_MUX_MASK(offset))) != HDP_MUX_GPOVAL(offset)?

Hi, oops, you're right !

> 
>> +static void stm32_hdp_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
>> +{
>> +       struct stm32_hdp *hdp = gpiochip_get_data(gc);
>> +
>> +       if (value)
>> +               writel_relaxed(BIT(offset), hdp->base + HDP_GPOSET);
>> +       else
>> +               writel_relaxed(BIT(offset), hdp->base + HDP_GPOCLR);
>> +}
> 
> Can't you just use GPIO_GENERIC for this?
> 
> bgpio_init(gc, dev, ARRAY_SIZE(stm32_hdp_pins), // == 8
>      hdp->base + HDP_VAL,
>      hdp->base + HDP_GPOSET,
>      hdp->base + HDP_GPOCLR,
>      NULL,
>      NULL,
>      0);
> 
> The default behaviour of GPIO MMIO is to read the output register
> for the value if the line is in output mode.
> 
> You may wanna override the .get_direction() callback after bgpio_init()
> and before registering the chip, either with what you have or what
> I described above.

I didn't know about it, I'll take a look and provide a V3.

Thank you,

Clément

> Yours,
> Linus Walleij