Re: [U-Boot] [PATCH 1/9] disk/part.c: Expose a list of available block drivers

[Alexander Graf]

On 15.01.16 01:46, Simon Glass wrote:
> Hi Alex,
> 
> On 14 January 2016 at 16:33, Alexander Graf  wrote:
>>
>>
>> On 15.01.16 00:11, Simon Glass wrote:
>>> Hi Alexander,
>>>
>>> On 22 December 2015 at 06:57, Alexander Graf  wrote:
 We have a pretty nice and generic interface to ask for a specific block
 device. However, that one is still based around the magic notion that
 we know the driver name.

 In order to be able to write fully generic disk access code, expose a list
 of all available block drivers.

 Signed-off-by: Alexander Graf 
 ---
  disk/part.c| 25 +
  include/part.h |  2 ++
  2 files changed, 27 insertions(+)

 diff --git a/disk/part.c b/disk/part.c
 index 909712e..5bc64c7 100644
 --- a/disk/part.c
 +++ b/disk/part.c
 @@ -26,6 +26,31 @@ struct block_drvr {
 int (*select_hwpart)(int dev_num, int hwpart);
  };

 +const char *available_block_drvrs[] = {
 +#if defined(CONFIG_CMD_IDE)
 +   "ide",
 +#endif
 +#if defined(CONFIG_CMD_SATA)
 +   "sata",
 +#endif
 +#if defined(CONFIG_CMD_SCSI)
 +   "scsi",
 +#endif
 +#if defined(CONFIG_CMD_USB) && defined(CONFIG_USB_STORAGE)
 +   "usb",
 +#endif
 +#if defined(CONFIG_MMC)
 +   "mmc",
 +#endif
 +#if defined(CONFIG_SYSTEMACE)
 +   "ace",
 +#endif
 +#if defined(CONFIG_SANDBOX)
 +   "host",
 +#endif
 +   NULL,
 +};
>>>
>>> You seem to be duplicating block_drvr[]. Can we not just use that?
>>
>> It would mean that we'd have to make it public then - to me it looked
>> like people kept it static for a reason.
>>
>> However if everyone's happy if I expose it (and the struct definition
>> behind it), I'm certainly more than happy to move to that one instead :).
> 
> But you have created a public one which is a copy of part of it, so it
> doesn't seem like much of a benefit :-)
> 
> How about adding a function to return the device name of a member?

I wouldn't know the struct size, so I couldn't walk the array, right? :)

> Then you should be able to avoid copying it, and avoid making it
> global.
> 
> I'm very interested in your series, and hope to review the rest of it
> (and try it out) soon!

Wait for v2, I'm just polishing it up :). It fixes a good number of
issues on real hardware. I have the code running well on BBB and HiKey now.


Alex
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/9] disk/part.c: Expose a list of available block drivers

[Simon Glass]

Hi Alex,

On 14 January 2016 at 16:33, Alexander Graf  wrote:
>
>
> On 15.01.16 00:11, Simon Glass wrote:
>> Hi Alexander,
>>
>> On 22 December 2015 at 06:57, Alexander Graf  wrote:
>>> We have a pretty nice and generic interface to ask for a specific block
>>> device. However, that one is still based around the magic notion that
>>> we know the driver name.
>>>
>>> In order to be able to write fully generic disk access code, expose a list
>>> of all available block drivers.
>>>
>>> Signed-off-by: Alexander Graf 
>>> ---
>>>  disk/part.c| 25 +
>>>  include/part.h |  2 ++
>>>  2 files changed, 27 insertions(+)
>>>
>>> diff --git a/disk/part.c b/disk/part.c
>>> index 909712e..5bc64c7 100644
>>> --- a/disk/part.c
>>> +++ b/disk/part.c
>>> @@ -26,6 +26,31 @@ struct block_drvr {
>>> int (*select_hwpart)(int dev_num, int hwpart);
>>>  };
>>>
>>> +const char *available_block_drvrs[] = {
>>> +#if defined(CONFIG_CMD_IDE)
>>> +   "ide",
>>> +#endif
>>> +#if defined(CONFIG_CMD_SATA)
>>> +   "sata",
>>> +#endif
>>> +#if defined(CONFIG_CMD_SCSI)
>>> +   "scsi",
>>> +#endif
>>> +#if defined(CONFIG_CMD_USB) && defined(CONFIG_USB_STORAGE)
>>> +   "usb",
>>> +#endif
>>> +#if defined(CONFIG_MMC)
>>> +   "mmc",
>>> +#endif
>>> +#if defined(CONFIG_SYSTEMACE)
>>> +   "ace",
>>> +#endif
>>> +#if defined(CONFIG_SANDBOX)
>>> +   "host",
>>> +#endif
>>> +   NULL,
>>> +};
>>
>> You seem to be duplicating block_drvr[]. Can we not just use that?
>
> It would mean that we'd have to make it public then - to me it looked
> like people kept it static for a reason.
>
> However if everyone's happy if I expose it (and the struct definition
> behind it), I'm certainly more than happy to move to that one instead :).

