[PATCH v2 0/4] pwm: meson: Support constant and polarity bits

[PATCH v2 0/4] pwm: meson: Support constant and polarity bits

[George Stark]

This patch series add support for amlogic's newer PWM IPs hardware features:
constant and polarity bits.

Using polarity bit for inverting output signal allows to identify inversion
in .get_state() callback which can only rely on data read from registers.

Using constant bit allows to have steady output level when duty cycle is zero or
equal to period. Without this bit there will always be single-clock spikes on output.

Those bits are supported in axg, g12 and newer SoC families like s4, a1 etc.
Tested on g12, a1.

Changes in v2:
  pwm: meson: Support constant and polarity bits
    - drop separate set_constant() and set_polarity() and move register writings
      into enable() and disable()
  pwm: meson: Simplify get_state() callback
    - add new patch to make .get_state() callback consistent. Since I add new
      fields to struct meson_pwm_channel either we should fill back all of them
      from registers or not at all.
  pwm: meson: Use separate device id data for axg and g12
    - add splitting amlogic,meson8-pwm-v2 into amlogic,meson-axg-pwm-v2 and
      amlogic,meson-g12-pwm-v2 with pwm_meson_axg_v2_data for both compatibles.
    - update commit message
    - add tag: Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
  pwm: meson: Enable constant and polarity features for g12, axg, s4
    - add enabling const and polarity to pwm_meson_axg_v2_data
    - add tag: Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
  link to v1: [1]

[1] https://lore.kernel.org/linux-arm-kernel/20241007193203.1753326-4-gnstark@salutedevices.com/T/

George Stark (4):
  pwm: meson: Simplify get_state() callback
  pwm: meson: Support constant and polarity bits
  pwm: meson: Use separate device id data for axg and g12
  pwm: meson: Enable constant and polarity features for g12, axg, s4

 drivers/pwm/pwm-meson.c | 103 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 92 insertions(+), 11 deletions(-)

--
2.25.1

[PATCH v2 1/4] pwm: meson: Simplify get_state() callback

[George Stark]

In .get_state() callback meson_pwm_channel struct are used to store
lo and hi reg values but they are never reused after that so
for clearness use local variable instead.

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/pwm/pwm-meson.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 98e6c1533312..2ef632caebcc 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -310,6 +310,7 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct meson_pwm *meson = to_meson_pwm(chip);
 	struct meson_pwm_channel_data *channel_data;
 	struct meson_pwm_channel *channel;
+	unsigned int hi, lo;
 	u32 value;
 
 	channel = &meson->channels[pwm->hwpwm];
@@ -319,11 +320,11 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	state->enabled = value & channel_data->pwm_en_mask;
 
 	value = readl(meson->base + channel_data->reg_offset);
-	channel->lo = FIELD_GET(PWM_LOW_MASK, value);
-	channel->hi = FIELD_GET(PWM_HIGH_MASK, value);
+	lo = FIELD_GET(PWM_LOW_MASK, value);
+	hi = FIELD_GET(PWM_HIGH_MASK, value);
 
-	state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi);
-	state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi);
+	state->period = meson_pwm_cnt_to_ns(chip, pwm, lo + hi);
+	state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, hi);
 
 	state->polarity = PWM_POLARITY_NORMAL;
 
-- 
2.25.1

[PATCH v2 2/4] pwm: meson: Support constant and polarity bits

[George Stark]

Newer meson PWM IPs support constant and polarity bits. Support them to
correctly implement constant and inverted output levels.

Using constant bit allows to have truly stable low or high output level.
Since hi and low regs internally increment its values by 1 just writing
zero to any of them gives 1 clock count impulse. If constant bit is set
zero value in hi and low regs is not incremented.

Using polarity bit instead of swapping hi and low reg values allows to
correctly identify inversion in .get_state().

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/pwm/pwm-meson.c | 63 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 2ef632caebcc..974c3c74768c 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -6,7 +6,7 @@
  * PWM output is achieved by calculating a clock that permits calculating
  * two periods (low and high). The counter then has to be set to switch after
  * N cycles for the first half period.
- * The hardware has no "polarity" setting. This driver reverses the period
+ * Partly the hardware has no "polarity" setting. This driver reverses the period
  * cycles (the low length is inverted with the high length) for
  * PWM_POLARITY_INVERSED. This means that .get_state cannot read the polarity
  * from the hardware.
@@ -56,6 +56,10 @@
 #define MISC_B_CLK_SEL_SHIFT	6
 #define MISC_A_CLK_SEL_SHIFT	4
 #define MISC_CLK_SEL_MASK	0x3
+#define MISC_B_CONSTANT_EN	BIT(29)
+#define MISC_A_CONSTANT_EN	BIT(28)
+#define MISC_B_INVERT_EN	BIT(27)
+#define MISC_A_INVERT_EN	BIT(26)
 #define MISC_B_EN		BIT(1)
 #define MISC_A_EN		BIT(0)
 
