Messages in this thread | | | From | Tzung-Bi Shih <> | Date | Fri, 25 Jun 2021 17:18:30 +0800 | Subject | Re: [PATCH 2/3] media: mtk-jpegenc: use component framework to manage jpg HW |
| |
On Wed, Jun 23, 2021 at 2:06 PM kyrie.wu <kyrie.wu@mediatek.com> wrote: > Mtk jpeg encoder has several hardware, one HW may register a device > node; use component framework to manage jpg HW device node, > in this case, one device node could represent all jpg HW. Can roughly understand. But could you rephrase your sentences?
> #include <media/videobuf2-core.h> > #include <media/videobuf2-dma-contig.h> > #include <soc/mediatek/smi.h> > +#include <linux/component.h> Maintain the alphabetical order.
> +void mtk_jpeg_put_buf(struct mtk_jpeg_dev *jpeg) > +{ > + struct mtk_jpeg_ctx *ctx = NULL; > + struct vb2_v4l2_buffer *dst_buffer = NULL; > + struct list_head *temp_entry = NULL; > + struct list_head *pos = NULL; > + struct mtk_jpeg_src_buf *dst_done_buf = NULL, *tmp_dst_done_buf = NULL; Remove the initialization if they don't need to.
> + unsigned long flags; > + > + ctx = jpeg->hw_param.curr_ctx; > + if (!ctx) { > + pr_err("%s : %d, comp_jpeg ctx fail !!!\n", __func__, __LINE__); Use dev_err().
> + return; > + } > + > + dst_buffer = jpeg->hw_param.dst_buffer; > + if (!dst_buffer) { > + pr_err("%s : %d, comp_jpeg dst_buffer fail !!!\n", > + __func__, __LINE__); Use dev_err().
> + if (!(irq_status & JPEG_ENC_INT_STATUS_DONE)) { > + pr_err("%s : %d, jpeg encode failed\n", __func__, __LINE__); Use dev_err().
> +void mtk_jpegenc_timeout_work(struct work_struct *work) > +{ > + struct mtk_jpeg_dev *jpeg = container_of(work, struct mtk_jpeg_dev, > + job_timeout_work.work); > + struct mtk_jpeg_ctx *ctx = NULL; It doesn't need to initialize.
> +static const struct of_device_id mtk_jpegenc_drv_ids[] = { Remove the extra space between "static" and "const".
> + { > + .compatible = "mediatek,mt8195-jpgenc0", > + .data = (void *)MTK_JPEGENC_HW0, > + }, > + { > + .compatible = "mediatek,mt8195-jpgenc1", > + .data = (void *)MTK_JPEGENC_HW1, > + }, Using compatible strings to separate them doesn't sound like a scalable method.
> #include <linux/kernel.h> > #include <media/videobuf2-core.h> > #include <media/videobuf2-dma-contig.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/slab.h> > +#include <linux/component.h> > +#include <linux/clk.h> > +#include <linux/pm_runtime.h> Maintain the alphabetical order.
> #include "mtk_jpeg_enc_hw.h" > +#include "mtk_jpeg_core.h" Maintain the alphabetical order.
> +int mtk_jpegenc_init_pm(struct mtk_jpeg_dev *mtkdev) > +{ > + struct platform_device *pdev; > + struct mtk_jpegenc_pm *pm; > + struct mtk_jpegenc_clk *jpegenc_clk; > + struct mtk_jpegenc_clk_info *clk_info; > + int i = 0, ret = 0; They don't need to initialize.
> + pdev = mtkdev->plat_dev; > + pm = &mtkdev->pm; To be concise, can inline to above when declaring the variables.
> + jpegenc_clk->clk_num = > + of_property_count_strings(pdev->dev.of_node, "clock-names"); > + if (jpegenc_clk->clk_num > 0) { > + jpegenc_clk->clk_info = devm_kcalloc(&pdev->dev, > + jpegenc_clk->clk_num, > + sizeof(*clk_info), > + GFP_KERNEL); > + if (!jpegenc_clk->clk_info) > + return -ENOMEM; > + } else { > + pr_err("Failed to get jpegenc clock count\n"); Use dev_err(). > + return -EINVAL; > + } Would prefer the block turn to be:
if (... <= 0) { ... return -EINVAL; }
... = devm_kcalloc(...); if (!...) return -ENOMEM;
> + for (i = 0; i < jpegenc_clk->clk_num; i++) { > + clk_info = &jpegenc_clk->clk_info[i]; > + ret = of_property_read_string_index(pdev->dev.of_node, > + "clock-names", i, > + &clk_info->clk_name); > + if (ret) { > + pr_err("Failed to get jpegenc clock name id = %d", i); Use dev_err().
> + return ret; > + } > + > + clk_info->jpegenc_clk = devm_clk_get(&pdev->dev, > + clk_info->clk_name); > + if (IS_ERR(clk_info->jpegenc_clk)) { > + pr_err("devm_clk_get (%d)%s fail", > + i, clk_info->clk_name); Use dev_err().
> + pm_runtime_enable(&pdev->dev); > + return ret; return 0;
> +void mtk_jpegenc_release_pm(struct mtk_jpeg_dev *dev) > +{ > + pm_runtime_disable(dev->pm.dev); > +} Would prefer this function to be more "symmetric" to mtk_jpegenc_init_pm().
For example:
void mtk_jpegenc_release_pm(struct mtk_jpeg_dev *mtkdev) { struct platform_device *pdev = mtkdev->plat_dev; pm_runtime_disable(&pdev->dev); }
That way, it doesn't rely on whether mtkdev->pm is set or not.
> + ret = devm_request_irq(&pdev->dev, dev->jpegenc_irq, > + irq_handler, 0, pdev->name, dev); > + if (ret) { > + dev_err(&pdev->dev, "Failed to install dev->jpegenc_irq %d (%d)", > + dev->jpegenc_irq, ret); > + > + return -ENOENT; How about just returning ret?
> + } > + > + //disable_irq(dev->jpegenc_irq); Remove it.
> + ret = component_add(&pdev->dev, &mtk_jpegenc_hw_component_ops); > + if (ret) { > + dev_err(&pdev->dev, "Failed to component_add: %d\n", ret); > + goto err; > + } How about just returning component_add(...)?
> +err: > + mtk_jpegenc_release_pm(dev); Would expect the platform driver to have a .remove() callback and invoke the mtk_jpegenc_release_pm() too.
> +static const struct of_device_id mtk_jpegenc_hw_ids[] = { > + { > + .compatible = "mediatek,mt8195-jpgenc0", > + .data = mtk_jpegenc_hw_irq_handler, > + }, > + { .compatible = "mediatek,mt8195-jpgenc1", > + .data = mtk_jpegenc_hw_irq_handler, > + }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, mtk_jpegenc_hw_ids); Had the same concern in dt-bindings patch. Does it really need multiple compatible strings to separate?
Also, the block should guard by using CONFIG_OF.
> +struct platform_driver mtk_jpegenc_hw_driver = { > + .probe = mtk_jpegenc_hw_probe, > + .driver = { > + .name = "mtk-jpegenc-hw", > + .of_match_table = mtk_jpegenc_hw_ids, Should guard by using of_match_ptr().
Hi, after reading the patch for a while, I realized it is way too big to me so that I didn't go through too much detail (especially the component framework part). Could you further divide the series into smaller pieces? For example: - part i. refactor to make modifying code easier - part ii. leverage component framework - part iii. add new code for MT8195 I would expect part i and ii don't change the original behavior.
| |