lkml.org 
[lkml]   [2015]   [Oct]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/8] regulator: Hi655x: Add support for Hi655x regulator
    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。




    \
     
     \ /
      Last update: 2015-10-08 10:01    [W:4.221 / U:1.776 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site