linux-kernel - Re: [PATCH V7 3/5] regulator: Add a regulator driver for the PM8008 PMIC

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 28 Feb 2022 19:48:14 +0530
From:   "Satya Priya Kakitapalli (Temp)" <quic_c_skakit@...cinc.com>
To:     Stephen Boyd <swboyd@...omium.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>
CC:     Lee Jones <lee.jones@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Das Srinagesh <gurus@...eaurora.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_collinsd@...cinc.com>,
        <quic_subbaram@...cinc.com>, <quic_jprakash@...cinc.com>
Subject: Re: [PATCH V7 3/5] regulator: Add a regulator driver for the PM8008
 PMIC


On 2/19/2022 7:20 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-02-18 03:01:01)
>> Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
>> containing 7 LDO regulators.  Add a PM8008 regulator driver to
>> support PMIC regulator management via the regulator framework.
>>
>> Signed-off-by: Satya Priya <quic_c_skakit@...cinc.com>
>> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
>> new file mode 100644
>> index 0000000..1c52864
>> --- /dev/null
>> +++ b/drivers/regulator/qcom-pm8008-regulator.c
>> @@ -0,0 +1,205 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */
>> +
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
>> +
>> +#define VSET_STEP_MV                   8
>> +#define VSET_STEP_UV                   (VSET_STEP_MV * 1000)
>> +
>> +#define LDO_ENABLE_REG(base)           ((base) + 0x46)
>> +#define ENABLE_BIT                     BIT(7)
> This is SPMI_COMMON_REG_ENABLE and SPMI_COMMON_ENABLE_MASK
>
>> +
>> +#define LDO_VSET_LB_REG(base)          ((base) + 0x40)
>> +
>> +#define LDO_STEPPER_CTL_REG(base)      ((base) + 0x3b)
>> +#define DEFAULT_VOLTAGE_STEPPER_RATE   38400
>> +#define STEP_RATE_MASK                 GENMASK(1, 0)
>> +
>> +struct regulator_data {
> Please use a name like spmi_regulator_data :) Or pm8008_regulator_data?
> Something that isn't so generic.


Okay.


> It seems similar to qcom_spmi-regulator.c so is there some reason we
> can't shove this into there? Less code for things that aren't relevant I
> guess?


As mentioned earlier, the register layout is not compatible and some 
other differences w.r.t to code like headrooms, using mfd driver to 
register ldos etc..


>> +       const char                      *name;
>> +       const char                      *supply_name;
>> +       u16                             base;
>> +       int                             min_uv;
>> +       int                             max_uv;
>> +       int                             min_dropout_uv;
>> +       const struct linear_range       *voltage_range;