Re: [PATCH 1/3] gpio: idio-16: Introduce the ACCES IDIO-16 GPIO library module

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>]

On Sun, Sep 11, 2022 at 04:34:38PM -0400, William Breathitt Gray wrote:
> Exposes consumer library functions to facilitate communication with
> devices within the ACCES IDIO-16 family such as the 104-IDIO-16 and
> the PCI-IDIO-16.
> 
> A CONFIG_GPIO_IDIO_16 Kconfig option is introduced by this patch.
> Modules wanting access to these idio-16 library functions should select
> this Kconfig option and import the IDIO_16 symbol namespace.

...

> +int idio_16_get(struct idio_16 __iomem *const reg, const unsigned long offset)
> +{
> +	const unsigned long mask = BIT(offset);
> +
> +	if (offset < 8)
> +		return !!(ioread8(&reg->out0_7) & mask);
> +
> +	if (offset < 16)
> +		return !!(ioread8(&reg->out8_15) & (mask >> 8));
> +
> +	if (offset < 24)
> +		return !!(ioread8(&reg->in0_7) & (mask >> 16));
> +
> +	return !!(ioread8(&reg->in8_15) & (mask >> 24));

For the sake of robustness, since it's a library, I would do

	if (offset < 32)
		...

	return -EINVAL;

> +}
> +EXPORT_SYMBOL_NS_GPL(idio_16_get, IDIO_16);

If there is not expected to be more than a single namespace, you may define it
just once for all via

#define DEFAULT_SYMBOL_NAMESPACE IDIO_16

And honestly, I would add also GPIO prefix to it, GPIO_IDIO_16.

...

> +	if (*mask & GENMASK(7, 0))
> +		bitmap_set_value8(bits, ioread8(&reg->out0_7), 0);
> +	if (*mask & GENMASK(15, 8))
> +		bitmap_set_value8(bits, ioread8(&reg->out8_15), 8);
> +	if (*mask & GENMASK(23, 16))
> +		bitmap_set_value8(bits, ioread8(&reg->in0_7), 16);
> +	if (*mask & GENMASK(31, 24))
> +		bitmap_set_value8(bits, ioread8(&reg->in8_15), 24);

So, the addresses of the ports are not expected to be continuous?

...

> +void idio_16_set(struct idio_16 __iomem *const reg,
> +		 struct idio_16_state *const state, const unsigned long offset,
> +		 const unsigned long value)
> +{
> +	unsigned long flags;
> +
> +	/* offsets greater than 15 are input-only signals */
> +	if (offset > 15)

For the sake of consistency:

	if (offset >= 16)

> +		return;
> +
> +	spin_lock_irqsave(&state->lock, flags);

> +	if (value)
> +		set_bit(offset, state->out_state);
> +	else
> +		clear_bit(offset, state->out_state);

assign_bit()

But I'm wondering why do you need the atomic bitops under the lock?

> +	if (offset < 8)
> +		iowrite8(bitmap_get_value8(state->out_state, 0), &reg->out0_7);
> +	else
> +		iowrite8(bitmap_get_value8(state->out_state, 8), &reg->out8_15);
> +
> +	spin_unlock_irqrestore(&state->lock, flags);
> +}

...

> +	for_each_set_clump8(offset, port_mask, mask, IDIO_16_NOUT) {
> +		value = bitmap_get_value8(bits, offset);
> +		out_state = bitmap_get_value8(state->out_state, offset);
> +
> +		out_state = (out_state & ~port_mask) | (value & port_mask);
> +		bitmap_set_value8(state->out_state, out_state, offset);

This looks like bitmap_replace(). It might require to redesign the flow a bit.

> +		if (offset < 8)
> +			iowrite8(out_state, &reg->out0_7);
> +		else
> +			iowrite8(out_state, &reg->out8_15);
> +	}

...

> +static inline int idio_16_get_direction(const unsigned long offset)
> +{
> +	return (offset < IDIO_16_NOUT) ? 0 : 1;

	return (offset >= IDIO_16_NOUT) ? 1 : 0;

?

> +}

-- 
With Best Regards,
Andy Shevchenko