On 14/07/2023 03:50, Yuji Ishikawa wrote: > Add support to Video Input Interface on Toshiba Visconti ARM SoCs. > The interface device includes CSI2 Receiver, > frame grabber, video DMAC and image signal processor. > > A driver instance provides three /dev/videoX device files; > one for RGB image capture, another one for optional RGB capture > with different parameters and the last one for RAW capture. > ... > +static int visconti_viif_parse_dt(struct viif_device *viif_dev) > +{ > + struct device_node *of = viif_dev->dev->of_node; > + struct v4l2_fwnode_endpoint fw_ep; > + struct viif_subdev *viif_sd; > + struct device_node *ep; > + unsigned int i; > + int num_ep; > + int ret; > + > + memset(&fw_ep, 0, sizeof(struct v4l2_fwnode_endpoint)); Why you cannot initialize it in declaration? = { 0 }? > + > + num_ep = of_graph_get_endpoint_count(of); > + if (!num_ep) > + return -ENODEV; > + > + ret = visconti_viif_init_async_subdevs(viif_dev, num_ep); > + if (ret) > + return ret; > + > + for (i = 0; i < num_ep; i++) { > + ep = of_graph_get_endpoint_by_regs(of, 0, i); > + if (!ep) { > + dev_err(viif_dev->dev, "No subdevice connected on endpoint %u.\n", i); > + ret = -ENODEV; > + goto error_put_node; > + } > + > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &fw_ep); > + if (ret) { > + dev_err(viif_dev->dev, "Unable to parse endpoint #%u.\n", i); > + goto error_put_node; > + } > + > + if (fw_ep.bus_type != V4L2_MBUS_CSI2_DPHY || > + fw_ep.bus.mipi_csi2.num_data_lanes == 0) { > + dev_err(viif_dev->dev, "missing CSI-2 properties in endpoint\n"); > + ret = -EINVAL; > + goto error_put_node; > + } > + > + /* Setup the ceu subdevice and the async subdevice. */ > + viif_sd = &viif_dev->subdevs[i]; > + INIT_LIST_HEAD(&viif_sd->asd.list); > + > + viif_sd->num_lane = fw_ep.bus.mipi_csi2.num_data_lanes; > + viif_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > + viif_sd->asd.match.fwnode = > + fwnode_graph_get_remote_port_parent(of_fwnode_handle(ep)); > + > + viif_dev->asds[i] = &viif_sd->asd; > + of_node_put(ep); > + } > + > + return num_ep; > + > +error_put_node: > + of_node_put(ep); > + return ret; > +} > + > +static const struct of_device_id visconti_viif_of_table[] = { > + { > + .compatible = "toshiba,visconti5-viif", > + }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, visconti_viif_of_table); > + > +#define NUM_IRQS 3 > +#define IRQ_ID_STR "viif" > + > +static int visconti_viif_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct viif_device *viif_dev; > + dma_addr_t tables_dma; > + int ret, i, num_sd; > + > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36)); > + if (ret) > + return ret; > + > + viif_dev = devm_kzalloc(dev, sizeof(*viif_dev), GFP_KERNEL); > + if (!viif_dev) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, viif_dev); > + viif_dev->dev = dev; > + > + spin_lock_init(&viif_dev->regbuf_lock); > + mutex_init(&viif_dev->pow_lock); > + mutex_init(&viif_dev->stream_lock); > + > + viif_dev->capture_reg = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(viif_dev->capture_reg)) > + return PTR_ERR(viif_dev->capture_reg); > + > + viif_dev->csi2host_reg = devm_platform_ioremap_resource(pdev, 1); > + if (IS_ERR(viif_dev->csi2host_reg)) > + return PTR_ERR(viif_dev->csi2host_reg); > + > + viif_dev->hwaif_reg = devm_platform_ioremap_resource(pdev, 2); > + if (IS_ERR(viif_dev->hwaif_reg)) > + return PTR_ERR(viif_dev->hwaif_reg); > + > + viif_dev->mpu_reg = devm_platform_ioremap_resource(pdev, 3); > + if (IS_ERR(viif_dev->mpu_reg)) > + return PTR_ERR(viif_dev->mpu_reg); > + > + viif_dev->run_flag_main = false; > + > + for (i = 0; i < NUM_IRQS; i++) { > + ret = platform_get_irq(pdev, i); > + if (ret < 0) { > + dev_err(dev, "failed to acquire irq resource\n"); > + return ret; return dev_err_probe() > + } > + viif_dev->irq[i] = ret; > + ret = devm_request_irq(dev, viif_dev->irq[i], visconti_viif_irq, 0, IRQ_ID_STR, > + viif_dev); > + if (ret) { > + dev_err(dev, "irq request failed\n"); return dev_err_probe() > + return ret; > + } > + } > + > + viif_dev->tables = > + dma_alloc_wc(dev, sizeof(struct viif_table_area), &tables_dma, GFP_KERNEL); > + if (!viif_dev->tables) { > + dev_err(dev, "dma_alloc_wc failed\n"); Are you sure DMA memory allocation errors shall be printed? > + return -ENOMEM; > + } > + viif_dev->tables_dma = (struct viif_table_area *)tables_dma; > + > + /* power control */ Drop the comment, it is useless. > + pm_runtime_enable(dev); > + > + /* build media_dev */ > + viif_dev->media_dev.hw_revision = 0; > + strscpy(viif_dev->media_dev.model, VIIF_DRIVER_NAME, sizeof(viif_dev->media_dev.model)); > + viif_dev->media_dev.dev = dev; > + /* TODO: platform:visconti-viif-0,1,2,3 for each VIIF driver instance */ > + snprintf(viif_dev->media_dev.bus_info, sizeof(viif_dev->media_dev.bus_info), "%s-0", > + VIIF_BUS_INFO_BASE); > + media_device_init(&viif_dev->media_dev); > + > + /* build v4l2_dev */ > + viif_dev->v4l2_dev.mdev = &viif_dev->media_dev; > + ret = v4l2_device_register(dev, &viif_dev->v4l2_dev); > + if (ret) > + goto error_dma_free; > + > + ret = media_device_register(&viif_dev->media_dev); > + if (ret) { > + dev_err(dev, "Failed to register media device: %d\n", ret); > + goto error_v4l2_unregister; dev_err_probe > + } > + > + ret = visconti_viif_csi2rx_register(viif_dev); > + if (ret) { > + dev_err(dev, "failed to register csi2rx sub node: %d\n", ret); dev_err_probe > + goto error_media_unregister; > + } > + > + ret = visconti_viif_isp_register(viif_dev); > + if (ret) { > + dev_err(dev, "failed to register isp sub node: %d\n", ret); dev_err_probe > + goto error_media_unregister; > + } > + ret = visconti_viif_capture_register(viif_dev); > + if (ret) { > + dev_err(dev, "failed to register capture node: %d\n", ret); dev_err_probe > + goto error_media_unregister; > + } > + > + /* handle subdevices in device tree */ > + num_sd = visconti_viif_parse_dt(viif_dev); > + if (ret < 0) { > + ret = num_sd; ret = dev_err_probe > + goto error_media_unregister; > + } > + > + viif_dev->notifier.v4l2_dev = &viif_dev->v4l2_dev; > + v4l2_async_nf_init(&viif_dev->notifier); > + for (i = 0; i < num_sd; i++) > + __v4l2_async_nf_add_subdev(&viif_dev->notifier, viif_dev->asds[i]); > + viif_dev->notifier.ops = &viif_notify_ops; > + ret = v4l2_async_nf_register(&viif_dev->v4l2_dev, &viif_dev->notifier); > + if (ret) > + goto error_media_unregister; > + > + viif_dev->wq = create_workqueue("visconti-viif"); > + if (!viif_dev->wq) > + return -ENOMEM; No error cleanup? > + INIT_WORK(&viif_dev->work, visconti_viif_wthread_l1info); > + > + return 0; > + > +error_media_unregister: > + media_device_unregister(&viif_dev->media_dev); > +error_v4l2_unregister: > + v4l2_device_unregister(&viif_dev->v4l2_dev); > +error_dma_free: > + pm_runtime_disable(dev); > + dma_free_wc(&pdev->dev, sizeof(struct viif_table_area), viif_dev->tables, > + (dma_addr_t)viif_dev->tables_dma); > + return ret; > +} > + > +static int visconti_viif_remove(struct platform_device *pdev) > +{ > + struct viif_device *viif_dev = platform_get_drvdata(pdev); > + > + destroy_workqueue(viif_dev->wq); > + visconti_viif_isp_unregister(viif_dev); > + visconti_viif_capture_unregister(viif_dev); > + v4l2_async_nf_unregister(&viif_dev->notifier); Why these three are not called in probe error paths? > + media_device_unregister(&viif_dev->media_dev); > + v4l2_device_unregister(&viif_dev->v4l2_dev); > + pm_runtime_disable(&pdev->dev); > + dma_free_wc(&pdev->dev, sizeof(struct viif_table_area), viif_dev->tables, > + (dma_addr_t)viif_dev->tables_dma); > + > + return 0; > +} > + > +static int visconti_viif_runtime_suspend(struct device *dev) > +{ > + /* This callback is kicked when the last device-file is closed */ > + struct viif_device *viif_dev = dev_get_drvdata(dev); > + > + mutex_lock(&viif_dev->pow_lock); Why? > + visconti_viif_hw_off(viif_dev); > + mutex_unlock(&viif_dev->pow_lock); > + > + return 0; > +} > + > +static int visconti_viif_runtime_resume(struct device *dev) > +{ > + /* This callback is kicked when the first device-file is opened */ > + struct viif_device *viif_dev = dev_get_drvdata(dev); > + > + viif_dev->rawpack_mode = (u32)VIIF_RAWPACK_DISABLE; > + > + mutex_lock(&viif_dev->pow_lock); Why? > + > + /* Initialize HWD driver */ > + visconti_viif_hw_on(viif_dev); > + > + /* VSYNC mask setting of MAIN unit */ > + viif_main_vsync_set_irq_mask(viif_dev, MASK_INT_M_SYNC_MASK_SET); > + > + /* STATUS error mask setting of MAIN unit */ > + viif_main_status_err_set_irq_mask(viif_dev, MASK_INT_M_DELAY_INT_ERROR); > + > + /* VSYNC mask settings of SUB unit */ > + viif_sub_vsync_set_irq_mask(viif_dev, MASK_INT_S_SYNC_MASK_SET); > + > + /* STATUS error mask setting(unmask) of SUB unit */ > + viif_sub_status_err_set_irq_mask(viif_dev, > + MASK_INT_S_RESERVED_SET | MASK_INT_S_DELAY_INT_ERROR); > + > + mutex_unlock(&viif_dev->pow_lock); > + > + return 0; > +} > + Best regards, Krzysztof