On 09/19/2016 04:41 PM, Sudeep Holla wrote:

Hi Neil,

On 07/09/16 16:34, Neil Armstrong wrote:

quoted

Add indirection table to permit multiple command values for legacy support.

I wrote the most of the patch and you changed the author too ;)

Sorry, forgot this ! v4 will have it !

quoted

Signed-off-by: Neil Armstrong <redacted>
---
 drivers/firmware/arm_scpi.c | 145 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 127 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 4388937..9a87687 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c

[..]

quoted

@@ -161,6 +194,7 @@ struct scpi_drvinfo {
     u32 protocol_version;
     u32 firmware_version;
     int num_chans;
+    int *scpi_cmds;
     atomic_t next_chan;
     struct scpi_ops *scpi_ops;
     struct scpi_chan *channels;

@@ -390,6 +424,19 @@ static u32 scpi_get_version(void)
     return scpi_info->protocol_version;
 }

+static inline int check_cmd(unsigned int offset)
+{
+    if (offset >= CMD_MAX_COUNT ||

If we call scpi_send_message internally(as it's static) why is this
check needed ?

quoted

+        !scpi_info ||
+        !scpi_info->scpi_cmds)

Will be even reach to this point if above is true ?

quoted

+        return -EINVAL;
+
+    if (scpi_info->scpi_cmds[offset] < 0)
+        return -EOPNOTSUPP;

IMO just above couple of lines in the beginning of scpi_send_message
will suffice. You can just add this to my original patch.

Will do.

quoted

 static int
 scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
 {

@@ -397,8 +444,13 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
     struct clk_get_info clk;
     __le16 le_clk_id = cpu_to_le16(clk_id);

-    ret = scpi_send_message(SCPI_CMD_GET_CLOCK_INFO, &le_clk_id,
-                sizeof(le_clk_id), &clk, sizeof(clk));
+    ret = check_cmd(CMD_GET_CLOCK_INFO);
+    if (ret)
+        return ret;
+

It's totally unnecessary to add check in each and every function calling
scpi_send_message, why not add it to scpi_send_message instead.

This was my first thought, I should have stayed at this !

Thanks,
Neil