| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAA8EJpoL9vCAUgWmHcoxppo_gJqaw_xqdYqcJkS6Xza-5aSh3A@mail.gmail.com>
Date: Thu, 11 Apr 2024 13:58:09 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: quic_fenglinw@...cinc.com
Cc: kernel@...cinc.com, Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, linux-arm-msm@...r.kernel.org,
linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v9 2/4] input: pm8xxx-vibrator: refactor to support new
SPMI vibrator
On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
<devnull+quic_fenglinw.quicinc.com@...nel.org> wrote:
>
> From: Fenglin Wu <quic_fenglinw@...cinc.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@...cinc.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.
> .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;
>
>
> --
> 2.25.1
>
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists