On 07/12/2024 07:16, Song Chen wrote: >>> } >>> - pdata->buck_gpios[i] = gpio; >>> + >>> + /* SET GPIO*/ >> >> What is a SET GPIO? >> >>> + snprintf(label, sizeof(label), "%s%d", "S5M8767 SET", i + 1); >> >> Why using "SET" as name, not the actual name it is used for? Buck DVS? > > from below snippets: > s5m8767_pmic_probe of drivers/regulator/s5m8767.c > ret = devm_gpio_request(&pdev->dev, pdata->buck_gpios[0], > "S5M8767 SET1"); > if (ret) > return ret; > > ret = devm_gpio_request(&pdev->dev, pdata->buck_gpios[1], > "S5M8767 SET2"); > if (ret) > return ret; > > ret = devm_gpio_request(&pdev->dev, pdata->buck_gpios[2], > "S5M8767 SET3"); Yeah, your code is fine. > > and arch/arm/boot/dts/samsung/exynos5250-spring.dts > > s5m8767,pmic-buck-dvs-gpios = <&gpd1 0 GPIO_ACTIVE_LOW>, /* DVS1 */ > <&gpd1 1 GPIO_ACTIVE_LOW>, /* DVS2 */ > <&gpd1 2 GPIO_ACTIVE_LOW>; /* DVS3 */ > > s5m8767,pmic-buck-ds-gpios = <&gpx2 3 GPIO_ACTIVE_LOW>, /* SET1 */ > <&gpx2 4 GPIO_ACTIVE_LOW>, /* SET2 */ > <&gpx2 5 GPIO_ACTIVE_LOW>; /* SET3 */ > >> >>> + gpiod_set_consumer_name(pdata->buck_gpios[i], label); >>> + gpiod_direction_output(pdata->buck_gpios[i], >>> + (pdata->buck_default_idx >> (2 - i)) & 0x1); >> >> This is not an equivalent code. You set values for GPIOs 0-1 even if >> requesting GPIO 2 fails. >> >> On which board did you test it? > > You are right ,it's not equivalent with original code, i will fix it. > but i have a question here: > > ret = devm_gpio_request(&pdev->dev, pdata->buck_gpios[0], > "S5M8767 SET1"); > if (ret) > return ret; > > ret = devm_gpio_request(&pdev->dev, pdata->buck_gpios[1], > "S5M8767 SET2"); > if (ret) > return ret; > > ret = devm_gpio_request(&pdev->dev, pdata->buck_gpios[2], > "S5M8767 SET3"); > if (ret) > return ret; > > if it fails to request buck_gpios[2] after successfully requests > buck_gpios[0] and buck_gpios[1], the probe fails as well, should it call > gpiod_put to return gpio resource? Aren't you using devm interface? Please read the API. You do not need to put anything, unless you use some other interface and I missed the point of the question. Best regards, Krzysztof
