Re: [PATCH 06/23] gpio: sysfs: clean up chip class-device handling
On Mon, Apr 27, 2015 at 12:54:41PM +0900, Alexandre Courbot wrote: > On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold wrote: > > Clean gpio-chip class device registration and deregistration. > > > > The class device is registered when a gpio-chip is added (or from > > gpiolib_sysfs_init post-core init call), and deregistered when the chip > > is removed. > > > > Store the class device in struct gpio_chip directly rather than do a > > class-device lookup on deregistration. This also removes the need for > > the exported flag. > > > > Signed-off-by: Johan Hovold > > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > > index f1b36593ec9f..8c26855fc6ec 100644 > > --- a/include/linux/gpio/driver.h > > +++ b/include/linux/gpio/driver.h > > @@ -20,6 +20,7 @@ struct seq_file; > > * struct gpio_chip - abstract a GPIO controller > > * @label: for diagnostics > > * @dev: optional device providing the GPIOs > > + * @cdev: class device (may be NULL) > > Maybe a comment explaining that this field is non-NULL when a chip is > exported would be useful to understand how it is used in the code? I've added comments where the field is used. I didn't want to get into explaining sysfs implementation details in the header file, but the "may be NULL" is there as a hint to actually look at the code. And a gpio chip will always be registered with driver core (rather than "exported" ;) ) until it is removed. [ Currently we also allow for "late" registration, though. ] This is related to the issue discussed in my last mail, and again the plan is to let chip registration be used for more than the sysfs interface. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/23] gpio: sysfs: clean up chip class-device handling
On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold wrote:
> Clean gpio-chip class device registration and deregistration.
>
> The class device is registered when a gpio-chip is added (or from
> gpiolib_sysfs_init post-core init call), and deregistered when the chip
> is removed.
>
> Store the class device in struct gpio_chip directly rather than do a
> class-device lookup on deregistration. This also removes the need for
> the exported flag.
>
> Signed-off-by: Johan Hovold
> ---
> drivers/gpio/gpiolib-sysfs.c | 39 +--
> include/linux/gpio/driver.h | 4 ++--
> 2 files changed, 15 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index c433f075f349..2f8bdce792f6 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -567,7 +567,7 @@ int gpiod_export(struct gpio_desc *desc, bool
> direction_may_change)
> mutex_lock(&sysfs_lock);
>
> /* check if chip is being removed */
> - if (!chip || !chip->exported) {
> + if (!chip || !chip->cdev) {
> status = -ENODEV;
> goto fail_unlock;
> }
> @@ -752,7 +752,6 @@ EXPORT_SYMBOL_GPL(gpiod_unexport);
>
> int gpiochip_export(struct gpio_chip *chip)
> {
> - int status;
> struct device *dev;
>
> /* Many systems register gpio chips for SOC support very early,
> @@ -768,41 +767,29 @@ int gpiochip_export(struct gpio_chip *chip)
> chip, gpiochip_groups,
> "gpiochip%d", chip->base);
> if (IS_ERR(dev))
> - status = PTR_ERR(dev);
> - else
> - status = 0;
> + return PTR_ERR(dev);
>
> mutex_lock(&sysfs_lock);
> - chip->exported = (status == 0);
> + chip->cdev = dev;
> mutex_unlock(&sysfs_lock);
>
> - if (status)
> - chip_dbg(chip, "%s: status %d\n", __func__, status);
> -
> - return status;
> + return 0;
> }
>
> void gpiochip_unexport(struct gpio_chip *chip)
> {
> - int status;
> - struct device *dev;
> struct gpio_desc *desc;
> unsigned int i;
>
> - dev = class_find_device(&gpio_class, NULL, chip, match_export);
> - if (dev) {
> - put_device(dev);
> - device_unregister(dev);
> - /* prevent further gpiod exports */
> - mutex_lock(&sysfs_lock);
> - chip->exported = false;
> - mutex_unlock(&sysfs_lock);
> - status = 0;
> - } else
> - status = -ENODEV;
> + if (!chip->cdev)
> + return;
>
> - if (status)
> - chip_dbg(chip, "%s: status %d\n", __func__, status);
> + device_unregister(chip->cdev);
> +
> + /* prevent further gpiod exports */
> + mutex_lock(&sysfs_lock);
> + chip->cdev = NULL;
> + mutex_unlock(&sysfs_lock);
>
> /* unregister gpiod class devices owned by sysfs */
> for (i = 0; i < chip->ngpio; i++) {
> @@ -830,7 +817,7 @@ static int __init gpiolib_sysfs_init(void)
> */
> spin_lock_irqsave(&gpio_lock, flags);
> list_for_each_entry(chip, &gpio_chips, list) {
> - if (chip->exported)
> + if (chip->cdev)
> continue;
>
> /*
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index f1b36593ec9f..8c26855fc6ec 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -20,6 +20,7 @@ struct seq_file;
> * struct gpio_chip - abstract a GPIO controller
> * @label: for diagnostics
> * @dev: optional device providing the GPIOs
> + * @cdev: class device (may be NULL)
Maybe a comment explaining that this field is non-NULL when a chip is
exported would be useful to understand how it is used in the code?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 06/23] gpio: sysfs: clean up chip class-device handling
Clean gpio-chip class device registration and deregistration.
The class device is registered when a gpio-chip is added (or from
gpiolib_sysfs_init post-core init call), and deregistered when the chip
is removed.
Store the class device in struct gpio_chip directly rather than do a
class-device lookup on deregistration. This also removes the need for
the exported flag.
Signed-off-by: Johan Hovold
---
drivers/gpio/gpiolib-sysfs.c | 39 +--
include/linux/gpio/driver.h | 4 ++--
2 files changed, 15 insertions(+), 28 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index c433f075f349..2f8bdce792f6 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -567,7 +567,7 @@ int gpiod_export(struct gpio_desc *desc, bool
direction_may_change)
mutex_lock(&sysfs_lock);
/* check if chip is being removed */
- if (!chip || !chip->exported) {
+ if (!chip || !chip->cdev) {
status = -ENODEV;
goto fail_unlock;
}
@@ -752,7 +752,6 @@ EXPORT_SYMBOL_GPL(gpiod_unexport);
int gpiochip_export(struct gpio_chip *chip)
{
- int status;
struct device *dev;
/* Many systems register gpio chips for SOC support very early,
@@ -768,41 +767,29 @@ int gpiochip_export(struct gpio_chip *chip)
chip, gpiochip_groups,
"gpiochip%d", chip->base);
if (IS_ERR(dev))
- status = PTR_ERR(dev);
- else
- status = 0;
+ return PTR_ERR(dev);
mutex_lock(&sysfs_lock);
- chip->exported = (status == 0);
+ chip->cdev = dev;
mutex_unlock(&sysfs_lock);
- if (status)
- chip_dbg(chip, "%s: status %d\n", __func__, status);
-
- return status;
+ return 0;
}
void gpiochip_unexport(struct gpio_chip *chip)
{
- int status;
- struct device *dev;
struct gpio_desc *desc;
unsigned int i;
- dev = class_find_device(&gpio_class, NULL, chip, match_export);
- if (dev) {
- put_device(dev);
- device_unregister(dev);
- /* prevent further gpiod exports */
- mutex_lock(&sysfs_lock);
- chip->exported = false;
- mutex_unlock(&sysfs_lock);
- status = 0;
- } else
- status = -ENODEV;
+ if (!chip->cdev)
+ return;
- if (status)
- chip_dbg(chip, "%s: status %d\n", __func__, status);
+ device_unregister(chip->cdev);
+
+ /* prevent further gpiod exports */
+ mutex_lock(&sysfs_lock);
+ chip->cdev = NULL;
+ mutex_unlock(&sysfs_lock);
/* unregister gpiod class devices owned by sysfs */
for (i = 0; i < chip->ngpio; i++) {
@@ -830,7 +817,7 @@ static int __init gpiolib_sysfs_init(void)
*/
spin_lock_irqsave(&gpio_lock, flags);
list_for_each_entry(chip, &gpio_chips, list) {
- if (chip->exported)
+ if (chip->cdev)
continue;
/*
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index f1b36593ec9f..8c26855fc6ec 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -20,6 +20,7 @@ struct seq_file;
* struct gpio_chip - abstract a GPIO controller
* @label: for diagnostics
* @dev: optional device providing the GPIOs
+ * @cdev: class device (may be NULL)
* @owner: helps prevent removal of modules exporting active GPIOs
* @list: links gpio_chips together for traversal
* @request: optional hook for chip-specific activation, such as
@@ -57,7 +58,6 @@ struct seq_file;
* implies that if the chip supports IRQs, these IRQs need to be threaded
* as the chip access may sleep when e.g. reading out the IRQ status
* registers.
- * @exported: flags if the gpiochip is exported for use from sysfs. Private.
* @irq_not_threaded: flag must be set if @can_sleep is set but the
* IRQs don't need to be threaded
*
@@ -74,6 +74,7 @@ struct seq_file;
struct gpio_chip {
const char *label;
struct device *dev;
+ struct device *cdev;
struct module *owner;
struct list_headlist;
@@ -109,7 +110,6 @@ struct gpio_chip {
const char *const *names;
boolcan_sleep;
boolirq_not_threaded;
- boolexported;
#ifdef CONFIG_GPIOLIB_IRQCHIP
/*
--
2.0.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
