[PATCH 0/4] Simplify mediatek clk driver probes

[PATCH 0/4] Simplify mediatek clk driver probes

[Stephen Boyd]

I suggested this some time ago but nobody has gotten around to doing it
so far. Well now I have! Here's a patch series that cuts down the
boiler-plate code that Mediatek clk drivers have to multiplex probe
amongst the device match data.

Rob/Frank, I'd prefer to take the first patch via clk tree so I can
merge it all in the next release. Otherwise, I can pull a stable commit
and merge it that way in case you want to take it through your tree.

Cc: Ryder Lee <ryder.lee@mediatek.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>

Stephen Boyd (4):
  of/device: Add a way to probe drivers by match data
  clk: mediatek: Convert to platform_driver_probe_by_match_data()
  clk: mediatek: Drop THIS_MODULE from platform_driver
  clk: mediatek: Simplify single driver probes

 drivers/clk/mediatek/clk-mt2701-g3d.c | 28 +++---------------------
 drivers/clk/mediatek/clk-mt2701.c     | 20 +----------------
 drivers/clk/mediatek/clk-mt2712.c     | 21 +-----------------
 drivers/clk/mediatek/clk-mt6797.c     | 20 +----------------
 drivers/clk/mediatek/clk-mt7622-aud.c | 28 +++---------------------
 drivers/clk/mediatek/clk-mt7622-eth.c | 20 +----------------
 drivers/clk/mediatek/clk-mt7622-hif.c | 20 +----------------
 drivers/clk/mediatek/clk-mt7622.c     | 20 +----------------
 drivers/of/device.c                   | 31 +++++++++++++++++++++++++++
 include/linux/of_device.h             |  1 +
 10 files changed, 44 insertions(+), 165 deletions(-)

-- 
Sent by a computer through tubes

[PATCH 1/4] of/device: Add a way to probe drivers by match data

[Stephen Boyd]

We have a handful of clk drivers that have a collection of slightly
variant device support keyed off of the compatible string. In each of
these drivers, we demux the variant and then call the "real" probe
function based on whatever is stored in the match data for that
compatible string. Let's generalize this function so that it can be
re-used as the platform_driver probe function directly.

Cc: Ryder Lee <ryder.lee@mediatek.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/of/device.c       | 31 +++++++++++++++++++++++++++++++
 include/linux/of_device.h |  1 +
 2 files changed, 32 insertions(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 0f27fad9fe94..8381f33ed2d8 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -195,6 +195,37 @@ const void *of_device_get_match_data(const struct device *dev)
 }
 EXPORT_SYMBOL(of_device_get_match_data);
 
+/**
+ * platform_driver_probe_by_of_match_data - Probe a platform device using match data
+ * @pdev: platform device to probe
+ *
+ * For use by device drivers that multiplex their probe function through DT
+ * match data. Drivers can use this function to call their platform
+ * device probe directly without having to implement a wrapper function.
+ *
+ * static const struct of_device_id probe_funcs[] = {
+ *      { .compatible = "compat,foo", .data = foo_probe },
+ *      {}
+ * };
+ *
+ * struct platform_driver foo_driver = {
+ *      .probe = platform_driver_probe_by_of_match_data,
+ *      .driver = {
+ *            of_match_table = probe_funcs,
+ *      },
+ * };
+ *
+ */
+int platform_driver_probe_by_of_match_data(struct platform_device *pdev)
+{
+	int (*probe_func)(struct platform_device *pdev);
+
+	probe_func = of_device_get_match_data(&pdev->dev);
+
+	return probe_func(pdev);
+}
+EXPORT_SYMBOL_GPL(platform_driver_probe_by_of_match_data);
+
 static ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len)
 {
 	const char *compat;
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 8d31e39dd564..4de84691d1c6 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -33,6 +33,7 @@ extern int of_device_add(struct platform_device *pdev);
 extern int of_device_register(struct platform_device *ofdev);
 extern void of_device_unregister(struct platform_device *ofdev);
 
+extern int platform_driver_probe_by_of_match_data(struct platform_device *pdev);
 extern const void *of_device_get_match_data(const struct device *dev);
 
 extern ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len);
-- 
Sent by a computer through tubes

[PATCH 2/4] clk: mediatek: Convert to platform_driver_probe_by_match_data()

[Stephen Boyd]

Use this function to reduce the boiler-plate code that the mediatek clk
driver needs to implement to probe clks for different devices within the
same driver.

