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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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