Re: [PATCH] pinctrl: rockchip: Add rk3328 pinctrl support
From: Heiko Stuebner <hidden>
Date: 2017-01-27 16:30:40
Also in:
linux-rockchip, lkml
Hi David, Am Sonntag, 22. Januar 2017, 16:38:06 CET schrieb David Wu:
From: "david.wu" <redacted> This patch supports 3bit width iomux type. Note, the iomux of following pins are special, need to be handled specially. - gpio2_b0 ~ gpio2_b6 - gpio2_b7 - gpio2_c7 - gpio3_b0 - gpio3_b1 ~ gpio3_b7 And therefore add IOMUX_RECALCED_FLAG to indicate which iomux source of the bank need to be recalced. If the mux recalced callback and IOMUX_RECALCED_FLAG were set, recalc the related pins' iomux. Signed-off-by: david.wu <redacted>
Patch description should pay a lot of attention to explaining _why_ a change is necessary. In general, please split the patch in at least 3 individual patches: - addition of 3bit mux support (that part looks good) - that recalculation-part ... which I'm still struggling to understand but will hopefully have understood down below - addition of actual rk3328-support (rk3328-specific functions and
quoted hunk ↗ jump to hunk
diff --git a/drivers/pinctrl/pinctrl-rockchip.cb/drivers/pinctrl/pinctrl-rockchip.c index 08765f5..cc05753 100644--- a/drivers/pinctrl/pinctrl-rockchip.c +++ b/drivers/pinctrl/pinctrl-rockchip.c@@ -75,6 +75,8 @@ enum rockchip_pinctrl_type { #define IOMUX_WIDTH_4BIT BIT(1) #define IOMUX_SOURCE_PMU BIT(2) #define IOMUX_UNROUTED BIT(3) +#define IOMUX_WIDTH_3BIT BIT(4) +#define IOMUX_RECALCED_FLAG BIT(5)
leave out the "_FLAG" bit please, makes it shorter and its state as flag is clearly visible [...]
quoted hunk ↗ jump to hunk
@@ -355,6 +359,24 @@ struct rockchip_pinctrl { unsigned int nfunctions; }; +/** + * struct rockchip_mux_recalced_data: represent a pin iomux data. + * @num: bank num. + * @bit: index at register or used to calc index. + * @min_pin: the min pin. + * @max_pin: the max pin. + * @reg: the register offset. + * @mask: mask bit + */ +struct rockchip_mux_recalced_data { + u8 num; + u8 bit; + int min_pin; + int max_pin; + int reg; + int mask;
please reorganize num, min_pin, max_pin are the queried values while reg, bit, mask are the result values Mixing these makes it hard to understand. Same for the table below.
quoted hunk ↗ jump to hunk
+}; + static struct regmap_config rockchip_regmap_config = { .reg_bits = 32, .val_bits = 32,@@ -514,13 +536,83 @@ static void rockchip_dt_free_map(struct pinctrl_dev*pctldev, * Hardware access */ +static const struct rockchip_mux_recalced_data rk3328_mux_recalced_data[] = { + { + .num = 2, + .bit = 0x2, + .min_pin = 8, + .max_pin = 14, + .reg = 0x24, + .mask = 0x3 + }, + { + .num = 2, + .bit = 0, + .min_pin = 15, + .max_pin = 15, + .reg = 0x28, + .mask = 0x7 + }, + {
nit: coding style, "}, {"
+ .num = 2,
+ .bit = 14,
+ .min_pin = 23,
+ .max_pin = 23,
+ .reg = 0x30,
+ .mask = 0x3
+ },
+ {
+ .num = 3,
+ .bit = 0,
+ .min_pin = 8,
+ .max_pin = 8,
+ .reg = 0x40,
+ .mask = 0x7
+ },
+ {
+ .num = 3,
+ .bit = 0x2,
+ .min_pin = 9,
+ .max_pin = 15,
+ .reg = 0x44,
+ .mask = 0x3I think I don't fully understand what this is supposed to do. In the TRM you send me at 0x44 all bits are marked as reserved and the other registers also look very strange. I guess GPIO2CH_IOMUX shows the thing you're trying solve, with gpio2_c6 being at [5:3] but gpio2_c7 got moved to [15:14] out of its natural position. Chip designers have strange ideas it seems. Heiko