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 <[email protected]>
---
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,
+ .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 <[email protected]> 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 <[email protected]>
>> ---
>> 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:27, Fenglin Wu <[email protected]> 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 <[email protected]>
> ---
> 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
>
--
With best wishes
Dmitry
On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <[email protected]> wrote:
>
>
>
> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
> > On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
> >>
> >
> >
--
With best wishes
Dmitry
On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <[email protected]> wrote:
>>
>>
>>
>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]> wrote:
>
>
>
> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
> > On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <[email protected]> wrote:
> >>
> >>
> >>
> >> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> 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
> >>>>
> >>>
> >>>
> >
> >
> >
--
With best wishes
Dmitry
On 7/18/2023 7:04 PM, Dmitry Baryshkov wrote:
> On Tue, 18 Jul 2023 at 13:55, Fenglin Wu <[email protected]> wrote:
>>
>>
>>
>> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
>>> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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", ®_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 <[email protected]> wrote:
>
>
>
> On 7/18/2023 7:04 PM, Dmitry Baryshkov wrote:
> > On Tue, 18 Jul 2023 at 13:55, Fenglin Wu <[email protected]> wrote:
> >>
> >>
> >>
> >> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <[email protected]> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
> >>>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <[email protected]> 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 <[email protected]>
> >>>>>> ---
> >>>>>> 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", ®_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
> >>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >
--
With best wishes
Dmitry
On 7/19/2023 4:02 PM, Dmitry Baryshkov wrote:
> On Wed, 19 Jul 2023 at 07:09, Fenglin Wu <[email protected]> wrote:
>>
>>
>>
>> On 7/18/2023 7:04 PM, Dmitry Baryshkov wrote:
>>> On Tue, 18 Jul 2023 at 13:55, Fenglin Wu <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
>>>>>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <[email protected]> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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", ®_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
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>
>
>