[v2,2/2] Input: pm8xxx-vib - Add support for more PMICs

  Add support for vibrator module inside PMI632, PM7250B, PM7325B.
It is very similar to vibrator inside PM8xxx but just the drive
amplitude is controlled through 2 bytes registers.

Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
  

On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
> It is very similar to vibrator inside PM8xxx but just the drive
> amplitude is controlled through 2 bytes registers.
>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
>  drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index 04cb87efd799..213fdfd47c7f 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
>         unsigned int drv_addr;
>         unsigned int drv_mask;
>         unsigned int drv_shift;
> +       unsigned int drv_addr2;
> +       unsigned int drv_mask2;
> +       unsigned int drv_shift2;
>         unsigned int drv_en_manual_mask;
>  };
>
> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
>         .drv_en_manual_mask = 0,
>  };
>
> +static struct pm8xxx_regs pmi632_regs = {
> +       .enable_addr = 0x5746,
> +       .enable_mask = BIT(7),
> +       .drv_addr = 0x5740,
> +       .drv_mask = 0xff,
> +       .drv_shift = 0,
> +       .drv_addr2 = 0x5741,
> +       .drv_mask2 = 0x0f,
> +       .drv_shift2 = 8,

I see that you are just expanding what was done for SSBI PMICs and
later expanded to support pm8916. However it might be better to drop
the hardcoded .drv_addr (and drv_addr2) and read address from DT
instead.

> +       .drv_en_manual_mask = 0,
> +};
> +
> +static struct pm8xxx_regs pm7250b_regs = {
> +       .enable_addr = 0x5346,
> +       .enable_mask = BIT(7),
> +       .drv_addr = 0x5340,
> +       .drv_mask = 0xff,
> +       .drv_shift = 0,
> +       .drv_addr2 = 0x5341,
> +       .drv_mask2 = 0x0f,
> +       .drv_shift2 = 8,
> +       .drv_en_manual_mask = 0,
> +};
> +
> +static struct pm8xxx_regs pm7325b_regs = {
> +       .enable_addr = 0xdf46,
> +       .enable_mask = BIT(7),
> +       .drv_addr = 0xdf40,
> +       .drv_mask = 0xff,
> +       .drv_shift = 0,
> +       .drv_addr2 = 0xdf41,
> +       .drv_mask2 = 0x0f,
> +       .drv_shift2 = 8,
> +       .drv_en_manual_mask = 0,
> +};
> +
>  /**
>   * struct pm8xxx_vib - structure to hold vibrator data
>   * @vib_input_dev: input device supporting force feedback
> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>                 return rc;
>
>         vib->reg_vib_drv = val;
> +       if (regs->drv_addr2 != 0 && on) {
> +               val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
> +               rc = regmap_write(vib->regmap, regs->drv_addr2, val);
> +               if (rc < 0)
> +                       return rc;
> +       }
>
>         if (regs->enable_mask)
>                 rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
>         { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>         { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>         { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
> +       { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
> +       { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
> +       { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> --
> 2.25.1
>
  

On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>
>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
>> It is very similar to vibrator inside PM8xxx but just the drive
>> amplitude is controlled through 2 bytes registers.
>>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>> ---
>>   drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>> index 04cb87efd799..213fdfd47c7f 100644
>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
>>          unsigned int drv_addr;
>>          unsigned int drv_mask;
>>          unsigned int drv_shift;
>> +       unsigned int drv_addr2;
>> +       unsigned int drv_mask2;
>> +       unsigned int drv_shift2;
>>          unsigned int drv_en_manual_mask;
>>   };
>>
>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
>>          .drv_en_manual_mask = 0,
>>   };
>>
>> +static struct pm8xxx_regs pmi632_regs = {
>> +       .enable_addr = 0x5746,
>> +       .enable_mask = BIT(7),
>> +       .drv_addr = 0x5740,
>> +       .drv_mask = 0xff,
>> +       .drv_shift = 0,
>> +       .drv_addr2 = 0x5741,
>> +       .drv_mask2 = 0x0f,
>> +       .drv_shift2 = 8,
> 
> I see that you are just expanding what was done for SSBI PMICs and
> later expanded to support pm8916. However it might be better to drop
> the hardcoded .drv_addr (and drv_addr2) and read address from DT
> instead.
> 

Right, this is the simplest change without updating the code logic too 
much. If we decided to read .drv_addr and .drv_add2 from DT, we will 
have to read .enable_addr along with all other mask/shift for each 
register address from DT as well because they are not consistent from 
target to target. I don't know how would you suggest to add the DT 
properties for all of them, but if we end up to add a property for each 
of them, it won't be cleaner than hard-coding them.


>> +       .drv_en_manual_mask = 0,
>> +};
>> +
>> +static struct pm8xxx_regs pm7250b_regs = {
>> +       .enable_addr = 0x5346,
>> +       .enable_mask = BIT(7),
>> +       .drv_addr = 0x5340,
>> +       .drv_mask = 0xff,
>> +       .drv_shift = 0,
>> +       .drv_addr2 = 0x5341,
>> +       .drv_mask2 = 0x0f,
>> +       .drv_shift2 = 8,
>> +       .drv_en_manual_mask = 0,
>> +};
>> +
>> +static struct pm8xxx_regs pm7325b_regs = {
>> +       .enable_addr = 0xdf46,
>> +       .enable_mask = BIT(7),
>> +       .drv_addr = 0xdf40,
>> +       .drv_mask = 0xff,
>> +       .drv_shift = 0,
>> +       .drv_addr2 = 0xdf41,
>> +       .drv_mask2 = 0x0f,
>> +       .drv_shift2 = 8,
>> +       .drv_en_manual_mask = 0,
>> +};
>> +
>>   /**
>>    * struct pm8xxx_vib - structure to hold vibrator data
>>    * @vib_input_dev: input device supporting force feedback
>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>>                  return rc;
>>
>>          vib->reg_vib_drv = val;
>> +       if (regs->drv_addr2 != 0 && on) {
>> +               val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
>> +               rc = regmap_write(vib->regmap, regs->drv_addr2, val);
>> +               if (rc < 0)
>> +                       return rc;
>> +       }
>>
>>          if (regs->enable_mask)
>>                  rc = regmap_update_bits(vib->regmap, regs->enable_addr,
>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
>>          { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>>          { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>>          { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
>> +       { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
>> +       { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
>> +       { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
>>          { }
>>   };
>>   MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
>> --
>> 2.25.1
>>
> 
>
  

On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
>
>
> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
> > On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>
> >> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
> >> It is very similar to vibrator inside PM8xxx but just the drive
> >> amplitude is controlled through 2 bytes registers.
> >>
> >> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> >> ---
> >>   drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
> >>   1 file changed, 48 insertions(+)
> >>
> >> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> >> index 04cb87efd799..213fdfd47c7f 100644
> >> --- a/drivers/input/misc/pm8xxx-vibrator.c
> >> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> >> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
> >>          unsigned int drv_addr;
> >>          unsigned int drv_mask;
> >>          unsigned int drv_shift;
> >> +       unsigned int drv_addr2;
> >> +       unsigned int drv_mask2;
> >> +       unsigned int drv_shift2;
> >>          unsigned int drv_en_manual_mask;
> >>   };
> >>
> >> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
> >>          .drv_en_manual_mask = 0,
> >>   };
> >>
> >> +static struct pm8xxx_regs pmi632_regs = {
> >> +       .enable_addr = 0x5746,
> >> +       .enable_mask = BIT(7),
> >> +       .drv_addr = 0x5740,
> >> +       .drv_mask = 0xff,
> >> +       .drv_shift = 0,
> >> +       .drv_addr2 = 0x5741,
> >> +       .drv_mask2 = 0x0f,
> >> +       .drv_shift2 = 8,
> >
> > I see that you are just expanding what was done for SSBI PMICs and
> > later expanded to support pm8916. However it might be better to drop
> > the hardcoded .drv_addr (and drv_addr2) and read address from DT
> > instead.
> >
>
> Right, this is the simplest change without updating the code logic too
> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
> have to read .enable_addr along with all other mask/shift for each
> register address from DT as well because they are not consistent from
> target to target. I don't know how would you suggest to add the DT
> properties for all of them, but if we end up to add a property for each
> of them, it won't be cleaner than hard-coding them.

No, we (correctly) have device compatibles for that. The issue with
hardcoding register addresses is that it adds extra issues here.

If I understand correctly, we have several 'generation':
- SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
- older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
- new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6

For the last generation you are adding three independent entries,
while the block looks the same. If you remove drv_addr (and get it
from reg property), it would allow us to keep only the functional data
in struct pm8xxxx_regs (masks / shifts).

>
>
> >> +       .drv_en_manual_mask = 0,
> >> +};
> >> +
> >> +static struct pm8xxx_regs pm7250b_regs = {
> >> +       .enable_addr = 0x5346,
> >> +       .enable_mask = BIT(7),
> >> +       .drv_addr = 0x5340,
> >> +       .drv_mask = 0xff,
> >> +       .drv_shift = 0,
> >> +       .drv_addr2 = 0x5341,
> >> +       .drv_mask2 = 0x0f,
> >> +       .drv_shift2 = 8,
> >> +       .drv_en_manual_mask = 0,
> >> +};
> >> +
> >> +static struct pm8xxx_regs pm7325b_regs = {
> >> +       .enable_addr = 0xdf46,
> >> +       .enable_mask = BIT(7),
> >> +       .drv_addr = 0xdf40,
> >> +       .drv_mask = 0xff,
> >> +       .drv_shift = 0,
> >> +       .drv_addr2 = 0xdf41,
> >> +       .drv_mask2 = 0x0f,
> >> +       .drv_shift2 = 8,
> >> +       .drv_en_manual_mask = 0,
> >> +};
> >> +
> >>   /**
> >>    * struct pm8xxx_vib - structure to hold vibrator data
> >>    * @vib_input_dev: input device supporting force feedback
> >> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> >>                  return rc;
> >>
> >>          vib->reg_vib_drv = val;
> >> +       if (regs->drv_addr2 != 0 && on) {
> >> +               val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
> >> +               rc = regmap_write(vib->regmap, regs->drv_addr2, val);
> >> +               if (rc < 0)
> >> +                       return rc;
> >> +       }
> >>
> >>          if (regs->enable_mask)
> >>                  rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> >> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
> >>          { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
> >>          { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
> >>          { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
> >> +       { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
> >> +       { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
> >> +       { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
> >>          { }
> >>   };
> >>   MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> >> --
> >> 2.25.1
> >>
> >
> >
  

On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>
>>
>>
>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>
>>>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
>>>> It is very similar to vibrator inside PM8xxx but just the drive
>>>> amplitude is controlled through 2 bytes registers.
>>>>
>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>> ---
>>>>    drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
>>>>    1 file changed, 48 insertions(+)
>>>>
>>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>>>> index 04cb87efd799..213fdfd47c7f 100644
>>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>>>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
>>>>           unsigned int drv_addr;
>>>>           unsigned int drv_mask;
>>>>           unsigned int drv_shift;
>>>> +       unsigned int drv_addr2;
>>>> +       unsigned int drv_mask2;
>>>> +       unsigned int drv_shift2;
>>>>           unsigned int drv_en_manual_mask;
>>>>    };
>>>>
>>>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
>>>>           .drv_en_manual_mask = 0,
>>>>    };
>>>>
>>>> +static struct pm8xxx_regs pmi632_regs = {
>>>> +       .enable_addr = 0x5746,
>>>> +       .enable_mask = BIT(7),
>>>> +       .drv_addr = 0x5740,
>>>> +       .drv_mask = 0xff,
>>>> +       .drv_shift = 0,
>>>> +       .drv_addr2 = 0x5741,
>>>> +       .drv_mask2 = 0x0f,
>>>> +       .drv_shift2 = 8,
>>>
>>> I see that you are just expanding what was done for SSBI PMICs and
>>> later expanded to support pm8916. However it might be better to drop
>>> the hardcoded .drv_addr (and drv_addr2) and read address from DT
>>> instead.
>>>
>>
>> Right, this is the simplest change without updating the code logic too
>> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
>> have to read .enable_addr along with all other mask/shift for each
>> register address from DT as well because they are not consistent from
>> target to target. I don't know how would you suggest to add the DT
>> properties for all of them, but if we end up to add a property for each
>> of them, it won't be cleaner than hard-coding them.
> 
> No, we (correctly) have device compatibles for that. The issue with
> hardcoding register addresses is that it adds extra issues here.
> 
> If I understand correctly, we have several 'generation':
> - SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
> - older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
> - new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6
> 
> For the last generation you are adding three independent entries,
> while the block looks the same. If you remove drv_addr (and get it
> from reg property), it would allow us to keep only the functional data
> in struct pm8xxxx_regs (masks / shifts).
> 

Okay, let me know if I understood it correctly, this is what you are 
suggesting:

   - hard code the mask/shifts and still keep them in struct pm8xxx_regs,
     combine the drv_mask2 to the upper byte of the drv_mask, so we will
     have following data structure for the 3rd generation vibrator

     static struct pm8xxx_regs pm7250b_regs = {
         .enable_addr = 0x5346,
         .enable_mask = BIT(7),
         .drv_mask = 0xfff,
         .drv_shift = 0,
         .drv_en_manual_mask = 0,
     };


   - move the drv_addr/drv_addr2 into DT, read them from 'reg' property.
     Because of 'mfd/qcom,spmi-pmic.yaml' has defined the 'address-cells'
     as 1 and the 'size-cells' as 0 for qcom spmi devices, we couldn't
     specify the address size to 2 even the drv_addr for the 3rd
     generation vibrator is 2 adjacent bytes. So we will end of having
     following DT scheme:

       For the 2nd generation which only has drv_addr
	vibrator@c041 {
              compatible = "qcom,pm8916-vib";
              reg = <0xc041>;  /* drv_addr */
              ...
	};

       For the 3rd generation which has both drv_addr and drv_addr2
         vibrator@5340 {
              compatible = "qcom,pm7250b-vib";
	     reg = <0x5340>,  /* drv_addr */
		   <0x5341>;  /* drv_addr2 */
	     ...
	};

Not sure how do you feel, I actually don't see too much benefit than 
hard-coding them in the driver.
We will end up having code to check how many u32 value in the 'reg' and 
only assign it to drv_addr2 when the 2nd is available, also when 
programming drv_addr2 register, the driver will always assume the mask 
is in the upper byte of the drv_mask and the shift to the drive level is 
8 (this seems hacky to me and it was my biggest concern while I made 
this change, and it led me to defining drv_shift2/drv_mask2 along with 
drv_addr2).



>>
>>
>>>> +       .drv_en_manual_mask = 0,
>>>> +};
>>>> +
>>>> +static struct pm8xxx_regs pm7250b_regs = {
>>>> +       .enable_addr = 0x5346,
>>>> +       .enable_mask = BIT(7),
>>>> +       .drv_addr = 0x5340,
>>>> +       .drv_mask = 0xff,
>>>> +       .drv_shift = 0,
>>>> +       .drv_addr2 = 0x5341,
>>>> +       .drv_mask2 = 0x0f,
>>>> +       .drv_shift2 = 8,
>>>> +       .drv_en_manual_mask = 0,
>>>> +};
>>>> +
>>>> +static struct pm8xxx_regs pm7325b_regs = {
>>>> +       .enable_addr = 0xdf46,
>>>> +       .enable_mask = BIT(7),
>>>> +       .drv_addr = 0xdf40,
>>>> +       .drv_mask = 0xff,
>>>> +       .drv_shift = 0,
>>>> +       .drv_addr2 = 0xdf41,
>>>> +       .drv_mask2 = 0x0f,
>>>> +       .drv_shift2 = 8,
>>>> +       .drv_en_manual_mask = 0,
>>>> +};
>>>> +
>>>>    /**
>>>>     * struct pm8xxx_vib - structure to hold vibrator data
>>>>     * @vib_input_dev: input device supporting force feedback
>>>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>>>>                   return rc;
>>>>
>>>>           vib->reg_vib_drv = val;
>>>> +       if (regs->drv_addr2 != 0 && on) {
>>>> +               val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
>>>> +               rc = regmap_write(vib->regmap, regs->drv_addr2, val);
>>>> +               if (rc < 0)
>>>> +                       return rc;
>>>> +       }
>>>>
>>>>           if (regs->enable_mask)
>>>>                   rc = regmap_update_bits(vib->regmap, regs->enable_addr,
>>>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
>>>>           { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>>>>           { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>>>>           { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
>>>> +       { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
>>>> +       { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
>>>> +       { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
>>>>           { }
>>>>    };
>>>>    MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
>>>> --
>>>> 2.25.1
>>>>
>>>
>>>
> 
> 
>
  

On Tue, 18 Jul 2023 at 13:55, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
>
>
> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
> > On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>>>
> >>>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
> >>>> It is very similar to vibrator inside PM8xxx but just the drive
> >>>> amplitude is controlled through 2 bytes registers.
> >>>>
> >>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> >>>> ---
> >>>>    drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
> >>>>    1 file changed, 48 insertions(+)
> >>>>
> >>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> >>>> index 04cb87efd799..213fdfd47c7f 100644
> >>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
> >>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> >>>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
> >>>>           unsigned int drv_addr;
> >>>>           unsigned int drv_mask;
> >>>>           unsigned int drv_shift;
> >>>> +       unsigned int drv_addr2;
> >>>> +       unsigned int drv_mask2;
> >>>> +       unsigned int drv_shift2;
> >>>>           unsigned int drv_en_manual_mask;
> >>>>    };
> >>>>
> >>>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
> >>>>           .drv_en_manual_mask = 0,
> >>>>    };
> >>>>
> >>>> +static struct pm8xxx_regs pmi632_regs = {
> >>>> +       .enable_addr = 0x5746,
> >>>> +       .enable_mask = BIT(7),
> >>>> +       .drv_addr = 0x5740,
> >>>> +       .drv_mask = 0xff,
> >>>> +       .drv_shift = 0,
> >>>> +       .drv_addr2 = 0x5741,
> >>>> +       .drv_mask2 = 0x0f,
> >>>> +       .drv_shift2 = 8,
> >>>
> >>> I see that you are just expanding what was done for SSBI PMICs and
> >>> later expanded to support pm8916. However it might be better to drop
> >>> the hardcoded .drv_addr (and drv_addr2) and read address from DT
> >>> instead.
> >>>
> >>
> >> Right, this is the simplest change without updating the code logic too
> >> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
> >> have to read .enable_addr along with all other mask/shift for each
> >> register address from DT as well because they are not consistent from
> >> target to target. I don't know how would you suggest to add the DT
> >> properties for all of them, but if we end up to add a property for each
> >> of them, it won't be cleaner than hard-coding them.
> >
> > No, we (correctly) have device compatibles for that. The issue with
> > hardcoding register addresses is that it adds extra issues here.
> >
> > If I understand correctly, we have several 'generation':
> > - SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
> > - older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
> > - new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6
> >
> > For the last generation you are adding three independent entries,
> > while the block looks the same. If you remove drv_addr (and get it
> > from reg property), it would allow us to keep only the functional data
> > in struct pm8xxxx_regs (masks / shifts).
> >
>
> Okay, let me know if I understood it correctly, this is what you are
> suggesting:
>
>    - hard code the mask/shifts and still keep them in struct pm8xxx_regs,
>      combine the drv_mask2 to the upper byte of the drv_mask, so we will
>      have following data structure for the 3rd generation vibrator
>
>      static struct pm8xxx_regs pm7250b_regs = {
>          .enable_addr = 0x5346,
>          .enable_mask = BIT(7),
>          .drv_mask = 0xfff,
>          .drv_shift = 0,
>          .drv_en_manual_mask = 0,
>      };
>
>
>    - move the drv_addr/drv_addr2 into DT, read them from 'reg' property.
>      Because of 'mfd/qcom,spmi-pmic.yaml' has defined the 'address-cells'
>      as 1 and the 'size-cells' as 0 for qcom spmi devices, we couldn't
>      specify the address size to 2 even the drv_addr for the 3rd
>      generation vibrator is 2 adjacent bytes. So we will end of having
>      following DT scheme:
>
>        For the 2nd generation which only has drv_addr
>         vibrator@c041 {
>               compatible = "qcom,pm8916-vib";
>               reg = <0xc041>;  /* drv_addr */

