Hello Brian

On Tue, Feb 17, 2026 at 10:14 AM Brian Masney [off-list ref] wrote:

Hi Anirudh,

On Mon, Feb 16, 2026 at 04:16:34PM -0600, Anirudh Srinivasan wrote:

quoted

Add driver for clock controller in Tenstorrent Atlantis SoC. This version
of the driver coves clocks from RCPU syscon.

...covers clocks..

Thank you for your comments. I will address the typos and add the
static modifier for the different variables that you suggested.

quoted

+
+struct atlantis_clk_gate_shared_config {
+     u32 reg_offset;
+     u32 enable;
+     unsigned int *share_count;

Why is this a pointer? Could this just be a plain unsigned int since
all occurrences of this are dereferenced?

We have a group of gate clocks that have a single enable bit shared
among them (instead of individual enable bits for each clock). We need
to keep track of the number of clocks within a group that have
requested an enable, and only unset the bit if all the clocks are
disabled. share_count is used to keep track of this. It gets updated
by each clock. Hence it's a pointer (and the mutexes around access to
it).

quoted

+static int atlantis_clk_gate_is_enabled(struct clk_hw *hw)
+{
+     struct atlantis_clk_gate *gate = hw_to_atlantis_clk_gate(hw);
+     u32 val;
+
+     regmap_read(gate->common.regmap, gate->config.reg_offset, &val);
+
+     val &= gate->config.enable;
+
+     return val ? 1 : 0;

What do you think about this instead?

    return !!val;

Ack

quoted

+static int atlantis_clk_gate_shared_enable(struct clk_hw *hw)
+{
+     struct atlantis_clk_gate_shared *gate =
+             hw_to_atlantis_clk_gate_shared(hw);
+     bool need_enable;
+     u32 reg;
+
+     scoped_guard(spinlock_irqsave, gate->config.refcount_lock)
+     {
+             need_enable = (*gate->config.share_count)++ == 0;
+             if (need_enable) {
+                     regmap_read(gate->common.regmap,
+                                 gate->config.reg_offset, &reg);
+                     reg |= gate->config.enable;
+                     regmap_write(gate->common.regmap,
+                                  gate->config.reg_offset, reg);
+             }
+     }
+
+     if (need_enable) {
+             regmap_read(gate->common.regmap, gate->config.reg_offset, &reg);
+
+             if (!(reg & gate->config.enable)) {
+                     pr_warn("%s: gate enable %d failed to enable\n",
+                             clk_hw_get_name(hw), gate->config.enable);
+                     return -EIO;
+             }
+     }

Should this check be done within the scoped_guard?

The lock is used only for access to *gate->config.share_count. Since
we aren't reading that here, it isn't put inside the lock.

quoted

+static int atlantis_prcm_clocks_register(struct device *dev,
+                                      struct regmap *regmap,
+                                      const struct atlantis_prcm_data *data)
+{
+     struct clk_hw_onecell_data *clk_data;
+     int i, ret;
+     size_t num_clks = data->num;
+
+     clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, data->num),
+                             GFP_KERNEL);
+     if (!clk_data)
+             return -ENOMEM;
+
+     for (i = 0; i < data->num; i++) {
+             struct clk_hw *hw = data->hws[i];
+             const char *name = hw->init->name;
+             struct atlantis_clk_common *common =
+                     hw_to_atlantis_clk_common(hw);

You can join these two lines into one and you'll be at 83 characters.
checkpatch.pl now allows up to 100.

quoted

+             common->regmap = regmap;
+
+             ret = devm_clk_hw_register(dev, hw);
+
+             if (ret) {
+                     dev_err(dev, "Cannot register clock %d - %s\n", i,
+                             name);

This will be at 81 characters if the lines are joined.

I used clang-format to do the formatting here and it seems to have
picked a line length of 80? I can change this

However, bigger question is if this message should be dropped entirely? If
this condition occurs, an error is logged here, and a second message will be
logged in atlantis_prcm_probe() below.

quoted

+                     return ret;
+             }
+
+             clk_data->hws[common->clkid] = hw;
+     }
+
+     clk_data->num = num_clks;
+
+     ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
+     if (ret)
+             dev_err(dev, "failed to add clock hardware provider (%d)\n",
+                     ret);

Should this message also be dropped as well?

So you're suggesting that we just print a single error message instead
of multiple. I can change it to be like that.

Brian