Re: [PATCH v6 3/3] clk: tenstorrent: Add Atlantis clock controller driver

[Brian Masney]

Hi Anirudh,

On Tue, Feb 17, 2026 at 05:12:38PM -0600, Anirudh Srinivasan wrote:
> On Tue, Feb 17, 2026 at 10:14 AM Brian Masney <bmasney@xxxxxxxxxx> wrote:
> > On Mon, Feb 16, 2026 at 04:16:34PM -0600, Anirudh Srinivasan wrote:
> > > 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.
> 
> > > +
> > > +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).

The code currently has:

struct atlantis_clk_gate_shared_config {
	...
	unsigned int *share_count;
}

That pointer is dereferenced like this in several places:

    need_enable = (*gate->config.share_count)++ == 0;

I don't see why the pointer is needed. Can you drop the pointer
and the dereference like this?

struct atlantis_clk_gate_shared_config {
	...
	unsigned int share_count;
}

    need_enable = gate->config.share_count++ == 0;

[ snip ]

> > 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.
> >
> > > +                     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.

Yes, and sounds good!

Brian