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

Message ID 20181106183609.207702-2-sboyd@kernel.org
State New
Headers show
Series
  • Simplify mediatek clk driver probes
Related show

Commit Message

Stephen Boyd Nov. 6, 2018, 6:36 p.m. UTC
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(+)

Comments

Rob Herring Nov. 6, 2018, 8:44 p.m. UTC | #1
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
Stephen Boyd Nov. 7, 2018, 6:37 p.m. UTC | #2
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.
Matthias Brugger Nov. 8, 2018, 8:29 a.m. UTC | #3
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);
>
Stephen Boyd Nov. 8, 2018, 5:58 p.m. UTC | #4
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?
Geert Uytterhoeven Nov. 9, 2018, 9:56 a.m. UTC | #5
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
Matthias Brugger Nov. 9, 2018, 10:29 a.m. UTC | #6
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
Geert Uytterhoeven Nov. 9, 2018, 10:36 a.m. UTC | #7
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
Rob Herring Nov. 9, 2018, 4:30 p.m. UTC | #8
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
Frank Rowand Nov. 9, 2018, 4:56 p.m. UTC | #9
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
>
Stephen Boyd Nov. 9, 2018, 4:59 p.m. UTC | #10
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.
Geert Uytterhoeven Nov. 9, 2018, 7:18 p.m. UTC | #11
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
Stephen Boyd Nov. 30, 2018, 12:28 a.m. UTC | #12
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?
Rob Herring Nov. 30, 2018, 1:01 a.m. UTC | #13
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
Stephen Boyd Nov. 30, 2018, 7:03 a.m. UTC | #14
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?

Patch
diff mbox series

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