Re: [PATCH 1/3] pinctrl: cherryview: Don't use pin/offset 0 to mark an interrupt-line as unused

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

Hi Hans,

Few minor comments below.

On Thu, Nov 18, 2021 at 12:16:12AM +0100, Hans de Goede wrote:
> offset/pin 0 is a perfectly valid offset, so stop using it to have

Offset/pin (with capital O).

> the special meaning of interrupt-line not used in the intr_lines.

interrupt line

> Instead introduce a new special INTR_LINE_UNUSED value which is never
> a valid offset and use that to indicate unused interrupt-lines.

interrupt lines

> Cc: Yauhen Kharuzhy <jekhor@xxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
>  drivers/pinctrl/intel/pinctrl-cherryview.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index 980099028cf8..a46f9e5a4748 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -73,6 +73,8 @@ struct intel_pad_context {
>  	u32 padctrl1;
>  };
>  
> +#define INTR_LINE_UNUSED		U32_MAX
> +
>  /**
>   * struct intel_community_context - community context for Cherryview
>   * @intr_lines: Mapping between 16 HW interrupt wires and GPIO offset (in GPIO number space)
> @@ -812,7 +814,7 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
>  		/* Reset the interrupt mapping */
>  		for (i = 0; i < ARRAY_SIZE(cctx->intr_lines); i++) {
>  			if (cctx->intr_lines[i] == offset) {
> -				cctx->intr_lines[i] = 0;
> +				cctx->intr_lines[i] = INTR_LINE_UNUSED;
>  				break;
>  			}
>  		}
> @@ -1319,7 +1321,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
>  		else
>  			handler = handle_edge_irq;
>  
> -		if (!cctx->intr_lines[intsel]) {
> +		if (cctx->intr_lines[intsel] == INTR_LINE_UNUSED) {
>  			irq_set_handler_locked(d, handler);
>  			cctx->intr_lines[intsel] = pin;
>  		}
> @@ -1412,6 +1414,12 @@ static void chv_gpio_irq_handler(struct irq_desc *desc)
>  		unsigned int offset;
>  
>  		offset = cctx->intr_lines[intr_line];
> +		if (offset == INTR_LINE_UNUSED) {
> +			dev_err(pctrl->dev, "Interrupt on unused interrupt line %u\n",

Let's be consistent with the logging so no capital letter here =>
"interrupt on .."

> +				intr_line);
> +			continue;
> +		}
> +
>  		generic_handle_domain_irq(gc->irq.domain, offset);
>  	}
>  
> @@ -1620,9 +1628,10 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
>  	struct intel_community *community;
>  	struct device *dev = &pdev->dev;
>  	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	struct intel_community_context *cctx;
>  	struct intel_pinctrl *pctrl;
>  	acpi_status status;
> -	int ret, irq;
> +	int i, ret, irq;
>  
>  	soc_data = intel_pinctrl_get_soc_data(pdev);
>  	if (IS_ERR(soc_data))
> @@ -1663,6 +1672,10 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
>  	if (!pctrl->context.communities)
>  		return -ENOMEM;
>  
> +	cctx = &pctrl->context.communities[0];
> +	for (i = 0; i < ARRAY_SIZE(cctx->intr_lines); i++)
> +		cctx->intr_lines[i] = INTR_LINE_UNUSED;
> +
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
> -- 
> 2.31.1