No. This is <0xc000>.

>               ...
>         };
>
>        For the 3rd generation which has both drv_addr and drv_addr2
>          vibrator@5340 {
>               compatible = "qcom,pm7250b-vib";
>              reg = <0x5340>,  /* drv_addr */
>                    <0x5341>;  /* drv_addr2 */
>              ...
>         };
>
> Not sure how do you feel, I actually don't see too much benefit than
> hard-coding them in the driver.
> We will end up having code to check how many u32 value in the 'reg' and
> only assign it to drv_addr2 when the 2nd is available, also when
> programming drv_addr2 register, the driver will always assume the mask
> is in the upper byte of the drv_mask and the shift to the drive level is
> 8 (this seems hacky to me and it was my biggest concern while I made
> this change, and it led me to defining drv_shift2/drv_mask2 along with
> drv_addr2).

We only need drv_addr2 if drv_mask has more than 8 bits. So you don't
have to specify it in the DT. It is always equal to base_reg + 0x41.
The same way drv_addr is always equal to base_reg + 0x40 for all
SPMI-based PMIC vibrator devices.

>
>
>
> >>
> >>
> >>>> +       .drv_en_manual_mask = 0,
> >>>> +};
> >>>> +
> >>>> +static struct pm8xxx_regs pm7250b_regs = {
> >>>> +       .enable_addr = 0x5346,
> >>>> +       .enable_mask = BIT(7),
> >>>> +       .drv_addr = 0x5340,
> >>>> +       .drv_mask = 0xff,
> >>>> +       .drv_shift = 0,
> >>>> +       .drv_addr2 = 0x5341,
> >>>> +       .drv_mask2 = 0x0f,
> >>>> +       .drv_shift2 = 8,
> >>>> +       .drv_en_manual_mask = 0,
> >>>> +};
> >>>> +
> >>>> +static struct pm8xxx_regs pm7325b_regs = {
> >>>> +       .enable_addr = 0xdf46,
> >>>> +       .enable_mask = BIT(7),
> >>>> +       .drv_addr = 0xdf40,
> >>>> +       .drv_mask = 0xff,
> >>>> +       .drv_shift = 0,
> >>>> +       .drv_addr2 = 0xdf41,
> >>>> +       .drv_mask2 = 0x0f,
> >>>> +       .drv_shift2 = 8,
> >>>> +       .drv_en_manual_mask = 0,
> >>>> +};
> >>>> +
> >>>>    /**
> >>>>     * struct pm8xxx_vib - structure to hold vibrator data
> >>>>     * @vib_input_dev: input device supporting force feedback
> >>>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> >>>>                   return rc;
> >>>>
> >>>>           vib->reg_vib_drv = val;
> >>>> +       if (regs->drv_addr2 != 0 && on) {
> >>>> +               val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
> >>>> +               rc = regmap_write(vib->regmap, regs->drv_addr2, val);
> >>>> +               if (rc < 0)
> >>>> +                       return rc;
> >>>> +       }
> >>>>
> >>>>           if (regs->enable_mask)
> >>>>                   rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> >>>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
> >>>>           { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
> >>>>           { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
> >>>>           { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
> >>>> +       { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
> >>>> +       { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
> >>>> +       { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
> >>>>           { }
> >>>>    };
> >>>>    MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> >>>> --
> >>>> 2.25.1
> >>>>
> >>>
> >>>
> >
> >
> >
  

On 7/18/2023 7:04 PM, Dmitry Baryshkov wrote:
> On Tue, 18 Jul 2023 at 13:55, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>
>>
>>
>> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
>>> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>>>
>>>>>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
>>>>>> It is very similar to vibrator inside PM8xxx but just the drive
>>>>>> amplitude is controlled through 2 bytes registers.
>>>>>>
>>>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>>>> ---
>>>>>>     drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
>>>>>>     1 file changed, 48 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>>>>>> index 04cb87efd799..213fdfd47c7f 100644
>>>>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>>>>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>>>>>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
>>>>>>            unsigned int drv_addr;
>>>>>>            unsigned int drv_mask;
>>>>>>            unsigned int drv_shift;
>>>>>> +       unsigned int drv_addr2;
>>>>>> +       unsigned int drv_mask2;
>>>>>> +       unsigned int drv_shift2;
>>>>>>            unsigned int drv_en_manual_mask;
>>>>>>     };
>>>>>>
>>>>>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
>>>>>>            .drv_en_manual_mask = 0,
>>>>>>     };
>>>>>>
>>>>>> +static struct pm8xxx_regs pmi632_regs = {
>>>>>> +       .enable_addr = 0x5746,
>>>>>> +       .enable_mask = BIT(7),
>>>>>> +       .drv_addr = 0x5740,
>>>>>> +       .drv_mask = 0xff,
>>>>>> +       .drv_shift = 0,
>>>>>> +       .drv_addr2 = 0x5741,
>>>>>> +       .drv_mask2 = 0x0f,
>>>>>> +       .drv_shift2 = 8,
>>>>>
>>>>> I see that you are just expanding what was done for SSBI PMICs and
>>>>> later expanded to support pm8916. However it might be better to drop
>>>>> the hardcoded .drv_addr (and drv_addr2) and read address from DT
>>>>> instead.
>>>>>
>>>>
>>>> Right, this is the simplest change without updating the code logic too
>>>> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
>>>> have to read .enable_addr along with all other mask/shift for each
>>>> register address from DT as well because they are not consistent from
>>>> target to target. I don't know how would you suggest to add the DT
>>>> properties for all of them, but if we end up to add a property for each
>>>> of them, it won't be cleaner than hard-coding them.
>>>
>>> No, we (correctly) have device compatibles for that. The issue with
>>> hardcoding register addresses is that it adds extra issues here.
>>>
>>> If I understand correctly, we have several 'generation':
>>> - SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
>>> - older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
>>> - new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6
>>>
>>> For the last generation you are adding three independent entries,
>>> while the block looks the same. If you remove drv_addr (and get it
>>> from reg property), it would allow us to keep only the functional data
>>> in struct pm8xxxx_regs (masks / shifts).
>>>
>>
>> Okay, let me know if I understood it correctly, this is what you are
>> suggesting:
>>
>>     - hard code the mask/shifts and still keep them in struct pm8xxx_regs,
>>       combine the drv_mask2 to the upper byte of the drv_mask, so we will
>>       have following data structure for the 3rd generation vibrator
>>
>>       static struct pm8xxx_regs pm7250b_regs = {
>>           .enable_addr = 0x5346,
>>           .enable_mask = BIT(7),
>>           .drv_mask = 0xfff,
>>           .drv_shift = 0,
>>           .drv_en_manual_mask = 0,
>>       };
>>
>>
>>     - move the drv_addr/drv_addr2 into DT, read them from 'reg' property.
>>       Because of 'mfd/qcom,spmi-pmic.yaml' has defined the 'address-cells'
>>       as 1 and the 'size-cells' as 0 for qcom spmi devices, we couldn't
>>       specify the address size to 2 even the drv_addr for the 3rd
>>       generation vibrator is 2 adjacent bytes. So we will end of having
>>       following DT scheme:
>>
>>         For the 2nd generation which only has drv_addr
>>          vibrator@c041 {
>>                compatible = "qcom,pm8916-vib";
>>                reg = <0xc041>;  /* drv_addr */
> 
> No. This is <0xc000>.
> 
>>                ...
>>          };
>>
>>         For the 3rd generation which has both drv_addr and drv_addr2
>>           vibrator@5340 {
>>                compatible = "qcom,pm7250b-vib";
>>               reg = <0x5340>,  /* drv_addr */
>>                     <0x5341>;  /* drv_addr2 */
>>               ...
>>          };
>>
>> Not sure how do you feel, I actually don't see too much benefit than
>> hard-coding them in the driver.
>> We will end up having code to check how many u32 value in the 'reg' and
>> only assign it to drv_addr2 when the 2nd is available, also when
>> programming drv_addr2 register, the driver will always assume the mask
>> is in the upper byte of the drv_mask and the shift to the drive level is
>> 8 (this seems hacky to me and it was my biggest concern while I made
>> this change, and it led me to defining drv_shift2/drv_mask2 along with
>> drv_addr2).
> 
> We only need drv_addr2 if drv_mask has more than 8 bits. So you don't
> have to specify it in the DT. It is always equal to base_reg + 0x41.
> The same way drv_addr is always equal to base_reg + 0x40 for all
> SPMI-based PMIC vibrator devices.
> 

