On Tue, Apr 25, 2017 at 05:31:37PM +0100, Russell King - ARM Linux wrote: > On Tue, Apr 25, 2017 at 02:30:07PM +0200, Thomas Bogendoerfer wrote: > > If CONFIG_HAVE_CLOCK is not set, return values of clk_get(), > > devm_clk_get(), devm_get_clk_from_child(), clk_get_parent() > > and clk_get_sys() are wrong. According to spec these functions > > should either return a pointer to a struct clk or a valid IS_ERR > > condition. NULL is neither, so returning ERR_PTR(-ENODEV) makes > > more sense. > > That's wrong. When the clk API is disabled, the expected behaviour is > that drivers will not fail. Returning ERR_PTR(-ENODEV) will cause them > to fail, so will break platforms. > > NAK. > then we have to go through all drivers, which could work with and without HAVE_CLK and replace the IS_ERR() checks with something like IS_ERR_OR_NULL(). Easy for the part I'm interested in the first place. > > Without this change serial console on SNI RM400 machines (MIPS arch) > > is broken, because sccnxp driver doesn't get a valid clock rate. > > So the driver needs to depeond on HAVE_CLOCK. the driver worked without HAVE_CLOCK before just fine, and while it got invaded by CLK API it got broken. No problem to fix that, but just looking at include/linux/clk.h: /** * devm_clk_get - lookup and obtain a managed reference to a clock producer. * @dev: device for clock "consumer" * @id: clock consumer ID * * Returns a struct clk corresponding to the clock producer, or * valid IS_ERR() condition containing errno. The implementation I would expect either no replacement for that, if !HAVE_CLOCK or simple a sane result code... and NULL isn't sane at least with the description above... But still it's your call. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ]
