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 <[email protected]>
---
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
On Tue, Dec 5, 2017 at 11:04 AM, Sinan Kaya <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> + 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
>
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Kind regards,
Sakari Ailus
e-mail: [email protected]
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 <[email protected]>
> ---
> 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
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.
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
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.
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
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
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.
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.