Re:Re: [PATCH v6 08/13] gpio: regmap: support write_data_after_dir and girq

[zl020895]

Hi Andy,

Thanks for the review.

> Are you going to fix this HW in the next version of the SoC?
No — Ambarella does not use write_data_after_dir. It only preserves the
existing ARM PL061 quirk already documented in gpio-pl061 (data writes
ignored while the pin is still an input). Only pl061_arm sets the flag.

I will also make the first gpio_regmap_set() conditional so the quirk
path writes once after direction_output, not twice.

> This needs to be in a separate update. Also we need to understand why
> it is required.
OK, girq will be a separate patch. PL061 keeps a custom chained
irqchip (not regmap-irq); gpio_regmap today only takes irq_domain or
regmap_irq_chip, so we passed girq to keep the usual gpio_irq_chip +
gpiochip_add flow. Open to using a caller-created irq_domain instead if
you prefer.

> Is pahole happy with the layout?
Will check and reorder if needed.

Best regards,
Long Zhao

At 2026-09-04 19:55:45, "Andy Shevchenko" <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>On Fri, Sep 04, 2026 at 02:38:15PM +0800, Long Zhao via B4 Relay wrote:
>
>Thanks for this change, my comments below.
>
>> Add an optional write-after-direction-output quirk for controllers
>> that ignore data-register writes while a line is still configured as
>> input,
>
>The chips that require this are buggy. Are you going to fix this HW in the next
>version of the SoC?
>
>> and allow drivers to pass an existing gpio_irq_chip through
>> gpio_regmap_register() so IRQ setup can stay with the caller.
>
>This needs to be in a separate update. Also we need to understand why
>it is required. The caller should be able to create the respective IRQ
>domain which is important.
>
>...
>
>> struct gpio_regmap {
>>  	unsigned int reg_dir_out_base;
>>  	unsigned long *fixed_direction_mask;
>>  	unsigned long *fixed_direction_output;
>> +	bool write_data_after_dir;
>
>Is `pahole` happy with the layout?
>
>>  #ifdef CONFIG_REGMAP_IRQ
>>  	int regmap_irq_line;
>
>...
>
>> static int gpio_regmap_direction_output(struct gpio_chip *chip,
>
>>  	gpio_regmap_set(chip, offset, value);
>
>^^^ (see below)
>
>>  
>> -	return gpio_regmap_set_direction(chip, offset, true);
>> +	ret = gpio_regmap_set_direction(chip, offset, true);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Some controllers ignore data writes while the line is still an input. */
>> +	if (gpio->write_data_after_dir)
>> +		gpio_regmap_set(chip, offset, value);
>
>This can lead to two writes to the same register, the above needs to be
>conditional as well.
>
>-- 
>With Best Regards,
>Andy Shevchenko
>