[PATCH v3 0/3] adc-joystick: Add polled support

[PATCH v3 0/3] adc-joystick: Add polled support

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Add support to the existing adc-joystick driver to support polling
rather than relying on triggered buffers. This is useful for devices
that do not offer triggered buffers in hardware. Code adapted from
changes made by Maya Matuszczyk <maccraft123mc@gmail.com>.

Changes from V2:
 - Changed parameter from "adc-joystick,polled" to
   "adc-joystick,no-hardware-trigger" as it is more representative of
   what the driver and hardware are doing.

Changes from V1:
 - Removed driver compatible string of "adc-joystick-polled".
 - Added new optional boolean value of "adc-joystick,polled".
 - Cleaned up if statements regarding polling behavior.


Chris Morgan (3):
  dt-bindings: adc-joystick: add adc-joystick,no-hardware-trigger
  Input: adc-joystick - Add polled input device support
  arm64: dts: rockchip: Update joystick to polled for Odroid-Go2

 .../bindings/input/adc-joystick.yaml          |  9 +++-
 .../boot/dts/rockchip/rk3326-odroid-go2.dts   |  1 +
 drivers/input/joystick/adc-joystick.c         | 52 +++++++++++++++----
 3 files changed, 50 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] dt-bindings: adc-joystick: add adc-joystick,no-hardware-trigger

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Add documentation for adc-joystick,no-hardware-trigger. New device-tree
properties have been added.

- adc-joystick,no-hardware-trigger: A boolean value noting the joystick
				    device should be polled rather than
				    use a triggered buffer.

Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 .../devicetree/bindings/input/adc-joystick.yaml          | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/adc-joystick.yaml b/Documentation/devicetree/bindings/input/adc-joystick.yaml
index 2ee04e03bc22..627cc6c40191 100644
--- a/Documentation/devicetree/bindings/input/adc-joystick.yaml
+++ b/Documentation/devicetree/bindings/input/adc-joystick.yaml
@@ -12,12 +12,19 @@ maintainers:
 
 description: >
   Bindings for joystick devices connected to ADC controllers supporting
-  the Industrial I/O subsystem.
+  the Industrial I/O subsystem. Supports both polled devices where no
+  iio trigger is available and non-polled devices which are triggered
+  by iio.
 
 properties:
   compatible:
     const: adc-joystick
 
+  adc-joystick,no-hardware-trigger:
+    type: boolean
+    description:
+      If the device does not support triggered buffers and needs to be polled.
+
   io-channels:
     minItems: 1
     maxItems: 1024
-- 
2.25.1

Re: [PATCH v3 1/3] dt-bindings: adc-joystick: add adc-joystick,no-hardware-trigger

[Artur Rojek]

On 2022-06-13 21:23, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add documentation for adc-joystick,no-hardware-trigger. New device-tree
> properties have been added.
> 
> - adc-joystick,no-hardware-trigger: A boolean value noting the joystick
> 				    device should be polled rather than
> 				    use a triggered buffer.
> 
> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> ---
>  .../devicetree/bindings/input/adc-joystick.yaml          | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/adc-joystick.yaml
> b/Documentation/devicetree/bindings/input/adc-joystick.yaml
> index 2ee04e03bc22..627cc6c40191 100644
> --- a/Documentation/devicetree/bindings/input/adc-joystick.yaml
> +++ b/Documentation/devicetree/bindings/input/adc-joystick.yaml
> @@ -12,12 +12,19 @@ maintainers:
> 
>  description: >
>    Bindings for joystick devices connected to ADC controllers 
> supporting
> -  the Industrial I/O subsystem.
> +  the Industrial I/O subsystem. Supports both polled devices where no
> +  iio trigger is available and non-polled devices which are triggered
> +  by iio.
> 
>  properties:
>    compatible:
>      const: adc-joystick
> 
> +  adc-joystick,no-hardware-trigger:
I'm against using Device Tree for this functionality. See my reply to 
patch 2/3 for details.
But in case we do end up going DT way, I would much prefer going with 
Rob's suggestion of using the existing `poll-interval` input property.

Cheers,
Artur
> +    type: boolean
> +    description:
> +      If the device does not support triggered buffers and needs to be 
> polled.
> +
>    io-channels:
>      minItems: 1
>      maxItems: 1024

Re: [PATCH v3 1/3] dt-bindings: adc-joystick: add adc-joystick,no-hardware-trigger

[Krzysztof Kozlowski]

On 14/06/2022 18:50, Artur Rojek wrote:
> On 2022-06-13 21:23, Chris Morgan wrote:
>> From: Chris Morgan <macromorgan@hotmail.com>
>>
>> Add documentation for adc-joystick,no-hardware-trigger. New device-tree
>> properties have been added.
>>
>> - adc-joystick,no-hardware-trigger: A boolean value noting the joystick
>> 				    device should be polled rather than
>> 				    use a triggered buffer.
>>
>> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
>> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
>> ---
>>  .../devicetree/bindings/input/adc-joystick.yaml          | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/input/adc-joystick.yaml
>> b/Documentation/devicetree/bindings/input/adc-joystick.yaml
>> index 2ee04e03bc22..627cc6c40191 100644
>> --- a/Documentation/devicetree/bindings/input/adc-joystick.yaml
>> +++ b/Documentation/devicetree/bindings/input/adc-joystick.yaml
>> @@ -12,12 +12,19 @@ maintainers:
>>
>>  description: >
>>    Bindings for joystick devices connected to ADC controllers 
>> supporting
>> -  the Industrial I/O subsystem.
>> +  the Industrial I/O subsystem. Supports both polled devices where no
>> +  iio trigger is available and non-polled devices which are triggered
>> +  by iio.
>>
>>  properties:
>>    compatible:
>>      const: adc-joystick
>>
>> +  adc-joystick,no-hardware-trigger:
> I'm against using Device Tree for this functionality. See my reply to 
> patch 2/3 for details.

I am surprised to see v3 after that comment...

> But in case we do end up going DT way, I would much prefer going with 
> Rob's suggestion of using the existing `poll-interval` input property.

+1 here as well, if above does not work.


Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: adc-joystick: add adc-joystick,no-hardware-trigger

[Chris Morgan]

On Wed, Jun 15, 2022 at 10:23:55AM -0700, Krzysztof Kozlowski wrote:
> On 14/06/2022 18:50, Artur Rojek wrote:
> > On 2022-06-13 21:23, Chris Morgan wrote:
> >> From: Chris Morgan <macromorgan@hotmail.com>
> >>
> >> Add documentation for adc-joystick,no-hardware-trigger. New device-tree
> >> properties have been added.
> >>
> >> - adc-joystick,no-hardware-trigger: A boolean value noting the joystick
> >> 				    device should be polled rather than
> >> 				    use a triggered buffer.
> >>
> >> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> >> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> >> ---
> >>  .../devicetree/bindings/input/adc-joystick.yaml          | 9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/input/adc-joystick.yaml
> >> b/Documentation/devicetree/bindings/input/adc-joystick.yaml
> >> index 2ee04e03bc22..627cc6c40191 100644
> >> --- a/Documentation/devicetree/bindings/input/adc-joystick.yaml
> >> +++ b/Documentation/devicetree/bindings/input/adc-joystick.yaml
> >> @@ -12,12 +12,19 @@ maintainers:
> >>
> >>  description: >
> >>    Bindings for joystick devices connected to ADC controllers 
> >> supporting
> >> -  the Industrial I/O subsystem.
> >> +  the Industrial I/O subsystem. Supports both polled devices where no
> >> +  iio trigger is available and non-polled devices which are triggered
> >> +  by iio.
> >>
> >>  properties:
> >>    compatible:
> >>      const: adc-joystick
> >>
> >> +  adc-joystick,no-hardware-trigger:
> > I'm against using Device Tree for this functionality. See my reply to 
> > patch 2/3 for details.
> 
> I am surprised to see v3 after that comment...

No, I'm sorry I must have missed this comment or glossed over it when
updating a v3. I've tested checking for INDIO_ALL_BUFFER_MODES and that
doesn't seem to trigger the desired behavior (since I think maybe
something in my kernel config is adding INDIO_BUFFER_TRIGGERED, and I'd
hate for behavior to be dependent on what's in the config versus what's
in the hardware). Would it be sufficient to check for
"(INDIO_BUFFER_HARDWARE | INDIO_BUFFER_SOFTWARE)"?

Thank you.

> 
> > But in case we do end up going DT way, I would much prefer going with 
> > Rob's suggestion of using the existing `poll-interval` input property.
> 
> +1 here as well, if above does not work.
> 
> 
> Best regards,
> Krzysztof

[PATCH v3 2/3] Input: adc-joystick - Add polled input device support

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Add polled input device support to the adc-joystick driver. This is
useful for devices which do not have hardware capable triggers on
their SARADC. Code modified from adc-joystick.c changes made by Maya
Matuszczyk.

Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/input/joystick/adc-joystick.c | 52 +++++++++++++++++++++------
 1 file changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
index 78ebca7d400a..dc01cd0214d2 100644
--- a/drivers/input/joystick/adc-joystick.c
+++ b/drivers/input/joystick/adc-joystick.c
@@ -13,6 +13,10 @@
 
 #include <asm/unaligned.h>
 
