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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 07 Jan 2019 13:55:42 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Brian Masney <masneyb@...tation.org>, andy.gross@...aro.org,
        bjorn.andersson@...aro.org, lee.jones@...aro.org,
        linus.walleij@...aro.org
Cc:     marc.zyngier@....com, shawnguo@...nel.org, dianders@...omium.org,
        linux-gpio@...r.kernel.org, nicolas.dechesne@...aro.org,
        niklas.cassel@...aro.org, david.brown@...aro.org,
        robh+dt@...nel.org, mark.rutland@....com, thierry.reding@...il.com,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v2 4/5] qcom: spmi-gpio: add support for hierarchical IRQ chip

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);
> +}
> +