Hi Geert,
Thanks for the feedback.
> Subject: Re: [RFC 4/8] pwm: rzg2l-gpt: Add support for linking with POEG
>
> Hi Biju,
>
> On Tue, May 10, 2022 at 5:11 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
>
> 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.
OK, will use a numeric property as default POEGGroup.
Cheers,
Biju
> 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
