[U-Boot] [PATCH v3 01/12] dm: core: implement dev_map_phsymem()

[Simon Glass]

Hi,

On 4 May 2016 at 05:19, Vignesh R <vigneshr at ti.com> wrote:
> This API helps to map physical register addresss pace of device to
> virtual address space easily. Its just a wrapper around map_physmem()
> with MAP_NOCACHE flag.
>
> Signed-off-by: Vignesh R <vigneshr at ti.com>
> Suggested-by: Simon Glass <sjg at chromium.org>
> ---
>
> v3: Explicitly include header file

A few nits, sorry this review is late.

Please fix spelling of dev_map_phsymemin the commit subject.

>
> v2: New patch
>
>  drivers/core/device.c | 6 ++++++
>  include/dm/device.h   | 9 +++++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 1322991d6c7b..6b19b4b8c7a0 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -10,6 +10,7 @@
>   */
>
>  #include <common.h>
> +#include <asm/io.h>
>  #include <fdtdec.h>
>  #include <fdt_support.h>
>  #include <malloc.h>
> @@ -678,6 +679,11 @@ void *dev_get_addr_ptr(struct udevice *dev)
>         return (void *)(uintptr_t)dev_get_addr_index(dev, 0);
>  }
>
> +void *dev_map_physmem(struct udevice *dev, unsigned long size)
> +{
> +       return map_physmem(dev_get_addr(dev), size, MAP_NOCACHE);

What happens if dev_get_addr() returns 0. Does it work OK? If not,
please add a check for it.

> +}
> +
>  bool device_has_children(struct udevice *dev)
>  {
>         return !list_empty(&dev->child_head);
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 8970fc015c7e..5253b5410d9a 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -463,6 +463,15 @@ fdt_addr_t dev_get_addr(struct udevice *dev);
>   */
>  void *dev_get_addr_ptr(struct udevice *dev);
>
> +/* * dev_map_physmem() - Map bus memory into CPU space

This is fine, but please add more detail on following lines. You need
to mention that it reads the device address from the 'reg' property
using dev_get_addr().

> + *
> + * @dev: Pointer to device
> + * @size: size of the memory to map
> + *
> + * @return addr

Something like:

@return mapped address, or 0 if the device does not have an address or
the mapping failed

> + */
> +void *dev_map_physmem(struct udevice *dev, unsigned long size);
> +
>  /**
>   * dev_get_addr_index() - Get the indexed reg property of a device
>   *
> --
> 2.8.2
>

Regards,
Simon