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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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






[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux