Re: [PATCH v3 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs

[Lina Iyer]

Hi Stephen,

I will address all the comments in the next spin of the patch. Here are
some responses to the questions.

On Tue, Mar 06 2018 at 12:45 -0700, Stephen Boyd wrote:
> Quoting Lina Iyer (2018-03-02 08:43:08)
[...]
> +#include <linux/module.h>
>
> If the driver doesn't become tristate, this should become export.h
> instead of module.h
MODULE_DEVICE_TABLE seems to need this.

[...]
> > +
> > +static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int m, int n,
> > +                             u32 data)
> > +{
> > +       write_tcs_reg(drv, reg, m, n, data);
> > +       for (;;) {
> > +               if (data == read_tcs_reg(drv, reg, m, n))
> > +                       break;
> > +               udelay(1);
>
> Hopefully this never gets stuck. Add a timeout?
No, it wont. The read is to make sure that the write went through before
we exit this call.

[...]
> > +               list_del(&resp->list);
> > +               spin_unlock_irqrestore(&drv->drv_lock, flags);
> > +               free_response(resp);
>
> But all this function does is free the structure? Will it do more later?
Hopefully, I would like to use a pre-allocateed pool instead of alloc
and free.

> +               for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
> > +                       addr = read_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, j);
> > +                       for (k = 0; k < msg->num_payload; k++) {
> > +                               if (addr == msg->payload[k].addr)
> > +                                       return -EBUSY;
> > +                       }
> > +               }
> > +       }
>
> There isn't any way to do this in software only? Hopefully this isn't
> costly to read the TCS to see if something matches.
It is, but not too expensive. The alternatives involves more locking..

Thanks,
Lina