Re: [PATCH v1 4/4] pinctrl: intel: Convert capability list to features

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Thu, Jan 07, 2021 at 09:02:00PM +0200, Andy Shevchenko wrote:
> Communities can have features provided in the capability list.
> Traverse the list and convert to respective features.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 41 +++++++++++++++++++++++++--
>  drivers/pinctrl/intel/pinctrl-intel.h |  4 +++
>  2 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 00979acb0203..3d9f22ee987a 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -29,6 +29,16 @@
>  #define REVID_SHIFT			16
>  #define REVID_MASK			GENMASK(31, 16)
>  
> +#define CAPLIST				0x004
> +#define CAPLIST_ID_SHIFT		16
> +#define CAPLIST_ID_MASK			GENMASK(23, 16)
> +#define CAPLIST_ID_GPIO_HW_INFO		1
> +#define CAPLIST_ID_PWM			2
> +#define CAPLIST_ID_BLINK		3
> +#define CAPLIST_ID_EXP			4
> +#define CAPLIST_NEXT_SHIFT		0
> +#define CAPLIST_NEXT_MASK		GENMASK(15, 0)
> +
>  #define PADBAR				0x00c
>  
>  #define PADOWN_BITS			4
> @@ -1472,7 +1482,7 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
>  	for (i = 0; i < pctrl->ncommunities; i++) {
>  		struct intel_community *community = &pctrl->communities[i];
>  		void __iomem *regs;
> -		u32 padbar;
> +		u32 offset;
>  		u32 value;
>  
>  		*community = pctrl->soc->communities[i];
> @@ -1492,11 +1502,36 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
>  			break;
>  		}
>  
> +		/* Determine community features based on the capabilities */
> +		offset = CAPLIST;
> +		do {
> +			value = readl(regs + offset);
> +			switch ((value & CAPLIST_ID_MASK) >> CAPLIST_ID_SHIFT) {
> +			case CAPLIST_ID_GPIO_HW_INFO:
> +				community->features |= PINCTRL_FEATURE_GPIO_HW_INFO;
> +				break;
> +			case CAPLIST_ID_PWM:
> +				community->features |= PINCTRL_FEATURE_PWM;
> +				break;
> +			case CAPLIST_ID_BLINK:
> +				community->features |= PINCTRL_FEATURE_BLINK;
> +				break;
> +			case CAPLIST_ID_EXP:
> +				community->features |= PINCTRL_FEATURE_EXP;
> +				break;
> +			default:
> +				break;
> +			}
> +			offset = (value & CAPLIST_NEXT_MASK) >> CAPLIST_NEXT_SHIFT;

I suggest adding some check, like that we visited the previous offset
already, so that we do not loop here forever if we find wrongly
formatted capability list.

Otherwise looks good.

> +		} while (offset);