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@xxxxxxxxx> wrote:
> > >
> > > > From: Chris Morgan <macromorgan@xxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > > > Signed-off-by: Chris Morgan <macromorgan@xxxxxxxxxxx>
> > > 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@xxxxxxxxxx>
> > >
> > > > ---
> > > > 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@xxxxxxxxxxxxxx>
> >
> > >
> > > > 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