But you have created a public one which is a copy of part of it, so it
doesn't seem like much of a benefit :-)

How about adding a function to return the device name of a member?
Then you should be able to avoid copying it, and avoid making it
global.

I'm very interested in your series, and hope to review the rest of it
(and try it out) soon!

>
>
> Alex
>
>>
>>> +
>>>  static const struct block_drvr block_drvr[] = {
>>>  #if defined(CONFIG_CMD_IDE)
>>> { .name = "ide", .get_dev = ide_get_dev, },
>>> diff --git a/include/part.h b/include/part.h
>>> index 720a867..dc2a78b 100644
>>> --- a/include/part.h
>>> +++ b/include/part.h
>>> @@ -122,6 +122,8 @@ int get_device(const char *ifname, const char *dev_str,
>>>  int get_device_and_partition(const char *ifname, const char *dev_part_str,
>>>  block_dev_desc_t **dev_desc,
>>>  disk_partition_t *info, int allow_whole_dev);
>>> +
>>> +extern const char *available_block_drvrs[];
>>>  #else
>>>  static inline block_dev_desc_t *get_dev(const char *ifname, int dev)
>>>  { return NULL; }
>>> --
>>> 2.1.4
>> Regards,
>> Simon
>>

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/9] disk/part.c: Expose a list of available block drivers

[Alexander Graf]

On 15.01.16 00:11, Simon Glass wrote:
> Hi Alexander,
> 
> On 22 December 2015 at 06:57, Alexander Graf  wrote:
>> We have a pretty nice and generic interface to ask for a specific block
>> device. However, that one is still based around the magic notion that
>> we know the driver name.
>>
>> In order to be able to write fully generic disk access code, expose a list
>> of all available block drivers.
>>
>> Signed-off-by: Alexander Graf 
>> ---
>>  disk/part.c| 25 +
>>  include/part.h |  2 ++
>>  2 files changed, 27 insertions(+)
>>
>> diff --git a/disk/part.c b/disk/part.c
>> index 909712e..5bc64c7 100644
>> --- a/disk/part.c
>> +++ b/disk/part.c
>> @@ -26,6 +26,31 @@ struct block_drvr {
>> int (*select_hwpart)(int dev_num, int hwpart);
>>  };
>>
>> +const char *available_block_drvrs[] = {
>> +#if defined(CONFIG_CMD_IDE)
>> +   "ide",
>> +#endif
>> +#if defined(CONFIG_CMD_SATA)
>> +   "sata",
>> +#endif
>> +#if defined(CONFIG_CMD_SCSI)
>> +   "scsi",
>> +#endif
>> +#if defined(CONFIG_CMD_USB) && defined(CONFIG_USB_STORAGE)
>> +   "usb",
>> +#endif
>> +#if defined(CONFIG_MMC)
>> +   "mmc",
>> +#endif
>> +#if defined(CONFIG_SYSTEMACE)
>> +   "ace",
>> +#endif
>> +#if defined(CONFIG_SANDBOX)
>> +   "host",
>> +#endif
>> +   NULL,
>> +};
> 
> You seem to be duplicating block_drvr[]. Can we not just use that?

It would mean that we'd have to make it public then - to me it looked
like people kept it static for a reason.

However if everyone's happy if I expose it (and the struct definition
behind it), I'm certainly more than happy to move to that one instead :).


Alex

