Hi Uwe, Thanks for the feedback. Sorry for the delay as I was busy with improving network performance on RZ/G2L platforms. > -----Original Message----- > From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> > Sent: Monday, December 25, 2023 6:29 PM > Subject: Re: [PATCH v17 4/4] pwm: rzg2l-gpt: Add support for gpt linking > with poeg > > On Mon, Nov 20, 2023 at 11:33:07AM +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). > > > > 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> > > --- > > v16->v17: > > * No change > > v15->v16: > > * No change. > > v14->v15: > > * Updated commit description by replacing "This patch add"-> "Add". > > v3->v14: > > * Removed the parenthesis for RZG2L_MAX_POEG_GROUPS. > > * Renamed rzg2l_gpt_parse_properties()->rzg2l_gpt_poeg_init() as it not > only parse > > the properties but also implements the needed register writes. > > * Added acomment here about the purpose of the function > > rzg2l_gpt_poeg_init() > > * Removed magic numbers from rzg2l_gpt_poeg_init() > > * Fixed resource leak in rzg2l_gpt_poeg_init(). > > * Moved the patch from series[1] to here [1] > > https://lore.kernel.org/linux-renesas-soc/20221215205843.4074504-1-bij > > u.das.jz@xxxxxxxxxxxxxx/T/#t > > 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] > > --- > > drivers/pwm/pwm-rzg2l-gpt.c | 83 > > +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 83 insertions(+) > > > > diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c > > index 428e6e577db6..a309131db8ee 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,6 +71,8 @@ > > #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 > > @@ -76,6 +85,9 @@ > > > > #define RZG2L_GET_CH_OFFS(i) (0x100 * (i)) > > > > +#define RZG2L_MAX_POEG_GROUPS 4 > > +#define RZG2L_LAST_POEG_GROUP 3 > > + > > struct rzg2l_gpt_chip { > > struct pwm_chip chip; > > void __iomem *mmio; > > @@ -88,6 +100,7 @@ struct rzg2l_gpt_chip { > > u32 user_count[RZG2L_MAX_HW_CHANNELS]; > > u32 enable_count[RZG2L_MAX_HW_CHANNELS]; > > DECLARE_BITMAP(ch_en_bits, 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) @@ -454,6 +467,75 @@ static void > rzg2l_gpt_reset_assert_pm_disable(void *data) > > reset_control_assert(rzg2l_gpt->rstc); > > } > > > > +/* > > + * 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_poeg_init(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 you use of_for_each_phandle() here, you don't need to determine the > length first. of_for_each_phandle() will iterate 6 times for the below[1] list of phandle and channel index pair tuples. What I need is just loop 3 times and get the poeg_group and gpt_channel from it. So, determining length is simpler than looping 6 times. So, are you ok with existing logic? [1] renesas,poegs = <&poeggd 4>, <&poeggb 1>, <&poegga 2>; > > > + 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 >= %d\n", > > + of_args.args[0], RZG2L_MAX_HW_CHANNELS); > > + of_node_put(of_args.np); > > + return; > > + } > > + > > + bitpos = of_args.args[0]; > > This can be moved further down, and so nearer to where it is actually > used. OK Cheers, Biju
