Messages in this thread | | | Date | Wed, 25 Oct 2023 17:54:10 +0800 | Subject | Re: [RESEND PATCH v6 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support | From | Fenglin Wu <> |
| |
On 10/9/2023 12:01 PM, Fenglin Wu wrote: > > > 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. >
Hi Dmitry,
Can you please help to comment if this looks good for you? I actually have pushed a V7 to address your last comment before you made this one. V7 change: https://lore.kernel.org/linux-arm-msm/20230927-pm8xxx-vibrator-v7-1-b5d8c92ce818@quicinc.com/, just want to know how to move forward. Thanks
Fenglin
> >> Thanks. >>
| |