RE: [PATCH v7 2/5] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver

From: yuji2.ishikawa
Date: Tue Jul 25 2023 - 02:16:07 EST

Next message: Costa Shulyupin: "[PATCH] docs: move powerpc under arch"
Previous message: Zhangjin Wu: "Re: [PATCH v1 1/8] tools/nolibc: add support for powerpc"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Helo Krzysztof
Thank you for your review comments.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Friday, July 14, 2023 5:00 PM
> To: ishikawa yuji(石川悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@xxxxxxxxxxxxx>; Hans Verkuil <hverkuil@xxxxxxxxx>; Sakari
> Ailus <sakari.ailus@xxxxxx>; Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx>; Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; iwamatsu nobuhiro(岩松信洋 ○ＤＩＴＣ□ＤＩＴ○
> ＯＳＴ) <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>; Mark Brown
> <broonie@xxxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 2/5] media: platform: visconti: Add Toshiba Visconti
> Video Input Interface driver
>
> 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 }?
>

I'll initialize it at declaration.
Same for similar cases.

> > +
> > + 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()
>

I'll use dev_err_probe().
Same for other suggestions.

> > + }
> > + 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()
>

I'll use 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?
>

Printing this error is useless for users in general?
If so, I'll drop this debug output.

> > + return -ENOMEM;
> > + }
> > + viif_dev->tables_dma = (struct viif_table_area *)tables_dma;
> > +
> > + /* power control */
>
> Drop the comment, it is useless.
>

I'll drop the comment

> > + 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
>

I'll use 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
>

I'll use 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
>

I'll use 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
>

I'll use 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
>

I'll use 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?
>

There should be. I'll add cleanup operations.

> > + 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?
>

There should be. I'll add calls of unregister functions.

> > + 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?
>

I'll drop this mutex call.

I was not sure if runtime_suspend() and runtime_resume() are not processed at the same time.
Now I understand callbacks are mutually exclusive (as documented in runtime_pm.rst).

> > + 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?
>

I'll drop this mutex call.

> > +
> > + /* 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

Best regards,
Yuji

Next message: Costa Shulyupin: "[PATCH] docs: move powerpc under arch"
Previous message: Zhangjin Wu: "Re: [PATCH v1 1/8] tools/nolibc: add support for powerpc"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]