On 2020-04-16 3:55 pm, Russell King - ARM Linux admin wrote:
On Thu, Apr 16, 2020 at 03:37:40PM +0100, Robin Murphy wrote:
On 2020-04-16 2:50 pm, Andrew Lunn wrote:
[...]
Clocking with Marvell devices has always been interesting. Core IP
like this gets reused between different generations of SoCs. The
original Orion5x had no clock control at all. Latter SoCs have had
more and more complex clock trees. So care has to be taken to not
change old behaviour when adding support for new clocks.
FWIW, that sounds like a good argument for encoding the clock requirements
of each variant in the of_match_data, so the driver doesn't have to simply
trust the DT and hope.
Please read my patches. This is exactly what I'm doing. I'm preserving
as closely as possible the current driver behaviour while adding support
for the Armada 8040 PWM while keeping compatibility with older DT.
And I'm doing that by keying off the match data, exactly as you're
suggesting above.
AFAICS you're encoding the *PWM capability* in the match data and using
that to extend the existing behaviour, which comprises using soc_variant
to maybe treat the stashed error code as fatal somewhere else much later
if CONFIG_PWM happens to be enabled, and is subtle enough that at least
two reviewers overlooked or failed to make sense of it.
Compare and contrast with how self-contained and obvious this is:
- mvchip->clk = devm_clk_get(&pdev->dev, NULL);
- /* Not all SoCs require a clock.*/
- if (!IS_ERR(mvchip->clk))
- clk_prepare_enable(mvchip->clk);
+ /* Not all SoCs require a clock.*/
+ if (data->needs_clock)
+ mvchip->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(mvchip->clk))
+ return PTR_ERR(mvchip_clk);
+ clk_prepare_enable(mvchip->clk);
+ }
If achieving the same end result by very different and roundabout means
constitutes "exactly the same thing", does me having written this email
mean that my house is exactly the same as the Arm office and someone
else will be along to clean the kitchen shortly? Here's hoping... :D
Robin.