@@ -68,6 +72,8 @@ static struct meson_pwm_channel_data {
 	u8		clk_div_shift;
 	u8		clk_en_shift;
 	u32		pwm_en_mask;
+	u32		const_en_mask;
+	u32		inv_en_mask;
 } meson_pwm_per_channel_data[MESON_NUM_PWMS] = {
 	{
 		.reg_offset	= REG_PWM_A,
@@ -75,6 +81,8 @@ static struct meson_pwm_channel_data {
 		.clk_div_shift	= MISC_A_CLK_DIV_SHIFT,
 		.clk_en_shift	= MISC_A_CLK_EN_SHIFT,
 		.pwm_en_mask	= MISC_A_EN,
+		.const_en_mask	= MISC_A_CONSTANT_EN,
+		.inv_en_mask	= MISC_A_INVERT_EN,
 	},
 	{
 		.reg_offset	= REG_PWM_B,
@@ -82,6 +90,8 @@ static struct meson_pwm_channel_data {
 		.clk_div_shift	= MISC_B_CLK_DIV_SHIFT,
 		.clk_en_shift	= MISC_B_CLK_EN_SHIFT,
 		.pwm_en_mask	= MISC_B_EN,
+		.const_en_mask	= MISC_B_CONSTANT_EN,
+		.inv_en_mask	= MISC_B_INVERT_EN,
 	}
 };
 
@@ -89,6 +99,8 @@ struct meson_pwm_channel {
 	unsigned long rate;
 	unsigned int hi;
 	unsigned int lo;
+	bool constant;
+	bool inverted;
 
 	struct clk_mux mux;
 	struct clk_divider div;
@@ -99,6 +111,8 @@ struct meson_pwm_channel {
 struct meson_pwm_data {
 	const char *const parent_names[MESON_NUM_MUX_PARENTS];
 	int (*channels_init)(struct pwm_chip *chip);
+	bool has_constant;
+	bool has_polarity;
 };
 
 struct meson_pwm {
@@ -160,7 +174,7 @@ static int meson_pwm_calc(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * Fixing this needs some care however as some machines might rely on
 	 * this.
 	 */
-	if (state->polarity == PWM_POLARITY_INVERSED)
+	if (state->polarity == PWM_POLARITY_INVERSED && !meson->data->has_polarity)
 		duty = period - duty;
 
 	freq = div64_u64(NSEC_PER_SEC * 0xffffULL, period);
@@ -187,9 +201,11 @@ static int meson_pwm_calc(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (duty == period) {
 		channel->hi = cnt;
 		channel->lo = 0;
+		channel->constant = true;
 	} else if (duty == 0) {
 		channel->hi = 0;
 		channel->lo = cnt;
+		channel->constant = true;
 	} else {
 		duty_cnt = mul_u64_u64_div_u64(fin_freq, duty, NSEC_PER_SEC);
 
@@ -197,6 +213,7 @@ static int meson_pwm_calc(struct pwm_chip *chip, struct pwm_device *pwm,
 
 		channel->hi = duty_cnt;
 		channel->lo = cnt - duty_cnt;
+		channel->constant = false;
 	}
 
 	channel->rate = fin_freq;
@@ -204,6 +221,14 @@ static int meson_pwm_calc(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
+static inline void meson_pwm_assign_bit(u32 *data, u32 mask, bool value)
+{
+	if (value)
+		*data |= mask;
+	else
+		*data &= ~mask;
+}
+
 static void meson_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct meson_pwm *meson = to_meson_pwm(chip);
@@ -227,6 +252,15 @@ static void meson_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	value = readl(meson->base + REG_MISC_AB);
 	value |= channel_data->pwm_en_mask;
+
+	if (meson->data->has_constant)
+		meson_pwm_assign_bit(&value, channel_data->const_en_mask,
+				     channel->constant);
+
+	if (meson->data->has_polarity)
+		meson_pwm_assign_bit(&value, channel_data->inv_en_mask,
+				     channel->inverted);
+
 	writel(value, meson->base + REG_MISC_AB);
 
 	spin_unlock_irqrestore(&meson->lock, flags);
@@ -235,13 +269,22 @@ static void meson_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 static void meson_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct meson_pwm *meson = to_meson_pwm(chip);
+	struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm];
+	struct meson_pwm_channel_data *channel_data;
 	unsigned long flags;
 	u32 value;
 
+	channel_data = &meson_pwm_per_channel_data[pwm->hwpwm];
+
 	spin_lock_irqsave(&meson->lock, flags);
 
 	value = readl(meson->base + REG_MISC_AB);
-	value &= ~meson_pwm_per_channel_data[pwm->hwpwm].pwm_en_mask;
+	value &= ~channel_data->pwm_en_mask;
+
+	if (meson->data->has_polarity)
+		meson_pwm_assign_bit(&value, channel_data->inv_en_mask,
+				     channel->inverted);
+
 	writel(value, meson->base + REG_MISC_AB);
 
 	spin_unlock_irqrestore(&meson->lock, flags);
@@ -254,10 +297,12 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm];
 	int err = 0;
 
+	channel->inverted = (state->polarity == PWM_POLARITY_INVERSED);
+
 	if (!state->enabled) {
-		if (state->polarity == PWM_POLARITY_INVERSED) {
+		if (channel->inverted && !meson->data->has_polarity) {
 			/*
-			 * This IP block revision doesn't have an "always high"
+			 * Some of IP block revisions don't have an "always high"
 			 * setting which we can use for "inverted disabled".
 			 * Instead we achieve this by setting mux parent with
 			 * highest rate and minimum divider value, resulting
@@ -271,6 +316,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			channel->rate = ULONG_MAX;
 			channel->hi = ~0;
 			channel->lo = 0;
+			channel->constant = true;
 
 			meson_pwm_enable(chip, pwm);
 		} else {
@@ -319,6 +365,11 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	value = readl(meson->base + REG_MISC_AB);
 	state->enabled = value & channel_data->pwm_en_mask;
 
+	if (meson->data->has_polarity && (value & channel_data->inv_en_mask))
+		state->polarity = PWM_POLARITY_INVERSED;
+	else
+		state->polarity = PWM_POLARITY_NORMAL;
+
 	value = readl(meson->base + channel_data->reg_offset);
 	lo = FIELD_GET(PWM_LOW_MASK, value);
 	hi = FIELD_GET(PWM_HIGH_MASK, value);
@@ -326,8 +377,6 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	state->period = meson_pwm_cnt_to_ns(chip, pwm, lo + hi);
 	state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, hi);
 
-	state->polarity = PWM_POLARITY_NORMAL;
-
 	return 0;
 }
 
-- 
2.25.1

[PATCH v2 3/4] pwm: meson: Use separate device id data for axg and g12

[George Stark]

Add separate devce id data for compatibles: amlogic,meson-g12a-ee-pwm,
amlogic,meson-axg-pwm-v2, amlogic,meson-g12-pwm-v2 due to those PWM
modules have different set of features than meson8.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/pwm/pwm-meson.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 974c3c74768c..d9d51f0af103 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -565,6 +565,11 @@ static const struct meson_pwm_data pwm_axg_ao_data = {
 	.channels_init = meson_pwm_init_channels_meson8b_legacy,
 };
 
+static const struct meson_pwm_data pwm_g12a_ee_data = {
+	.parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
+	.channels_init = meson_pwm_init_channels_meson8b_legacy,
+};
+
 static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
 	.parent_names = { "xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5" },
 	.channels_init = meson_pwm_init_channels_meson8b_legacy,
@@ -579,6 +584,10 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
 	.channels_init = meson_pwm_init_channels_meson8b_v2,
 };
 
+static const struct meson_pwm_data pwm_meson_axg_v2_data = {
+	.channels_init = meson_pwm_init_channels_meson8b_v2,
+};
+
 static const struct meson_pwm_data pwm_s4_data = {
 	.channels_init = meson_pwm_init_channels_s4,
 };
@@ -588,6 +597,14 @@ static const struct of_device_id meson_pwm_matches[] = {
 		.compatible = "amlogic,meson8-pwm-v2",
 		.data = &pwm_meson8_v2_data
 	},
+	{
+		.compatible = "amlogic,meson-axg-pwm-v2",
+		.data = &pwm_meson_axg_v2_data
+	},
+	{
+		.compatible = "amlogic,meson-g12-pwm-v2",
+		.data = &pwm_meson_axg_v2_data
+	},
 	/* The following compatibles are obsolete */
 	{
 		.compatible = "amlogic,meson8b-pwm",
@@ -611,7 +628,7 @@ static const struct of_device_id meson_pwm_matches[] = {
 	},
 	{
 		.compatible = "amlogic,meson-g12a-ee-pwm",
-		.data = &pwm_meson8b_data
+		.data = &pwm_g12a_ee_data
 	},
 	{
 		.compatible = "amlogic,meson-g12a-ao-pwm-ab",
-- 
2.25.1

[PATCH v2 4/4] pwm: meson: Enable constant and polarity features for g12, axg, s4

[George Stark]

g12, axg and s4 SoC families support constant and polarity bits
so enable those features in corresponding chip data structs.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/pwm/pwm-meson.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index d9d51f0af103..ca822174f7ba 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -558,26 +558,36 @@ static const struct meson_pwm_data pwm_gxbb_ao_data = {
 static const struct meson_pwm_data pwm_axg_ee_data = {
 	.parent_names = { "xtal", "fclk_div5", "fclk_div4", "fclk_div3" },
 	.channels_init = meson_pwm_init_channels_meson8b_legacy,
+	.has_constant = true,
+	.has_polarity = true,
 };
 
 static const struct meson_pwm_data pwm_axg_ao_data = {
 	.parent_names = { "xtal", "axg_ao_clk81", "fclk_div4", "fclk_div5" },
 	.channels_init = meson_pwm_init_channels_meson8b_legacy,
+	.has_constant = true,
+	.has_polarity = true,
 };
 
 static const struct meson_pwm_data pwm_g12a_ee_data = {
 	.parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
 	.channels_init = meson_pwm_init_channels_meson8b_legacy,
+	.has_constant = true,
+	.has_polarity = true,
 };
 
 static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
 	.parent_names = { "xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5" },
 	.channels_init = meson_pwm_init_channels_meson8b_legacy,
+	.has_constant = true,
+	.has_polarity = true,
 };
 
 static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
 	.parent_names = { "xtal", "g12a_ao_clk81", NULL, NULL },
 	.channels_init = meson_pwm_init_channels_meson8b_legacy,
+	.has_constant = true,
+	.has_polarity = true,
 };
 
 static const struct meson_pwm_data pwm_meson8_v2_data = {
@@ -586,10 +596,14 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
 
 static const struct meson_pwm_data pwm_meson_axg_v2_data = {
 	.channels_init = meson_pwm_init_channels_meson8b_v2,
+	.has_constant = true,
+	.has_polarity = true,
 };
 
 static const struct meson_pwm_data pwm_s4_data = {
 	.channels_init = meson_pwm_init_channels_s4,
+	.has_constant = true,
+	.has_polarity = true,
 };
 
 static const struct of_device_id meson_pwm_matches[] = {
-- 
2.25.1

Re: [PATCH v2 1/4] pwm: meson: Simplify get_state() callback

[kernel test robot]

Hi George,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.12-rc3 next-20241018]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/George-Stark/pwm-meson-Simplify-get_state-callback/20241016-232751
base:   linus/master
patch link:    https://lore.kernel.org/r/20241016152553.2321992-2-gnstark%40salutedevices.com
patch subject: [PATCH v2 1/4] pwm: meson: Simplify get_state() callback
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20241020/202410201612.QJbPOweL-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241020/202410201612.QJbPOweL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410201612.QJbPOweL-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/pwm/pwm-meson.c: In function 'meson_pwm_get_state':
>> drivers/pwm/pwm-meson.c:312:35: warning: variable 'channel' set but not used [-Wunused-but-set-variable]
     312 |         struct meson_pwm_channel *channel;
         |                                   ^~~~~~~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]


vim +/channel +312 drivers/pwm/pwm-meson.c

c375bcbaabdb92 Martin Blumenstingl 2019-06-12  306  
6c452cff79f8bf Uwe Kleine-König    2022-12-02  307  static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
211ed630753d2f Neil Armstrong      2016-08-22  308  			       struct pwm_state *state)
211ed630753d2f Neil Armstrong      2016-08-22  309  {
211ed630753d2f Neil Armstrong      2016-08-22  310  	struct meson_pwm *meson = to_meson_pwm(chip);
c375bcbaabdb92 Martin Blumenstingl 2019-06-12  311  	struct meson_pwm_channel_data *channel_data;
c375bcbaabdb92 Martin Blumenstingl 2019-06-12 @312  	struct meson_pwm_channel *channel;
2acdf419b01bae George Stark        2024-10-16  313  	unsigned int hi, lo;
329db102a26da0 Heiner Kallweit     2023-05-24  314  	u32 value;
211ed630753d2f Neil Armstrong      2016-08-22  315  
c375bcbaabdb92 Martin Blumenstingl 2019-06-12  316  	channel = &meson->channels[pwm->hwpwm];
c375bcbaabdb92 Martin Blumenstingl 2019-06-12  317  	channel_data = &meson_pwm_per_channel_data[pwm->hwpwm];
211ed630753d2f Neil Armstrong      2016-08-22  318  
211ed630753d2f Neil Armstrong      2016-08-22  319  	value = readl(meson->base + REG_MISC_AB);
329db102a26da0 Heiner Kallweit     2023-05-24  320  	state->enabled = value & channel_data->pwm_en_mask;
c375bcbaabdb92 Martin Blumenstingl 2019-06-12  321  
c375bcbaabdb92 Martin Blumenstingl 2019-06-12  322  	value = readl(meson->base + channel_data->reg_offset);
2acdf419b01bae George Stark        2024-10-16  323  	lo = FIELD_GET(PWM_LOW_MASK, value);
2acdf419b01bae George Stark        2024-10-16  324  	hi = FIELD_GET(PWM_HIGH_MASK, value);
c375bcbaabdb92 Martin Blumenstingl 2019-06-12  325  
2acdf419b01bae George Stark        2024-10-16  326  	state->period = meson_pwm_cnt_to_ns(chip, pwm, lo + hi);
2acdf419b01bae George Stark        2024-10-16  327  	state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, hi);
6c452cff79f8bf Uwe Kleine-König    2022-12-02  328  
8caa81eb950cb2 Uwe Kleine-König    2023-03-22  329  	state->polarity = PWM_POLARITY_NORMAL;
8caa81eb950cb2 Uwe Kleine-König    2023-03-22  330  
6c452cff79f8bf Uwe Kleine-König    2022-12-02  331  	return 0;
211ed630753d2f Neil Armstrong      2016-08-22  332  }
211ed630753d2f Neil Armstrong      2016-08-22  333  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/4] pwm: meson: Simplify get_state() callback

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

On Wed, Oct 16, 2024 at 06:25:50PM +0300, George Stark wrote:
> In .get_state() callback meson_pwm_channel struct are used to store
> lo and hi reg values but they are never reused after that so
> for clearness use local variable instead.
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>  drivers/pwm/pwm-meson.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 98e6c1533312..2ef632caebcc 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -310,6 +310,7 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct meson_pwm *meson = to_meson_pwm(chip);
>  	struct meson_pwm_channel_data *channel_data;
>  	struct meson_pwm_channel *channel;
> +	unsigned int hi, lo;
>  	u32 value;
>  
>  	channel = &meson->channels[pwm->hwpwm];
> @@ -319,11 +320,11 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	state->enabled = value & channel_data->pwm_en_mask;
>  
>  	value = readl(meson->base + channel_data->reg_offset);
> -	channel->lo = FIELD_GET(PWM_LOW_MASK, value);
> -	channel->hi = FIELD_GET(PWM_HIGH_MASK, value);
> +	lo = FIELD_GET(PWM_LOW_MASK, value);
> +	hi = FIELD_GET(PWM_HIGH_MASK, value);
>  
> -	state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi);
> -	state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi);
> +	state->period = meson_pwm_cnt_to_ns(chip, pwm, lo + hi);
> +	state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, hi);
>  
>  	state->polarity = PWM_POLARITY_NORMAL;

Fine for me if you drop the local variable channel as found by the build
bot.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] pwm: meson: Support constant and polarity bits

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4262 bytes --]

Hello George,

there are two minor things I dislike in this patch/driver. But I'm not
sure the alternatives are objectively considerably better. See below and
judge yourself.

On Wed, Oct 16, 2024 at 06:25:51PM +0300, George Stark wrote:
> Newer meson PWM IPs support constant and polarity bits. Support them to
> correctly implement constant and inverted output levels.
> 
> Using constant bit allows to have truly stable low or high output level.
> Since hi and low regs internally increment its values by 1 just writing
> zero to any of them gives 1 clock count impulse. If constant bit is set
> zero value in hi and low regs is not incremented.
> 
> Using polarity bit instead of swapping hi and low reg values allows to
> correctly identify inversion in .get_state().
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>  drivers/pwm/pwm-meson.c | 63 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 56 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 2ef632caebcc..974c3c74768c 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -6,7 +6,7 @@
>   * PWM output is achieved by calculating a clock that permits calculating
>   * two periods (low and high). The counter then has to be set to switch after
>   * N cycles for the first half period.
> - * The hardware has no "polarity" setting. This driver reverses the period
> + * Partly the hardware has no "polarity" setting. This driver reverses the period
>   * cycles (the low length is inverted with the high length) for
>   * PWM_POLARITY_INVERSED. This means that .get_state cannot read the polarity
>   * from the hardware.
> @@ -56,6 +56,10 @@
>  #define MISC_B_CLK_SEL_SHIFT	6
>  #define MISC_A_CLK_SEL_SHIFT	4
>  #define MISC_CLK_SEL_MASK	0x3
> +#define MISC_B_CONSTANT_EN	BIT(29)
> +#define MISC_A_CONSTANT_EN	BIT(28)
> +#define MISC_B_INVERT_EN	BIT(27)
> +#define MISC_A_INVERT_EN	BIT(26)
>  #define MISC_B_EN		BIT(1)
>  #define MISC_A_EN		BIT(0)
>  
> @@ -68,6 +72,8 @@ static struct meson_pwm_channel_data {
>  	u8		clk_div_shift;
>  	u8		clk_en_shift;
>  	u32		pwm_en_mask;
> +	u32		const_en_mask;
> +	u32		inv_en_mask;
>  } meson_pwm_per_channel_data[MESON_NUM_PWMS] = {
>  	{
>  		.reg_offset	= REG_PWM_A,
> @@ -75,6 +81,8 @@ static struct meson_pwm_channel_data {
>  		.clk_div_shift	= MISC_A_CLK_DIV_SHIFT,
>  		.clk_en_shift	= MISC_A_CLK_EN_SHIFT,
>  		.pwm_en_mask	= MISC_A_EN,
> +		.const_en_mask	= MISC_A_CONSTANT_EN,
> +		.inv_en_mask	= MISC_A_INVERT_EN,
>  	},
>  	{
>  		.reg_offset	= REG_PWM_B,
> @@ -82,6 +90,8 @@ static struct meson_pwm_channel_data {
>  		.clk_div_shift	= MISC_B_CLK_DIV_SHIFT,
>  		.clk_en_shift	= MISC_B_CLK_EN_SHIFT,
>  		.pwm_en_mask	= MISC_B_EN,
> +		.const_en_mask	= MISC_B_CONSTANT_EN,
> +		.inv_en_mask	= MISC_B_INVERT_EN,
>  	}
>  };

So the generic register description describes the const and invert bits,
but it doesn't apply to all IPs. Thinking about that, I wonder why this
struct exists at all. I would have done this as follows:

	#define MESON_PWM_REG_PWM(chan)		(0 + 4 * (chan))

	#define MESON_PWM_REG_MISC		(8)
	#define MESON_PWM_REG_MISC_EN(chan)		BIT(chan)
	#define MESON_PWM_REG_MISC_CLK_SEL(chan)	GENMASK(5 + 2 * (chan), 4 + 2 * (chan))
	....

and then use these constants directly (with pwm->hwpwm as parameter if
needed) in the code. I would expect this to result in more efficient and
smaller code.

> @@ -227,6 +252,15 @@ static void meson_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  
>  	value = readl(meson->base + REG_MISC_AB);
>  	value |= channel_data->pwm_en_mask;
> +
> +	if (meson->data->has_constant)
> +		meson_pwm_assign_bit(&value, channel_data->const_en_mask,
> +				     channel->constant);

Personally I'd prefer:

	value &= ~MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
	if (meson->data->has_constant && channel->constant)
		value |= MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);

even though your variant only mentions the mask once. While it has this
repetition, it's clear what happens without having to know what
meson_pwm_assign_bit() does. Maybe that's subjective?

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] pwm: meson: Support constant and polarity bits

[George Stark]

Hello Uwe

Thanks for the review.

On 11/4/24 12:32, Uwe Kleine-König wrote:
> Hello George,
> 
> there are two minor things I dislike in this patch/driver. But I'm not
> sure the alternatives are objectively considerably better. See below and
> judge yourself.

...

>> @@ -68,6 +72,8 @@ static struct meson_pwm_channel_data {
>>   	u8		clk_div_shift;
>>   	u8		clk_en_shift;
>>   	u32		pwm_en_mask;
>> +	u32		const_en_mask;
>> +	u32		inv_en_mask;
>>   } meson_pwm_per_channel_data[MESON_NUM_PWMS] = {
>>   	{
>>   		.reg_offset	= REG_PWM_A,
>> @@ -75,6 +81,8 @@ static struct meson_pwm_channel_data {
>>   		.clk_div_shift	= MISC_A_CLK_DIV_SHIFT,
>>   		.clk_en_shift	= MISC_A_CLK_EN_SHIFT,
>>   		.pwm_en_mask	= MISC_A_EN,
>> +		.const_en_mask	= MISC_A_CONSTANT_EN,
>> +		.inv_en_mask	= MISC_A_INVERT_EN,
>>   	},
>>   	{
>>   		.reg_offset	= REG_PWM_B,
>> @@ -82,6 +90,8 @@ static struct meson_pwm_channel_data {
>>   		.clk_div_shift	= MISC_B_CLK_DIV_SHIFT,
>>   		.clk_en_shift	= MISC_B_CLK_EN_SHIFT,
>>   		.pwm_en_mask	= MISC_B_EN,
>> +		.const_en_mask	= MISC_B_CONSTANT_EN,
>> +		.inv_en_mask	= MISC_B_INVERT_EN,
>>   	}
>>   };
> 
> So the generic register description describes the const and invert bits,
> but it doesn't apply to all IPs. Thinking about that, I wonder why this
> struct exists at all. I would have done this as follows:
> 
> 	#define MESON_PWM_REG_PWM(chan)		(0 + 4 * (chan))
> 
> 	#define MESON_PWM_REG_MISC		(8)
> 	#define MESON_PWM_REG_MISC_EN(chan)		BIT(chan)
> 	#define MESON_PWM_REG_MISC_CLK_SEL(chan)	GENMASK(5 + 2 * (chan), 4 + 2 * (chan))
> 	....
> 
> and then use these constants directly (with pwm->hwpwm as parameter if
> needed) in the code. I would expect this to result in more efficient and
> smaller code.

I've been looking into this driver for more than a year and got used to
it so much so never thought about changing the foundations :) Although 
it's an interesting thought.

1. I took meson_pwm_enable() without
const patches and reimplemented it using only defines (e.g. w/o local
var channel_data) and objdumped current and new versions. New version
turned out to be one instruction longer (arm64, gcc, default -O2). So 
total difference in executable code may be not that significant although
we can win in C-code line count.

2. Things like
#define MISC_B_EN		BIT(1)
#define MISC_A_EN		BIT(0)
is more straightforward and can be matched to the datasheet easier
comparing to (a + b * (chan)) things.

So I'm not sure either.

>> @@ -227,6 +252,15 @@ static void meson_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>>   
>>   	value = readl(meson->base + REG_MISC_AB);
>>   	value |= channel_data->pwm_en_mask;
>> +
>> +	if (meson->data->has_constant)
>> +		meson_pwm_assign_bit(&value, channel_data->const_en_mask,
>> +				     channel->constant);
> 
> Personally I'd prefer:
> 
> 	value &= ~MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
> 	if (meson->data->has_constant && channel->constant)
> 		value |= MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
> 
> even though your variant only mentions the mask once. While it has this
> repetition, it's clear what happens without having to know what
> meson_pwm_assign_bit() does. Maybe that's subjective?
> 

Actually I also don't like meson_pwm_assign_bit() too match and I'm
surprised there's no something like this in the kernel already.
I again objdumped versions meson_pwm_assign_bit() vs double mask 
repetition. Unconditional bit clearing takes only a single instruction:

// value &= ~channel_data->const_en_mask;
9ac:	0a250040 	bic	w0, w2, w5

So in the current series I could drop meson_pwm_assign_bit() and use:

value &= ~channel_data->const_en_mask;
if (meson->data->has_constant && channel->constant)
	value |= channel_data->const_en_mask;

If it's decided now or later to drop meson_pwm_channel_data then
w\o meson_pwm_assign_bit() future patch will be line-to-line change.

What you think?

> Best regards
> Uwe

-- 
Best regards
George

Re: [PATCH v2 2/4] pwm: meson: Support constant and polarity bits

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4335 bytes --]

On Wed, Nov 06, 2024 at 04:54:41PM +0300, George Stark wrote:
> On 11/4/24 12:32, Uwe Kleine-König wrote:
> > > @@ -68,6 +72,8 @@ static struct meson_pwm_channel_data {
> > >   	u8		clk_div_shift;
> > >   	u8		clk_en_shift;
> > >   	u32		pwm_en_mask;
> > > +	u32		const_en_mask;
> > > +	u32		inv_en_mask;
> > >   } meson_pwm_per_channel_data[MESON_NUM_PWMS] = {
> > >   	{
> > >   		.reg_offset	= REG_PWM_A,
> > > @@ -75,6 +81,8 @@ static struct meson_pwm_channel_data {
> > >   		.clk_div_shift	= MISC_A_CLK_DIV_SHIFT,
> > >   		.clk_en_shift	= MISC_A_CLK_EN_SHIFT,
> > >   		.pwm_en_mask	= MISC_A_EN,
> > > +		.const_en_mask	= MISC_A_CONSTANT_EN,
> > > +		.inv_en_mask	= MISC_A_INVERT_EN,
> > >   	},
> > >   	{
> > >   		.reg_offset	= REG_PWM_B,
> > > @@ -82,6 +90,8 @@ static struct meson_pwm_channel_data {
> > >   		.clk_div_shift	= MISC_B_CLK_DIV_SHIFT,
> > >   		.clk_en_shift	= MISC_B_CLK_EN_SHIFT,
> > >   		.pwm_en_mask	= MISC_B_EN,
> > > +		.const_en_mask	= MISC_B_CONSTANT_EN,
> > > +		.inv_en_mask	= MISC_B_INVERT_EN,
> > >   	}
> > >   };
> > 
> > So the generic register description describes the const and invert bits,
> > but it doesn't apply to all IPs. Thinking about that, I wonder why this
> > struct exists at all. I would have done this as follows:
> > 
> > 	#define MESON_PWM_REG_PWM(chan)		(0 + 4 * (chan))
> > 
> > 	#define MESON_PWM_REG_MISC		(8)
> > 	#define MESON_PWM_REG_MISC_EN(chan)		BIT(chan)
> > 	#define MESON_PWM_REG_MISC_CLK_SEL(chan)	GENMASK(5 + 2 * (chan), 4 + 2 * (chan))
> > 	....
> > 
> > and then use these constants directly (with pwm->hwpwm as parameter if
> > needed) in the code. I would expect this to result in more efficient and
> > smaller code.
> 
> I've been looking into this driver for more than a year and got used to
> it so much so never thought about changing the foundations :) Although it's
> an interesting thought.
> 
> 1. I took meson_pwm_enable() without
> const patches and reimplemented it using only defines (e.g. w/o local
> var channel_data) and objdumped current and new versions. New version
> turned out to be one instruction longer (arm64, gcc, default -O2). So total
> difference in executable code may be not that significant although
> we can win in C-code line count.

Oh, I indeed would have expected a slight advantage size-wise. Maybe the
compiler already optimizes out the meson_pwm_per_channel_data variable.

> 2. Things like
> #define MISC_B_EN		BIT(1)
> #define MISC_A_EN		BIT(0)
> is more straightforward and can be matched to the datasheet easier
> comparing to (a + b * (chan)) things.

In my (subjective) view that comparison isn't hard with the
parametrised definition.

> So I'm not sure either.
> 
> > > @@ -227,6 +252,15 @@ static void meson_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > >   	value = readl(meson->base + REG_MISC_AB);
> > >   	value |= channel_data->pwm_en_mask;
> > > +
> > > +	if (meson->data->has_constant)
> > > +		meson_pwm_assign_bit(&value, channel_data->const_en_mask,
> > > +				     channel->constant);
> > 
> > Personally I'd prefer:
> > 
> > 	value &= ~MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
> > 	if (meson->data->has_constant && channel->constant)
> > 		value |= MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
> > 
> > even though your variant only mentions the mask once. While it has this
> > repetition, it's clear what happens without having to know what
> > meson_pwm_assign_bit() does. Maybe that's subjective?
> 
> Actually I also don't like meson_pwm_assign_bit() too match and I'm
> surprised there's no something like this in the kernel already.
> I again objdumped versions meson_pwm_assign_bit() vs double mask repetition.
> Unconditional bit clearing takes only a single instruction:
> 
> // value &= ~channel_data->const_en_mask;
> 9ac:	0a250040 	bic	w0, w2, w5
> 
> So in the current series I could drop meson_pwm_assign_bit() and use:
> 
> value &= ~channel_data->const_en_mask;
> if (meson->data->has_constant && channel->constant)
> 	value |= channel_data->const_en_mask;
> 
> If it's decided now or later to drop meson_pwm_channel_data then
> w\o meson_pwm_assign_bit() future patch will be line-to-line change.
> 
> What you think?

Sounds sensible.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] pwm: meson: Support constant and polarity bits

[George Stark]

Hello Uwe

On 11/7/24 11:41, Uwe Kleine-König wrote:
> On Wed, Nov 06, 2024 at 04:54:41PM +0300, George Stark wrote:
>> On 11/4/24 12:32, Uwe Kleine-König wrote:
>>>> @@ -68,6 +72,8 @@ static struct meson_pwm_channel_data {
>>>>    	u8		clk_div_shift;
>>>>    	u8		clk_en_shift;
>>>>    	u32		pwm_en_mask;
>>>> +	u32		const_en_mask;
>>>> +	u32		inv_en_mask;
>>>>    } meson_pwm_per_channel_data[MESON_NUM_PWMS] = {
>>>>    	{
>>>>    		.reg_offset	= REG_PWM_A,
>>>> @@ -75,6 +81,8 @@ static struct meson_pwm_channel_data {
>>>>    		.clk_div_shift	= MISC_A_CLK_DIV_SHIFT,
>>>>    		.clk_en_shift	= MISC_A_CLK_EN_SHIFT,
>>>>    		.pwm_en_mask	= MISC_A_EN,
>>>> +		.const_en_mask	= MISC_A_CONSTANT_EN,
>>>> +		.inv_en_mask	= MISC_A_INVERT_EN,
>>>>    	},
>>>>    	{
>>>>    		.reg_offset	= REG_PWM_B,
>>>> @@ -82,6 +90,8 @@ static struct meson_pwm_channel_data {
>>>>    		.clk_div_shift	= MISC_B_CLK_DIV_SHIFT,
>>>>    		.clk_en_shift	= MISC_B_CLK_EN_SHIFT,
>>>>    		.pwm_en_mask	= MISC_B_EN,
>>>> +		.const_en_mask	= MISC_B_CONSTANT_EN,
>>>> +		.inv_en_mask	= MISC_B_INVERT_EN,
>>>>    	}
>>>>    };

...

>>> Personally I'd prefer:
>>>
>>> 	value &= ~MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
>>> 	if (meson->data->has_constant && channel->constant)
>>> 		value |= MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
>>>
>>> even though your variant only mentions the mask once. While it has this
>>> repetition, it's clear what happens without having to know what
>>> meson_pwm_assign_bit() does. Maybe that's subjective?
>>
>> Actually I also don't like meson_pwm_assign_bit() too match and I'm
>> surprised there's no something like this in the kernel already.
>> I again objdumped versions meson_pwm_assign_bit() vs double mask repetition.
>> Unconditional bit clearing takes only a single instruction:
>>
>> // value &= ~channel_data->const_en_mask;
>> 9ac:	0a250040 	bic	w0, w2, w5
>>
>> So in the current series I could drop meson_pwm_assign_bit() and use:
>>
>> value &= ~channel_data->const_en_mask;
>> if (meson->data->has_constant && channel->constant)
>> 	value |= channel_data->const_en_mask;
>>
>> If it's decided now or later to drop meson_pwm_channel_data then
>> w\o meson_pwm_assign_bit() future patch will be line-to-line change.
>>
>> What you think?
> 
> Sounds sensible.

While changing the patch to drop meson_pwm_assign_bit() one thing
concerned me on the approach:

value &= ~channel_data->const_en_mask;
if (meson->data->has_constant && channel->constant)
	value |= channel_data->const_en_mask;

that we reset bit in the value var even if that bit is not supported on
the current SoC. I checked several datasheets for old SoCs and those 
bits are marked as unused (not even reserved) and I've never seen those
bits set. Still I'd offer to use precise condition for changing those 
bit. I'll send v3 let's discuss it again if you think I bother too much.

> 
> Best regards
> Uwe

-- 
Best regards
George

Re: [PATCH v2 2/4] pwm: meson: Support constant and polarity bits

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3585 bytes --]

Hello George,

On Tue, Nov 19, 2024 at 03:51:34PM +0300, George Stark wrote:
> On 11/7/24 11:41, Uwe Kleine-König wrote:
> > On Wed, Nov 06, 2024 at 04:54:41PM +0300, George Stark wrote:
> > > On 11/4/24 12:32, Uwe Kleine-König wrote:
> > > > > @@ -68,6 +72,8 @@ static struct meson_pwm_channel_data {
> > > > >    	u8		clk_div_shift;
> > > > >    	u8		clk_en_shift;
> > > > >    	u32		pwm_en_mask;
> > > > > +	u32		const_en_mask;
> > > > > +	u32		inv_en_mask;
> > > > >    } meson_pwm_per_channel_data[MESON_NUM_PWMS] = {
> > > > >    	{
> > > > >    		.reg_offset	= REG_PWM_A,
> > > > > @@ -75,6 +81,8 @@ static struct meson_pwm_channel_data {
> > > > >    		.clk_div_shift	= MISC_A_CLK_DIV_SHIFT,
> > > > >    		.clk_en_shift	= MISC_A_CLK_EN_SHIFT,
> > > > >    		.pwm_en_mask	= MISC_A_EN,
> > > > > +		.const_en_mask	= MISC_A_CONSTANT_EN,
> > > > > +		.inv_en_mask	= MISC_A_INVERT_EN,
> > > > >    	},
> > > > >    	{
> > > > >    		.reg_offset	= REG_PWM_B,
> > > > > @@ -82,6 +90,8 @@ static struct meson_pwm_channel_data {
> > > > >    		.clk_div_shift	= MISC_B_CLK_DIV_SHIFT,
> > > > >    		.clk_en_shift	= MISC_B_CLK_EN_SHIFT,
> > > > >    		.pwm_en_mask	= MISC_B_EN,
> > > > > +		.const_en_mask	= MISC_B_CONSTANT_EN,
> > > > > +		.inv_en_mask	= MISC_B_INVERT_EN,
> > > > >    	}
> > > > >    };
> 
> ...
> 
> > > > Personally I'd prefer:
> > > > 
> > > > 	value &= ~MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
> > > > 	if (meson->data->has_constant && channel->constant)
> > > > 		value |= MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
> > > > 
> > > > even though your variant only mentions the mask once. While it has this
> > > > repetition, it's clear what happens without having to know what
> > > > meson_pwm_assign_bit() does. Maybe that's subjective?
> > > 
> > > Actually I also don't like meson_pwm_assign_bit() too match and I'm
> > > surprised there's no something like this in the kernel already.
> > > I again objdumped versions meson_pwm_assign_bit() vs double mask repetition.
> > > Unconditional bit clearing takes only a single instruction:
> > > 
> > > // value &= ~channel_data->const_en_mask;
> > > 9ac:	0a250040 	bic	w0, w2, w5
> > > 
> > > So in the current series I could drop meson_pwm_assign_bit() and use:
> > > 
> > > value &= ~channel_data->const_en_mask;
> > > if (meson->data->has_constant && channel->constant)
> > > 	value |= channel_data->const_en_mask;
> > > 
> > > If it's decided now or later to drop meson_pwm_channel_data then
> > > w\o meson_pwm_assign_bit() future patch will be line-to-line change.
> > > 
> > > What you think?
> > 
> > Sounds sensible.
> 
> While changing the patch to drop meson_pwm_assign_bit() one thing
> concerned me on the approach:
> 
> value &= ~channel_data->const_en_mask;
> if (meson->data->has_constant && channel->constant)
> 	value |= channel_data->const_en_mask;
> 
> that we reset bit in the value var even if that bit is not supported on
> the current SoC. I checked several datasheets for old SoCs and those bits
> are marked as unused (not even reserved) and I've never seen those
> bits set. Still I'd offer to use precise condition for changing those bit.
> I'll send v3 let's discuss it again if you think I bother too much.

Usually writing zeros to unused bits is a safe procedure. If the
hardware we're talking to is a newer variant of the supported stuff, the
hardware engineer did something wrong if keeping the read bits or
writing them as zero is incompatible. So either way is fine for me.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]