On Mon, Mar 27, 2023 at 04:47:31PM +0300, Abel Vesa wrote:
> + /* For now this driver only supports ICE version 3 and 4. */
> + if (major != 3 && major != 4) {
> + dev_warn(dev, "Unsupported ICE version: v%d.%d.%d\n",
> + major, minor, step);
> + return false;
> + }
Version 4 support was not in the original. This ought to be mentioned in the
commit message.
> +struct qcom_ice *of_qcom_ice_get(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct qcom_ice *ice = ERR_PTR(-EPROBE_DEFER);
> + struct device_node *node;
> + struct resource *res;
> + void __iomem *base;
> +
> + if (!dev || !dev->of_node)
> + return ERR_PTR(-ENODEV);
> +
> + /* legacy has ice reg range in the consumer DT node */
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice");
> + if (res) {
> + base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(base))
> + return base;
> +
> + /* create ICE instance using consumer dev */
> + return qcom_ice_create(pdev, base);
> + }
> +
> + node = of_parse_phandle(dev->of_node, "qcom,ice", 0);
> + if (!node) {
> + ice = NULL;
> + goto out;
> + }
I think a longer comment in this code explaining the legacy implementation vs.
the new implementation would be helpful.
> + pdev = of_find_device_by_node(node);
> + if (!pdev) {
> + dev_err(dev, "Cannot find device node %s\n", node->name);
> + goto out;
> + }
It is hard to understand the return value in this case, since
'ice = ERR_PTR(-EPROBE_DEFER)' happens way above. Maybe do:
if (!pdev) {
dev_err(dev, "Cannot find device node %s\n", node->name);
ice = ERR_PTR(-EPROBE_DEFER);
goto out;
}
> +
> + ice = platform_get_drvdata(pdev);
> + if (!ice) {
> + dev_err(dev, "Cannot get ice\n");
> + put_device(&pdev->dev);
> + return ERR_PTR(-ENODEV);
> + }
Can this error message be more descriptive?
Otherwise this patch is looking good, thanks!
- Eric
