Re: [PATCH 5/7] clk: clocking-wizard: add user clock monitor support

[Harry Austen]

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);