[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-renesas-soc
Subject: RE: [PATCH v3 3/3] pwm: rzg2l-gpt: Add support for gpt linking with poeg
From: Biju Das <biju.das.jz () bp ! renesas ! com>
Date: 2023-02-28 12:10:32
Message-ID: OS0PR01MB59229501EDB0E9D12015B7FF86AC9 () OS0PR01MB5922 ! jpnprd01 ! prod ! outlook ! com
[Download RAW message or body]
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@bp.renesas.com>
> > ---
> > 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@bp.renesas.com/
> > ---
> > 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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic