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

[Paul Cercueil]

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