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
