| 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: <15a28ddc-9883-42d5-a008-b2ea22d8becc@oss.qualcomm.com>
Date: Fri, 28 Nov 2025 11:52:05 +0530
From: Praveen Talari <praveen.talari@....qualcomm.com>
To: Bjorn Andersson <andersson@...nel.org>
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
Hi Bjorn,
Thank you for review.
On 11/26/2025 9:00 PM, Bjorn Andersson wrote:
> 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.
>
Moving the serial engine setup to geni_i2c_init() API for a cleaner
probe function and utilizes the PM runtime API to control resources
instead of direct clock-related APIs for better resource management.
Enables reusability of the serial engine initialization in future use
cases like hibernation and deep sleep features where hardware context is
lost.
I hope the commit text above should be appropriate.
Thanks,
Praveen
> 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