On Mon, 28 Aug 2017 11:57:07 -0400
William Breathitt Gray <vilhelm.gray@xxxxxxxxx> wrote:
> On Sun, Aug 20, 2017 at 12:40:02PM +0100, Jonathan Cameron wrote:
> >On Mon, 31 Jul 2017 12:03:23 -0400
> >William Breathitt Gray <vilhelm.gray@xxxxxxxxx> wrote:
> >
> >> This patch introduces the IIO generic counter interface for supporting
> >> counter devices. The generic counter interface serves as a catch-all to
> >> enable rudimentary support for devices that qualify as counters. More
> >> specific and apt counter interfaces may be developed on top of the
> >> generic counter interface, and those interfaces should be used by
> >> drivers when possible rather than the generic counter interface.
> >>
> >> In the context of the IIO generic counter interface, a counter is
> >> defined as a device that reports one or more "counter values" based on
> >> the state changes of one or more "counter signals" as evaluated by a
> >> defined "counter function."
> >>
> >> The IIO generic counter interface piggybacks off of the IIO core. This
> >> is primarily used to leverage the existing sysfs setup: the IIO_COUNT
> >> channel attributes represent the "counter value," while the IIO_SIGNAL
> >> channel attributes represent the "counter signal;" auxilary IIO_COUNT
> >> attributes represent the "counter signal" connections and their
> >> respective state change configurations which trigger an associated
> >> "counter function" evaluation.
> >>
> >> The iio_counter_ops structure serves as a container for driver callbacks
> >> to communicate with the device; function callbacks are provided to read
> >> and write various "counter signals" and "counter values," and set and
> >> get the "trigger mode" and "function mode" for various "counter signals"
> >> and "counter values" respectively.
> >>
> >> To support a counter device, a driver must first allocate the available
> >> "counter signals" via iio_counter_signal structures. These "counter
> >> signals" should be stored as an array and set to the init_signals member
> >> of an allocated iio_counter structure before the counter is registered.
> >>
> >> "Counter values" may be allocated via iio_counter_value structures, and
> >> respective "counter signal" associations made via iio_counter_trigger
> >> structures. Initial associated iio_counter_trigger structures may be
> >> stored as an array and set to the the init_triggers member of the
> >> respective iio_counter_value structure. These iio_counter_value
> >> structures may be set to the init_values member of an allocated
> >> iio_counter structure before the counter is registered if so desired.
> >>
> >> A counter device is registered to the system by passing the respective
> >> initialized iio_counter structure to the iio_counter_register function;
> >> similarly, the iio_counter_unregister function unregisters the
> >> respective counter.
> >>
> >> Signed-off-by: William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
> >
> >Hi William,
> >
> >A few minor points inline as a starting point. I'm going to want to dig
> >into this in a lot more detail but don't have the time today (or possibly
> >for a few more weeks - sorry about that!)
>
> Thank you for the quick review. Looking over your review points and my
> code again, it's become rather apparent to me that my implementation is
> burdenly opaque with its lack of apt comments to document the rationale
> of certain sections of code. I'll repair to remedy that in version 2 of
> this patchset.
>
> By the way, feel free to take your time reviewing, I'm in no particular
> rush myself. In fact, with the anticipated proper documentation coming I
> expect version 2 to a bit easily to digest for you than this intial
> patchset. As well, in general I prefer to get this interface committed
> late but correct, rather than soon but immature.
All sounds good. There is a fair bit here, so not surprising that
it gets a bit complex :)
>
> I've provided my responses inline to your specific review points.
>
> Sincerely,
>
> William Breathitt Gray
<snip>
> >> +
> >> +static ssize_t __iio_counter_trigger_mode_write(struct iio_dev *indio_dev,
> >> + uintptr_t priv, const struct iio_chan_spec *chan, const char *buf,
> >> + size_t len)
> >> +{
> >> + struct iio_counter *const counter = iio_priv(indio_dev);
> >> + struct iio_counter_value *value;
> >> + ssize_t err;
> >> + struct iio_counter_trigger *trigger;
> >> + const int signal_id = *(int *)((void *)priv);
> >Given you don't go through the void * cast in _read I'm thinking it's not
> >needed here either.
>
> I'm aware that we typically do not follow the C99 standard in the Linux
> kernel code, but since the uintptr_t typedef is modeled after the C99
> uintptr_t, I considered it appropriate to follow C99 in this case: the
> C99 standard requires an explicit conversion to void * from intptr_t
> before converting to another pointer type for dereferencing.
Fair enough.
>
> Despite the standard requirement, I agree that it is awkward to see the
> explicit cast to void * (likely because uintptr_t is not intended to be
> dereferenced in the context of the standard), so I'll be willing to
> remove it in this case if you believe it'll make the code clearer to
> understand.
>
> Just out of curiousity: why was uintptr_t choosen for this parameter
> rather than void *?
I'd imagine it's about explicitly tracking userspace pointers rather than
kernel ones. They could probably have have a uvoidptr_t but for some
reason decided not to...
<snip>
> >> +
> >> +static int __iio_counter_write_raw(struct iio_dev *indio_dev,
> >> + struct iio_chan_spec const *chan, int val, int val2, long mask)
> >> +{
> >> + struct iio_counter *const counter = iio_priv(indio_dev);
> >> + struct iio_counter_signal *signal;
> >> + int retval;
> >> + struct iio_counter_value *value;
> >> +
> >> + if (mask != IIO_CHAN_INFO_RAW)
> >> + return -EINVAL;
> >> +
> >> + switch (chan->type) {
> >> + case IIO_SIGNAL:
> >> + if (!counter->ops->signal_write)
> >> + return -EINVAL;
> >> +
> >> + mutex_lock(&counter->signal_list_lock);
> >> + signal = __iio_counter_signal_find_by_id(counter,
> >> + chan->channel2);
> >> + if (!signal) {
> >> + mutex_unlock(&counter->signal_list_lock);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + retval = counter->ops->signal_write(counter, signal, val, val2);
> >> + mutex_unlock(&counter->signal_list_lock);
> >> +
> >> + return retval;
> >> + case IIO_COUNT:
> >> + if (!counter->ops->value_write)
> >> + return -EINVAL;
> >> +
> >> + mutex_lock(&counter->value_list_lock);
> >> + value = __iio_counter_value_find_by_id(counter, chan->channel2);
> >> + if (!value) {
> >> + mutex_unlock(&counter->value_list_lock);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + retval = counter->ops->value_write(counter, value, val, val2);
> >> + mutex_unlock(&counter->value_list_lock);
> >> +
> >> + return retval;
> >> + default:
> >
> >This case definitely needs a comment!
> >I think you overwrite any write_raw callbacks passed in and hence this
> >is a infinite recursion...
> >Could be wrong though ;)
>
> I apologize, this looks like one of those cases where I should have
> provided some better documentation about the architecture of generic
> counter interface implementation. I'll make sure to make this clearer in
> the version 2 documentation and comments.
>
> However, I'll try to explain what's going on. The generic counter
> interface sits a layer above the IIO core, so drivers which use the
> generic counter interface do not directly supply IIO core write_raw
> callbacks, but rather provide generic counter signal_write/value_write
> callbacks (a passed write_raw callback is possible for non-counter IIO
> channels).
>
> The generic counter interface hooks itself via __iio_counter_write_raw
> to the IIO core write_raw. The __iio_counter_write_raw function handles
> the mapping to ensure that signal_write gets called for a generic
> counter Signal, value_write gets called for a generic counter Value, and
> a passed write_raw gets called for a passed non-counter IIO channel.
>
> The reason for this __iio_counter_write_raw function is to provide an
> abstraction for the signal_write/value_write functions to have access to
> generic counter interface parameters not available via the regular
> write_raw parameters. In theory, a driver utilizing the generic counter
> interface for a counter device does not need to know that it's utilizing
> IIO core under the hood since to it can handle all its counter device
> needs via the signal_write/value_write callbacks.
Thanks for the explanation. I'd somehow missed that entirely.
<snip>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
