Re: [PATCH v3 3/4] pwm: meson: change clk/pwm gate from mask to bit

From: Heiner Kallweit <hkallweit1@gmail.com>
To: Thierry Reding <thierry.reding@gmail.com>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: "Jerome Brunet" <jbrunet@baylibre.com>,
	"Neil Armstrong" <narmstrong@baylibre.com>,
	"Kevin Hilman" <khilman@baylibre.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"open list:ARM/Amlogic Meson..."
	<linux-amlogic@lists.infradead.org>,
	linux-pwm@vger.kernel.org
Subject: Re: [PATCH v3 3/4] pwm: meson: change clk/pwm gate from mask to bit
Date: Thu, 13 Apr 2023 23:17:59 +0200	[thread overview]
Message-ID: <61c1266c-407f-6d26-d9aa-1f9a0fc02e12@gmail.com> (raw)
In-Reply-To: <ZDfGJM24xZbfSur5@orome>

On 13.04.2023 11:06, Thierry Reding wrote:
> On Wed, Apr 12, 2023 at 10:47:05PM +0200, Martin Blumenstingl wrote:
>> Hi Heiner,
>>
>> On Wed, Apr 12, 2023 at 9:23 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>
>>> Change single-bit values from mask to bit. This facilitates
>>> CCF initialization for the clock gate in a follow-up patch.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> #
>> meson8b-odroidc1, sm1-x96-air
>>
>> [...]
>>>  #define REG_MISC_AB            0x8
>>> -#define MISC_B_CLK_EN          BIT(23)
>>> -#define MISC_A_CLK_EN          BIT(15)
>>> +#define MISC_B_CLK_EN          23
>>> +#define MISC_A_CLK_EN          15
>>>  #define MISC_CLK_DIV_MASK      0x7f
>>>  #define MISC_B_CLK_DIV_SHIFT   16
>>>  #define MISC_A_CLK_DIV_SHIFT   8
>>>  #define MISC_B_CLK_SEL_SHIFT   6
>>>  #define MISC_A_CLK_SEL_SHIFT   4
>>>  #define MISC_CLK_SEL_MASK      0x3
>>> -#define MISC_B_EN              BIT(1)
>>> -#define MISC_A_EN              BIT(0)
>>> +#define MISC_B_EN              1
>>> +#define MISC_A_EN              0
>> Personally I'm fine with this change but it's not how I would have done it:
>> - I would have kept the BIT() macro for MISC_{A,B}_EN
>> - then I would have renamed MISC_{A,}_CLK_EN to
>> MISC_{A,B}_CLK_EN_SHIFT (to be consistent with _SHIFT of the mux and
>> divider) and drop the BIT() macro there (like you did)
>>
>> This is possibly/likely personal preference, so my suggestion is to
>> wait for some more feedback.
> 
> It looks like these aren't used outside the meson_pwm_per_channel_data
> array, so why bother with a #define (and any potential inconsistencies)
> in the first place?
> 
I think we follow a common pattern here and first define constants for all
registers and bits/fields. Having the register layout defined in one place
makes it easier to check it against the chip datasheet.
However I'm not sure whether this is what you were referring to.

> Thierry


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel