* Re: [PATCH 1/3] dt-bindings: mtk-jpeg: Add binding for MT8195 JPG
[not found] ` <1624428350-1424-2-git-send-email-kyrie.wu@mediatek.com>
@ 2021-06-25 9:18 ` Tzung-Bi Shih
[not found] ` <1625039759.22769.3.camel@mhfsdcap03>
0 siblings, 1 reply; 3+ messages in thread
From: Tzung-Bi Shih @ 2021-06-25 9:18 UTC (permalink / raw)
To: kyrie.wu
Cc: Hans Verkuil, Mauro Carvalho Chehab, Rob Herring, Rick Chang,
Bin Liu, Matthias Brugger, Tzung-Bi Shih,
Project_Global_Chrome_Upstream_Group, linux-media, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek, Tomasz Figa,
xia.jiang, maoguang.meng, srv_heupstream
On Wed, Jun 23, 2021 at 2:06 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt | 3 +++
> 1 file changed, 3 insertions(+)
Note: the patch won't apply after [1].
[1]: https://lore.kernel.org/patchwork/patch/1445298/
> Required properties:
> - compatible : "mediatek,mt2701-jpgenc"
> +- compatible : "mediatek,mt8195-jpgenc"
> +- compatible : "mediatek,mt8195-jpgenc0"
> +- compatible : "mediatek,mt8195-jpgenc1"
Why it needs 3 compatible strings?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/3] media: mtk-jpegenc: use component framework to manage jpg HW
[not found] ` <1624428350-1424-3-git-send-email-kyrie.wu@mediatek.com>
@ 2021-06-25 9:18 ` Tzung-Bi Shih
0 siblings, 0 replies; 3+ messages in thread
From: Tzung-Bi Shih @ 2021-06-25 9:18 UTC (permalink / raw)
To: kyrie.wu
Cc: Hans Verkuil, Mauro Carvalho Chehab, Rob Herring, Rick Chang,
Bin Liu, Matthias Brugger, Tzung-Bi Shih,
Project_Global_Chrome_Upstream_Group, linux-media, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek, Tomasz Figa,
xia.jiang, maoguang.meng, srv_heupstream
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/3] dt-bindings: mtk-jpeg: Add binding for MT8195 JPG
[not found] ` <1625039759.22769.3.camel@mhfsdcap03>
@ 2021-07-05 8:09 ` Tzung-Bi Shih
0 siblings, 0 replies; 3+ messages in thread
From: Tzung-Bi Shih @ 2021-07-05 8:09 UTC (permalink / raw)
To: kyrie.wu
Cc: Hans Verkuil, Mauro Carvalho Chehab, Rob Herring, Rick Chang,
Bin Liu, Matthias Brugger, Tzung-Bi Shih,
Project_Global_Chrome_Upstream_Group, linux-media, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek, Tomasz Figa,
xia.jiang, maoguang.meng, srv_heupstream
On Wed, Jun 30, 2021 at 3:56 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> Mt8195 has two hardwares, "mediatek,mt8195-jpgenc0" for hw0 and
> "mediatek,mt8195-jpgenc1" for HW1. These two nodes will register
> hardware interrupt, initialize clock, power domain, remap register base
> addr and other operations. But the device node is not registered.
> "mediatek,mt8195-jpgenc" will register the device node to represent jpeg
> encode device. Then the component framework is used to manage the above
> two hardwares.
Please don't top-posting. Inline your replies so that people can
easily follow the discussion.
I still don't quite understand why it needs to introduce 2 compatible
strings. If hw0 and hw1 are different from interrupts, clocks, power
domain, and etc, couldn't you use the same compatible string (e.g.
"mt8195-jpgenc") and provide them different DT attributes?
> On Fri, 2021-06-25 at 17:18 +0800, Tzung-Bi Shih wrote:
> > On Wed, Jun 23, 2021 at 2:06 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> > > Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt | 3 +++
> > > 1 file changed, 3 insertions(+)
> > Note: the patch won't apply after [1].
> >
> > [1]: https://lore.kernel.org/patchwork/patch/1445298/
> >
> > > Required properties:
> > > - compatible : "mediatek,mt2701-jpgenc"
> > > +- compatible : "mediatek,mt8195-jpgenc"
> > > +- compatible : "mediatek,mt8195-jpgenc0"
> > > +- compatible : "mediatek,mt8195-jpgenc1"
> > Why it needs 3 compatible strings?
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-07-05 8:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1624428350-1424-1-git-send-email-kyrie.wu@mediatek.com>
[not found] ` <1624428350-1424-2-git-send-email-kyrie.wu@mediatek.com>
2021-06-25 9:18 ` [PATCH 1/3] dt-bindings: mtk-jpeg: Add binding for MT8195 JPG Tzung-Bi Shih
[not found] ` <1625039759.22769.3.camel@mhfsdcap03>
2021-07-05 8:09 ` Tzung-Bi Shih
[not found] ` <1624428350-1424-3-git-send-email-kyrie.wu@mediatek.com>
2021-06-25 9:18 ` [PATCH 2/3] media: mtk-jpegenc: use component framework to manage jpg HW Tzung-Bi Shih
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).