Re: [PATCH v11 2/2] memory: mtk-smi: mt8188: Add SMI reset and clamp

["Friday Yang (杨阳)" <Friday.Yang@mediatek.com>]

On Sat, 2025-10-18 at 18:41 +0200, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 17/09/2025 14:07, Friday Yang wrote:
> >  static int mtk_smi_device_link_common(struct device *dev, struct
> > device **com_dev)
> >  {
> >       struct platform_device *smi_com_pdev;
> > @@ -638,6 +711,46 @@ static int mtk_smi_dts_clk_init(struct device
> > *dev, struct mtk_smi *smi,
> >       return ret;
> >  }
> > 
> > +static int mtk_smi_larb_parse_syscon(struct mtk_smi_larb *larb,
> > int larbid)
> > +{
> > +     struct device *dev = larb->dev;
> > +     const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
> > +     int ret;
> > +
> > +     larb->smi_comm_in_port_id = larb_gen->clamp_port[larbid];
> > +     larb->smi_comm_syscon = syscon_regmap_lookup_by_phandle(dev-
> > >of_node,
> > +                                                             "medi
> > atek,smi");
> 
> The code already parses this phandle (in other place). Why do you
> need
> second time?
> 

Thanks for comments. We did parse the 'mediatek,smi' property in
mtk_smi_device_link_common. We need to modify
'mtk_smi_device_link_common' function if this need to be fixed.
Below is the modification, is this acceptable for you?

static int mtk_smi_device_link_common(struct device *dev, struct device
**com_dev,bool require_clamp)
{
...
	struct mtk_smi_larb *larb;
	struct mtk_smi_larb_gen *larb_gen;
	int larbid, ret;

	smi_com_node = of_parse_phandle(dev->of_node, "mediatek,smi",
0);
	if (!smi_com_node)
		return -EINVAL;
	smi_com_pdev = of_find_device_by_node(smi_com_node);
...
	if (require_clamp) {
		larb = container_of(com_dev, struct mtk_smi_larb,
smi_common_dev);
		larb_gen = larb->larb_gen;
		larbid = larb->larb_id;
		larb->smi_comm_in_port_id = larb_gen-
>clamp_port[larbid];
		larb->smi_comm_syscon =
syscon_node_to_regmap(smi_com_node);
		if (IS_ERR(larb->smi_comm_syscon)) {
			dev_err(dev, "Failed to get smi syscon for larb %d\n", larbid);
			ret = PTR_ERR(larb->smi_comm_syscon);
			larb->smi_comm_syscon = NULL;
			goto err_remove_link;
		}
	}
	of_node_put(smi_com_node);
	return 0;

err_remove_link:
	device_link_remove(dev, smi_com_dev);
err_put_device:
	put_device(&smi_com_pdev->dev);
err_put_node:
	of_node_put(smi_com_node);
	return ret;
}

static int mtk_smi_larb_probe(struct platform_device *pdev)
{
	bool require_clamp;
...

	/* The larbid are sequential for IOMMU if this property is not
present */
	ret = of_property_read_s32(dev->of_node, "mediatek,larb-id",
&larb->larbid);
	if (ret == -EINVAL)
		goto add_dev_link;
	else if (ret || larb->larbid >= MTK_LARB_NR_MAX)
		return dev_err_probe(dev, -EINVAL, "Invalid
larbid:%d\n", larb->larbid);

	if (larb->larb_gen->clamp_port && larb->larb_gen-
>clamp_port[larb->larbid])
		require_clamp = true;

add_dev_link:
	ret = mtk_smi_device_link_common(dev, &larb->smi_common_dev,
require_clamp);
	if (ret < 0)
		return ret;

	/*
	 * Only SMI LARBs in camera, image and IPE subsys need to
	 * apply clamp and reset operations, others can be skipped.
	 */
	if (require_clamp) {
		ret = mtk_smi_larb_parse_reset(larb);
		if (ret)
			goto err_link_remove;
	}

pm_runtime_en:
	pm_runtime_enable(dev);
...
}

static int mtk_smi_common_probe(struct platform_device *pdev)
{
...
	if (common->plat->type == MTK_SMI_GEN2_SUB_COMM) {
		ret = mtk_smi_device_link_common(dev, &common-
>smi_common_dev, false);
		if (ret < 0)
			return ret;
	}
...
}

> > +     if (IS_ERR(larb->smi_comm_syscon)) {
> > +             ret = PTR_ERR(larb->smi_comm_syscon);
> > +             larb->smi_comm_syscon = NULL;
> > +             return dev_err_probe(dev, ret,
> > +                                  "Failed to get smi syscon for
> > larb %d\n", larbid);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int mtk_smi_larb_parse_reset(struct mtk_smi_larb *larb)
> > +{
> > +     struct device *dev = larb->dev;
> > +     int ret;
> > +
> > +     larb->rst_con = devm_reset_control_get_exclusive(dev,
> > "larb");
> > +     if (IS_ERR(larb->rst_con))
> > +             return dev_err_probe(dev, PTR_ERR(larb->rst_con),
> > +                                  "Failed to get reset
> > controller\n");
> 
> 
> This looks like ABI break. Aren't all devices affected?
> 

This does not affect other devices, we use this 'clamp_port' to
judge whether to apply clamp or not in 'mtk_smi_larb_probe', 
like below:
	if (larb->larb_gen->clamp_port && larb->larb_gen-
>clamp_port[larb->larbid])
		require_clamp = true;


static const struct mtk_smi_larb_gen mtk_smi_larb_mt8188 = {
	...
	.clamp_port                 = mtk_smi_larb_clamp_port_mt8188,
};

> > +     larb->nb.notifier_call = mtk_smi_genpd_callback;
> > +     ret = dev_pm_genpd_add_notifier(dev, &larb->nb);
> > +     if (ret) {
> > +             larb->nb.notifier_call = NULL;
> > +             return dev_err_probe(dev, ret,
> > +                                  "Failed to add genpd
> > callback\n");
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> 
> Best regards,
> Krzysztof