| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ar5t2wdmxzvog7smlwbg3skg6ga35au6uiahfe3rlnmumlmpyr@572sf6ru6424>
Date: Wed, 26 Nov 2025 09:30:06 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Praveen Talari <praveen.talari@....qualcomm.com>
Cc: Andi Shyti <andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Mukesh Kumar Savaliya <mukesh.savaliya@....qualcomm.com>, Viken Dadhaniya <viken.dadhaniya@....qualcomm.com>,
Konrad Dybcio <konradybcio@...nel.org>, linux-arm-msm@...r.kernel.org, linux-i2c@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, psodagud@...cinc.com,
djaggi@...cinc.com, quic_msavaliy@...cinc.com, quic_vtanuku@...cinc.com,
quic_arandive@...cinc.com, quic_shazhuss@...cinc.com
Subject: Re: [PATCH v1 08/12] i2c: qcom-geni: Isolate serial engine setup
On Sat, Nov 22, 2025 at 10:30:14AM +0530, Praveen Talari wrote:
> Move serial engine configuration from probe to geni_i2c_init().
>
> Relocating the serial engine setup to a dedicated initialization function
> enhances code clarity and simplifies future modifications.
Please enhance commit message clarity. I don't think "code clarity" is
your most significant reason for this change, and "simplifies future
modification" is completely vague.
Be specific, the reader of this commit message hasn't implemented the
next set of commits, so they don't understand why this helps.
If the reason is that this simplifies the error handling around the
resource acquisition in the next patches, write that.
If my guess is wrong and the sole reason for you change is that you
don't like 179 lines long functions, then just say that.
Regards,
Bjorn
>
> Signed-off-by: Praveen Talari <praveen.talari@....qualcomm.com>
> ---
> drivers/i2c/busses/i2c-qcom-geni.c | 148 ++++++++++++++---------------
> 1 file changed, 73 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 3a04016db2c3..4111afe2713e 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -976,10 +976,75 @@ static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
> return ret;
> }
>
> +static int geni_i2c_init(struct geni_i2c_dev *gi2c)
> +{
> + const struct geni_i2c_desc *desc = NULL;
> + u32 proto, tx_depth;
> + bool fifo_disable;
> + int ret;
> +
> + ret = pm_runtime_resume_and_get(gi2c->se.dev);
> + if (ret < 0) {
> + dev_err(gi2c->se.dev, "error turning on device :%d\n", ret);
> + return ret;
> + }
> +
> + proto = geni_se_read_proto(&gi2c->se);
> + if (proto == GENI_SE_INVALID_PROTO) {
> + ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
> + if (ret) {
> + dev_err_probe(gi2c->se.dev, ret, "i2c firmware load failed ret: %d\n", ret);
> + goto err;
> + }
> + } else if (proto != GENI_SE_I2C) {
> + ret = dev_err_probe(gi2c->se.dev, -ENXIO, "Invalid proto %d\n", proto);
> + goto err;
> + }
> +
> + desc = device_get_match_data(gi2c->se.dev);
> + if (desc && desc->no_dma_support)
> + fifo_disable = false;
> + else
> + fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
> +
> + if (fifo_disable) {
> + /* FIFO is disabled, so we can only use GPI DMA */
> + gi2c->gpi_mode = true;
> + ret = setup_gpi_dma(gi2c);
> + if (ret)
> + goto err;
> +
> + dev_dbg(gi2c->se.dev, "Using GPI DMA mode for I2C\n");
> + } else {
> + gi2c->gpi_mode = false;
> + tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
> +
> + /* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
> + if (!tx_depth && desc)
> + tx_depth = desc->tx_fifo_depth;
> +
> + if (!tx_depth) {
> + ret = dev_err_probe(gi2c->se.dev, -EINVAL,
> + "Invalid TX FIFO depth\n");
> + goto err;
> + }
> +
> + gi2c->tx_wm = tx_depth - 1;
> + geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
> + geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
> + PACKING_BYTES_PW, true, true, true);
> +
> + dev_dbg(gi2c->se.dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
> + }
> +
> +err:
> + pm_runtime_put(gi2c->se.dev);
> + return ret;
> +}
> +
> static int geni_i2c_probe(struct platform_device *pdev)
> {
> struct geni_i2c_dev *gi2c;
> - u32 proto, tx_depth, fifo_disable;
> int ret;
> struct device *dev = &pdev->dev;
> const struct geni_i2c_desc *desc = NULL;
> @@ -1059,79 +1124,19 @@ static int geni_i2c_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - ret = clk_prepare_enable(gi2c->core_clk);
> - if (ret)
> - return ret;
> -
> - ret = geni_se_resources_on(&gi2c->se);
> - if (ret) {
> - dev_err_probe(dev, ret, "Error turning on resources\n");
> - goto err_clk;
> - }
> - proto = geni_se_read_proto(&gi2c->se);
> - if (proto == GENI_SE_INVALID_PROTO) {
> - ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
> - if (ret) {
> - dev_err_probe(dev, ret, "i2c firmware load failed ret: %d\n", ret);
> - goto err_resources;
> - }
> - } else if (proto != GENI_SE_I2C) {
> - ret = dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
> - goto err_resources;
> - }
> -
> - if (desc && desc->no_dma_support)
> - fifo_disable = false;
> - else
> - fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
> -
> - if (fifo_disable) {
> - /* FIFO is disabled, so we can only use GPI DMA */
> - gi2c->gpi_mode = true;
> - ret = setup_gpi_dma(gi2c);
> - if (ret)
> - goto err_resources;
> -
> - dev_dbg(dev, "Using GPI DMA mode for I2C\n");
> - } else {
> - gi2c->gpi_mode = false;
> - tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
> -
> - /* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
> - if (!tx_depth && desc)
> - tx_depth = desc->tx_fifo_depth;
> -
> - if (!tx_depth) {
> - ret = dev_err_probe(dev, -EINVAL,
> - "Invalid TX FIFO depth\n");
> - goto err_resources;
> - }
> -
> - gi2c->tx_wm = tx_depth - 1;
> - geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
> - geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
> - PACKING_BYTES_PW, true, true, true);
> -
> - dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
> - }
> -
> - clk_disable_unprepare(gi2c->core_clk);
> - ret = geni_se_resources_off(&gi2c->se);
> - if (ret) {
> - dev_err_probe(dev, ret, "Error turning off resources\n");
> - goto err_dma;
> - }
> -
> - ret = geni_icc_disable(&gi2c->se);
> - if (ret)
> - goto err_dma;
> -
> gi2c->suspended = 1;
> pm_runtime_set_suspended(gi2c->se.dev);
> pm_runtime_set_autosuspend_delay(gi2c->se.dev, I2C_AUTO_SUSPEND_DELAY);
> pm_runtime_use_autosuspend(gi2c->se.dev);
> pm_runtime_enable(gi2c->se.dev);
>
> + ret = geni_i2c_init(gi2c);
> + if (ret < 0) {
> + dev_err(gi2c->se.dev, "I2C init failed :%d\n", ret);
> + pm_runtime_disable(gi2c->se.dev);
> + goto err_dma;
> + }
> +
> ret = i2c_add_adapter(&gi2c->adap);
> if (ret) {
> dev_err_probe(dev, ret, "Error adding i2c adapter\n");
> @@ -1143,13 +1148,6 @@ static int geni_i2c_probe(struct platform_device *pdev)
>
> return ret;
>
> -err_resources:
> - geni_se_resources_off(&gi2c->se);
> -err_clk:
> - clk_disable_unprepare(gi2c->core_clk);
> -
> - return ret;
> -
> err_dma:
> release_gpi_dma(gi2c);
>
> --
> 2.34.1
>
Powered by blists - more mailing lists