Cc: Ryder Lee <ryder.lee@mediatek.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/mediatek/clk-mt2701.c     | 20 +-------------------
 drivers/clk/mediatek/clk-mt2712.c     | 20 +-------------------
 drivers/clk/mediatek/clk-mt6797.c     | 20 +-------------------
 drivers/clk/mediatek/clk-mt7622-eth.c | 20 +-------------------
 drivers/clk/mediatek/clk-mt7622-hif.c | 20 +-------------------
 drivers/clk/mediatek/clk-mt7622.c     | 20 +-------------------
 6 files changed, 6 insertions(+), 114 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt2701.c b/drivers/clk/mediatek/clk-mt2701.c
index ab6ab07f53e6..5dd8fb98ce49 100644
--- a/drivers/clk/mediatek/clk-mt2701.c
+++ b/drivers/clk/mediatek/clk-mt2701.c
@@ -1009,26 +1009,8 @@ static const struct of_device_id of_match_clk_mt2701[] = {
 	}
 };
 
-static int clk_mt2701_probe(struct platform_device *pdev)
-{
-	int (*clk_init)(struct platform_device *);
-	int r;
-
-	clk_init = of_device_get_match_data(&pdev->dev);
-	if (!clk_init)
-		return -EINVAL;
-
-	r = clk_init(pdev);
-	if (r)
-		dev_err(&pdev->dev,
-			"could not register clock provider: %s: %d\n",
-			pdev->name, r);
-
-	return r;
-}
-
 static struct platform_driver clk_mt2701_drv = {
-	.probe = clk_mt2701_probe,
+	.probe = platform_driver_probe_by_of_match_data,
 	.driver = {
 		.name = "clk-mt2701",
 		.of_match_table = of_match_clk_mt2701,
diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
index 991d4093726e..17c031d110ff 100644
--- a/drivers/clk/mediatek/clk-mt2712.c
+++ b/drivers/clk/mediatek/clk-mt2712.c
@@ -1441,26 +1441,8 @@ static const struct of_device_id of_match_clk_mt2712[] = {
 	}
 };
 
-static int clk_mt2712_probe(struct platform_device *pdev)
-{
-	int (*clk_probe)(struct platform_device *);
-	int r;
-
-	clk_probe = of_device_get_match_data(&pdev->dev);
-	if (!clk_probe)
-		return -EINVAL;
-
-	r = clk_probe(pdev);
-	if (r != 0)
-		dev_err(&pdev->dev,
-			"could not register clock provider: %s: %d\n",
-			pdev->name, r);
-
-	return r;
-}
-
 static struct platform_driver clk_mt2712_drv = {
-	.probe = clk_mt2712_probe,
+	.probe = platform_driver_probe_by_of_match_data,
 	.driver = {
 		.name = "clk-mt2712",
 		.owner = THIS_MODULE,
diff --git a/drivers/clk/mediatek/clk-mt6797.c b/drivers/clk/mediatek/clk-mt6797.c
index 5702bc974ed9..2d7ff3098cc9 100644
--- a/drivers/clk/mediatek/clk-mt6797.c
+++ b/drivers/clk/mediatek/clk-mt6797.c
@@ -680,26 +680,8 @@ static const struct of_device_id of_match_clk_mt6797[] = {
 	}
 };
 
-static int clk_mt6797_probe(struct platform_device *pdev)
-{
-	int (*clk_init)(struct platform_device *);
-	int r;
-
-	clk_init = of_device_get_match_data(&pdev->dev);
-	if (!clk_init)
-		return -EINVAL;
-
-	r = clk_init(pdev);
-	if (r)
-		dev_err(&pdev->dev,
-			"could not register clock provider: %s: %d\n",
-			pdev->name, r);
-
-	return r;
-}
-
 static struct platform_driver clk_mt6797_drv = {
-	.probe = clk_mt6797_probe,
+	.probe = platform_driver_probe_by_of_match_data,
 	.driver = {
 		.name = "clk-mt6797",
 		.of_match_table = of_match_clk_mt6797,
diff --git a/drivers/clk/mediatek/clk-mt7622-eth.c b/drivers/clk/mediatek/clk-mt7622-eth.c
index 6328127bbb3c..2ce825f19275 100644
--- a/drivers/clk/mediatek/clk-mt7622-eth.c
+++ b/drivers/clk/mediatek/clk-mt7622-eth.c
@@ -127,26 +127,8 @@ static const struct of_device_id of_match_clk_mt7622_eth[] = {
 	}
 };
 
-static int clk_mt7622_eth_probe(struct platform_device *pdev)
-{
-	int (*clk_init)(struct platform_device *);
-	int r;
-
-	clk_init = of_device_get_match_data(&pdev->dev);
-	if (!clk_init)
-		return -EINVAL;
-
-	r = clk_init(pdev);
-	if (r)
-		dev_err(&pdev->dev,
-			"could not register clock provider: %s: %d\n",
-			pdev->name, r);
-
-	return r;
-}
-
 static struct platform_driver clk_mt7622_eth_drv = {
-	.probe = clk_mt7622_eth_probe,
+	.probe = platform_driver_probe_by_of_match_data,
 	.driver = {
 		.name = "clk-mt7622-eth",
 		.of_match_table = of_match_clk_mt7622_eth,
diff --git a/drivers/clk/mediatek/clk-mt7622-hif.c b/drivers/clk/mediatek/clk-mt7622-hif.c
index a6e8534276c6..65f13808155d 100644
--- a/drivers/clk/mediatek/clk-mt7622-hif.c
+++ b/drivers/clk/mediatek/clk-mt7622-hif.c
@@ -140,26 +140,8 @@ static const struct of_device_id of_match_clk_mt7622_hif[] = {
 	}
 };
 
-static int clk_mt7622_hif_probe(struct platform_device *pdev)
-{
-	int (*clk_init)(struct platform_device *);
-	int r;
-
-	clk_init = of_device_get_match_data(&pdev->dev);
-	if (!clk_init)
-		return -EINVAL;
-
-	r = clk_init(pdev);
-	if (r)
-		dev_err(&pdev->dev,
-			"could not register clock provider: %s: %d\n",
-			pdev->name, r);
-
-	return r;
-}
-
 static struct platform_driver clk_mt7622_hif_drv = {
-	.probe = clk_mt7622_hif_probe,
+	.probe = platform_driver_probe_by_of_match_data,
 	.driver = {
 		.name = "clk-mt7622-hif",
 		.of_match_table = of_match_clk_mt7622_hif,
diff --git a/drivers/clk/mediatek/clk-mt7622.c b/drivers/clk/mediatek/clk-mt7622.c
index 92f7e32770c6..d7310fb5964f 100644
--- a/drivers/clk/mediatek/clk-mt7622.c
+++ b/drivers/clk/mediatek/clk-mt7622.c
@@ -746,26 +746,8 @@ static const struct of_device_id of_match_clk_mt7622[] = {
 	}
 };
 
-static int clk_mt7622_probe(struct platform_device *pdev)
-{
-	int (*clk_init)(struct platform_device *);
-	int r;
-
-	clk_init = of_device_get_match_data(&pdev->dev);
-	if (!clk_init)
-		return -EINVAL;
-
-	r = clk_init(pdev);
-	if (r)
-		dev_err(&pdev->dev,
-			"could not register clock provider: %s: %d\n",
-			pdev->name, r);
-
-	return r;
-}
-
 static struct platform_driver clk_mt7622_drv = {
-	.probe = clk_mt7622_probe,
+	.probe = platform_driver_probe_by_of_match_data,
 	.driver = {
 		.name = "clk-mt7622",
 		.of_match_table = of_match_clk_mt7622,
-- 
Sent by a computer through tubes

[PATCH 3/4] clk: mediatek: Drop THIS_MODULE from platform_driver

[Stephen Boyd]

This is already set by platform_driver_register(), so we can remove it
here.

Cc: Ryder Lee <ryder.lee@mediatek.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/mediatek/clk-mt2712.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
index 17c031d110ff..55f81f9bfdb5 100644
--- a/drivers/clk/mediatek/clk-mt2712.c
+++ b/drivers/clk/mediatek/clk-mt2712.c
@@ -1445,7 +1445,6 @@ static struct platform_driver clk_mt2712_drv = {
 	.probe = platform_driver_probe_by_of_match_data,
 	.driver = {
 		.name = "clk-mt2712",
-		.owner = THIS_MODULE,
 		.of_match_table = of_match_clk_mt2712,
 	},
 };
-- 
Sent by a computer through tubes

[PATCH 4/4] clk: mediatek: Simplify single driver probes

[Stephen Boyd]

We don't need to do the multiplex probe design here when we only have
one compatible string. Just setup probe to point at the one probe
function for now.

Cc: Ryder Lee <ryder.lee@mediatek.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/mediatek/clk-mt2701-g3d.c | 28 +++------------------------
 drivers/clk/mediatek/clk-mt7622-aud.c | 28 +++------------------------
 2 files changed, 6 insertions(+), 50 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt2701-g3d.c b/drivers/clk/mediatek/clk-mt2701-g3d.c
index 1328c112a38f..58b13f12051a 100644
--- a/drivers/clk/mediatek/clk-mt2701-g3d.c
+++ b/drivers/clk/mediatek/clk-mt2701-g3d.c
@@ -58,34 +58,12 @@ static int clk_mt2701_g3dsys_init(struct platform_device *pdev)
 }
 
 static const struct of_device_id of_match_clk_mt2701_g3d[] = {
-	{
-		.compatible = "mediatek,mt2701-g3dsys",
-		.data = clk_mt2701_g3dsys_init,
-	}, {
-		/* sentinel */
-	}
+	{ .compatible = "mediatek,mt2701-g3dsys", },
+	{ /* sentinel */ }
 };
 
-static int clk_mt2701_g3d_probe(struct platform_device *pdev)
-{
-	int (*clk_init)(struct platform_device *);
-	int r;
-
-	clk_init = of_device_get_match_data(&pdev->dev);
-	if (!clk_init)
-		return -EINVAL;
-
-	r = clk_init(pdev);
-	if (r)
-		dev_err(&pdev->dev,
-			"could not register clock provider: %s: %d\n",
-			pdev->name, r);
-
-	return r;
-}
-
 static struct platform_driver clk_mt2701_g3d_drv = {
-	.probe = clk_mt2701_g3d_probe,
+	.probe = clk_mt2701_g3dsys_init,
 	.driver = {
 		.name = "clk-mt2701-g3d",
 		.of_match_table = of_match_clk_mt2701_g3d,
diff --git a/drivers/clk/mediatek/clk-mt7622-aud.c b/drivers/clk/mediatek/clk-mt7622-aud.c
index 4f3d47b41b3e..2b10d13a8a37 100644
--- a/drivers/clk/mediatek/clk-mt7622-aud.c
+++ b/drivers/clk/mediatek/clk-mt7622-aud.c
@@ -171,34 +171,12 @@ static int clk_mt7622_audiosys_init(struct platform_device *pdev)
 }
 
 static const struct of_device_id of_match_clk_mt7622_aud[] = {
-	{
-		.compatible = "mediatek,mt7622-audsys",
-		.data = clk_mt7622_audiosys_init,
-	}, {
-		/* sentinel */
-	}
+	{ .compatible = "mediatek,mt7622-audsys", },
+	{ /* sentinel */ }
 };
 
-static int clk_mt7622_aud_probe(struct platform_device *pdev)
-{
-	int (*clk_init)(struct platform_device *);
-	int r;
-
-	clk_init = of_device_get_match_data(&pdev->dev);
-	if (!clk_init)
-		return -EINVAL;
-
-	r = clk_init(pdev);
-	if (r)
-		dev_err(&pdev->dev,
-			"could not register clock provider: %s: %d\n",
-			pdev->name, r);
-
-	return r;
-}
-
 static struct platform_driver clk_mt7622_aud_drv = {
-	.probe = clk_mt7622_aud_probe,
+	.probe = clk_mt7622_audiosys_init,
 	.driver = {
 		.name = "clk-mt7622-aud",
 		.of_match_table = of_match_clk_mt7622_aud,
-- 
Sent by a computer through tubes

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Rob Herring]

On Tue, Nov 6, 2018 at 12:36 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> We have a handful of clk drivers that have a collection of slightly
> variant device support keyed off of the compatible string. In each of
> these drivers, we demux the variant and then call the "real" probe
> function based on whatever is stored in the match data for that
> compatible string. Let's generalize this function so that it can be
> re-used as the platform_driver probe function directly.

This looks really hacky to me. It sounds kind of general, but really
only works if we have match data that's a single function and we lose
any type checking on the function. What about things other than
platform devices?

Rob

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Stephen Boyd]

Quoting Rob Herring (2018-11-06 12:44:52)
> On Tue, Nov 6, 2018 at 12:36 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > We have a handful of clk drivers that have a collection of slightly
> > variant device support keyed off of the compatible string. In each of
> > these drivers, we demux the variant and then call the "real" probe
> > function based on whatever is stored in the match data for that
> > compatible string. Let's generalize this function so that it can be
> > re-used as the platform_driver probe function directly.
> 
> This looks really hacky to me. It sounds kind of general, but really
> only works if we have match data that's a single function and we lose
> any type checking on the function.

I don't know what this means. Are you saying that we lose the ability to
type check the function pointer stored in the data member? I don't have
a good answer for this besides it's not any worse than the status quo
for the mediatek drivers.

One alternative is to add a DT only array to the platform_driver struct
that has the platform driver probe function type and matches the index
in the of_device_id table. Then we can add some logic into
platform_drv_probe() to look for this table being set and find the probe
function to call. Then we still have match data for anything that wants
that (maybe it could be passed in to the probe function) at the cost of
having another array. I don't have a use-case for this right now so I'm
not sure this is a great idea.

  struct of_platform_driver_probe_func {
  	int (*probe)(struct platform_device *pdev);
  };

  struct of_platform_driver_probe_func mtk_probes[] = {
  	mtk_probe1,
	mtk_probe2,
	mtk_probe3,
  };

  struct platform_driver mtk_driver = {
  	.of_probes = &mtk_probes;
  	.driver = {
		.name = "mtk-foo";
		.of_match_table = mtk_match_table,
	},
  };

> What about things other than
> platform devices?
> 

I suppose those would need similar functions that take the right struct
type and match the driver probe function signature. To make the above
more generic we could try to figure out a way to put the 'of_probes'
array inside struct device_driver. That would require dropping
platform_device specifically from the probe functions and having those
take a plain 'struct device' that those probe functions turn into the
appropriate structure with to_platform_device() or to_i2c_client()?

So the example would become

  struct of_driver_probe_func {
  	int (*probe)(struct device *dev);
  };

  struct of_driver_probe_func mtk_probes[] = {
  	mtk_probe1,
	mtk_probe2,
	mtk_probe3,
  };

  struct platform_driver mtk_driver = {
  	.driver = {
		.name = "mtk-foo";
		.of_match_table = mtk_match_table,
		.of_probes = &mtk_probes;
	},
  };

And the probe functions might need to container_of() the device pointer
to get the struct they know they need. The probe function could also be
added to of_device_id and then we would have to look and see if that
pointer is populated when the device is matched in generic device code.

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Matthias Brugger]

On 06/11/2018 19:36, Stephen Boyd wrote:
> We have a handful of clk drivers that have a collection of slightly
> variant device support keyed off of the compatible string. In each of
> these drivers, we demux the variant and then call the "real" probe
> function based on whatever is stored in the match data for that
> compatible string. Let's generalize this function so that it can be
> re-used as the platform_driver probe function directly.
> 
> Cc: Ryder Lee <ryder.lee@mediatek.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  drivers/of/device.c       | 31 +++++++++++++++++++++++++++++++
>  include/linux/of_device.h |  1 +
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 0f27fad9fe94..8381f33ed2d8 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -195,6 +195,37 @@ const void *of_device_get_match_data(const struct device *dev)
>  }
>  EXPORT_SYMBOL(of_device_get_match_data);
>  
> +/**
> + * platform_driver_probe_by_of_match_data - Probe a platform device using match data
> + * @pdev: platform device to probe
> + *
> + * For use by device drivers that multiplex their probe function through DT
> + * match data. Drivers can use this function to call their platform
> + * device probe directly without having to implement a wrapper function.
> + *
> + * static const struct of_device_id probe_funcs[] = {
> + *      { .compatible = "compat,foo", .data = foo_probe },
> + *      {}
> + * };
> + *
> + * struct platform_driver foo_driver = {
> + *      .probe = platform_driver_probe_by_of_match_data,
> + *      .driver = {
> + *            of_match_table = probe_funcs,
> + *      },
> + * };
> + *
> + */
> +int platform_driver_probe_by_of_match_data(struct platform_device *pdev)
> +{
> +	int (*probe_func)(struct platform_device *pdev);
> +
> +	probe_func = of_device_get_match_data(&pdev->dev);

Shouldn't we check if probe_func is not NULL?

> +
> +	return probe_func(pdev);
> +}
> +EXPORT_SYMBOL_GPL(platform_driver_probe_by_of_match_data);
> +
>  static ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len)
>  {
>  	const char *compat;
> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> index 8d31e39dd564..4de84691d1c6 100644
> --- a/include/linux/of_device.h
> +++ b/include/linux/of_device.h
> @@ -33,6 +33,7 @@ extern int of_device_add(struct platform_device *pdev);
>  extern int of_device_register(struct platform_device *ofdev);
>  extern void of_device_unregister(struct platform_device *ofdev);
>  
> +extern int platform_driver_probe_by_of_match_data(struct platform_device *pdev);
>  extern const void *of_device_get_match_data(const struct device *dev);
>  
>  extern ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len);
> 

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Stephen Boyd]

Quoting Matthias Brugger (2018-11-08 00:29:46)
> On 06/11/2018 19:36, Stephen Boyd wrote:
> > +int platform_driver_probe_by_of_match_data(struct platform_device *pdev)
> > +{
> > +     int (*probe_func)(struct platform_device *pdev);
> > +
> > +     probe_func = of_device_get_match_data(&pdev->dev);
> 
> Shouldn't we check if probe_func is not NULL?

Is the oops from the NULL pointer deref insufficient?

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Geert Uytterhoeven]

Hi Stephen,

On Wed, Nov 7, 2018 at 7:37 PM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Rob Herring (2018-11-06 12:44:52)
> > On Tue, Nov 6, 2018 at 12:36 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > We have a handful of clk drivers that have a collection of slightly
> > > variant device support keyed off of the compatible string. In each of
> > > these drivers, we demux the variant and then call the "real" probe
> > > function based on whatever is stored in the match data for that
> > > compatible string. Let's generalize this function so that it can be
> > > re-used as the platform_driver probe function directly.
> >
> > This looks really hacky to me. It sounds kind of general, but really
> > only works if we have match data that's a single function and we lose
> > any type checking on the function.
>
> I don't know what this means. Are you saying that we lose the ability to
> type check the function pointer stored in the data member? I don't have
> a good answer for this besides it's not any worse than the status quo
> for the mediatek drivers.

The .data field has always been void *, and is used for storing different
things, depending on the driver.
It's already up to the driver writer to get that right.

> One alternative is to add a DT only array to the platform_driver struct
> that has the platform driver probe function type and matches the index
> in the of_device_id table. Then we can add some logic into
> platform_drv_probe() to look for this table being set and find the probe
> function to call. Then we still have match data for anything that wants
> that (maybe it could be passed in to the probe function) at the cost of
> having another array. I don't have a use-case for this right now so I'm
> not sure this is a great idea.
>
>   struct of_platform_driver_probe_func {
>         int (*probe)(struct platform_device *pdev);
>   };
>
>   struct of_platform_driver_probe_func mtk_probes[] = {
>         mtk_probe1,
>         mtk_probe2,
>         mtk_probe3,
>   };
>
>   struct platform_driver mtk_driver = {
>         .of_probes = &mtk_probes;
>         .driver = {
>                 .name = "mtk-foo";
>                 .of_match_table = mtk_match_table,
>         },
>   };

This looks worse to me: people tend to be very good at keeping multiple
arrays in sync :-(

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Matthias Brugger]

On 08/11/2018 18:58, Stephen Boyd wrote:
> Quoting Matthias Brugger (2018-11-08 00:29:46)
>> On 06/11/2018 19:36, Stephen Boyd wrote:
>>> +int platform_driver_probe_by_of_match_data(struct platform_device *pdev)
>>> +{
>>> +     int (*probe_func)(struct platform_device *pdev);
>>> +
>>> +     probe_func = of_device_get_match_data(&pdev->dev);
>>
>> Shouldn't we check if probe_func is not NULL?
> 
> Is the oops from the NULL pointer deref insufficient?
> 

Do you think we should crash the machine if someone uses the call wrongly? Or
should we provide the possibility to error out on the caller side?

Regards,
Matthias

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Geert Uytterhoeven]

Hi Matthias,

On Fri, Nov 9, 2018 at 11:29 AM Matthias Brugger <matthias.bgg@gmail.com> wrote:
> On 08/11/2018 18:58, Stephen Boyd wrote:
> > Quoting Matthias Brugger (2018-11-08 00:29:46)
> >> On 06/11/2018 19:36, Stephen Boyd wrote:
> >>> +int platform_driver_probe_by_of_match_data(struct platform_device *pdev)
> >>> +{
> >>> +     int (*probe_func)(struct platform_device *pdev);
> >>> +
> >>> +     probe_func = of_device_get_match_data(&pdev->dev);
> >>
> >> Shouldn't we check if probe_func is not NULL?
> >
> > Is the oops from the NULL pointer deref insufficient?
>
> Do you think we should crash the machine if someone uses the call wrongly? Or
> should we provide the possibility to error out on the caller side?

I believe that would be a bug in the driver, to be discovered ASAP.
So yes, please do crash ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Rob Herring]

On Fri, Nov 9, 2018 at 4:36 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Matthias,
>
> On Fri, Nov 9, 2018 at 11:29 AM Matthias Brugger <matthias.bgg@gmail.com> wrote:
> > On 08/11/2018 18:58, Stephen Boyd wrote:
> > > Quoting Matthias Brugger (2018-11-08 00:29:46)
> > >> On 06/11/2018 19:36, Stephen Boyd wrote:
> > >>> +int platform_driver_probe_by_of_match_data(struct platform_device *pdev)
> > >>> +{
> > >>> +     int (*probe_func)(struct platform_device *pdev);
> > >>> +
> > >>> +     probe_func = of_device_get_match_data(&pdev->dev);
> > >>
> > >> Shouldn't we check if probe_func is not NULL?
> > >
> > > Is the oops from the NULL pointer deref insufficient?
> >
> > Do you think we should crash the machine if someone uses the call wrongly? Or
> > should we provide the possibility to error out on the caller side?
>
> I believe that would be a bug in the driver, to be discovered ASAP.
> So yes, please do crash ;-)

Depends on the driver. If it's not needed to get a console out, then
it should just WARN. Otherwise, you've got to go enable earlycon to
see the crash. Of course, someone could just as easily stick data in
here instead of a function ptr and we'll be back to a crash.

Rob

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Frank Rowand]

On 11/9/18 2:36 AM, Geert Uytterhoeven wrote:
> Hi Matthias,
> 
> On Fri, Nov 9, 2018 at 11:29 AM Matthias Brugger <matthias.bgg@gmail.com> wrote:
>> On 08/11/2018 18:58, Stephen Boyd wrote:
>>> Quoting Matthias Brugger (2018-11-08 00:29:46)
>>>> On 06/11/2018 19:36, Stephen Boyd wrote:
>>>>> +int platform_driver_probe_by_of_match_data(struct platform_device *pdev)
>>>>> +{
>>>>> +     int (*probe_func)(struct platform_device *pdev);
>>>>> +
>>>>> +     probe_func = of_device_get_match_data(&pdev->dev);
>>>>
>>>> Shouldn't we check if probe_func is not NULL?
>>>
>>> Is the oops from the NULL pointer deref insufficient?
>>
>> Do you think we should crash the machine if someone uses the call wrongly? Or
>> should we provide the possibility to error out on the caller side?
> 
> I believe that would be a bug in the driver, to be discovered ASAP.
> So yes, please do crash ;-)

This is one of Linus' pet peeves.  He does not think crashing the
machine is the proper choice (as a general statement).

-Frank

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Stephen Boyd]

Quoting Geert Uytterhoeven (2018-11-09 01:56:01)
> On Wed, Nov 7, 2018 at 7:37 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > Quoting Rob Herring (2018-11-06 12:44:52)
> > > On Tue, Nov 6, 2018 at 12:36 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >         int (*probe)(struct platform_device *pdev);
> >   };
> >
> >   struct of_platform_driver_probe_func mtk_probes[] = {
> >         mtk_probe1,
> >         mtk_probe2,
> >         mtk_probe3,
> >   };
> >
> >   struct platform_driver mtk_driver = {
> >         .of_probes = &mtk_probes;
> >         .driver = {
> >                 .name = "mtk-foo";
> >                 .of_match_table = mtk_match_table,
> >         },
> >   };
> 
> This looks worse to me: people tend to be very good at keeping multiple
> arrays in sync :-(

To be _not_ very good? Agreed, and so specifying the probe function as
another member in struct of_device_id seems to be the way to go.

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Geert Uytterhoeven]

