diff mbox series

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

Message ID 20200417202859.35427-5-contact@artur-rojek.eu (mailing list archive)
State New, archived
Headers show
Series [RESEND,v5,1/5] IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx | expand

Commit Message

Artur Rojek April 17, 2020, 8:28 p.m. UTC
Add a driver for joystick devices connected to ADC controllers
supporting the Industrial I/O subsystem.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Tested-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

 v2: - sanity check supported channel format on probe,
     - rename adc_joystick_disable to a more sensible adc_joystick_cleanup, 
     - enforce correct axis order by checking the `reg` property of
       child nodes

 v3-v5: no change

 drivers/input/joystick/Kconfig        |  10 ++
 drivers/input/joystick/Makefile       |   1 +
 drivers/input/joystick/adc-joystick.c | 245 ++++++++++++++++++++++++++
 3 files changed, 256 insertions(+)
 create mode 100644 drivers/input/joystick/adc-joystick.c

Comments

Andy Shevchenko April 17, 2020, 9:10 p.m. UTC | #1
On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <contact@artur-rojek.eu> wrote:
>
> Add a driver for joystick devices connected to ADC controllers
> supporting the Industrial I/O subsystem.

...

> +#include <linux/of.h>

Do you really need this? (See below as well)

...

> +               sign = (tolower(joy->chans[i].channel->scan_type.sign) == 's');

Too many parentheses. But here it's up to you,

...

> +               case 2:

> +                       val = ((const u16 *)data)[i];

Can't you do this in each branch below?

> +                       if (endianness == IIO_BE)
> +                               val = be16_to_cpu(val);
> +                       else if (endianness == IIO_LE)
> +                               val = le16_to_cpu(val);
> +                       break;

...

> +       device_for_each_child_node(dev, child) {
> +               ret = fwnode_property_read_u32(child, "reg", &i);
> +               if (ret || i >= num_axes) {
> +                       dev_err(dev, "reg invalid or missing");
> +                       goto err;
> +               }
> +
> +               if (fwnode_property_read_u32(child, "linux,code",
> +                                            &axes[i].code)) {
> +                       dev_err(dev, "linux,code invalid or missing");
> +                       goto err;
> +               }
> +
> +               if (fwnode_property_read_u32_array(child, "abs-range",
> +                                                  axes[i].range, 2)) {
> +                       dev_err(dev, "abs-range invalid or missing");
> +                       goto err;
> +               }

> +       }
> +
> +       joy->axes = axes;
> +
> +       return 0;
> +
> +err:
> +       fwnode_handle_put(child);

> +       return -EINVAL;

Can we avoid shadowing the actual error code?

...

> +       bits = joy->chans[0].channel->scan_type.storagebits;

> +       if (!bits || (bits >> 3) > 2) {

Wouldn't be clear to use simple 'bits > 16'?

> +               dev_err(dev, "Unsupported channel storage size");
> +               return -EINVAL;
> +       }

...

> +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.
On top of that, you are using device property API, OF use in this case
is contradictory (at lest to some extend).

> +       },
> +       .probe = adc_joystick_probe,
> +};
Paul Cercueil April 17, 2020, 9:23 p.m. UTC | #2
Hi Andy,

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:
>> 
>>  Add a driver for joystick devices connected to ADC controllers
>>  supporting the Industrial I/O subsystem.
> 
> ...
> 
>>  +#include <linux/of.h>
> 
> Do you really need this? (See below as well)
> 
> ...
> 
>>  +               sign = 
>> (tolower(joy->chans[i].channel->scan_type.sign) == 's');
> 
> Too many parentheses. But here it's up to you,
> 
> ...
> 
>>  +               case 2:
> 
>>  +                       val = ((const u16 *)data)[i];
> 
> Can't you do this in each branch below?
> 
>>  +                       if (endianness == IIO_BE)
>>  +                               val = be16_to_cpu(val);
>>  +                       else if (endianness == IIO_LE)
>>  +                               val = le16_to_cpu(val);
>>  +                       break;
> 
> ...
> 
>>  +       device_for_each_child_node(dev, child) {
>>  +               ret = fwnode_property_read_u32(child, "reg", &i);
>>  +               if (ret || i >= num_axes) {
>>  +                       dev_err(dev, "reg invalid or missing");
>>  +                       goto err;
>>  +               }
>>  +
>>  +               if (fwnode_property_read_u32(child, "linux,code",
>>  +                                            &axes[i].code)) {
>>  +                       dev_err(dev, "linux,code invalid or 
>> missing");
>>  +                       goto err;
>>  +               }
>>  +
>>  +               if (fwnode_property_read_u32_array(child, 
>> "abs-range",
>>  +                                                  axes[i].range, 
>> 2)) {
>>  +                       dev_err(dev, "abs-range invalid or 
>> missing");
>>  +                       goto err;
>>  +               }
> 
>>  +       }
>>  +
>>  +       joy->axes = axes;
>>  +
>>  +       return 0;
>>  +
>>  +err:
>>  +       fwnode_handle_put(child);
> 
>>  +       return -EINVAL;
> 
> Can we avoid shadowing the actual error code?
> 
> ...
> 
>>  +       bits = joy->chans[0].channel->scan_type.storagebits;
> 
>>  +       if (!bits || (bits >> 3) > 2) {
> 
> Wouldn't be clear to use simple 'bits > 16'?
> 
>>  +               dev_err(dev, "Unsupported channel storage size");
>>  +               return -EINVAL;
>>  +       }
> 
> ...
> 
>>  +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?

> 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, doesn't mean that it shouldn't have a table to probe 
from devicetree.

-Paul

> 
>>  +       },
>>  +       .probe = adc_joystick_probe,
>>  +};
> 
> --
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko April 17, 2020, 9:49 p.m. UTC | #3
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.

