On Sat, 18 Apr 2020 19:25:15 +0200
Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:
> Hi Jonathan,
>
> Le sam. 18 avril 2020 à 15:22, Jonathan Cameron <jic23@xxxxxxxxxx> a
> écrit :
> > On Sat, 18 Apr 2020 15:24:58 +0200
> > Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:
> >
> >> 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
> >
> > 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?
Exactly. Weird isn't it :)
>
> -Paul
>
>