Thanks. I got it now, I agree this will be beneficial for the case that 
different PMICs have the same vibrator module but with different 
register base address. I am going to change it to this way, let me know 
if this is what you thought:

@@ -25,6 +29,9 @@ struct pm8xxx_regs {
         unsigned int drv_addr;
         unsigned int drv_mask;
         unsigned int drv_shift;
+       unsigned int drv_addr2;
+       unsigned int drv_mask2;
+       unsigned int drv_shift2;
         unsigned int drv_en_manual_mask;
  };

+static struct pm8xxx_regs spmi_vib_regs = {
+       .enable_mask = BIT(7),
+       .drv_mask = 0xff,
+       .drv_shift = 0,
+       .drv_mask2 = 0xf,
+       .drv_shift2 = 8,
+       .drv_en_manual_mask = 0,
+};
+

+#define SPMI_VIB_VSET_LB_REG   0x40
+#define SPMI_VIB_VSET_UP_REG   0x41
+#define SPMI_VIB_EN_CTL_REG    0x46
+

         regs = of_device_get_match_data(&pdev->dev);

+       if (regs->drv_addr == 0) {
+               rc = fwnode_property_read_u32(pdev->dev.fwnode,
+                               "reg", &reg_base);
+               if (rc < 0)
+                       return rc;
+
+               regs->enable_addr = reg_base + SPMI_VIB_EN_CTL_REG;
+               regs->drv_addr = reg_base + SPMI_VIB_VSET_LB_REG;
+               regs->drv_addr2 = reg_base + SPMI_VIB_VSET_UP_REG;
+       }
+


