Hi Mugunthan and Bin,
On 14 February 2016 at 20:03, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Sun, Feb 7, 2016 at 4:29 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 3 February 2016 at 04:59, Mugunthan V N mugunthanvnm@ti.com wrote:
Implement scsi_init() api to probe driver model based sata devices.
Signed-off-by: Mugunthan V N mugunthanvnm@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@chromium.org
Regards, Simon