Hi David,

Am Donnerstag, 7. Januar 2016, 15:41:38 schrieb David Wu:

[...]

According to the doc excerpts, wouldn't the big special handling only be 
necessary for for gpio3b5 alone (+ other banks) ... i.e. the one setting being 
split over both registers?
If so I think doing something like the following might be easier to 
understand? Same of course for the set-counterpart. [Beware if I missed up the 
indices somewhere :-) ]

rmask_bits = RK3399_DRV_3BITS_PER_PIN;
switch(bit) {
case 0 ... 12:
	/* regular case, nothing to do */
	break;
case 15:
	/* drive-strength offset is special, as it is spread over 2 registers */
	ret = regmap_read(regmap, reg, &data);
	if (ret)
		return ret;

	ret = regmap_read(regmap, reg + 0x4, &temp);
	if (ret)
		return ret;

	/*
	 * the bit data[15] contains bit 0 of the value
	 * while temp[1:0] containts bits 2 and 1
	 */
	data >>= 15;
	temp &= 0x3;
	temp <<= 1;
	data |= temp;

	return rockchip_perpin_drv_list[drv_type][data];
case 18 ... 21:
	/* setting fully enclosed in the second register */
	rmask_bits = RK3399_DRV_3BITS_PER_PIN;
	reg += 4;
	bit -= 18;
}

+			/* need to read regs twice */
+			ret = regmap_read(regmap, reg, &temp);
+			if (ret)
+				return ret;
+
+			temp >>= bit;
+			rmask_bits = 16 - bit;
+			temp &= (1 << rmask_bits) - 1;
+
+			reg = reg + 0x4;
+			ret = regmap_read(regmap, reg, &data);
+			if (ret)
+				return ret;
+
+			rmask_bits = RK3399_DRV_3BITS_PER_PIN
+				+ bit - 16;
+			data &= (1 << rmask_bits) - 1;
+			data <<= 16 - bit;
+			data |= temp;
+
+			return rockchip_perpin_drv_list[drv_type][data];
+		}
+
+		rmask_bits = RK3399_DRV_3BITS_PER_PIN;
+		break;
+	case DRV_TYPE_IO_DEFAULT:
+	case DRV_TYPE_IO_1V8_OR_3V0:
+	case DRV_TYPE_IO_1V8_ONLY:
+		rmask_bits = RK3288_DRV_BITS_PER_PIN;
+		break;
+	default:
+		dev_err(info->dev, "unsupported pinctrl drive type: %d\n",
+			drv_type);
+		return -EINVAL;
+	}
+
 	ret = regmap_read(regmap, reg, &data);
 	if (ret)
 		return ret;

 	data >>= bit;
-	data &= (1 << RK3288_DRV_BITS_PER_PIN) - 1;
+	data &= (1 << rmask_bits) - 1;

-	return rockchip_perpin_drv_list[data];
+	return rockchip_perpin_drv_list[drv_type][data];
 }

 static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,

[...]

Apart from a change similar to the above, should the setting maybe also 
directly return and not depend on the final write?

+		} else {
+			rmask_bits = RK3399_DRV_3BITS_PER_PIN;
+		}
+		break;
+	case DRV_TYPE_IO_DEFAULT:
+	case DRV_TYPE_IO_1V8_OR_3V0:
+	case DRV_TYPE_IO_1V8_ONLY:
+		rmask_bits = RK3288_DRV_BITS_PER_PIN;
+		break;
+	default:
+		spin_unlock_irqrestore(&bank->slock, flags);
+		dev_err(info->dev, "unsupported pinctrl drive type: %d\n",
+			drv_type);
+		return -EINVAL;
+
+	}
+
 	/* enable the write to the equivalent lower bits */
-	data = ((1 << RK3288_DRV_BITS_PER_PIN) - 1) << (bit + 16);
+	data = ((1 << rmask_bits) - 1) << (bit + 16);
 	rmask = data | (data >> 16);
 	data |= (ret << bit);

[...]

DRV_TYPE_IO_DEFAULT instead of 0 please.