> 
>> +
>>  static const struct block_drvr block_drvr[] = {
>>  #if defined(CONFIG_CMD_IDE)
>> { .name = "ide", .get_dev = ide_get_dev, },
>> diff --git a/include/part.h b/include/part.h
>> index 720a867..dc2a78b 100644
>> --- a/include/part.h
>> +++ b/include/part.h
>> @@ -122,6 +122,8 @@ int get_device(const char *ifname, const char *dev_str,
>>  int get_device_and_partition(const char *ifname, const char *dev_part_str,
>>  block_dev_desc_t **dev_desc,
>>  disk_partition_t *info, int allow_whole_dev);
>> +
>> +extern const char *available_block_drvrs[];
>>  #else
>>  static inline block_dev_desc_t *get_dev(const char *ifname, int dev)
>>  { return NULL; }
>> --
>> 2.1.4
> Regards,
> Simon
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/9] disk/part.c: Expose a list of available block drivers

[Simon Glass]

Hi Alexander,

On 22 December 2015 at 06:57, Alexander Graf  wrote:
> We have a pretty nice and generic interface to ask for a specific block
> device. However, that one is still based around the magic notion that
> we know the driver name.
>
> In order to be able to write fully generic disk access code, expose a list
> of all available block drivers.
>
> Signed-off-by: Alexander Graf 
> ---
>  disk/part.c| 25 +
>  include/part.h |  2 ++
>  2 files changed, 27 insertions(+)
>
> diff --git a/disk/part.c b/disk/part.c
> index 909712e..5bc64c7 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -26,6 +26,31 @@ struct block_drvr {
> int (*select_hwpart)(int dev_num, int hwpart);
>  };
>
> +const char *available_block_drvrs[] = {
> +#if defined(CONFIG_CMD_IDE)
> +   "ide",
> +#endif
> +#if defined(CONFIG_CMD_SATA)
> +   "sata",
> +#endif
> +#if defined(CONFIG_CMD_SCSI)
> +   "scsi",
> +#endif
> +#if defined(CONFIG_CMD_USB) && defined(CONFIG_USB_STORAGE)
> +   "usb",
> +#endif
> +#if defined(CONFIG_MMC)
> +   "mmc",
> +#endif
> +#if defined(CONFIG_SYSTEMACE)
> +   "ace",
> +#endif
> +#if defined(CONFIG_SANDBOX)
> +   "host",
> +#endif
> +   NULL,
> +};

You seem to be duplicating block_drvr[]. Can we not just use that?

> +
>  static const struct block_drvr block_drvr[] = {
>  #if defined(CONFIG_CMD_IDE)
> { .name = "ide", .get_dev = ide_get_dev, },
> diff --git a/include/part.h b/include/part.h
> index 720a867..dc2a78b 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -122,6 +122,8 @@ int get_device(const char *ifname, const char *dev_str,
>  int get_device_and_partition(const char *ifname, const char *dev_part_str,
>  block_dev_desc_t **dev_desc,
>  disk_partition_t *info, int allow_whole_dev);
> +
> +extern const char *available_block_drvrs[];
>  #else
>  static inline block_dev_desc_t *get_dev(const char *ifname, int dev)
>  { return NULL; }
> --
> 2.1.4
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/9] disk/part.c: Expose a list of available block drivers

[Tom Rini]

On Tue, Dec 22, 2015 at 02:57:48PM +0100, Alexander Graf wrote:

> We have a pretty nice and generic interface to ask for a specific block
> device. However, that one is still based around the magic notion that
> we know the driver name.
> 
> In order to be able to write fully generic disk access code, expose a list
> of all available block drivers.
> 
> Signed-off-by: Alexander Graf 

Reviewed-by: Tom Rini 

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 1/9] disk/part.c: Expose a list of available block drivers

[Alexander Graf]

We have a pretty nice and generic interface to ask for a specific block
device. However, that one is still based around the magic notion that
we know the driver name.

In order to be able to write fully generic disk access code, expose a list
of all available block drivers.

Signed-off-by: Alexander Graf 
---
 disk/part.c| 25 +
 include/part.h |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/disk/part.c b/disk/part.c
index 909712e..5bc64c7 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -26,6 +26,31 @@ struct block_drvr {
int (*select_hwpart)(int dev_num, int hwpart);
 };
 
+const char *available_block_drvrs[] = {
+#if defined(CONFIG_CMD_IDE)
+   "ide",
+#endif
+#if defined(CONFIG_CMD_SATA)
+   "sata",
+#endif
+#if defined(CONFIG_CMD_SCSI)
+   "scsi",
+#endif
+#if defined(CONFIG_CMD_USB) && defined(CONFIG_USB_STORAGE)
+   "usb",
+#endif
+#if defined(CONFIG_MMC)
+   "mmc",
+#endif
+#if defined(CONFIG_SYSTEMACE)
+   "ace",
+#endif
+#if defined(CONFIG_SANDBOX)
+   "host",
+#endif
+   NULL,
+};
+
 static const struct block_drvr block_drvr[] = {
 #if defined(CONFIG_CMD_IDE)
{ .name = "ide", .get_dev = ide_get_dev, },
diff --git a/include/part.h b/include/part.h
index 720a867..dc2a78b 100644
--- a/include/part.h
+++ b/include/part.h
@@ -122,6 +122,8 @@ int get_device(const char *ifname, const char *dev_str,
 int get_device_and_partition(const char *ifname, const char *dev_part_str,
 block_dev_desc_t **dev_desc,
 disk_partition_t *info, int allow_whole_dev);
+
+extern const char *available_block_drvrs[];
 #else
 static inline block_dev_desc_t *get_dev(const char *ifname, int dev)
 { return NULL; }
-- 
2.1.4

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot