Messages in this thread | | | Date | Mon, 9 Oct 2023 12:01:03 +0800 | Subject | Re: [RESEND PATCH v6 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support | From | Fenglin Wu <> |
| |
On 10/1/2023 12:17 AM, Dmitry Torokhov wrote: > On Mon, Sep 25, 2023 at 10:54:45AM +0800, Fenglin Wu wrote: >> >> >> On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote: >>>> + >>>> + switch (vib->data->hw_type) { >>>> + case SSBI_VIB: >>>> mask = SSBI_VIB_DRV_LEVEL_MASK; >>>> shift = SSBI_VIB_DRV_SHIFT; >>>> + break; >>>> + case SPMI_VIB: >>>> + mask = SPMI_VIB_DRV_LEVEL_MASK; >>>> + shift = SPMI_VIB_DRV_SHIFT; >>>> + break; >>>> + case SPMI_VIB_GEN2: >>>> + mask = SPMI_VIB_GEN2_DRV_MASK; >>>> + shift = SPMI_VIB_GEN2_DRV_SHIFT; >>>> + break; >>>> + default: >>>> + return -EINVAL; >>> Could you please move the switch to the previous patch? Then it would >>> be more obvious that you are just adding the SPMI_VIB_GEN2 here. >>> >>> Other than that LGTM. >> >> Sure, I can move the switch to the previous refactoring patch. > > Actually, the idea of having a const "reg" or "chip", etc. structure is > to avoid this kind of runtime checks based on hardware type and instead > use common computation. I believe you need to move mask and shift into > the chip-specific structure and avoid defining hw_type. >
Actually, the main motivation for adding 'hw_type' is to avoid reading 'reg_base' from DT for SSBI_VIB. It can also help to simplify the 'pm8xxx_vib_data' structure and make following code logic more straightforward and easier to understand(check hw_type instead of checking specific constant reg/mask value), it has been used in following places:
1) Avoid reading 'reg_base' from DT for SSBI_VIB. 2) Only do manual-mode-mask-write for SSBI_VIB. Previously, it was achieved by giving a valid 'drv_en_manual_mask' value only for SSBI_VIB, having hw_type make it more straightforward. 3) Not writing VIB_EN register for SSBI_VIB. A similar strategy was used previously, only write VIB_EN register when 'enable_mask' is valid, checking hw_type make it more straightforward. 4) To achieve different drive step size for SPMI_VIB (100mV per step) and SPMI_VIB_GEN2 (1mV per step). 5) Do different VIB_DRV mask and shift assignment for SPMI_VIB and SPMI_VIB_GEN2 6) Only write VIB_DRV2 for SPMI_VIB_GEN2.
> Thanks. >
| |