Hi Stephen,

On Fri, Nov 9, 2018 at 5:59 PM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Geert Uytterhoeven (2018-11-09 01:56:01)
> > On Wed, Nov 7, 2018 at 7:37 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > Quoting Rob Herring (2018-11-06 12:44:52)
> > > > On Tue, Nov 6, 2018 at 12:36 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >         int (*probe)(struct platform_device *pdev);
> > >   };
> > >
> > >   struct of_platform_driver_probe_func mtk_probes[] = {
> > >         mtk_probe1,
> > >         mtk_probe2,
> > >         mtk_probe3,
> > >   };
> > >
> > >   struct platform_driver mtk_driver = {
> > >         .of_probes = &mtk_probes;
> > >         .driver = {
> > >                 .name = "mtk-foo";
> > >                 .of_match_table = mtk_match_table,
> > >         },
> > >   };
> >
> > This looks worse to me: people tend to be very good at keeping multiple
> > arrays in sync :-(
>
> To be _not_ very good? Agreed, and so specifying the probe function as
> another member in struct of_device_id seems to be the way to go.

Correct, sometimes sarcasm doesn't arrive well at the other end of the
electronic tunnel...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Stephen Boyd]

Quoting Stephen Boyd (2018-11-07 10:37:31)
> appropriate structure with to_platform_device() or to_i2c_client()?
> 
> So the example would become
> 
>   struct of_driver_probe_func {
>         int (*probe)(struct device *dev);
>   };
> 
>   struct of_driver_probe_func mtk_probes[] = {
>         mtk_probe1,
>         mtk_probe2,
>         mtk_probe3,
>   };
> 
>   struct platform_driver mtk_driver = {
>         .driver = {
>                 .name = "mtk-foo";
>                 .of_match_table = mtk_match_table,
>                 .of_probes = &mtk_probes;
>         },
>   };
> 
> And the probe functions might need to container_of() the device pointer
> to get the struct they know they need. The probe function could also be
> added to of_device_id and then we would have to look and see if that
> pointer is populated when the device is matched in generic device code.
> 

