| Message ID | 1512493493-6464-1-git-send-email-okaya@codeaurora.org |
|---|---|
| Headers | show |
| Series | dmaengine: qcom_hidma: add support for bugfixed HW | expand |
On Tue, Dec 5, 2017 at 6:04 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > OF has of_device_get_match_data() function to extract driver specific data > structure. Add a similar function for ACPI. > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Not sure yet, sorry. > --- > drivers/acpi/bus.c | 12 ++++++++++++ > include/linux/acpi.h | 6 ++++++ > 2 files changed, 18 insertions(+) > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index 4d0979e..b271eb1 100644 > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -785,6 +785,18 @@ const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids, > } > EXPORT_SYMBOL_GPL(acpi_match_device); > > +void *acpi_get_match_data(const struct device *dev) > +{ > + const struct acpi_device_id *match; > + > + match = acpi_match_device(dev->driver->acpi_match_table, dev); Shouldn't this check dev->driver against NULL before dereferencing it? > + if (!match) > + return NULL; > + > + return (void *)match->driver_data; > +} > +EXPORT_SYMBOL_GPL(acpi_get_match_data); > + > int acpi_match_device_ids(struct acpi_device *device, > const struct acpi_device_id *ids) > { > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 502af53..a927260 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -584,6 +584,7 @@ extern int acpi_nvs_for_each_region(int (*func)(__u64, __u64, void *), > const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids, > const struct device *dev); > > +void *acpi_get_match_data(const struct device *dev); > extern bool acpi_driver_match_device(struct device *dev, > const struct device_driver *drv); > int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *); > @@ -755,6 +756,11 @@ static inline const struct acpi_device_id *acpi_match_device( > return NULL; > } > > +static inline void *acpi_get_match_data(const struct device *dev) > +{ > + return NULL; > +} > + > static inline bool acpi_driver_match_device(struct device *dev, > const struct device_driver *drv) > { > -- > 1.9.1 > > -- > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 5, 2017 at 6:04 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > Introduce new ACPI and OF device ids for thw HW along with the helper > functions. > > Changes from v5: > * use struct device as a calling parameter to get_match_data() callback > so that we can reuse the existing OF API. > * revert the change on acpi_get_match_data() to V4. > > Sinan Kaya (7): > Documentation: DT: qcom_hidma: Bump HW revision for the bugfixed HW > ACPI / bus: Introduce acpi_get_match_data() function > device property: Introduce a common API to fetch device match data > OF: properties: Implement get_match_data() callback > ACPI: properties: Implement get_match_data() callback > dmaengine: qcom_hidma: Add support for the new revision > dmaengine: qcom_hidma: Add identity register support Sakari, can you please have a look at this series? I'm particularly interested in your opinion on patches [2-3/7] and [5/7]. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sinan, Thanks for the update. Just one small comment below. On Tue, Dec 05, 2017 at 12:04:48PM -0500, Sinan Kaya wrote: > There is an OF/ACPI function to obtain the driver data. We want to hide > OF/ACPI details from the device drivers and abstract following the device > family of functions. > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/base/property.c | 6 ++++++ > include/linux/fwnode.h | 4 ++++ > include/linux/property.h | 2 ++ > 3 files changed, 12 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 7ed99c1..65bf6f2 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -1335,3 +1335,9 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode, > return fwnode_call_int_op(fwnode, graph_parse_endpoint, endpoint); > } > EXPORT_SYMBOL(fwnode_graph_parse_endpoint); > + > +void *device_get_match_data(struct device *dev) > +{ > + return fwnode_call_ptr_op(dev_fwnode(dev), get_match_data, dev); > +} > +EXPORT_SYMBOL_GPL(device_get_match_data); > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h > index 0c35b6c..ab9aab5 100644 > --- a/include/linux/fwnode.h > +++ b/include/linux/fwnode.h > @@ -15,6 +15,7 @@ > #include <linux/types.h> > > struct fwnode_operations; > +struct device; > > struct fwnode_handle { > struct fwnode_handle *secondary; > @@ -66,6 +67,7 @@ struct fwnode_reference_args { > * endpoint node. > * @graph_get_port_parent: Return the parent node of a port node. > * @graph_parse_endpoint: Parse endpoint for port and endpoint id. > + * @get_match_data: Return the driver match data. Could you arrange the new get_match_data op closer to the other ops that don't deal with the graphs? Such as device_is_available. As the ops are dealing generally with fwnodes, I might call this device_get_match_data to explicitly mention it's for devices. > */ > struct fwnode_operations { > void (*get)(struct fwnode_handle *fwnode); > @@ -101,6 +103,8 @@ struct fwnode_operations { > (*graph_get_port_parent)(struct fwnode_handle *fwnode); > int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode, > struct fwnode_endpoint *endpoint); > + void *(*get_match_data)(const struct fwnode_handle *fwnode, > + struct device *dev); Same comment about the order here. > }; > > #define fwnode_has_op(fwnode, op) \ > diff --git a/include/linux/property.h b/include/linux/property.h > index 6bebee1..01fa55b 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -275,6 +275,8 @@ int device_add_properties(struct device *dev, > > enum dev_dma_attr device_get_dma_attr(struct device *dev); > > +void *device_get_match_data(struct device *dev); > + > int device_get_phy_mode(struct device *dev); > > void *device_get_mac_address(struct device *dev, char *addr, int alen);
On Tue, Dec 05, 2017 at 12:04:48PM -0500, Sinan Kaya wrote: > @@ -101,6 +103,8 @@ struct fwnode_operations { > (*graph_get_port_parent)(struct fwnode_handle *fwnode); > int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode, > struct fwnode_endpoint *endpoint); > + void *(*get_match_data)(const struct fwnode_handle *fwnode, > + struct device *dev); You can make dev const, too.
On Tue, Dec 05, 2017 at 11:05:52PM +0100, Rafael J. Wysocki wrote: > On Tue, Dec 5, 2017 at 6:04 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > > Introduce new ACPI and OF device ids for thw HW along with the helper > > functions. > > > > Changes from v5: > > * use struct device as a calling parameter to get_match_data() callback > > so that we can reuse the existing OF API. > > * revert the change on acpi_get_match_data() to V4. > > > > Sinan Kaya (7): > > Documentation: DT: qcom_hidma: Bump HW revision for the bugfixed HW > > ACPI / bus: Introduce acpi_get_match_data() function > > device property: Introduce a common API to fetch device match data > > OF: properties: Implement get_match_data() callback > > ACPI: properties: Implement get_match_data() callback > > dmaengine: qcom_hidma: Add support for the new revision > > dmaengine: qcom_hidma: Add identity register support > > Sakari, can you please have a look at this series? > > I'm particularly interested in your opinion on patches [2-3/7] and [5/7]. Reviewed. Thanks for the heads up!
On 12/7/2017 7:29 AM, Sakari Ailus wrote: > Hi Sinan, > > Thanks for the update. > > Just one small comment below. > [snip] >> >> struct fwnode_handle { >> struct fwnode_handle *secondary; >> @@ -66,6 +67,7 @@ struct fwnode_reference_args { >> * endpoint node. >> * @graph_get_port_parent: Return the parent node of a port node. >> * @graph_parse_endpoint: Parse endpoint for port and endpoint id. >> + * @get_match_data: Return the driver match data. > > Could you arrange the new get_match_data op closer to the other ops that > don't deal with the graphs? Such as device_is_available. As the ops are > dealing generally with fwnodes, I might call this device_get_match_data to > explicitly mention it's for devices. done > >> */ >> struct fwnode_operations { >> void (*get)(struct fwnode_handle *fwnode); >> @@ -101,6 +103,8 @@ struct fwnode_operations { >> (*graph_get_port_parent)(struct fwnode_handle *fwnode); >> int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode, >> struct fwnode_endpoint *endpoint); >> + void *(*get_match_data)(const struct fwnode_handle *fwnode, >> + struct device *dev); > > Same comment about the order here. ok > >> }; >> >> #define fwnode_has_op(fwnode, op) \ >> diff --git a/include/linux/property.h b/include/linux/property.h >> index 6bebee1..01fa55b 100644 >> --- a/include/linux/property.h >> +++ b/include/linux/property.h >> @@ -275,6 +275,8 @@ int device_add_properties(struct device *dev, >> >> enum dev_dma_attr device_get_dma_attr(struct device *dev); >> >> +void *device_get_match_data(struct device *dev); >> + >> int device_get_phy_mode(struct device *dev); >> >> void *device_get_mac_address(struct device *dev, char *addr, int alen); >
On 12/7/2017 7:40 AM, Sakari Ailus wrote: > On Tue, Dec 05, 2017 at 12:04:48PM -0500, Sinan Kaya wrote: >> @@ -101,6 +103,8 @@ struct fwnode_operations { >> (*graph_get_port_parent)(struct fwnode_handle *fwnode); >> int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode, >> struct fwnode_endpoint *endpoint); >> + void *(*get_match_data)(const struct fwnode_handle *fwnode, >> + struct device *dev); > > You can make dev const, too. > done, I couldn't change device_get_match_data() parameter const due to dev_fwnode() function. from /local/mnt/workspace/projects/caf/kernel/drivers/base/property.c:13: /local/mnt/workspace/projects/caf/kernel/drivers/base/property.c:1341:39: warning: passing argument 1 of 'dev_fwnode' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data,
On Thu, Dec 07, 2017 at 03:17:52PM -0500, Sinan Kaya wrote: > On 12/7/2017 7:40 AM, Sakari Ailus wrote: > > On Tue, Dec 05, 2017 at 12:04:48PM -0500, Sinan Kaya wrote: > >> @@ -101,6 +103,8 @@ struct fwnode_operations { > >> (*graph_get_port_parent)(struct fwnode_handle *fwnode); > >> int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode, > >> struct fwnode_endpoint *endpoint); > >> + void *(*get_match_data)(const struct fwnode_handle *fwnode, > >> + struct device *dev); > > > > You can make dev const, too. > > > > done, I couldn't change device_get_match_data() parameter const due to > dev_fwnode() function. > > from /local/mnt/workspace/projects/caf/kernel/drivers/base/property.c:13: > > /local/mnt/workspace/projects/caf/kernel/drivers/base/property.c:1341:39: warning: passing argument 1 of 'dev_fwnode' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] > return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, Right. Makes sense. I guess it's not perhaps worth it introducing dev_fwnode_const just for this. devices are seldom if ever const anyway.
On Thu, Dec 7, 2017 at 11:06 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > On Thu, Dec 07, 2017 at 03:17:52PM -0500, Sinan Kaya wrote: >> On 12/7/2017 7:40 AM, Sakari Ailus wrote: >> > On Tue, Dec 05, 2017 at 12:04:48PM -0500, Sinan Kaya wrote: >> >> @@ -101,6 +103,8 @@ struct fwnode_operations { >> >> (*graph_get_port_parent)(struct fwnode_handle *fwnode); >> >> int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode, >> >> struct fwnode_endpoint *endpoint); >> >> + void *(*get_match_data)(const struct fwnode_handle *fwnode, >> >> + struct device *dev); >> > >> > You can make dev const, too. >> > >> >> done, I couldn't change device_get_match_data() parameter const due to >> dev_fwnode() function. >> >> from /local/mnt/workspace/projects/caf/kernel/drivers/base/property.c:13: >> >> /local/mnt/workspace/projects/caf/kernel/drivers/base/property.c:1341:39: warning: passing argument 1 of 'dev_fwnode' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] >> return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, > > Right. Makes sense. > > I guess it's not perhaps worth it introducing dev_fwnode_const just for > this. devices are seldom if ever const anyway. They cannot be const. Had they been const, it wouldn't have been possible to register them even. :-) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 07, 2017 at 11:19:51PM +0100, Rafael J. Wysocki wrote: > On Thu, Dec 7, 2017 at 11:06 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > On Thu, Dec 07, 2017 at 03:17:52PM -0500, Sinan Kaya wrote: > >> On 12/7/2017 7:40 AM, Sakari Ailus wrote: > >> > On Tue, Dec 05, 2017 at 12:04:48PM -0500, Sinan Kaya wrote: > >> >> @@ -101,6 +103,8 @@ struct fwnode_operations { > >> >> (*graph_get_port_parent)(struct fwnode_handle *fwnode); > >> >> int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode, > >> >> struct fwnode_endpoint *endpoint); > >> >> + void *(*get_match_data)(const struct fwnode_handle *fwnode, > >> >> + struct device *dev); > >> > > >> > You can make dev const, too. > >> > > >> > >> done, I couldn't change device_get_match_data() parameter const due to > >> dev_fwnode() function. > >> > >> from /local/mnt/workspace/projects/caf/kernel/drivers/base/property.c:13: > >> > >> /local/mnt/workspace/projects/caf/kernel/drivers/base/property.c:1341:39: warning: passing argument 1 of 'dev_fwnode' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] > >> return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, > > > > Right. Makes sense. > > > > I guess it's not perhaps worth it introducing dev_fwnode_const just for > > this. devices are seldom if ever const anyway. > > They cannot be const. Had they been const, it wouldn't have been > possible to register them even. :-) In general no, but this function does not change the device in any way, therefore it could be const in principle.