On Wed Jul 24, 2024 at 12:29 AM BST, Stephen Boyd wrote: > Quoting Harry Austen (2024-07-20 05:01:53) > > Xilinx clocking wizard IP core supports monitoring of up to four > > optional user clock inputs, with a corresponding interrupt for > > notification in change of clock state (stop, underrun, overrun or > > glitch). Give userspace access to this monitor logic through use of the > > UIO framework. > > > > Use presence of the user monitor interrupt description in devicetree to > > indicate whether or not the UIO device should be registered. Also, this > > functionality is only supported from v6.0 onwards, so add indication of > > support to the device match data, in order to be tied to the utilised > > compatible string. > > > > Signed-off-by: Harry Austen <hpausten@xxxxxxxxxxxxxx> > > --- > > diff --git a/drivers/clk/xilinx/Kconfig b/drivers/clk/xilinx/Kconfig > > index 051756953558b..907a435694687 100644 > > --- a/drivers/clk/xilinx/Kconfig > > +++ b/drivers/clk/xilinx/Kconfig > > @@ -21,6 +21,7 @@ config COMMON_CLK_XLNX_CLKWZRD > > tristate "Xilinx Clocking Wizard" > > depends on OF > > depends on HAS_IOMEM > > + depends on UIO > > If I have a pre-v6.0 device I probably don't want UIO though. Perhaps > you should use the auxiliary bus framework to register a device that is > otherwise unused and then have the uio driver live in drivers/uio and > match that device made here. I think you can have 'imply UIO' if you > like to put a weak Kconfig dependency. Yeah I wasn't particularly happy about the UIO usage here. It seems like a nice way to leave control up to the user in userspace though, rather than adding a bunch of device attributes to read status registers, configure interrupts etc. but I agree the dependency isn't good here. Will look into your suggestion for v2. > > > help > > Support for the Xilinx Clocking Wizard IP core clock generator. > > Adds support for clocking wizard and compatible. > > diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > > index 7b262d73310fe..2d419e8ad4419 100644 > > --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > > +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > > @@ -1165,6 +1209,17 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > return -EINVAL; > > } > > > > + data = device_get_match_data(&pdev->dev); > > + if (data && data->supports_monitor) { > > + irq = platform_get_irq(pdev, 0); > > + if (irq > 0) { > > + ret = clk_wzrd_setup_monitor(&pdev->dev, irq, > > + platform_get_resource(pdev, IORESOURCE_IO, 0)); > > Any reason this can't be > > ret = clk_wzrd_setup_monitor(pdev); > if (ret) > return ret; > > and then all the surrounding code be moved into the function, including > the dev_err_probe()? That makes a lot more sense. Don't know what I was doing. Thanks! > > > + if (ret) > > + return dev_err_probe(&pdev->dev, ret, "failed to setup monitor\n"); > > + } > > + } > > + > > ret = of_property_read_u32(np, "xlnx,nr-outputs", &nr_outputs);