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

[Lina Iyer <ilina@codeaurora.org>]

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