Re: [PATCH v3 2/3] clk: renesas: Add family-specific clock driver for RZ/V2H(P)

[Lad, Prabhakar]

Hi Geert,

On Mon, Jul 15, 2024 at 10:42 AM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jul 15, 2024 at 10:44 AM Lad, Prabhakar
> <prabhakar.csengg@xxxxxxxxx> wrote:
> > On Fri, Jul 12, 2024 at 6:11 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > On Fri, Jul 12, 2024 at 5:29 PM Lad, Prabhakar
> > > <prabhakar.csengg@xxxxxxxxx> wrote:
<snip>
> > /**
> >  * struct rzv2h_reset - Reset definitions
> >  *
> >  * @reset_index: reset register index
> >  * @reset_bit: reset bit
> >  * @mon_index: monitor register index
> >  * @mon_bit: monitor bit
> >  */
> > struct rzv2h_reset {
> >     u8 reset_index;
> >     u8 reset_bit;
> >     u8 mon_index;
> >     u8 mon_bit;
> > };
> >
> > #define DEF_RST_BASE(_resindex, _resbit, _monindex, _monbit)    \
> >     { \
> >         .reset_index = (_resindex), \
> >         .reset_bit = (_resbit), \
> >         .mon_index = (_monindex), \
> >         .mon_bit = (_monbit), \
> >     }
> >
> > #define DEF_RST(_resindex, _resbit, _monindex, _monbit)    \
> >     DEF_RST_BASE(_resindex, _resbit, _monindex, _monbit)
> >
> >
> > in rzv2h_cpg_probe() (.num_resets = ARRAY_SIZE(r9a09g057_resets))
> >
> >     resets = devm_kmalloc_array(dev, info->num_resets, sizeof(struct
> > rzv2h_reset), GFP_KERNEL);
> >     if (!resets)
> >         return -ENOMEM;
> >
> >     for (i = 0; i < priv->num_resets; i++)
> >         memcpy(&resets[i], &info->resets[i], sizeof(struct rzv2h_reset));
>
> You can combine both using devm_kmemdup().
>
Thanks for the pointer.

> > And have the below xlate function that will convert id into index ie
> > index into rests array.
> >
> > static int rzv2h_get_reset_index(struct rzv2h_cpg_priv *priv,
> >                  unsigned long id)
> > {
> >     u8 reset_index = id / 16;
> >     u8 reset_bit = id % 16;
> >     unsigned int i;
> >
> >     for (i = 0; i < priv->num_resets; i++) {
> >         if (priv->resets[i].reset_index == reset_index &&
> >             priv->resets[i].reset_bit == reset_bit)
> >             return i;
> >     }
> >
> >     return -EINVAL;
> > }
> >
> > static int rzv2h_cpg_reset_xlate(struct reset_controller_dev *rcdev,
> >                  const struct of_phandle_args *reset_spec)
> > {
> >     struct rzv2h_cpg_priv *priv = rcdev_to_priv(rcdev);
> >     unsigned int id = reset_spec->args[0];
> >     int index = rzv2h_get_reset_index(priv, id);
> >
> >     if (index < 0) {
> >         dev_err(rcdev->dev, "Invalid reset index %u\n", id);
> >         return -EINVAL;
> >     }
> >
> >     return index;
> > }
> >
> >
> > rzv2h_cpg_assert() and rzv2h_cpg_deassert() which will use an id that
> > can directly index into resets[] array.
> >
> > Please let me know if this is OK.
>
> That would work, too, at the expense of needing a loop for look-up
> (traditional trade-off between memory and time ;-)
> But look-up is only done once (per device), so that should be fine.
>
> It all depends on how many resets you will end up using...
> Memory allocation also has a granularity, so once you have more
> than a specific number of resets, you better use a sparse array,
> and simple indexing.
>
Ok got you, I'll update it and send a new version.

Cheers,
Prabhakar