Messages in this thread | | | Subject | Re: [PATCH v5 2/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver | From | Srinivas Kandagatla <> | Date | Wed, 2 Dec 2020 15:17:37 +0000 |
| |
On 02/12/2020 09:56, Srinivas Kandagatla wrote: >>> + case PIN_CONFIG_SLEW_RATE: >>> + if (arg > LPI_SLEW_RATE_MAX) { >>> + dev_err(pctldev->dev, "invalid slew rate %u for pin: >>> %d\n", >>> + arg, group); >>> + return -EINVAL; >>> + } >>> + >>> + slew_offset = g->slew_offset; >>> + if (slew_offset == NO_SLEW) >>> + break; >>> + >>> + mutex_lock(&pctrl->slew_access_lock); >>> + sval = ioread32(pctrl->slew_base + LPI_SLEW_RATE_CTL_REG); >>> + >>> + for (i = 0; i < LPI_SLEW_BITS_SIZE; i++) { >>> + assign_bit(slew_offset, &sval, arg & 0x01); >>> + slew_offset++; >>> + arg = arg >> 1; >>> + } >> >> Isn't this loop just the same as >> >> FIELD_SET(3 << slew_offset, arg & 3, sval)
None of FIELD_* or replace_bits apis will work here, as the mask passed to those macros should be a constant #define. Passing variable to them in mask will result in compile error!
mask in this case is retrieved at runtime.
I think we should live with the existing code unless there is a strong reason for it to change! Or a better alternative.
--srini
>
> This will not work FIELD_SET will not clear any bits wich are already > set! assing_bit will work, but we could do better by adding slew_mask > per pin rather than slew_offset which should allow us to use > replace_bits straight away.
| |