| Message ID | 1512493493-6464-5-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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>
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
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.
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 >
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; }
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; > } > > >
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);
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(+)