Re: [PATCH v2 5/8] pinctrl: add pinctrl driver for Rockchip SoCs

["Heiko Stübner" <heiko@sntech.de>]

Am Samstag, 8. Juni 2013, 01:13:48 schrieb Heiko Stübner:
> Am Freitag, 7. Juni 2013, 14:53:51 schrieb Linus Walleij:
> > On Thu, Jun 6, 2013 at 9:11 PM, Heiko Stübner <heiko@sntech.de> wrote:
> > > +       for (i = 0, j = 0; i < size; i += 4, j++) {
> > > +               unsigned long pinconf;
> > > +
> > > +               num = be32_to_cpu(*list++);
> > > +               bank = bank_num_to_bank(info, num);
> > > +               if (IS_ERR(bank))
> > > +                       return PTR_ERR(bank);
> > > +
> > > +               pin = be32_to_cpu(*list++);
> > > +               grp->pins[j] = bank->pin_base + pin;
> > > +               grp->func[j] = be32_to_cpu(*list++);
> > > +
> > > +               pinconf = be32_to_cpu(*list++);
> > > +               switch(pinconf) {
> > > +               case RK_PINCTRL_NONE:
> > > +                       bias = PIN_CONFIG_BIAS_DISABLE;
> > > +                       break;
> > > +               case RK_PINCTRL_PULL_PIN_DEFAULT:
> > > +                       bias = PIN_CONFIG_BIAS_PULL_PIN_DEFAULT;
> > > +                       break;
> > > +               case RK_PINCTRL_PULL_UP:
> > > +                       bias = PIN_CONFIG_BIAS_PULL_UP;
> > > +                       break;
> > > +               case RK_PINCTRL_PULL_DOWN:
> > > +                       bias = PIN_CONFIG_BIAS_PULL_DOWN;
> > > +                       break;
> > > +               }
> > > +
> > > +               grp->configs[j] = pinconf_to_config_packed(bias, 0);
> > > +       }
> > 
> > I would like this to be added to
> > drivers/pinctrl/pinconf-generic.c
> > as a utility function, with the header in
> > drivers/pinctrl/pinconf.h
> > 
> > Also in a separate patch.
> > 
> > > +++ b/include/dt-bindings/pinctrl/rockchip.h
> > 
> > (...)
> > 
> > > +#define RK_PINCTRL_NONE                        0
> > > +#define RK_PINCTRL_PULL_PIN_DEFAULT    (1 << 0)
> > > +#define RK_PINCTRL_PULL_UP             (1 << 1)
> > > +#define RK_PINCTRL_PULL_DOWN           (1 << 2)
> > 
> > So I'd like these moved to /include/dt-bindings/pinctrl/pinconfig.h
> > and used as a separete include. Drop the RK_* prefix as it
> > will be universal and use a PINCONFIG_* prefix instead
> > of PINCTRL_*.
> > 
> > I think that is the route we need to take.
> > 
> > Bonus if you implement more config options from
> > pinconf-generic.h but otherwise me and others will have
> > to do it.
> 
> Gah, damn me for always wanting to get bonus points ;-),
> 
> I did play around a bit with the idea you described above and came up
> with the stuff a bit below. This is in no way meant to be finished, I
> also only was able to compile test it yet, but it looks like it might
> work when I test it tomorrow.
> 
> So if possible I would just like to get a "looks halfway sane, continue"
> or "completely wrong track" please :-) :
> 
> 
> constants in dt-bindings/pinctrl/pinconfig.h:
> -----------------------------
> #define PINCONFIG_NONE			0
> #define PINCONFIG_BIAS_DISABLE		(1 << 0)
> #define PINCONFIG_BIAS_HIGH_IMPEDANCE	(1 << 1)
> #define PINCONFIG_BIAS_BUS_HOLD		(1 << 2)
> #define PINCONFIG_BIAS_PULL_PIN_DEFAULT	(1 << 3)
> #define PINCONFIG_BIAS_PULL_UP		(1 << 4)
> #define PINCONFIG_BIAS_PULL_DOWN	(1 << 5)
> #define PINCONFIG_DRIVE_PUSH_PULL	(1 << 6)
> #define PINCONFIG_DRIVE_OPEN_DRAIN	(1 << 7)
> #define PINCONFIG_DRIVE_OPEN_SOURCE	(1 << 8)
> #define PINCONFIG_INPUT_SCHMITT_ENABLE	(1 << 9)
> #define PINCONFIG_INPUT_SCHMITT_DISABLE	(1 << 10)
> #define PINCONFIG_LOW_POWER_MODE	(1 << 11)
> #define PINCONFIG_OUTPUT_LOW		(1 << 12)
> #define PINCONFIG_OUTPUT_HIGH		(1 << 13)
> 
> 
> and code including how the rockchip driver could look like:
> ---------
> diff --git a/drivers/pinctrl/pinconf-generic.c
> b/drivers/pinctrl/pinconf-generic.c index 9a6812b..35c40be 100644
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
> @@ -139,3 +139,61 @@ void pinconf_generic_dump_config(struct pinctrl_dev
> *pctldev, }
>  EXPORT_SYMBOL_GPL(pinconf_generic_dump_config);
>  #endif
> +
> +/*
> + * Maps the devicetree config bits to actual pinconf values.
> + * The array indizes match the bits set in dt-bindings/pinctrl/pinconf.h
> + * and the array should contain an entry for each bit defined there
> + */
> +static unsigned long conf_map[] = {
> +	PIN_CONF_PACKED(PIN_CONFIG_BIAS_DISABLE, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_BIAS_BUS_HOLD, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_BIAS_PULL_UP, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_BIAS_PULL_DOWN, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_DRIVE_PUSH_PULL, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_DRIVE_OPEN_DRAIN, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_DRIVE_OPEN_SOURCE, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1),
> +	PIN_CONF_PACKED(PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_LOW_POWER_MODE, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_OUTPUT, 0),
> +	PIN_CONF_PACKED(PIN_CONFIG_OUTPUT, 1),
> +};
> +
> +/*
> + * Parse a pinconf bitmap from a devicetree entry into individual pin
> configs. + * @pinconf: the bitmap containing config bits
> + * @configs: after the function returns contains a pointer to an array of
> + *	     pin configs
> + * @nconfigs: number of entries of configs
> + */
> +int pinconf_generic_parse_dt_bitmap(unsigned long pinconf,
> +				    unsigned long *configs,
> +				    unsigned int *nconfigs)
> +{
> +	int bit;
> +	int i;
> +
> +	*nconfigs = hweight_long(pinconf);
> +	configs = kzalloc(*nconfigs * sizeof(unsigned long), GFP_KERNEL);
> +
> +	i = 0;
> +	while (pinconf && i < *nconfigs) {
> +		bit = __ffs(pinconf);
> +		pinconf &= ~BIT(i);
> +
> +		if (bit > ARRAY_SIZE(conf_map)) {
> +			pr_err("%s: unknown bit %d\n", __func__, bit);
> +			kfree(configs);
> +			return -EINVAL;
> +		}
> +
> +		configs[i] = conf_map[bit];
> +
> +		i++;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
> index 92c7267..0fab53a 100644
> --- a/drivers/pinctrl/pinconf.h
> +++ b/drivers/pinctrl/pinconf.h
> @@ -123,3 +123,9 @@ static inline void pinconf_generic_dump_config(struct
> pinctrl_dev *pctldev, return;
>  }
>  #endif
> +
> +#ifdef CONFIG_GENERIC_PINCONF
> +int pinconf_generic_parse_dt_bitmap(unsigned long pinconf,
> +				    unsigned long *configs,
> +				    unsigned int *nconfigs);
> +#endif
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index b77e241..88398df 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -40,6 +40,7 @@
>  #include <dt-bindings/pinctrl/rockchip.h>
> 
>  #include "core.h"
> +#include "pinconf.h"
> 
>  /* GPIO control registers */
>  #define GPIO_SWPORT_DR		0x00
> @@ -639,7 +640,8 @@ static int rockchip_pinctrl_parse_groups(struct
> device_node *np, int num;
>  	int i, j;
>  	int pin;
> -	int bias;
> +	int ret;
> +	int nconfigs;
> 
>  	dev_dbg(info->dev, "group(%d): %s\n", index, np->name);
> 
> @@ -683,22 +685,16 @@ static int rockchip_pinctrl_parse_groups(struct
> device_node *np, grp->func[j] = be32_to_cpu(*list++);
> 
>  		pinconf = be32_to_cpu(*list++);
> -		switch(pinconf) {
> -		case RK_PINCTRL_NONE:
> -			bias = PIN_CONFIG_BIAS_DISABLE;
> -			break;
> -		case RK_PINCTRL_PULL_PIN_DEFAULT:
> -			bias = PIN_CONFIG_BIAS_PULL_PIN_DEFAULT;
> -			break;
> -		case RK_PINCTRL_PULL_UP:
> -			bias = PIN_CONFIG_BIAS_PULL_UP;
> -			break;
> -		case RK_PINCTRL_PULL_DOWN:
> -			bias = PIN_CONFIG_BIAS_PULL_DOWN;
> -			break;
> -		}
> 
> -		grp->configs[j] = pinconf_to_config_packed(bias, 0);
> +		ret = pinconf_generic_parse_dt_bitmap(pinconf,
> +				    &grp->configs[j], &nconfigs);

of course this part is obviously wrong, which I just realized, because the 
configs array only contains usigned longs

it must be more like 
pinconf_generic_parse_dt_bitmap(pinconf, configs, &nconfigs)
...
grp->configs[j] = configs[0];

or better yet simply carrying the created configs without copying entries or 
limiting the number of configs


> +		if (ret)
> +			return ret;
> +
> +		if (nconfigs != 1) {
> +			pr_err("unexpected number of config entries\n");
> +			return -EINVAL;
> +		}
>  	}
> 
>  	return 0;