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

Paul Cercueil <paul@xxxxxxxxxxxxxxx> · Sat, 18 Apr 2020 14:10:12 +0200

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