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

[William Breathitt Gray <william.gray@xxxxxxxxxx>]

On Tue, Sep 13, 2022 at 07:16:23PM +0300, Andy Shevchenko wrote:
> On Sun, Sep 11, 2022 at 04:34:38PM -0400, William Breathitt Gray wrote:
> > +	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?

No, unfortunately the IDIO-16 devices allocate the FET outputs to byte
offsets 0 and 4 while the isolated inputs are allocated to byte offsets
1 and 5. I don't know the design reason for the split but that's the
reason I'm reading these addresses by byte rather than by word.

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

I don't think atomic bitops are necessary in this case because of the
lock as you pointedly out, but I felt using these made the intention of
the code clearer. Is there a non-atomic version of assign_bit(), or do
you recommend I use bitwise operations directly here instead?

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

I have no particular preference in this case, so I can switch this to
the >= version for consistency with the rest of the code.

Thanks,

William Breathitt Gray

Attachment: signature.asc
Description: PGP signature