+#define ADC_JSK_POLL_INTERVAL	16
+#define ADC_JSK_POLL_MIN	8
+#define ADC_JSK_POLL_MAX	32
+
 struct adc_joystick_axis {
 	u32 code;
 	s32 range[2];
@@ -26,8 +30,21 @@ struct adc_joystick {
 	struct adc_joystick_axis *axes;
 	struct iio_channel *chans;
 	int num_chans;
+	bool polled;
 };
 
+static void adc_joystick_poll(struct input_dev *input)
+{
+	struct adc_joystick *joy = input_get_drvdata(input);
+	int i, val;
+
+	for (i = 0; i < joy->num_chans; i++) {
+		iio_read_channel_raw(&joy->chans[i], &val);
+		input_report_abs(input, joy->axes[i].code, val);
+	}
+	input_sync(input);
+}
+
 static int adc_joystick_handle(const void *data, void *private)
 {
 	struct adc_joystick *joy = private;
@@ -215,8 +232,19 @@ static int adc_joystick_probe(struct platform_device *pdev)
 	joy->input = input;
 	input->name = pdev->name;
 	input->id.bustype = BUS_HOST;
-	input->open = adc_joystick_open;
-	input->close = adc_joystick_close;
+
+	if (device_property_read_bool(dev, "adc-joystick,no-hardware-trigger"))
+		joy->polled = 1;
+
+	if (joy->polled) {
+		input_setup_polling(input, adc_joystick_poll);
+		input_set_poll_interval(input, ADC_JSK_POLL_INTERVAL);
+		input_set_min_poll_interval(input, ADC_JSK_POLL_MIN);
+		input_set_max_poll_interval(input, ADC_JSK_POLL_MAX);
+	} else {
+		input->open = adc_joystick_open;
+		input->close = adc_joystick_close;
+	}
 
 	error = adc_joystick_set_axes(dev, joy);
 	if (error)
@@ -229,16 +257,18 @@ static int adc_joystick_probe(struct platform_device *pdev)
 		return error;
 	}
 
-	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\n");
-		return PTR_ERR(joy->buffer);
-	}
+	if (!joy->polled) {
+		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\n");
+			return PTR_ERR(joy->buffer);
+		}
 
-	error = devm_add_action_or_reset(dev, adc_joystick_cleanup, joy->buffer);
-	if (error)  {
-		dev_err(dev, "Unable to add action\n");
-		return error;
+		error = devm_add_action_or_reset(dev, adc_joystick_cleanup, joy->buffer);
+		if (error)  {
+			dev_err(dev, "Unable to add action\n");
+			return error;
+		}
 	}
 
 	return 0;
-- 
2.25.1

Re: [PATCH v3 2/3] Input: adc-joystick - Add polled input device support

[Artur Rojek]

On 2022-06-13 21:23, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add polled input device support to the adc-joystick driver. This is
> useful for devices which do not have hardware capable triggers on
> their SARADC. Code modified from adc-joystick.c changes made by Maya
> Matuszczyk.
> 
> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>

Hi Chris,

Comments inline. I also Cc'd Paul and Jonathan, who were attached in v2.

> ---
>  drivers/input/joystick/adc-joystick.c | 52 +++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/input/joystick/adc-joystick.c
> b/drivers/input/joystick/adc-joystick.c
> index 78ebca7d400a..dc01cd0214d2 100644
> --- a/drivers/input/joystick/adc-joystick.c
> +++ b/drivers/input/joystick/adc-joystick.c
> @@ -13,6 +13,10 @@
> 
>  #include <asm/unaligned.h>
> 
> +#define ADC_JSK_POLL_INTERVAL	16
> +#define ADC_JSK_POLL_MIN	8
> +#define ADC_JSK_POLL_MAX	32
> +
>  struct adc_joystick_axis {
>  	u32 code;
>  	s32 range[2];
> @@ -26,8 +30,21 @@ struct adc_joystick {
>  	struct adc_joystick_axis *axes;
>  	struct iio_channel *chans;
>  	int num_chans;
> +	bool polled;
>  };
> 
> +static void adc_joystick_poll(struct input_dev *input)
> +{
> +	struct adc_joystick *joy = input_get_drvdata(input);
> +	int i, val;
> +
> +	for (i = 0; i < joy->num_chans; i++) {
> +		iio_read_channel_raw(&joy->chans[i], &val);
> +		input_report_abs(input, joy->axes[i].code, val);
> +	}
> +	input_sync(input);
> +}
> +
>  static int adc_joystick_handle(const void *data, void *private)
>  {
>  	struct adc_joystick *joy = private;
> @@ -215,8 +232,19 @@ static int adc_joystick_probe(struct 
> platform_device *pdev)
>  	joy->input = input;
>  	input->name = pdev->name;
>  	input->id.bustype = BUS_HOST;
> -	input->open = adc_joystick_open;
> -	input->close = adc_joystick_close;
> +
> +	if (device_property_read_bool(dev, 
> "adc-joystick,no-hardware-trigger"))
> +		joy->polled = 1;
As mentioned in v2, I don't think a DT property is required here. 
Assuming the polled mode is a fallback for devices with no buffers, just 
do:
```
	joy->polled = !(joy->chans[0].indio_dev->modes &
			INDIO_ALL_BUFFER_MODES);
```
> +
> +	if (joy->polled) {
> +		input_setup_polling(input, adc_joystick_poll);
> +		input_set_poll_interval(input, ADC_JSK_POLL_INTERVAL);
> +		input_set_min_poll_interval(input, ADC_JSK_POLL_MIN);
> +		input_set_max_poll_interval(input, ADC_JSK_POLL_MAX);
> +	} else {
> +		input->open = adc_joystick_open;
> +		input->close = adc_joystick_close;
> +	}
> 
>  	error = adc_joystick_set_axes(dev, joy);
>  	if (error)
> @@ -229,16 +257,18 @@ static int adc_joystick_probe(struct
> platform_device *pdev)
>  		return error;
>  	}
> 
> -	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\n");
> -		return PTR_ERR(joy->buffer);
> -	}
> +	if (!joy->polled) {
> +		joy->buffer = iio_channel_get_all_cb(dev, adc_joystick_handle, joy);
Please maintain line discipline of 80 chars to stay consistent with the 
rest of this driver.
> +		if (IS_ERR(joy->buffer)) {
> +			dev_err(dev, "Unable to allocate callback buffer\n");
> +			return PTR_ERR(joy->buffer);
> +		}
> 
> -	error = devm_add_action_or_reset(dev, adc_joystick_cleanup, 
> joy->buffer);
> -	if (error)  {
> -		dev_err(dev, "Unable to add action\n");
> -		return error;
> +		error = devm_add_action_or_reset(dev, adc_joystick_cleanup, 
> joy->buffer);
Same here.

Cheers,
Artur
> +		if (error)  {
> +			dev_err(dev, "Unable to add action\n");
> +			return error;
> +		}
>  	}
> 
>  	return 0;

Re: [PATCH v3 2/3] Input: adc-joystick - Add polled input device support

[Chris Morgan]

On Wed, Jun 15, 2022 at 03:43:07AM +0200, Artur Rojek wrote:
> On 2022-06-13 21:23, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Add polled input device support to the adc-joystick driver. This is
> > useful for devices which do not have hardware capable triggers on
> > their SARADC. Code modified from adc-joystick.c changes made by Maya
> > Matuszczyk.
> > 
> > Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> 
> Hi Chris,
> 
> Comments inline. I also Cc'd Paul and Jonathan, who were attached in v2.
> 
> > ---
> >  drivers/input/joystick/adc-joystick.c | 52 +++++++++++++++++++++------
> >  1 file changed, 41 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/input/joystick/adc-joystick.c
> > b/drivers/input/joystick/adc-joystick.c
> > index 78ebca7d400a..dc01cd0214d2 100644
> > --- a/drivers/input/joystick/adc-joystick.c
> > +++ b/drivers/input/joystick/adc-joystick.c
> > @@ -13,6 +13,10 @@
> > 
> >  #include <asm/unaligned.h>
> > 
> > +#define ADC_JSK_POLL_INTERVAL	16
> > +#define ADC_JSK_POLL_MIN	8
> > +#define ADC_JSK_POLL_MAX	32
> > +
> >  struct adc_joystick_axis {
> >  	u32 code;
> >  	s32 range[2];
> > @@ -26,8 +30,21 @@ struct adc_joystick {
> >  	struct adc_joystick_axis *axes;
> >  	struct iio_channel *chans;
> >  	int num_chans;
> > +	bool polled;
> >  };
> > 
> > +static void adc_joystick_poll(struct input_dev *input)
> > +{
> > +	struct adc_joystick *joy = input_get_drvdata(input);
> > +	int i, val;
> > +
> > +	for (i = 0; i < joy->num_chans; i++) {
> > +		iio_read_channel_raw(&joy->chans[i], &val);
> > +		input_report_abs(input, joy->axes[i].code, val);
> > +	}
> > +	input_sync(input);
> > +}
> > +
> >  static int adc_joystick_handle(const void *data, void *private)
> >  {
> >  	struct adc_joystick *joy = private;
> > @@ -215,8 +232,19 @@ static int adc_joystick_probe(struct
> > platform_device *pdev)
> >  	joy->input = input;
> >  	input->name = pdev->name;
> >  	input->id.bustype = BUS_HOST;
> > -	input->open = adc_joystick_open;
> > -	input->close = adc_joystick_close;
> > +
> > +	if (device_property_read_bool(dev,
> > "adc-joystick,no-hardware-trigger"))
> > +		joy->polled = 1;
> As mentioned in v2, I don't think a DT property is required here. Assuming
> the polled mode is a fallback for devices with no buffers, just do:
> ```
> 	joy->polled = !(joy->chans[0].indio_dev->modes &
> 			INDIO_ALL_BUFFER_MODES);
> ```

Understood. I attempted this and noticed that it was showing I have
INDIO_BUFFER_TRIGGERED in addition to INDIO_DIRECT_MODE (the
INDIO_DIRECT_MODE is the only one specified at the hardware level
though). Should I just check for INDIO_BUFFER_SOFTWARE &
INDIO_BUFFER_HARDWARE instead? I think it's possible that the inclusion
of the industrialio_triggered_buffer module in my kernel is adding
this to the channel somehow?

Thank you.

> > +
> > +	if (joy->polled) {
> > +		input_setup_polling(input, adc_joystick_poll);
> > +		input_set_poll_interval(input, ADC_JSK_POLL_INTERVAL);
> > +		input_set_min_poll_interval(input, ADC_JSK_POLL_MIN);
> > +		input_set_max_poll_interval(input, ADC_JSK_POLL_MAX);
> > +	} else {
> > +		input->open = adc_joystick_open;
> > +		input->close = adc_joystick_close;
> > +	}
> > 
> >  	error = adc_joystick_set_axes(dev, joy);
> >  	if (error)
> > @@ -229,16 +257,18 @@ static int adc_joystick_probe(struct
> > platform_device *pdev)
> >  		return error;
> >  	}
> > 
> > -	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\n");
> > -		return PTR_ERR(joy->buffer);
> > -	}
> > +	if (!joy->polled) {
> > +		joy->buffer = iio_channel_get_all_cb(dev, adc_joystick_handle, joy);
> Please maintain line discipline of 80 chars to stay consistent with the rest
> of this driver.

Understood, sorry about that.

> > +		if (IS_ERR(joy->buffer)) {
> > +			dev_err(dev, "Unable to allocate callback buffer\n");
> > +			return PTR_ERR(joy->buffer);
> > +		}
> > 
> > -	error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
> > joy->buffer);
> > -	if (error)  {
> > -		dev_err(dev, "Unable to add action\n");
> > -		return error;
> > +		error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
> > joy->buffer);
> Same here.

Ditto.

> 
> Cheers,
> Artur
> > +		if (error)  {
> > +			dev_err(dev, "Unable to add action\n");
> > +			return error;
> > +		}
> >  	}
> > 
> >  	return 0;

