[U-Boot] [RFC PATCH 2/5] scsi: Extract device detection algorithm

[Michal Simek]

On 19.11.2016 14:48, Simon Glass wrote:
> Hi Michal,
> 
> On 18 November 2016 at 08:44, Michal Simek <michal.simek at xilinx.com> wrote:
>> The patch enables running detection algorithm on block device
>> description structure.
>>
>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>> ---
>>
>>  common/scsi.c | 134 ++++++++++++++++++++++++++++++++--------------------------
>>  1 file changed, 73 insertions(+), 61 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> Please see below.
> 
>>
>> diff --git a/common/scsi.c b/common/scsi.c
>> index 0bce91dfa099..a02c7a505639 100644
>> --- a/common/scsi.c
>> +++ b/common/scsi.c
>> @@ -480,15 +480,79 @@ static void scsi_init_dev_desc(struct blk_desc *dev_desc, int devnum)
>>  #endif
>>  }
>>
>> +static int scsi_detect_dev(ccb *pccb, struct blk_desc *dev_desc, int lun)
> 
> Can you comment this function?

will do.

> 
>> +{
>> +       unsigned char perq, modi;
>> +       lbaint_t capacity;
>> +       unsigned long blksz;
>> +
>> +       pccb->lun = lun;
>> +       pccb->pdata = (unsigned char *)&tempbuff;
>> +       pccb->datalen = 512;
>> +       scsi_setup_inquiry(pccb);
>> +       if (scsi_exec(pccb) != true) {
>> +               if (pccb->contr_stat == SCSI_SEL_TIME_OUT) {
>> +                       /*
>> +                         * selection timeout => assuming no
>> +                         * device present
>> +                         */
>> +                       debug("Selection timeout ID %d\n",
>> +                             pccb->target);
>> +                       return -ETIMEDOUT;
>> +               }
>> +               scsi_print_error(pccb);
>> +               return -ENODEV;
>> +       }
>> +       perq = tempbuff[0];
>> +       modi = tempbuff[1];
>> +       if ((perq & 0x1f) == 0x1f)
>> +               return -ENODEV; /* skip unknown devices */
>> +       if ((modi & 0x80) == 0x80) /* drive is removable */
>> +               dev_desc->removable = true;
>> +       /* get info for this device */
>> +       scsi_ident_cpy((unsigned char *)&dev_desc
>> +                               [0].vendor[0],
>> +                       &tempbuff[8], 8);
>> +       scsi_ident_cpy((unsigned char *)&dev_desc
>> +                               [0].product[0],
> 
> Can you use more of the line here? You have 80 columns!

will fix.

> 
>> +                       &tempbuff[16], 16);
>> +       scsi_ident_cpy((unsigned char *)&dev_desc
>> +                               [0].revision[0],
>> +                       &tempbuff[32], 4);
>> +       dev_desc->target = pccb->target;
>> +       dev_desc->lun = pccb->lun;
>> +
>> +       pccb->datalen = 0;
>> +       scsi_setup_test_unit_ready(pccb);
>> +       if (scsi_exec(pccb) != true) {
>> +               if (dev_desc->removable) {
>> +                       dev_desc->type = perq;
>> +                       goto removable;
>> +               }
>> +               scsi_print_error(pccb);
>> +               return -EINVAL;
>> +       }
>> +       if (scsi_read_capacity(pccb, &capacity, &blksz)) {
>> +               scsi_print_error(pccb);
>> +               return -EINVAL;
> 
> Should you not return the error from scsi_read_capacity()?
> 

scsi_read_capacity returns 1 or 0 error value which is just useless.

Thanks,
Michal