| Message ID | 20200409013947.12667-3-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: pl111: Fix module loading issues | expand |
Hi Rob. On Wed, Apr 08, 2020 at 07:39:46PM -0600, Rob Herring wrote: > The init VExpress variants currently instantiates a 'muxfpga' driver for > the sole purpose of getting a regmap for it. There's no reason to > instantiate a driver and doing so just complicates things. The muxfpga > driver also isn't unregistered properly on module unload. Let's > just simplify all this this by just calling > devm_regmap_init_vexpress_config() directly. > > Cc: Eric Anholt <eric@anholt.net> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Rob Herring <robh@kernel.org> Procastinating, so I took a look at this. Nice simplification - on nit below. Acked-by: Sam Ravnborg <sam@ravnborg.org> > --- > drivers/gpu/drm/pl111/pl111_versatile.c | 21 +++---------- > drivers/gpu/drm/pl111/pl111_vexpress.c | 42 ------------------------- > drivers/gpu/drm/pl111/pl111_vexpress.h | 7 ----- > 3 files changed, 4 insertions(+), 66 deletions(-) > > diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c > index 09aeaffb7660..8c2551088f26 100644 > --- a/drivers/gpu/drm/pl111/pl111_versatile.c > +++ b/drivers/gpu/drm/pl111/pl111_versatile.c > @@ -8,6 +8,7 @@ > #include <linux/of.h> > #include <linux/of_platform.h> > #include <linux/regmap.h> > +#include <linux/vexpress.h> > > #include "pl111_versatile.h" > #include "pl111_vexpress.h" > @@ -325,17 +326,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv) > versatile_clcd_type = (enum versatile_clcd)clcd_id->data; > > /* Versatile Express special handling */ > - if (versatile_clcd_type == VEXPRESS_CLCD_V2M) { > + if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) { > struct platform_device *pdev; > - > - /* Registers a driver for the muxfpga */ > - ret = vexpress_muxfpga_init(); > - if (ret) { > - dev_err(dev, "unable to initialize muxfpga driver\n"); > - of_node_put(np); > - return ret; > - } > - > /* Call into deep Vexpress configuration API */ > pdev = of_find_device_by_node(np); > if (!pdev) { > @@ -343,13 +335,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv) > of_node_put(np); > return -EPROBE_DEFER; > } > - map = dev_get_drvdata(&pdev->dev); > - if (!map) { > - dev_err(dev, "sysreg has not yet probed\n"); > - platform_device_put(pdev); > - of_node_put(np); > - return -EPROBE_DEFER; > - } > + map = devm_regmap_init_vexpress_config(&pdev->dev); > + platform_device_put(pdev); > } else { > map = syscon_node_to_regmap(np); > } On the following line there is: if (IS_ERR(map)) { dev_err(dev, "no Versatile syscon regmap\n"); return PTR_ERR(map); } The error message no longer tell if this was devm_regmap_init_vexpress_config() or syscon_node_to_regmap() that caused the error. Sam > diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c > index 350570fe06b5..37ed3f1da820 100644 > --- a/drivers/gpu/drm/pl111/pl111_vexpress.c > +++ b/drivers/gpu/drm/pl111/pl111_vexpress.c > @@ -94,45 +94,3 @@ int pl111_vexpress_clcd_init(struct device *dev, > > return 0; > } > - > -/* > - * This sets up the regmap pointer that will then be retrieved by > - * the detection code in pl111_versatile.c and passed in to the > - * pl111_vexpress_clcd_init() function above. > - */ > -static int vexpress_muxfpga_probe(struct platform_device *pdev) > -{ > - struct device *dev = &pdev->dev; > - struct regmap *map; > - > - map = devm_regmap_init_vexpress_config(&pdev->dev); > - if (IS_ERR(map)) > - return PTR_ERR(map); > - dev_set_drvdata(dev, map); > - > - return 0; > -} > - > -static const struct of_device_id vexpress_muxfpga_match[] = { > - { .compatible = "arm,vexpress-muxfpga", }, > - {} > -}; > - > -static struct platform_driver vexpress_muxfpga_driver = { > - .driver = { > - .name = "vexpress-muxfpga", > - .of_match_table = of_match_ptr(vexpress_muxfpga_match), > - }, > - .probe = vexpress_muxfpga_probe, > -}; > - > -int vexpress_muxfpga_init(void) > -{ > - int ret; > - > - ret = platform_driver_register(&vexpress_muxfpga_driver); > - /* -EBUSY just means this driver is already registered */ > - if (ret == -EBUSY) > - ret = 0; > - return ret; > -} > diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h > index 5d3681bb4c00..bb54864ca91e 100644 > --- a/drivers/gpu/drm/pl111/pl111_vexpress.h > +++ b/drivers/gpu/drm/pl111/pl111_vexpress.h > @@ -10,8 +10,6 @@ int pl111_vexpress_clcd_init(struct device *dev, > struct pl111_drm_dev_private *priv, > struct regmap *map); > > -int vexpress_muxfpga_init(void); > - > #else > > static inline int pl111_vexpress_clcd_init(struct device *dev, > @@ -21,9 +19,4 @@ static inline int pl111_vexpress_clcd_init(struct device *dev, > return -ENODEV; > } > > -static inline int vexpress_muxfpga_init(void) > -{ > - return 0; > -} > - > #endif > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Apr 9, 2020 at 9:16 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Rob. > > On Wed, Apr 08, 2020 at 07:39:46PM -0600, Rob Herring wrote: > > The init VExpress variants currently instantiates a 'muxfpga' driver for > > the sole purpose of getting a regmap for it. There's no reason to > > instantiate a driver and doing so just complicates things. The muxfpga > > driver also isn't unregistered properly on module unload. Let's > > just simplify all this this by just calling > > devm_regmap_init_vexpress_config() directly. > > > > Cc: Eric Anholt <eric@anholt.net> > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Rob Herring <robh@kernel.org> > > Procastinating, so I took a look at this. > Nice simplification - on nit below. > Acked-by: Sam Ravnborg <sam@ravnborg.org> > > --- > > drivers/gpu/drm/pl111/pl111_versatile.c | 21 +++---------- > > drivers/gpu/drm/pl111/pl111_vexpress.c | 42 ------------------------- > > drivers/gpu/drm/pl111/pl111_vexpress.h | 7 ----- > > 3 files changed, 4 insertions(+), 66 deletions(-) > > > > diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c > > index 09aeaffb7660..8c2551088f26 100644 > > --- a/drivers/gpu/drm/pl111/pl111_versatile.c > > +++ b/drivers/gpu/drm/pl111/pl111_versatile.c > > @@ -8,6 +8,7 @@ > > #include <linux/of.h> > > #include <linux/of_platform.h> > > #include <linux/regmap.h> > > +#include <linux/vexpress.h> > > > > #include "pl111_versatile.h" > > #include "pl111_vexpress.h" > > @@ -325,17 +326,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv) > > versatile_clcd_type = (enum versatile_clcd)clcd_id->data; > > > > /* Versatile Express special handling */ > > - if (versatile_clcd_type == VEXPRESS_CLCD_V2M) { > > + if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) { > > struct platform_device *pdev; > > - > > - /* Registers a driver for the muxfpga */ > > - ret = vexpress_muxfpga_init(); > > - if (ret) { > > - dev_err(dev, "unable to initialize muxfpga driver\n"); > > - of_node_put(np); > > - return ret; > > - } > > - > > /* Call into deep Vexpress configuration API */ > > pdev = of_find_device_by_node(np); > > if (!pdev) { > > @@ -343,13 +335,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv) > > of_node_put(np); > > return -EPROBE_DEFER; > > } > > - map = dev_get_drvdata(&pdev->dev); > > - if (!map) { > > - dev_err(dev, "sysreg has not yet probed\n"); > > - platform_device_put(pdev); > > - of_node_put(np); > > - return -EPROBE_DEFER; > > - } > > + map = devm_regmap_init_vexpress_config(&pdev->dev); > > + platform_device_put(pdev); > > } else { > > map = syscon_node_to_regmap(np); > > } > > On the following line there is: > if (IS_ERR(map)) { > dev_err(dev, "no Versatile syscon regmap\n"); > return PTR_ERR(map); > } > > The error message no longer tell if this was > devm_regmap_init_vexpress_config() or syscon_node_to_regmap() that > caused the error. Hopefully you'd know what platform you are on. In any case, it's changed after patch 3. Rob
On Thu, Apr 9, 2020 at 3:39 AM Rob Herring <robh@kernel.org> wrote: > The init VExpress variants currently instantiates a 'muxfpga' driver for > the sole purpose of getting a regmap for it. There's no reason to > instantiate a driver and doing so just complicates things. The muxfpga > driver also isn't unregistered properly on module unload. Let's > just simplify all this this by just calling > devm_regmap_init_vexpress_config() directly. > > Cc: Eric Anholt <eric@anholt.net> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Rob Herring <robh@kernel.org> OK... looking at this. > + if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) { > struct platform_device *pdev; > - > - /* Registers a driver for the muxfpga */ > - ret = vexpress_muxfpga_init(); > - if (ret) { > - dev_err(dev, "unable to initialize muxfpga driver\n"); > - of_node_put(np); > - return ret; > - } > - > /* Call into deep Vexpress configuration API */ > pdev = of_find_device_by_node(np); So this finds the platform device for compatible "arm,vexpress-muxfpga", ha! > + map = devm_regmap_init_vexpress_config(&pdev->dev); > + platform_device_put(pdev); So then you can just do it like that. Clever! Hats off for digging through my (unnecessary complex) code. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c index 09aeaffb7660..8c2551088f26 100644 --- a/drivers/gpu/drm/pl111/pl111_versatile.c +++ b/drivers/gpu/drm/pl111/pl111_versatile.c @@ -8,6 +8,7 @@ #include <linux/of.h> #include <linux/of_platform.h> #include <linux/regmap.h> +#include <linux/vexpress.h> #include "pl111_versatile.h" #include "pl111_vexpress.h" @@ -325,17 +326,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv) versatile_clcd_type = (enum versatile_clcd)clcd_id->data; /* Versatile Express special handling */ - if (versatile_clcd_type == VEXPRESS_CLCD_V2M) { + if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) { struct platform_device *pdev; - - /* Registers a driver for the muxfpga */ - ret = vexpress_muxfpga_init(); - if (ret) { - dev_err(dev, "unable to initialize muxfpga driver\n"); - of_node_put(np); - return ret; - } - /* Call into deep Vexpress configuration API */ pdev = of_find_device_by_node(np); if (!pdev) { @@ -343,13 +335,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv) of_node_put(np); return -EPROBE_DEFER; } - map = dev_get_drvdata(&pdev->dev); - if (!map) { - dev_err(dev, "sysreg has not yet probed\n"); - platform_device_put(pdev); - of_node_put(np); - return -EPROBE_DEFER; - } + map = devm_regmap_init_vexpress_config(&pdev->dev); + platform_device_put(pdev); } else { map = syscon_node_to_regmap(np); } diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c index 350570fe06b5..37ed3f1da820 100644 --- a/drivers/gpu/drm/pl111/pl111_vexpress.c +++ b/drivers/gpu/drm/pl111/pl111_vexpress.c @@ -94,45 +94,3 @@ int pl111_vexpress_clcd_init(struct device *dev, return 0; } - -/* - * This sets up the regmap pointer that will then be retrieved by - * the detection code in pl111_versatile.c and passed in to the - * pl111_vexpress_clcd_init() function above. - */ -static int vexpress_muxfpga_probe(struct platform_device *pdev) -{ - struct device *dev = &pdev->dev; - struct regmap *map; - - map = devm_regmap_init_vexpress_config(&pdev->dev); - if (IS_ERR(map)) - return PTR_ERR(map); - dev_set_drvdata(dev, map); - - return 0; -} - -static const struct of_device_id vexpress_muxfpga_match[] = { - { .compatible = "arm,vexpress-muxfpga", }, - {} -}; - -static struct platform_driver vexpress_muxfpga_driver = { - .driver = { - .name = "vexpress-muxfpga", - .of_match_table = of_match_ptr(vexpress_muxfpga_match), - }, - .probe = vexpress_muxfpga_probe, -}; - -int vexpress_muxfpga_init(void) -{ - int ret; - - ret = platform_driver_register(&vexpress_muxfpga_driver); - /* -EBUSY just means this driver is already registered */ - if (ret == -EBUSY) - ret = 0; - return ret; -} diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h index 5d3681bb4c00..bb54864ca91e 100644 --- a/drivers/gpu/drm/pl111/pl111_vexpress.h +++ b/drivers/gpu/drm/pl111/pl111_vexpress.h @@ -10,8 +10,6 @@ int pl111_vexpress_clcd_init(struct device *dev, struct pl111_drm_dev_private *priv, struct regmap *map); -int vexpress_muxfpga_init(void); - #else static inline int pl111_vexpress_clcd_init(struct device *dev, @@ -21,9 +19,4 @@ static inline int pl111_vexpress_clcd_init(struct device *dev, return -ENODEV; } -static inline int vexpress_muxfpga_init(void) -{ - return 0; -} - #endif
The init VExpress variants currently instantiates a 'muxfpga' driver for the sole purpose of getting a regmap for it. There's no reason to instantiate a driver and doing so just complicates things. The muxfpga driver also isn't unregistered properly on module unload. Let's just simplify all this this by just calling devm_regmap_init_vexpress_config() directly. Cc: Eric Anholt <eric@anholt.net> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Rob Herring <robh@kernel.org> --- drivers/gpu/drm/pl111/pl111_versatile.c | 21 +++---------- drivers/gpu/drm/pl111/pl111_vexpress.c | 42 ------------------------- drivers/gpu/drm/pl111/pl111_vexpress.h | 7 ----- 3 files changed, 4 insertions(+), 66 deletions(-)