Re: [PATCH v3 2/3] Input: adc-joystick - Add polled input device support

[Artur Rojek]

On 2022-06-15 17:12, Chris Morgan wrote:
> On Wed, Jun 15, 2022 at 03:43:07AM +0200, Artur Rojek wrote:
>> On 2022-06-13 21:23, Chris Morgan wrote:
>> > From: Chris Morgan <macromorgan@hotmail.com>
>> >
>> > Add polled input device support to the adc-joystick driver. This is
>> > useful for devices which do not have hardware capable triggers on
>> > their SARADC. Code modified from adc-joystick.c changes made by Maya
>> > Matuszczyk.
>> >
>> > Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
>> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
>> 
>> Hi Chris,
>> 
>> Comments inline. I also Cc'd Paul and Jonathan, who were attached in 
>> v2.
>> 
>> > ---
>> >  drivers/input/joystick/adc-joystick.c | 52 +++++++++++++++++++++------
>> >  1 file changed, 41 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/input/joystick/adc-joystick.c
>> > b/drivers/input/joystick/adc-joystick.c
>> > index 78ebca7d400a..dc01cd0214d2 100644
>> > --- a/drivers/input/joystick/adc-joystick.c
>> > +++ b/drivers/input/joystick/adc-joystick.c
>> > @@ -13,6 +13,10 @@
>> >
>> >  #include <asm/unaligned.h>
>> >
>> > +#define ADC_JSK_POLL_INTERVAL	16
>> > +#define ADC_JSK_POLL_MIN	8
>> > +#define ADC_JSK_POLL_MAX	32
>> > +
>> >  struct adc_joystick_axis {
>> >  	u32 code;
>> >  	s32 range[2];
>> > @@ -26,8 +30,21 @@ struct adc_joystick {
>> >  	struct adc_joystick_axis *axes;
>> >  	struct iio_channel *chans;
>> >  	int num_chans;
>> > +	bool polled;
>> >  };
>> >
>> > +static void adc_joystick_poll(struct input_dev *input)
>> > +{
>> > +	struct adc_joystick *joy = input_get_drvdata(input);
>> > +	int i, val;
>> > +
>> > +	for (i = 0; i < joy->num_chans; i++) {
>> > +		iio_read_channel_raw(&joy->chans[i], &val);
>> > +		input_report_abs(input, joy->axes[i].code, val);
>> > +	}
>> > +	input_sync(input);
>> > +}
>> > +
>> >  static int adc_joystick_handle(const void *data, void *private)
>> >  {
>> >  	struct adc_joystick *joy = private;
>> > @@ -215,8 +232,19 @@ static int adc_joystick_probe(struct
>> > platform_device *pdev)
>> >  	joy->input = input;
>> >  	input->name = pdev->name;
>> >  	input->id.bustype = BUS_HOST;
>> > -	input->open = adc_joystick_open;
>> > -	input->close = adc_joystick_close;
>> > +
>> > +	if (device_property_read_bool(dev,
>> > "adc-joystick,no-hardware-trigger"))
>> > +		joy->polled = 1;
>> As mentioned in v2, I don't think a DT property is required here. 
>> Assuming
>> the polled mode is a fallback for devices with no buffers, just do:
>> ```
>> 	joy->polled = !(joy->chans[0].indio_dev->modes &
>> 			INDIO_ALL_BUFFER_MODES);
>> ```
> 
> Understood. I attempted this and noticed that it was showing I have
> INDIO_BUFFER_TRIGGERED in addition to INDIO_DIRECT_MODE (the
> INDIO_DIRECT_MODE is the only one specified at the hardware level
> though). Should I just check for INDIO_BUFFER_SOFTWARE &
> INDIO_BUFFER_HARDWARE instead? I think it's possible that the inclusion
> of the industrialio_triggered_buffer module in my kernel is adding
> this to the channel somehow?
Having INDIO_BUFFER_TRIGGERED means that your saradc is capable of using 
the existing flow. You should be able to register a software trigger and 
use the adc-joystick driver without further issues.
That said, this is where it gets problematic - there is no way to create 
an IIO trigger via Device Tree, since triggers don't describe any piece 
of hardware, and you shouldn't need to register it at runtime 
(configfs/sysfs) for communication between two kernel drivers either. At 
the same time, it's not adc-joystick's job to register an external 
trigger.

Jonathan,
I don't know what the proper approach to this should be, perhaps you 
could assist?

Cheers,
Artur
> 
> Thank you.
> 
>> > +
>> > +	if (joy->polled) {
>> > +		input_setup_polling(input, adc_joystick_poll);
>> > +		input_set_poll_interval(input, ADC_JSK_POLL_INTERVAL);
>> > +		input_set_min_poll_interval(input, ADC_JSK_POLL_MIN);
>> > +		input_set_max_poll_interval(input, ADC_JSK_POLL_MAX);
>> > +	} else {
>> > +		input->open = adc_joystick_open;
>> > +		input->close = adc_joystick_close;
>> > +	}
>> >
>> >  	error = adc_joystick_set_axes(dev, joy);
>> >  	if (error)
>> > @@ -229,16 +257,18 @@ static int adc_joystick_probe(struct
>> > platform_device *pdev)
>> >  		return error;
>> >  	}
>> >
>> > -	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\n");
>> > -		return PTR_ERR(joy->buffer);
>> > -	}
>> > +	if (!joy->polled) {
>> > +		joy->buffer = iio_channel_get_all_cb(dev, adc_joystick_handle, joy);
>> Please maintain line discipline of 80 chars to stay consistent with 
>> the rest
>> of this driver.
> 
> Understood, sorry about that.
> 
>> > +		if (IS_ERR(joy->buffer)) {
>> > +			dev_err(dev, "Unable to allocate callback buffer\n");
>> > +			return PTR_ERR(joy->buffer);
>> > +		}
>> >
>> > -	error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
>> > joy->buffer);
>> > -	if (error)  {
>> > -		dev_err(dev, "Unable to add action\n");
>> > -		return error;
>> > +		error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
>> > joy->buffer);
>> Same here.
> 
> Ditto.
> 
>> 
>> Cheers,
>> Artur
>> > +		if (error)  {
>> > +			dev_err(dev, "Unable to add action\n");
>> > +			return error;
>> > +		}
>> >  	}
>> >
>> >  	return 0;

