Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1293320

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

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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.

> +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.

> +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.

You need to at least split the map and set_voltage_sel operations.

I've stopped reviewing here.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v3 0/5] Add Support for Hi6220 PMIC Hi6553 MFD Core Chen Feng <puck.chen@hisilicon.com> - 2015-12-15 14:00 +0100
  [PATCH v3 4/5] regulator: add regulator driver of hi655x PMIC Chen Feng <puck.chen@hisilicon.com> - 2015-12-15 14:00 +0100
    Re: [PATCH v3 4/5] regulator: add regulator driver of hi655x PMIC Mark Brown <broonie@kernel.org> - 2015-12-16 20:20 +0100
      Re: [PATCH v3 4/5] regulator: add regulator driver of hi655x PMIC chenfeng <puck.chen@hisilicon.com> - 2015-12-17 04:20 +0100
  [PATCH v3 2/5] doc: bindings: Document for hi655x regulator driver Chen Feng <puck.chen@hisilicon.com> - 2015-12-15 14:00 +0100
  [PATCH v3 5/5] hisilicon/dts: Add hi655x pmic dts node Chen Feng <puck.chen@hisilicon.com> - 2015-12-15 14:00 +0100
    Re: [PATCH v3 5/5] hisilicon/dts: Add hi655x pmic dts node chenfeng <puck.chen@hisilicon.com> - 2015-12-17 04:40 +0100
      Re: [PATCH v3 5/5] hisilicon/dts: Add hi655x pmic dts node Mark Brown <broonie@kernel.org> - 2015-12-18 19:00 +0100
        Re: [PATCH v3 5/5] hisilicon/dts: Add hi655x pmic dts node chenfeng <puck.chen@hisilicon.com> - 2015-12-21 04:10 +0100
          Re: [PATCH v3 5/5] hisilicon/dts: Add hi655x pmic dts node chenfeng <puck.chen@hisilicon.com> - 2015-12-21 07:40 +0100
            Re: [PATCH v3 5/5] hisilicon/dts: Add hi655x pmic dts node Mark Brown <broonie@kernel.org> - 2015-12-23 01:50 +0100
              Re: [PATCH v3 5/5] hisilicon/dts: Add hi655x pmic dts node chenfeng <puck.chen@hisilicon.com> - 2015-12-24 04:00 +0100
          Re: [PATCH v3 5/5] hisilicon/dts: Add hi655x pmic dts node Mark Brown <broonie@kernel.org> - 2015-12-22 17:10 +0100

csiph-web