Hi David,
Am Sonntag, 22. Januar 2017, 16:38:06 CET schrieb David Wu:
> From: "david.wu" <david.wu at rock-chips.com>
>
> 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 <david.wu at rock-chips.com>
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
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/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
[...]
> @@ -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.
> +};
> +
> 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 = 0x3
I 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
