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

[Paul Cercueil <paul@crapouillou.net>]

Hi Jonathan,

Le sam. 18 avril 2020 à 15:22, Jonathan Cameron <jic23@kernel.org> a 
écrit :
> On Sat, 18 Apr 2020 15:24:58 +0200
> Paul Cercueil <paul@crapouillou.net> wrote:
> 
>>  Le sam. 18 avril 2020 à 15:42, Andy Shevchenko
>>  <andy.shevchenko@gmail.com> a écrit :
>>  > On Sat, Apr 18, 2020 at 3:10 PM Paul Cercueil 
>> <paul@crapouillou.net>
>>  > wrote:
>>  >>  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.
>>  >
>>  > 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
> 
> Look up PRP0001 ACPI ID.  Magic trick ;)
> 
> https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html?highlight=PRP0001
> 
> It allows you to define an ACPI device in DSDT that is instantiated
> from what is effectively the DT binding including the id table.

So what you're saying, is that the OF table should be present, even 
though CONFIG_OF is not set, just in case it is probed from ACPI?

-Paul