| Message ID | 20240411-pm8xxx-vibrator-new-design-v9-2-7bf56cb92b28@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for vibrator in multiple PMICs | expand |
On 4/11/2024 10:05 PM, Dmitry Baryshkov wrote: > On Thu, 11 Apr 2024 at 16:45, Fenglin Wu <quic_fenglinw@quicinc.com> wrote: >> >> >> >> On 2024/4/11 18:58, Dmitry Baryshkov wrote: >>> On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay >>> <devnull+quic_fenglinw.quicinc.com@kernel.org> wrote: >>>> >>>> From: Fenglin Wu <quic_fenglinw@quicinc.com> >>>> >>>> Currently, vibrator control register addresses are hard coded, >>>> including the base address and offsets, it's not flexible to >>>> support new SPMI vibrator module which is usually included in >>>> different PMICs with different base address. Refactor it by using >>>> the base address defined in devicetree. >>>> >>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com> >>>> --- >>>> drivers/input/misc/pm8xxx-vibrator.c | 42 ++++++++++++++++++++++++------------ >>>> 1 file changed, 28 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c >>>> index 89f0f1c810d8..2959edca8eb9 100644 >>>> --- a/drivers/input/misc/pm8xxx-vibrator.c >>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c >>>> @@ -20,26 +20,26 @@ >>>> #define MAX_FF_SPEED 0xff >>>> >>>> struct pm8xxx_regs { >>>> - unsigned int enable_addr; >>>> + unsigned int enable_offset; >>>> unsigned int enable_mask; >>>> >>>> - unsigned int drv_addr; >>>> + unsigned int drv_offset; >>>> unsigned int drv_mask; >>>> unsigned int drv_shift; >>>> unsigned int drv_en_manual_mask; >>>> }; >>>> >>>> static const struct pm8xxx_regs pm8058_regs = { >>>> - .drv_addr = 0x4A, >>>> + .drv_offset = 0x4A, >>> >>> If the DT already has reg = <0x4a> and you add drv_offset = 0x4a, >>> which register will be used by the driver? >>> >>> Also, while we are at it, please downcase all the hex numbers that you >>> are touching. >>> >> For SSBI vibrator, the "reg" value defined in DT is not used, see below. >> >> >>>> .drv_mask = 0xf8, >>>> .drv_shift = 3, >>>> .drv_en_manual_mask = 0xfc, >>>> }; >>>> >>>> static struct pm8xxx_regs pm8916_regs = { >>>> - .enable_addr = 0xc046, >>>> + .enable_offset = 0x46, >> >> [...] >> >>>> @@ -170,7 +173,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev) >>>> struct pm8xxx_vib *vib; >>>> struct input_dev *input_dev; >>>> int error; >>>> - unsigned int val; >>>> + unsigned int val, reg_base = 0; >>>> const struct pm8xxx_regs *regs; >>>> >>>> vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL); >>>> @@ -190,13 +193,24 @@ static int pm8xxx_vib_probe(struct platform_device *pdev) >>>> >>>> regs = of_device_get_match_data(&pdev->dev); >>>> >>>> + if (regs->enable_offset != 0) { >>>> + error = fwnode_property_read_u32(pdev->dev.fwnode, "reg", ®_base); >>>> + if (error < 0) { >>>> + dev_err(&pdev->dev, "Failed to read reg address, rc=%d\n", error); >>>> + return error; >>>> + } >>>> + } >>>> + >>>> + vib->enable_addr = reg_base + regs->enable_offset; >>>> + vib->drv_addr = reg_base + regs->drv_offset; >> >> The reg_base is initialized as 0 and it is assigned as the "reg" value >> defined in DT only for SPMI vibrators. > > Please don't. This is counterintuitive. We have reg in DT. We should > be using it. > Hmm, the original driver doesn't use the reg value defined in DT at all, Anyway, I can make the SSBI offset to 0, so the base address defined in the DT will be always added regardless of SSBI or SPMI vibrator. Let me know. Thanks >> >>>> + >>>> /* operate in manual mode */ >>>> - error = regmap_read(vib->regmap, regs->drv_addr, &val); >>>> + error = regmap_read(vib->regmap, vib->drv_addr, &val); >>>> if (error < 0) >>>> return error; >>>> >>>> val &= regs->drv_en_manual_mask; >>>> - error = regmap_write(vib->regmap, regs->drv_addr, val); >>>> + error = regmap_write(vib->regmap, vib->drv_addr, val); >>>> if (error < 0) >>>> return error; >>>> >>>> >>>> -- >>>> 2.25.1 >>>> >>>> >>> >>> > > >
On Fri, 12 Apr 2024 at 06:41, Fenglin Wu <quic_fenglinw@quicinc.com> wrote: > > > > On 4/11/2024 10:05 PM, Dmitry Baryshkov wrote: > > On Thu, 11 Apr 2024 at 16:45, Fenglin Wu <quic_fenglinw@quicinc.com> wrote: > >> > >> > >> > >> On 2024/4/11 18:58, Dmitry Baryshkov wrote: > >>> On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay > >>> <devnull+quic_fenglinw.quicinc.com@kernel.org> wrote: > >>>> > >>>> From: Fenglin Wu <quic_fenglinw@quicinc.com> > >>>> > >>>> Currently, vibrator control register addresses are hard coded, > >>>> including the base address and offsets, it's not flexible to > >>>> support new SPMI vibrator module which is usually included in > >>>> different PMICs with different base address. Refactor it by using > >>>> the base address defined in devicetree. > >>>> > >>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com> > >>>> --- > >>>> drivers/input/misc/pm8xxx-vibrator.c | 42 ++++++++++++++++++++++++------------ > >>>> 1 file changed, 28 insertions(+), 14 deletions(-) > >>>> > >>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c > >>>> index 89f0f1c810d8..2959edca8eb9 100644 > >>>> --- a/drivers/input/misc/pm8xxx-vibrator.c > >>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c > >>>> @@ -20,26 +20,26 @@ > >>>> #define MAX_FF_SPEED 0xff > >>>> > >>>> struct pm8xxx_regs { > >>>> - unsigned int enable_addr; > >>>> + unsigned int enable_offset; > >>>> unsigned int enable_mask; > >>>> > >>>> - unsigned int drv_addr; > >>>> + unsigned int drv_offset; > >>>> unsigned int drv_mask; > >>>> unsigned int drv_shift; > >>>> unsigned int drv_en_manual_mask; > >>>> }; > >>>> > >>>> static const struct pm8xxx_regs pm8058_regs = { > >>>> - .drv_addr = 0x4A, > >>>> + .drv_offset = 0x4A, > >>> > >>> If the DT already has reg = <0x4a> and you add drv_offset = 0x4a, > >>> which register will be used by the driver? > >>> > >>> Also, while we are at it, please downcase all the hex numbers that you > >>> are touching. > >>> > >> For SSBI vibrator, the "reg" value defined in DT is not used, see below. > >> > >> > >>>> .drv_mask = 0xf8, > >>>> .drv_shift = 3, > >>>> .drv_en_manual_mask = 0xfc, > >>>> }; > >>>> > >>>> static struct pm8xxx_regs pm8916_regs = { > >>>> - .enable_addr = 0xc046, > >>>> + .enable_offset = 0x46, > >> > >> [...] > >> > >>>> @@ -170,7 +173,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev) > >>>> struct pm8xxx_vib *vib; > >>>> struct input_dev *input_dev; > >>>> int error; > >>>> - unsigned int val; > >>>> + unsigned int val, reg_base = 0; > >>>> const struct pm8xxx_regs *regs; > >>>> > >>>> vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL); > >>>> @@ -190,13 +193,24 @@ static int pm8xxx_vib_probe(struct platform_device *pdev) > >>>> > >>>> regs = of_device_get_match_data(&pdev->dev); > >>>> > >>>> + if (regs->enable_offset != 0) { > >>>> + error = fwnode_property_read_u32(pdev->dev.fwnode, "reg", ®_base); > >>>> + if (error < 0) { > >>>> + dev_err(&pdev->dev, "Failed to read reg address, rc=%d\n", error); > >>>> + return error; > >>>> + } > >>>> + } > >>>> + > >>>> + vib->enable_addr = reg_base + regs->enable_offset; > >>>> + vib->drv_addr = reg_base + regs->drv_offset; > >> > >> The reg_base is initialized as 0 and it is assigned as the "reg" value > >> defined in DT only for SPMI vibrators. > > > > Please don't. This is counterintuitive. We have reg in DT. We should > > be using it. > > > Hmm, the original driver doesn't use the reg value defined in DT at all, > Anyway, I can make the SSBI offset to 0, so the base address defined in > the DT will be always added regardless of SSBI or SPMI vibrator. Let me > know. Yes, please. This follows the logic of the SPMI change too. > Thanks > > >> > >>>> + > >>>> /* operate in manual mode */ > >>>> - error = regmap_read(vib->regmap, regs->drv_addr, &val); > >>>> + error = regmap_read(vib->regmap, vib->drv_addr, &val); > >>>> if (error < 0) > >>>> return error; > >>>> > >>>> val &= regs->drv_en_manual_mask; > >>>> - error = regmap_write(vib->regmap, regs->drv_addr, val); > >>>> + error = regmap_write(vib->regmap, vib->drv_addr, val); > >>>> if (error < 0) > >>>> return error; > >>>> > >>>> > >>>> -- > >>>> 2.25.1 > >>>> > >>>> > >>> > >>> > > > > > >
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c index 89f0f1c810d8..2959edca8eb9 100644 --- a/drivers/input/misc/pm8xxx-vibrator.c +++ b/drivers/input/misc/pm8xxx-vibrator.c @@ -20,26 +20,26 @@ #define MAX_FF_SPEED 0xff struct pm8xxx_regs { - unsigned int enable_addr; + unsigned int enable_offset; unsigned int enable_mask; - unsigned int drv_addr; + unsigned int drv_offset; unsigned int drv_mask; unsigned int drv_shift; unsigned int drv_en_manual_mask; }; static const struct pm8xxx_regs pm8058_regs = { - .drv_addr = 0x4A, + .drv_offset = 0x4A, .drv_mask = 0xf8, .drv_shift = 3, .drv_en_manual_mask = 0xfc, }; static struct pm8xxx_regs pm8916_regs = { - .enable_addr = 0xc046, + .enable_offset = 0x46, .enable_mask = BIT(7), - .drv_addr = 0xc041, + .drv_offset = 0x41, .drv_mask = 0x1F, .drv_shift = 0, .drv_en_manual_mask = 0, @@ -51,6 +51,8 @@ static struct pm8xxx_regs pm8916_regs = { * @work: work structure to set the vibration parameters * @regmap: regmap for register read/write * @regs: registers' info + * @enable_addr: vibrator enable register + * @drv_addr: vibrator drive strength register * @speed: speed of vibration set from userland * @active: state of vibrator * @level: level of vibration to set in the chip @@ -61,6 +63,8 @@ struct pm8xxx_vib { struct work_struct work; struct regmap *regmap; const struct pm8xxx_regs *regs; + unsigned int enable_addr; + unsigned int drv_addr; int speed; int level; bool active; @@ -83,15 +87,15 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on) else val &= ~regs->drv_mask; - rc = regmap_write(vib->regmap, regs->drv_addr, val); + rc = regmap_write(vib->regmap, vib->drv_addr, val); if (rc < 0) return rc; vib->reg_vib_drv = val; if (regs->enable_mask) - rc = regmap_update_bits(vib->regmap, regs->enable_addr, - regs->enable_mask, on ? ~0 : 0); + rc = regmap_update_bits(vib->regmap, vib->enable_addr, + regs->enable_mask, on ? regs->enable_mask : 0); return rc; } @@ -103,11 +107,10 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on) static void pm8xxx_work_handler(struct work_struct *work) { struct pm8xxx_vib *vib = container_of(work, struct pm8xxx_vib, work); - const struct pm8xxx_regs *regs = vib->regs; - int rc; unsigned int val; + int rc; - rc = regmap_read(vib->regmap, regs->drv_addr, &val); + rc = regmap_read(vib->regmap, vib->drv_addr, &val); if (rc < 0) return; @@ -170,7 +173,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev) struct pm8xxx_vib *vib; struct input_dev *input_dev; int error; - unsigned int val; + unsigned int val, reg_base = 0; const struct pm8xxx_regs *regs; vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL); @@ -190,13 +193,24 @@ static int pm8xxx_vib_probe(struct platform_device *pdev) regs = of_device_get_match_data(&pdev->dev); + if (regs->enable_offset != 0) { + error = fwnode_property_read_u32(pdev->dev.fwnode, "reg", ®_base); + if (error < 0) { + dev_err(&pdev->dev, "Failed to read reg address, rc=%d\n", error); + return error; + } + } + + vib->enable_addr = reg_base + regs->enable_offset; + vib->drv_addr = reg_base + regs->drv_offset; + /* operate in manual mode */ - error = regmap_read(vib->regmap, regs->drv_addr, &val); + error = regmap_read(vib->regmap, vib->drv_addr, &val); if (error < 0) return error; val &= regs->drv_en_manual_mask; - error = regmap_write(vib->regmap, regs->drv_addr, val); + error = regmap_write(vib->regmap, vib->drv_addr, val); if (error < 0) return error;