Messages in this thread | | | Date | Sat, 18 Apr 2020 15:24:58 +0200 | From | Paul Cercueil <> | Subject | Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver. |
| |
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
| |