* [PATCH V4 1/2] dt-bindings: iio: light: isl29018: update power supply name @ 2018-12-11 3:24 Anson Huang 2018-12-11 3:24 ` [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support Anson Huang 0 siblings, 1 reply; 7+ messages in thread From: Anson Huang @ 2018-12-11 3:24 UTC (permalink / raw) To: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland, linux-iio, devicetree, linux-kernel, festevam, preid Cc: dl-linux-imx According to datasheet, the isl29018 has "vddd/vdda" power supply, and isl29023/isl29035 ONLY has "vdd" power supply, update the power supply name with "vdd" and "vdda" according to datasheet to cover all devices and avoid confusion. Signed-off-by: Anson Huang <Anson.Huang@nxp.com> --- Documentation/devicetree/bindings/iio/light/isl29018.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/iio/light/isl29018.txt b/Documentation/devicetree/bindings/iio/light/isl29018.txt index b9bbde3..36f737d 100644 --- a/Documentation/devicetree/bindings/iio/light/isl29018.txt +++ b/Documentation/devicetree/bindings/iio/light/isl29018.txt @@ -15,7 +15,9 @@ Optional properties: Refer to interrupt-controller/interrupts.txt for generic interrupt client node bindings. - - vcc-supply: phandle to the regulator that provides power to the sensor. + - vdd-supply: phandle to the regulator that provides vdd power to the sensor. + + - vdda-supply: phandle to the regulator that provides vdda power to the sensor. Example: -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support 2018-12-11 3:24 [PATCH V4 1/2] dt-bindings: iio: light: isl29018: update power supply name Anson Huang @ 2018-12-11 3:24 ` Anson Huang 2018-12-11 3:36 ` Phil Reid 0 siblings, 1 reply; 7+ messages in thread From: Anson Huang @ 2018-12-11 3:24 UTC (permalink / raw) To: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland, linux-iio, devicetree, linux-kernel, festevam, preid Cc: dl-linux-imx The light sensor's power supply could be controlled by regulator on some platforms, such as i.MX6Q-SABRESD board, the light sensor isl29023's power supply is controlled by a GPIO fixed regulator, need to make sure the regulator is enabled before any operation of sensor, this patch adds optional vdd/vdda regulator operation support. Signed-off-by: Anson Huang <Anson.Huang@nxp.com> --- ChangeLog since V3: - improve the error handling of devm_regulator_get_optional; - make sure regulators are disabled in error path. --- drivers/iio/light/isl29018.c | 83 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 3 deletions(-) diff --git a/drivers/iio/light/isl29018.c b/drivers/iio/light/isl29018.c index b45400f..a21652b 100644 --- a/drivers/iio/light/isl29018.c +++ b/drivers/iio/light/isl29018.c @@ -23,6 +23,7 @@ #include <linux/mutex.h> #include <linux/delay.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include <linux/slab.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> @@ -95,6 +96,8 @@ struct isl29018_chip { struct isl29018_scale scale; int prox_scheme; bool suspended; + struct regulator *vdd_reg; + struct regulator *vdda_reg; }; static int isl29018_set_integration_time(struct isl29018_chip *chip, @@ -735,6 +738,34 @@ static int isl29018_probe(struct i2c_client *client, mutex_init(&chip->lock); + chip->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd"); + if (!IS_ERR(chip->vdd_reg)) { + err = regulator_enable(chip->vdd_reg); + if (err) { + dev_err(&client->dev, "failed to enable VDD regulator\n"); + return err; + } + } else { + err = PTR_ERR(chip->vdd_reg); + if (err != -ENODEV) + return err; + } + + chip->vdda_reg = devm_regulator_get_optional(&client->dev, "vdda"); + if (!IS_ERR(chip->vdda_reg)) { + err = regulator_enable(chip->vdda_reg); + if (err) { + dev_err(&client->dev, "failed to enable VDDA regulator\n"); + if (!IS_ERR(chip->vdd_reg)) + regulator_disable(chip->vdd_reg); + return err; + } + } else { + err = PTR_ERR(chip->vdda_reg); + if (err != -ENODEV) + return err; + } + chip->type = dev_id; chip->calibscale = 1; chip->ucalibscale = 0; @@ -747,12 +778,12 @@ static int isl29018_probe(struct i2c_client *client, if (IS_ERR(chip->regmap)) { err = PTR_ERR(chip->regmap); dev_err(&client->dev, "regmap initialization fails: %d\n", err); - return err; + goto disable_regulators; } err = isl29018_chip_init(chip); if (err) - return err; + goto disable_regulators; indio_dev->info = isl29018_chip_info_tbl[dev_id].indio_info; indio_dev->channels = isl29018_chip_info_tbl[dev_id].channels; @@ -761,13 +792,24 @@ static int isl29018_probe(struct i2c_client *client, indio_dev->dev.parent = &client->dev; indio_dev->modes = INDIO_DIRECT_MODE; - return devm_iio_device_register(&client->dev, indio_dev); + err = devm_iio_device_register(&client->dev, indio_dev); + if (!err) + return 0; + +disable_regulators: + if (!IS_ERR(chip->vdd_reg)) + regulator_disable(chip->vdd_reg); + if (!IS_ERR(chip->vdda_reg)) + regulator_disable(chip->vdda_reg); + + return err; } #ifdef CONFIG_PM_SLEEP static int isl29018_suspend(struct device *dev) { struct isl29018_chip *chip = iio_priv(dev_get_drvdata(dev)); + int ret; mutex_lock(&chip->lock); @@ -777,6 +819,22 @@ static int isl29018_suspend(struct device *dev) * So we do not have much to do here. */ chip->suspended = true; + if (!IS_ERR(chip->vdd_reg)) { + ret = regulator_disable(chip->vdd_reg); + if (ret) { + dev_err(dev, "failed to disable VDD regulator\n"); + mutex_unlock(&chip->lock); + return ret; + } + } + if (!IS_ERR(chip->vdda_reg)) { + ret = regulator_disable(chip->vdda_reg); + if (ret) { + dev_err(dev, "failed to disable VDDA regulator\n"); + mutex_unlock(&chip->lock); + return ret; + } + } mutex_unlock(&chip->lock); @@ -790,6 +848,25 @@ static int isl29018_resume(struct device *dev) mutex_lock(&chip->lock); + if (!IS_ERR(chip->vdd_reg)) { + err = regulator_enable(chip->vdd_reg); + if (err) { + dev_err(dev, "failed to enable VDD regulator\n"); + mutex_unlock(&chip->lock); + return err; + } + } + if (!IS_ERR(chip->vdda_reg)) { + err = regulator_enable(chip->vdda_reg); + if (err) { + dev_err(dev, "failed to enable VDDA regulator\n"); + if (!IS_ERR(chip->vdd_reg)) + regulator_disable(chip->vdd_reg); + mutex_unlock(&chip->lock); + return err; + } + } + err = isl29018_chip_init(chip); if (!err) chip->suspended = false; -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support 2018-12-11 3:24 ` [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support Anson Huang @ 2018-12-11 3:36 ` Phil Reid 2018-12-11 3:43 ` Anson Huang 0 siblings, 1 reply; 7+ messages in thread From: Phil Reid @ 2018-12-11 3:36 UTC (permalink / raw) To: Anson Huang, jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland, linux-iio, devicetree, linux-kernel, festevam Cc: dl-linux-imx On 11/12/2018 11:24 am, Anson Huang wrote: > The light sensor's power supply could be controlled by regulator > on some platforms, such as i.MX6Q-SABRESD board, the light sensor > isl29023's power supply is controlled by a GPIO fixed regulator, > need to make sure the regulator is enabled before any operation of > sensor, this patch adds optional vdd/vdda regulator operation support. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > ChangeLog since V3: > - improve the error handling of devm_regulator_get_optional; > - make sure regulators are disabled in error path. > --- > drivers/iio/light/isl29018.c | 83 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 80 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/light/isl29018.c b/drivers/iio/light/isl29018.c > index b45400f..a21652b 100644 > --- a/drivers/iio/light/isl29018.c > +++ b/drivers/iio/light/isl29018.c > @@ -23,6 +23,7 @@ > #include <linux/mutex.h> > #include <linux/delay.h> > #include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > #include <linux/slab.h> > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > @@ -95,6 +96,8 @@ struct isl29018_chip { > struct isl29018_scale scale; > int prox_scheme; > bool suspended; > + struct regulator *vdd_reg; > + struct regulator *vdda_reg; > }; > > static int isl29018_set_integration_time(struct isl29018_chip *chip, > @@ -735,6 +738,34 @@ static int isl29018_probe(struct i2c_client *client, > > mutex_init(&chip->lock); > > + chip->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd"); > + if (!IS_ERR(chip->vdd_reg)) { > + err = regulator_enable(chip->vdd_reg); > + if (err) { > + dev_err(&client->dev, "failed to enable VDD regulator\n"); > + return err; > + } > + } else { > + err = PTR_ERR(chip->vdd_reg); > + if (err != -ENODEV) > + return err; > + } > + > + chip->vdda_reg = devm_regulator_get_optional(&client->dev, "vdda"); > + if (!IS_ERR(chip->vdda_reg)) { > + err = regulator_enable(chip->vdda_reg); > + if (err) { > + dev_err(&client->dev, "failed to enable VDDA regulator\n"); > + if (!IS_ERR(chip->vdd_reg)) > + regulator_disable(chip->vdd_reg); > + return err; > + } > + } else { > + err = PTR_ERR(chip->vdda_reg); > + if (err != -ENODEV) > + return err; maybe goto disable_regulators; to disable vdd. > + } > + > chip->type = dev_id; > chip->calibscale = 1; > chip->ucalibscale = 0; > @@ -747,12 +778,12 @@ static int isl29018_probe(struct i2c_client *client, > if (IS_ERR(chip->regmap)) { > err = PTR_ERR(chip->regmap); > dev_err(&client->dev, "regmap initialization fails: %d\n", err); > - return err; > + goto disable_regulators; > } > > err = isl29018_chip_init(chip); > if (err) > - return err; > + goto disable_regulators; > > indio_dev->info = isl29018_chip_info_tbl[dev_id].indio_info; > indio_dev->channels = isl29018_chip_info_tbl[dev_id].channels; > @@ -761,13 +792,24 @@ static int isl29018_probe(struct i2c_client *client, > indio_dev->dev.parent = &client->dev; > indio_dev->modes = INDIO_DIRECT_MODE; > > - return devm_iio_device_register(&client->dev, indio_dev); > + err = devm_iio_device_register(&client->dev, indio_dev); > + if (!err) > + return 0; > + > +disable_regulators: > + if (!IS_ERR(chip->vdd_reg)) > + regulator_disable(chip->vdd_reg); > + if (!IS_ERR(chip->vdda_reg)) > + regulator_disable(chip->vdda_reg); > + > + return err; > } > [snip] -- Regards Phil ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support 2018-12-11 3:36 ` Phil Reid @ 2018-12-11 3:43 ` Anson Huang 2018-12-11 3:56 ` Phil Reid 0 siblings, 1 reply; 7+ messages in thread From: Anson Huang @ 2018-12-11 3:43 UTC (permalink / raw) To: Phil Reid, jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland, linux-iio, devicetree, linux-kernel, festevam Cc: dl-linux-imx Hi, Phil Best Regards! Anson Huang > -----Original Message----- > From: Phil Reid [mailto:preid@electromag.com.au] > Sent: 2018年12月11日 11:36 > To: Anson Huang <anson.huang@nxp.com>; jic23@kernel.org; > knaack.h@gmx.de; lars@metafoo.de; pmeerw@pmeerw.net; > robh+dt@kernel.org; mark.rutland@arm.com; linux-iio@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > festevam@gmail.com > Cc: dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda > regulator operation support > > On 11/12/2018 11:24 am, Anson Huang wrote: > > The light sensor's power supply could be controlled by regulator on > > some platforms, such as i.MX6Q-SABRESD board, the light sensor > > isl29023's power supply is controlled by a GPIO fixed regulator, need > > to make sure the regulator is enabled before any operation of sensor, > > this patch adds optional vdd/vdda regulator operation support. > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > --- > > ChangeLog since V3: > > - improve the error handling of devm_regulator_get_optional; > > - make sure regulators are disabled in error path. > > --- > > drivers/iio/light/isl29018.c | 83 > ++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 80 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iio/light/isl29018.c > > b/drivers/iio/light/isl29018.c index b45400f..a21652b 100644 > > --- a/drivers/iio/light/isl29018.c > > +++ b/drivers/iio/light/isl29018.c > > @@ -23,6 +23,7 @@ > > #include <linux/mutex.h> > > #include <linux/delay.h> > > #include <linux/regmap.h> > > +#include <linux/regulator/consumer.h> > > #include <linux/slab.h> > > #include <linux/iio/iio.h> > > #include <linux/iio/sysfs.h> > > @@ -95,6 +96,8 @@ struct isl29018_chip { > > struct isl29018_scale scale; > > int prox_scheme; > > bool suspended; > > + struct regulator *vdd_reg; > > + struct regulator *vdda_reg; > > }; > > > > static int isl29018_set_integration_time(struct isl29018_chip *chip, > > @@ -735,6 +738,34 @@ static int isl29018_probe(struct i2c_client > > *client, > > > > mutex_init(&chip->lock); > > > > + chip->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd"); > > + if (!IS_ERR(chip->vdd_reg)) { > > + err = regulator_enable(chip->vdd_reg); > > + if (err) { > > + dev_err(&client->dev, "failed to enable VDD regulator\n"); > > + return err; > > + } > > + } else { > > + err = PTR_ERR(chip->vdd_reg); > > + if (err != -ENODEV) > > + return err; > > + } > > + > > + chip->vdda_reg = devm_regulator_get_optional(&client->dev, "vdda"); > > + if (!IS_ERR(chip->vdda_reg)) { > > + err = regulator_enable(chip->vdda_reg); > > + if (err) { > > + dev_err(&client->dev, "failed to enable VDDA regulator\n"); > > + if (!IS_ERR(chip->vdd_reg)) > > + regulator_disable(chip->vdd_reg); > > + return err; > > + } > > + } else { > > + err = PTR_ERR(chip->vdda_reg); > > + if (err != -ENODEV) > > + return err; > maybe goto disable_regulators; to disable vdd. Agree, I will use " goto disable_regulators;" in both here and upper regulator enable fail case. Please help review V5, thanks. Anson > > > + } > > + > > chip->type = dev_id; > > chip->calibscale = 1; > > chip->ucalibscale = 0; > > @@ -747,12 +778,12 @@ static int isl29018_probe(struct i2c_client *client, > > if (IS_ERR(chip->regmap)) { > > err = PTR_ERR(chip->regmap); > > dev_err(&client->dev, "regmap initialization fails: %d\n", err); > > - return err; > > + goto disable_regulators; > > } > > > > err = isl29018_chip_init(chip); > > if (err) > > - return err; > > + goto disable_regulators; > > > > indio_dev->info = isl29018_chip_info_tbl[dev_id].indio_info; > > indio_dev->channels = isl29018_chip_info_tbl[dev_id].channels; > > @@ -761,13 +792,24 @@ static int isl29018_probe(struct i2c_client *client, > > indio_dev->dev.parent = &client->dev; > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > - return devm_iio_device_register(&client->dev, indio_dev); > > + err = devm_iio_device_register(&client->dev, indio_dev); > > + if (!err) > > + return 0; > > + > > +disable_regulators: > > + if (!IS_ERR(chip->vdd_reg)) > > + regulator_disable(chip->vdd_reg); > > + if (!IS_ERR(chip->vdda_reg)) > > + regulator_disable(chip->vdda_reg); > > + > > + return err; > > } > > > [snip] > > -- > Regards > Phil ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support 2018-12-11 3:43 ` Anson Huang @ 2018-12-11 3:56 ` Phil Reid 2018-12-11 4:29 ` Anson Huang 0 siblings, 1 reply; 7+ messages in thread From: Phil Reid @ 2018-12-11 3:56 UTC (permalink / raw) To: Anson Huang, jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland, linux-iio, devicetree, linux-kernel, festevam Cc: dl-linux-imx G'day Anson, Just pulled up the datasheet for this chip. The absolute max for Vdda is speced as Vddd +/-0.5 With a note that Vdda should be externally shorted to Vddd. On 11/12/2018 11:43 am, Anson Huang wrote: > Hi, Phil > > Best Regards! > Anson Huang > >> -----Original Message----- >> From: Phil Reid [mailto:preid@electromag.com.au] >> Sent: 2018年12月11日 11:36 >> To: Anson Huang <anson.huang@nxp.com>; jic23@kernel.org; >> knaack.h@gmx.de; lars@metafoo.de; pmeerw@pmeerw.net; >> robh+dt@kernel.org; mark.rutland@arm.com; linux-iio@vger.kernel.org; >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; >> festevam@gmail.com >> Cc: dl-linux-imx <linux-imx@nxp.com> >> Subject: Re: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda >> regulator operation support >> >> On 11/12/2018 11:24 am, Anson Huang wrote: >>> The light sensor's power supply could be controlled by regulator on >>> some platforms, such as i.MX6Q-SABRESD board, the light sensor >>> isl29023's power supply is controlled by a GPIO fixed regulator, need >>> to make sure the regulator is enabled before any operation of sensor, >>> this patch adds optional vdd/vdda regulator operation support. >>> >>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com> >>> --- >>> ChangeLog since V3: >>> - improve the error handling of devm_regulator_get_optional; >>> - make sure regulators are disabled in error path. >>> --- >>> drivers/iio/light/isl29018.c | 83 >> ++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 80 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/iio/light/isl29018.c >>> b/drivers/iio/light/isl29018.c index b45400f..a21652b 100644 >>> --- a/drivers/iio/light/isl29018.c >>> +++ b/drivers/iio/light/isl29018.c >>> @@ -23,6 +23,7 @@ >>> #include <linux/mutex.h> >>> #include <linux/delay.h> >>> #include <linux/regmap.h> >>> +#include <linux/regulator/consumer.h> >>> #include <linux/slab.h> >>> #include <linux/iio/iio.h> >>> #include <linux/iio/sysfs.h> >>> @@ -95,6 +96,8 @@ struct isl29018_chip { >>> struct isl29018_scale scale; >>> int prox_scheme; >>> bool suspended; >>> + struct regulator *vdd_reg; >>> + struct regulator *vdda_reg; >>> }; >>> >>> static int isl29018_set_integration_time(struct isl29018_chip *chip, >>> @@ -735,6 +738,34 @@ static int isl29018_probe(struct i2c_client >>> *client, >>> >>> mutex_init(&chip->lock); >>> >>> + chip->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd"); >>> + if (!IS_ERR(chip->vdd_reg)) { >>> + err = regulator_enable(chip->vdd_reg); >>> + if (err) { >>> + dev_err(&client->dev, "failed to enable VDD regulator\n"); >>> + return err; >>> + } >>> + } else { >>> + err = PTR_ERR(chip->vdd_reg); >>> + if (err != -ENODEV) >>> + return err; >>> + } >>> + >>> + chip->vdda_reg = devm_regulator_get_optional(&client->dev, "vdda"); >>> + if (!IS_ERR(chip->vdda_reg)) { >>> + err = regulator_enable(chip->vdda_reg); >>> + if (err) { >>> + dev_err(&client->dev, "failed to enable VDDA regulator\n"); >>> + if (!IS_ERR(chip->vdd_reg)) >>> + regulator_disable(chip->vdd_reg); >>> + return err; Not sure about this case at the call to enable failed so I think you only want the first one to be disabled. You could add another goto statement thou, see below. >>> + } >>> + } else { >>> + err = PTR_ERR(chip->vdda_reg); >>> + if (err != -ENODEV) >>> + return err; >> maybe goto disable_regulators; to disable vdd. > > Agree, I will use " goto disable_regulators;" in both here and upper regulator enable fail case. > Please help review V5, thanks. Here its safe to call both as vdda_reg will be an error ptr. > > Anson > >> >>> + } >>> + >>> chip->type = dev_id; >>> chip->calibscale = 1; >>> chip->ucalibscale = 0; >>> @@ -747,12 +778,12 @@ static int isl29018_probe(struct i2c_client *client, >>> if (IS_ERR(chip->regmap)) { >>> err = PTR_ERR(chip->regmap); >>> dev_err(&client->dev, "regmap initialization fails: %d\n", err); >>> - return err; >>> + goto disable_regulators; >>> } >>> >>> err = isl29018_chip_init(chip); >>> if (err) >>> - return err; >>> + goto disable_regulators; >>> >>> indio_dev->info = isl29018_chip_info_tbl[dev_id].indio_info; >>> indio_dev->channels = isl29018_chip_info_tbl[dev_id].channels; >>> @@ -761,13 +792,24 @@ static int isl29018_probe(struct i2c_client *client, >>> indio_dev->dev.parent = &client->dev; >>> indio_dev->modes = INDIO_DIRECT_MODE; >>> >>> - return devm_iio_device_register(&client->dev, indio_dev); >>> + err = devm_iio_device_register(&client->dev, indio_dev); >>> + if (!err) >>> + return 0; >>> + >>> +disable_regulators: >>> + if (!IS_ERR(chip->vdd_reg)) >>> + regulator_disable(chip->vdd_reg); >>> + if (!IS_ERR(chip->vdda_reg)) >>> + regulator_disable(chip->vdda_reg); >>> + eg: extra label here. Order is changed thou, which may be a problem. It's in the reverse order to the enable, which is usually what you want but the datasheet concerns above may be an issue. disable_regulators: if (!IS_ERR(chip->vdda_reg)) regulator_disable(chip->vdda_reg); disable_regulator_vdd: if (!IS_ERR(chip->vdd_reg)) regulator_disable(chip->vdd_reg); >>> + return err; >>> } >>> >> [snip] >> -- Regards Phil ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support 2018-12-11 3:56 ` Phil Reid @ 2018-12-11 4:29 ` Anson Huang 2018-12-11 5:02 ` Phil Reid 0 siblings, 1 reply; 7+ messages in thread From: Anson Huang @ 2018-12-11 4:29 UTC (permalink / raw) To: Phil Reid, jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland, linux-iio, devicetree, linux-kernel, festevam Cc: dl-linux-imx Hi, Phil Best Regards! Anson Huang > -----Original Message----- > From: Phil Reid [mailto:preid@electromag.com.au] > Sent: 2018年12月11日 11:56 > To: Anson Huang <anson.huang@nxp.com>; jic23@kernel.org; > knaack.h@gmx.de; lars@metafoo.de; pmeerw@pmeerw.net; > robh+dt@kernel.org; mark.rutland@arm.com; linux-iio@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > festevam@gmail.com > Cc: dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda > regulator operation support > > G'day Anson, > > Just pulled up the datasheet for this chip. > > The absolute max for Vdda is speced as Vddd +/-0.5 With a note that Vdda > should be externally shorted to Vddd. The data sheet says vdda should be connected to vdd externally, then I think we should just use one regulator vdd, then it will be much easier for the driver, what do you think? Anson. > > > On 11/12/2018 11:43 am, Anson Huang wrote: > > Hi, Phil > > > > Best Regards! > > Anson Huang > > > >> -----Original Message----- > >> From: Phil Reid [mailto:preid@electromag.com.au] > >> Sent: 2018年12月11日 11:36 > >> To: Anson Huang <anson.huang@nxp.com>; jic23@kernel.org; > >> knaack.h@gmx.de; lars@metafoo.de; pmeerw@pmeerw.net; > >> robh+dt@kernel.org; mark.rutland@arm.com; linux-iio@vger.kernel.org; > >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > >> festevam@gmail.com > >> Cc: dl-linux-imx <linux-imx@nxp.com> > >> Subject: Re: [PATCH V4 2/2] iio: light: isl29018: add optional > >> vdd/vdda regulator operation support > >> > >> On 11/12/2018 11:24 am, Anson Huang wrote: > >>> The light sensor's power supply could be controlled by regulator on > >>> some platforms, such as i.MX6Q-SABRESD board, the light sensor > >>> isl29023's power supply is controlled by a GPIO fixed regulator, > >>> need to make sure the regulator is enabled before any operation of > >>> sensor, this patch adds optional vdd/vdda regulator operation support. > >>> > >>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > >>> --- > >>> ChangeLog since V3: > >>> - improve the error handling of devm_regulator_get_optional; > >>> - make sure regulators are disabled in error path. > >>> --- > >>> drivers/iio/light/isl29018.c | 83 > >> ++++++++++++++++++++++++++++++++++++++++++-- > >>> 1 file changed, 80 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/iio/light/isl29018.c > >>> b/drivers/iio/light/isl29018.c index b45400f..a21652b 100644 > >>> --- a/drivers/iio/light/isl29018.c > >>> +++ b/drivers/iio/light/isl29018.c > >>> @@ -23,6 +23,7 @@ > >>> #include <linux/mutex.h> > >>> #include <linux/delay.h> > >>> #include <linux/regmap.h> > >>> +#include <linux/regulator/consumer.h> > >>> #include <linux/slab.h> > >>> #include <linux/iio/iio.h> > >>> #include <linux/iio/sysfs.h> > >>> @@ -95,6 +96,8 @@ struct isl29018_chip { > >>> struct isl29018_scale scale; > >>> int prox_scheme; > >>> bool suspended; > >>> + struct regulator *vdd_reg; > >>> + struct regulator *vdda_reg; > >>> }; > >>> > >>> static int isl29018_set_integration_time(struct isl29018_chip > >>> *chip, @@ -735,6 +738,34 @@ static int isl29018_probe(struct > >>> i2c_client *client, > >>> > >>> mutex_init(&chip->lock); > >>> > >>> + chip->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd"); > >>> + if (!IS_ERR(chip->vdd_reg)) { > >>> + err = regulator_enable(chip->vdd_reg); > >>> + if (err) { > >>> + dev_err(&client->dev, "failed to enable VDD regulator\n"); > >>> + return err; > >>> + } > >>> + } else { > >>> + err = PTR_ERR(chip->vdd_reg); > >>> + if (err != -ENODEV) > >>> + return err; > >>> + } > >>> + > >>> + chip->vdda_reg = devm_regulator_get_optional(&client->dev, > "vdda"); > >>> + if (!IS_ERR(chip->vdda_reg)) { > >>> + err = regulator_enable(chip->vdda_reg); > >>> + if (err) { > >>> + dev_err(&client->dev, "failed to enable VDDA > regulator\n"); > >>> + if (!IS_ERR(chip->vdd_reg)) > >>> + regulator_disable(chip->vdd_reg); > >>> + return err; > Not sure about this case at the call to enable failed so I think you only want > the first one to be disabled. > You could add another goto statement thou, see below. > > > >>> + } > >>> + } else { > >>> + err = PTR_ERR(chip->vdda_reg); > >>> + if (err != -ENODEV) > >>> + return err; > >> maybe goto disable_regulators; to disable vdd. > > > > Agree, I will use " goto disable_regulators;" in both here and upper regulator > enable fail case. > > Please help review V5, thanks. > > Here its safe to call both as vdda_reg will be an error ptr. > > > > > Anson > > > >> > >>> + } > >>> + > >>> chip->type = dev_id; > >>> chip->calibscale = 1; > >>> chip->ucalibscale = 0; > >>> @@ -747,12 +778,12 @@ static int isl29018_probe(struct i2c_client > *client, > >>> if (IS_ERR(chip->regmap)) { > >>> err = PTR_ERR(chip->regmap); > >>> dev_err(&client->dev, "regmap initialization fails: %d\n", err); > >>> - return err; > >>> + goto disable_regulators; > >>> } > >>> > >>> err = isl29018_chip_init(chip); > >>> if (err) > >>> - return err; > >>> + goto disable_regulators; > >>> > >>> indio_dev->info = isl29018_chip_info_tbl[dev_id].indio_info; > >>> indio_dev->channels = isl29018_chip_info_tbl[dev_id].channels; > >>> @@ -761,13 +792,24 @@ static int isl29018_probe(struct i2c_client > *client, > >>> indio_dev->dev.parent = &client->dev; > >>> indio_dev->modes = INDIO_DIRECT_MODE; > >>> > >>> - return devm_iio_device_register(&client->dev, indio_dev); > >>> + err = devm_iio_device_register(&client->dev, indio_dev); > >>> + if (!err) > >>> + return 0; > >>> + > >>> +disable_regulators: > >>> + if (!IS_ERR(chip->vdd_reg)) > >>> + regulator_disable(chip->vdd_reg); > >>> + if (!IS_ERR(chip->vdda_reg)) > >>> + regulator_disable(chip->vdda_reg); > >>> + > eg: extra label here. > Order is changed thou, which may be a problem. > It's in the reverse order to the enable, which is usually what you want but the > datasheet concerns above may be an issue. > > > disable_regulators: > if (!IS_ERR(chip->vdda_reg)) > regulator_disable(chip->vdda_reg); > disable_regulator_vdd: > if (!IS_ERR(chip->vdd_reg)) > regulator_disable(chip->vdd_reg); > > >>> + return err; > >>> } > >>> > >> [snip] > >> > > > > -- > Regards > Phil ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support 2018-12-11 4:29 ` Anson Huang @ 2018-12-11 5:02 ` Phil Reid 0 siblings, 0 replies; 7+ messages in thread From: Phil Reid @ 2018-12-11 5:02 UTC (permalink / raw) To: Anson Huang, jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland, linux-iio, devicetree, linux-kernel, festevam Cc: dl-linux-imx On 11/12/2018 12:29 pm, Anson Huang wrote: >> G'day Anson, >> >> Just pulled up the datasheet for this chip. >> >> The absolute max for Vdda is speced as Vddd ±0.5 With a note that Vdda >> should be externally shorted to Vddd. > The data sheet says vdda should be connected to vdd externally, then I think we should just > use one regulator vdd, then it will be much easier for the driver, what do you think? > Yes I think that makes sense. -- Regards Phil ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-11 5:02 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-11 3:24 [PATCH V4 1/2] dt-bindings: iio: light: isl29018: update power supply name Anson Huang 2018-12-11 3:24 ` [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support Anson Huang 2018-12-11 3:36 ` Phil Reid 2018-12-11 3:43 ` Anson Huang 2018-12-11 3:56 ` Phil Reid 2018-12-11 4:29 ` Anson Huang 2018-12-11 5:02 ` Phil Reid
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).