[PATCH v4 3/9] pinctrl: single: support pinconf generic

[tony@atomide.com (Tony Lindgren)]

Hi,

Noticed few more things while playing with this.

* Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
>  static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
>  				unsigned pin, unsigned long *config)
>  {
> +	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param param = pinconf_to_config_param(*config);
> +	unsigned data;
> +	u32 offset;

	I suggest adding:

	int res = -ENOTSUPP;

> +
> +	offset = pin * (pcs->width / BITS_PER_BYTE);
> +	data = pcs->read(pcs->base + offset);
> +
> +	switch (param) {
> +	case PIN_CONFIG_POWER_SOURCE:
> +		if (pcs->psmask == PCS_OFF_DISABLED
> +			|| pcs->psshift == PCS_OFF_DISABLED)
> +			return -ENOTSUPP;
> +		data &= pcs->psmask;
> +		data = data >> pcs->psshift;
> +		*config = data;
> +		return 0;
> +		break;

	then you can do:

		res = 0;
		break;

	instead of the return 0 and break.

> +	case PIN_CONFIG_BIAS_DISABLE:
> +		if (pcs->bmask == PCS_OFF_DISABLED
> +			|| pcs->bshift == PCS_OFF_DISABLED
> +			|| pcs->bdis == PCS_OFF_DISABLED)
> +			return -ENOTSUPP;
> +		data &= pcs->bmask;
> +		*config = 0;
> +		if (data == pcs->bdis)
> +			return 0;
> +		else
> +			return -EINVAL;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		if (pcs->bmask == PCS_OFF_DISABLED
> +			|| pcs->bshift == PCS_OFF_DISABLED
> +			|| pcs->bpullup == PCS_OFF_DISABLED)
> +			return -ENOTSUPP;
> +		data &= pcs->bmask;
> +		*config = 0;
> +		if (data == pcs->bpullup)
> +			return 0;
> +		else
> +			return -EINVAL;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		if (pcs->bmask == PCS_OFF_DISABLED
> +			|| pcs->bshift == PCS_OFF_DISABLED
> +			|| pcs->bpulldown == PCS_OFF_DISABLED)
> +			return -ENOTSUPP;
> +		data &= pcs->bmask;
> +		*config = 0;
> +		if (data == pcs->bpulldown)
> +			return 0;
> +		else
> +			return -EINVAL;
> +		break;
> +	default:
> +		break;
> +	}
>  	return -ENOTSUPP;

	return res;

>  }


Did you forget to add the input schmitt handling to pcs_pinconf_get()
above?
  
>  static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
>  				unsigned pin, unsigned long config)
>  {
> -	return -ENOTSUPP;
> +	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param config_param = pinconf_to_config_param(config);
> +	unsigned ret, mask = ~0UL;
> +	u32 offset, data;
> +
> +	switch (config_param) {
> +	case PIN_CONFIG_POWER_SOURCE:
> +		if (pcs->psmask == PCS_OFF_DISABLED
> +			|| pcs->psshift == PCS_OFF_DISABLED)
> +			return 0;
> +		mask = pcs->psmask;
> +		data = (pinconf_to_config_argument(config) << pcs->psshift)
> +			& pcs->psmask;
> +		break;
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		if (pcs->bmask == PCS_OFF_DISABLED
> +			|| pcs->bshift == PCS_OFF_DISABLED)
> +			return 0;
> +		mask = pcs->bmask;
> +		data = (pinconf_to_config_argument(config) << pcs->bshift)
> +			& pcs->bmask;
> +		break;
> +	default:
> +		return 0;

Here we should probably return -ENOTSUPP instead for the unhandled
ones?

> +	}
> +	offset = pin * (pcs->width / BITS_PER_BYTE);
> +	ret = pcs->read(pcs->base + offset) & ~mask;
> +	pcs->write(ret | data, pcs->base + offset);
> +	return 0;
>  }

Then looks like pcs_pinconf_set() is also missing handling for few
of the things configured in the probe?

Regards,

Tony