Date:   Sun, 2 Apr 2023 10:39:02 -0400
From:   William Breathitt Gray <william.gray@...aro.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
        Mark Brown <broonie@...nel.org>, techsupport@...systems.com,
        Paul Demetrotion <pdemetrotion@...systems.com>,
        Michael Walle <michael@...le.cc>
Subject: Re: [PATCH v5 3/3] gpio: ws16c48: Migrate to the regmap API

On Mon, Mar 27, 2023 at 02:26:37PM +0300, Andy Shevchenko wrote:
> On Sun, Mar 26, 2023 at 12:25:59PM -0400, William Breathitt Gray wrote:
> > The regmap API supports IO port accessors so we can take advantage of
> > regmap abstractions rather than handling access to the device registers
> > directly in the driver.
> > 
> > The WinSystems WS16C48 provides the following registers:
> > 
> >     Offset 0x0-0x5: Port 0-5 I/O
> >     Offset 0x6: Int_Pending
> >     Offset 0x7: Page/Lock
> >     Offset 0x8-0xA (Page 1): Pol_0-Pol_2
> >     Offset 0x8-0xA (Page 2): Enab_0-Enab_2
> >     Offset 0x8-0xA (Page 3): Int_ID0-Int_ID2
> > 
> > Port 0-5 I/O provides access to 48 lines of digital I/O across six
> > registers, each bit position corresponding to the respective line.
> > Writing a 1 to a respective bit position causes that output pin to sink
> > current, while writing a 0 to the same bit position causes that output
> > pin to go to a high-impedance state and allows it to be used an input.
> > Reads on a port report the inverted state (0 = high, 1 = low) of an I/O
> > pin when used in input mode. Interrupts are supported on Port 0-2.
> > 
> > Int_Pending is a read-only register that reports the combined state of
> > the INT_ID0 through INT_ID2 registers; an interrupt pending is indicated
> > when any of the low three bits are set.
> > 
> > The Page/Lock register provides the following bits:
> > 
> >     Bit 0-5: Port 0-5 I/O Lock
> >     Bit 6-7: Page 0-3 Selection
> > 
> > For Bits 0-5, writing a 1 to a respective bit position locks the output
> > state of the corresponding I/O port. Writing the page number to Bits 6-7
> > selects that respective register page for use.
> > 
> > Pol_0-Pol_2 are accessible when Page 1 is selected. Writing a 1 to a
> > respective bit position selects the rising edge detection interrupts for
> > that input line, while writing a 0 to the same bit position selects the
> > falling edge detection interrupts.
> > 
> > Enab_0-Enab_2 are accessible when Page 2 is selected. Writing a 1 to a
> > respective bit position enables interrupts for that input line, while
> > writing a 0 to that same bit position clears and disables interrupts for
> > that input line.
> > 
> > Int_ID0-Int_ID2 are accessible when Page 3 is selected. A respective bit
> > when read as a 1 indicates that an edge of the polarity set in the
> > corresponding polarity register was detected for the corresponding input
> > line. Writing any value to this register clears all pending interrupts
> > for the register.
> 
> ...
> 
> > +static const struct regmap_config ws16c48_regmap_config = {
> > +	.reg_bits = 8,
> > +	.reg_stride = 1,
> > +	.val_bits = 8,
> > +	.io_port = true,
> > +	.max_register = 0xA,
> > +	.wr_table = &ws16c48_wr_table,
> > +	.rd_table = &ws16c48_rd_table,
> > +	.volatile_table = &ws16c48_volatile_table,
> > +	.cache_type = REGCACHE_FLAT,
> > +};
> 
> Do we need regmap lock?

We make regmap calls within an interrupt context in this driver so I
think we need to disable the default regmap mutex locking behavior; I
believe the current raw_spin_lock locking in this driver is enough to
protect access.

> >  /**
> >   * struct ws16c48_gpio - GPIO device private data structure
> > - * @chip:	instance of the gpio_chip
> > - * @io_state:	bit I/O state (whether bit is set to input or output)
> > - * @out_state:	output bits state
> > + * @map:	regmap for the device
> >   * @lock:	synchronization lock to prevent I/O race conditions
> >   * @irq_mask:	I/O bits affected by interrupts
> > - * @flow_mask:	IRQ flow type mask for the respective I/O bits
> > - * @reg:	I/O address offset for the device registers
> >   */
> >  struct ws16c48_gpio {
> > -	struct gpio_chip chip;
> > -	unsigned char io_state[6];
> > -	unsigned char out_state[6];
> > +	struct regmap *map;
> >  	raw_spinlock_t lock;
> > -	unsigned long irq_mask;
> > -	unsigned long flow_mask;
> > -	struct ws16c48_reg __iomem *reg;
> > +	u8 irq_mask[WS16C48_NUM_IRQS / WS16C48_NGPIO_PER_REG];
> 
> Looking at this (and also thinking about the previous patch) perhaps this
> should be declared as
> 
> 	DECLARE_BITMAP(...);
> 
> and corresponding bit ops to be used?

The irq_mask array is just used to store the previous mask_buf state for
comparision against the next time in the ws16c48_handle_mask_sync();
we're just checking if mask_buf has changed so we don't needless write
out to hardware.

I think declaring as BITMAP would obscure the intention of this array,
as well as increase the amount of code in ws16c48_handle_mask_sync()
because mask_buf is not declared as a bitmap and would need to be
converted for comparision against a bitmap irq_mask. So I believe we
should keep irq_mask as an array of u8 for now.

What might be worth discussing is whether the regmap_irq should
internally utilize BITMAP rather than passing around array elements and
indices. The regmap_irq buffer arrays (such as mask_buf) appear to
operate essentially as makeshift bitmaps, so I suspect they could
benefit from the Linux bitmap API.

> > +static int ws16c48_handle_pre_irq(void *const irq_drv_data)
> >  {
> > +	struct ws16c48_gpio *const ws16c48gpio = irq_drv_data;
> >  
> > +	/* Lock to prevent Page/Lock register change while we handle IRQ */
> > +	raw_spin_lock(&ws16c48gpio->lock);
> >  
> >  	return 0;
> >  }
> 
> Hmm... Don't we have irq bus lock and unlock callbacks for this?

The WS16C48 shares the same address space for its interrupt polarity,
IRQ masking, and IRQ status/ACK registers; a page register is utilize in
order to multiplex between the three register pages.

Because the same address space is shared between these different
functions, we use the ws16c48gpio->lock to coordinate access between
them to prevent the registers from being clobbered. For example,
interrupt polarity is set in irq_set_type(), while IRQ masking and
ACKing occurs in irq_bus_sync_unlock(), and IRQ status reading happens
in regmap_irq_thread().

The lock acquired in ws16c48_handle_pre_irq() is for just this last
function of checking the IRQ status. We can't hold it through
irq_bus_lock() because we have to handle IRQ masking and ACKing in
irq_bus_sync_unlock().

> > +static int ws16c48_handle_post_irq(void *const irq_drv_data)
> >  {
> > +	struct ws16c48_gpio *const ws16c48gpio = irq_drv_data;
> >  
> > +	raw_spin_unlock(&ws16c48gpio->lock);
> >  
> >  	return 0;
> >  }
> 
> Ditto.
> 
> Also shouldn't you annotate them for sparse so it won't complain about
> unbalanced locks?

Yes, I'll annotate them with __acquires and __releases respectively.

William Breathitt Gray

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists