[1/8] dm: add dev_read_u32()

Message ID 1514566812-16781-2-git-send-email-yamada.masahiro@socionext.com
State Accepted
Commit 3ab48f62230b6753bf5535655c40e3b975825bd9
Headers show
Series
  • mmc: some fixes for core and add HS200 support for sdhci-cadence
Related show

Commit Message

Masahiro Yamada Dec. 29, 2017, 5 p.m.
dev_read_u32_default() always returns something even when the property
is missing.  So, it is impossible to do nothing in the case.  One
solution is to use ofnode_read_u32() instead, but adding dev_read_u32()
will be helpful.

BTW, Linux has an equvalent function, device_property_read_u32();
it is clearer that it reads a property.  I cannot understand the
behavior of dev_read_u32() from its name.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

BTW, I do not understand why we want *_default helpers.
It is pretty easy to do equivalent without _default.

  priv->val = default;    /* default in case property is missing */
  dev_read_u32(dev, "foo-property", &priv->val);

On the other hand, if we want to do nothing for missing property,
we will end up with clumsy code.

  /* we do not want to overwrite priv->val if property is missing */
  if (dev_read_bool(dev, "foo-property") {
         /* default value '0' will not be used anyway */
         priv->val = dev_read_u32_default(dev, "foo-property", 0);
  }

One more BTW, it was just painful to write equivalent code twice
for CONFIG_DM_DEV_READ_INLINE.


 drivers/core/read.c |  5 +++++
 include/dm/read.h   | 16 ++++++++++++++++
 2 files changed, 21 insertions(+)

Comments

Simon Glass Jan. 8, 2018, 4:38 a.m. | #1
Hi Masahiro,

On 29 December 2017 at 10:00, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> dev_read_u32_default() always returns something even when the property
> is missing.  So, it is impossible to do nothing in the case.  One
> solution is to use ofnode_read_u32() instead, but adding dev_read_u32()
> will be helpful.
>
> BTW, Linux has an equvalent function, device_property_read_u32();
> it is clearer that it reads a property.  I cannot understand the
> behavior of dev_read_u32() from its name.

I decided that 'property' was an unnecessary word since there is
nothing else you can ready a value from in the DT.

>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> BTW, I do not understand why we want *_default helpers.
> It is pretty easy to do equivalent without _default.
>
>   priv->val = default;    /* default in case property is missing */
>   dev_read_u32(dev, "foo-property", &priv->val);
>

Your example explains it. Instead, we can do:

priv->val = dev_read_u32_default(dev,"foo-property", default);

which is simpler to understand and does not need a comment.

> On the other hand, if we want to do nothing for missing property,
> we will end up with clumsy code.
>
>   /* we do not want to overwrite priv->val if property is missing */
>   if (dev_read_bool(dev, "foo-property") {
>          /* default value '0' will not be used anyway */
>          priv->val = dev_read_u32_default(dev, "foo-property", 0);
>   }

priv->val = dev_read_u32_default(dev,"foo-property", priv->val);

>
> One more BTW, it was just painful to write equivalent code twice
> for CONFIG_DM_DEV_READ_INLINE.

This helps reduce code size for the case where we don't use livetree
(SPL). Any ideas on how to make it easier?

>
>
>  drivers/core/read.c |  5 +++++
>  include/dm/read.h   | 16 ++++++++++++++++
>  2 files changed, 21 insertions(+)

Regards,
Simon

Patch

diff --git a/drivers/core/read.c b/drivers/core/read.c
index 5d440ce..e027041 100644
--- a/drivers/core/read.c
+++ b/drivers/core/read.c
@@ -10,6 +10,11 @@ 
 #include <mapmem.h>
 #include <dm/of_access.h>
 
+int dev_read_u32(struct udevice *dev, const char *propname, u32 *outp)
+{
+	return ofnode_read_u32(dev_ofnode(dev), propname, outp);
+}
+
 int dev_read_u32_default(struct udevice *dev, const char *propname, int def)
 {
 	return ofnode_read_u32_default(dev_ofnode(dev), propname, def);
diff --git a/include/dm/read.h b/include/dm/read.h
index 8114037..5cacec8 100644
--- a/include/dm/read.h
+++ b/include/dm/read.h
@@ -46,6 +46,16 @@  static inline bool dev_of_valid(struct udevice *dev)
 
 #ifndef CONFIG_DM_DEV_READ_INLINE
 /**
+ * dev_read_u32() - read a 32-bit integer from a device's DT property
+ *
+ * @dev:	device to read DT property from
+ * @propname:	name of the property to read from
+ * @outp:	place to put value (if found)
+ * @return 0 if OK, -ve on error
+ */
+int dev_read_u32(struct udevice *dev, const char *propname, u32 *outp);
+
+/**
  * dev_read_u32_default() - read a 32-bit integer from a device's DT property
  *
  * @dev:	device to read DT property from
@@ -412,6 +422,12 @@  int dev_read_resource_byname(struct udevice *dev, const char *name,
 
 #else /* CONFIG_DM_DEV_READ_INLINE is enabled */
 
+static inline int dev_read_u32(struct udevice *dev,
+			       const char *propname, u32 *outp)
+{
+	return ofnode_read_u32(dev_ofnode(dev), propname, outp);
+}
+
 static inline int dev_read_u32_default(struct udevice *dev,
 				       const char *propname, int def)
 {