Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.

From: Paul Cercueil
Date: Sat Apr 18 2020 - 13:25:31 EST

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?

-Paul