Messages in this thread | | | Date | Sat, 18 Apr 2020 19:25:15 +0200 | From | Paul Cercueil <> | Subject | Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver. |
| |
Hi Jonathan,
Le sam. 18 avril 2020 à 15:22, Jonathan Cameron <jic23@kernel.org> a écrit : > On Sat, 18 Apr 2020 15:24:58 +0200 > Paul Cercueil <paul@crapouillou.net> wrote: > >> Le sam. 18 avril 2020 à 15:42, Andy Shevchenko >> <andy.shevchenko@gmail.com> a écrit : >> > On Sat, Apr 18, 2020 at 3:10 PM Paul Cercueil >> <paul@crapouillou.net> >> > wrote: >> >> Le sam. 18 avril 2020 à 14:57, Andy Shevchenko >> >> <andy.shevchenko@gmail.com> a écrit : >> >> > On Sat, Apr 18, 2020 at 1:48 AM Paul Cercueil >> >> <paul@crapouillou.net> >> >> > wrote: >> >> >> Le sam. 18 avril 2020 à 0:49, Andy Shevchenko >> >> >> <andy.shevchenko@gmail.com> a écrit : >> >> >> > On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil >> >> >> <paul@crapouillou.net> >> >> >> > wrote: >> >> >> >> Le sam. 18 avril 2020 à 0:10, Andy Shevchenko >> >> >> >> <andy.shevchenko@gmail.com> a écrit : >> >> >> >> > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek >> >> >> >> <contact@artur-rojek.eu> >> >> >> >> > wrote: >> > >> > ... >> > >> >> >> >> >> +#include <linux/of.h> >> >> >> >> > >> >> >> >> > Do you really need this? (See below as well) >> >> >> > >> >> >> >> >> +static const struct of_device_id >> >> adc_joystick_of_match[] = >> >> >> { >> >> >> >> >> + { .compatible = "adc-joystick", }, >> >> >> >> >> + { }, >> >> >> >> >> +}; >> >> >> >> >> +MODULE_DEVICE_TABLE(of, adc_joystick_of_match); >> >> >> >> >> + >> >> >> >> >> +static struct platform_driver adc_joystick_driver >> = { >> >> >> >> >> + .driver = { >> >> >> >> >> + .name = "adc-joystick", >> >> >> >> > >> >> >> >> >> + .of_match_table = >> >> >> >> >> of_match_ptr(adc_joystick_of_match), >> >> >> >> > >> >> >> >> > Drop this a bit harmful of_match_ptr() macro. It >> should go >> >> >> with >> >> >> >> ugly >> >> >> >> > #ifdeffery. Here you simple introduced a compiler >> warning. >> >> >> >> >> >> >> >> I assume you mean #ifdef around the of_device_id + >> module >> >> table >> >> >> >> macro? >> >> >> > >> >> >> > Yes. >> >> >> > >> >> >> >> > On top of that, you are using device property API, OF >> use >> >> in >> >> >> this >> >> >> >> case >> >> >> >> > is contradictory (at lest to some extend). >> >> >> >> >> >> >> >> I don't see why. The fact that the driver can work when >> >> probed >> >> >> from >> >> >> >> platform code >> >> >> > >> >> >> > Ha-ha, tell me how. I would like to be very surprised. >> >> >> >> >> >> iio_map_array_register(), >> >> >> pinctrl_register_mappings(), >> >> >> platform_add_devices(), >> >> >> >> >> >> you're welcome. >> >> > >> >> > I think above has no relation to what I'm talking about. >> >> >> >> Yes it does. It allows you to map the IIO channels, set the >> pinctrl >> >> configurations and register a device from platform code instead >> of >> >> devicetree. >> > >> > I'm not talking about other drivers, I'm talking about this >> driver and >> > how it will be instantiated. Above, according to the code, can't >> be >> > comprehensive to fulfill this. >> >> This is how the platform devices were instanciated on JZ4740 before >> we >> switched everything to devicetree. >> >> >> > How *this* driver can work as a platform instantiated one? >> >> > We seems have a conceptual misunderstanding here. >> >> > >> >> > For example, how can probe of this driver not fail, if it is >> not >> >> > backed by a DT/ACPI properties? >> >> >> >> platform_device_add_properties(). >> > >> > Yes, I waited for this. And seems you don't understand the (scope >> of) >> > API, you are trying to insist this driver can be used as a >> platform >> > one. >> > Sorry, I must to disappoint you, it can't. Above interface is >> created >> > solely for quirks to support (broken) DT/ACPI tables. It's not >> > supposed to be used as a main source for the device properties. >> >> The fact that it was designed for something else doesn't mean it >> can't >> be used. >> >> Anyway, this discussion is pointless. I don't think anybody would >> want >> to do that. >> >> >> >> >> doesn't mean that it shouldn't have a table to probe >> >> >> >> from devicetree. >> >> >> > >> >> >> > I didn't get what you are talking about here. The idea of >> >> >> _unified_ >> >> >> > device property API is to get rid of OF-centric code in >> >> favour of >> >> >> more >> >> >> > generic approach. Mixing those two can be done only in >> >> specific >> >> >> cases >> >> >> > (here is not the one). >> >> >> >> >> >> And how are we mixing those two here? The only OF-centric >> thing >> >> >> here is >> >> >> the device table, which is required if we want the driver to >> >> probe >> >> >> from >> >> >> devicetree. >> >> > >> >> > Table is fine(JFYI the types and sections are defined outside >> of >> >> OF >> >> > stuff, though being [heavily] used by it) , API >> (of_match_ptr() >> >> macro >> >> > use) is not. >> >> >> >> Sorry, but that's just stupid. Please have a look at how >> >> of_match_ptr() >> >> macro is defined in <linux/of.h>. >> > >> > Call it whatever you want, but above code is broken. >> >> of_match_ptr() is basically defined like this: >> >> #ifdef CONFIG_OF >> #define of_match_ptr(x) (x) >> #else >> #define of_match_ptr(x) NULL >> #endif >> >> So please, enlighten me, tell me what is so wrong about what's being >> done here. >> >> > It needs either of: >> > - ugly ifdeffery >> > - dropping of_match_ptr() >> > - explicit dependence to OF >> > >> > My choice is second one. Because it makes code better and allows >> also >> > ACPI to use this driver (usually) without changes. >> >> And how is unconditionally compiling the of_match_table make it >> magically probe from ACPI, without a acpi_match_table? >> >> -Paul > > Look up PRP0001 ACPI ID. Magic trick ;) > > https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html?highlight=PRP0001 > > It allows you to define an ACPI device in DSDT that is instantiated > from what is effectively the DT binding including the id table.
So what you're saying, is that the OF table should be present, even though CONFIG_OF is not set, just in case it is probed from ACPI?
-Paul
| |