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

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4506 bytes --]

Hello Biju,

On Thu, Jun 04, 2026 at 10:56:33AM +0100, Biju wrote:
> From: Biju Das <biju.das.jz@bp.renesas.com>
> 
> 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?

> 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?

> +/*
> + * 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*.

> +	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 */

> +		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.

> +	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.

> +			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.

> +	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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]