Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751078AbeABXGh (ORCPT + 1 other); Tue, 2 Jan 2018 18:06:37 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:56570 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750917AbeABXGf (ORCPT ); Tue, 2 Jan 2018 18:06:35 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 12FE660285 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=sboyd@codeaurora.org Date: Tue, 2 Jan 2018 15:06:33 -0800 From: Stephen Boyd To: Geert Uytterhoeven Cc: Geert Uytterhoeven , Michael Turquette , linux-clk , Linux Kernel Mailing List Subject: Re: [PATCH] clk: Fix debugfs_create_*() usage Message-ID: <20180102230633.GM7997@codeaurora.org> References: <1514906853-9364-1-git-send-email-geert+renesas@glider.be> <20180102192320.GJ7997@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On 01/02, Geert Uytterhoeven wrote: > On Tue, Jan 2, 2018 at 8:23 PM, Stephen Boyd wrote: > > On 01/02, Geert Uytterhoeven wrote: > >> --- a/drivers/clk/clk.c > >> +++ b/drivers/clk/clk.c > >> @@ -58,7 +58,7 @@ struct clk_core { > >> unsigned long new_rate; > >> struct clk_core *new_parent; > >> struct clk_core *new_child; > >> - unsigned long flags; > >> + unsigned int flags; > > > > This doesn't look good. > > Why not? > It's not like flags is used with bitops, which would mandate unsigned long. > And you can't start using bits 32-63 without changing flags to u64, else > the extra bits are not available on 32-bit platforms. Fair enough. We don't need to change it if we print better information in debugfs though. That's all I'm saying. > > >> @@ -2600,43 +2600,43 @@ static int clk_debug_create_one(struct clk_core *core, struct dentry *pdentry) > >> > >> core->dentry = d; > >> > >> - d = debugfs_create_u32("clk_rate", S_IRUGO, core->dentry, > >> - (u32 *)&core->rate); > >> + d = debugfs_create_ulong("clk_rate", S_IRUGO, core->dentry, > >> + &core->rate); > > > > As you're changing these lines, can you also change S_IRUGO to > > the octal values. That's the preferred style now. > > Yes, I can. That would be a separate patch, though. Uhhh ok. I will fold them together if you don't :) > > >> d = debugfs_create_x32("clk_flags", S_IRUGO, core->dentry, > >> - (u32 *)&core->flags); > >> + &core->flags); > > > > Maybe we need a new debugfs API like debugfs_create_ulong_hex() > > or something that prints out an unsigned long as a hex value? > > That's possible. I already have that locally (for another user which uses > u32 or u64 depending on platform). > My main worry was the change from 0xXXXXXXXX to 0xXXXXXXXXXXXXXXXX > on 64-bit platforms, which you don't seem to see as a blocker, as > debugfs isn't ABI? That's right. Debugfs isn't an ABI. > > > clk_flags file would have something like > > > > CLK_SET_RATE_PARENT > > CLK_SET_RATE_GATE > > > > if those flags are set. > > But some flags are internal to platform-specific drivers, right? Nope. Platform specific drivers shouldn't be passing internal flags in this field. It's for the clk core to use. Perhaps we should enforce that by failing non-core flags on registration. I've been catching this in review so far. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project