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

[Simon Glass]

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?

> +{
> +       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!

> +                       &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()?

> +       }
> +       dev_desc->lba = capacity;
> +       dev_desc->blksz = blksz;
> +       dev_desc->log2blksz = LOG2(dev_desc->blksz);
> +       dev_desc->type = perq;
> +       part_init(&dev_desc[0]);
> +removable:
> +       return 0;
> +}
> +

[...]

Regards,
Simon