On 16/10/12 04:20, Roland Stigge wrote:

Hi Ryan,

thank you for your feedback, I will include it, except for some points
noted below:

quoted

quoted

+		gbc->mask |= BIT(bit);
+
+		/* collect gpios that are specified together, represented by
+		 * neighboring bits
+		 */
+		remap = &gbc->remap[gbc->nremap - 1];

This looks broken. If gbc was re-alloced above (index < 0) then
gbc->remap == NULL and this will oops?

No, because I took care that even though index can be < 0, the resulting
pointer is never dereferenced for -1.

Ah, I see. I think its a bit non-obvious and flaky though, since it
looks like you are both dereferencing a potentially NULL pointer, and
indexing an array with -1.

Even changing it to this I think makes it a bit more clear:

	if (gbc->remap == 0 ||
            bit - i != gbc->remap[gbc->nremap - 1].offset)
		gbc->nremap++;
		gbc->remap = krealloc(...);
		...

If you want to keep your way, at the very least I think it deserves a
comment, since it is easy to misread.

quoted

The remap functionality isn't very well explained

Documenting now in gpio.h like this:

/*
 * struct gpio_remap - a structure for describing a bit mapping
 * @mask:       a bit mask
 * @offset:     how many bits to shift to the left (negative: to the
 *              right)
 *
 * When we are mapping bit values from one word to another (here: from
 * GPIO block domain to GPIO driver domain), we first mask them out
 * with mask and shift them as specified with offset. More complicated
 * mappings are done by grouping several of those structs and adding
 * the results together.
 */
struct gpio_remap {
        int     mask;
        int     offset;
};

Looks good. Thanks.

If you find an issue, please let me know. Works fine for me. Have you
tried? :-)

No, I was just looking at the code, and misread it.

quoted

quoted

+unsigned gpio_block_get(const struct gpio_block *block)
+{
+	struct gpio_block_chip *gbc;
+	int i, j;
+	unsigned values = 0;
+
+	for (i = 0; i < block->nchip; i++) {
+		unsigned remapped = 0;
+
+		gbc = &block->gbc[i];
+
+		if (gbc->gc->get_block) {
+			remapped = gbc->gc->get_block(gbc->gc, gbc->mask);
+		} else { /* emulate */
+			unsigned bit = 1;
+
+			for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
+				if (gbc->mask & bit)

A proper bitmask might be more ideal for this. It would remove the
sizeof(unsigned) restriction and allow you to use for_each_set_bit for
these loops.

In a previous version of these patches, I actually had a generic bit
mask which was in turn awkward to handle, especially for the bit
remapping. Stijn brought me to the idea that for pragmatic reasons, 32
bit access is fully sufficient in most cases.

I also needed userland access (via sysfs), so there was no way of
accessing a block except via an int.

When there are GPIO drivers where we seriously need (and can handle
simultaneously) more than 32 bits, we can still extend the API. For now,
the cases where it is used is typically creating 8/16/32 bit busses with
GPIO lines, and on 64bit architectures even 64bit busses.

For this, the current API is working fine, even enabling userland access
via sysfs.

Fair enough. I didn't see the first round of patches. You probably can
still use for_each_set_bit though (maybe convert the mask to unsigned
long first to match the bitops API):

	for_each_set_bit(j, &gbc->mask, BITS_PER_LONG)
		...

quoted

quoted

+			unsigned bit = 1;
+
+			for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
+				if (gbc->mask & bit)
+					gbc->gc->set(gbc->gc, gbc->gc->base + j,
+						     (remapped >> j) & 1);
+			}

This doesn't clear pins which are set to zero?

It does. gbc->mask only masks which bits to set and clear. remapped
contains the actual bit values to set. 0 or 1.

Ugh, for some reason I was thinking that the gpio set function only
drove bits that were set in the mask (and had an analogous clear
function). Ignore me :-).

~Ryan