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

Paul Cercueil
2020-04-18T12:10:33Z

Le sam. 18 avril 2020 à 14:57, Andy Shevchenko
<andy.shevchenko@gmail.com> a écrit :
> On Sat, Apr 18, 2020 at 1:48 AM Paul Cercueil <paul@crapouillou.net>
> wrote:
>>
>>
>>
>> Le sam. 18 avril 2020 à 0:49, Andy Shevchenko
>> <andy.shevchenko@gmail.com> a écrit :
>> > On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil
>> <paul@crapouillou.net>
>> > wrote:
>> >> Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
>> >> <andy.shevchenko@gmail.com> a écrit :
>> >> > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>> >> <contact@artur-rojek.eu>
>> >> > 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