On 25 January 2016 at 12:19, Timur Tabi <timur@xxxxxxxxxxxxxx> wrote:
> On Fri, Jan 22, 2016 at 7:07 PM, Prashanth Prakash
> <pprakash@xxxxxxxxxxxxxx> wrote:
>> static void __iomem *pcc_comm_addr;
>> static u64 comm_base_addr;
>> static int pcc_subspace_idx = -1;
>> -static u16 pcc_cmd_delay;
>> static bool pcc_channel_acquired;
>> +static ktime_t deadline;
>>
>> /*
>> * Arbitrary Retries in case the remote processor is slow to respond
>> - * to PCC commands.
>> + * to PCC commands. Keeping it high enough to cover emulators where
>> + * the processors run painfully slow.
>> */
>> #define NUM_RETRIES 500
>>
>> +static int check_pcc_chan(void)
>> +{
>> + int result = -EIO;
>> + struct acpi_pcct_shared_memory *generic_comm_base =
>> + (struct acpi_pcct_shared_memory *) pcc_comm_addr;
>
> You're typecasting away the __iomem, and you don't need a typecast for
> void pointers. This should be:
>
> struct acpi_pcct_shared_memory __iomem *generic_comm_base = pcc_comm_addr;
>
>> + ktime_t next_deadline = ktime_add(ktime_get(), deadline);
>> + int retries = 0;
>> +
>> + /* Retry in case the remote processor was too slow to catch up. */
>> + while (!ktime_after(ktime_get(), next_deadline)) {
>> + if (readw_relaxed(&generic_comm_base->status) & PCC_CMD_COMPLETE) {
>> + result = 0;
>> + break;
>> + }
>> + /*
>> + * Reducing the bus traffic in case this loop takes longer than
>> + * a few retries.
>> + */
>> + udelay(3);
>> + retries++;
>
> You don't actually use 'retries' in this function, so just delete it.
> Besides, you should be using readl_poll_timeout, instead of this loop.
> It handles the udelay and the ktime for you.
>
>> + }
>> +
>> + return result;
>> +}
>> +
>> static int send_pcc_cmd(u16 cmd)
>> {
>> - int retries, result = -EIO;
>> - struct acpi_pcct_hw_reduced *pcct_ss = pcc_channel->con_priv;
>> + int ret = -EIO;
>
> Please be consistent in your return variable names. Sometimes you use
> 'result', sometimes 'ret'. Pick one (my preference is 'ret') and use
> it exclusively.
>
>> struct acpi_pcct_shared_memory *generic_comm_base =
>> (struct acpi_pcct_shared_memory *) pcc_comm_addr;
>
> You have the same problem with __iomem here.
>
> And don't forget to CC: me and Ashwin on all future versions of this patchset.
Prashanth has been doing the right thing all along. I was CC'd on all
his patchwork, but you changed it (and removed me) while replying.
Unless Rafael thinks otherwise, I see no major issues in V2, so there
is no need for a respin.
Ashwin
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
