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

["Yu-Chun Lin [林祐君]" <eleanor.lin@realtek.com>]

Hi,

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

Best Regards,
Yu-Chun

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