Hello Srinivas and Charles,
On Thu, Nov 23, 2017 at 10:07:03AM +0000, Charles Keepax wrote:
On Wed, Nov 15, 2017 at 02:10:41PM +0000, srinivas.kandagatla@linaro.org wrote:
From: Sagar Dharia sdharia@codeaurora.org
This controller driver programs manager, interface, and framer devices for Qualcomm's slimbus HW block. Manager component currently implements logical address setting, and messaging interface. Interface device reports bus synchronization information, and framer device clocks the bus from the time it's woken up, until clock-pause is executed by the manager device.
Signed-off-by: Sagar Dharia sdharia@codeaurora.org Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org
[...]
+static void qcom_slim_rxwq(struct work_struct *work) +{
- u8 buf[SLIM_MSGQ_BUF_LEN];
- u8 mc, mt, len;
- int i, ret;
- struct qcom_slim_ctrl *ctrl = container_of(work, struct qcom_slim_ctrl,
wd);- while ((slim_get_current_rxbuf(ctrl, buf)) != -ENODATA) {
len = SLIM_HEADER_GET_RL(buf[0]);mt = SLIM_HEADER_GET_MT(buf[0]);mc = SLIM_HEADER_GET_MC(buf[1]);if (mt == SLIM_MSG_MT_CORE &&mc == SLIM_MSG_MC_REPORT_PRESENT) {u8 laddr;struct slim_eaddr ea;u8 e_addr[6];for (i = 0; i < 6; i++)e_addr[i] = buf[7-i];ea.manf_id = (u16)(e_addr[5] << 8) | e_addr[4];ea.prod_code = (u16)(e_addr[3] << 8) | e_addr[2];ea.dev_index = e_addr[1];ea.instance = e_addr[0];If we are just bitshifting this out of the bytes does it really make it much more clear to reverse the byte order first? Feels like you might as well shift it out of buf directly.
In any case, there is a predefined function to make this code a little nicer in <asm/byteorder.h>:
le16_to_cpu(x): Converts the 16-bit little endian value x to CPU-endian le16_to_cpup(p): Converts the 16-bit little endian value pointed to by p to CPU-endian
If you use le16_to_cpup, you need to cast your pointer to __le16 *:
ea.manf_id = le16_to_cpup((__le16 *)&e_addr[4]);
Like Charles, I don't quite see the point of the for loop that fills e_addr. I guess it did effectively a byteswap, so the original code, that assumed little-endian, could simply dereference a u16 *. This does not make a lot of sense anymore, once you use properly (CPU-)endian- independent code. (Of course, you'll need to replace le16 with be16 if you drop that loop.)
Thanks, Jonathan Neuschäfer