| Message ID | 20230922083801.3056724-4-quic_fenglinw@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND,v6,1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator | expand |
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. > >> }
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. Thanks.
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. >
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. >>
On 2023/10/1 0:17, 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. > > Thanks. Hi Dmitry, The v7 changes have been pending for a while, I am not sure if you are still insist on this. As I explained, I actually did it this way in v2 and it got updated to this by following other comments. Can you respond and tell me if you prefer changes similar to v2? I can update and push v8 by following your suggestion. v7: https://lore.kernel.org/linux-arm-msm/20231108-pm8xxx-vibrator-v7-0-632c731d25a8@quicinc.com/ v2: https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-3-quic_fenglinw@quicinc.com/ Thanks >
Hi Fenglin, On Thu, Mar 28, 2024 at 02:52:32PM +0800, Fenglin Wu wrote: > > > On 2023/10/1 0:17, 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. > > > > Thanks. > > Hi Dmitry, > > The v7 changes have been pending for a while, I am not sure if you are still > insist on this. As I explained, I actually did it this way in v2 and it got > updated to this by following other comments. > > Can you respond and tell me if you prefer changes similar to v2? I can > update and push v8 by following your suggestion. > > v7: https://lore.kernel.org/linux-arm-msm/20231108-pm8xxx-vibrator-v7-0-632c731d25a8@quicinc.com/ > > v2: https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-3-quic_fenglinw@quicinc.com/ Yes, I believe what you had in v2 was better, and Dmitry Baryshkov's comments on v2 were also great. You can have 2 styles of code - you have a hw type for each regulator and then use it to do conditional logic in the code. If you do it this way you and you need to add a new device type or model you have to go through the code and validate all the checks. Or you could have a structure that is defined flexibly enough to cover all existing permutations, and you rely on the data in it to control the behavior. You should not mix the 2 styles, as this just makes the code more confusing. Thanks.
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c index d6b468324c77..990e8a9ac018 100644 --- a/drivers/input/misc/pm8xxx-vibrator.c +++ b/drivers/input/misc/pm8xxx-vibrator.c @@ -21,6 +21,13 @@ #define SPMI_VIB_DRV_LEVEL_MASK GENMASK(4, 0) #define SPMI_VIB_DRV_SHIFT 0 +#define SPMI_VIB_GEN2_DRV_REG 0x40 +#define SPMI_VIB_GEN2_DRV_MASK GENMASK(7, 0) +#define SPMI_VIB_GEN2_DRV_SHIFT 0 +#define SPMI_VIB_GEN2_DRV2_REG 0x41 +#define SPMI_VIB_GEN2_DRV2_MASK GENMASK(3, 0) +#define SPMI_VIB_GEN2_DRV2_SHIFT 8 + #define SPMI_VIB_EN_REG 0x46 #define SPMI_VIB_EN_BIT BIT(7) @@ -33,12 +40,14 @@ enum vib_hw_type { SSBI_VIB, SPMI_VIB, + SPMI_VIB_GEN2 }; struct pm8xxx_vib_data { enum vib_hw_type hw_type; unsigned int enable_addr; unsigned int drv_addr; + unsigned int drv2_addr; }; static const struct pm8xxx_vib_data ssbi_vib_data = { @@ -52,6 +61,13 @@ static const struct pm8xxx_vib_data spmi_vib_data = { .drv_addr = SPMI_VIB_DRV_REG, }; +static const struct pm8xxx_vib_data spmi_vib_gen2_data = { + .hw_type = SPMI_VIB_GEN2, + .enable_addr = SPMI_VIB_EN_REG, + .drv_addr = SPMI_VIB_GEN2_DRV_REG, + .drv2_addr = SPMI_VIB_GEN2_DRV2_REG, +}; + /** * struct pm8xxx_vib - structure to hold vibrator data * @vib_input_dev: input device supporting force feedback @@ -85,12 +101,24 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on) { int rc; unsigned int val = vib->reg_vib_drv; - u32 mask = SPMI_VIB_DRV_LEVEL_MASK; - u32 shift = SPMI_VIB_DRV_SHIFT; + u32 mask, shift; - if (vib->data->hw_type == SSBI_VIB) { + + 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; } if (on) @@ -104,6 +132,19 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on) vib->reg_vib_drv = val; + if (vib->data->hw_type == SPMI_VIB_GEN2) { + mask = SPMI_VIB_GEN2_DRV2_MASK; + shift = SPMI_VIB_GEN2_DRV2_SHIFT; + if (on) + val = (vib->level >> shift) & mask; + else + val = 0; + rc = regmap_update_bits(vib->regmap, + vib->reg_base + vib->data->drv2_addr, mask, val); + if (rc < 0) + return rc; + } + if (vib->data->hw_type == SSBI_VIB) return 0; @@ -128,10 +169,13 @@ static void pm8xxx_work_handler(struct work_struct *work) vib->active = true; vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) + VIB_MIN_LEVEL_mV; - vib->level /= 100; + if (vib->data->hw_type != SPMI_VIB_GEN2) + vib->level /= 100; } else { vib->active = false; - vib->level = VIB_MIN_LEVEL_mV / 100; + vib->level = VIB_MIN_LEVEL_mV; + if (vib->data->hw_type != SPMI_VIB_GEN2) + vib->level /= 100; } pm8xxx_vib_set(vib, vib->active); @@ -266,6 +310,7 @@ static const struct of_device_id pm8xxx_vib_id_table[] = { { .compatible = "qcom,pm8058-vib", .data = &ssbi_vib_data }, { .compatible = "qcom,pm8921-vib", .data = &ssbi_vib_data }, { .compatible = "qcom,pm8916-vib", .data = &spmi_vib_data }, + { .compatible = "qcom,pmi632-vib", .data = &spmi_vib_gen2_data }, { } }; MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);