| 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: <053c9571-d709-2826-fced-a00dd7255b8b@quicinc.com>
Date: Tue, 25 Jul 2023 14:16:50 +0800
From: Fenglin Wu <quic_fenglinw@...cinc.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <robh+dt@...nel.org>,
<agross@...nel.org>, <andersson@...nel.org>,
<dmitry.baryshkov@...aro.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
<linux-input@...r.kernel.org>
CC: <quic_collinsd@...cinc.com>, <quic_subbaram@...cinc.com>,
<quic_kamalw@...cinc.com>, <jestar@....qualcomm.com>
Subject: Re: [PATCH v3 1/3] input: pm8xxx-vib: refactor to easily support new
SPMI vibrator
On 7/25/2023 1:52 PM, Krzysztof Kozlowski wrote:
> On 25/07/2023 07:41, Fenglin Wu wrote:
>> Currently, all vibrator control register addresses are hard coded,
>> including the base address and the offset, it's not flexible to support
>> new SPMI vibrator module which is usually included in different PMICs
>> with different base address. Refactor this by introducing the HW type
>> terminology and contain the register offsets/masks/shifts in reg_filed
>> data structures corresponding to each vibrator type, and the base address
>> value defined in 'reg' property will be added for SPMI vibrators.
>>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@...cinc.com>
>> ---
>> drivers/input/misc/pm8xxx-vibrator.c | 130 ++++++++++++++++-----------
>> 1 file changed, 77 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>> index 04cb87efd799..77bb0018d4e1 100644
>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>> @@ -12,36 +12,36 @@
>> #include <linux/regmap.h>
>> #include <linux/slab.h>
>>
>> +#define SSBI_VIB_DRV_EN_MANUAL_MASK 0xfc
>> +#define SSBI_VIB_DRV_LEVEL_MASK 0xf8
>> +#define SSBI_VIB_DRV_SHIFT 3
>> +#define SPMI_VIB_DRV_LEVEL_MASK 0xff
>> +#define SPMI_VIB_DRV_SHIFT 0
>> +
>> #define VIB_MAX_LEVEL_mV (3100)
>> #define VIB_MIN_LEVEL_mV (1200)
>> #define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
>>
>> #define MAX_FF_SPEED 0xff
>>
>> -struct pm8xxx_regs {
>> - unsigned int enable_addr;
>> - unsigned int enable_mask;
>> +enum pm8xxx_vib_type {
>> + SSBI_VIB,
>> + SPMI_VIB_GEN1,
>> +};
>>
>> - unsigned int drv_addr;
>> - unsigned int drv_mask;
>> - unsigned int drv_shift;
>> - unsigned int drv_en_manual_mask;
>> +enum {
>> + VIB_DRV_REG,
>> + VIB_EN_REG,
>> + VIB_MAX_REG,
>> };
>>
>> -static const struct pm8xxx_regs pm8058_regs = {
>> - .drv_addr = 0x4A,
>> - .drv_mask = 0xf8,
>> - .drv_shift = 3,
>> - .drv_en_manual_mask = 0xfc,
>> +static struct reg_field ssbi_vib_regs[VIB_MAX_REG] = {
>
> Change from const to non-const is wrong. How do you support multiple
> devices? No, this is way too fragile now.
>
The register definition is no longer used as the match data, hw_type is
used.
The last suggestion was getting the register base address from the DT
and it has to be added into the offset of SPMI vibrator registers
(either in the previous hard-coded format or the later the reg_filed
data structure), so it's not appropriated to make it constant.
I don't understand this question: "How do you support multiple devices?"
For SSBI vibrator, since all the registers are fixed, and I would assume
that there is no chance to support multiple vibrator devices on the same
SSBI bus. If they are not on the same bus, the regmap device will be
different while the registers definition is the same, and we are still
able to support multiple devices, right?
The similar story for SPMI vibrators and it can support multiple devices
if they are located on different SPMI bus, or even if they are on the
same SPMI bus but just having different SID or PID.
> ...
>
>>
>> /*
>> * pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
>> @@ -168,38 +166,65 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>> {
>> struct pm8xxx_vib *vib;
>> struct input_dev *input_dev;
>> - int error;
>> + struct device *dev = &pdev->dev;
>> + struct regmap *regmap;
>> + struct reg_field *regs;
>> + int error, i;
>> unsigned int val;
>> - const struct pm8xxx_regs *regs;
>> + u32 reg_base;
>>
>> - vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
>> + vib = devm_kzalloc(dev, sizeof(*vib), GFP_KERNEL);
>
> Not really related. Split cleanup from new features.
Okay, will keep as it is.
>
>> if (!vib)
>> return -ENOMEM;
>>
>> - vib->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> - if (!vib->regmap)
>> + regmap = dev_get_regmap(dev->parent, NULL);
>> + if (!regmap)
>> return -ENODEV;
>>
>> - input_dev = devm_input_allocate_device(&pdev->dev);
>> + input_dev = devm_input_allocate_device(dev);
>> if (!input_dev)
>> return -ENOMEM;
>>
>> INIT_WORK(&vib->work, pm8xxx_work_handler);
>> vib->vib_input_dev = input_dev;
>>
>> - regs = of_device_get_match_data(&pdev->dev);
>> + vib->hw_type = (enum pm8xxx_vib_type)of_device_get_match_data(dev);
>>
>> - /* operate in manual mode */
>> - error = regmap_read(vib->regmap, regs->drv_addr, &val);
>> + regs = ssbi_vib_regs;
>> + if (vib->hw_type != SSBI_VIB) {
>> + error = fwnode_property_read_u32(dev->fwnode, "reg", ®_base);
>> + if (error < 0) {
>> + dev_err(dev, "Failed to read reg address, rc=%d\n", error);
>> + return error;
>> + }
>> +
>> + if (vib->hw_type == SPMI_VIB_GEN1)
>> + regs = spmi_vib_gen1_regs;
>> +
>> + for (i = 0; i < VIB_MAX_REG; i++)
>> + if (regs[i].reg != 0)
>> + regs[i].reg += reg_base;
>> + }
>> +
>> + error = devm_regmap_field_bulk_alloc(dev, regmap, vib->r_fields, regs, VIB_MAX_REG);
>> if (error < 0)
>> + {
>
> That's not a Linux coding style.
>
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
>
>> + dev_err(dev, "Failed to allocate regmap failed, rc=%d\n", error);
>
> No need to print errors on allocation failures.
>
Will fix it.
Thanks
>> return error;
>> + }
>>
>> - val &= regs->drv_en_manual_mask;
>> - error = regmap_write(vib->regmap, regs->drv_addr, val);
>> + error = regmap_field_read(vib->r_fields[VIB_DRV_REG], &val);
>> if (error < 0)
>> return error;
>
>
> Best regards,
> Krzysztof
>
Powered by blists - more mailing lists