Re: [PATCH] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6

[Srinivasa Rao Mandadapu]

Thanks for Your time Bjorn!!!

On 10/5/2021 10:31 PM, Bjorn Andersson wrote:
> On Fri 01 Oct 09:24 PDT 2021, Srinivasa Rao Mandadapu wrote:
>
> > Add support for soundwire 1.6 version to gate RX/TX bus clock.
> >
> > Signed-off-by: Venkata Prasad Potturu <potturu@xxxxxxxxxxxxxx>
> > Signed-off-by: Srinivasa Rao Mandadapu <srivasam@xxxxxxxxxxxxxx>
> > ---
> >   drivers/soundwire/qcom.c | 12 +++++++++++-
> >   1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> > index 0ef79d6..599b3ed 100644
> > --- a/drivers/soundwire/qcom.c
> > +++ b/drivers/soundwire/qcom.c
> > @@ -127,6 +127,7 @@ struct qcom_swrm_ctrl {
> >   	struct device *dev;
> >   	struct regmap *regmap;
> >   	void __iomem *mmio;
> > +	char __iomem *swrm_hctl_reg;
> >   	struct completion broadcast;
> >   	struct completion enumeration;
> >   	struct work_struct slave_work;
> > @@ -610,6 +611,12 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
> >   	val = FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, ctrl->rows_index);
> >   	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, ctrl->cols_index);
> >   
> > +	if (ctrl->swrm_hctl_reg) {
> > +		val = ioread32(ctrl->swrm_hctl_reg);
> > +		val &= 0xFFFFFFFD;
> That's a tricky way of saying:
>
> 	val &= ~BIT(1);
>
> That said, naming bit 1 is still a very good thing.
Okay. will change accordingly.
> > +		iowrite32(val, ctrl->swrm_hctl_reg);
> > +	}
> > +
> >   	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
> >   
> >   	/* Enable Auto enumeration */
> > @@ -1200,7 +1207,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
> >   	struct qcom_swrm_ctrl *ctrl;
> >   	const struct qcom_swrm_data *data;
> >   	int ret;
> > -	u32 val;
> > +	int val, swrm_hctl_reg = 0;
> >   
> >   	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> >   	if (!ctrl)
> > @@ -1251,6 +1258,9 @@ static int qcom_swrm_probe(struct platform_device *pdev)
> >   	ctrl->bus.port_ops = &qcom_swrm_port_ops;
> >   	ctrl->bus.compute_params = &qcom_swrm_compute_params;
> >   
> > +	if (!of_property_read_u32(dev->of_node, "qcom,swrm-hctl-reg", &swrm_hctl_reg))
> > +		ctrl->swrm_hctl_reg = devm_ioremap(&pdev->dev, swrm_hctl_reg, 0x4);
> Nack.
>
> You may not pull an address to a single register out of an undocumented
> DT property and blindly ioremap that.
>
> And you surely should check for errors here, to avoid magical errors
> caused by this ioremap failing and your bit not being cleared.
>
> Thanks,
> Bjorn
Okay. will add error check, as Bossart Suggested.
> > +
> >   	ret = qcom_swrm_get_port_config(ctrl);
> >   	if (ret)
> >   		goto err_clk;
> > --
> > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
> > is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.