Re: [PATCH 4/8] clk: renesas: cpg-mssr: Add support for reset control

[Geert Uytterhoeven]

Hi Philipp,

On Fri, Jan 20, 2017 at 4:57 PM, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote:
> On Fri, 2017-01-20 at 15:08 +0100, Geert Uytterhoeven wrote:
>> Add optional support for the Reset Control feature of the Renesas Clock
>> Pulse Generator / Module Standby and Software Reset module on R-Car
>> Gen2, R-Car Gen3, and RZ/G1 SoCs.
>
> Is there a reason to make this optional?

With "optional", I mean that I don't select CONFIG_RESET_CONTROLLER, and
make the reset controller code depend on CONFIG_RESET_CONTROLLER.
So far we don't have any mandatory users.

>> This allows to reset SoC devices using the Reset Controller API.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> Looks good to me,
>
> Acked-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>

Thanks!

> Just a small issue below,
>
>> ---
>>  drivers/clk/renesas/renesas-cpg-mssr.c | 122 +++++++++++++++++++++++++++++++++
>>  1 file changed, 122 insertions(+)
>>
>> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
>> index f1161a585c57e433..ea4af714ac14603a 100644
>> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
>> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> [...]
>> +static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
>> +                       unsigned long id)
>> +{
>> +     struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
>> +     unsigned int reg = id / 32;
>> +     unsigned int bit = id % 32;
>> +     u32 bitmask = BIT(bit);
>
> Here you have a bitmask = BIT(bit) variable.

Because there are two users in the function.

>> +     unsigned long flags;
>> +     u32 value;
>> +
>> +     dev_dbg(priv->dev, "reset %u%02u\n", reg, bit);
>> +
>> +     /* Reset module */
>> +     spin_lock_irqsave(&priv->rmw_lock, flags);
>> +     value = readl(priv->base + SRCR(reg));
>> +     value |= bitmask;
>
> Here you use it.
>
>> +     writel(value, priv->base + SRCR(reg));
>> +     spin_unlock_irqrestore(&priv->rmw_lock, flags);
>> +
>> +     /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */
>> +     udelay(35);
>> +
>> +     /* Release module from reset state */
>> +     writel(bitmask, priv->base + SRSTCLR(reg));
>> +
>> +     return 0;
>> +}
>> +
>> +static int cpg_mssr_assert(struct reset_controller_dev *rcdev, unsigned long id)
>> +{
>> +     struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
>> +     unsigned int reg = id / 32;
>> +     unsigned int bit = id % 32;
>
> Here you haven't.
>
>> +     unsigned long flags;
>> +     u32 value;
>> +
>> +     dev_dbg(priv->dev, "assert %u%02u\n", reg, bit);
>> +
>> +     spin_lock_irqsave(&priv->rmw_lock, flags);
>> +     value = readl(priv->base + SRCR(reg));
>> +     writel(value | BIT(bit), priv->base + SRCR(reg));
>
> Here you don't.

Because there's a single user in the function.

>> +     spin_unlock_irqrestore(&priv->rmw_lock, flags);
>> +     return 0;
>> +}
>> +
>> +static int cpg_mssr_deassert(struct reset_controller_dev *rcdev,
>> +                          unsigned long id)
>> +{
>> +     struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
>> +     unsigned int reg = id / 32;
>> +     unsigned int bit = id % 32;
>> +
>> +     dev_dbg(priv->dev, "deassert %u%02u\n", reg, bit);
>> +
>> +     writel(BIT(bit), priv->base + SRSTCLR(reg));
>
> And here ...
>
>> +     return 0;
>> +}
>> +
>> +static int cpg_mssr_status(struct reset_controller_dev *rcdev,
>> +                        unsigned long id)
>> +{
>> +     struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
>> +     unsigned int reg = id / 32;
>> +     unsigned int bit = id % 32;
>> +
>> +     return !!(readl(priv->base + SRCR(reg)) & BIT(bit));
>
> And here neither.
>
> I'd choose one variant over the other for consistency.

OK, I'll use the "bitmask" variable in all functions.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds