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

[Jonathan Cameron]

On Sat, 18 Apr 2020 19:25:15 +0200
Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:

> Hi Jonathan,
> 
> Le sam. 18 avril 2020 Ã 15:22, Jonathan Cameron <jic23@xxxxxxxxxx> a 
> Ãcrit :
> > On Sat, 18 Apr 2020 15:24:58 +0200
> > Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:
> >   
> >>  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  
> > 
> > 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?

Exactly.  Weird isn't it :)



> 
> -Paul
> 
>