[U-Boot] [PATCH v2 3/7] drivers: block: disk-uclass: implement scsi_init()

[Simon Glass]

Hi Mugunthan and Bin,

On 14 February 2016 at 20:03, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Sun, Feb 7, 2016 at 4:29 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 3 February 2016 at 04:59, Mugunthan V N <mugunthanvnm at ti.com> wrote:
>>>
>>> Implement scsi_init() api to probe driver model based sata
>>> devices.
>>>
>>> Signed-off-by: Mugunthan V N <mugunthanvnm at ti.com>
>>> ---
>>>  drivers/block/disk-uclass.c | 39 +++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 39 insertions(+)
>>
>> This patch seems odd to me. I would hope that scsi_init() would go
>> away with driver model and it would happen as part of the controller
>> probe. But of course we cannot probe SCSI from UCLASS_DISK. I think
>> the uclass 'disk' is too broad to be useful.
>>
>
> I agree. I raised similar comment before that this just looks a place
> holder for the SCSI stuff.
>
>> So I am wondering whether the decision to use UCLASS_DISK instead of
>> UCLASS_AHCI was a mistake.
>>
>> Perhaps instead we should have:
>>
>> UCLASS_AHCI
>> UCLASS_SCSI
>> UCLASS_MMC
>> etc...
>>
>
> I still think UCLASS_AHCI is not a good name. Maybe UCLASS_ATA as we
> are talking about protocols here (SCSI, MMC).

UCLASS_ATA seems confusing. How would we distinguish IDE and SATA?

BTW since your email I have sent a patch to implement block devices.
>From what I can tell the above approach will work well. I think our
uclasses should mirror the current IF_TYPE values:

/* Interface types: */
#define IF_TYPE_UNKNOWN 0
#define IF_TYPE_IDE 1
#define IF_TYPE_SCSI 2
#define IF_TYPE_ATAPI 3
#define IF_TYPE_USB 4
#define IF_TYPE_DOC 5
#define IF_TYPE_MMC 6
#define IF_TYPE_SD 7
#define IF_TYPE_SATA 8
#define IF_TYPE_HOST 9

>
>> and each of these devices can have a UCLASS_BLK under them (the block device).
>>
>> Possibly we could even have a dummy UCLASS_DISK device under each of
>> the above, but I'm not sure that is useful.
>>
>> What do you think?
>>
>
> [snip]
>
> Regards,
> Bin

Since this patch is presumably destined for the current release and we
don't intend to change UCLASS_DISK for that release, I think this
patch can go in as is. We can fix it up later. Also note that
scsi_get_device() is not the same as scsi_get_dev().

It would be better to use uclass_get_device() though.

Reviewed-by: Simon Glass <sjg at chromium.org>

Regards,
Simon