On 1/7/20 1:10 AM, Frank Rowand wrote:
> On 1/6/20 5:34 PM, Frank Rowand wrote:
>> On 12/30/19 7:38 AM, Geert Uytterhoeven wrote:
>>> As GPIO hogs are configured at GPIO controller initialization time,
>>> adding/removing GPIO hogs in DT overlays does not work.
>>>
>>> Add support for GPIO hogs described in DT overlays by registering an OF
>>> reconfiguration notifier, to handle the addition and removal of GPIO hog
>>> subnodes to/from a GPIO controller device node.
>>>
>>> Note that when a GPIO hog device node is being removed, its "gpios"
>>> properties is no longer available, so we have to keep track of which
>>> node a hog belongs to, which is done by adding a pointer to the hog's
>>> device node to struct gpio_desc.
>>
>> If I have read the patches and the existing overlay source correctly,
>> then some observations:
>>
>> - A gpio hog node added in an overlay will be properly processed.
>>
>> - A gpio hog node already existing in the live devicetree, but with a
>> non-active status will be properly processed if the status of the
>> gpio hog node is changed to "ok" in the overlay.
>>
>> - If a gpio hog node already exists in the live devicetree with an
>> active status, then any updated or added properties in that gpio
>> hog node in the overlay will have no effect.
>>
>> There is a scenario where the updated property would have an effect:
>> apply a second overlay that sets the status to inactive, then apply
>> a third overlay that sets the status back to active. This is a
>> rather contrived example and I think it should be documented as
>> not supported and the result undefined.
>
> I went back and double checked the related code. For gpio hog nodes
> that are in a non-overlay, the status property is checked because
> of_gpiochip_scan_gpios() uses for_each_available_child_of_node()
> to search for gpio hog nodes, and for_each_available_child_of_node()
> checks the status property. But in the case of a gpio hog node
> added by an overlay, of_gpio_notify() does not check the status
> property in the gpio hog node. The check for the status property
> should be added to of_gpio_notify().
One more thing I have not thought through is the case where the
overlay contains both the gpio node and the gpio hog node. I'll
walk through that tomorrow.
-Frank
>
> -Frank
>
>>
>> It would be good to document this explicitly.
>>
>>
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>>> ---
>>> drivers/gpio/gpiolib-of.c | 84 +++++++++++++++++++++++++++++++++++++++
>>> drivers/gpio/gpiolib-of.h | 2 +
>>> drivers/gpio/gpiolib.c | 14 +++++--
>>> drivers/gpio/gpiolib.h | 3 ++
>>> 4 files changed, 100 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>>> index dfae797846bb746b..89a6138ac0a4b506 100644
>>> --- a/drivers/gpio/gpiolib-of.c
>>> +++ b/drivers/gpio/gpiolib-of.c
>>> @@ -649,6 +649,10 @@ static int of_gpiochip_add_hog(struct gpio_chip *chip, struct device_node *hog)
>>> ret = gpiod_hog(desc, name, lflags, dflags);
>>> if (ret < 0)
>>> return ret;
>>> +
>>> +#ifdef CONFIG_OF_DYNAMIC
>>> + desc->hog = hog;
>>> +#endif
>>> }
>>>
>>> return 0;
>>> @@ -676,11 +680,91 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>>> of_node_put(np);
>>> return ret;
>>> }
>>> +
>>> + of_node_set_flag(np, OF_POPULATED);
>>> }
>>>
>>> return 0;
>>> }
>>>
>>> +#ifdef CONFIG_OF_DYNAMIC
>>> +/**
>>> + * of_gpiochip_remove_hog - Remove all hogs in a hog device node
>>> + * @chip: gpio chip to act on
>>> + * @hog: device node describing the hogs
>>> + */
>>> +static void of_gpiochip_remove_hog(struct gpio_chip *chip,
>>> + struct device_node *hog)
>>> +{
>>> + struct gpio_desc *descs = chip->gpiodev->descs;
>>> + unsigned int i;
>>> +
>>> + for (i = 0; i < chip->ngpio; i++) {
>>> + if (test_bit(FLAG_IS_HOGGED, &descs[i].flags) &&
>>> + descs[i].hog == hog)
>>> + gpiochip_free_own_desc(&descs[i]);
>>> + }
>>> +}
>>> +
>>> +static int of_gpiochip_match_node(struct gpio_chip *chip, void *data)
>>> +{
>>> + return chip->gpiodev->dev.of_node == data;
>>> +}
>>> +
>>> +static struct gpio_chip *of_find_gpiochip_by_node(struct device_node *np)
>>> +{
>>> + return gpiochip_find(np, of_gpiochip_match_node);
>>> +}
>>> +
>>> +static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
>>> + void *arg)
>>> +{
>>> + struct of_reconfig_data *rd = arg;
>>> + struct gpio_chip *chip;
>>> + int ret;
>>> +
>>> + switch (of_reconfig_get_state_change(action, arg)) {
>>> + case OF_RECONFIG_CHANGE_ADD:
>>> + if (!of_property_read_bool(rd->dn, "gpio-hog"))
>>> + return NOTIFY_OK; /* not for us */
>>> +
>>> + if (of_node_test_and_set_flag(rd->dn, OF_POPULATED))
>>> + return NOTIFY_OK;
>>
>> I don't think OF_POPULATED could be already set. It seems to be a
>> bug if it is.
>>
>>
>>> +
>>> + chip = of_find_gpiochip_by_node(rd->dn->parent);
>>> + if (chip == NULL)
>>> + return NOTIFY_OK; /* not for us */
>>
>> If I understand correctly, "not for us" is a misleading comment.
>> The notification is for the node rd->dn->parent, but the device
>> does not exist, so we can't do the hogging at the moment. (If the
>> device is created later, then the gpio hog child node will exist,
>> and the init will "do the right thing" with the hog node -- so
>> not a problem.)
>>
>>
>>> +
>>> + ret = of_gpiochip_add_hog(chip, rd->dn);
>>> + if (ret < 0) {
>>> + pr_err("%s: failed to add hogs for %pOF\n", __func__,
>>> + rd->dn);
>>> + of_node_clear_flag(rd->dn, OF_POPULATED);
>>> + return notifier_from_errno(ret);
>>> + }
>>> + break;
>>> +
>>> + case OF_RECONFIG_CHANGE_REMOVE:
>>> + if (!of_node_check_flag(rd->dn, OF_POPULATED))
>>> + return NOTIFY_OK; /* already depopulated */
>>
>> I don't think OF_POPULATED could be already cleared. It seems to be a
>> bug if it is.
>>
>>> +
>>> + chip = of_find_gpiochip_by_node(rd->dn->parent);
>>> + if (chip == NULL)
>>> + return NOTIFY_OK; /* not for us */
>>
>> Again, a misleading comment.
>>
>>> +
>>> + of_gpiochip_remove_hog(chip, rd->dn);
>>> + of_node_clear_flag(rd->dn, OF_POPULATED);
>>> + break;
>>> + }
>>> +
>>> + return NOTIFY_OK;
>>> +}
>>> +
>>> +struct notifier_block gpio_of_notifier = {
>>> + .notifier_call = of_gpio_notify,
>>> +};
>>> +#endif /* CONFIG_OF_DYNAMIC */
>>> +
>>> /**
>>> * of_gpio_simple_xlate - translate gpiospec to the GPIO number and flags
>>> * @gc: pointer to the gpio_chip structure
>>> diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
>>> index 9768831b1fe2f25b..ed26664f153782fc 100644
>>> --- a/drivers/gpio/gpiolib-of.h
>>> +++ b/drivers/gpio/gpiolib-of.h
>>> @@ -35,4 +35,6 @@ static inline bool of_gpio_need_valid_mask(const struct gpio_chip *gc)
>>> }
>>> #endif /* CONFIG_OF_GPIO */
>>>
>>> +extern struct notifier_block gpio_of_notifier;
>>> +
>>> #endif /* GPIOLIB_OF_H */
>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> index bff5ac774d870b67..ef12cfcaf0962c1c 100644
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -2952,6 +2952,9 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
>>> clear_bit(FLAG_PULL_DOWN, &desc->flags);
>>> clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
>>> clear_bit(FLAG_IS_HOGGED, &desc->flags);
>>> +#ifdef CONFIG_OF_DYNAMIC
>>> + desc->hog = NULL;
>>> +#endif
>>> ret = true;
>>> }
>>>
>>> @@ -5145,10 +5148,15 @@ static int __init gpiolib_dev_init(void)
>>> if (ret < 0) {
>>> pr_err("gpiolib: failed to allocate char dev region\n");
>>> bus_unregister(&gpio_bus_type);
>>> - } else {
>>> - gpiolib_initialized = true;
>>> - gpiochip_setup_devs();
>>> + return ret;
>>> }
>>> +
>>> + gpiolib_initialized = true;
>>> + gpiochip_setup_devs();
>>> +
>>> + if (IS_ENABLED(CONFIG_OF_DYNAMIC))
>>> + WARN_ON(of_reconfig_notifier_register(&gpio_of_notifier));
>>> +
>>> return ret;
>>> }
>>> core_initcall(gpiolib_dev_init);
>>> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
>>> index a4a759920faa48ab..7af9931e8572304a 100644
>>> --- a/drivers/gpio/gpiolib.h
>>> +++ b/drivers/gpio/gpiolib.h
>>> @@ -118,6 +118,9 @@ struct gpio_desc {
>>> const char *label;
>>> /* Name of the GPIO */
>>> const char *name;
>>> +#ifdef CONFIG_OF_DYNAMIC
>>> + struct device_node *hog;
>>> +#endif
>>> };
>>>
>>> int gpiod_request(struct gpio_desc *desc, const char *label);
>>>
>>
>>
>
>
