On 09/01/2024 12:58, Philipp Zabel wrote:
- /* Not freed in normal path, persisent subsyst data */
- rgpio_dev = kzalloc(sizeof(*rgpio_dev), GFP_KERNEL);
Since this is persistent, instead of letting the reset-gpio driver call of_parse_phandle_with_args() again, this could be passed in via platform data. Is there a reason not to do that instead?
We can pass it as read only platform data, but we cannot pass the ownership. This is associated with registered platform device, not with bound one device->driver.
Imagine case:
- modprobe reset-gpio,
- Driver is bound to the device,
- unbind (echo > unbind)
- rmmod
- goto 1
Keeping ownership on the list is fine, the reset-gpio driver makes its own copy of of_phandle_args anyway. I was just wondering whether it could make this copy from platform data instead of from the of_parse_phandle_with_args() return value.
Looks like it could. This could save us few lines of code in reset-gpio.c. I'll try it.
[...]
@@ -839,21 +960,50 @@ __of_reset_control_get(struct device_node *node, const char *id, int index, index, &args); if (ret == -EINVAL) return ERR_PTR(ret);
- if (ret)
return optional ? NULL : ERR_PTR(ret);
- if (ret) {
/** There can be only one reset-gpio for regular devices, so* don't bother with GPIO index.*/I don't understand this comment. The GPIO index should be checked as part of __reset_gpios_args_match(), or should it not?
This and earlier comment are result of a bit hacky approach to the problem: how to find reset controllers for that GPIO?
The point is that our reset gpio controller has only 1 reset, thus of_reset_n_cells=1. However args_count from of_parse_handle is >0, which later is compared in reset core:
https://elixir.bootlin.com/linux/latest/source/drivers/reset/core.c#L859
That part we need to match.
I could make the reset-gpio driver to have of_reset_n_cells=2, but what would be the point? The rest of the cells are not really relevant, because you cannot refer to this reset gpio controller with any other arguments.
To remind: my solution spawns one reset-gpio controller for one GPIO.
Thank you. I think we could also just make that check
if (WARN_ON(!rcdev->of_args && ...))
instead and skip the of_xlate call in that case (or implement of_xlate in the reset-gpio driver to make this more explicit).
Ack
ret = of_parse_phandle_with_args(node, "reset-gpios", "#gpio-cells",0, &args);if (ret)return optional ? NULL : ERR_PTR(ret);
- mutex_lock(&reset_list_mutex);
- rcdev = NULL;
- list_for_each_entry(r, &reset_controller_list, list) {
if (args.np == r->of_node) {rcdev = r;break;}
gpio_fallback = true;Is there a reason not just call __reset_add_reset_gpio_device() here? With that, there should be no need to call __reset_find_rcdev() twice.
Hm, could be, although not sure if code would be simpler.
This entire function handles two cases:
- Get normal reset controller ("resets" OF property),
- If above fails, get reset-gpio controller ("reset-gpios" OF property)
Therefore the entire solution is following approach:
- of_parse_phandle(resets)
1b. error? Then of_parse_phandle(reset-gpios) 2. Find reset-controller based on any of above phandles. 3. error? Check if we created reset-gpios platform device. If not: create new reset-gpios platform device/ 3b. Platform device could probe, so lookup again for reset controller or defer probe.
What type of flow do you propose?
I propose to reorder after parsing the phandles: check/create the gpio platform device right after parsing the gpio handle. Only then lock reset_list_mutex look for the rcdev.
- of_parse_phandle(resets)
1b. error? Then of_parse_phandle(reset-gpios) 2b. gpio? Then check if we created reset-gpios platform device. If not: create new reset-gpios platform device/, defer if probe failed 3. Lock reset_list_mutex, find reset-controller based on any of above phandles.
Could work, let me try. I have impression this was my first approach which resulted in a bit more complicated code, but I don't remember the details now.
}
- mutex_lock(&reset_list_mutex);
- rcdev = __reset_find_rcdev(&args, gpio_fallback, NULL);
This gets called with args as parsed. If there is a match, this will overwrite args (in the gpio_fallback case) and return NULL.
Overwrite not complete. It will only overwrite args_count and return a valid rcdev. I do not see overwriting in case of returning NULL.
Sorry, I meant to write
"This gets called with args as parsed. If there is a match, this will overwrite args (in the gpio_fallback case) _or_ return NULL."
at least at the end, when I understood the following.
- if (!rcdev) {
So in this non-NULL branch there was no overwriting.
rstc = ERR_PTR(-EPROBE_DEFER);goto out;
if (gpio_fallback) {/** Registering reset-gpio device might cause immediate* bind, thus taking reset_list_mutex lock via* reset_controller_register().*/mutex_unlock(&reset_list_mutex);ret = __reset_add_reset_gpio_device(node, &args);So this will also be called with args as parsed.
mutex_lock(&reset_list_mutex);if (ret) {rstc = ERR_PTR(ret);goto out;}/** Success: reset-gpio could probe immediately, so* re-check the lookup.*/rcdev = __reset_find_rcdev(&args, gpio_fallback, NULL);And this will again be called with args as parsed and overwrite args again.>
if (!rcdev) {rstc = ERR_PTR(-EPROBE_DEFER);goto out;}/* Success, rcdev is valid thus do not bail out */} else {rstc = ERR_PTR(-EPROBE_DEFER);goto out;}So at this point args is overwritten in the gpio_fallback case. I would find it much clearer to just overwrite args here and make the first parameter to __reset_find_rcdev() const.
I think I get your point. Overwriting happens after we store the original of_args, but the code is indeed not intuitive. I think I can move it further, as you suggested.
Now I think we can skip the overwriting altogether and just adapt the following of_reset_n_cells check ad of_xlate call as mentioned above.
Yep!
Best regards, Krzysztof