Re: [PATCH v3 3/7] gpio: regmap: Add gpio_regmap_operation and write-enable support

[Andy Shevchenko]

On Fri, Jul 17, 2026 at 03:38:35PM +0000, Yu-Chun Lin [林祐君] wrote:
> > On Thu, Jul 16, 2026 at 12:55:37PM +0200, Michael Walle wrote:
> > > On Thu Jul 16, 2026 at 11:40 AM CEST, Andy Shevchenko wrote:
> > > > On Thu, Jul 16, 2026 at 11:08:55AM +0200, Michael Walle wrote:
> > > >> On Thu Jul 16, 2026 at 10:27 AM CEST, Andy Shevchenko wrote:
> > > >> > On Thu, Jul 16, 2026 at 02:26:14PM +0800, Yu-Chun Lin wrote:

...

> > > >> > From the above list I tend to the approach 2, but this might
> > > >> > require to have GPIO regmap level of locking. I'm a bit lost in
> > > >> > the context, though. I assume we need a fresh start, id est issue
> > > >> > a v6 with approach 2 or 3 in place and summarize the choices in
> > > >> > the cover letter, so we can understand what has been considered.
> > > >>
> > > >> I don't really like approach 3. You'd need to check if the regs of
> > > >> both xlate calls are the same. With the sample code above, you
> > > >> silently drop the first xlate'd reg.
> > > >
> > > > If I rank the proposals, the worst is #1, the best is #2.
> > > >
> > > >> And honestly, it really seems like a one-off. What controllers, are
> > > >> there that need a write enable bit. The real problem seems to be
> > > >> the assumption that we operate on just one bit. IOW we either set
> > > >> mask or don't set mask in gpio_regmap_set().
> > > >
> > > > Yes, we should KISS.
> > >
> > > But IMHO #2 and #3 are not KISS. Approach 2 is just a way of adding
> > > some kind of pre op to a gpio set. Just tying it to a write enable
> > > feature. That kinda bothers me. It might also be useful for other
> > > things, too. So don't tie it to just write enable. And who is doing a
> > > write disable if it's not self clearing for example. Probably Some
> > > kind of post op :)
> > >
> > > Approach 3 is a way to change the value of the written value - in a
> > > restricted way, as is is just doing a OR with both values.
> > >
> > > Also approach 2 might not even work if the hardware requires the write
> > > enable bit set in the *same* write as the gpio set bit. Thus, we might
> > > need both anyway in the future.
> > >
> > > >> For a more generic solution, we should be able to control the
> > > >> written value. We could add another .value_xlate().
> > > >
> > > > Maybe not now?
> > >
> > > But if not now, then when? I wouldn't add the write enable feature and
> > > later a more generic solution which also covers the write enable
> > > feature.
> > 
> > Taking into account how it's done in HW, I think the whole approach can be
> > folded to just a boolean flag  (or a simply bit shift) in the config.
> > Wouldn't it work?
> 
> To respect Michael's perspective on keeping WREN-specific code out of the
> framework, I think adding a value_xlate callback in the config is a cleaner
> approach. It allows us to wrap this hardware quirk within the callback and let
> the consumer driver handle it entirely.
> 
> To ensure it is truly generic, I will not introduce any WREN-specific operation
> flags. Instead, value_xlate will reuse the gpio_regmap_operation enum
> (e.g., GPIO_REGMAP_OP_SET, GPIO_REGMAP_OP_SET_DIR).

OK! But don't forget add a good documentation that users will understand
the logic behind it.

> > > > As per IPs, Synopsys IPs (not exactly GPIO) likes to have that kind
> > > > of "protection". So, from HW perspective it's kinda pattern, and it
> > > > might be possible to see more IPs (including GPIO) that follow it in
> > > > some cases.

-- 
With Best Regards,
Andy Shevchenko