@@ -242,6 +277,7 @@ static const struct of_device_id 
pm8xxx_vib_id_table[] = {
         { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
         { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
         { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
+       ( .compabitle = "qcom,spmi-vib", .data = &spmi_vib_regs },
         { }


>>
>>
>>
>>>>
>>>>
>>>>>> +       .drv_en_manual_mask = 0,
>>>>>> +};
>>>>>> +
>>>>>> +static struct pm8xxx_regs pm7250b_regs = {
>>>>>> +       .enable_addr = 0x5346,
>>>>>> +       .enable_mask = BIT(7),
>>>>>> +       .drv_addr = 0x5340,
>>>>>> +       .drv_mask = 0xff,
>>>>>> +       .drv_shift = 0,
>>>>>> +       .drv_addr2 = 0x5341,
>>>>>> +       .drv_mask2 = 0x0f,
>>>>>> +       .drv_shift2 = 8,
>>>>>> +       .drv_en_manual_mask = 0,
>>>>>> +};
>>>>>> +
>>>>>> +static struct pm8xxx_regs pm7325b_regs = {
>>>>>> +       .enable_addr = 0xdf46,
>>>>>> +       .enable_mask = BIT(7),
>>>>>> +       .drv_addr = 0xdf40,
>>>>>> +       .drv_mask = 0xff,
>>>>>> +       .drv_shift = 0,
>>>>>> +       .drv_addr2 = 0xdf41,
>>>>>> +       .drv_mask2 = 0x0f,
>>>>>> +       .drv_shift2 = 8,
>>>>>> +       .drv_en_manual_mask = 0,
>>>>>> +};
>>>>>> +
>>>>>>     /**
>>>>>>      * struct pm8xxx_vib - structure to hold vibrator data
>>>>>>      * @vib_input_dev: input device supporting force feedback
>>>>>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>>>>>>                    return rc;
>>>>>>
>>>>>>            vib->reg_vib_drv = val;
>>>>>> +       if (regs->drv_addr2 != 0 && on) {
>>>>>> +               val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
>>>>>> +               rc = regmap_write(vib->regmap, regs->drv_addr2, val);
>>>>>> +               if (rc < 0)
>>>>>> +                       return rc;
>>>>>> +       }
>>>>>>
>>>>>>            if (regs->enable_mask)
>>>>>>                    rc = regmap_update_bits(vib->regmap, regs->enable_addr,
>>>>>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
>>>>>>            { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>>>>>>            { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>>>>>>            { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
>>>>>> +       { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
>>>>>> +       { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
>>>>>> +       { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
>>>>>>            { }
>>>>>>     };
>>>>>>     MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
>
  

On Wed, 19 Jul 2023 at 07:09, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
>
>
> On 7/18/2023 7:04 PM, Dmitry Baryshkov wrote:
> > On Tue, 18 Jul 2023 at 13:55, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
> >>>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>>>>>
> >>>>>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
> >>>>>> It is very similar to vibrator inside PM8xxx but just the drive
> >>>>>> amplitude is controlled through 2 bytes registers.
> >>>>>>
> >>>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> >>>>>> ---
> >>>>>>     drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
> >>>>>>     1 file changed, 48 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> >>>>>> index 04cb87efd799..213fdfd47c7f 100644
> >>>>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
> >>>>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> >>>>>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
> >>>>>>            unsigned int drv_addr;
> >>>>>>            unsigned int drv_mask;
> >>>>>>            unsigned int drv_shift;
> >>>>>> +       unsigned int drv_addr2;

Unused

> >>>>>> +       unsigned int drv_mask2;
> >>>>>> +       unsigned int drv_shift2;
> >>>>>>            unsigned int drv_en_manual_mask;
> >>>>>>     };
> >>>>>>
> >>>>>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
> >>>>>>            .drv_en_manual_mask = 0,
> >>>>>>     };
> >>>>>>
> >>>>>> +static struct pm8xxx_regs pmi632_regs = {
> >>>>>> +       .enable_addr = 0x5746,
> >>>>>> +       .enable_mask = BIT(7),
> >>>>>> +       .drv_addr = 0x5740,
> >>>>>> +       .drv_mask = 0xff,
> >>>>>> +       .drv_shift = 0,
> >>>>>> +       .drv_addr2 = 0x5741,
> >>>>>> +       .drv_mask2 = 0x0f,
> >>>>>> +       .drv_shift2 = 8,
> >>>>>
> >>>>> I see that you are just expanding what was done for SSBI PMICs and
> >>>>> later expanded to support pm8916. However it might be better to drop
> >>>>> the hardcoded .drv_addr (and drv_addr2) and read address from DT
> >>>>> instead.
> >>>>>
> >>>>
> >>>> Right, this is the simplest change without updating the code logic too
> >>>> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
> >>>> have to read .enable_addr along with all other mask/shift for each
> >>>> register address from DT as well because they are not consistent from
> >>>> target to target. I don't know how would you suggest to add the DT
> >>>> properties for all of them, but if we end up to add a property for each
> >>>> of them, it won't be cleaner than hard-coding them.
> >>>
> >>> No, we (correctly) have device compatibles for that. The issue with
> >>> hardcoding register addresses is that it adds extra issues here.
> >>>
> >>> If I understand correctly, we have several 'generation':
> >>> - SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
> >>> - older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
> >>> - new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6
> >>>
> >>> For the last generation you are adding three independent entries,
> >>> while the block looks the same. If you remove drv_addr (and get it
> >>> from reg property), it would allow us to keep only the functional data
> >>> in struct pm8xxxx_regs (masks / shifts).
> >>>
> >>
> >> Okay, let me know if I understood it correctly, this is what you are
> >> suggesting:
> >>
> >>     - hard code the mask/shifts and still keep them in struct pm8xxx_regs,
> >>       combine the drv_mask2 to the upper byte of the drv_mask, so we will
> >>       have following data structure for the 3rd generation vibrator
> >>
> >>       static struct pm8xxx_regs pm7250b_regs = {
> >>           .enable_addr = 0x5346,
> >>           .enable_mask = BIT(7),
> >>           .drv_mask = 0xfff,
> >>           .drv_shift = 0,
> >>           .drv_en_manual_mask = 0,
> >>       };
> >>
> >>
> >>     - move the drv_addr/drv_addr2 into DT, read them from 'reg' property.
> >>       Because of 'mfd/qcom,spmi-pmic.yaml' has defined the 'address-cells'
> >>       as 1 and the 'size-cells' as 0 for qcom spmi devices, we couldn't
> >>       specify the address size to 2 even the drv_addr for the 3rd
> >>       generation vibrator is 2 adjacent bytes. So we will end of having
> >>       following DT scheme:
> >>
> >>         For the 2nd generation which only has drv_addr
> >>          vibrator@c041 {
> >>                compatible = "qcom,pm8916-vib";
> >>                reg = <0xc041>;  /* drv_addr */
> >
> > No. This is <0xc000>.
> >
> >>                ...
> >>          };
> >>
> >>         For the 3rd generation which has both drv_addr and drv_addr2
> >>           vibrator@5340 {
> >>                compatible = "qcom,pm7250b-vib";
> >>               reg = <0x5340>,  /* drv_addr */
> >>                     <0x5341>;  /* drv_addr2 */
> >>               ...
> >>          };
> >>
> >> Not sure how do you feel, I actually don't see too much benefit than
> >> hard-coding them in the driver.
> >> We will end up having code to check how many u32 value in the 'reg' and
> >> only assign it to drv_addr2 when the 2nd is available, also when
> >> programming drv_addr2 register, the driver will always assume the mask
> >> is in the upper byte of the drv_mask and the shift to the drive level is
> >> 8 (this seems hacky to me and it was my biggest concern while I made
> >> this change, and it led me to defining drv_shift2/drv_mask2 along with
> >> drv_addr2).
> >
> > We only need drv_addr2 if drv_mask has more than 8 bits. So you don't
> > have to specify it in the DT. It is always equal to base_reg + 0x41.
> > The same way drv_addr is always equal to base_reg + 0x40 for all
> > SPMI-based PMIC vibrator devices.
> >
>
> Thanks. I got it now, I agree this will be beneficial for the case that
> different PMICs have the same vibrator module but with different
> register base address. I am going to change it to this way, let me know
> if this is what you thought:
>
> @@ -25,6 +29,9 @@ struct pm8xxx_regs {
>          unsigned int drv_addr;
>          unsigned int drv_mask;
>          unsigned int drv_shift;
> +       unsigned int drv_addr2;
> +       unsigned int drv_mask2;
> +       unsigned int drv_shift2;
>          unsigned int drv_en_manual_mask;
>   };
>
> +static struct pm8xxx_regs spmi_vib_regs = {
> +       .enable_mask = BIT(7),
> +       .drv_mask = 0xff,
> +       .drv_shift = 0,
> +       .drv_mask2 = 0xf,
> +       .drv_shift2 = 8,
> +       .drv_en_manual_mask = 0,
> +};

Ideally the static data should be const. I'd suggest moving
drv_addr/drv_addr2 to struct pm8xxx_vib.

> +
>
> +#define SPMI_VIB_VSET_LB_REG   0x40
> +#define SPMI_VIB_VSET_UP_REG   0x41
> +#define SPMI_VIB_EN_CTL_REG    0x46
> +
>
>          regs = of_device_get_match_data(&pdev->dev);
>
> +       if (regs->drv_addr == 0) {
> +               rc = fwnode_property_read_u32(pdev->dev.fwnode,
> +                               "reg", &reg_base);
> +               if (rc < 0)
> +                       return rc;
> +
> +               regs->enable_addr = reg_base + SPMI_VIB_EN_CTL_REG;
> +               regs->drv_addr = reg_base + SPMI_VIB_VSET_LB_REG;
> +               regs->drv_addr2 = reg_base + SPMI_VIB_VSET_UP_REG;

Yes, this looks good (except s/regs->/vib->/). Moreover this also
applies to pm8916. I'd suggest splitting this into two patches: first,
refactor pm8916 support to use reg, then add support for new devices.

> +       }
> +
>
>
> @@ -242,6 +277,7 @@ static const struct of_device_id
> pm8xxx_vib_id_table[] = {
>          { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>          { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>          { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
> +       ( .compabitle = "qcom,spmi-vib", .data = &spmi_vib_regs },
>          { }
>
>
> >>
> >>
> >>
> >>>>
> >>>>
> >>>>>> +       .drv_en_manual_mask = 0,
> >>>>>> +};
> >>>>>> +
> >>>>>> +static struct pm8xxx_regs pm7250b_regs = {
> >>>>>> +       .enable_addr = 0x5346,
> >>>>>> +       .enable_mask = BIT(7),
> >>>>>> +       .drv_addr = 0x5340,
> >>>>>> +       .drv_mask = 0xff,
> >>>>>> +       .drv_shift = 0,
> >>>>>> +       .drv_addr2 = 0x5341,
> >>>>>> +       .drv_mask2 = 0x0f,
> >>>>>> +       .drv_shift2 = 8,
> >>>>>> +       .drv_en_manual_mask = 0,
> >>>>>> +};
> >>>>>> +
> >>>>>> +static struct pm8xxx_regs pm7325b_regs = {
> >>>>>> +       .enable_addr = 0xdf46,
> >>>>>> +       .enable_mask = BIT(7),
> >>>>>> +       .drv_addr = 0xdf40,
> >>>>>> +       .drv_mask = 0xff,
> >>>>>> +       .drv_shift = 0,
> >>>>>> +       .drv_addr2 = 0xdf41,
> >>>>>> +       .drv_mask2 = 0x0f,
> >>>>>> +       .drv_shift2 = 8,
> >>>>>> +       .drv_en_manual_mask = 0,
> >>>>>> +};
> >>>>>> +
> >>>>>>     /**
> >>>>>>      * struct pm8xxx_vib - structure to hold vibrator data
> >>>>>>      * @vib_input_dev: input device supporting force feedback
> >>>>>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> >>>>>>                    return rc;
> >>>>>>
> >>>>>>            vib->reg_vib_drv = val;
> >>>>>> +       if (regs->drv_addr2 != 0 && on) {
> >>>>>> +               val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
> >>>>>> +               rc = regmap_write(vib->regmap, regs->drv_addr2, val);
> >>>>>> +               if (rc < 0)
> >>>>>> +                       return rc;
> >>>>>> +       }
> >>>>>>
> >>>>>>            if (regs->enable_mask)
> >>>>>>                    rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> >>>>>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
> >>>>>>            { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
> >>>>>>            { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
> >>>>>>            { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
> >>>>>> +       { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
> >>>>>> +       { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
> >>>>>> +       { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
> >>>>>>            { }
> >>>>>>     };
> >>>>>>     MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> >>>>>> --
> >>>>>> 2.25.1
> >>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >
  

On 7/19/2023 4:02 PM, Dmitry Baryshkov wrote:
> On Wed, 19 Jul 2023 at 07:09, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>
>>
>>
>> On 7/18/2023 7:04 PM, Dmitry Baryshkov wrote:
>>> On Tue, 18 Jul 2023 at 13:55, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
>>>>>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>>>>>
>>>>>>>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
>>>>>>>> It is very similar to vibrator inside PM8xxx but just the drive
>>>>>>>> amplitude is controlled through 2 bytes registers.
>>>>>>>>
>>>>>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>>>>>> ---
>>>>>>>>      drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 48 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>>>>>>>> index 04cb87efd799..213fdfd47c7f 100644
>>>>>>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>>>>>>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>>>>>>>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
>>>>>>>>             unsigned int drv_addr;
>>>>>>>>             unsigned int drv_mask;
>>>>>>>>             unsigned int drv_shift;
>>>>>>>> +       unsigned int drv_addr2;
> 
> Unused
> 
>>>>>>>> +       unsigned int drv_mask2;
>>>>>>>> +       unsigned int drv_shift2;
>>>>>>>>             unsigned int drv_en_manual_mask;
>>>>>>>>      };
>>>>>>>>
>>>>>>>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
>>>>>>>>             .drv_en_manual_mask = 0,
>>>>>>>>      };
>>>>>>>>
>>>>>>>> +static struct pm8xxx_regs pmi632_regs = {
>>>>>>>> +       .enable_addr = 0x5746,
>>>>>>>> +       .enable_mask = BIT(7),
>>>>>>>> +       .drv_addr = 0x5740,
>>>>>>>> +       .drv_mask = 0xff,
>>>>>>>> +       .drv_shift = 0,
>>>>>>>> +       .drv_addr2 = 0x5741,
>>>>>>>> +       .drv_mask2 = 0x0f,
>>>>>>>> +       .drv_shift2 = 8,
>>>>>>>
>>>>>>> I see that you are just expanding what was done for SSBI PMICs and
>>>>>>> later expanded to support pm8916. However it might be better to drop
>>>>>>> the hardcoded .drv_addr (and drv_addr2) and read address from DT
>>>>>>> instead.
>>>>>>>
>>>>>>
>>>>>> Right, this is the simplest change without updating the code logic too
>>>>>> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
>>>>>> have to read .enable_addr along with all other mask/shift for each
>>>>>> register address from DT as well because they are not consistent from
>>>>>> target to target. I don't know how would you suggest to add the DT
>>>>>> properties for all of them, but if we end up to add a property for each
>>>>>> of them, it won't be cleaner than hard-coding them.
>>>>>
>>>>> No, we (correctly) have device compatibles for that. The issue with
>>>>> hardcoding register addresses is that it adds extra issues here.
>>>>>
>>>>> If I understand correctly, we have several 'generation':
>>>>> - SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
>>>>> - older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
>>>>> - new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6
>>>>>
>>>>> For the last generation you are adding three independent entries,
>>>>> while the block looks the same. If you remove drv_addr (and get it
>>>>> from reg property), it would allow us to keep only the functional data
>>>>> in struct pm8xxxx_regs (masks / shifts).
>>>>>
>>>>
>>>> Okay, let me know if I understood it correctly, this is what you are
>>>> suggesting:
>>>>
>>>>      - hard code the mask/shifts and still keep them in struct pm8xxx_regs,
>>>>        combine the drv_mask2 to the upper byte of the drv_mask, so we will
>>>>        have following data structure for the 3rd generation vibrator
>>>>
>>>>        static struct pm8xxx_regs pm7250b_regs = {
>>>>            .enable_addr = 0x5346,
>>>>            .enable_mask = BIT(7),
>>>>            .drv_mask = 0xfff,
>>>>            .drv_shift = 0,
>>>>            .drv_en_manual_mask = 0,
>>>>        };
>>>>
>>>>
>>>>      - move the drv_addr/drv_addr2 into DT, read them from 'reg' property.
>>>>        Because of 'mfd/qcom,spmi-pmic.yaml' has defined the 'address-cells'
>>>>        as 1 and the 'size-cells' as 0 for qcom spmi devices, we couldn't
>>>>        specify the address size to 2 even the drv_addr for the 3rd
>>>>        generation vibrator is 2 adjacent bytes. So we will end of having
>>>>        following DT scheme:
>>>>
>>>>          For the 2nd generation which only has drv_addr
>>>>           vibrator@c041 {
>>>>                 compatible = "qcom,pm8916-vib";
>>>>                 reg = <0xc041>;  /* drv_addr */
>>>
>>> No. This is <0xc000>.
>>>
>>>>                 ...
>>>>           };
>>>>
>>>>          For the 3rd generation which has both drv_addr and drv_addr2
>>>>            vibrator@5340 {
>>>>                 compatible = "qcom,pm7250b-vib";
>>>>                reg = <0x5340>,  /* drv_addr */
>>>>                      <0x5341>;  /* drv_addr2 */
>>>>                ...
>>>>           };
>>>>
>>>> Not sure how do you feel, I actually don't see too much benefit than
>>>> hard-coding them in the driver.
>>>> We will end up having code to check how many u32 value in the 'reg' and
>>>> only assign it to drv_addr2 when the 2nd is available, also when
>>>> programming drv_addr2 register, the driver will always assume the mask
>>>> is in the upper byte of the drv_mask and the shift to the drive level is
>>>> 8 (this seems hacky to me and it was my biggest concern while I made
>>>> this change, and it led me to defining drv_shift2/drv_mask2 along with
>>>> drv_addr2).
>>>
>>> We only need drv_addr2 if drv_mask has more than 8 bits. So you don't
>>> have to specify it in the DT. It is always equal to base_reg + 0x41.
>>> The same way drv_addr is always equal to base_reg + 0x40 for all
>>> SPMI-based PMIC vibrator devices.
>>>
>>
>> Thanks. I got it now, I agree this will be beneficial for the case that
>> different PMICs have the same vibrator module but with different
>> register base address. I am going to change it to this way, let me know
>> if this is what you thought:
>>
>> @@ -25,6 +29,9 @@ struct pm8xxx_regs {
>>           unsigned int drv_addr;
>>           unsigned int drv_mask;
>>           unsigned int drv_shift;
>> +       unsigned int drv_addr2;
>> +       unsigned int drv_mask2;
>> +       unsigned int drv_shift2;
>>           unsigned int drv_en_manual_mask;
>>    };
>>
>> +static struct pm8xxx_regs spmi_vib_regs = {
>> +       .enable_mask = BIT(7),
>> +       .drv_mask = 0xff,
>> +       .drv_shift = 0,
>> +       .drv_mask2 = 0xf,
>> +       .drv_shift2 = 8,
>> +       .drv_en_manual_mask = 0,
>> +};
> 
> Ideally the static data should be const. I'd suggest moving
> drv_addr/drv_addr2 to struct pm8xxx_vib.
> 
>> +
>>
>> +#define SPMI_VIB_VSET_LB_REG   0x40
>> +#define SPMI_VIB_VSET_UP_REG   0x41
>> +#define SPMI_VIB_EN_CTL_REG    0x46
>> +
>>
>>           regs = of_device_get_match_data(&pdev->dev);
>>
>> +       if (regs->drv_addr == 0) {
>> +               rc = fwnode_property_read_u32(pdev->dev.fwnode,
>> +                               "reg", &reg_base);
>> +               if (rc < 0)
>> +                       return rc;
>> +
>> +               regs->enable_addr = reg_base + SPMI_VIB_EN_CTL_REG;
>> +               regs->drv_addr = reg_base + SPMI_VIB_VSET_LB_REG;
>> +               regs->drv_addr2 = reg_base + SPMI_VIB_VSET_UP_REG;
> 
> Yes, this looks good (except s/regs->/vib->/). Moreover this also
> applies to pm8916. I'd suggest splitting this into two patches: first,
> refactor pm8916 support to use reg, then add support for new devices.

Thanks. I will refactor this, test it, and send it out. The only problem 
  is I don't have a pm8916 device with me, but I guess the change should 
be straightforward and I will rely on the test result on my PM7550BA 
device which has the vibrator with the latest generation.

> 
>> +       }
>> +
>>
>>
>> @@ -242,6 +277,7 @@ static const struct of_device_id
>> pm8xxx_vib_id_table[] = {
>>           { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>>           { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>>           { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
>> +       ( .compabitle = "qcom,spmi-vib", .data = &spmi_vib_regs },
>>           { }
>>
>>
>>>>
>>>>
>>>>
>>>>>>
>>>>>>
>>>>>>>> +       .drv_en_manual_mask = 0,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static struct pm8xxx_regs pm7250b_regs = {
>>>>>>>> +       .enable_addr = 0x5346,
>>>>>>>> +       .enable_mask = BIT(7),
>>>>>>>> +       .drv_addr = 0x5340,
>>>>>>>> +       .drv_mask = 0xff,
>>>>>>>> +       .drv_shift = 0,
>>>>>>>> +       .drv_addr2 = 0x5341,
>>>>>>>> +       .drv_mask2 = 0x0f,
>>>>>>>> +       .drv_shift2 = 8,
>>>>>>>> +       .drv_en_manual_mask = 0,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static struct pm8xxx_regs pm7325b_regs = {
>>>>>>>> +       .enable_addr = 0xdf46,
>>>>>>>> +       .enable_mask = BIT(7),
>>>>>>>> +       .drv_addr = 0xdf40,
>>>>>>>> +       .drv_mask = 0xff,
>>>>>>>> +       .drv_shift = 0,
>>>>>>>> +       .drv_addr2 = 0xdf41,
>>>>>>>> +       .drv_mask2 = 0x0f,
>>>>>>>> +       .drv_shift2 = 8,
>>>>>>>> +       .drv_en_manual_mask = 0,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>      /**
>>>>>>>>       * struct pm8xxx_vib - structure to hold vibrator data
>>>>>>>>       * @vib_input_dev: input device supporting force feedback
>>>>>>>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>>>>>>>>                     return rc;
>>>>>>>>
>>>>>>>>             vib->reg_vib_drv = val;
>>>>>>>> +       if (regs->drv_addr2 != 0 && on) {
>>>>>>>> +               val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
>>>>>>>> +               rc = regmap_write(vib->regmap, regs->drv_addr2, val);
>>>>>>>> +               if (rc < 0)
>>>>>>>> +                       return rc;
>>>>>>>> +       }
>>>>>>>>
>>>>>>>>             if (regs->enable_mask)
>>>>>>>>                     rc = regmap_update_bits(vib->regmap, regs->enable_addr,
>>>>>>>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
>>>>>>>>             { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>>>>>>>>             { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>>>>>>>>             { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
>>>>>>>> +       { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
>>>>>>>> +       { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
>>>>>>>> +       { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
>>>>>>>>             { }
>>>>>>>>      };
>>>>>>>>      MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
>>>>>>>> --
>>>>>>>> 2.25.1
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
>