On Sat, Apr 19, 2025 at 8:47 PM Marcelo Schmitt marcelo.schmitt1@gmail.com wrote:
...
@@ -411,14 +417,15 @@ static ssize_t ad5933_store(struct device *dev, ret = ad5933_cmd(st, 0); break; case AD5933_OUT_SETTLING_CYCLES:
val = clamp(val, (u16)0, (u16)0x7FF);
val = clamp(val, (u16)0, (u16)AD5933_MAX_SETTLING_CYCLES); st->settling_cycles = val;
/* 2x, 4x handling, see datasheet */
/* Encode value for register: D10..D0 *//* Datasheet Table 13: If cycles > 1022 -> val/4, set bits D10=1, D9=1 */ if (val > 1022)
val = (val >> 2) | (3 << 9);else if (val > 511)val = (val >> 1) | BIT(9);
val = (val >> 2) | AD5933_SETTLE_MUL_4X;then this would become something like
reg_data &= ~AD5933_SETTLE_MUL_MSK; reg_data |= FIELD_PREP(AD5933_SETTLE_MUL_MSK, AD5933_SETTLE_MUL_4X); reg_data &= ~AD5933_SETTLING_TIME_CYCLES_MSK; reg_data |= FIELD_PREP(AD5933_SETTLING_TIME_CYCLES_MSK, val >> 2);I currently have: val >>= 2; val |= FIELD_PREP(AD5933_SETTLING_MULTIPLIER_MASK, AD5933_SETTLING_MULTIPLIER_VAL_X4); which assumes val only has bits within a certain range which I believe is the case here but not completely sure. Would it be better to clear the bits regardless and then apply said operations?
Nah, since val is being clamped to max settling time value, I think this is good.
Got it.
...
Though, I guess it would then be preferable to use masks and bitfield macros for all other places where we handle register data in ad5933 driver. Probably something for a different patch (if worth it).
I separated the original fix from the refactoring so the patches stay modular. I can apply the use of masks and bitfield macros for other register data. Should the refactoring be all in one patch or spread across multiple?
I believe all mask/bitfield refactoring can be done in one patch.
Sounds good.