Re: [PATCH v2 4/5] qcom: spmi-gpio: add support for hierarchical IRQ chip

[Stephen Boyd <sboyd@xxxxxxxxxx>]

Quoting Brian Masney (2019-01-06 18:11:44)
> @@ -762,11 +763,14 @@ static int pmic_gpio_of_xlate(struct gpio_chip *chip,
>  static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
>  {
>         struct pmic_gpio_state *state = gpiochip_get_data(chip);
> -       struct pmic_gpio_pad *pad;
> +       struct irq_fwspec fwspec;
>  
> -       pad = state->ctrl->desc->pins[pin].drv_data;
> +       fwspec.fwnode = state->fwnode;
> +       fwspec.param_count = 2;
> +       fwspec.param[0] = pin + 1;

There seems to be PMIC_GPIO_PHYSICAL_OFFSET for these purposes.

> +       fwspec.param[1] = IRQ_TYPE_NONE;
>  
> -       return pad->irq;
> +       return irq_create_fwspec_mapping(&fwspec);
>  }
>  
>  static void pmic_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> @@ -936,6 +940,86 @@ static int pmic_gpio_populate(struct pmic_gpio_state *state,
>         return 0;
>  }
>  
> +static struct irq_chip pmic_gpio_irq_chip = {
> +       .name = "spmi-gpio",
> +       .irq_ack = irq_chip_ack_parent,
> +       .irq_mask = irq_chip_mask_parent,
> +       .irq_unmask = irq_chip_unmask_parent,
> +       .irq_set_type = irq_chip_set_type_parent,
> +       .irq_set_wake = irq_chip_set_wake_parent,
> +       .flags = IRQCHIP_MASK_ON_SUSPEND,
> +};
> +
> +static int pmic_gpio_irq_activate(struct irq_domain *domain,
> +                                 struct irq_data *data, bool reserve)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;
> +
> +       return gpiochip_lock_as_irq(&state->chip, data->hwirq);
> +}
> +
> +static void pmic_gpio_irq_deactivate(struct irq_domain *domain,
> +                                    struct irq_data *data)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;
> +
> +       gpiochip_unlock_as_irq(&state->chip, data->hwirq);
> +}

I meant these sorts of wrappers that could be used by other drivers
possibly, if all they need to store in their domain->host_data is the
gpiochip structure (they can probably indirect through the gpiochip
again to get their driver specific structure).

int gpiochip_irq_domain_activate(struct irq_domain *domain,
				 struct irq_data *data, bool reserve)
{
	struct gpiochip *chip = domain->host_data;

	return gpiochip_lock_as_irq(chip, data->hwirq);
}

void gpiochip_irq_domain_deactivate(struct irq_domain *domain,
				    struct irq_data *data)
{
	struct gpiochip *chip = domain->host_data;

	return gpiochip_unlock_as_irq(chip, data->hwirq);
}

> +
> +static int pmic_gpio_domain_translate(struct irq_domain *domain,
> +                                     struct irq_fwspec *fwspec,
> +                                     unsigned long *hwirq,
> +                                     unsigned int *type)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;
> +
> +       if (fwspec->param_count != 2 || fwspec->param[0] >= state->chip.ngpio)
> +               return -EINVAL;
> +
> +       *hwirq = fwspec->param[0] - 1;

There seems to be PMIC_GPIO_PHYSICAL_OFFSET for these purposes.

> +       *type = fwspec->param[1];
> +
> +       return 0;
> +}
> +
> +static int pmic_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +                                 unsigned int nr_irqs, void *data)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;
> +       struct irq_fwspec *fwspec = data;
> +       struct irq_fwspec parent_fwspec;
> +       irq_hw_number_t hwirq;
> +       unsigned int type;
> +       int ret, i;
> +
> +       ret = pmic_gpio_domain_translate(domain, fwspec, &hwirq, &type);
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < nr_irqs; i++)
> +               irq_domain_set_info(domain, virq + i, hwirq + i,
> +                                   &pmic_gpio_irq_chip, state,
> +                                   handle_level_irq, NULL, NULL);
> +
> +       parent_fwspec.fwnode = domain->parent->fwnode;
> +       parent_fwspec.param_count = 4;
> +       parent_fwspec.param[0] = 0;
> +       parent_fwspec.param[1] = fwspec->param[0] + 0xc0 - 1;

Is the -1 there twice, once in pmic_gpio_domain_translate() and then
here?

> +       parent_fwspec.param[2] = 0;
> +       parent_fwspec.param[3] = fwspec->param[1];
> +
> +       return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> +                                           &parent_fwspec);
> +}
> +