Re: [PATCH v3 2/3] Input: adc-joystick - Add polled input device support

[Jonathan Cameron]

On Sat, 18 Jun 2022 15:08:29 +0200
Artur Rojek <contact@artur-rojek.eu> wrote:

> On 2022-06-15 17:12, Chris Morgan wrote:
> > On Wed, Jun 15, 2022 at 03:43:07AM +0200, Artur Rojek wrote:  
> >> On 2022-06-13 21:23, Chris Morgan wrote:  
> >> > From: Chris Morgan <macromorgan@hotmail.com>
> >> >
> >> > Add polled input device support to the adc-joystick driver. This is
> >> > useful for devices which do not have hardware capable triggers on
> >> > their SARADC. Code modified from adc-joystick.c changes made by Maya
> >> > Matuszczyk.
> >> >
> >> > Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> >> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>  
> >> 
> >> Hi Chris,
> >> 
> >> Comments inline. I also Cc'd Paul and Jonathan, who were attached in 
> >> v2.
+CC linux-iio

> >>   
> >> > ---
> >> >  drivers/input/joystick/adc-joystick.c | 52 +++++++++++++++++++++------
> >> >  1 file changed, 41 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/drivers/input/joystick/adc-joystick.c
> >> > b/drivers/input/joystick/adc-joystick.c
> >> > index 78ebca7d400a..dc01cd0214d2 100644
> >> > --- a/drivers/input/joystick/adc-joystick.c
> >> > +++ b/drivers/input/joystick/adc-joystick.c
> >> > @@ -13,6 +13,10 @@
> >> >
> >> >  #include <asm/unaligned.h>
> >> >
> >> > +#define ADC_JSK_POLL_INTERVAL	16
> >> > +#define ADC_JSK_POLL_MIN	8
> >> > +#define ADC_JSK_POLL_MAX	32
> >> > +
> >> >  struct adc_joystick_axis {
> >> >  	u32 code;
> >> >  	s32 range[2];
> >> > @@ -26,8 +30,21 @@ struct adc_joystick {
> >> >  	struct adc_joystick_axis *axes;
> >> >  	struct iio_channel *chans;
> >> >  	int num_chans;
> >> > +	bool polled;
> >> >  };
> >> >
> >> > +static void adc_joystick_poll(struct input_dev *input)
> >> > +{
> >> > +	struct adc_joystick *joy = input_get_drvdata(input);
> >> > +	int i, val;
> >> > +
> >> > +	for (i = 0; i < joy->num_chans; i++) {
> >> > +		iio_read_channel_raw(&joy->chans[i], &val);
> >> > +		input_report_abs(input, joy->axes[i].code, val);
> >> > +	}
> >> > +	input_sync(input);
> >> > +}
> >> > +
> >> >  static int adc_joystick_handle(const void *data, void *private)
> >> >  {
> >> >  	struct adc_joystick *joy = private;
> >> > @@ -215,8 +232,19 @@ static int adc_joystick_probe(struct
> >> > platform_device *pdev)
> >> >  	joy->input = input;
> >> >  	input->name = pdev->name;
> >> >  	input->id.bustype = BUS_HOST;
> >> > -	input->open = adc_joystick_open;
> >> > -	input->close = adc_joystick_close;
> >> > +
> >> > +	if (device_property_read_bool(dev,
> >> > "adc-joystick,no-hardware-trigger"))
> >> > +		joy->polled = 1;  
> >> As mentioned in v2, I don't think a DT property is required here. 
> >> Assuming
> >> the polled mode is a fallback for devices with no buffers, just do:
> >> ```
> >> 	joy->polled = !(joy->chans[0].indio_dev->modes &
> >> 			INDIO_ALL_BUFFER_MODES);
> >> ```  
> > 
> > Understood. I attempted this and noticed that it was showing I have
> > INDIO_BUFFER_TRIGGERED in addition to INDIO_DIRECT_MODE (the
> > INDIO_DIRECT_MODE is the only one specified at the hardware level
> > though). Should I just check for INDIO_BUFFER_SOFTWARE &
> > INDIO_BUFFER_HARDWARE instead? I think it's possible that the inclusion
> > of the industrialio_triggered_buffer module in my kernel is adding
> > this to the channel somehow?  
> Having INDIO_BUFFER_TRIGGERED means that your saradc is capable of using 
> the existing flow. You should be able to register a software trigger and 
> use the adc-joystick driver without further issues.
> That said, this is where it gets problematic - there is no way to create 
> an IIO trigger via Device Tree, since triggers don't describe any piece 
> of hardware, and you shouldn't need to register it at runtime 
> (configfs/sysfs) for communication between two kernel drivers either. At 
> the same time, it's not adc-joystick's job to register an external 
> trigger.
> 
> Jonathan,
> I don't know what the proper approach to this should be, perhaps you 
> could assist?

You are correct in your description above. Device tree folk take the view
that sysfs / hrtimer etc triggers are a policy decision so don't belong
in device tree.  In general you need some userspace code to stitch up
the trigger anyway (even ADCs that provide triggers of their own often
have several). 

An alternative that may make sense here would be for the adc-joystick
driver to provide a trigger of it's own. That's easy enough to do,
but as things stand we don't provide a way to control the attached
trigger from other kernel drivers (i.e. you can't do the equivalent
of writing current_trigger for another device).

It's probably not implausible to add that though.  Is it worth it for
a joystick (vs doing what is done here), maybe not.

It would be worth doing if we cared about high performance (for some ADCs
anyway) but here we don't really so the polled read functions are fine.

Note many ADC drivers only support running in either polled or buffered
mode in IIO because polling random channels when doing highly optimised
accesses tends to make the drivers complex.  Hence you might find this
doesn't work for all setups...

Thanks,

Jonathan



> 
> Cheers,
> Artur
> > 
> > Thank you.
> >   
> >> > +
> >> > +	if (joy->polled) {
> >> > +		input_setup_polling(input, adc_joystick_poll);
> >> > +		input_set_poll_interval(input, ADC_JSK_POLL_INTERVAL);
> >> > +		input_set_min_poll_interval(input, ADC_JSK_POLL_MIN);
> >> > +		input_set_max_poll_interval(input, ADC_JSK_POLL_MAX);
> >> > +	} else {
> >> > +		input->open = adc_joystick_open;
> >> > +		input->close = adc_joystick_close;
> >> > +	}
> >> >
> >> >  	error = adc_joystick_set_axes(dev, joy);
> >> >  	if (error)
> >> > @@ -229,16 +257,18 @@ static int adc_joystick_probe(struct
> >> > platform_device *pdev)
> >> >  		return error;
> >> >  	}
> >> >
> >> > -	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\n");
> >> > -		return PTR_ERR(joy->buffer);
> >> > -	}
> >> > +	if (!joy->polled) {
> >> > +		joy->buffer = iio_channel_get_all_cb(dev, adc_joystick_handle, joy);  
> >> Please maintain line discipline of 80 chars to stay consistent with 
> >> the rest
> >> of this driver.  
> > 
> > Understood, sorry about that.
> >   
> >> > +		if (IS_ERR(joy->buffer)) {
> >> > +			dev_err(dev, "Unable to allocate callback buffer\n");
> >> > +			return PTR_ERR(joy->buffer);
> >> > +		}
> >> >
> >> > -	error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
> >> > joy->buffer);
> >> > -	if (error)  {
> >> > -		dev_err(dev, "Unable to add action\n");
> >> > -		return error;
> >> > +		error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
> >> > joy->buffer);  
> >> Same here.  
> > 
> > Ditto.
> >   
> >> 
> >> Cheers,
> >> Artur  
> >> > +		if (error)  {
> >> > +			dev_err(dev, "Unable to add action\n");
> >> > +			return error;
> >> > +		}
> >> >  	}
> >> >
> >> >  	return 0;  

Re: [PATCH v3 2/3] Input: adc-joystick - Add polled input device support

[Artur Rojek]

