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.
