| Message ID | 20210526193327.70468-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,1/2] can: mcp251xfd: Try to get crystal clock rate from property | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
On 26.05.2021 22:33:26, Andy Shevchenko wrote: > In some configurations, mainly ACPI-based, the clock frequency of the device > is supplied by very well established 'clock-frequency' property. Hence, try > to get it from the property at last if no other providers are available. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > v2: new patch > drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c > index e0ae00e34c7b..e42f87c3f2ec 100644 > --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c > +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c > @@ -2856,7 +2856,7 @@ static int mcp251xfd_probe(struct spi_device *spi) > struct gpio_desc *rx_int; > struct regulator *reg_vdd, *reg_xceiver; > struct clk *clk; > - u32 freq; > + u32 freq, rate; > int err; > > if (!spi->irq) > @@ -2883,11 +2883,16 @@ static int mcp251xfd_probe(struct spi_device *spi) > return dev_err_probe(&spi->dev, PTR_ERR(reg_xceiver), > "Failed to get Transceiver regulator!\n"); > > - clk = devm_clk_get(&spi->dev, NULL); > + /* Always ask for fixed clock rate from a property. */ > + device_property_read_u32(&spi->dev, "clock-frequency", &rate); what about error handling....? > + > + clk = devm_clk_get_optional(&spi->dev, NULL); > if (IS_ERR(clk)) > return dev_err_probe(&spi->dev, PTR_ERR(clk), > "Failed to get Oscillator (clock)!\n"); > freq = clk_get_rate(clk); > + if (freq == 0) > + freq = rate; ... this means we don't fail if there is neither a clk nor a clock-frequency property. I've send a v3 to fix this. > > /* Sanity check */ > if (freq < MCP251XFD_SYSCLOCK_HZ_MIN || > -- > 2.30.2 > > Marc
On Mon, May 31, 2021 at 10:47:20AM +0200, Marc Kleine-Budde wrote: > On 26.05.2021 22:33:26, Andy Shevchenko wrote: > > In some configurations, mainly ACPI-based, the clock frequency of the device > > is supplied by very well established 'clock-frequency' property. Hence, try > > to get it from the property at last if no other providers are available. > > return dev_err_probe(&spi->dev, PTR_ERR(reg_xceiver), > > "Failed to get Transceiver regulator!\n"); > > > > - clk = devm_clk_get(&spi->dev, NULL); > > + /* Always ask for fixed clock rate from a property. */ > > + device_property_read_u32(&spi->dev, "clock-frequency", &rate); > > what about error handling....? Not needed, but rate should be assigned to 0, which is missed. > > + clk = devm_clk_get_optional(&spi->dev, NULL); > > if (IS_ERR(clk)) > > return dev_err_probe(&spi->dev, PTR_ERR(clk), > > "Failed to get Oscillator (clock)!\n"); > > freq = clk_get_rate(clk); > > + if (freq == 0) > > + freq = rate; > > ... this means we don't fail if there is neither a clk nor a > clock-frequency property. The following will check for it (which is already in the code) if (freq <= MCP251XFD_SYSCLOCK_HZ_MAX / MCP251XFD_OSC_PLL_MULTIPLIER) { > I've send a v3 to fix this. You mean I have to send v3? Sure!
On Mon, May 31, 2021 at 12:58:29PM +0300, Andy Shevchenko wrote: > On Mon, May 31, 2021 at 10:47:20AM +0200, Marc Kleine-Budde wrote: > > On 26.05.2021 22:33:26, Andy Shevchenko wrote: > > > In some configurations, mainly ACPI-based, the clock frequency of the device > > > is supplied by very well established 'clock-frequency' property. Hence, try > > > to get it from the property at last if no other providers are available. > > > > return dev_err_probe(&spi->dev, PTR_ERR(reg_xceiver), > > > "Failed to get Transceiver regulator!\n"); > > > > > > - clk = devm_clk_get(&spi->dev, NULL); > > > + /* Always ask for fixed clock rate from a property. */ > > > + device_property_read_u32(&spi->dev, "clock-frequency", &rate); > > > > what about error handling....? > > Not needed, but rate should be assigned to 0, which is missed. > > > > + clk = devm_clk_get_optional(&spi->dev, NULL); > > > if (IS_ERR(clk)) > > > return dev_err_probe(&spi->dev, PTR_ERR(clk), > > > "Failed to get Oscillator (clock)!\n"); > > > freq = clk_get_rate(clk); > > > + if (freq == 0) > > > + freq = rate; > > > > ... this means we don't fail if there is neither a clk nor a > > clock-frequency property. > > The following will check for it (which is already in the code) > > if (freq <= MCP251XFD_SYSCLOCK_HZ_MAX / MCP251XFD_OSC_PLL_MULTIPLIER) { Even before, actually, /* Sanity check */ if (freq < MCP251XFD_SYSCLOCK_HZ_MIN || freq > MCP251XFD_SYSCLOCK_HZ_MAX) { > > I've send a v3 to fix this. > > You mean I have to send v3? > Sure! So, I am going to send a v3 with amended commit message and assigning rate to 0.
On 31.05.2021 12:58:29, Andy Shevchenko wrote: > On Mon, May 31, 2021 at 10:47:20AM +0200, Marc Kleine-Budde wrote: > > On 26.05.2021 22:33:26, Andy Shevchenko wrote: > > > In some configurations, mainly ACPI-based, the clock frequency of the device > > > is supplied by very well established 'clock-frequency' property. Hence, try > > > to get it from the property at last if no other providers are available. > > > > return dev_err_probe(&spi->dev, PTR_ERR(reg_xceiver), > > > "Failed to get Transceiver regulator!\n"); > > > > > > - clk = devm_clk_get(&spi->dev, NULL); > > > + /* Always ask for fixed clock rate from a property. */ > > > + device_property_read_u32(&spi->dev, "clock-frequency", &rate); > > > > what about error handling....? > > Not needed, but rate should be assigned to 0, which is missed. > > > > + clk = devm_clk_get_optional(&spi->dev, NULL); > > > if (IS_ERR(clk)) > > > return dev_err_probe(&spi->dev, PTR_ERR(clk), > > > "Failed to get Oscillator (clock)!\n"); > > > freq = clk_get_rate(clk); > > > + if (freq == 0) > > > + freq = rate; > > > > ... this means we don't fail if there is neither a clk nor a > > clock-frequency property. > > The following will check for it (which is already in the code) > > if (freq <= MCP251XFD_SYSCLOCK_HZ_MAX / MCP251XFD_OSC_PLL_MULTIPLIER) { Good point. > > I've send a v3 to fix this. > > You mean I have to send v3? > Sure! I have send a v3, see: https://lore.kernel.org/r/20210531084444.1785397-1-mkl@pengutronix.de Marc
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c index e0ae00e34c7b..e42f87c3f2ec 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c @@ -2856,7 +2856,7 @@ static int mcp251xfd_probe(struct spi_device *spi) struct gpio_desc *rx_int; struct regulator *reg_vdd, *reg_xceiver; struct clk *clk; - u32 freq; + u32 freq, rate; int err; if (!spi->irq) @@ -2883,11 +2883,16 @@ static int mcp251xfd_probe(struct spi_device *spi) return dev_err_probe(&spi->dev, PTR_ERR(reg_xceiver), "Failed to get Transceiver regulator!\n"); - clk = devm_clk_get(&spi->dev, NULL); + /* Always ask for fixed clock rate from a property. */ + device_property_read_u32(&spi->dev, "clock-frequency", &rate); + + clk = devm_clk_get_optional(&spi->dev, NULL); if (IS_ERR(clk)) return dev_err_probe(&spi->dev, PTR_ERR(clk), "Failed to get Oscillator (clock)!\n"); freq = clk_get_rate(clk); + if (freq == 0) + freq = rate; /* Sanity check */ if (freq < MCP251XFD_SYSCLOCK_HZ_MIN ||
In some configurations, mainly ACPI-based, the clock frequency of the device is supplied by very well established 'clock-frequency' property. Hence, try to get it from the property at last if no other providers are available. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- v2: new patch drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)