RE: [PATCH v6 03/11] pwm: rzg2l-gpt: Add support for gpt linking with poeg

[Biju Das]

Hello Uwe,

Thanks for the feedback.

> -----Original Message-----
> From: Uwe Kleine-König <ukleinek@xxxxxxxxxx>
> Sent: 16 July 2026 09:26
> Subject: Re: [PATCH v6 03/11] pwm: rzg2l-gpt: Add support for gpt linking with poeg
> 
> Hello Biju,
> 
> On Thu, Jun 04, 2026 at 10:56:33AM +0100, Biju wrote:
> > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > 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).
> 
> What is a dead time error?

Hardware manual does not mention any thing related to dead time error.

I believe it is standard one

"
Dead-time error is the unwanted change in average output voltage and current
caused by adding a safety delay between complementary switching transistors.

To prevent a short circuit ("shoot-through"), the high-side and low-side
transistors in a switching leg must never turn on simultaneously.
The intentional delay where both switches are off is called dead-time.
However, this safety window distorts the ideal PWM waveform, creating an error."

> 
> > Add support for linking poeg group with gpt, so that gpt can control
> > the output disable function by adding rzg2l_gpt_poeg_init() to parse
> > the renesas,poegs device tree property and establish links between
> > POEG groups (A–D) and GPT hardware channels (0–7). For each valid,
> > enabled POEG phandle entry, the driver:
> >  - Reads the renesas,poeg-id from the POEG node and validates it against
> >    the supported range
> >  - Records the GPT–POEG association in a per-chip bitmap
> > (poeg_gpt_link)
> >  - Configures GTINTAD to route the output disable request to the correct
> >    POEG group
> >  - Configures GTIOR (OADF/OBDF fields) to set both output pins to
> >    high-impedance on an output disable event
> 
> For my understanding: If GPT is linked to a POEG, an error detected by GPT makes the pin High-Z?

It is configurable. Currently the driver configured for
High-Z as default.

b10, b9 OADF[1:0] All 0 R/W GTIOCA Pin Disable Value Setting

0 0: Prohibit output disable
0 1: Set GTIOCA pin to Hi-Z on output disable
1 0: Set GTIOCA pin to 0 on output disable
1 1: Set GTIOCA pin to 1 on output disable.

> 
> > +/*
> > + * This function links a POEG group{A,B,C,D} with a GPT channel{0..7}
> > +and
> > + * configures the pin for output disable.
> > + */
> > +static int rzg2l_gpt_poeg_init(struct platform_device *pdev,
> > +			       struct rzg2l_gpt_chip *rzg2l_gpt) {
> > +	const char *poeg_name = "renesas,poegs";
> > +	struct of_phandle_args of_args;
> > +	struct property *poegs;
> > +	unsigned int i;
> > +	u32 poeg_grp;
> > +	u32 bitpos;
> > +	int cells;
> > +	int ret;
> > +
> > +	poegs = of_find_property(pdev->dev.of_node, poeg_name, NULL);
> > +	if (!poegs)
> > +		return 0;
> > +
> > +	cells = of_property_count_u32_elems(pdev->dev.of_node, poeg_name);
> 
> It's a bit sad that of_find_property() is called twice here. But I didn't spot a function that implements
> what
> of_property_count_u32_elems() does for a given struct property*.

OK.

> 
> > +	if (cells < 0)
> > +		return cells;
> > +
> > +	if (cells & 1)
> 
> Maybe add a comment here like:
> 
> 	/* poegs is a list of pairs, so cells must be even */

OK.

> 
> > +		return -EINVAL;
> > +
> > +	cells >>= 1;
> 
> I think a better name for `cells` from here on would be beneficial, something like `num_poeg_pairs`. For
> before here the name isn't optimal, but I don't have a spontanious suggestion here. `len` comes to mind.

OK will use `num_poeg_pairs` instead of cells.

> 
> > +	for (i = 0; i < cells; i++) {
> > +		ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
> > +						       poeg_name, 1, i,
> > +						       &of_args);
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (of_args.args[0] >= RZG2L_MAX_HW_CHANNELS) {
> > +			dev_err(&pdev->dev, "Invalid channel %u >= %u\n",
> > +				of_args.args[0], RZG2L_MAX_HW_CHANNELS);
> 
> Given that rzg2l_gpt_poeg_init() is called from .probe() only, use
> dev_err_probe() here.

Agreed.


> 
> > +			goto err_of_node;
> > +		}
> > +
> > +		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)) {
> > +			if (poeg_grp > RZG2L_LAST_POEG_GROUP) {
> > +				dev_err(&pdev->dev, "Invalid poeg group %u > %u\n",
> > +					poeg_grp, RZG2L_LAST_POEG_GROUP);
> > +				goto err_of_node;
> > +			}
> > +
> > +			bitpos = of_args.args[0] + poeg_grp * RZG2L_MAX_HW_CHANNELS;
> > +			set_bit(bitpos, rzg2l_gpt->poeg_gpt_link);
> > +
> > +			rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTINTAD(of_args.args[0]),
> > +					 RZG2L_GTINTAD_GRP_MASK, poeg_grp << 24);
> > +
> > +			rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR(of_args.args[0]),
> > +					 RZG2L_GTIOR_OBDF | RZG2L_GTIOR_OADF,
> > +					 RZG2L_GTIOR_PIN_DISABLE_SETTING);
> > +		}
> > +
> > +		of_node_put(of_args.np);
> > +	}
> > +
> > +	return 0;
> > +
> > +err_of_node:
> > +	of_node_put(of_args.np);
> 
> Would be great if this could be prettified using __free.

Will use __free.

Cheers,
Biju

> 
> > +	return -EINVAL;
> > +}
> > +
> >  static int rzg2l_gpt_probe(struct platform_device *pdev)  {
> >  	struct rzg2l_gpt_chip *rzg2l_gpt;
> > @@ -426,6 +515,10 @@ static int rzg2l_gpt_probe(struct platform_device *pdev)
> >  	if (rzg2l_gpt->rate_khz * KILO != rate)
> >  		return dev_err_probe(dev, -EINVAL, "Rate is not multiple of 1000");
> >
> > +	ret = rzg2l_gpt_poeg_init(pdev, rzg2l_gpt);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to link gpt with poeg\n");
> > +
> >  	mutex_init(&rzg2l_gpt->lock);
> >
> >  	chip->ops = &rzg2l_gpt_ops;
> 
> Best regards
> Uwe