| Message ID | 20240912104630.1868285-2-ardb+git@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c/synquacer: Deal with optional PCLK correctly | expand |
(trying to merge t and cc fields from Ard's and Andy's messages) Le 12/09/2024 à 12:46, Ard Biesheuvel a écrit : > From: Ard Biesheuvel <ardb@kernel.org> > > ACPI boot does not provide clocks and regulators, but instead, provides > the PCLK rate directly, and enables the clock in firmware. So deal > gracefully with this. > > Fixes: 55750148e559 ("i2c: synquacer: Fix an error handling path in synquacer_i2c_probe()") Hi, If that matters, I'm not sure that the Fixes tag is correct. IIUC, either it is a new functionally that is added (now it works with ACPI...), or if considered as a fix, then I think that it is linked to commit 0d676a6c4390 ("i2c: add support for Socionext SynQuacer I2C controller"). I don't think that 55750148e559 introduced a regression. The issue seems to be there since the beginning. Agreed? If yes, then it may be needed to backport it in older kernels too. CJ > Cc: <stable@vger.kernel.org> > Cc: Andi Shyti <andi.shyti@kernel.org> > Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > https://lore.kernel.org/all/CAMj1kXFH+zB_YuUS+vaEpguhuVGLYbQw55VNDCxnBfSPe6b-nw@mail.gmail.com/T/#u > > drivers/i2c/busses/i2c-synquacer.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c > index 4eccbcd0fbfc..bbb9062669e4 100644 > --- a/drivers/i2c/busses/i2c-synquacer.c > +++ b/drivers/i2c/busses/i2c-synquacer.c > @@ -550,12 +550,13 @@ static int synquacer_i2c_probe(struct platform_device *pdev) > device_property_read_u32(&pdev->dev, "socionext,pclk-rate", > &i2c->pclkrate); > > - pclk = devm_clk_get_enabled(&pdev->dev, "pclk"); > + pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk"); > if (IS_ERR(pclk)) > return dev_err_probe(&pdev->dev, PTR_ERR(pclk), > "failed to get and enable clock\n"); > > - i2c->pclkrate = clk_get_rate(pclk); > + if (pclk) > + i2c->pclkrate = clk_get_rate(pclk); > > if (i2c->pclkrate < SYNQUACER_I2C_MIN_CLK_RATE || > i2c->pclkrate > SYNQUACER_I2C_MAX_CLK_RATE)
Le 12/09/2024 à 19:12, Ard Biesheuvel a écrit : > On Thu, 12 Sept 2024 at 19:11, Marion & Christophe JAILLET > <christophe.jaillet@wanadoo.fr> wrote: >> >> (trying to merge t and cc fields from Ard's and Andy's messages) >> >> >> Le 12/09/2024 à 12:46, Ard Biesheuvel a écrit : >>> From: Ard Biesheuvel <ardb@kernel.org> >>> >>> ACPI boot does not provide clocks and regulators, but instead, provides >>> the PCLK rate directly, and enables the clock in firmware. So deal >>> gracefully with this. >>> >>> Fixes: 55750148e559 ("i2c: synquacer: Fix an error handling path in synquacer_i2c_probe()") >> >> Hi, >> >> If that matters, I'm not sure that the Fixes tag is correct. >> >> IIUC, either it is a new functionally that is added (now it works with >> ACPI...), or if considered as a fix, then I think that it is linked to >> commit 0d676a6c4390 ("i2c: add support for Socionext SynQuacer I2C >> controller"). >> >> I don't think that 55750148e559 introduced a regression. The issue seems >> to be there since the beginning. Agreed? >> > > No. > > The original code used IS_ERR_OR_NULL() to explicitly permit the case > where no clock exists at all. Got it, this is not related to the removed _OR_NULL, but to the change of logic I've introduce. if (!IS_ERR) do_something_and_continue; if_NOENT_was_returned_we_still_get_there(); which became: if (IS_ERR) return; we_can_get_there_with_NOENT_anymore(); My bad! CJ > > This has worked fine with ACPI boot for many years before this fix was applied. > >> If yes, then it may be needed to backport it in older kernels too. >> > > No, it used to work. The fix is what broke ACPI boot. >
diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c index 4eccbcd0fbfc..bbb9062669e4 100644 --- a/drivers/i2c/busses/i2c-synquacer.c +++ b/drivers/i2c/busses/i2c-synquacer.c @@ -550,12 +550,13 @@ static int synquacer_i2c_probe(struct platform_device *pdev) device_property_read_u32(&pdev->dev, "socionext,pclk-rate", &i2c->pclkrate); - pclk = devm_clk_get_enabled(&pdev->dev, "pclk"); + pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk"); if (IS_ERR(pclk)) return dev_err_probe(&pdev->dev, PTR_ERR(pclk), "failed to get and enable clock\n"); - i2c->pclkrate = clk_get_rate(pclk); + if (pclk) + i2c->pclkrate = clk_get_rate(pclk); if (i2c->pclkrate < SYNQUACER_I2C_MIN_CLK_RATE || i2c->pclkrate > SYNQUACER_I2C_MAX_CLK_RATE)