On 6/15/2020 5:00 AM, Mark Brown wrote: > On Fri, Jun 12, 2020 at 04:19:16PM -0700, Wesley Cheng wrote: > >> +++ b/drivers/regulator/qcom_usb_vbus-regulator.c >> @@ -0,0 +1,147 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2020, The Linux Foundation. All rights reserved. >> + */ > > Please make the entire comment a C++ one so things look more > intentional. > Hi Mark, Sure, will do. >> +static int qcom_usb_vbus_enable(struct regulator_dev *rdev) >> +{ > >> +static int qcom_usb_vbus_disable(struct regulator_dev *rdev) >> +{ > >> +static int qcom_usb_vbus_is_enabled(struct regulator_dev *rdev) >> +{ > > These operations can all be replaced by regulator_is_enabled_regmap() > and friends. > Got it. This simplifies the driver a lot. Thanks for the tip. >> + init_data.constraints.valid_ops_mask |= REGULATOR_CHANGE_STATUS; > > No, this is broken - regulators should not override the constraints the > machine sets. > Understood. I decided to go with of_get_regulator_init_data() to initialize the init_data parameter. This should take care of the constraint settings. -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project