Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
From: Andy Shevchenko
Date: Sat Apr 18 2020 - 07:57:46 EST
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.
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?
> >> 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.
--
With Best Regards,
Andy Shevchenko