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
Attachment:
signature.asc
Description: PGP signature
