On 16/01/2025 15:00, Tudor Ambarus wrote:
> +
> +/**
> + * acpm_put_handle() - release the handle acquired by acpm_get_by_phandle.
> + * @handle: Handle acquired by acpm_get_by_phandle.
> + */
> +static void acpm_handle_put(const struct acpm_handle *handle)
> +{
> + struct acpm_info *acpm = handle_to_acpm_info(handle);
> + struct device *dev = acpm->dev;
> +
> + module_put(dev->driver->owner);
> + /* Drop reference taken with of_find_device_by_node(). */
> + put_device(dev);
> +}
> +
> +/**
> + * devm_acpm_release() - devres release method.
> + * @dev: pointer to device.
> + * @res: pointer to resource.
> + */
> +static void devm_acpm_release(struct device *dev, void *res)
> +{
> + acpm_handle_put(*(struct acpm_handle **)res);
> +}
> +
> +/**
> + * acpm_get_by_phandle() - get the ACPM handle using DT phandle.
> + * @dev: device pointer for the consumer device.
> + *
> + * Return: pointer to handle on success, ERR_PTR(-errno) otherwise.
> + */
> +static const struct acpm_handle *acpm_get_by_phandle(struct device *dev)
"by_phandle" takes the name of the property with phandle as an argument,
because otherwise you do not have here phandle part at all in the
interface (see syscon API).
Other option would be by actual phandle - see of_find_node_by_phandle().
I would propose in such case only acpm_get() or maybe better of_acpm_get()?
> +{
> + struct device_node *np __free(device_node);
That's not the expected syntax. You miss here initializer/constructor or
this should be placed in place of initialization (just like everywhere)
... or just drop cleanup because it does not make things much
smaller/easier.
> + struct platform_device *pdev;
> + struct device_link *link;
> + struct acpm_info *acpm;
> +
> + if (!dev || !dev->of_node)
> + return ERR_PTR(-ENODEV);
> +
> + np = of_parse_phandle(dev->of_node, "exynos,acpm_ipc", 0);
You need bindings for this somewhere and fix the underscore->hyphen...
and vendor prefix. It really would not be accepted that way so please
post consumer bindings anywhere.
> + if (!np)
> + return ERR_PTR(-ENODEV);
> +
> + pdev = of_find_device_by_node(np);
> + if (!pdev) {
> + dev_err(dev, "Cannot find device node %s\n", np->name);
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + acpm = platform_get_drvdata(pdev);
> + if (!acpm) {
> + dev_err(dev, "Cannot get drvdata from %s\n",
> + dev_name(&pdev->dev));
> + platform_device_put(pdev);
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + if (!try_module_get(pdev->dev.driver->owner)) {
> + dev_err(dev, "Cannot get module reference.\n");
> + platform_device_put(pdev);
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
> + if (!link) {
> + dev_err(&pdev->dev,
> + "Failed to create device link to consumer %s.\n",
> + dev_name(dev));
> + platform_device_put(pdev);
> + module_put(pdev->dev.driver->owner);
If you keep usage of __free(), it is fine. If you drop it, then
implement common exit/error handling path with gotos.
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return &acpm->handle;
> +}
> +
Rest looked good to me.
Best regards,
Krzysztof
