Re: [RFC PATCH v3 14/35] drivers/irqchip: Add SH7751 Internal INTC drivers.

Hi Sato-san,

Thanks for your patch!

On Sat, Oct 14, 2023 at 4:54 PM Yoshinori Sato
<[email protected]> wrote:
> SH7751 Internal interrupt controler driver.

controller

>
> Signed-off-by: Yoshinori Sato <[email protected]>

> --- /dev/null
> +++ b/drivers/irqchip/irq-renesas-sh7751.c

> +static int __init load_ipr_map(struct device_node *intc,
> +                              struct sh7751_intc_priv *priv)
> +{
> +       int num_ipr;
> +       struct iprmap *p;
> +       u32 val[3];
> +       unsigned int i, idx;
> +
> +       num_ipr = of_property_count_elems_of_size(intc, "renesas,ipr-map", sizeof(u32) * 3);
> +       if (num_ipr < 0)
> +               return num_ipr;
> +
> +       priv->iprmap = kcalloc(num_ipr, sizeof(struct iprmap), GFP_KERNEL);
> +       if (priv->iprmap == NULL) {
> +               pr_err("%s: Failed to alloc memory\n", intc->name);

No need to print anything, as the memory allocation core code already
takes care of that.

> +               return -ENOMEM;
> +       }
> +       priv->num_ipr = num_ipr;
> +
> +       for (p = priv->iprmap, idx = 0; num_ipr > 0; p++, num_ipr--) {
> +               for (i = 0; i < 3; idx++, i++) {
> +                       if (of_property_read_u32_index(intc, "renesas,ipr-map",
> +                                                      idx, &val[i])) {

You can use of_find_property() once to find the property, and iterate
its values using of_prop_next_u32().

> +                               pr_err("%s: Failed to load ipr-map\n", intc->name);
> +                               kfree(priv->iprmap);
> +                               return -EINVAL;
> +                       }
> +               }
> +               p->irq = evt2irq(val[0]);
> +               p->reg = val[1];
> +               p->bits = val[2];
> +       }
> +       /* This table will be searched using bsearch, so sort it. */
> +       sort(priv->iprmap, priv->num_ipr, sizeof(struct iprmap),
> +            sort_ipr_cmp, sort_ipr_swap);
> +
> +       for (p = priv->iprmap, idx = 0; idx < priv->num_ipr; p++, idx++)
> +               pr_debug("irq=%04x reg=%02x bits=%d\n", p->irq, p->reg, p->bits);
> +       return 0;
> +}
> +
> +static int __init sh7751_intc_of_init(struct device_node *intc,
> +                                     struct device_node *parent)
> +{
> +       u16 icr;
> +       struct irq_domain *domain;
> +       void *intc_baseaddr;
> +       void *intc_baseaddr2;
> +       struct sh7751_intc_priv *priv;
> +       int ret;
> +
> +       intc_baseaddr = of_iomap(intc, 0);
> +       intc_baseaddr2 = of_iomap(intc, 1);
> +       if (IS_ERR(intc_baseaddr)) {

of_iomap() returns NULL on failure.
Missing check for intc_baseaddr2.

> +               pr_err("%pOFP: Invalid register definition\n", intc);
> +               return -EINVAL;
> +       }
> +
> +       priv = kzalloc(sizeof(struct sh7751_intc_priv), GFP_KERNEL);
> +       if (priv == NULL) {
> +               pr_err("%pOFP: Failed to alloc memory\n", intc);

No need to print anything, as the memory allocation core code already
takes care of that.

> +               return -ENOMEM;
> +       }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds