[PATCH V6 0/7] dmaengine: qcom_hidma: add support for bugfixed HW

[PATCH V6 0/7] dmaengine: qcom_hidma: add support for bugfixed HW

[Sinan Kaya]

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

 .../devicetree/bindings/dma/qcom_hidma_mgmt.txt    |  4 +--
 drivers/acpi/bus.c                                 | 12 +++++++
 drivers/acpi/property.c                            |  7 ++++
 drivers/base/property.c                            |  6 ++++
 drivers/dma/qcom/hidma.c                           | 41 ++++++++++------------
 drivers/of/property.c                              |  7 ++++
 include/linux/acpi.h                               |  6 ++++
 include/linux/fwnode.h                             |  4 +++
 include/linux/property.h                           |  2 ++
 9 files changed, 65 insertions(+), 24 deletions(-)

-- 
1.9.1

[PATCH V6 1/7] Documentation: DT: qcom_hidma: Bump HW revision for the bugfixed HW

[Sinan Kaya]

A new version of the HIDMA IP has been released with bug fixes. Bumping the
hardware version to differentiate from others.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
index 55492c2..5d93d6d 100644
--- a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
+++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
@@ -47,8 +47,8 @@ When the OS is not in control of the management interface (i.e. it's a guest),
 the channel nodes appear on their own, not under a management node.
 
 Required properties:
-- compatible: must contain "qcom,hidma-1.0" for initial HW or "qcom,hidma-1.1"
-for MSI capable HW.
+- compatible: must contain "qcom,hidma-1.0" for initial HW or
+  "qcom,hidma-1.1"/"qcom,hidma-1.2" for MSI capable HW.
 - reg: Addresses for the transfer and event channel
 - interrupts: Should contain the event interrupt
 - desc-count: Number of asynchronous requests this channel can handle
-- 
1.9.1

[PATCH V6 2/7] ACPI / bus: Introduce acpi_get_match_data() function

[Sinan Kaya]

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>
---
 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);
+	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

[PATCH V6 3/7] device property: Introduce a common API to fetch device match data

[Sinan Kaya]

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.
  */
 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);
 };
 
 #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);
-- 
1.9.1

[PATCH V6 4/7] OF: properties: Implement get_match_data() callback

[Sinan Kaya]

Now that we have a get_match_data() callback as part of the firmware node,
implement the OF specific piece for it.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/of/property.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 264c355..9964169 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -981,6 +981,12 @@ static int of_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 	return 0;
 }
 
+void *of_fwnode_get_match_data(const struct fwnode_handle *fwnode,
+			       struct device *dev)
+{
+	return (void *)of_device_get_match_data(dev);
+}
+
 const struct fwnode_operations of_fwnode_ops = {
 	.get = of_fwnode_get,
 	.put = of_fwnode_put,
@@ -996,5 +1002,6 @@ static int of_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 	.graph_get_remote_endpoint = of_fwnode_graph_get_remote_endpoint,
 	.graph_get_port_parent = of_fwnode_graph_get_port_parent,
 	.graph_parse_endpoint = of_fwnode_graph_parse_endpoint,
+	.get_match_data = of_fwnode_get_match_data,
 };
 EXPORT_SYMBOL_GPL(of_fwnode_ops);
-- 
1.9.1

[PATCH V6 5/7] ACPI: properties: Implement get_match_data() callback

[Sinan Kaya]

Now that we have a get_match_data() callback as part of the firmware node,
implement the ACPI specific piece for it.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/acpi/property.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index e26ea20..38b599b 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1271,6 +1271,12 @@ static int acpi_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 	return 0;
 }
 
+static void *acpi_fwnode_get_match_data(const struct fwnode_handle *fwnode,
+					struct device *dev)
+{
+	return acpi_get_match_data(dev);
+}
+
 #define DECLARE_ACPI_FWNODE_OPS(ops) \
 	const struct fwnode_operations ops = {				\
 		.device_is_available = acpi_fwnode_device_is_available, \
@@ -1289,6 +1295,7 @@ static int acpi_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 			acpi_fwnode_graph_get_remote_endpoint,		\
 		.graph_get_port_parent = acpi_fwnode_get_parent,	\
 		.graph_parse_endpoint = acpi_fwnode_graph_parse_endpoint, \
+		.get_match_data = acpi_fwnode_get_match_data,		\
 	};								\
 	EXPORT_SYMBOL_GPL(ops)
 
-- 
1.9.1

[PATCH V6 6/7] dmaengine: qcom_hidma: Add support for the new revision

[Sinan Kaya]

Add support for probing the newer HW and also organize MSI capable hardware
into an array for maintenance reasons.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/dma/qcom/hidma.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
index e366985..c146c6d 100644
--- a/drivers/dma/qcom/hidma.c
+++ b/drivers/dma/qcom/hidma.c
@@ -50,6 +50,7 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/of_dma.h>
+#include <linux/of_device.h>
 #include <linux/property.h>
 #include <linux/delay.h>
 #include <linux/acpi.h>
@@ -104,6 +105,9 @@ static void hidma_free(struct hidma_dev *dmadev)
 module_param(nr_desc_prm, uint, 0644);
 MODULE_PARM_DESC(nr_desc_prm, "number of descriptors (default: 0)");
 
+enum hidma_cap {
+	HIDMA_MSI_CAP = 1,
+};
 
 /* process completed descriptors */
 static void hidma_process_completed(struct hidma_chan *mchan)
@@ -736,25 +740,12 @@ static int hidma_request_msi(struct hidma_dev *dmadev,
 #endif
 }
 
-static bool hidma_msi_capable(struct device *dev)
+static bool hidma_test_capability(struct device *dev, enum hidma_cap test_cap)
 {
-	struct acpi_device *adev = ACPI_COMPANION(dev);
-	const char *of_compat;
-	int ret = -EINVAL;
-
-	if (!adev || acpi_disabled) {
-		ret = device_property_read_string(dev, "compatible",
-						  &of_compat);
-		if (ret)
-			return false;
+	enum hidma_cap cap;
 
-		ret = strcmp(of_compat, "qcom,hidma-1.1");
-	} else {
-#ifdef CONFIG_ACPI
-		ret = strcmp(acpi_device_hid(adev), "QCOM8062");
-#endif
-	}
-	return ret == 0;
+	cap = (enum hidma_cap) device_get_match_data(dev);
+	return cap ? ((cap & test_cap) > 0) : 0;
 }
 
 static int hidma_probe(struct platform_device *pdev)
@@ -834,8 +825,7 @@ static int hidma_probe(struct platform_device *pdev)
 	 * Determine the MSI capability of the platform. Old HW doesn't
 	 * support MSI.
 	 */
-	msi = hidma_msi_capable(&pdev->dev);
-
+	msi = hidma_test_capability(&pdev->dev, HIDMA_MSI_CAP);
 	device_property_read_u32(&pdev->dev, "desc-count",
 				 &dmadev->nr_descriptors);
 
@@ -953,7 +943,8 @@ static int hidma_remove(struct platform_device *pdev)
 #if IS_ENABLED(CONFIG_ACPI)
 static const struct acpi_device_id hidma_acpi_ids[] = {
 	{"QCOM8061"},
-	{"QCOM8062"},
+	{"QCOM8062", HIDMA_MSI_CAP},
+	{"QCOM8063", HIDMA_MSI_CAP},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, hidma_acpi_ids);
@@ -961,7 +952,8 @@ static int hidma_remove(struct platform_device *pdev)
 
 static const struct of_device_id hidma_match[] = {
 	{.compatible = "qcom,hidma-1.0",},
-	{.compatible = "qcom,hidma-1.1",},
+	{.compatible = "qcom,hidma-1.1", .data = (void *)(HIDMA_MSI_CAP),},
+	{.compatible = "qcom,hidma-1.2", .data = (void *)(HIDMA_MSI_CAP),},
 	{},
 };
 MODULE_DEVICE_TABLE(of, hidma_match);
-- 
1.9.1

[PATCH V6 7/7] dmaengine: qcom_hidma: Add identity register support

[Sinan Kaya]

The location for destination event channel register has been relocated from
offset 0x28 to 0x40. Update the code accordingly.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/dma/qcom/hidma.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
index c146c6d..963cc52 100644
--- a/drivers/dma/qcom/hidma.c
+++ b/drivers/dma/qcom/hidma.c
@@ -107,6 +107,7 @@ static void hidma_free(struct hidma_dev *dmadev)
 
 enum hidma_cap {
 	HIDMA_MSI_CAP = 1,
+	HIDMA_IDENTITY_CAP,
 };
 
 /* process completed descriptors */
@@ -838,7 +839,10 @@ static int hidma_probe(struct platform_device *pdev)
 	if (!dmadev->nr_descriptors)
 		dmadev->nr_descriptors = HIDMA_NR_DEFAULT_DESC;
 
-	dmadev->chidx = readl(dmadev->dev_trca + 0x28);
+	if (hidma_test_capability(&pdev->dev, HIDMA_IDENTITY_CAP))
+		dmadev->chidx = readl(dmadev->dev_trca + 0x40);
+	else
+		dmadev->chidx = readl(dmadev->dev_trca + 0x28);
 
 	/* Set DMA mask to 64 bits. */
 	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
@@ -944,7 +948,7 @@ static int hidma_remove(struct platform_device *pdev)
 static const struct acpi_device_id hidma_acpi_ids[] = {
 	{"QCOM8061"},
 	{"QCOM8062", HIDMA_MSI_CAP},
-	{"QCOM8063", HIDMA_MSI_CAP},
+	{"QCOM8063", (HIDMA_MSI_CAP | HIDMA_IDENTITY_CAP)},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, hidma_acpi_ids);
@@ -953,7 +957,8 @@ static int hidma_remove(struct platform_device *pdev)
 static const struct of_device_id hidma_match[] = {
 	{.compatible = "qcom,hidma-1.0",},
 	{.compatible = "qcom,hidma-1.1", .data = (void *)(HIDMA_MSI_CAP),},
-	{.compatible = "qcom,hidma-1.2", .data = (void *)(HIDMA_MSI_CAP),},
+	{.compatible = "qcom,hidma-1.2",
+	 .data = (void *)(HIDMA_MSI_CAP | HIDMA_IDENTITY_CAP),},
 	{},
 };
 MODULE_DEVICE_TABLE(of, hidma_match);
-- 
1.9.1

[PATCH V6 4/7] OF: properties: Implement get_match_data() callback

[Rob Herring]

On Tue, Dec 5, 2017 at 11:04 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
> Now that we have a get_match_data() callback as part of the firmware node,
> implement the OF specific piece for it.
>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/of/property.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 264c355..9964169 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -981,6 +981,12 @@ static int of_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>         return 0;
>  }
>
> +void *of_fwnode_get_match_data(const struct fwnode_handle *fwnode,

Should be static. With that, for patches 3 and 4:

Reviewed-by: Rob Herring <robh@kernel.org>

> +                              struct device *dev)
> +{
> +       return (void *)of_device_get_match_data(dev);
> +}
> +
>  const struct fwnode_operations of_fwnode_ops = {
>         .get = of_fwnode_get,
>         .put = of_fwnode_put,
> @@ -996,5 +1002,6 @@ static int of_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>         .graph_get_remote_endpoint = of_fwnode_graph_get_remote_endpoint,
>         .graph_get_port_parent = of_fwnode_graph_get_port_parent,
>         .graph_parse_endpoint = of_fwnode_graph_parse_endpoint,
> +       .get_match_data = of_fwnode_get_match_data,
>  };
>  EXPORT_SYMBOL_GPL(of_fwnode_ops);
> --
> 1.9.1
>

[PATCH V6 2/7] ACPI / bus: Introduce acpi_get_match_data() function

[Rafael J. Wysocki]

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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V6 0/7] dmaengine: qcom_hidma: add support for bugfixed HW

[Rafael J. Wysocki]

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

[PATCH V6 2/7] ACPI / bus: Introduce acpi_get_match_data() function

[Sinan Kaya]

On 12/5/2017 5:04 PM, Rafael J. Wysocki wrote:
> 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.

OK. no problem.

> 
>> ---
>>  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?

I'm adding these checks. I realized that acpi_match_device() is not validating
the ids either.

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -789,6 +789,12 @@ void *acpi_get_match_data(const struct device *dev)
 {
        const struct acpi_device_id *match;

+       if (!dev->driver)
+               return NULL;
+
+       if (!dev->driver->acpi_match_table)
+               return NULL;
+

I'll hold onto posting this until I hear back from Sakari.

Sinan

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V6 3/7] device property: Introduce a common API to fetch device match data

[Sakari Ailus]

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);

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus at iki.fi

[PATCH V6 4/7] OF: properties: Implement get_match_data() callback

[Sakari Ailus]

Hi Sinan,

On Tue, Dec 05, 2017 at 12:04:49PM -0500, Sinan Kaya wrote:
> Now that we have a get_match_data() callback as part of the firmware node,
> implement the OF specific piece for it.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/of/property.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 264c355..9964169 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -981,6 +981,12 @@ static int of_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>  	return 0;
>  }
>  
> +void *of_fwnode_get_match_data(const struct fwnode_handle *fwnode,
> +			       struct device *dev)

static, as Rob mentioned.

> +{
> +	return (void *)of_device_get_match_data(dev);
> +}
> +
>  const struct fwnode_operations of_fwnode_ops = {
>  	.get = of_fwnode_get,
>  	.put = of_fwnode_put,
> @@ -996,5 +1002,6 @@ static int of_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>  	.graph_get_remote_endpoint = of_fwnode_graph_get_remote_endpoint,
>  	.graph_get_port_parent = of_fwnode_graph_get_port_parent,
>  	.graph_parse_endpoint = of_fwnode_graph_parse_endpoint,
> +	.get_match_data = of_fwnode_get_match_data,

Please arrange right after device_is_available, the same applies to the
ACPI equivalent (5th patch).

>  };
>  EXPORT_SYMBOL_GPL(of_fwnode_ops);

With the above changes plus the ones in my comments on 3rd patch, on
patches 3--5:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus at iki.fi

[PATCH V6 3/7] device property: Introduce a common API to fetch device match data

[Sakari Ailus]

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.

-- 
Sakari Ailus
e-mail: sakari.ailus at iki.fi

[PATCH V6 0/7] dmaengine: qcom_hidma: add support for bugfixed HW

[Sakari Ailus]

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!

-- 
Sakari Ailus
e-mail: sakari.ailus at iki.fi

[PATCH V6 4/7] OF: properties: Implement get_match_data() callback

[Lothar Waßmann]

Hi,

On Tue,  5 Dec 2017 12:04:49 -0500 Sinan Kaya wrote:
> Now that we have a get_match_data() callback as part of the firmware node,
> implement the OF specific piece for it.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/of/property.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 264c355..9964169 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -981,6 +981,12 @@ static int of_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>  	return 0;
>  }
>  
> +void *of_fwnode_get_match_data(const struct fwnode_handle *fwnode,
> +			       struct device *dev)
Shouldn't this be 'const void *of_fwnode_get_match_data'



Lothar Wa?mann

[PATCH V6 4/7] OF: properties: Implement get_match_data() callback

[Sinan Kaya]

On 12/7/2017 8:10 AM, Lothar Wa?mann wrote:
>> +void *of_fwnode_get_match_data(const struct fwnode_handle *fwnode,
>> +			       struct device *dev)
> Shouldn't this be 'const void *of_fwnode_get_match_data

OF keeps the driver data as a (const void*) internally. ACPI keeps the
driver data as kernel_ulong_t in struct acpi_device_id.

I tried to find the middle ground here by converting output to void*
but not keeping const.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V6 4/7] OF: properties: Implement get_match_data() callback

[Lothar Waßmann]

Hi,

