| Message ID | 20190729100339.24054-1-hslester96@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio: tsl2772: Use device-managed API | expand |
Hi Chuhong, On Mon, Jul 29, 2019 at 06:03:39PM +0800, Chuhong Yuan wrote: > Use devm_iio_device_register to simplify > the code. > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com> Thank you for the patch. The patch description doesn't match what all is done below. This should also be broken up into multiple patches since a patch should only do one thing. The regulator changes should be in their own patch, and some of the devm changes may require multiple patches. When writing your changelog, if your patch description has the word 'and', then that may be a hint that you need to break up your patch a little bit. That's not always the case, but something to keep in mind. A few minor comments below. > --- > Changes in v2: > - Use regulator_bulk_() to shrink driver > size. > - Utilize more devm functions to simplify > the code. > - Remove several redundant functions. > > drivers/iio/light/tsl2772.c | 116 +++++++++++------------------------- > 1 file changed, 34 insertions(+), 82 deletions(-) > > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c > index 83cece921843..946537c8586f 100644 > --- a/drivers/iio/light/tsl2772.c > +++ b/drivers/iio/light/tsl2772.c > @@ -131,7 +131,10 @@ enum { > enum { > TSL2772_CHIP_UNKNOWN = 0, > TSL2772_CHIP_WORKING = 1, > - TSL2772_CHIP_SUSPENDED = 2 > + TSL2772_CHIP_SUSPENDED = 2, > + TSL2772_SUPPLY_VDD = 0, > + TSL2772_SUPPLY_VDDIO = 1, > + TSL2772_NUM_SUPPLIES = 2 > }; This is a really minor nitpick but can these either use a #define or be placed in its own enum block? > > /* Per-device data */ > @@ -161,8 +164,7 @@ struct tsl2772_chip { > struct mutex prox_mutex; > struct mutex als_mutex; > struct i2c_client *client; > - struct regulator *vdd_supply; > - struct regulator *vddio_supply; > + struct regulator_bulk_data reg[TSL2772_NUM_SUPPLIES]; Since there's other changes, maybe name this 'supplies'? I think of 'reg' as an address. > u16 prox_data; > struct tsl2772_als_info als_cur_info; > struct tsl2772_settings settings; > @@ -697,46 +699,7 @@ static void tsl2772_disable_regulators_action(void *_data) > { > struct tsl2772_chip *chip = _data; > > - regulator_disable(chip->vdd_supply); > - regulator_disable(chip->vddio_supply); > -} > - > -static int tsl2772_enable_regulator(struct tsl2772_chip *chip, > - struct regulator *regulator) > -{ > - int ret; > - > - ret = regulator_enable(regulator); > - if (ret < 0) { > - dev_err(&chip->client->dev, "Failed to enable regulator: %d\n", > - ret); > - return ret; > - } > - > - return 0; > -} > - > -static struct regulator *tsl2772_get_regulator(struct tsl2772_chip *chip, > - char *name) > -{ > - struct regulator *regulator; > - int ret; > - > - regulator = devm_regulator_get(&chip->client->dev, name); > - if (IS_ERR(regulator)) { > - if (PTR_ERR(regulator) != -EPROBE_DEFER) > - dev_err(&chip->client->dev, > - "Failed to get %s regulator %d\n", > - name, (int)PTR_ERR(regulator)); > - > - return regulator; > - } > - > - ret = tsl2772_enable_regulator(chip, regulator); > - if (ret < 0) > - return ERR_PTR(ret); > - > - return regulator; > + regulator_bulk_disable(ARRAY_SIZE(chip->reg), chip->reg); > } > > static int tsl2772_chip_on(struct iio_dev *indio_dev) > @@ -860,6 +823,13 @@ static int tsl2772_chip_off(struct iio_dev *indio_dev) > return tsl2772_write_control_reg(chip, 0x00); > } > > +static void tsl2772_chip_off_action(void *data) > +{ > + struct iio_dev *indio_dev = data; > + > + tsl2772_chip_off(indio_dev); > +} > + > /** > * tsl2772_invoke_change - power cycle the device to implement the user > * parameters > @@ -1797,20 +1767,22 @@ static int tsl2772_probe(struct i2c_client *clientp, > chip->client = clientp; > i2c_set_clientdata(clientp, indio_dev); > > - chip->vddio_supply = tsl2772_get_regulator(chip, "vddio"); > - if (IS_ERR(chip->vddio_supply)) > - return PTR_ERR(chip->vddio_supply); > + chip->reg[TSL2772_SUPPLY_VDD].supply = "vdd"; > + chip->reg[TSL2772_SUPPLY_VDDIO].supply = "vddio"; > > - chip->vdd_supply = tsl2772_get_regulator(chip, "vdd"); > - if (IS_ERR(chip->vdd_supply)) { > - regulator_disable(chip->vddio_supply); > - return PTR_ERR(chip->vdd_supply); > - } > + ret = devm_regulator_bulk_get(&clientp->dev, ARRAY_SIZE(chip->reg), > + chip->reg); > + if (ret < 0) > + return ret; Add a dev_err like the other error paths in probe function. Users can use the tracing subsystem to see why this failed but an error message in dmesg is much easier for users to find. Also be sure to check for the EPROBE_DEFER case. Brian
Brian Masney <masneyb@onstation.org> 于2019年7月29日周一 下午6:59写道： > > Hi Chuhong, > > On Mon, Jul 29, 2019 at 06:03:39PM +0800, Chuhong Yuan wrote: > > Use devm_iio_device_register to simplify > > the code. > > > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com> > > Thank you for the patch. The patch description doesn't match what all is > done below. This should also be broken up into multiple patches since a > patch should only do one thing. The regulator changes should be in their > own patch, and some of the devm changes may require multiple patches. > When writing your changelog, if your patch description has the word > 'and', then that may be a hint that you need to break up your patch a > little bit. That's not always the case, but something to keep in mind. > Thanks for your advice. I will split it into two patches in next version. One is to use devm to simpliy the code. The other is to use regulator_bulk_() to shrink driver size. > A few minor comments below. > > > --- > > Changes in v2: > > - Use regulator_bulk_() to shrink driver > > size. > > - Utilize more devm functions to simplify > > the code. > > - Remove several redundant functions. > > > > drivers/iio/light/tsl2772.c | 116 +++++++++++------------------------- > > 1 file changed, 34 insertions(+), 82 deletions(-) > > > > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c > > index 83cece921843..946537c8586f 100644 > > --- a/drivers/iio/light/tsl2772.c > > +++ b/drivers/iio/light/tsl2772.c > > @@ -131,7 +131,10 @@ enum { > > enum { > > TSL2772_CHIP_UNKNOWN = 0, > > TSL2772_CHIP_WORKING = 1, > > - TSL2772_CHIP_SUSPENDED = 2 > > + TSL2772_CHIP_SUSPENDED = 2, > > + TSL2772_SUPPLY_VDD = 0, > > + TSL2772_SUPPLY_VDDIO = 1, > > + TSL2772_NUM_SUPPLIES = 2 > > }; > > This is a really minor nitpick but can these either use a #define or be > placed in its own enum block? > I refer to drivers/iio/adc/ad7766.c when I use regulator_bulk_(). This file puts them in the enum block. There are also files using #define, like drivers/iio/dac/ad5064.c and ad5449.c. I think both of them are okay. > > > > /* Per-device data */ > > @@ -161,8 +164,7 @@ struct tsl2772_chip { > > struct mutex prox_mutex; > > struct mutex als_mutex; > > struct i2c_client *client; > > - struct regulator *vdd_supply; > > - struct regulator *vddio_supply; > > + struct regulator_bulk_data reg[TSL2772_NUM_SUPPLIES]; > > Since there's other changes, maybe name this 'supplies'? I think of > 'reg' as an address. > Indeed there are files choosing supplies as the name. But in iio, all regulator_bulk_data use reg (or regs, vref_reg) as the name. So I also use reg as the name to keep consistency with others. > > u16 prox_data; > > struct tsl2772_als_info als_cur_info; > > struct tsl2772_settings settings; > > @@ -697,46 +699,7 @@ static void tsl2772_disable_regulators_action(void *_data) > > { > > struct tsl2772_chip *chip = _data; > > > > - regulator_disable(chip->vdd_supply); > > - regulator_disable(chip->vddio_supply); > > -} > > - > > -static int tsl2772_enable_regulator(struct tsl2772_chip *chip, > > - struct regulator *regulator) > > -{ > > - int ret; > > - > > - ret = regulator_enable(regulator); > > - if (ret < 0) { > > - dev_err(&chip->client->dev, "Failed to enable regulator: %d\n", > > - ret); > > - return ret; > > - } > > - > > - return 0; > > -} > > - > > -static struct regulator *tsl2772_get_regulator(struct tsl2772_chip *chip, > > - char *name) > > -{ > > - struct regulator *regulator; > > - int ret; > > - > > - regulator = devm_regulator_get(&chip->client->dev, name); > > - if (IS_ERR(regulator)) { > > - if (PTR_ERR(regulator) != -EPROBE_DEFER) > > - dev_err(&chip->client->dev, > > - "Failed to get %s regulator %d\n", > > - name, (int)PTR_ERR(regulator)); > > - > > - return regulator; > > - } > > - > > - ret = tsl2772_enable_regulator(chip, regulator); > > - if (ret < 0) > > - return ERR_PTR(ret); > > - > > - return regulator; > > + regulator_bulk_disable(ARRAY_SIZE(chip->reg), chip->reg); > > } > > > > static int tsl2772_chip_on(struct iio_dev *indio_dev) > > @@ -860,6 +823,13 @@ static int tsl2772_chip_off(struct iio_dev *indio_dev) > > return tsl2772_write_control_reg(chip, 0x00); > > } > > > > +static void tsl2772_chip_off_action(void *data) > > +{ > > + struct iio_dev *indio_dev = data; > > + > > + tsl2772_chip_off(indio_dev); > > +} > > + > > /** > > * tsl2772_invoke_change - power cycle the device to implement the user > > * parameters > > @@ -1797,20 +1767,22 @@ static int tsl2772_probe(struct i2c_client *clientp, > > chip->client = clientp; > > i2c_set_clientdata(clientp, indio_dev); > > > > - chip->vddio_supply = tsl2772_get_regulator(chip, "vddio"); > > - if (IS_ERR(chip->vddio_supply)) > > - return PTR_ERR(chip->vddio_supply); > > + chip->reg[TSL2772_SUPPLY_VDD].supply = "vdd"; > > + chip->reg[TSL2772_SUPPLY_VDDIO].supply = "vddio"; > > > > - chip->vdd_supply = tsl2772_get_regulator(chip, "vdd"); > > - if (IS_ERR(chip->vdd_supply)) { > > - regulator_disable(chip->vddio_supply); > > - return PTR_ERR(chip->vdd_supply); > > - } > > + ret = devm_regulator_bulk_get(&clientp->dev, ARRAY_SIZE(chip->reg), > > + chip->reg); > > + if (ret < 0) > > + return ret; > > Add a dev_err like the other error paths in probe function. Users can > use the tracing subsystem to see why this failed but an error message > in dmesg is much easier for users to find. Also be sure to check for the > EPROBE_DEFER case. > I will do this in next version. Regards, Chuhong > Brian
diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c index 83cece921843..946537c8586f 100644 --- a/drivers/iio/light/tsl2772.c +++ b/drivers/iio/light/tsl2772.c @@ -131,7 +131,10 @@ enum { enum { TSL2772_CHIP_UNKNOWN = 0, TSL2772_CHIP_WORKING = 1, - TSL2772_CHIP_SUSPENDED = 2 + TSL2772_CHIP_SUSPENDED = 2, + TSL2772_SUPPLY_VDD = 0, + TSL2772_SUPPLY_VDDIO = 1, + TSL2772_NUM_SUPPLIES = 2 }; /* Per-device data */ @@ -161,8 +164,7 @@ struct tsl2772_chip { struct mutex prox_mutex; struct mutex als_mutex; struct i2c_client *client; - struct regulator *vdd_supply; - struct regulator *vddio_supply; + struct regulator_bulk_data reg[TSL2772_NUM_SUPPLIES]; u16 prox_data; struct tsl2772_als_info als_cur_info; struct tsl2772_settings settings; @@ -697,46 +699,7 @@ static void tsl2772_disable_regulators_action(void *_data) { struct tsl2772_chip *chip = _data; - regulator_disable(chip->vdd_supply); - regulator_disable(chip->vddio_supply); -} - -static int tsl2772_enable_regulator(struct tsl2772_chip *chip, - struct regulator *regulator) -{ - int ret; - - ret = regulator_enable(regulator); - if (ret < 0) { - dev_err(&chip->client->dev, "Failed to enable regulator: %d\n", - ret); - return ret; - } - - return 0; -} - -static struct regulator *tsl2772_get_regulator(struct tsl2772_chip *chip, - char *name) -{ - struct regulator *regulator; - int ret; - - regulator = devm_regulator_get(&chip->client->dev, name); - if (IS_ERR(regulator)) { - if (PTR_ERR(regulator) != -EPROBE_DEFER) - dev_err(&chip->client->dev, - "Failed to get %s regulator %d\n", - name, (int)PTR_ERR(regulator)); - - return regulator; - } - - ret = tsl2772_enable_regulator(chip, regulator); - if (ret < 0) - return ERR_PTR(ret); - - return regulator; + regulator_bulk_disable(ARRAY_SIZE(chip->reg), chip->reg); } static int tsl2772_chip_on(struct iio_dev *indio_dev) @@ -860,6 +823,13 @@ static int tsl2772_chip_off(struct iio_dev *indio_dev) return tsl2772_write_control_reg(chip, 0x00); } +static void tsl2772_chip_off_action(void *data) +{ + struct iio_dev *indio_dev = data; + + tsl2772_chip_off(indio_dev); +} + /** * tsl2772_invoke_change - power cycle the device to implement the user * parameters @@ -1797,20 +1767,22 @@ static int tsl2772_probe(struct i2c_client *clientp, chip->client = clientp; i2c_set_clientdata(clientp, indio_dev); - chip->vddio_supply = tsl2772_get_regulator(chip, "vddio"); - if (IS_ERR(chip->vddio_supply)) - return PTR_ERR(chip->vddio_supply); + chip->reg[TSL2772_SUPPLY_VDD].supply = "vdd"; + chip->reg[TSL2772_SUPPLY_VDDIO].supply = "vddio"; - chip->vdd_supply = tsl2772_get_regulator(chip, "vdd"); - if (IS_ERR(chip->vdd_supply)) { - regulator_disable(chip->vddio_supply); - return PTR_ERR(chip->vdd_supply); - } + ret = devm_regulator_bulk_get(&clientp->dev, ARRAY_SIZE(chip->reg), + chip->reg); + if (ret < 0) + return ret; + + ret = regulator_bulk_enable(ARRAY_SIZE(chip->reg), chip->reg); + if (ret < 0) + return ret; - ret = devm_add_action(&clientp->dev, tsl2772_disable_regulators_action, - chip); + ret = devm_add_action_or_reset(&clientp->dev, + tsl2772_disable_regulators_action, + chip); if (ret < 0) { - tsl2772_disable_regulators_action(chip); dev_err(&clientp->dev, "Failed to setup regulator cleanup action %d\n", ret); return ret; @@ -1877,15 +1849,14 @@ static int tsl2772_probe(struct i2c_client *clientp, if (ret < 0) return ret; - ret = iio_device_register(indio_dev); - if (ret) { - tsl2772_chip_off(indio_dev); - dev_err(&clientp->dev, - "%s: iio registration failed\n", __func__); + ret = devm_add_action_or_reset(&clientp->dev, + tsl2772_chip_off_action, + indio_dev); + + if (ret < 0) return ret; - } - return 0; + return devm_iio_device_register(&clientp->dev, indio_dev); } static int tsl2772_suspend(struct device *dev) @@ -1895,8 +1866,7 @@ static int tsl2772_suspend(struct device *dev) int ret; ret = tsl2772_chip_off(indio_dev); - regulator_disable(chip->vdd_supply); - regulator_disable(chip->vddio_supply); + regulator_bulk_disable(ARRAY_SIZE(chip->reg), chip->reg); return ret; } @@ -1907,32 +1877,15 @@ static int tsl2772_resume(struct device *dev) struct tsl2772_chip *chip = iio_priv(indio_dev); int ret; - ret = tsl2772_enable_regulator(chip, chip->vddio_supply); + ret = regulator_bulk_enable(ARRAY_SIZE(chip->reg), chip->reg); if (ret < 0) return ret; - ret = tsl2772_enable_regulator(chip, chip->vdd_supply); - if (ret < 0) { - regulator_disable(chip->vddio_supply); - return ret; - } - usleep_range(TSL2772_BOOT_MIN_SLEEP_TIME, TSL2772_BOOT_MAX_SLEEP_TIME); return tsl2772_chip_on(indio_dev); } -static int tsl2772_remove(struct i2c_client *client) -{ - struct iio_dev *indio_dev = i2c_get_clientdata(client); - - tsl2772_chip_off(indio_dev); - - iio_device_unregister(indio_dev); - - return 0; -} - static const struct i2c_device_id tsl2772_idtable[] = { { "tsl2571", tsl2571 }, { "tsl2671", tsl2671 }, @@ -1979,7 +1932,6 @@ static struct i2c_driver tsl2772_driver = { }, .id_table = tsl2772_idtable, .probe = tsl2772_probe, - .remove = tsl2772_remove, }; module_i2c_driver(tsl2772_driver);
Use devm_iio_device_register to simplify the code. Signed-off-by: Chuhong Yuan <hslester96@gmail.com> --- Changes in v2: - Use regulator_bulk_() to shrink driver size. - Utilize more devm functions to simplify the code. - Remove several redundant functions. drivers/iio/light/tsl2772.c | 116 +++++++++++------------------------- 1 file changed, 34 insertions(+), 82 deletions(-)