Received-SPF: pass (google.com: domain of linux-kernel+bounces-140295-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240411-pm8xxx-vibrator-new-design-v9-0-7bf56cb92b28@quicinc.com>
 <20240411-pm8xxx-vibrator-new-design-v9-2-7bf56cb92b28@quicinc.com>
In-Reply-To: <20240411-pm8xxx-vibrator-new-design-v9-2-7bf56cb92b28@quicinc.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Thu, 11 Apr 2024 13:58:09 +0300
Message-ID: <CAA8EJpoL9vCAUgWmHcoxppo_gJqaw_xqdYqcJkS6Xza-5aSh3A@mail.gmail.com>
Subject: Re: [PATCH v9 2/4] input: pm8xxx-vibrator: refactor to support new
 SPMI vibrator
To: quic_fenglinw@quicinc.com
Cc: kernel@quicinc.com, Andy Gross <agross@kernel.org>, 
	Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, 
	Dmitry Torokhov <dmitry.torokhov@gmail.com>, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, linux-arm-msm@vger.kernel.org, 
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, 
	devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

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.

>         .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", &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