On Thu, 7 Dec 2017 09:45:31 -0500 Sinan Kaya wrote:
> On 12/7/2017 8:10 AM, Lothar Wa?mann wrote:
> >> +void *of_fwnode_get_match_data(const struct fwnode_handle *fwnode,
> >> +			       struct device *dev)
> > Shouldn't this be 'const void *of_fwnode_get_match_data
> 
> OF keeps the driver data as a (const void*) internally. ACPI keeps the
> driver data as kernel_ulong_t in struct acpi_device_id.
> 
> I tried to find the middle ground here by converting output to void*
> but not keeping const.
> 
It should be no problem to cast a (const void *) to an unsigned long
data type (without const qualifier).


Lothar Wa?mann

[PATCH V6 4/7] OF: properties: Implement get_match_data() callback

[Sinan Kaya]

On 12/7/2017 10:20 AM, Lothar Wa?mann wrote:
> Hi,
> 
> On Thu, 7 Dec 2017 09:45:31 -0500 Sinan Kaya wrote:
>> On 12/7/2017 8:10 AM, Lothar Wa?mann wrote:
>>>> +void *of_fwnode_get_match_data(const struct fwnode_handle *fwnode,
>>>> +			       struct device *dev)
>>> Shouldn't this be 'const void *of_fwnode_get_match_data
>>
>> OF keeps the driver data as a (const void*) internally. ACPI keeps the
>> driver data as kernel_ulong_t in struct acpi_device_id.
>>
>> I tried to find the middle ground here by converting output to void*
>> but not keeping const.
>>
> It should be no problem to cast a (const void *) to an unsigned long
> data type (without const qualifier).
> 

It is the other way around. If I change this API to return a a (const void*),
the device_get_match_data() function need to return a (const void *).

While implementing the ACPI piece, I have to convert an unsigned long to
(const void *) in ACPI code so that the APIs are compatible.

> 
> Lothar Wa?mann
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V6 3/7] device property: Introduce a common API to fetch device match data

[Sinan Kaya]

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);
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V6 3/7] device property: Introduce a common API to fetch device match data

[Sinan Kaya]

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,



-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V6 3/7] device property: Introduce a common API to fetch device match data

[Sakari Ailus]

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.

-- 
Sakari Ailus
e-mail: sakari.ailus at iki.fi

[PATCH V6 3/7] device property: Introduce a common API to fetch device match data

[Rafael J. Wysocki]

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. :-)

[PATCH V6 4/7] OF: properties: Implement get_match_data() callback

[Lothar Waßmann]

Hi,

On Thu, 7 Dec 2017 12:50:50 -0500 Sinan Kaya wrote:
> On 12/7/2017 10:20 AM, Lothar Wa?mann wrote:
> > Hi,
> > 
> > On Thu, 7 Dec 2017 09:45:31 -0500 Sinan Kaya wrote:
> >> On 12/7/2017 8:10 AM, Lothar Wa?mann wrote:
> >>>> +void *of_fwnode_get_match_data(const struct fwnode_handle *fwnode,
> >>>> +			       struct device *dev)
> >>> Shouldn't this be 'const void *of_fwnode_get_match_data
> >>
> >> OF keeps the driver data as a (const void*) internally. ACPI keeps the
> >> driver data as kernel_ulong_t in struct acpi_device_id.
> >>
> >> I tried to find the middle ground here by converting output to void*
> >> but not keeping const.
> >>
> > It should be no problem to cast a (const void *) to an unsigned long
> > data type (without const qualifier).
> > 
> 
> It is the other way around. If I change this API to return a a (const void*),
> the device_get_match_data() function need to return a (const void *).
> 
> While implementing the ACPI piece, I have to convert an unsigned long to
> (const void *) in ACPI code so that the APIs are compatible.
> 
That's true, but I don't see any problem with that. Your
device_get_match_data() is merely a wrapper around of_device_get_match_data()
which returns a const pointer. I see no reason to change this to a
non-const pointer by the wrapper function.


Lothar Wa?mann

[PATCH V6 4/7] OF: properties: Implement get_match_data() callback

[Lothar Waßmann]

Hi,

On Thu, 7 Dec 2017 12:50:50 -0500 Sinan Kaya wrote:
> On 12/7/2017 10:20 AM, Lothar Wa?mann wrote:
> > Hi,
> > 
> > On Thu, 7 Dec 2017 09:45:31 -0500 Sinan Kaya wrote:
> >> On 12/7/2017 8:10 AM, Lothar Wa?mann wrote:
> >>>> +void *of_fwnode_get_match_data(const struct fwnode_handle *fwnode,
> >>>> +			       struct device *dev)
> >>> Shouldn't this be 'const void *of_fwnode_get_match_data
> >>
> >> OF keeps the driver data as a (const void*) internally. ACPI keeps the
> >> driver data as kernel_ulong_t in struct acpi_device_id.
> >>
> >> I tried to find the middle ground here by converting output to void*
> >> but not keeping const.
> >>
> > It should be no problem to cast a (const void *) to an unsigned long
> > data type (without const qualifier).
> > 
> 
> It is the other way around. If I change this API to return a a (const void*),
> the device_get_match_data() function need to return a (const void *).
> 
> While implementing the ACPI piece, I have to convert an unsigned long to
> (const void *) in ACPI code so that the APIs are compatible.
> 
Just one more remark: Do you need write access to the data the pointer
returned by device_get_match_data() or of_fwnode_get_match_data()
points to?
If not, the return type of those functions should be 'const void *'.


Lothar Wa?mann

[PATCH V6 4/7] OF: properties: Implement get_match_data() callback

[Sinan Kaya]

On 12/8/2017 4:11 AM, Lothar Wa?mann wrote:
>> While implementing the ACPI piece, I have to convert an unsigned long to
>> (const void *) in ACPI code so that the APIs are compatible.
>>
> Just one more remark: Do you need write access to the data the pointer
> returned by device_get_match_data() or of_fwnode_get_match_data()
> points to?
> If not, the return type of those functions should be 'const void *'.

Yes, the only reason driver is trying to obtain this data pointer is to modify
members of it. I did a quick test with your suggestion.

test.c: In function ?main?:
test.c:15:2: error: assignment of member ?m? in read-only object
  t->m = 3;
  ^

struct test
{
        int m;
};

int main(void)
{
        const void *ptr;
        unsigned long l =4;
        const struct test *t;

        ptr = (const void *)l;

        t = ptr;
        t->m = 3;
}



-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V6 4/7] OF: properties: Implement get_match_data() callback

[Sinan Kaya]

On 12/8/2017 9:33 AM, Sinan Kaya wrote:
> On 12/8/2017 4:11 AM, Lothar Wa?mann wrote:
>>> While implementing the ACPI piece, I have to convert an unsigned long to
>>> (const void *) in ACPI code so that the APIs are compatible.
>>>
>> Just one more remark: Do you need write access to the data the pointer
>> returned by device_get_match_data() or of_fwnode_get_match_data()
>> points to?
>> If not, the return type of those functions should be 'const void *'.
> 
> Yes, the only reason driver is trying to obtain this data pointer is to modify
> members of it. I did a quick test with your suggestion.

I guess I should soften my statement here. I look at examples, they seem to be read.
Even for my HIDMA case:

static const struct of_device_id hidma_match[] = {
	{.compatible = "qcom,hidma-1.0",},
	{.compatible = "qcom,hidma-1.1", .data = (void *)(HIDMA_MSI_CAP),},
	{.compatible = "qcom,hidma-1.2",
	 .data = (void *)(HIDMA_MSI_CAP | HIDMA_IDENTITY_CAP),},
	{},
}; 

I can change the return type to (const void*) if Rob, Rafael and Sakari agrees.
I didn't really like converting a long to const void*. That's my personal opinion.

> 
> test.c: In function ?main?:
> test.c:15:2: error: assignment of member ?m? in read-only object
>   t->m = 3;
>   ^
> 
> struct test
> {
>         int m;
> };
> 
> int main(void)
> {
>         const void *ptr;
>         unsigned long l =4;
>         const struct test *t;
> 
>         ptr = (const void *)l;
> 
>         t = ptr;
>         t->m = 3;
> }
> 
> 
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V6 3/7] device property: Introduce a common API to fetch device match data

[Sakari Ailus]

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.

-- 
Sakari Ailus
sakari.ailus at linux.intel.com