Re: [PATCH v3 4/4] clk: bcm: Add BCM63268 timer clock and reset driver

[Stephen Boyd]

Quoting Álvaro Fernández Rojas (2023-03-21 13:10:22)
> diff --git a/drivers/clk/bcm/clk-bcm63268-timer.c b/drivers/clk/bcm/clk-bcm63268-timer.c
> new file mode 100644
> index 000000000000..6a1fdd193cb5
> --- /dev/null
> +++ b/drivers/clk/bcm/clk-bcm63268-timer.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * BCM63268 Timer Clock and Reset Controller Driver
[...]
> +
> +static inline struct bcm63268_tclkrst_hw *
> +to_bcm63268_timer_reset(struct reset_controller_dev *rcdev)
> +{
> +       return container_of(rcdev, struct bcm63268_tclkrst_hw, rcdev);
> +}
> +
> +static int bcm63268_timer_reset_update(struct reset_controller_dev *rcdev,
> +                               unsigned long id, bool assert)
> +{
> +       struct bcm63268_tclkrst_hw *reset = to_bcm63268_timer_reset(rcdev);
> +       unsigned long flags;
> +       uint32_t val;
> +
> +       spin_lock_irqsave(&reset->lock, flags);
> +       val = __raw_readl(reset->regs);

Use regular ol readl() here, unless you have some need for no barrires
or byte swapping.

> +       if (assert)
> +               val &= ~BIT(id);
> +       else
> +               val |= BIT(id);
> +       __raw_writel(val, reset->regs);

Same.

> +       spin_unlock_irqrestore(&reset->lock, flags);
> +
> +       return 0;
> +}
> +
[...]
> +
> +static int bcm63268_tclk_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       const struct bcm63268_tclk_table_entry *entry, *table;
> +       struct bcm63268_tclkrst_hw *hw;
> +       struct clk_hw *clk;
> +       u8 maxbit = 0;
> +       int i, ret;
> +
> +       table = of_device_get_match_data(dev);

Use device_get_match_data() instead.

> +       if (!table)
> +               return -EINVAL;
> +
> +       for (entry = table; entry->name; entry++)
> +               maxbit = max(maxbit, entry->bit);
> +       maxbit++;
> +
> +       hw = devm_kzalloc(&pdev->dev, struct_size(hw, data.hws, maxbit),
> +                         GFP_KERNEL);
> +       if (!hw)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, hw);
> +
> +       spin_lock_init(&hw->lock);
> +
> +       hw->data.num = maxbit;
> +       for (i = 0; i < maxbit; i++)
> +               hw->data.hws[i] = ERR_PTR(-ENODEV);
> +
> +       hw->regs = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(hw->regs))
> +               return PTR_ERR(hw->regs);
> +
> +       for (entry = table; entry->name; entry++) {
> +               clk = clk_hw_register_gate(dev, entry->name, NULL, 0,

Use devm?

> +                                          hw->regs, entry->bit,
> +                                          CLK_GATE_BIG_ENDIAN, &hw->lock);
> +               if (IS_ERR(clk)) {
> +                       ret = PTR_ERR(clk);
> +                       goto out_err;
> +               }
> +
> +               hw->data.hws[entry->bit] = clk;
> +       }
> +
> +       ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +                                         &hw->data);
> +       if (ret)
> +               return ret;
> +
> +       hw->rcdev.of_node = dev->of_node;
> +       hw->rcdev.ops = &bcm63268_timer_reset_ops;
> +
> +       ret = devm_reset_controller_register(dev, &hw->rcdev);
> +       if (ret)
> +               dev_err(dev, "Failed to register reset controller\n");
> +
> +       return 0;
> +
> +out_err:
> +       for (i = 0; i < hw->data.num; i++) {
> +               if (!IS_ERR(hw->data.hws[i]))
> +                       clk_hw_unregister_gate(hw->data.hws[i]);

And then drop this?

> +       }
> +
> +       return ret;
> +}
> +
> +static const struct of_device_id bcm63268_tclk_dt_ids[] = {
> +       {
> +               .compatible = "brcm,bcm63268-timer-clocks",
> +               .data = &bcm63268_timer_clocks,

Are you planning on adding more SoCs to this driver? The data can
currently be always assumed to be bcm63268_timer_clocks

> +       }, {
> +               /* sentinel */
> +       }
> +};
> +