2016-08-19 12:52 GMT+09:00 Stephen Warren swarren@wwwdotorg.org:
On 08/18/2016 07:49 PM, Wenyou.Yang@microchip.com wrote:
Add Simon and Andreas in loop
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:56 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@xilinx.com Subject: Re: [U-Boot] [PATCH v1] clk: clk-uclass: Check ops pointer before use it
On 08/17/2016 09:53 PM, Wenyou.Yang@microchip.com wrote:
Hi Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年8月18日 11:46 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com; wenyou.yang@atmel.com Cc: u-boot@lists.denx.de; swarren@nvidia.com; michal.simek@xilinx.com Subject: Re: [U-Boot] [PATCH v1] clk: clk-uclass: Check ops pointer before use it
On 08/17/2016 06:30 PM, Wenyou.Yang@microchip.com wrote:
HI Stephen,
> -----Original Message----- > From: Stephen Warren [mailto:swarren@wwwdotorg.org] > Sent: 2016年8月17日 23:59 > To: Wenyou Yang wenyou.yang@atmel.com > Cc: U-Boot Mailing List u-boot@lists.denx.de; Stephen Warren > swarren@nvidia.com; Michal Simek michal.simek@xilinx.com > Subject: Re: [U-Boot] [PATCH v1] clk: clk-uclass: Check ops pointer > before use it > > On 08/17/2016 01:05 AM, Wenyou Yang wrote: >> >> Add check ops pointer before use it. Otherwise, it will cause the >> runtime error for the clk devices without ops callback. > > > Other uclasses like reset, power domain, and mailbox don't do this. > All drivers must have an ops pointer, or they can't be useful. I'm > not sure this patch is necessary. Is it just a debugging aid so if > the driver writer forgets to set the ops pointer the system will > error out rather than crashing? If so, a post-bind hook in the > uclass that
refuses the driver if it hasn't set the ops pointer would be better.
Sorry, I don't agree with you.
Not all drivers have an ops pointer.
If you grep U_BOOT_DRIVER , you will find that there are some drivers without
an ops pointer.
We should not suppose a driver should have something, I think.
But without an ops pointer, the driver can do nothing and provide no services. Why is that useful?
There are some nodes without compatible in device tree, as a child of some node, for example, pinctrl child node or for my code peripheral clock node.
These nodes also need to be bound before using them. They require such driver
to bind them.
That seems unrelated. A node without a compatible value won't instantiate a device object, and hence needs no driver.
But there are such nodes and drivers (i.e., U_BOOT_DRIVER) existed in U-Boot, as I said before.
Is it harmful to add this check?
I know, if not, it will produce a wild pointer, such as NULL->of_xlate.
I see this in drivers/clk/at91/{pmc,sckc}.c. The drivers in those files exist solely to cause DT sub-nodes to be enumerated, and aren't clock providers themselves. I believe the fix is to change them from UCLASS_CLK to UCLASS_SOMETHING_ELSE, e.g. UCLASS_MISC. ops are already optional for that uclass.
Right. drivers/clk/at91/{pmc,sckc}.c are doing wrong.
I think UCLASS_SIMPLE_BUS is the best because you do not have to bind child nodes explicitly. simple_bus_post_bind() will do it for you.