On 2022-06-19 17:32, Jonathan Cameron wrote:
> On Sat, 18 Jun 2022 15:08:29 +0200
> Artur Rojek <contact@artur-rojek.eu> wrote:
> 
>> On 2022-06-15 17:12, Chris Morgan wrote:
>> > On Wed, Jun 15, 2022 at 03:43:07AM +0200, Artur Rojek wrote:
>> >> On 2022-06-13 21:23, Chris Morgan wrote:
>> >> > From: Chris Morgan <macromorgan@hotmail.com>
>> >> >
>> >> > Add polled input device support to the adc-joystick driver. This is
>> >> > useful for devices which do not have hardware capable triggers on
>> >> > their SARADC. Code modified from adc-joystick.c changes made by Maya
>> >> > Matuszczyk.
>> >> >
>> >> > Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
>> >> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
>> >>
>> >> Hi Chris,
>> >>
>> >> Comments inline. I also Cc'd Paul and Jonathan, who were attached in
>> >> v2.
> +CC linux-iio
> 
>> >>
>> >> > ---
>> >> >  drivers/input/joystick/adc-joystick.c | 52 +++++++++++++++++++++------
>> >> >  1 file changed, 41 insertions(+), 11 deletions(-)
>> >> >
>> >> > diff --git a/drivers/input/joystick/adc-joystick.c
>> >> > b/drivers/input/joystick/adc-joystick.c
>> >> > index 78ebca7d400a..dc01cd0214d2 100644
>> >> > --- a/drivers/input/joystick/adc-joystick.c
>> >> > +++ b/drivers/input/joystick/adc-joystick.c
>> >> > @@ -13,6 +13,10 @@
>> >> >
>> >> >  #include <asm/unaligned.h>
>> >> >
>> >> > +#define ADC_JSK_POLL_INTERVAL	16
>> >> > +#define ADC_JSK_POLL_MIN	8
>> >> > +#define ADC_JSK_POLL_MAX	32
>> >> > +
>> >> >  struct adc_joystick_axis {
>> >> >  	u32 code;
>> >> >  	s32 range[2];
>> >> > @@ -26,8 +30,21 @@ struct adc_joystick {
>> >> >  	struct adc_joystick_axis *axes;
>> >> >  	struct iio_channel *chans;
>> >> >  	int num_chans;
>> >> > +	bool polled;
>> >> >  };
>> >> >
>> >> > +static void adc_joystick_poll(struct input_dev *input)
>> >> > +{
>> >> > +	struct adc_joystick *joy = input_get_drvdata(input);
>> >> > +	int i, val;
>> >> > +
>> >> > +	for (i = 0; i < joy->num_chans; i++) {
>> >> > +		iio_read_channel_raw(&joy->chans[i], &val);
Perhaps check the return value and leave early on error.
>> >> > +		input_report_abs(input, joy->axes[i].code, val);
>> >> > +	}
>> >> > +	input_sync(input);
>> >> > +}
>> >> > +
>> >> >  static int adc_joystick_handle(const void *data, void *private)
>> >> >  {
>> >> >  	struct adc_joystick *joy = private;
>> >> > @@ -215,8 +232,19 @@ static int adc_joystick_probe(struct
>> >> > platform_device *pdev)
>> >> >  	joy->input = input;
>> >> >  	input->name = pdev->name;
>> >> >  	input->id.bustype = BUS_HOST;
>> >> > -	input->open = adc_joystick_open;
>> >> > -	input->close = adc_joystick_close;
>> >> > +
>> >> > +	if (device_property_read_bool(dev,
>> >> > "adc-joystick,no-hardware-trigger"))
>> >> > +		joy->polled = 1;
>> >> As mentioned in v2, I don't think a DT property is required here.
>> >> Assuming
>> >> the polled mode is a fallback for devices with no buffers, just do:
>> >> ```
>> >> 	joy->polled = !(joy->chans[0].indio_dev->modes &
>> >> 			INDIO_ALL_BUFFER_MODES);
>> >> ```
>> >
>> > Understood. I attempted this and noticed that it was showing I have
>> > INDIO_BUFFER_TRIGGERED in addition to INDIO_DIRECT_MODE (the
>> > INDIO_DIRECT_MODE is the only one specified at the hardware level
>> > though). Should I just check for INDIO_BUFFER_SOFTWARE &
>> > INDIO_BUFFER_HARDWARE instead? I think it's possible that the inclusion
>> > of the industrialio_triggered_buffer module in my kernel is adding
>> > this to the channel somehow?
>> Having INDIO_BUFFER_TRIGGERED means that your saradc is capable of 
>> using
>> the existing flow. You should be able to register a software trigger 
>> and
>> use the adc-joystick driver without further issues.
>> That said, this is where it gets problematic - there is no way to 
>> create
>> an IIO trigger via Device Tree, since triggers don't describe any 
>> piece
>> of hardware, and you shouldn't need to register it at runtime
>> (configfs/sysfs) for communication between two kernel drivers either. 
>> At
>> the same time, it's not adc-joystick's job to register an external
>> trigger.
>> 
>> Jonathan,
>> I don't know what the proper approach to this should be, perhaps you
>> could assist?
> 
> You are correct in your description above. Device tree folk take the 
> view
> that sysfs / hrtimer etc triggers are a policy decision so don't belong
> in device tree.  In general you need some userspace code to stitch up
> the trigger anyway (even ADCs that provide triggers of their own often
> have several).
> 
> An alternative that may make sense here would be for the adc-joystick
> driver to provide a trigger of it's own. That's easy enough to do,
> but as things stand we don't provide a way to control the attached
> trigger from other kernel drivers (i.e. you can't do the equivalent
> of writing current_trigger for another device).
> 
> It's probably not implausible to add that though.  Is it worth it for
> a joystick (vs doing what is done here), maybe not.
> 
> It would be worth doing if we cared about high performance (for some 
> ADCs
> anyway) but here we don't really so the polled read functions are fine.
> 
> Note many ADC drivers only support running in either polled or buffered
> mode in IIO because polling random channels when doing highly optimised
> accesses tends to make the drivers complex.  Hence you might find this
> doesn't work for all setups...
> 
> Thanks,
> 
> Jonathan

Jonathan,
thanks for the detailed answer.

Chris,
In light of the above, I think the best course of action should be to 
keep your polling code. It looks like your saradc has introduced [1] 
`INDIO_BUFFER_TRIGGERED` support exclusively for this very 
(adc-joystick) case. This means we can't use `indio_dev->modes` to 
determine whether the joystick can be polled, otherwise we might break 
behavior of existing hardware. I suggest that we get back to passing 
this intention via optional `poll-interval` DT property, that enables 
the polling path if present (indiscriminately of the IIO mode). Let's 
see what the Device Tree folk say about that :)

Cheers,
Artur

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4e130dc7b41348b13684f0758c26cc6cf72a3449
> 
> 
> 
>> 
>> Cheers,
>> Artur
>> >
>> > Thank you.
>> >
>> >> > +
>> >> > +	if (joy->polled) {
>> >> > +		input_setup_polling(input, adc_joystick_poll);
>> >> > +		input_set_poll_interval(input, ADC_JSK_POLL_INTERVAL);
>> >> > +		input_set_min_poll_interval(input, ADC_JSK_POLL_MIN);
>> >> > +		input_set_max_poll_interval(input, ADC_JSK_POLL_MAX);
>> >> > +	} else {
>> >> > +		input->open = adc_joystick_open;
>> >> > +		input->close = adc_joystick_close;
>> >> > +	}
>> >> >
>> >> >  	error = adc_joystick_set_axes(dev, joy);
>> >> >  	if (error)
>> >> > @@ -229,16 +257,18 @@ static int adc_joystick_probe(struct
>> >> > platform_device *pdev)
>> >> >  		return error;
>> >> >  	}
>> >> >
>> >> > -	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\n");
>> >> > -		return PTR_ERR(joy->buffer);
>> >> > -	}
>> >> > +	if (!joy->polled) {
>> >> > +		joy->buffer = iio_channel_get_all_cb(dev, adc_joystick_handle, joy);
>> >> Please maintain line discipline of 80 chars to stay consistent with
>> >> the rest
>> >> of this driver.
>> >
>> > Understood, sorry about that.
>> >
>> >> > +		if (IS_ERR(joy->buffer)) {
>> >> > +			dev_err(dev, "Unable to allocate callback buffer\n");
>> >> > +			return PTR_ERR(joy->buffer);
>> >> > +		}
>> >> >
>> >> > -	error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
>> >> > joy->buffer);
>> >> > -	if (error)  {
>> >> > -		dev_err(dev, "Unable to add action\n");
>> >> > -		return error;
>> >> > +		error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
>> >> > joy->buffer);
>> >> Same here.
>> >
>> > Ditto.
>> >
>> >>
>> >> Cheers,
>> >> Artur
>> >> > +		if (error)  {
>> >> > +			dev_err(dev, "Unable to add action\n");
>> >> > +			return error;
>> >> > +		}
>> >> >  	}
>> >> >
>> >> >  	return 0;

Re: [PATCH v3 2/3] Input: adc-joystick - Add polled input device support

[Chris Morgan]

