RE: [PATCH v3] regulator: pfuze100: add pfuze100 regulator driver
Thanks Mark, I will send V4. -Original Message- From: Mark Brown [mailto:broo...@kernel.org] Sent: Saturday, July 20, 2013 1:31 AM To: Gong Yibin-B38343 Cc: grant.lik...@linaro.org; rob.herr...@calxeda.com; r...@landley.net; lgirdw...@gmail.com; shawn@linaro.org; devicetree-disc...@lists.ozlabs.org; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] regulator: pfuze100: add pfuze100 regulator driver On Fri, Jul 19, 2013 at 06:14:51PM +0800, Robin Gong wrote: > Add pfuze100 regulator driver. Looks good, there's some minor issues below but they should be small. > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -158,6 +158,13 @@ config REGULATOR_MC13892 > Say y here to support the regulators found on the Freescale MC13892 > PMIC. > > +config REGULATOR_PFUZE100 > + tristate "Support regulators on Freescale PFUZE100 PMIC" > + depends on I2C > + help > + Say y here to support the regulators found on the Freescale PFUZE100 > + PMIC. > + Please keep this and the Makefile sorted - if there's issues merging then please use my topic/kconfig as a base. > + if (val & 0x40) > + ramp_delay = 5 / (4 * ramp_delay); > + else > + ramp_delay = 25000 / (2 * ramp_delay); > + if (id <= PFUZE100_SW1C) > + ramp_delay = 25000 / (2 * ramp_delay); This is a bit confusing as there's a cumalaitve modification applied to ramp_delay in the case that id is less than SW1C but it's not obvious that this is intentional given the calculations. Please change this to make it clearer what the intended logic is (assuming it's not buggy) - something like if ... else if ... else for example. > + ret = regmap_read(pfuze_chip->regmap, PFUZE100_REVID, ); > + if (ret) > + return ret; > + dev_info(pfuze_chip->dev, > + "Full lay: %x ,Metal lay: %x\n", > + (value & 0xf0) >> 4, value & 0x0f); The space should be after not before the comma in the displayed string. > + dev_info(pfuze_chip->dev, "FAB: %x , FIN: %x\n", > + (value & 0xc) >> 2, value & 0x3); Only a space after the , for normal formatting. ie both should look like "%x, bar". -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] regulator: pfuze100: add pfuze100 regulator driver
On Fri, Jul 19, 2013 at 06:14:51PM +0800, Robin Gong wrote: > Add pfuze100 regulator driver. Looks good, there's some minor issues below but they should be small. > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -158,6 +158,13 @@ config REGULATOR_MC13892 > Say y here to support the regulators found on the Freescale MC13892 > PMIC. > > +config REGULATOR_PFUZE100 > + tristate "Support regulators on Freescale PFUZE100 PMIC" > + depends on I2C > + help > + Say y here to support the regulators found on the Freescale PFUZE100 > + PMIC. > + Please keep this and the Makefile sorted - if there's issues merging then please use my topic/kconfig as a base. > + if (val & 0x40) > + ramp_delay = 5 / (4 * ramp_delay); > + else > + ramp_delay = 25000 / (2 * ramp_delay); > + if (id <= PFUZE100_SW1C) > + ramp_delay = 25000 / (2 * ramp_delay); This is a bit confusing as there's a cumalaitve modification applied to ramp_delay in the case that id is less than SW1C but it's not obvious that this is intentional given the calculations. Please change this to make it clearer what the intended logic is (assuming it's not buggy) - something like if ... else if ... else for example. > + ret = regmap_read(pfuze_chip->regmap, PFUZE100_REVID, ); > + if (ret) > + return ret; > + dev_info(pfuze_chip->dev, > + "Full lay: %x ,Metal lay: %x\n", > + (value & 0xf0) >> 4, value & 0x0f); The space should be after not before the comma in the displayed string. > + dev_info(pfuze_chip->dev, "FAB: %x , FIN: %x\n", > + (value & 0xc) >> 2, value & 0x3); Only a space after the , for normal formatting. ie both should look like "%x, bar". signature.asc Description: Digital signature
Re: [PATCH v3] regulator: pfuze100: add pfuze100 regulator driver
On Fri, Jul 19, 2013 at 06:14:51PM +0800, Robin Gong wrote: Add pfuze100 regulator driver. Looks good, there's some minor issues below but they should be small. --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -158,6 +158,13 @@ config REGULATOR_MC13892 Say y here to support the regulators found on the Freescale MC13892 PMIC. +config REGULATOR_PFUZE100 + tristate Support regulators on Freescale PFUZE100 PMIC + depends on I2C + help + Say y here to support the regulators found on the Freescale PFUZE100 + PMIC. + Please keep this and the Makefile sorted - if there's issues merging then please use my topic/kconfig as a base. + if (val 0x40) + ramp_delay = 5 / (4 * ramp_delay); + else + ramp_delay = 25000 / (2 * ramp_delay); + if (id = PFUZE100_SW1C) + ramp_delay = 25000 / (2 * ramp_delay); This is a bit confusing as there's a cumalaitve modification applied to ramp_delay in the case that id is less than SW1C but it's not obvious that this is intentional given the calculations. Please change this to make it clearer what the intended logic is (assuming it's not buggy) - something like if ... else if ... else for example. + ret = regmap_read(pfuze_chip-regmap, PFUZE100_REVID, value); + if (ret) + return ret; + dev_info(pfuze_chip-dev, + Full lay: %x ,Metal lay: %x\n, + (value 0xf0) 4, value 0x0f); The space should be after not before the comma in the displayed string. + dev_info(pfuze_chip-dev, FAB: %x , FIN: %x\n, + (value 0xc) 2, value 0x3); Only a space after the , for normal formatting. ie both should look like %x, bar. signature.asc Description: Digital signature
RE: [PATCH v3] regulator: pfuze100: add pfuze100 regulator driver
Thanks Mark, I will send V4. -Original Message- From: Mark Brown [mailto:broo...@kernel.org] Sent: Saturday, July 20, 2013 1:31 AM To: Gong Yibin-B38343 Cc: grant.lik...@linaro.org; rob.herr...@calxeda.com; r...@landley.net; lgirdw...@gmail.com; shawn@linaro.org; devicetree-disc...@lists.ozlabs.org; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] regulator: pfuze100: add pfuze100 regulator driver On Fri, Jul 19, 2013 at 06:14:51PM +0800, Robin Gong wrote: Add pfuze100 regulator driver. Looks good, there's some minor issues below but they should be small. --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -158,6 +158,13 @@ config REGULATOR_MC13892 Say y here to support the regulators found on the Freescale MC13892 PMIC. +config REGULATOR_PFUZE100 + tristate Support regulators on Freescale PFUZE100 PMIC + depends on I2C + help + Say y here to support the regulators found on the Freescale PFUZE100 + PMIC. + Please keep this and the Makefile sorted - if there's issues merging then please use my topic/kconfig as a base. + if (val 0x40) + ramp_delay = 5 / (4 * ramp_delay); + else + ramp_delay = 25000 / (2 * ramp_delay); + if (id = PFUZE100_SW1C) + ramp_delay = 25000 / (2 * ramp_delay); This is a bit confusing as there's a cumalaitve modification applied to ramp_delay in the case that id is less than SW1C but it's not obvious that this is intentional given the calculations. Please change this to make it clearer what the intended logic is (assuming it's not buggy) - something like if ... else if ... else for example. + ret = regmap_read(pfuze_chip-regmap, PFUZE100_REVID, value); + if (ret) + return ret; + dev_info(pfuze_chip-dev, + Full lay: %x ,Metal lay: %x\n, + (value 0xf0) 4, value 0x0f); The space should be after not before the comma in the displayed string. + dev_info(pfuze_chip-dev, FAB: %x , FIN: %x\n, + (value 0xc) 2, value 0x3); Only a space after the , for normal formatting. ie both should look like %x, bar. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
