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@xxxxxxxxxxxxxx> 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@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,
> > > >>
> > > >> 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&data=05%7C01%7C%7C43ade3c4216a47079d5208da52112487%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637912530824693621%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Gorf7r9PJr1CDBbRKVNXnRyBJBw2cy%2FLv9tPP24Q5B0%3D&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;