On Sun, Jun 19, 2022 at 06:31:17PM +0200, Artur Rojek wrote:
> On 2022-06-19 17:32, Jonathan Cameron wrote:
> > On Sat, 18 Jun 2022 15:08:29 +0200
> > Artur Rojek <contact@artur-rojek.eu> wrote:
> > 
> > > On 2022-06-15 17:12, Chris Morgan wrote:
> > > > On Wed, Jun 15, 2022 at 03:43:07AM +0200, Artur Rojek wrote:
> > > >> On 2022-06-13 21:23, Chris Morgan wrote:
> > > >> > From: Chris Morgan <macromorgan@hotmail.com>
> > > >> >
> > > >> > Add polled input device support to the adc-joystick driver. This is
> > > >> > useful for devices which do not have hardware capable triggers on
> > > >> > their SARADC. Code modified from adc-joystick.c changes made by Maya
> > > >> > Matuszczyk.
> > > >> >
> > > >> > Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> > > >> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > > >>
> > > >> Hi Chris,
> > > >>
> > > >> Comments inline. I also Cc'd Paul and Jonathan, who were attached in
> > > >> v2.
> > +CC linux-iio
> > 
> > > >>
> > > >> > ---
> > > >> >  drivers/input/joystick/adc-joystick.c | 52 +++++++++++++++++++++------
> > > >> >  1 file changed, 41 insertions(+), 11 deletions(-)
> > > >> >
> > > >> > diff --git a/drivers/input/joystick/adc-joystick.c
> > > >> > b/drivers/input/joystick/adc-joystick.c
> > > >> > index 78ebca7d400a..dc01cd0214d2 100644
> > > >> > --- a/drivers/input/joystick/adc-joystick.c
> > > >> > +++ b/drivers/input/joystick/adc-joystick.c
> > > >> > @@ -13,6 +13,10 @@
> > > >> >
> > > >> >  #include <asm/unaligned.h>
> > > >> >
> > > >> > +#define ADC_JSK_POLL_INTERVAL	16
> > > >> > +#define ADC_JSK_POLL_MIN	8
> > > >> > +#define ADC_JSK_POLL_MAX	32
> > > >> > +
> > > >> >  struct adc_joystick_axis {
> > > >> >  	u32 code;
> > > >> >  	s32 range[2];
> > > >> > @@ -26,8 +30,21 @@ struct adc_joystick {
> > > >> >  	struct adc_joystick_axis *axes;
> > > >> >  	struct iio_channel *chans;
> > > >> >  	int num_chans;
> > > >> > +	bool polled;
> > > >> >  };
> > > >> >
> > > >> > +static void adc_joystick_poll(struct input_dev *input)
> > > >> > +{
> > > >> > +	struct adc_joystick *joy = input_get_drvdata(input);
> > > >> > +	int i, val;
> > > >> > +
> > > >> > +	for (i = 0; i < joy->num_chans; i++) {
> > > >> > +		iio_read_channel_raw(&joy->chans[i], &val);
> Perhaps check the return value and leave early on error.

Okay, I'll do that.

> > > >> > +		input_report_abs(input, joy->axes[i].code, val);
> > > >> > +	}
> > > >> > +	input_sync(input);
> > > >> > +}
> > > >> > +
> > > >> >  static int adc_joystick_handle(const void *data, void *private)
> > > >> >  {
> > > >> >  	struct adc_joystick *joy = private;
> > > >> > @@ -215,8 +232,19 @@ static int adc_joystick_probe(struct
> > > >> > platform_device *pdev)
> > > >> >  	joy->input = input;
> > > >> >  	input->name = pdev->name;
> > > >> >  	input->id.bustype = BUS_HOST;
> > > >> > -	input->open = adc_joystick_open;
> > > >> > -	input->close = adc_joystick_close;
> > > >> > +
> > > >> > +	if (device_property_read_bool(dev,
> > > >> > "adc-joystick,no-hardware-trigger"))
> > > >> > +		joy->polled = 1;
> > > >> As mentioned in v2, I don't think a DT property is required here.
> > > >> Assuming
> > > >> the polled mode is a fallback for devices with no buffers, just do:
> > > >> ```
> > > >> 	joy->polled = !(joy->chans[0].indio_dev->modes &
> > > >> 			INDIO_ALL_BUFFER_MODES);
> > > >> ```
> > > >
> > > > Understood. I attempted this and noticed that it was showing I have
> > > > INDIO_BUFFER_TRIGGERED in addition to INDIO_DIRECT_MODE (the
> > > > INDIO_DIRECT_MODE is the only one specified at the hardware level
> > > > though). Should I just check for INDIO_BUFFER_SOFTWARE &
> > > > INDIO_BUFFER_HARDWARE instead? I think it's possible that the inclusion
> > > > of the industrialio_triggered_buffer module in my kernel is adding
> > > > this to the channel somehow?
> > > Having INDIO_BUFFER_TRIGGERED means that your saradc is capable of
> > > using
> > > the existing flow. You should be able to register a software trigger
> > > and
> > > use the adc-joystick driver without further issues.
> > > That said, this is where it gets problematic - there is no way to
> > > create
> > > an IIO trigger via Device Tree, since triggers don't describe any
> > > piece
> > > of hardware, and you shouldn't need to register it at runtime
> > > (configfs/sysfs) for communication between two kernel drivers
> > > either. At
> > > the same time, it's not adc-joystick's job to register an external
> > > trigger.
> > > 
> > > Jonathan,
> > > I don't know what the proper approach to this should be, perhaps you
> > > could assist?
> > 
> > You are correct in your description above. Device tree folk take the
> > view
> > that sysfs / hrtimer etc triggers are a policy decision so don't belong
> > in device tree.  In general you need some userspace code to stitch up
> > the trigger anyway (even ADCs that provide triggers of their own often
> > have several).
> > 
> > An alternative that may make sense here would be for the adc-joystick
> > driver to provide a trigger of it's own. That's easy enough to do,
> > but as things stand we don't provide a way to control the attached
> > trigger from other kernel drivers (i.e. you can't do the equivalent
> > of writing current_trigger for another device).
> > 
> > It's probably not implausible to add that though.  Is it worth it for
> > a joystick (vs doing what is done here), maybe not.
> > 
> > It would be worth doing if we cared about high performance (for some
> > ADCs
> > anyway) but here we don't really so the polled read functions are fine.
> > 
> > Note many ADC drivers only support running in either polled or buffered
> > mode in IIO because polling random channels when doing highly optimised
> > accesses tends to make the drivers complex.  Hence you might find this
> > doesn't work for all setups...
> > 
> > Thanks,
> > 
> > Jonathan
> 
> Jonathan,
> thanks for the detailed answer.
> 
> Chris,
> In light of the above, I think the best course of action should be to keep
> your polling code. It looks like your saradc has introduced [1]
> `INDIO_BUFFER_TRIGGERED` support exclusively for this very (adc-joystick)
> case. This means we can't use `indio_dev->modes` to determine whether the
> joystick can be polled, otherwise we might break behavior of existing
> hardware. I suggest that we get back to passing this intention via optional
> `poll-interval` DT property, that enables the polling path if present
> (indiscriminately of the IIO mode). Let's see what the Device Tree folk say
> about that :)
> 

Thanks. While I know there is a very strict policy of "don't break userspace"
I'm not aware of any consumers of the rockchip sardac using the adc-joystick
in production. I mean, it's in the tree for this one device but to this day
no one is shipping a distro with it to my knowledge, especially since the
driver has a major known issue I'm not able to fix at current time (has to do
with channels 1 and 2 being used instead of 0 and 1 for this specific SARADC).
What I'm getting at is if the current situation is wrong by adding the 
INDIO_BUFFER_TRIGGERED I don't think it's too late to change it right now.

Would the current poll code be best given the circumstances then? If you think
so, I'll go ahead and make all requested changes and resubmit it. I think I'm
understanding that what you want is for us to check for the existence of the
property of "poll-interval" which is the polling time in milliseconds. If this
value is present, use the polling code and if this value is not present, use
the existing code (regardless of what the hardware supports or doesn't).

Thank you once again for all your help and guidance.
-Chris

> Cheers,
> Artur
> 
> [1] https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2F%3Fid%3D4e130dc7b41348b13684f0758c26cc6cf72a3449&amp;data=05%7C01%7C%7C43ade3c4216a47079d5208da52112487%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637912530824693621%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Gorf7r9PJr1CDBbRKVNXnRyBJBw2cy%2FLv9tPP24Q5B0%3D&amp;reserved=0
> > 
> > 
> > 
> > > 
> > > Cheers,
> > > Artur
> > > >
> > > > Thank you.
> > > >
> > > >> > +
> > > >> > +	if (joy->polled) {
> > > >> > +		input_setup_polling(input, adc_joystick_poll);
> > > >> > +		input_set_poll_interval(input, ADC_JSK_POLL_INTERVAL);
> > > >> > +		input_set_min_poll_interval(input, ADC_JSK_POLL_MIN);
> > > >> > +		input_set_max_poll_interval(input, ADC_JSK_POLL_MAX);
> > > >> > +	} else {
> > > >> > +		input->open = adc_joystick_open;
> > > >> > +		input->close = adc_joystick_close;
> > > >> > +	}
> > > >> >
> > > >> >  	error = adc_joystick_set_axes(dev, joy);
> > > >> >  	if (error)
> > > >> > @@ -229,16 +257,18 @@ static int adc_joystick_probe(struct
> > > >> > platform_device *pdev)
> > > >> >  		return error;
> > > >> >  	}
> > > >> >
> > > >> > -	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\n");
> > > >> > -		return PTR_ERR(joy->buffer);
> > > >> > -	}
> > > >> > +	if (!joy->polled) {
> > > >> > +		joy->buffer = iio_channel_get_all_cb(dev, adc_joystick_handle, joy);
> > > >> Please maintain line discipline of 80 chars to stay consistent with
> > > >> the rest
> > > >> of this driver.
> > > >
> > > > Understood, sorry about that.
> > > >
> > > >> > +		if (IS_ERR(joy->buffer)) {
> > > >> > +			dev_err(dev, "Unable to allocate callback buffer\n");
> > > >> > +			return PTR_ERR(joy->buffer);
> > > >> > +		}
> > > >> >
> > > >> > -	error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
> > > >> > joy->buffer);
> > > >> > -	if (error)  {
> > > >> > -		dev_err(dev, "Unable to add action\n");
> > > >> > -		return error;
> > > >> > +		error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
> > > >> > joy->buffer);
> > > >> Same here.
> > > >
> > > > Ditto.
> > > >
> > > >>
> > > >> Cheers,
> > > >> Artur
> > > >> > +		if (error)  {
> > > >> > +			dev_err(dev, "Unable to add action\n");
> > > >> > +			return error;
> > > >> > +		}
> > > >> >  	}
> > > >> >
> > > >> >  	return 0;

Re: [PATCH v3 2/3] Input: adc-joystick - Add polled input device support

[Jonathan Cameron]

On Tue, 21 Jun 2022 16:41:01 -0500
Chris Morgan <macromorgan@hotmail.com> wrote:

> On Sun, Jun 19, 2022 at 06:31:17PM +0200, Artur Rojek wrote:
> > On 2022-06-19 17:32, Jonathan Cameron wrote:  
> > > On Sat, 18 Jun 2022 15:08:29 +0200
> > > Artur Rojek <contact@artur-rojek.eu> wrote:
> > >   
> > > > On 2022-06-15 17:12, Chris Morgan wrote:  
> > > > > On Wed, Jun 15, 2022 at 03:43:07AM +0200, Artur Rojek wrote:  
> > > > >> On 2022-06-13 21:23, Chris Morgan wrote:  
> > > > >> > From: Chris Morgan <macromorgan@hotmail.com>
> > > > >> >
> > > > >> > Add polled input device support to the adc-joystick driver. This is
> > > > >> > useful for devices which do not have hardware capable triggers on
> > > > >> > their SARADC. Code modified from adc-joystick.c changes made by Maya
> > > > >> > Matuszczyk.
> > > > >> >
> > > > >> > Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> > > > >> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>  
> > > > >>
> > > > >> Hi Chris,
> > > > >>
> > > > >> Comments inline. I also Cc'd Paul and Jonathan, who were attached in
> > > > >> v2.  
> > > +CC linux-iio
> > >   
> > > > >>  
> > > > >> > ---
> > > > >> >  drivers/input/joystick/adc-joystick.c | 52 +++++++++++++++++++++------
> > > > >> >  1 file changed, 41 insertions(+), 11 deletions(-)
> > > > >> >
> > > > >> > diff --git a/drivers/input/joystick/adc-joystick.c
> > > > >> > b/drivers/input/joystick/adc-joystick.c
> > > > >> > index 78ebca7d400a..dc01cd0214d2 100644
> > > > >> > --- a/drivers/input/joystick/adc-joystick.c
> > > > >> > +++ b/drivers/input/joystick/adc-joystick.c
> > > > >> > @@ -13,6 +13,10 @@
> > > > >> >
> > > > >> >  #include <asm/unaligned.h>
> > > > >> >
> > > > >> > +#define ADC_JSK_POLL_INTERVAL	16
> > > > >> > +#define ADC_JSK_POLL_MIN	8
> > > > >> > +#define ADC_JSK_POLL_MAX	32
> > > > >> > +
> > > > >> >  struct adc_joystick_axis {
> > > > >> >  	u32 code;
> > > > >> >  	s32 range[2];
> > > > >> > @@ -26,8 +30,21 @@ struct adc_joystick {
> > > > >> >  	struct adc_joystick_axis *axes;
> > > > >> >  	struct iio_channel *chans;
> > > > >> >  	int num_chans;
> > > > >> > +	bool polled;
> > > > >> >  };
> > > > >> >
> > > > >> > +static void adc_joystick_poll(struct input_dev *input)
> > > > >> > +{
> > > > >> > +	struct adc_joystick *joy = input_get_drvdata(input);
> > > > >> > +	int i, val;
> > > > >> > +
> > > > >> > +	for (i = 0; i < joy->num_chans; i++) {
> > > > >> > +		iio_read_channel_raw(&joy->chans[i], &val);  
> > Perhaps check the return value and leave early on error.  
> 
> Okay, I'll do that.
> 
> > > > >> > +		input_report_abs(input, joy->axes[i].code, val);
> > > > >> > +	}
> > > > >> > +	input_sync(input);
> > > > >> > +}
> > > > >> > +
> > > > >> >  static int adc_joystick_handle(const void *data, void *private)
> > > > >> >  {
> > > > >> >  	struct adc_joystick *joy = private;
> > > > >> > @@ -215,8 +232,19 @@ static int adc_joystick_probe(struct
> > > > >> > platform_device *pdev)
> > > > >> >  	joy->input = input;
> > > > >> >  	input->name = pdev->name;
> > > > >> >  	input->id.bustype = BUS_HOST;
> > > > >> > -	input->open = adc_joystick_open;
> > > > >> > -	input->close = adc_joystick_close;
> > > > >> > +
> > > > >> > +	if (device_property_read_bool(dev,
> > > > >> > "adc-joystick,no-hardware-trigger"))
> > > > >> > +		joy->polled = 1;  
> > > > >> As mentioned in v2, I don't think a DT property is required here.
> > > > >> Assuming
> > > > >> the polled mode is a fallback for devices with no buffers, just do:
> > > > >> ```
> > > > >> 	joy->polled = !(joy->chans[0].indio_dev->modes &
> > > > >> 			INDIO_ALL_BUFFER_MODES);
> > > > >> ```  
> > > > >
> > > > > Understood. I attempted this and noticed that it was showing I have
> > > > > INDIO_BUFFER_TRIGGERED in addition to INDIO_DIRECT_MODE (the
> > > > > INDIO_DIRECT_MODE is the only one specified at the hardware level
> > > > > though). Should I just check for INDIO_BUFFER_SOFTWARE &
> > > > > INDIO_BUFFER_HARDWARE instead? I think it's possible that the inclusion
> > > > > of the industrialio_triggered_buffer module in my kernel is adding
> > > > > this to the channel somehow?  
> > > > Having INDIO_BUFFER_TRIGGERED means that your saradc is capable of
> > > > using
> > > > the existing flow. You should be able to register a software trigger
> > > > and
> > > > use the adc-joystick driver without further issues.
> > > > That said, this is where it gets problematic - there is no way to
> > > > create
> > > > an IIO trigger via Device Tree, since triggers don't describe any
> > > > piece
> > > > of hardware, and you shouldn't need to register it at runtime
> > > > (configfs/sysfs) for communication between two kernel drivers
> > > > either. At
> > > > the same time, it's not adc-joystick's job to register an external
> > > > trigger.
> > > > 
> > > > Jonathan,
> > > > I don't know what the proper approach to this should be, perhaps you
> > > > could assist?  
> > > 
> > > You are correct in your description above. Device tree folk take the
> > > view
> > > that sysfs / hrtimer etc triggers are a policy decision so don't belong
> > > in device tree.  In general you need some userspace code to stitch up
> > > the trigger anyway (even ADCs that provide triggers of their own often
> > > have several).
> > > 
> > > An alternative that may make sense here would be for the adc-joystick
> > > driver to provide a trigger of it's own. That's easy enough to do,
> > > but as things stand we don't provide a way to control the attached
> > > trigger from other kernel drivers (i.e. you can't do the equivalent
> > > of writing current_trigger for another device).
> > > 
> > > It's probably not implausible to add that though.  Is it worth it for
> > > a joystick (vs doing what is done here), maybe not.
> > > 
> > > It would be worth doing if we cared about high performance (for some
> > > ADCs
> > > anyway) but here we don't really so the polled read functions are fine.
> > > 
> > > Note many ADC drivers only support running in either polled or buffered
> > > mode in IIO because polling random channels when doing highly optimised
> > > accesses tends to make the drivers complex.  Hence you might find this
> > > doesn't work for all setups...
> > > 
> > > Thanks,
> > > 
> > > Jonathan  
> > 
> > Jonathan,
> > thanks for the detailed answer.
> > 
> > Chris,
> > In light of the above, I think the best course of action should be to keep
> > your polling code. It looks like your saradc has introduced [1]
> > `INDIO_BUFFER_TRIGGERED` support exclusively for this very (adc-joystick)
> > case. This means we can't use `indio_dev->modes` to determine whether the
> > joystick can be polled, otherwise we might break behavior of existing
> > hardware. I suggest that we get back to passing this intention via optional
> > `poll-interval` DT property, that enables the polling path if present
> > (indiscriminately of the IIO mode). Let's see what the Device Tree folk say
> > about that :)
> >   
> 
> Thanks. While I know there is a very strict policy of "don't break userspace"
> I'm not aware of any consumers of the rockchip sardac using the adc-joystick
> in production. I mean, it's in the tree for this one device but to this day
> no one is shipping a distro with it to my knowledge, especially since the
> driver has a major known issue I'm not able to fix at current time (has to do
> with channels 1 and 2 being used instead of 0 and 1 for this specific SARADC).
> What I'm getting at is if the current situation is wrong by adding the 
> INDIO_BUFFER_TRIGGERED I don't think it's too late to change it right now.

No, better to leave that alone as it's correct, just not helpful to what you
want to do here.

> 
> Would the current poll code be best given the circumstances then? If you think
> so, I'll go ahead and make all requested changes and resubmit it. I think I'm
> understanding that what you want is for us to check for the existence of the
> property of "poll-interval" which is the polling time in milliseconds. If this
> value is present, use the polling code and if this value is not present, use
> the existing code (regardless of what the hardware supports or doesn't).

Sounds good to me.  We might get push back on dt binding though as
that doesn't sound very much like something that is about hardware rather
than policy.  It could be argued that a joystick works best at a particular
rate though so maybe we'll be fine.  We'll see when you send out the binding docs.

Jonathan

> 
> Thank you once again for all your help and guidance.
> -Chris
> 
> > Cheers,
> > Artur
> > 
> > [1] https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2F%3Fid%3D4e130dc7b41348b13684f0758c26cc6cf72a3449&amp;data=05%7C01%7C%7C43ade3c4216a47079d5208da52112487%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637912530824693621%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Gorf7r9PJr1CDBbRKVNXnRyBJBw2cy%2FLv9tPP24Q5B0%3D&amp;reserved=0  
> > > 
> > > 
> > >   
> > > > 
> > > > Cheers,
> > > > Artur  
> > > > >
> > > > > Thank you.
> > > > >  
> > > > >> > +
> > > > >> > +	if (joy->polled) {
> > > > >> > +		input_setup_polling(input, adc_joystick_poll);
> > > > >> > +		input_set_poll_interval(input, ADC_JSK_POLL_INTERVAL);
> > > > >> > +		input_set_min_poll_interval(input, ADC_JSK_POLL_MIN);
> > > > >> > +		input_set_max_poll_interval(input, ADC_JSK_POLL_MAX);
> > > > >> > +	} else {
> > > > >> > +		input->open = adc_joystick_open;
> > > > >> > +		input->close = adc_joystick_close;
> > > > >> > +	}
> > > > >> >
> > > > >> >  	error = adc_joystick_set_axes(dev, joy);
> > > > >> >  	if (error)
> > > > >> > @@ -229,16 +257,18 @@ static int adc_joystick_probe(struct
> > > > >> > platform_device *pdev)
> > > > >> >  		return error;
> > > > >> >  	}
> > > > >> >
> > > > >> > -	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\n");
> > > > >> > -		return PTR_ERR(joy->buffer);
> > > > >> > -	}
> > > > >> > +	if (!joy->polled) {
> > > > >> > +		joy->buffer = iio_channel_get_all_cb(dev, adc_joystick_handle, joy);  
> > > > >> Please maintain line discipline of 80 chars to stay consistent with
> > > > >> the rest
> > > > >> of this driver.  
> > > > >
> > > > > Understood, sorry about that.
> > > > >  
> > > > >> > +		if (IS_ERR(joy->buffer)) {
> > > > >> > +			dev_err(dev, "Unable to allocate callback buffer\n");
> > > > >> > +			return PTR_ERR(joy->buffer);
> > > > >> > +		}
> > > > >> >
> > > > >> > -	error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
> > > > >> > joy->buffer);
> > > > >> > -	if (error)  {
> > > > >> > -		dev_err(dev, "Unable to add action\n");
> > > > >> > -		return error;
> > > > >> > +		error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
> > > > >> > joy->buffer);  
> > > > >> Same here.  
> > > > >
> > > > > Ditto.
> > > > >  
> > > > >>
> > > > >> Cheers,
> > > > >> Artur  
> > > > >> > +		if (error)  {
> > > > >> > +			dev_err(dev, "Unable to add action\n");
> > > > >> > +			return error;
> > > > >> > +		}
> > > > >> >  	}
> > > > >> >
> > > > >> >  	return 0;  

Re: [PATCH v3 2/3] Input: adc-joystick - Add polled input device support

[Jonathan Cameron]

On Wed, 15 Jun 2022 03:43:07 +0200
Artur Rojek <contact@artur-rojek.eu> wrote:

> On 2022-06-13 21:23, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Add polled input device support to the adc-joystick driver. This is
> > useful for devices which do not have hardware capable triggers on
> > their SARADC. Code modified from adc-joystick.c changes made by Maya
> > Matuszczyk.
> > 
> > Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>  
> 
> Hi Chris,
> 
> Comments inline. I also Cc'd Paul and Jonathan, who were attached in v2.

Please cc linux-iio@vger.kernel.org on future versions.

> 
> > ---
> >  drivers/input/joystick/adc-joystick.c | 52 +++++++++++++++++++++------
> >  1 file changed, 41 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/input/joystick/adc-joystick.c
> > b/drivers/input/joystick/adc-joystick.c
> > index 78ebca7d400a..dc01cd0214d2 100644
> > --- a/drivers/input/joystick/adc-joystick.c
> > +++ b/drivers/input/joystick/adc-joystick.c
> > @@ -13,6 +13,10 @@
> > 
> >  #include <asm/unaligned.h>
> > 
> > +#define ADC_JSK_POLL_INTERVAL	16
> > +#define ADC_JSK_POLL_MIN	8
> > +#define ADC_JSK_POLL_MAX	32
> > +
> >  struct adc_joystick_axis {
> >  	u32 code;
> >  	s32 range[2];
> > @@ -26,8 +30,21 @@ struct adc_joystick {
> >  	struct adc_joystick_axis *axes;
> >  	struct iio_channel *chans;
> >  	int num_chans;
> > +	bool polled;
> >  };
> > 
> > +static void adc_joystick_poll(struct input_dev *input)
> > +{
> > +	struct adc_joystick *joy = input_get_drvdata(input);
> > +	int i, val;
> > +
> > +	for (i = 0; i < joy->num_chans; i++) {
> > +		iio_read_channel_raw(&joy->chans[i], &val);
> > +		input_report_abs(input, joy->axes[i].code, val);
> > +	}
> > +	input_sync(input);
> > +}
> > +
> >  static int adc_joystick_handle(const void *data, void *private)
> >  {
> >  	struct adc_joystick *joy = private;
> > @@ -215,8 +232,19 @@ static int adc_joystick_probe(struct 
> > platform_device *pdev)
> >  	joy->input = input;
> >  	input->name = pdev->name;
> >  	input->id.bustype = BUS_HOST;
> > -	input->open = adc_joystick_open;
> > -	input->close = adc_joystick_close;
> > +
> > +	if (device_property_read_bool(dev, 
> > "adc-joystick,no-hardware-trigger"))
> > +		joy->polled = 1;  
> As mentioned in v2, I don't think a DT property is required here. 
> Assuming the polled mode is a fallback for devices with no buffers, just 
> do:
> ```
> 	joy->polled = !(joy->chans[0].indio_dev->modes &
> 			INDIO_ALL_BUFFER_MODES);
> ```
> > +
> > +	if (joy->polled) {
> > +		input_setup_polling(input, adc_joystick_poll);
> > +		input_set_poll_interval(input, ADC_JSK_POLL_INTERVAL);
> > +		input_set_min_poll_interval(input, ADC_JSK_POLL_MIN);
> > +		input_set_max_poll_interval(input, ADC_JSK_POLL_MAX);
> > +	} else {
> > +		input->open = adc_joystick_open;
> > +		input->close = adc_joystick_close;
> > +	}
> > 
> >  	error = adc_joystick_set_axes(dev, joy);
> >  	if (error)
> > @@ -229,16 +257,18 @@ static int adc_joystick_probe(struct
> > platform_device *pdev)
> >  		return error;
> >  	}
> > 
> > -	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\n");
> > -		return PTR_ERR(joy->buffer);
> > -	}
> > +	if (!joy->polled) {
> > +		joy->buffer = iio_channel_get_all_cb(dev, adc_joystick_handle, joy);  
> Please maintain line discipline of 80 chars to stay consistent with the 
> rest of this driver.
> > +		if (IS_ERR(joy->buffer)) {
> > +			dev_err(dev, "Unable to allocate callback buffer\n");
> > +			return PTR_ERR(joy->buffer);
> > +		}
> > 
> > -	error = devm_add_action_or_reset(dev, adc_joystick_cleanup, 
> > joy->buffer);
> > -	if (error)  {
> > -		dev_err(dev, "Unable to add action\n");
> > -		return error;
> > +		error = devm_add_action_or_reset(dev, adc_joystick_cleanup, 
> > joy->buffer);  
> Same here.
> 
> Cheers,
> Artur
> > +		if (error)  {
> > +			dev_err(dev, "Unable to add action\n");
> > +			return error;
> > +		}
> >  	}
> > 
> >  	return 0;  

[PATCH v3 3/3] arm64: dts: rockchip: Update joystick to polled for Odroid-Go2

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Update the Odroid Go Advance to use "adc-joystick,no-hardware-trigger"
from the adc-joystick driver.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts b/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts
index ea0695b51ecd..ffa49b8dee15 100644
--- a/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts
@@ -24,6 +24,7 @@ chosen {
 
 	adc-joystick {
 		compatible = "adc-joystick";
+		adc-joystick,no-hardware-trigger;
 		io-channels = <&saradc 1>,
 			      <&saradc 2>;
 		#address-cells = <1>;
-- 
2.25.1