I guess I'll go down the path of extending the of_device_id structure?

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Rob Herring]

On Thu, Nov 29, 2018 at 6:28 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Stephen Boyd (2018-11-07 10:37:31)
> > appropriate structure with to_platform_device() or to_i2c_client()?
> >
> > So the example would become
> >
> >   struct of_driver_probe_func {
> >         int (*probe)(struct device *dev);
> >   };
> >
> >   struct of_driver_probe_func mtk_probes[] = {
> >         mtk_probe1,
> >         mtk_probe2,
> >         mtk_probe3,
> >   };
> >
> >   struct platform_driver mtk_driver = {
> >         .driver = {
> >                 .name = "mtk-foo";
> >                 .of_match_table = mtk_match_table,
> >                 .of_probes = &mtk_probes;
> >         },
> >   };
> >
> > And the probe functions might need to container_of() the device pointer
> > to get the struct they know they need. The probe function could also be
> > added to of_device_id and then we would have to look and see if that
> > pointer is populated when the device is matched in generic device code.
> >
>
> I guess I'll go down the path of extending the of_device_id structure?

Unfortunately, I don't think you can change of_device_id as it's part
of the kernel ABI.

Rob

Re: [PATCH 1/4] of/device: Add a way to probe drivers by match data

[Stephen Boyd]

Quoting Rob Herring (2018-11-29 17:01:54)
> On Thu, Nov 29, 2018 at 6:28 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Stephen Boyd (2018-11-07 10:37:31)
> > > appropriate structure with to_platform_device() or to_i2c_client()?
> > >
> > > So the example would become
> > >
> > >   struct of_driver_probe_func {
> > >         int (*probe)(struct device *dev);
> > >   };
> > >
> > >   struct of_driver_probe_func mtk_probes[] = {
> > >         mtk_probe1,
> > >         mtk_probe2,
> > >         mtk_probe3,
> > >   };
> > >
> > >   struct platform_driver mtk_driver = {
> > >         .driver = {
> > >                 .name = "mtk-foo";
> > >                 .of_match_table = mtk_match_table,
> > >                 .of_probes = &mtk_probes;
> > >         },
> > >   };
> > >
> > > And the probe functions might need to container_of() the device pointer
> > > to get the struct they know they need. The probe function could also be
> > > added to of_device_id and then we would have to look and see if that
> > > pointer is populated when the device is matched in generic device code.
> > >
> >
> > I guess I'll go down the path of extending the of_device_id structure?
> 
> Unfortunately, I don't think you can change of_device_id as it's part
> of the kernel ABI.

Ok. Then I'll go down the path of making it a parallel array?