On Wednesday 14 January 2015 14:11:16 Alexandre Courbot wrote:
> On Sat, Jan 10, 2015 at 12:19 AM, Rojhalat Ibrahim <[email protected]> wrote:
> > Introduce new functions for conveniently obtaining and disposing of an
> > entire
> > array of GPIOs with one function call.
>
> Excellent, thanks for following up with this! :)
>
> >
> > Suggested-by: Alexandre Courbot <[email protected]>
> > Signed-off-by: Rojhalat Ibrahim <[email protected]>
> > ---
> > Documentation/gpio/consumer.txt | 28 ++++++++++++-
> > drivers/gpio/gpiolib.c | 84
> > ++++++++++++++++++++++++++++++++++++++++
> > include/linux/gpio/consumer.h | 42 ++++++++++++++++++++
> > 3 files changed, 153 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/gpio/consumer.txt
> > b/Documentation/gpio/consumer.txt
> > index c67f806..d02f341 100644
> > --- a/Documentation/gpio/consumer.txt
> > +++ b/Documentation/gpio/consumer.txt
> > @@ -58,7 +58,6 @@ pattern where a GPIO is optional, the
> > gpiod_get_optional() and
> > gpiod_get_index_optional() functions can be used. These functions return
> > NULL
> > instead of -ENOENT if no GPIO has been assigned to the requested function:
> >
> > -
> > struct gpio_desc *gpiod_get_optional(struct device *dev,
> > const char *con_id,
> > enum gpiod_flags flags)
> > @@ -68,6 +67,29 @@ instead of -ENOENT if no GPIO has been assigned to the
> > requested function:
> > unsigned int index,
> > enum gpiod_flags flags)
> >
> > +For a function using multiple GPIOs all of those can be obtained with one
> > call:
> > +
> > + int gpiod_get_array(struct device *dev,
> > + const char *con_id,
> > + struct gpio_desc **desc_array,
> > + unsigned int count,
> > + enum gpiod_flags flags)
> > +
> > +In this case an array for storing the GPIO descriptors and the number of
> > GPIOs
> > +are required as additional arguments. This function either returns the
> > +requested number of GPIOs or an error code. It will not return a positive
> > +number if some but not all of the requested GPIOs could be obtained.
>
> I wonder if the following interface would not be more convenient, at
> least for the most common cases:
>
> struct gpiod_descs *gpiod_get_array(struct device *dev, const char
> *con_id, enum gpiod_flags flags);
>
> Which would *allocate* the array of descriptors to the right number of
> GPIOs and return them all. The current way of doing requires the user
> to 1) figure out how many GPIOs there are under (dev, con_id), 2)
> allocate a the GPIO array and 3) call this function. The array must
> also be explicitly freed after calling gpiod_put_array().
>
> struct gpiod_descs would be something like this:
>
> struct gpiod_descs {
> unsigned count;
> struct gpiod_desc*[];
> }
>
> It uses a zero-length array to store the actual descriptors and
> carries the count so the user does not have to remember it (it will
> always be needed anyway, and that way we don't have to pass it
> explicitly to gpiod_put_array()).
>
> The nice thing with this interface is that it works identically to
> gpiod_get(), excepted it can handle 1 GPIO or more. This would be
> handy in order to implement sysfs v2 on top of it (see
> https://lkml.org/lkml/2015/1/3/50).
>
> We could even have a macro to iterate over the GPIOs:
>
> #define for_each_gpio(gpiod_descs, gpiod_desc) ...
>
I see your point. It certainly makes sense.
The gpiod_get_array function would have to not only allocate the array of
descriptors but also the struct containing the pointer to the array.
Alternatively gpiod_get_array could return a struct gpiod_descs instead of
a pointer to such a struct:
struct gpiod_descs gpiod_get_array(struct device *dev,
const char
*con_id,
enum
gpiod_flags flags);
Errors could be returned either as a negative count or in an additional
struct member.
Would that also be an acceptable interface? Or should a gpiod_get_ function
always return a pointer?
> If you think your current interface is necessary because users need to
> work with already-allocated arrays of descriptors, could you then
> rename it under a more "private" name (maybe
> gpiod_fill_array/gpiod_free_array?) and build the
> gpiod_get_array/gpiod_put_array I suggest on top of it?
>
I think that's unnecessary for now.
> Also for the sake of completeness, may I suggest adding the following
> function to the gpiod interface:
>
> int gpiod_count(struct device *dev, const char *con_id);
>
> Which returns the number of GPIOs that are declared for (dev, con_id)?
> Right now gpiod_get_array() can only be used properly under the device
> tree (which provides of_gpio_named_count()), but we also need to
> enable it for ACPI and platform data.
>
> Having this function will allow users of gpiod_get_index() to know the
> boundaries they have to work with, no matter how the GPIO mappings are
> provided. It will also be needed if you implement the version of
> gpiod_get_array() I suggest.
>
> In your patch 2/2, you will then also want to replace the call to
> of_gpio_count() in mdio_mux_gpio_probe() by a call to gpiod_count().
>
I'll look into that.
> > +The following function behaves differently:
> > +
> > + int gpiod_get_array_optional(struct device *dev,
> > + const char *con_id,
> > + struct gpio_desc **desc_array,
> > + unsigned int could,
> > + enum gpiod_flags flags)
> > +
> > +This one returns the number of GPIOs actually available which can be
> > smaller
> > +than the requested number or even 0.
>
> There is no user for this function yet but I agree it is good to have.
>
So for the interface you propose that would mean:
gpiod_get_array returns with exactly the number of GPIO descriptors as
gpiod_count returns or an error code whereas gpiod_get_array_optional can also
return with fewer or no descriptors at all.
Right? Or is one of the two unnecessary?
> > +
> > Device-managed variants of these functions are also defined:
>
> Do you plan to also add devm variants of the new get_array() function(s)?
>
I probably should. Need to look into it.
> >
> > struct gpio_desc *devm_gpiod_get(struct device *dev, const char
> > *con_id,
> > @@ -91,6 +113,10 @@ A GPIO descriptor can be disposed of using the
> > gpiod_put() function:
> >
> > void gpiod_put(struct gpio_desc *desc)
> >
> > +For an array of GPIOs this function can be used:
> > +
> > + void gpiod_put_array(struct gpio_desc **desc_array, unsigned int
> > count)
> > +
> > It is strictly forbidden to use a descriptor after calling this function.
> > The
>
> This paragraph will probably require a s/this/these to be gramatically
> correct.
>
Right.
> > device-managed variant is, unsurprisingly:
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 56b7c5d..d45fa9c 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1925,6 +1925,76 @@ struct gpio_desc *__must_check
> > __gpiod_get_index_optional(struct device *dev,
> > EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
> >
> > /**
> > + * gpiod_get_array - obtain multiple GPIOs from a multi-index GPIO function
> > + * @dev: GPIO consumer, can be NULL for system-global GPIOs
> > + * @con_id: function within the GPIO consumer
> > + * @desc_array: descriptor array for the acquired GPIOs
> > + * @count: number of GPIOs to obtain in the consumer
> > + * @flags: optional GPIO initialization flags
> > + *
> > + * This function obtains the specified number of GPIOs starting with index > > 0
> > + * for functions that define several GPIOs.
> > + *
> > + * Return the actual number of acquired GPIOs, -ENOENT if the actual
> > number of
> > + * GPIOs assigned to the requested function is smaller than @count,
> > + * or another IS_ERR() code if an error occurred while trying to acquire
> > the
> > + * GPIOs.
> > + */
> > +int __must_check __gpiod_get_array(struct device *dev, const char *con_id,
> > + struct gpio_desc **desc_array,
> > + unsigned int count,
> > + enum gpiod_flags flags)
> > +{
> > + int r;
> > +
> > + r = gpiod_get_array_optional(dev, con_id, desc_array, count, flags);
> > + if ((r >= 0) && (r != count)) {
> > + gpiod_put_array(desc_array, r);
> > + return -ENOENT;
> > + }
> > + return r;
> > +}
> > +EXPORT_SYMBOL_GPL(__gpiod_get_array);
> > +
> > +/**
> > + * gpiod_get_array_optional - obtain multiple GPIOs from a multi-index GPIO
> > + * function
> > + * @dev: GPIO consumer, can be NULL for system-global GPIOs
> > + * @con_id: function within the GPIO consumer
> > + * @desc_array: descriptor array for the acquired GPIOs
> > + * @count: number of GPIOs to obtain in the consumer
> > + * @flags: optional GPIO initialization flags
> > + *
> > + * This is equivalent to gpiod_get_array(), except that when the actual
> > number
> > + * of GPIOs assigned to the requested function is smaller than @count it
> > will
> > + * not return an error but the number of GPIOs actually available.
> > + */
> > +int __must_check __gpiod_get_array_optional(struct device *dev,
> > + const char *con_id,
> > + struct gpio_desc **desc_array,
> > + unsigned int count,
> > + enum gpiod_flags flags)
> > +{
> > + struct gpio_desc *desc;
> > + unsigned int n;
> > +
> > + for (n = 0; n < count; ) {
> > + desc = gpiod_get_index(dev, con_id, n, flags);
> > + if (IS_ERR(desc)) {
> > + if (PTR_ERR(desc) != -ENOENT) {
> > + gpiod_put_array(desc_array, n);
> > + return PTR_ERR(desc);
> > + }
> > + break;
> > + }
> > + desc_array[n] = desc;
> > + n++;
> > + }
> > + return n;
> > +}
> > +EXPORT_SYMBOL_GPL(__gpiod_get_array_optional);
> > +
> > +/**
> > * gpiod_put - dispose of a GPIO descriptor
> > * @desc: GPIO descriptor to dispose of
> > *
> > @@ -1936,6 +2006,20 @@ void gpiod_put(struct gpio_desc *desc)
> > }
> > EXPORT_SYMBOL_GPL(gpiod_put);
> >
> > +/**
> > + * gpiod_put_array - dispose of multiple GPIO descriptors
> > + * @desc_array: GPIO descriptor array
> > + * @count: number of GPIOs in the array
> > + */
> > +void gpiod_put_array(struct gpio_desc **desc_array, unsigned int count)
> > +{
> > + unsigned int n;
> > +
> > + for (n = 0; n < count; n++)
> > + gpiod_put(desc_array[n]);
> > +}
> > +EXPORT_SYMBOL_GPL(gpiod_put_array);
>
> The implementation looks good to me. I'd just like you to consider the
> alternative interface I propose, as I suspect it might make the life
> of users easier.
>
Thanks for reviewing this.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
