Re: [PATCH v3 3/3] pwm: rzg2l-gpt: Add support for gpt linking with poeg

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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/20221214132232.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).

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

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

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?

> +				dev_err(&pdev->dev,
> +					"Invalid poeg group %d > 3\n", poeg_grp);

You're missing

+				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()?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux