| Message ID | 1422278260-108175-1-git-send-email-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Mon, Jan 26, 2015 at 03:17:40PM +0200, Heikki Krogerus wrote: > This extends the unified device property interface by adding > "Generic Property" to it for cases where device tree or ACPI > are not being used. > > That makes the unified device property interface cover also > most of the cases where information is extracted from custom > platform_data in the drivers. So if before we had to check > separately is there custom platform_data for a driver: > > if (pdata) > bar = pdata->bar; > else > device_property_read_u32(dev, "foo", &bar); > > we can now drop that check and simply always use the unified > device property interface. > > That makes it possible to drop a lot of boiler plate from > the drivers, plus quite a few header files under > include/linux/ describing those driver specific platform > data structures can be removed. Gentle ping on this.
On Monday, January 26, 2015 03:17:40 PM Heikki Krogerus wrote: > This extends the unified device property interface by adding > "Generic Property" to it for cases where device tree or ACPI > are not being used. > > That makes the unified device property interface cover also > most of the cases where information is extracted from custom > platform_data in the drivers. So if before we had to check > separately is there custom platform_data for a driver: > > if (pdata) > bar = pdata->bar; > else > device_property_read_u32(dev, "foo", &bar); > > we can now drop that check and simply always use the unified > device property interface. > > That makes it possible to drop a lot of boiler plate from > the drivers, plus quite a few header files under > include/linux/ describing those driver specific platform > data structures can be removed. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/base/property.c | 135 ++++++++++++++++++++++++++++++++++++++++++----- > include/linux/device.h | 3 ++ > include/linux/property.h | 17 ++++++ > 3 files changed, 143 insertions(+), 12 deletions(-) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index c458458..4ea6d27 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -15,6 +15,108 @@ > #include <linux/acpi.h> > #include <linux/of.h> > > +static struct dev_gen_prop *dev_prop_get(struct device *dev, const char *name) > +{ > + struct dev_gen_prop *prop; > + > + if (!dev->gen_prop) > + return NULL; > + > + for (prop = dev->gen_prop; prop->name; prop++) > + if (!strcmp(name, prop->name)) > + return prop; > + return NULL; > +} > + > +static int dev_prop_copy_array_u8(u8 *src, u8 *val, size_t nval) > +{ > + int i; > + > + for (i = 0; i < nval; i++) > + val[i] = src[i]; Use memcpy() perhaps? And below too? And then you may not need these helpers any more. > + return 0; > +} > + > +static int dev_prop_copy_array_u16(u16 *src, u16 *val, size_t nval) > +{ > + int i; > + > + for (i = 0; i < nval; i++) > + val[i] = src[i]; > + return 0; > +} > + > +static int dev_prop_copy_array_u32(u32 *src, u32 *val, size_t nval) > +{ > + int i; > + > + for (i = 0; i < nval; i++) > + val[i] = src[i]; > + return 0; > +} > + > +static int dev_prop_copy_array_u64(u64 *src, u64 *val, size_t nval) > +{ > + int i; > + > + for (i = 0; i < nval; i++) > + val[i] = src[i]; > + return 0; > +} > + > +static int dev_prop_copy_array_string(const char **src, const char **val, > + size_t nval) > +{ > + int i; > + > + for (i = 0; i < nval; i++) > + val[i] = src[i]; > + return 0; > +} > + > +static int dev_prop_read_array(struct device *dev, const char *name, > + enum dev_prop_type type, void *val, size_t nval) > +{ > + struct dev_gen_prop *prop; > + int ret = 0; > + > + prop = dev_prop_get(dev, name); > + if (!prop) > + return -ENODATA; > + > + if (prop->type != type) > + return -EPROTO; > + > + if (prop->nval < nval) > + return -EOVERFLOW; > + > + switch (prop->type) { > + case DEV_PROP_U8: > + ret = dev_prop_copy_array_u8((u8 *)prop->num, (u8 *)val, > + nval); > + break; > + case DEV_PROP_U16: > + ret = dev_prop_copy_array_u16((u16 *)prop->num, (u16 *)val, > + nval); > + break; > + case DEV_PROP_U32: > + ret = dev_prop_copy_array_u32((u32 *)prop->num, (u32 *)val, > + nval); > + break; > + case DEV_PROP_U64: > + ret = dev_prop_copy_array_u64(prop->num, (u64 *)val, nval); > + break; > + case DEV_PROP_STRING: > + ret = dev_prop_copy_array_string(prop->str, > + (const char **)val, nval); > + break; > + default: > + ret = -EINVAL; > + break; > + } > + return ret; > +} > + > /** > * device_property_present - check if a property of a device is present > * @dev: Device whose property is being checked > @@ -26,8 +128,9 @@ bool device_property_present(struct device *dev, const char *propname) > { > if (IS_ENABLED(CONFIG_OF) && dev->of_node) > return of_property_read_bool(dev->of_node, propname); > - > - return !acpi_dev_prop_get(ACPI_COMPANION(dev), propname, NULL); > + if (ACPI_HANDLE(dev)) > + return !acpi_dev_prop_get(ACPI_COMPANION(dev), propname, NULL); > + return !!dev_prop_get(dev, propname); > } > EXPORT_SYMBOL_GPL(device_property_present); > > @@ -55,8 +158,11 @@ EXPORT_SYMBOL_GPL(fwnode_property_present); > IS_ENABLED(CONFIG_OF) && _dev_->of_node ? \ > (OF_DEV_PROP_READ_ARRAY(_dev_->of_node, _propname_, _type_, \ > _val_, _nval_)) : \ > - acpi_dev_prop_read(ACPI_COMPANION(_dev_), _propname_, \ > - _proptype_, _val_, _nval_) > + ACPI_HANDLE(_dev_) ? \ > + acpi_dev_prop_read(ACPI_COMPANION(_dev_), _propname_, \ > + _proptype_, _val_, _nval_) : \ > + dev_prop_read_array(_dev_, _propname_, _proptype_, \ > + _val_, _nval_) > > /** > * device_property_read_u8_array - return a u8 array property of a device > @@ -169,10 +275,13 @@ EXPORT_SYMBOL_GPL(device_property_read_u64_array); > int device_property_read_string_array(struct device *dev, const char *propname, > const char **val, size_t nval) > { > - return IS_ENABLED(CONFIG_OF) && dev->of_node ? > - of_property_read_string_array(dev->of_node, propname, val, nval) : > - acpi_dev_prop_read(ACPI_COMPANION(dev), propname, > - DEV_PROP_STRING, val, nval); > + if (IS_ENABLED(CONFIG_OF) && dev->of_node) > + return of_property_read_string_array(dev->of_node, propname, > + val, nval); > + if (ACPI_HANDLE(dev)) > + return acpi_dev_prop_read(ACPI_COMPANION(dev), propname, > + DEV_PROP_STRING, val, nval); > + return dev_prop_read_array(dev, propname, DEV_PROP_STRING, val, nval); > } > EXPORT_SYMBOL_GPL(device_property_read_string_array); > > @@ -193,10 +302,12 @@ EXPORT_SYMBOL_GPL(device_property_read_string_array); > int device_property_read_string(struct device *dev, const char *propname, > const char **val) > { > - return IS_ENABLED(CONFIG_OF) && dev->of_node ? > - of_property_read_string(dev->of_node, propname, val) : > - acpi_dev_prop_read(ACPI_COMPANION(dev), propname, > - DEV_PROP_STRING, val, 1); > + if (IS_ENABLED(CONFIG_OF) && dev->of_node) > + return of_property_read_string(dev->of_node, propname, val); > + if (ACPI_HANDLE(dev)) > + return acpi_dev_prop_read(ACPI_COMPANION(dev), propname, > + DEV_PROP_STRING, val, 1); > + return dev_prop_read_array(dev, propname, DEV_PROP_STRING, val, 1); > } > EXPORT_SYMBOL_GPL(device_property_read_string); > > diff --git a/include/linux/device.h b/include/linux/device.h > index fb50673..738dc25 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -27,6 +27,7 @@ > #include <linux/ratelimit.h> > #include <linux/uidgid.h> > #include <linux/gfp.h> > +#include <linux/property.h> > #include <asm/device.h> > > struct device; > @@ -704,6 +705,7 @@ struct acpi_dev_node { > * @archdata: For arch-specific additions. > * @of_node: Associated device tree node. > * @acpi_node: Associated ACPI device node. > + * @gen_prop: Generic device property > * @devt: For creating the sysfs "dev". > * @id: device instance > * @devres_lock: Spinlock to protect the resource of the device. > @@ -780,6 +782,7 @@ struct device { > > struct device_node *of_node; /* associated device tree node */ > struct acpi_dev_node acpi_node; /* associated ACPI device node */ > + struct dev_gen_prop *gen_prop; /* generic device property */ That doesn't seem to go in the right direction to be honest. Actually, having introduced struct fwnode_handle, we should perhaps try to replace both of_node and acpi_node with a single struct fwnode_handle pointer and then add a new fwnode_type for the "pdata" stuff. If you don't want to deal with of_node, which I can understand easily, it may be worth trying with acpi_node alone at this point and once you have the fwnode_handle pointer, you might use it for both ACPI and "pdata"? Grant, Arnd, I wonder what you think? > > dev_t devt; /* dev_t, creates the sysfs "dev" */ > u32 id; /* device instance */ > diff --git a/include/linux/property.h b/include/linux/property.h > index a6a3d98..44e875f 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -26,6 +26,23 @@ enum dev_prop_type { > DEV_PROP_MAX, > }; > > +/** > + * struct dev_gen_prop - Generic Device Property > + * @name: property name > + * @nval: number of elements in the array > + * @str: value array of strings > + * @num: value array of numbers > + * > + * Used when of_node and acpi_node are missing. > + */ > +struct dev_gen_prop { > + enum dev_prop_type type; > + const char *name; > + size_t nval; > + const char **str; > + u64 *num; > +}; > + > bool device_property_present(struct device *dev, const char *propname); > int device_property_read_u8_array(struct device *dev, const char *propname, > u8 *val, size_t nval); >
On Thu, Mar 12, 2015 at 1:41 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Monday, January 26, 2015 03:17:40 PM Heikki Krogerus wrote: >> This extends the unified device property interface by adding >> "Generic Property" to it for cases where device tree or ACPI >> are not being used. >> >> That makes the unified device property interface cover also >> most of the cases where information is extracted from custom >> platform_data in the drivers. So if before we had to check >> separately is there custom platform_data for a driver: >> >> if (pdata) >> bar = pdata->bar; >> else >> device_property_read_u32(dev, "foo", &bar); >> >> we can now drop that check and simply always use the unified >> device property interface. >> >> That makes it possible to drop a lot of boiler plate from >> the drivers, plus quite a few header files under >> include/linux/ describing those driver specific platform >> data structures can be removed. >> >> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> [...] >> @@ -704,6 +705,7 @@ struct acpi_dev_node { >> * @archdata: For arch-specific additions. >> * @of_node: Associated device tree node. >> * @acpi_node: Associated ACPI device node. >> + * @gen_prop: Generic device property >> * @devt: For creating the sysfs "dev". >> * @id: device instance >> * @devres_lock: Spinlock to protect the resource of the device. >> @@ -780,6 +782,7 @@ struct device { >> >> struct device_node *of_node; /* associated device tree node */ >> struct acpi_dev_node acpi_node; /* associated ACPI device node */ >> + struct dev_gen_prop *gen_prop; /* generic device property */ > > That doesn't seem to go in the right direction to be honest. > > Actually, having introduced struct fwnode_handle, we should perhaps try to > replace both of_node and acpi_node with a single struct fwnode_handle pointer > and then add a new fwnode_type for the "pdata" stuff. I agree on this, but it will be a lot of work to convert... > If you don't want to deal with of_node, which I can understand easily, it > may be worth trying with acpi_node alone at this point and once you have > the fwnode_handle pointer, you might use it for both ACPI and "pdata"? > > Grant, Arnd, I wonder what you think? That makes sense, and we can populate the fwnode_handle even when using DT. That will allow a transition period to move everyone over to the fwnode_handle. g. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 13 March 2015 15:10:38 Grant Likely wrote: > On Thu, Mar 12, 2015 at 1:41 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Monday, January 26, 2015 03:17:40 PM Heikki Krogerus wrote: > > That doesn't seem to go in the right direction to be honest. > > > > Actually, having introduced struct fwnode_handle, we should perhaps try to > > replace both of_node and acpi_node with a single struct fwnode_handle pointer > > and then add a new fwnode_type for the "pdata" stuff. > > I agree on this, but it will be a lot of work to convert... > > > If you don't want to deal with of_node, which I can understand easily, it > > may be worth trying with acpi_node alone at this point and once you have > > the fwnode_handle pointer, you might use it for both ACPI and "pdata"? > > > > Grant, Arnd, I wonder what you think? > > That makes sense, and we can populate the fwnode_handle even when > using DT. That will allow a transition period to move everyone over to > the fwnode_handle. Agreed on both as well. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > diff --git a/drivers/base/property.c b/drivers/base/property.c > > index c458458..4ea6d27 100644 > > --- a/drivers/base/property.c > > +++ b/drivers/base/property.c > > @@ -15,6 +15,108 @@ > > #include <linux/acpi.h> > > #include <linux/of.h> > > > > +static struct dev_gen_prop *dev_prop_get(struct device *dev, const char *name) > > +{ > > + struct dev_gen_prop *prop; > > + > > + if (!dev->gen_prop) > > + return NULL; > > + > > + for (prop = dev->gen_prop; prop->name; prop++) > > + if (!strcmp(name, prop->name)) > > + return prop; > > + return NULL; > > +} > > + > > +static int dev_prop_copy_array_u8(u8 *src, u8 *val, size_t nval) > > +{ > > + int i; > > + > > + for (i = 0; i < nval; i++) > > + val[i] = src[i]; > > Use memcpy() perhaps? And below too? And then you may not need these > helpers any more. OK. <snip> > > @@ -780,6 +782,7 @@ struct device { > > > > struct device_node *of_node; /* associated device tree node */ > > struct acpi_dev_node acpi_node; /* associated ACPI device node */ > > + struct dev_gen_prop *gen_prop; /* generic device property */ > > That doesn't seem to go in the right direction to be honest. > > Actually, having introduced struct fwnode_handle, we should perhaps try to > replace both of_node and acpi_node with a single struct fwnode_handle pointer > and then add a new fwnode_type for the "pdata" stuff. > > If you don't want to deal with of_node, which I can understand easily, it > may be worth trying with acpi_node alone at this point and once you have > the fwnode_handle pointer, you might use it for both ACPI and "pdata"? It sounds like a better Idea to me too. Thanks,
diff --git a/drivers/base/property.c b/drivers/base/property.c index c458458..4ea6d27 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -15,6 +15,108 @@ #include <linux/acpi.h> #include <linux/of.h> +static struct dev_gen_prop *dev_prop_get(struct device *dev, const char *name) +{ + struct dev_gen_prop *prop; + + if (!dev->gen_prop) + return NULL; + + for (prop = dev->gen_prop; prop->name; prop++) + if (!strcmp(name, prop->name)) + return prop; + return NULL; +} + +static int dev_prop_copy_array_u8(u8 *src, u8 *val, size_t nval) +{ + int i; + + for (i = 0; i < nval; i++) + val[i] = src[i]; + return 0; +} + +static int dev_prop_copy_array_u16(u16 *src, u16 *val, size_t nval) +{ + int i; + + for (i = 0; i < nval; i++) + val[i] = src[i]; + return 0; +} + +static int dev_prop_copy_array_u32(u32 *src, u32 *val, size_t nval) +{ + int i; + + for (i = 0; i < nval; i++) + val[i] = src[i]; + return 0; +} + +static int dev_prop_copy_array_u64(u64 *src, u64 *val, size_t nval) +{ + int i; + + for (i = 0; i < nval; i++) + val[i] = src[i]; + return 0; +} + +static int dev_prop_copy_array_string(const char **src, const char **val, + size_t nval) +{ + int i; + + for (i = 0; i < nval; i++) + val[i] = src[i]; + return 0; +} + +static int dev_prop_read_array(struct device *dev, const char *name, + enum dev_prop_type type, void *val, size_t nval) +{ + struct dev_gen_prop *prop; + int ret = 0; + + prop = dev_prop_get(dev, name); + if (!prop) + return -ENODATA; + + if (prop->type != type) + return -EPROTO; + + if (prop->nval < nval) + return -EOVERFLOW; + + switch (prop->type) { + case DEV_PROP_U8: + ret = dev_prop_copy_array_u8((u8 *)prop->num, (u8 *)val, + nval); + break; + case DEV_PROP_U16: + ret = dev_prop_copy_array_u16((u16 *)prop->num, (u16 *)val, + nval); + break; + case DEV_PROP_U32: + ret = dev_prop_copy_array_u32((u32 *)prop->num, (u32 *)val, + nval); + break; + case DEV_PROP_U64: + ret = dev_prop_copy_array_u64(prop->num, (u64 *)val, nval); + break; + case DEV_PROP_STRING: + ret = dev_prop_copy_array_string(prop->str, + (const char **)val, nval); + break; + default: + ret = -EINVAL; + break; + } + return ret; +} + /** * device_property_present - check if a property of a device is present * @dev: Device whose property is being checked @@ -26,8 +128,9 @@ bool device_property_present(struct device *dev, const char *propname) { if (IS_ENABLED(CONFIG_OF) && dev->of_node) return of_property_read_bool(dev->of_node, propname); - - return !acpi_dev_prop_get(ACPI_COMPANION(dev), propname, NULL); + if (ACPI_HANDLE(dev)) + return !acpi_dev_prop_get(ACPI_COMPANION(dev), propname, NULL); + return !!dev_prop_get(dev, propname); } EXPORT_SYMBOL_GPL(device_property_present); @@ -55,8 +158,11 @@ EXPORT_SYMBOL_GPL(fwnode_property_present); IS_ENABLED(CONFIG_OF) && _dev_->of_node ? \ (OF_DEV_PROP_READ_ARRAY(_dev_->of_node, _propname_, _type_, \ _val_, _nval_)) : \ - acpi_dev_prop_read(ACPI_COMPANION(_dev_), _propname_, \ - _proptype_, _val_, _nval_) + ACPI_HANDLE(_dev_) ? \ + acpi_dev_prop_read(ACPI_COMPANION(_dev_), _propname_, \ + _proptype_, _val_, _nval_) : \ + dev_prop_read_array(_dev_, _propname_, _proptype_, \ + _val_, _nval_) /** * device_property_read_u8_array - return a u8 array property of a device @@ -169,10 +275,13 @@ EXPORT_SYMBOL_GPL(device_property_read_u64_array); int device_property_read_string_array(struct device *dev, const char *propname, const char **val, size_t nval) { - return IS_ENABLED(CONFIG_OF) && dev->of_node ? - of_property_read_string_array(dev->of_node, propname, val, nval) : - acpi_dev_prop_read(ACPI_COMPANION(dev), propname, - DEV_PROP_STRING, val, nval); + if (IS_ENABLED(CONFIG_OF) && dev->of_node) + return of_property_read_string_array(dev->of_node, propname, + val, nval); + if (ACPI_HANDLE(dev)) + return acpi_dev_prop_read(ACPI_COMPANION(dev), propname, + DEV_PROP_STRING, val, nval); + return dev_prop_read_array(dev, propname, DEV_PROP_STRING, val, nval); } EXPORT_SYMBOL_GPL(device_property_read_string_array); @@ -193,10 +302,12 @@ EXPORT_SYMBOL_GPL(device_property_read_string_array); int device_property_read_string(struct device *dev, const char *propname, const char **val) { - return IS_ENABLED(CONFIG_OF) && dev->of_node ? - of_property_read_string(dev->of_node, propname, val) : - acpi_dev_prop_read(ACPI_COMPANION(dev), propname, - DEV_PROP_STRING, val, 1); + if (IS_ENABLED(CONFIG_OF) && dev->of_node) + return of_property_read_string(dev->of_node, propname, val); + if (ACPI_HANDLE(dev)) + return acpi_dev_prop_read(ACPI_COMPANION(dev), propname, + DEV_PROP_STRING, val, 1); + return dev_prop_read_array(dev, propname, DEV_PROP_STRING, val, 1); } EXPORT_SYMBOL_GPL(device_property_read_string); diff --git a/include/linux/device.h b/include/linux/device.h index fb50673..738dc25 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -27,6 +27,7 @@ #include <linux/ratelimit.h> #include <linux/uidgid.h> #include <linux/gfp.h> +#include <linux/property.h> #include <asm/device.h> struct device; @@ -704,6 +705,7 @@ struct acpi_dev_node { * @archdata: For arch-specific additions. * @of_node: Associated device tree node. * @acpi_node: Associated ACPI device node. + * @gen_prop: Generic device property * @devt: For creating the sysfs "dev". * @id: device instance * @devres_lock: Spinlock to protect the resource of the device. @@ -780,6 +782,7 @@ struct device { struct device_node *of_node; /* associated device tree node */ struct acpi_dev_node acpi_node; /* associated ACPI device node */ + struct dev_gen_prop *gen_prop; /* generic device property */ dev_t devt; /* dev_t, creates the sysfs "dev" */ u32 id; /* device instance */ diff --git a/include/linux/property.h b/include/linux/property.h index a6a3d98..44e875f 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -26,6 +26,23 @@ enum dev_prop_type { DEV_PROP_MAX, }; +/** + * struct dev_gen_prop - Generic Device Property + * @name: property name + * @nval: number of elements in the array + * @str: value array of strings + * @num: value array of numbers + * + * Used when of_node and acpi_node are missing. + */ +struct dev_gen_prop { + enum dev_prop_type type; + const char *name; + size_t nval; + const char **str; + u64 *num; +}; + bool device_property_present(struct device *dev, const char *propname); int device_property_read_u8_array(struct device *dev, const char *propname, u8 *val, size_t nval);
This extends the unified device property interface by adding "Generic Property" to it for cases where device tree or ACPI are not being used. That makes the unified device property interface cover also most of the cases where information is extracted from custom platform_data in the drivers. So if before we had to check separately is there custom platform_data for a driver: if (pdata) bar = pdata->bar; else device_property_read_u32(dev, "foo", &bar); we can now drop that check and simply always use the unified device property interface. That makes it possible to drop a lot of boiler plate from the drivers, plus quite a few header files under include/linux/ describing those driver specific platform data structures can be removed. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/base/property.c | 135 ++++++++++++++++++++++++++++++++++++++++++----- include/linux/device.h | 3 ++ include/linux/property.h | 17 ++++++ 3 files changed, 143 insertions(+), 12 deletions(-)