Le sam. 18 avril 2020 à 15:42, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> a écrit :
On Sat, Apr 18, 2020 at 3:10 PM Paul Cercueil <paul@xxxxxxxxxxxxxxx>
wrote:
Le sam. 18 avril 2020 à 14:57, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> a écrit :
> On Sat, Apr 18, 2020 at 1:48 AM Paul Cercueil
<paul@xxxxxxxxxxxxxxx>
> wrote:
>> Le sam. 18 avril 2020 à 0:49, Andy Shevchenko
>> <andy.shevchenko@xxxxxxxxx> a écrit :
>> > On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil
>> <paul@xxxxxxxxxxxxxxx>
>> > wrote:
>> >> Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
>> >> <andy.shevchenko@xxxxxxxxx> a écrit :
>> >> > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>> >> <contact@xxxxxxxxxxxxxx>
>> >> > 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
