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.
> > 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.
> >> >> 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.
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.
--
With Best Regards,
Andy Shevchenko
