Hi Uwe, Thanks for the feedback. > Subject: Re: [PATCH v3 3/3] pwm: rzg2l-gpt: Add support for gpt linking with > poeg > > Hello, > > On Thu, Dec 15, 2022 at 08:58:43PM +0000, Biju Das wrote: > > The General PWM Timer (GPT) is capable of detecting "dead time error > > and short-circuits between output pins" and send Output disable > > request to poeg(Port Output Enable for GPT). > > > > This patch add support for linking poeg group with gpt, so that gpt > > can control the output disable function. > > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > > --- > > v2->v3: > > * Updated commit header and description > > * Added check for poeg group in rzg2l_gpt_parse_properties(). > > v1->v2: > > * Replaced id->poeg-id as per poeg bindings. > > This patch depend upon [1] > > [1] > > https://patchwork.kernel.org/project/linux-renesas-soc/patch/202212141 > > 32232.2835828-3-biju.das.jz@xxxxxxxxxxxxxx/ > > --- > > drivers/pwm/pwm-rzg2l-gpt.c | 76 > > +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 76 insertions(+) > > > > diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c > > index fa916f020061..6bf407550326 100644 > > --- a/drivers/pwm/pwm-rzg2l-gpt.c > > +++ b/drivers/pwm/pwm-rzg2l-gpt.c > > @@ -31,6 +31,7 @@ > > #define RZG2L_GTCR 0x2c > > #define RZG2L_GTUDDTYC 0x30 > > #define RZG2L_GTIOR 0x34 > > +#define RZG2L_GTINTAD 0x38 > > #define RZG2L_GTBER 0x40 > > #define RZG2L_GTCNT 0x48 > > #define RZG2L_GTCCRA 0x4c > > @@ -48,9 +49,15 @@ > > #define RZG2L_UP_COUNTING (RZG2L_GTUDDTYC_UP | RZG2L_GTUDDTYC_UDF) > > > > #define RZG2L_GTIOR_GTIOA GENMASK(4, 0) > > +#define RZG2L_GTIOR_OADF GENMASK(10, 9) > > #define RZG2L_GTIOR_GTIOB GENMASK(20, 16) > > +#define RZG2L_GTIOR_OBDF GENMASK(26, 25) > > #define RZG2L_GTIOR_OAE BIT(8) > > #define RZG2L_GTIOR_OBE BIT(24) > > +#define RZG2L_GTIOR_OADF_HIGH_IMP_ON_OUT_DISABLE BIT(9) > > +#define RZG2L_GTIOR_OBDF_HIGH_IMP_ON_OUT_DISABLE BIT(25) > > +#define RZG2L_GTIOR_PIN_DISABLE_SETTING \ > > + (RZG2L_GTIOR_OADF_HIGH_IMP_ON_OUT_DISABLE | > > +RZG2L_GTIOR_OBDF_HIGH_IMP_ON_OUT_DISABLE) > > > > #define RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE 0x07 > > #define RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE 0x1b > > @@ -64,12 +71,16 @@ > > #define RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH \ > > (FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE) > > | RZG2L_GTIOR_OBE) > > > > +#define RZG2L_GTINTAD_GRP_MASK GENMASK(25, 24) > > + > > #define RZG2L_GTCCR(i) (0x4c + 4 * (i)) > > > > #define RZG2L_MAX_HW_CHANNELS (8) > > #define RZG2L_CHANNELS_PER_IO (2) > > #define RZG2L_MAX_PWM_CHANNELS (RZG2L_MAX_HW_CHANNELS * > RZG2L_CHANNELS_PER_IO) > > > > +#define RZG2L_MAX_POEG_GROUPS (4) > > The parenthesis are not needed (ditto for RZG2L_MAX_HW_CHANNELS and > RZG2L_CHANNELS_PER_IO). Agreed. > > > + > > #define RZG2L_IS_IOB(a) ((a) & 0x1) > > #define RZG2L_GET_CH_INDEX(a) ((a) / 2) > > > > @@ -91,6 +102,7 @@ struct rzg2l_gpt_chip { > > */ > > u8 prescale[RZG2L_MAX_HW_CHANNELS]; > > unsigned int duty_cycle[RZG2L_MAX_PWM_CHANNELS]; > > + DECLARE_BITMAP(poeg_gpt_link, RZG2L_MAX_POEG_GROUPS * > > +RZG2L_MAX_HW_CHANNELS); > > }; > > > > static inline struct rzg2l_gpt_chip *to_rzg2l_gpt_chip(struct > > pwm_chip *chip) @@ -470,6 +482,69 @@ static void > rzg2l_gpt_reset_assert_pm_disable(void *data) > > reset_control_assert(rzg2l_gpt->rstc); > > } > > > > A comment here about the purpose of the function would be nice. Just from > reading the code it's totally unobvious what happens here. OK, will add the below comment. /* * This function links a poeg group{A,B,C,D} with a gpt channel{0..7} and * configure the pin for output disable. */ > > > +static void rzg2l_gpt_parse_properties(struct platform_device *pdev, > > + struct rzg2l_gpt_chip *rzg2l_gpt) { > > + struct of_phandle_args of_args; > > + unsigned int i; > > + u32 poeg_grp; > > + u32 bitpos; > > + int cells; > > + u32 offs; > > + int ret; > > + > > + cells = of_property_count_u32_elems(pdev->dev.of_node, > "renesas,poegs"); > > + if (cells == -EINVAL) > > + return; > > + > > + cells >>= 1; > > + for (i = 0; i < cells; i++) { > > + ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node, > > + "renesas,poegs", 1, i, > > + &of_args); > > + if (ret) { > > + dev_err(&pdev->dev, > > + "Failed to parse 'renesas,poegs' property\n"); > > + return; > > + } > > + > > + if (of_args.args[0] >= RZG2L_MAX_HW_CHANNELS) { > > + dev_err(&pdev->dev, > > + "Invalid channel %d > 7\n", of_args.args[0]); > > this hardcoded 7 is a bit ugly. Something like > > + dev_err(&pdev->dev, > + "Invalid channel %d >= %d\n", of_args.args[0], > +RZG2L_MAX_HW_CHANNELS); OK, will do. > > or > > + dev_err(&pdev->dev, > + "Invalid channel %d >= " > stringify(RZG2L_MAX_HW_CHANNELS) "\n", > +of_args.args[0]); > > is IMHO nicer. > > > + return; > > + } > > + > > + bitpos = of_args.args[0]; > > + if (!of_device_is_available(of_args.np)) { > > + /* It's fine to have a phandle to a non-enabled poeg. */ > > + of_node_put(of_args.np); > > + continue; > > + } > > + > > + if (!of_property_read_u32(of_args.np, "renesas,poeg-id", > &poeg_grp)) { > > + offs = RZG2L_GET_CH_OFFS(of_args.args[0]); > > + if (poeg_grp > 3) { > > Maybe a cpp define for this 3? OK, will use the macro RZG2L_LAST_POEG_GROUP. > > > + dev_err(&pdev->dev, > > + "Invalid poeg group %d > 3\n", poeg_grp); > > You're missing OK, will add here and above as well for the HW channel check. > > + of_node_put(of_args.np); > > here. > > > + return; > > + } > > + > > + bitpos += poeg_grp * RZG2L_MAX_HW_CHANNELS; > > + set_bit(bitpos, rzg2l_gpt->poeg_gpt_link); > > + > > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTINTAD, > > + RZG2L_GTINTAD_GRP_MASK, > > + poeg_grp << 24); > > + > > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTIOR, > > + RZG2L_GTIOR_OBDF | RZG2L_GTIOR_OADF, > > + RZG2L_GTIOR_PIN_DISABLE_SETTING); > > + } > > + > > + of_node_put(of_args.np); > > + } > > +} > > + > > static int rzg2l_gpt_probe(struct platform_device *pdev) { > > DECLARE_BITMAP(ch_en_bits, RZG2L_MAX_PWM_CHANNELS); @@ -512,6 +587,7 > > @@ static int rzg2l_gpt_probe(struct platform_device *pdev) > > if (ret < 0) > > goto clk_disable; > > > > + rzg2l_gpt_parse_properties(pdev, rzg2l_gpt); > > I don't like the function name. THe function doesn't only parse the > properties but also implements the needed register writes. Maybe > rzg2l_gpt_poeg_init()? OK will change the function to rzg2l_gpt_poeg_init() Cheers, Biju
