| 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 | expand |
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
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) {
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(+)