Re: [PATCH v5 3/3] gpio: ws16c48: Migrate to the regmap API

Andy Shevchenko
2023-03-27T11:26:45Z

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?

...

> /**
> * 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?

> };

...

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

...

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

...

> +exit_early:

exit_unlock() ?

> raw_spin_unlock_irqrestore(&ws16c48gpio->lock, flags);

> + return ret;

...

> + err = regmap_write(map, WS16C48_ENAB, 0x00);

WS16C48_ENAB + 0

(for the sake of symmetry with the below)?

> + if (err)
> + return err;
> + err = regmap_write(map, WS16C48_ENAB + 1, 0x00);
> + if (err)
> + return err;
> + err = regmap_write(map, WS16C48_ENAB + 2, 0x00);
> + if (err)
> + return err;

--
With Best Regards,
Andy Shevchenko