From: w.f@huawei•com (wangfei)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 4/8] regulator: Hi655x: Add support for Hi655x regulator
Date: Thu, 8 Oct 2015 15:38:01 +0800 [thread overview]
Message-ID: <56161D59.1020207@huawei.com> (raw)
In-Reply-To: <20150930175857.GK15635@sirena.org.uk>
On 2015/10/1 1:58, Mark Brown wrote:
> On Wed, Sep 30, 2015 at 07:05:07PM +0800, Fei Wang wrote:
>
>> +config REGULATOR_HI655X
>> + tristate "Hisilicon HI655X PMIC regulators support"
>> + depends on ARCH_HISI
>> + depends on MFD_HI655X_PMIC && OF
>
> You've got some tab/space confusion above. Also, can't we have an ||
> COMPILE_TEST here?
>
OK, i will add it.
>> +#define REG_VALUE_SETBITS(reg_value, pos, bits, bits_value) \
>> + (reg_value = (reg_value & \
>> + ~((((unsigned int)1 << bits) - 1) << pos)) | \
>> + ((unsigned int)(bits_value & \
>> + (((unsigned int)1 << bits) - 1)) << pos))
>> +
>> +#define REG_VALUE_GETBITS(reg_value, pos, bits) \
>> + ((reg_value >> pos) & (((unsigned int)1 << bits) - 1))
>
> These are just really hard to read, sorry, and they appear to duplicate
> existing regmap functionality. If there is a strong reason to add them
> consider doing so in the core and if you can then please at least make
> them regular inline functions rather than using macros. It's much safer
> and more readable.
>
i agree with you ?i will refactor all the unreadable code?
>> +static int hi655x_regulator_pmic_is_enabled(struct regulator_dev *rdev)
>> +{
>> + int ret = 0;
>> + unsigned int value = 0;
>> +
>> + struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
>> + struct hi655x_regulator_ctrl_regs *ctrl_regs = &(sreg->ctrl_regs);
>> + struct hi655x_regulator_ctrl_data *ctrl_data = &(sreg->ctrl_data);
>> +
>> + /*
>> + * regulator is all set,but the pmu is only subset.
>> + * maybe this "buck"/"ldo"/"lvs" is not contrl by a core.
>> + * and in regulator have a "status" member ("okey" or "disable").
>> + */
>
> I'm having a hard time parsing the above comment. Please also use the
> normal kernel comment style (this is a problem throughout the driver).
>
OK. i will modify all of these?
>> + regmap_read(rdev->regmap, ctrl_regs->status_reg, &value);
>> + ret = (int)REG_VALUE_GETBITS(value, ctrl_data->shift,
>> + ctrl_data->mask);
>
> This appears to just duplicate regulator core functionality for reading
> enable state from a bitfield? Also note that the cast here isn't a
> great advert for the macros above.
>
>> +static int hi655x_regulator_pmic_enable(struct regulator_dev *rdev)
>> +{
>> + int ret = 0;
>> + unsigned char value_u8 = 0;
>> + unsigned int value_u32 = 0;
>> + struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
>> + struct hi655x_regulator_ctrl_regs *ctrl_regs = &(sreg->ctrl_regs);
>> + struct hi655x_regulator_ctrl_data *ctrl_data = &(sreg->ctrl_data);
>> +
>> + REG_VALUE_SETBITS(value_u32, ctrl_data->shift, ctrl_data->mask, 0x1);
>> + value_u8 = (unsigned char)value_u32;
>> + regmap_write(rdev->regmap, ctrl_regs->enable_reg, value_u8);
>
> I'm not *entirely* sure what this is supposed to be doing but it looks
> like it's duplicating core functionality in a fashion that's really
> quite hard to read. Why not just use the core functions for setting
> bits?
>
>> + udelay(sreg->off_on_delay);
>
> Use the regualtor core delay functionality please.
OK?i will modify it?
>
>> +static int hi655x_regulator_pmic_list_voltage_linear(struct regulator_dev *rdev,
>> + unsigned int selector)
>
> This is *definitely* duplicating core functionality, I think you want to
> use regulator_list_voltage_linear_range() or possibly just plain
> _linear() and use separate operations for the LVS regulator.
>
> We at least need to restructure the code so that the core helper
> functions are used and we don't have regulator type decisions everywhere
> - the whole goal of having per regulator ops is to avoid having to open
> code decisions about which regulator we're dealing with into each op
> function.
>
OK?i will modify it?
>> +static unsigned int hi655x_regulator_pmic_get_mode(
>> + struct regulator_dev *rdev)
>> +{
>> + return REGULATOR_MODE_NORMAL;
>> +}
>
> Don't implement empty functions, remove all these.
>
>> + num_consumer_supplies = of_get_property(np,
>> + "hisilicon,num_consumer_supplies", NULL);
>> +
>> + if ((NULL == num_consumer_supplies) || (0 == *num_consumer_supplies)) {
>> + dev_warn(dev, "%s no consumer_supplies\n", __func__);
>> + return init_data;
>> + }
>
> Obviously the binding is completely undocumented but this is setting off
> alarm bells - why is the driver even considering consumers? Please make
> sure you are using the core regulator bindings rather than open coding
> something which translates into platform data (which is what this looks
> like).
>
> I'm not going to review any more of the DT code without binding
> documentation.
I will document the dt-binding first?
>
>> + /*
>> + *initdata mem will release auto;
>> + *this is kernel 3.10 import.
>> + */
>
> Remove anything related to your vendor kernel support, this is not
> relevant to upstream.
>
Thanks?Mark?i agree with you and will modify all of you mentioned soon?
next prev parent reply other threads:[~2015-10-08 7:38 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-30 11:05 [PATCH 0/8] Add Support for Hi6220 PMIC Hi6553 MFD Core and Regulator Fei Wang
2015-09-30 11:05 ` [PATCH 1/8] dt-bindings: pmic: Document Hi655x pmic driver Fei Wang
2015-09-30 17:39 ` Mark Brown
2015-10-08 6:33 ` wangfei
2015-10-12 16:49 ` Mark Brown
2015-10-01 7:56 ` Lee Jones
2015-10-01 7:58 ` Lee Jones
2015-10-08 7:03 ` wangfei
2015-09-30 11:05 ` [PATCH 2/8] mfd: Hi655x: Add support for PMIC Hi655x MFD Fei Wang
2015-09-30 11:05 ` [PATCH 3/8] arm64: dts: add Hi655x pmic node Fei Wang
2015-09-30 11:05 ` [PATCH 4/8] regulator: Hi655x: Add support for Hi655x regulator Fei Wang
2015-09-30 17:58 ` Mark Brown
2015-10-08 7:38 ` wangfei [this message]
2015-10-08 7:47 ` Javier Martinez Canillas
2015-09-30 11:05 ` [PATCH 5/8] arm64: dts: Add Hi655x regulator config node Fei Wang
2015-09-30 11:05 ` [PATCH 6/8] dt-bindings: mtcmos: Document Hi6220 mtcmos driver Fei Wang
2015-09-30 11:05 ` [PATCH 7/8] mtcmos: Hi6220: Add Hi6220 mtcmos regulator driver Fei Wang
2015-09-30 11:05 ` [PATCH 8/8] arm64: dts: Add Hi6220 mtcmos regulator node Fei Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56161D59.1020207@huawei.com \
--to=w.f@huawei$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox