Re: [PATCH v3 4/5] regulator: add regulator driver of hi655x PMIC

[chenfeng]

Mark,
Thanks very much for your review.

On 2015/12/17 3:16, Mark Brown wrote:
> On Tue, Dec 15, 2015 at 08:54:15PM +0800, Chen Feng wrote:
> 
>> +config REGULATOR_HI655X
>> +	tristate "Hisilicon HI655X PMIC regulators support"
>> +	depends on ARCH_HISI
>> +	depends on MFD_HI655X_PMIC && OF
>> +	help
>> +	  This driver provides support for the voltage regulators of the
>> +	  Hisilicon Hi655x PMIC device.
>> +
> 
> On the previous version of this patch I said:
> 
> | For both of these we should have an || COMPILE_TEST and there's no need
> | for either to be bool I can see, they should be tristate.
> 
> I see you have made this a tristate which is good but you've not enabled
> COMPILE_TEST or indicated why - there may be a very good reason for
> doing this but nobody has said what it is.  Please don't ignore review
> comments, people are generally making them for a reason and are likely
> to have the same concerns if issues remain unaddressed.  Having to
> repeat the same comments can get repetitive and make people question the
> value of time spent reviewing.  If you disagree with the review comments
> that's fine but you need to reply and discuss your concerns so that the
> reviewer can understand your decisions.
> 
Sorry for this,I am busy with other issue these days.

Really appreciate your comments.

I will add the COMPILE_TEST next version.

>> +static int hi655x_is_enabled(struct regulator_dev *rdev)
>> +{
>> +	unsigned int value = 0;
>> +
>> +	struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
>> +	struct hi655x_regulator_ctrl_regs *ctrl_regs = &regulator->ctrl_regs;
> 
> The style here is not what we normally do - normally the struct lookups
> would be first, then any other variables and there wouldn't be any blank
> lines in the variable declarations.  The same applies to quite a few
> functions.
> 

ok, I will fix the style problem.


>> +static int hi655x_set_voltage(struct regulator_dev *rdev,
>> +				int min_uV, int max_uV, unsigned *selector)
>> +{
> 
> As I commented on the previous version of this driver:
> 
> | Use the standard helpers, including one of the map_voltage()s and
> | set_voltage_sel_regmap(), don't open code them.
> 
I change the regulator_enable method to regulator_enable_regmap this version.

But,since the hi655x PMIC use status register to ensure the regulator is okay or not.

Three different register offset in these PMIC series.

enable register.
disable register.
status register.


I can't use regulator_is_enabled_regmap regulator_disable_regmap in helpers.c

int regulator_disable_regmap(struct regulator_dev *rdev)
{
	...
        return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
                                  rdev->desc->enable_mask, val);
	...
}
Only enable_reg in the desc struct.

I will change the set voltage to regulator_set_voltage_sel_regmap next version.


Thanks again.


> You need to at least split the map and set_voltage_sel operations.
> 
> I've stopped reviewing here.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/