Re: [PATCH v2 06/10] pinctrl: rockchip: add support for rk1108

[Heiko Stuebner]

Am Montag, 14. November 2016, 20:10:47 CET schrieb Andy Yan:
> This add pinctrl support for Rockchip RK1108 Soc.
> 
> Signed-off-by: Andy Yan <andy.yan@xxxxxxxxxxxxxx>

still looks mostly good. I think I've now compared every register offset with 
the TRM - they all look good. I've noticed two styling issues below, with 
those fixed:

Reviewed-by: Heiko Stuebner <heiko@xxxxxxxxx>

> ---
> Changes in v2:
> - add pull and drive-strength functionality
> 
>  drivers/pinctrl/pinctrl-rockchip.c | 87
> +++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1
> deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 49bf7dc..fcc89fb 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -59,6 +59,7 @@
>  #define GPIO_LS_SYNC		0x60
> 
>  enum rockchip_pinctrl_type {
> +	RK1108,
>  	RK2928,
>  	RK3066B,
>  	RK3188,
> @@ -624,6 +625,65 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux) return ret;
>  }
> 
> +#define RK1108_PULL_PMU_OFFSET		0x10
> +#define RK1108_PULL_OFFSET		0x110
> +#define RK1108_PULL_PINS_PER_REG	8
> +#define RK1108_PULL_BITS_PER_PIN	2
> +#define RK1108_PULL_BANK_STRIDE		16
> +
> +static void rk1108_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
> +					 int pin_num, struct regmap **regmap,
> +					 int *reg, u8 *bit)
> +{
> +	struct rockchip_pinctrl *info = bank->drvdata;
> +
> +	/* The first 24 pins of the first bank are located in PMU */
> +	if (bank->bank_num == 0) {
> +		*regmap = info->regmap_pmu;
> +		*reg = RK1108_PULL_PMU_OFFSET;
> +	} else {
> +		*reg = RK1108_PULL_OFFSET;
> +		*regmap = info->regmap_base;
> +		/* correct the offset, as we're starting with the 2nd bank */
> +		*reg -= 0x10;
> +		*reg += bank->bank_num * RK1108_PULL_BANK_STRIDE;
> +	}
> +
> +	*reg += ((pin_num / RK1108_PULL_PINS_PER_REG) * 4);
> +	*bit = (pin_num % RK1108_PULL_PINS_PER_REG);
> +	*bit *= RK1108_PULL_BITS_PER_PIN;
> +}
> +
> +#define RK1108_DRV_PMU_OFFSET           0x20
> +#define RK1108_DRV_GRF_OFFSET           0x210
> +#define RK1108_DRV_BITS_PER_PIN         2
> +#define RK1108_DRV_PINS_PER_REG         8
> +#define RK1108_DRV_BANK_STRIDE          16

styling nitpick, spaces instead of tabs between name and value. The pull 
constants are correct though and only the drv constants need a fixup.

> +
> +static void rk1108_calc_drv_reg_and_bit(struct rockchip_pin_bank *bank,
> +					int pin_num, struct regmap **regmap,
> +					int *reg, u8 *bit)
> +{
> +	struct rockchip_pinctrl *info = bank->drvdata;
> +
> +	/* The first 24 pins of the first bank are located in PMU */
> +	if (bank->bank_num == 0) {
> +		*regmap = info->regmap_pmu;
> +		*reg = RK1108_DRV_PMU_OFFSET;
> +	} else {
> +		*regmap = info->regmap_base;
> +		*reg = RK1108_DRV_GRF_OFFSET;
> +
> +		/* correct the offset, as we're starting with the 2nd bank */
> +		*reg -= 0x10;
> +		*reg += bank->bank_num * RK1108_DRV_BANK_STRIDE;
> +	}
> +
> +	*reg += ((pin_num / RK1108_DRV_PINS_PER_REG) * 4);
> +	*bit = pin_num % RK1108_DRV_PINS_PER_REG;
> +	*bit *= RK1108_DRV_BITS_PER_PIN;
> +}
> +
>  #define RK2928_PULL_OFFSET		0x118
>  #define RK2928_PULL_PINS_PER_REG	16
>  #define RK2928_PULL_BANK_STRIDE		8

[...]

> @@ -1385,7 +1448,6 @@ static int rockchip_pinconf_set(struct pinctrl_dev
> *pctldev, unsigned int pin, for (i = 0; i < num_configs; i++) {
>  		param = pinconf_to_config_param(configs[i]);
>  		arg = pinconf_to_config_argument(configs[i]);
> -

unrelated change that should be removed.

>  		switch (param) {
>  		case PIN_CONFIG_BIAS_DISABLE:
>  			rc =  rockchip_set_pull(bank, pin - bank->pin_base,