> + lrbp->command_type = hba->ufs_version == UFSHCI_VERSION_10 ||
> + hba->ufs_version == UFSHCI_VERSION_11 ?
> + UTP_CMD_TYPE_DEV_MANAGE :
> + UTP_CMD_TYPE_UFS_STORAGE;
I think a good old if/self or even a switch statement would be more
descriptive here:
switch (hba->ufs_version) {
case UFSHCI_VERSION_10:
case UFSHCI_VERSION_11:
lrbp->command_type = UTP_CMD_TYPE_DEV_MANAGE;
break;
default:
lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
break;
}
> + /* just copy the upiu request as it is */
> + memcpy(lrbp->ucd_req_ptr, req_upiu, GENERAL_UPIU_REQUEST_SIZE);
> +
> + if (desc_buff && rw == WRITE) {
> + descp = (u8 *)lrbp->ucd_req_ptr + GENERAL_UPIU_REQUEST_SIZE;
> + memcpy(descp, desc_buff, *buff_len);
> + *buff_len = 0;
> + }
The GENERAL_UPIU_REQUEST_SIZE usage here looks odd. Shouldn't we use
data structure sizes an pointer arithmetics? E.g.
memcpy(lrbp->ucd_req_ptr, req_upiu, sizeof(*lrbp->ucd_req_ptr));
if (desc_buff && rw == WRITE) {
memcpy(lrbp->ucd_req_ptr + 1, desc_buff, *buff_len);
*buff_len = 0;
}
preferably with a comment explaining the weird overallocation of
ucd_req_ptr over the declared data type in the existing driver.
> +
> + hba->dev_cmd.complete = &wait;
> +
> + /* Make sure descriptors are ready before ringing the doorbell */
> + wmb();
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + ufshcd_send_command(hba, tag);
I think the above wmb() is taken care of by the writel() inside
ufshcd_send_command.
> + /* just copy the upiu response as it is */
> + memcpy(rsp_upiu, lrbp->ucd_rsp_ptr, GENERAL_UPIU_REQUEST_SIZE);
sizeof again.
