Messages in this thread | | | Date | Thu, 27 Jul 2023 17:40:36 +0800 | Subject | Re: [PATCH v3 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator | From | Fenglin Wu <> |
| |
On 7/27/2023 5:22 PM, Krzysztof Kozlowski wrote: > On 27/07/2023 09:43, Fenglin Wu wrote: >> >> >> On 7/27/2023 3:07 PM, Krzysztof Kozlowski wrote: >>> On 25/07/2023 08:16, Fenglin Wu wrote: >>>>>> >>>>>> -static const struct pm8xxx_regs pm8058_regs = { >>>>>> - .drv_addr = 0x4A, >>>>>> - .drv_mask = 0xf8, >>>>>> - .drv_shift = 3, >>>>>> - .drv_en_manual_mask = 0xfc, >>>>>> +static struct reg_field ssbi_vib_regs[VIB_MAX_REG] = { >>>>> >>>>> Change from const to non-const is wrong. How do you support multiple >>>>> devices? No, this is way too fragile now. >>>>> >>>> >>>> The register definition is no longer used as the match data, hw_type is >>>> used. >>>> >>>> The last suggestion was getting the register base address from the DT >>>> and it has to be added into the offset of SPMI vibrator registers >>>> (either in the previous hard-coded format or the later the reg_filed >>>> data structure), so it's not appropriated to make it constant. >>>> >>>> I don't understand this question: "How do you support multiple devices?" >>>> For SSBI vibrator, since all the registers are fixed, and I would assume >>>> that there is no chance to support multiple vibrator devices on the same >>>> SSBI bus. If they are not on the same bus, the regmap device will be >>>> different while the registers definition is the same, and we are still >>>> able to support multiple devices, right? >>> >>> No, you have static memory. One device probes and changes static memory >>> to reg+=base1. Second device probes and changes the same to reg+=base2. >> >> Thanks, got it. I can update it with following 2 options: >> >> 1) keep the register definition in 'reg_filed' data structure and make >> it constant, copy it to a dynamically allocated memory before adding the >> 'reg_base' to the '.reg' variable. >> >> 2) Define the register offsets as constant data and add the 'reg_base' >> to the 'reg' while using 'regmap_read()'/'regmap_write()' functions. >> >> which one is the preferred way? > > Depends on the code. I am not sure if 2 would work with regmap_fields. > OTOH, I wonder if the device could just create its own regmap instead of > using parents? Then there would be no need of this offset dance. > > Anyway, adding offset only for some variants seems also not needed. You > should add offset to each variant, because each device has this offset. > > Best regards, > Krzysztof > Thanks for the suggestion.
The Qualcomm SPMI device has to use the 'regmap' from its parent with 16 'reg_bits' and 8 'val_bits' config, the higher 8-bit 'reg_bits' is the peripheral ID (PID) and it could be different in different PMICs even for the same type of HW module, and (PID << 8) is the 'reg_base' here.
I assume that you are not in favor of copying the constant data into a dynamic allocated memory, so I will go with option 2. Thanks
| |