From mboxrd@z Thu Jan  1 00:00:00 1970
From: Lina Iyer <ilina@codeaurora.org>
Subject: Re: [PATCH v3 01/10] drivers: qcom: rpmh-rsc: add RPMH controller
 for QCOM SoCs
Date: Fri, 9 Mar 2018 14:33:27 -0700
Message-ID: <20180309213327.GA2806@codeaurora.org>
References: <20180302164317.10554-1-ilina@codeaurora.org>
 <20180302164317.10554-2-ilina@codeaurora.org>
 <152036550693.218381.11708415467260101936@swboyd.mtv.corp.google.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Return-path: <linux-kernel-owner@vger.kernel.org>
Content-Disposition: inline
In-Reply-To: <152036550693.218381.11708415467260101936@swboyd.mtv.corp.google.com>
Sender: linux-kernel-owner@vger.kernel.org
To: Stephen Boyd <swboyd@chromium.org>
Cc: andy.gross@linaro.org, david.brown@linaro.org, linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, rnayak@codeaurora.org, bjorn.andersson@linaro.org, linux-kernel@vger.kernel.org
List-Id: linux-arm-msm@vger.kernel.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