> 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).
Paul Cercueil April 17, 2020, 10:48 p.m. UTC | #4
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.

>>  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.

-Paul

> 
> --
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko April 18, 2020, 11:57 a.m. UTC | #5
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.

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.
Paul Cercueil April 18, 2020, 12:10 p.m. UTC | #6
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.

> 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
Andy Shevchenko April 18, 2020, 12:42 p.m. UTC | #7
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.

> > 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.

> >>  >>  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.
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.
Paul Cercueil April 18, 2020, 1:24 p.m. UTC | #8
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
Jonathan Cameron April 18, 2020, 2:22 p.m. UTC | #9
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.

Jonathan

> 
>
Paul Cercueil April 18, 2020, 5:25 p.m. UTC | #10
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
Jonathan Cameron April 18, 2020, 6:20 p.m. UTC | #11
On Sat, 18 Apr 2020 19:25:15 +0200
Paul Cercueil <paul@crapouillou.net> wrote:

> 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?

Exactly.  Weird isn't it :)



> 
> -Paul
> 
>
diff mbox series

Patch

diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index 940b744639c7..efbc20ec5099 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -42,6 +42,16 @@  config JOYSTICK_A3D
 	  To compile this driver as a module, choose M here: the
 	  module will be called a3d.
 
+config JOYSTICK_ADC
+	tristate "Simple joystick connected over ADC"
+	depends on IIO
+	select IIO_BUFFER_CB
+	help
+	  Say Y here if you have a simple joystick connected over ADC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called adc-joystick.
+
 config JOYSTICK_ADI
 	tristate "Logitech ADI digital joysticks and gamepads"
 	select GAMEPORT
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 8656023f6ef5..58232b3057d3 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -6,6 +6,7 @@ 
 # Each configuration option enables a list of files.
 
 obj-$(CONFIG_JOYSTICK_A3D)		+= a3d.o
+obj-$(CONFIG_JOYSTICK_ADC)		+= adc-joystick.o
 obj-$(CONFIG_JOYSTICK_ADI)		+= adi.o
 obj-$(CONFIG_JOYSTICK_AMIGA)		+= amijoy.o
 obj-$(CONFIG_JOYSTICK_AS5011)		+= as5011.o
diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
new file mode 100644
index 000000000000..9cb9896da26e
--- /dev/null
+++ b/drivers/input/joystick/adc-joystick.c
@@ -0,0 +1,245 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Input driver for joysticks connected over ADC.
+ * Copyright (c) 2019-2020 Artur Rojek <contact@artur-rojek.eu>
+ */
+#include <linux/ctype.h>
+#include <linux/input.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+struct adc_joystick_axis {
+	u32 code;
+	s32 range[2];
+	s32 fuzz;
+	s32 flat;
+};
+
+struct adc_joystick {
+	struct input_dev *input;
+	struct iio_cb_buffer *buffer;
+	struct adc_joystick_axis *axes;
+	struct iio_channel *chans;
+	int num_chans;
+};
+
+static int adc_joystick_handle(const void *data, void *private)
+{
+	struct adc_joystick *joy = private;
+	enum iio_endian endianness;
+	int bytes, msb, val, i;
+	bool sign;
+
+	bytes = joy->chans[0].channel->scan_type.storagebits >> 3;
+
+	for (i = 0; i < joy->num_chans; ++i) {
+		endianness = joy->chans[i].channel->scan_type.endianness;
+		msb = joy->chans[i].channel->scan_type.realbits - 1;
+		sign = (tolower(joy->chans[i].channel->scan_type.sign) == 's');
+
+		switch (bytes) {
+		case 1:
+			val = ((const u8 *)data)[i];
+			break;
+		case 2:
+			val = ((const u16 *)data)[i];
+			if (endianness == IIO_BE)
+				val = be16_to_cpu(val);
+			else if (endianness == IIO_LE)
+				val = le16_to_cpu(val);
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		val >>= joy->chans[i].channel->scan_type.shift;
+		if (sign)
+			val = sign_extend32(val, msb);
+		else
+			val &= GENMASK(msb, 0);
+		input_report_abs(joy->input, joy->axes[i].code, val);
+	}
+
+	input_sync(joy->input);
+
+	return 0;
+}
+
+static int adc_joystick_open(struct input_dev *dev)
+{
+	struct adc_joystick *joy = input_get_drvdata(dev);
+	int ret;
+
+	ret = iio_channel_start_all_cb(joy->buffer);
+	if (ret)
+		dev_err(dev->dev.parent, "Unable to start callback buffer");
+
+	return ret;
+}
+
+static void adc_joystick_close(struct input_dev *dev)
+{
+	struct adc_joystick *joy = input_get_drvdata(dev);
+
+	iio_channel_stop_all_cb(joy->buffer);
+}
+
+static void adc_joystick_cleanup(void *data)
+{
+	iio_channel_release_all_cb(data);
+}
+
+static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy)
+{
+	struct adc_joystick_axis *axes;
+	struct fwnode_handle *child;
+	int num_axes, ret, i;
+
+	num_axes = device_get_child_node_count(dev);
+	if (!num_axes) {
+		dev_err(dev, "Unable to find child nodes");
+		return -EINVAL;
+	}
+
+	if (num_axes != joy->num_chans) {
+		dev_err(dev, "Got %d child nodes for %d channels",
+			num_axes, joy->num_chans);
+		return -EINVAL;
+	}
+
+	axes = devm_kmalloc_array(dev, num_axes, sizeof(*axes), GFP_KERNEL);
+	if (!axes)
+		return -ENOMEM;
+
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &i);
+		if (ret || i >= num_axes) {
+			dev_err(dev, "reg invalid or missing");
+			goto err;
+		}
+
+		if (fwnode_property_read_u32(child, "linux,code",
+					     &axes[i].code)) {
+			dev_err(dev, "linux,code invalid or missing");
+			goto err;
+		}
+
+		if (fwnode_property_read_u32_array(child, "abs-range",
+						   axes[i].range, 2)) {
+			dev_err(dev, "abs-range invalid or missing");
+			goto err;
+		}
+
+		fwnode_property_read_u32(child, "abs-fuzz",
+					 &axes[i].fuzz);
+		fwnode_property_read_u32(child, "abs-flat",
+					 &axes[i].flat);
+
+		input_set_abs_params(joy->input, axes[i].code,
+				     axes[i].range[0], axes[i].range[1],
+				     axes[i].fuzz,
+				     axes[i].flat);
+		input_set_capability(joy->input, EV_ABS, axes[i].code);
+	}
+
+	joy->axes = axes;
+
+	return 0;
+
+err:
+	fwnode_handle_put(child);
+	return -EINVAL;
+}
+
+static int adc_joystick_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct adc_joystick *joy;
+	struct input_dev *input;
+	int bits, ret, i;
+
+	joy = devm_kzalloc(dev, sizeof(*joy), GFP_KERNEL);
+	if (!joy)
+		return -ENOMEM;
+
+	joy->chans = devm_iio_channel_get_all(dev);
+	if (IS_ERR(joy->chans)) {
+		ret = PTR_ERR(joy->chans);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Unable to get IIO channels");
+		return ret;
+	}
+
+	/* Count how many channels we got. NULL terminated. */
+	while (joy->chans[joy->num_chans].indio_dev)
+		joy->num_chans++;
+
+	bits = joy->chans[0].channel->scan_type.storagebits;
+	if (!bits || (bits >> 3) > 2) {
+		dev_err(dev, "Unsupported channel storage size");
+		return -EINVAL;
+	}
+	for (i = 1; i < joy->num_chans; ++i)
+		if (joy->chans[i].channel->scan_type.storagebits != bits) {
+			dev_err(dev, "Channels must have equal storage size");
+			return -EINVAL;
+		}
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(dev, "Unable to allocate input device");
+		return -ENOMEM;
+	}
+
+	joy->input = input;
+	input->name = pdev->name;
+	input->id.bustype = BUS_HOST;
+	input->open = adc_joystick_open;
+	input->close = adc_joystick_close;
+
+	ret = adc_joystick_set_axes(dev, joy);
+	if (ret)
+		return ret;
+
+	input_set_drvdata(input, joy);
+	ret = input_register_device(input);
+	if (ret) {
+		dev_err(dev, "Unable to register input device: %d", ret);
+		return ret;
+	}
+
+	joy->buffer = iio_channel_get_all_cb(dev, adc_joystick_handle, joy);
+	if (IS_ERR(joy->buffer)) {
+		dev_err(dev, "Unable to allocate callback buffer");
+		return PTR_ERR(joy->buffer);
+	}
+
+	ret = devm_add_action_or_reset(dev, adc_joystick_cleanup, joy->buffer);
+	if (ret)
+		dev_err(dev, "Unable to add action");
+
+	return ret;
+}
+
+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),
+	},
+	.probe = adc_joystick_probe,
+};
+module_platform_driver(adc_joystick_driver);
+
+MODULE_DESCRIPTION("Input driver for joysticks connected over ADC");
+MODULE_AUTHOR("Artur Rojek <contact@artur-rojek.eu>");
+MODULE_LICENSE("GPL");