Re: [RFC 4/8] pwm: rzg2l-gpt: Add support for linking with POEG

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Biju,

On Tue, May 10, 2022 at 5:11 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> This patch add support for linking POEG group with pwm, so that
> POEG can control the output disable function.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/pwm/pwm-rzg2l-gpt.c
> +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> @@ -266,6 +291,36 @@ static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>         return ret;
>  }
>
> +static int rzg2l_gpt_parse_properties(struct platform_device *pdev,
> +                                     struct rzg2l_gpt_chip *pc)
> +{
> +       static const u64 poeg_grp_addr[] = {
> +               POEG_GRP_A_ADDR, POEG_GRP_B_ADDR, POEG_GRP_C_ADDR, POEG_GRP_D_ADDR
> +       };
> +       struct device_node *np;
> +       unsigned int i;
> +       u64 addr;
> +
> +       pc->poeg_grp = GRP_INVALID;
> +       np = of_parse_phandle(pdev->dev.of_node, "renesas,poeg-group", 0);
> +       if (!np)
> +               return 0;
> +
> +       if (!of_property_read_u64(np, "reg", &addr)) {
> +               for (i = 0; i < ARRAY_SIZE(poeg_grp_addr); i++) {
> +                       if (addr == poeg_grp_addr[i]) {

Matching on addresses looks fragile to me.
Of course this is code, not DT, so it can be changed later.

Possible alternatives:
  1. Use a numeric property instead of a phandle, so you can store
     its value directly into pc->poeg_grp.
     This loses the linking by phandle, though, which is nice to
     have, and might be useful for other purposes later.
  2. Add a "renesas,id" property to each POEGx DT node, cfr.
     Documentation/devicetree/bindings/media/renesas,vin.yaml.

> +                               pc->poeg_grp = i;
> +                               break;
> +                       }
> +               }
> +       }
> +
> +       if (np)
> +               of_node_put(np);
> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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