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。
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
