[PATCH v12 0/3] dt-bindings: adc-joystick: add poll-interval

[PATCH v12 0/3] dt-bindings: adc-joystick: add poll-interval

[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 V10:
 - Removed two unnecessary line breaks.

Changes from V9:
 - Simplified skipping of channel checking logic.
 - Simplified return logic for driver probe.

Changes from V8:
 - Changed logic for checking channels to skip checking of storagebits.
 - Changed comment format to comply with Linux style rules.

Changes from V7:
 - Dropped acks from adc-joystick.c changes, as it has been altered
   somewhat since v7.
 - Improved checking value of poll-interval to ensure explicit error
   handling in the event of malformed values.
 - Adjusted logic to not check for storagebits if polling is in use,
   which allows adc-joystick to work in polling mode for devices that
   utilize a mux (such as the Odroid Go Super and the Anbernic RG353).
 - Changed error message and channel size comparison logic slightly to
   ensure code can fit within 80 character column width.

Changes from V6:
 - Reverted back to "poll-interval" after mailing list discussion.
 - Referenced input.yaml for definition of poll-interval in adc-joystick
   documentation.

Changes from V5:
 - Corrected an error found by yamllint in the documentation.

Changes from V4:
 - Added unit property to devicetree attribute making it now
   "poll-interval-ms".
 - Simplified logic for setting polled value and made variable names
   more clear.

Changes from V3:
 - After lengthy discussion decided to support with existing attribute
   of "poll-interval". If present, the driver will use polling and if
   not present, it will use triggered buffers (as the driver does
   today).
 - Corrected some lines to honor the 80 character line-limit.
 - Check return value from polling method and return early if there
   is an error reading the data.

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 poll-interval
  Input: adc-joystick - Add polled input device support
  arm64: dts: rockchip: Update joystick to polled for OG2

 .../bindings/input/adc-joystick.yaml          |  5 +++
 .../boot/dts/rockchip/rk3326-odroid-go2.dts   |  1 +
 drivers/input/joystick/adc-joystick.c         | 44 +++++++++++++++++--
 3 files changed, 47 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v12 1/3] dt-bindings: adc-joystick: add poll-interval

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Add poll-interval support for the adc-joystick documentation. This is
an optional value and if not provided the adc-joystick works as it
does today (with buffers). If this value is provided, the adc-joystick
driver is polled at the specified interval. The existing attribute of
"poll-interval" was used instead of complying with property-units.yaml
after discussion of the issue on the mailing list.

Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Artur Rojek <contact@artur-rojek.eu>
---
 Documentation/devicetree/bindings/input/adc-joystick.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/adc-joystick.yaml b/Documentation/devicetree/bindings/input/adc-joystick.yaml
index 2ee04e03bc22..ab5caad055ea 100644
--- a/Documentation/devicetree/bindings/input/adc-joystick.yaml
+++ b/Documentation/devicetree/bindings/input/adc-joystick.yaml
@@ -14,6 +14,9 @@ description: >
   Bindings for joystick devices connected to ADC controllers supporting
   the Industrial I/O subsystem.
 
+allOf:
+  - $ref: input.yaml#
+
 properties:
   compatible:
     const: adc-joystick
@@ -28,6 +31,8 @@ properties:
       https://github.com/devicetree-org/dt-schema/blob/master/schemas/iio/iio-consumer.yaml
       for details.
 
+  poll-interval: true
+
   '#address-cells':
     const: 1
 
-- 
2.25.1

[PATCH v12 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 | 44 +++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
index 78ebca7d400a..77dfb7dd96eb 100644
--- a/drivers/input/joystick/adc-joystick.c
+++ b/drivers/input/joystick/adc-joystick.c
@@ -26,8 +26,23 @@ 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, ret;
+
+	for (i = 0; i < joy->num_chans; i++) {
+		ret = iio_read_channel_raw(&joy->chans[i], &val);
+		if (ret < 0)
+			return;
+		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;
@@ -179,6 +194,7 @@ static int adc_joystick_probe(struct platform_device *pdev)
 	int error;
 	int bits;
 	int i;
+	unsigned int poll_interval;
 
 	joy = devm_kzalloc(dev, sizeof(*joy), GFP_KERNEL);
 	if (!joy)
@@ -192,8 +208,21 @@ static int adc_joystick_probe(struct platform_device *pdev)
 		return error;
 	}
 
-	/* Count how many channels we got. NULL terminated. */
+	if (device_property_present(dev, "poll-interval")) {
+		error = device_property_read_u32(dev, "poll-interval",
+						 &poll_interval);
+		if (error)
+			return error;
+		joy->polled = true;
+	}
+
+	/*
+	 * Count how many channels we got. NULL terminated.
+	 * Do not check the storage size if using polling.
+	 */
 	for (i = 0; joy->chans[i].indio_dev; i++) {
+		if (joy->polled)
+			continue;
 		bits = joy->chans[i].channel->scan_type.storagebits;
 		if (!bits || bits > 16) {
 			dev_err(dev, "Unsupported channel storage size\n");
@@ -215,8 +244,14 @@ 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 (joy->polled) {
+		input_setup_polling(input, adc_joystick_poll);
+		input_set_poll_interval(input, poll_interval);
+	} else {
+		input->open = adc_joystick_open;
+		input->close = adc_joystick_close;
+	}
 
 	error = adc_joystick_set_axes(dev, joy);
 	if (error)
@@ -229,6 +264,9 @@ static int adc_joystick_probe(struct platform_device *pdev)
 		return error;
 	}
 
+	if (joy->polled)
+		return 0;
+
 	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");
-- 
2.25.1

[PATCH v12 3/3] arm64: dts: rockchip: Update joystick to polled for OG2

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Update the Odroid Go Advance to use "poll-interval" 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 415aa9ff8bd4..3389755dbdc8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts
@@ -26,6 +26,7 @@ adc-joystick {
 		compatible = "adc-joystick";
 		io-channels = <&saradc 1>,
 			      <&saradc 2>;
+		poll-interval = <60>;
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-- 
2.25.1

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

[Artur Rojek]

On 2022-08-06 16:20, Jonathan Cameron wrote:
> On Fri,  5 Aug 2022 12:10:15 -0500
> Chris Morgan <macroalpha82@gmail.com> 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,
> 
> Trying to avoid too much indentation has lead to an odd code structure.
> Still minor thing, so either way this looks fine to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
>>  drivers/input/joystick/adc-joystick.c | 44 
>> +++++++++++++++++++++++++--
>>  1 file changed, 41 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/input/joystick/adc-joystick.c 
>> b/drivers/input/joystick/adc-joystick.c
>> index 78ebca7d400a..77dfb7dd96eb 100644
>> --- a/drivers/input/joystick/adc-joystick.c
>> +++ b/drivers/input/joystick/adc-joystick.c
>> @@ -26,8 +26,23 @@ 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, ret;
>> +
>> +	for (i = 0; i < joy->num_chans; i++) {
>> +		ret = iio_read_channel_raw(&joy->chans[i], &val);
>> +		if (ret < 0)
>> +			return;
>> +		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;
>> @@ -179,6 +194,7 @@ static int adc_joystick_probe(struct 
>> platform_device *pdev)
>>  	int error;
>>  	int bits;
>>  	int i;
>> +	unsigned int poll_interval;
>> 
>>  	joy = devm_kzalloc(dev, sizeof(*joy), GFP_KERNEL);
>>  	if (!joy)
>> @@ -192,8 +208,21 @@ static int adc_joystick_probe(struct 
>> platform_device *pdev)
>>  		return error;
>>  	}
>> 
>> -	/* Count how many channels we got. NULL terminated. */
>> +	if (device_property_present(dev, "poll-interval")) {
>> +		error = device_property_read_u32(dev, "poll-interval",
>> +						 &poll_interval);
>> +		if (error)
>> +			return error;
>> +		joy->polled = true;
>> +	}
>> +
>> +	/*
>> +	 * Count how many channels we got. NULL terminated.
>> +	 * Do not check the storage size if using polling.
>> +	 */
>>  	for (i = 0; joy->chans[i].indio_dev; i++) {
>> +		if (joy->polled)
>> +			continue;
> 
> Whilst I can see why did this, it is a rather 'unusual' code structure
> and that makes me a tiny bit uncomfortable. However if everyone else
> is happy with this then fair enough (I see it was Artur's suggestion to
> handle it like this).
Yep, I'm fine with the way it is right now :)

Acked-by: Artur Rojek <contact@artur-rojek.eu>

> 
>>  		bits = joy->chans[i].channel->scan_type.storagebits;
>>  		if (!bits || bits > 16) {
>>  			dev_err(dev, "Unsupported channel storage size\n");
>> @@ -215,8 +244,14 @@ 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 (joy->polled) {
>> +		input_setup_polling(input, adc_joystick_poll);
>> +		input_set_poll_interval(input, poll_interval);
>> +	} else {
>> +		input->open = adc_joystick_open;
>> +		input->close = adc_joystick_close;
>> +	}
>> 
>>  	error = adc_joystick_set_axes(dev, joy);
>>  	if (error)
>> @@ -229,6 +264,9 @@ static int adc_joystick_probe(struct 
>> platform_device *pdev)
>>  		return error;
>>  	}
>> 
>> +	if (joy->polled)
>> +		return 0;
>> +
>>  	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");

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

[Jonathan Cameron]

On Fri,  5 Aug 2022 12:10:15 -0500
Chris Morgan <macroalpha82@gmail.com> 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,

Trying to avoid too much indentation has lead to an odd code structure.
Still minor thing, so either way this looks fine to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/input/joystick/adc-joystick.c | 44 +++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
> index 78ebca7d400a..77dfb7dd96eb 100644
> --- a/drivers/input/joystick/adc-joystick.c
> +++ b/drivers/input/joystick/adc-joystick.c
> @@ -26,8 +26,23 @@ 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, ret;
> +
> +	for (i = 0; i < joy->num_chans; i++) {
> +		ret = iio_read_channel_raw(&joy->chans[i], &val);
> +		if (ret < 0)
> +			return;
> +		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;
> @@ -179,6 +194,7 @@ static int adc_joystick_probe(struct platform_device *pdev)
>  	int error;
>  	int bits;
>  	int i;
> +	unsigned int poll_interval;
>  
>  	joy = devm_kzalloc(dev, sizeof(*joy), GFP_KERNEL);
>  	if (!joy)
> @@ -192,8 +208,21 @@ static int adc_joystick_probe(struct platform_device *pdev)
>  		return error;
>  	}
>  
> -	/* Count how many channels we got. NULL terminated. */
> +	if (device_property_present(dev, "poll-interval")) {
> +		error = device_property_read_u32(dev, "poll-interval",
> +						 &poll_interval);
> +		if (error)
> +			return error;
> +		joy->polled = true;
> +	}
> +
> +	/*
> +	 * Count how many channels we got. NULL terminated.
> +	 * Do not check the storage size if using polling.
> +	 */
>  	for (i = 0; joy->chans[i].indio_dev; i++) {
> +		if (joy->polled)
> +			continue;

Whilst I can see why did this, it is a rather 'unusual' code structure
and that makes me a tiny bit uncomfortable. However if everyone else
is happy with this then fair enough (I see it was Artur's suggestion to
handle it like this).

>  		bits = joy->chans[i].channel->scan_type.storagebits;
>  		if (!bits || bits > 16) {
>  			dev_err(dev, "Unsupported channel storage size\n");
> @@ -215,8 +244,14 @@ 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 (joy->polled) {
> +		input_setup_polling(input, adc_joystick_poll);
> +		input_set_poll_interval(input, poll_interval);
> +	} else {
> +		input->open = adc_joystick_open;
> +		input->close = adc_joystick_close;
> +	}
>  
>  	error = adc_joystick_set_axes(dev, joy);
>  	if (error)
> @@ -229,6 +264,9 @@ static int adc_joystick_probe(struct platform_device *pdev)
>  		return error;
>  	}
>  
> +	if (joy->polled)
> +		return 0;
> +
>  	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");

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

[Dmitry Torokhov]

On Sat, Aug 06, 2022 at 04:19:21PM +0200, Artur Rojek wrote:
> On 2022-08-06 16:20, Jonathan Cameron wrote:
> > On Fri,  5 Aug 2022 12:10:15 -0500
> > Chris Morgan <macroalpha82@gmail.com> 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,
> > 
> > Trying to avoid too much indentation has lead to an odd code structure.
> > Still minor thing, so either way this looks fine to me.
> > 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > > ---
> > >  drivers/input/joystick/adc-joystick.c | 44
> > > +++++++++++++++++++++++++--
> > >  1 file changed, 41 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/input/joystick/adc-joystick.c
> > > b/drivers/input/joystick/adc-joystick.c
> > > index 78ebca7d400a..77dfb7dd96eb 100644
> > > --- a/drivers/input/joystick/adc-joystick.c
> > > +++ b/drivers/input/joystick/adc-joystick.c
> > > @@ -26,8 +26,23 @@ 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, ret;
> > > +
> > > +	for (i = 0; i < joy->num_chans; i++) {
> > > +		ret = iio_read_channel_raw(&joy->chans[i], &val);
> > > +		if (ret < 0)
> > > +			return;
> > > +		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;
> > > @@ -179,6 +194,7 @@ static int adc_joystick_probe(struct
> > > platform_device *pdev)
> > >  	int error;
> > >  	int bits;
> > >  	int i;
> > > +	unsigned int poll_interval;
> > > 
> > >  	joy = devm_kzalloc(dev, sizeof(*joy), GFP_KERNEL);
> > >  	if (!joy)
> > > @@ -192,8 +208,21 @@ static int adc_joystick_probe(struct
> > > platform_device *pdev)
> > >  		return error;
> > >  	}
> > > 
> > > -	/* Count how many channels we got. NULL terminated. */
> > > +	if (device_property_present(dev, "poll-interval")) {
> > > +		error = device_property_read_u32(dev, "poll-interval",
> > > +						 &poll_interval);
> > > +		if (error)
> > > +			return error;
> > > +		joy->polled = true;


device_property_read_u32() return -EINVAL if property is not present, so
we can write:

	error = device_property_read_u32(dev, "poll-interval", &poll_interval);
	if (error) {
		/* -EINVAL means the property is absent. */
		if (error != -EINVAL)
			return error;
	} else if (poll_interval == 0) {
		dev_err(...);
		return -EINVAL;
	} else {
		joy->polled = true;
	}

> > > +	}
> > > +
> > > +	/*
> > > +	 * Count how many channels we got. NULL terminated.
> > > +	 * Do not check the storage size if using polling.
> > > +	 */
> > >  	for (i = 0; joy->chans[i].indio_dev; i++) {
> > > +		if (joy->polled)
> > > +			continue;
> > 
> > Whilst I can see why did this, it is a rather 'unusual' code structure
> > and that makes me a tiny bit uncomfortable. However if everyone else
> > is happy with this then fair enough (I see it was Artur's suggestion to
> > handle it like this).
> Yep, I'm fine with the way it is right now :)
> 
> Acked-by: Artur Rojek <contact@artur-rojek.eu>
> 
> > 
> > >  		bits = joy->chans[i].channel->scan_type.storagebits;
> > >  		if (!bits || bits > 16) {
> > >  			dev_err(dev, "Unsupported channel storage size\n");
> > > @@ -215,8 +244,14 @@ 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 (joy->polled) {
> > > +		input_setup_polling(input, adc_joystick_poll);
> > > +		input_set_poll_interval(input, poll_interval);
> > > +	} else {
> > > +		input->open = adc_joystick_open;
> > > +		input->close = adc_joystick_close;
> > > +	}
> > > 
> > >  	error = adc_joystick_set_axes(dev, joy);
> > >  	if (error)
> > > @@ -229,6 +264,9 @@ static int adc_joystick_probe(struct
> > > platform_device *pdev)
> > >  		return error;
> > >  	}
> > > 
> > > +	if (joy->polled)
> > > +		return 0;
> > > +

This is no longer compatible with the latest driver code as input device
registration has been moved to the very end, so you actually need to move
getting bugger and setting up cleanup action into the "else" clause of "if
(joy->polled)", even though it adds indentation level.

> > >  	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");

Thanks.

-- 
Dmitry

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

[Chris Morgan]

On Wed, Aug 10, 2022 at 05:58:48PM -0700, Dmitry Torokhov wrote:
> On Sat, Aug 06, 2022 at 04:19:21PM +0200, Artur Rojek wrote:
> > On 2022-08-06 16:20, Jonathan Cameron wrote:
> > > On Fri,  5 Aug 2022 12:10:15 -0500
> > > Chris Morgan <macroalpha82@gmail.com> 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,
> > > 
> > > Trying to avoid too much indentation has lead to an odd code structure.
> > > Still minor thing, so either way this looks fine to me.
> > > 
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > > ---
> > > >  drivers/input/joystick/adc-joystick.c | 44
> > > > +++++++++++++++++++++++++--
> > > >  1 file changed, 41 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/input/joystick/adc-joystick.c
> > > > b/drivers/input/joystick/adc-joystick.c
> > > > index 78ebca7d400a..77dfb7dd96eb 100644
> > > > --- a/drivers/input/joystick/adc-joystick.c
> > > > +++ b/drivers/input/joystick/adc-joystick.c
> > > > @@ -26,8 +26,23 @@ 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, ret;
> > > > +
> > > > +	for (i = 0; i < joy->num_chans; i++) {
> > > > +		ret = iio_read_channel_raw(&joy->chans[i], &val);
> > > > +		if (ret < 0)
> > > > +			return;
> > > > +		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;
> > > > @@ -179,6 +194,7 @@ static int adc_joystick_probe(struct
> > > > platform_device *pdev)
> > > >  	int error;
> > > >  	int bits;
> > > >  	int i;
> > > > +	unsigned int poll_interval;
> > > > 
> > > >  	joy = devm_kzalloc(dev, sizeof(*joy), GFP_KERNEL);
> > > >  	if (!joy)
> > > > @@ -192,8 +208,21 @@ static int adc_joystick_probe(struct
> > > > platform_device *pdev)
> > > >  		return error;
> > > >  	}
> > > > 
> > > > -	/* Count how many channels we got. NULL terminated. */
> > > > +	if (device_property_present(dev, "poll-interval")) {
> > > > +		error = device_property_read_u32(dev, "poll-interval",
> > > > +						 &poll_interval);
> > > > +		if (error)
> > > > +			return error;
> > > > +		joy->polled = true;
> 
> 
> device_property_read_u32() return -EINVAL if property is not present, so
> we can write:

Understood. Both ways can work but when I do the next revision I'll change it
to this.

> 
> 	error = device_property_read_u32(dev, "poll-interval", &poll_interval);
> 	if (error) {
> 		/* -EINVAL means the property is absent. */
> 		if (error != -EINVAL)
> 			return error;
> 	} else if (poll_interval == 0) {
> 		dev_err(...);
> 		return -EINVAL;
> 	} else {
> 		joy->polled = true;
> 	}
> 
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Count how many channels we got. NULL terminated.
> > > > +	 * Do not check the storage size if using polling.
> > > > +	 */
> > > >  	for (i = 0; joy->chans[i].indio_dev; i++) {
> > > > +		if (joy->polled)
> > > > +			continue;
> > > 
> > > Whilst I can see why did this, it is a rather 'unusual' code structure
> > > and that makes me a tiny bit uncomfortable. However if everyone else
> > > is happy with this then fair enough (I see it was Artur's suggestion to
> > > handle it like this).
> > Yep, I'm fine with the way it is right now :)
> > 
> > Acked-by: Artur Rojek <contact@artur-rojek.eu>
> > 
> > > 
> > > >  		bits = joy->chans[i].channel->scan_type.storagebits;
> > > >  		if (!bits || bits > 16) {
> > > >  			dev_err(dev, "Unsupported channel storage size\n");
> > > > @@ -215,8 +244,14 @@ 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 (joy->polled) {
> > > > +		input_setup_polling(input, adc_joystick_poll);
> > > > +		input_set_poll_interval(input, poll_interval);
> > > > +	} else {
> > > > +		input->open = adc_joystick_open;
> > > > +		input->close = adc_joystick_close;
> > > > +	}
> > > > 
> > > >  	error = adc_joystick_set_axes(dev, joy);
> > > >  	if (error)
> > > > @@ -229,6 +264,9 @@ static int adc_joystick_probe(struct
> > > > platform_device *pdev)
> > > >  		return error;
> > > >  	}
> > > > 
> > > > +	if (joy->polled)
> > > > +		return 0;
> > > > +
> 
> This is no longer compatible with the latest driver code as input device
> registration has been moved to the very end, so you actually need to move
> getting bugger and setting up cleanup action into the "else" clause of "if
> (joy->polled)", even though it adds indentation level.

I'm afraid I don't understand the issue you are describing.
input_register_device is called before this section, and since it's allocated
with devm we shouldn't need to clean it up in the event of an error, right?
The two functions below this line (including the one establishing the
joystick cleanup) are only required for non-polled devices.

Thank you.

> 
> > > >  	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");
> 
> Thanks.
> 
> -- 
> Dmitry