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

From: Paul Cercueil
Date: Sat Apr 18 2020 - 08:11:11 EST




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.

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().


>> 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>.

-Paul