Re: [PATCH v7 2/4] soc: qcom: Add AOSS QMP driver

[Bjorn Andersson]

On Thu 23 May 09:38 PDT 2019, Doug Anderson wrote:

> Hi,
> 
> On Tue, Apr 30, 2019 at 9:38 PM Bjorn Andersson
> <bjorn.andersson@xxxxxxxxxx> wrote:
> >
> > +static int qmp_qdss_clk_prepare(struct clk_hw *hw)
> > +{
> > +       struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
> > +       char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 1}";
> 
> nit: "static const" the buf?  No need to copy it to the stack each
> time.  In qmp_qdss_clk_unprepare() too.
> 

Thanks, that makes sense.

> ...your string is also now fixed at 34 bytes big (including the '\0').
> Do we still need to send exactly 96 bytes, or can we dumb this down to
> 36?  We'll get a compile error if we overflow, right?  If this truly
> needs to be exactly 96 bytes maybe qmp_send()'s error checks should
> check for things being exactly 96 bytes instead of checking for > and
> % 4.
> 

I double checked with my contacts and the only requirement here is that
memory has to be word-accessed, so I'll figure out a sane way to write
this.

> 
> > +static int qmp_qdss_clk_add(struct qmp *qmp)
> > +{
> > +       struct clk_init_data qdss_init = {
> > +               .ops = &qmp_qdss_clk_ops,
> > +               .name = "qdss",
> > +       };
> 
> Can't qdss_init be "static const"?  That had the advantage of not
> needing to construct it on the stack and also of it having a longer
> lifetime.  It looks like clk_register() stores the "hw" pointer in its
> structure and the "hw" structure will have a pointer here.  While I
> can believe that it never looks at it again, it's nice if that pointer
> doesn't point somewhere on an old stack.
> 

The purpose here was for clk_hw_register() to consume it and never look
back, but I agree that it's a bit fragile. I'll review Stephen's
proposed patch.

> I suppose we could go the other way and try to mark more stuff in this
> module as __init and __initdata, but even then at least the pointer
> won't be onto a stack.  ;-)
> 
> 
> > +       int ret;
> > +
> > +       qmp->qdss_clk.init = &qdss_init;
> > +       ret = clk_hw_register(qmp->dev, &qmp->qdss_clk);
> > +       if (ret < 0) {
> > +               dev_err(qmp->dev, "failed to register qdss clock\n");
> > +               return ret;
> > +       }
> > +
> > +       return of_clk_add_hw_provider(qmp->dev->of_node, of_clk_hw_simple_get,
> > +                                     &qmp->qdss_clk);
> 
> devm_clk_hw_register() and devm_of_clk_add_hw_provider()?  If you're
> worried about ordering you could always throw in
> devm_add_action_or_reset() to handle the qmp_pd_remove(), qmp_close()
> and mbox_free_channel().
> 
> ...with that you could fully get rid of qmp_remove() and also your
> setting of drvdata.
> 

Yeah, I was worried about qmp_close() before unregistering the clock.
I'll take another look, will at least have to fix the error handling on
of_clk_add_hw_provider()

> 
> > +static void qmp_pd_remove(struct qmp *qmp)
> > +{
> > +       struct genpd_onecell_data *data = &qmp->pd_data;
> > +       struct device *dev = qmp->dev;
> > +       int i;
> > +
> > +       of_genpd_del_provider(dev->of_node);
> > +
> > +       for (i = 0; i < data->num_domains; i++)
> > +               pm_genpd_remove(data->domains[i]);
> 
> Still feels like the above loop would be better as:
>   for (i = data->num_domains - 1; i >= 0; i--)
> 

To me this carries a message that the removal order is significant,
which I'm unable to convince myself that it is.

> 
> (BTW: any way you could add me to the CC list for future patches so I
> notice them earlier?)
> 

Yes of course, thanks for your review.

Regards,
Bjorn