Create a managed API wrapper to get all the bulk clocks and enable them
as it is a very common practice in many drivers. The second patch uses
this API to adapt to clk_bulk_* APIs in the exynos driver.
v1:
- https://lore.kernel.org/lkml/[email protected]/
v2:
- https://lore.kernel.org/lkml/[email protected]/
- Addressed Manivannan's comments to improve patch
v3:
- https://lore.kernel.org/all/[email protected]/
- Took Marek's suggestion to make a common bulk clk wrapper and use it in
the exynos driver
v4:
- Addressed Alim and Manivannan's comments
- Changed enabled->enable and disabled->disable in function name
- Remove num_clks out parameter as it is not required by user
- Removed exit callback and used function name directly in release
Shradha Todi (2):
clk: Provide managed helper to get and enable bulk clocks
PCI: exynos: Adapt to clk_bulk_* APIs
drivers/clk/clk-devres.c | 40 ++++++++++++++++++
drivers/pci/controller/dwc/pci-exynos.c | 54 ++-----------------------
include/linux/clk.h | 24 +++++++++++
3 files changed, 68 insertions(+), 50 deletions(-)
--
2.17.1
Provide a managed devm_clk_bulk* wrapper to get and enable all
bulk clocks in order to simplify drivers that keeps all clocks
enabled for the time of driver operation.
Suggested-by: Marek Szyprowski <[email protected]>
Signed-off-by: Shradha Todi <[email protected]>
---
drivers/clk/clk-devres.c | 40 ++++++++++++++++++++++++++++++++++++++++
include/linux/clk.h | 24 ++++++++++++++++++++++++
2 files changed, 64 insertions(+)
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 4fb4fd4b06bd..cbbd2cc339c3 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -182,6 +182,46 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
}
EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
+static void devm_clk_bulk_release_all_enable(struct device *dev, void *res)
+{
+ struct clk_bulk_devres *devres = res;
+
+ clk_bulk_disable_unprepare(devres->num_clks, devres->clks);
+ clk_bulk_put_all(devres->num_clks, devres->clks);
+}
+
+int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
+ struct clk_bulk_data **clks)
+{
+ struct clk_bulk_devres *devres;
+ int ret;
+
+ devres = devres_alloc(devm_clk_bulk_release_all_enable,
+ sizeof(*devres), GFP_KERNEL);
+ if (!devres)
+ return -ENOMEM;
+
+ ret = clk_bulk_get_all(dev, &devres->clks);
+ if (ret > 0) {
+ *clks = devres->clks;
+ devres->num_clks = ret;
+ } else {
+ devres_free(devres);
+ return ret;
+ }
+
+ ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
+ if (!ret) {
+ devres_add(dev, devres);
+ } else {
+ clk_bulk_put_all(devres->num_clks, devres->clks);
+ devres_free(devres);
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
+
static int devm_clk_match(struct device *dev, void *res, void *data)
{
struct clk **c = res;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1ef013324237..a005e709b7bd 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -438,6 +438,23 @@ int __must_check devm_clk_bulk_get_optional(struct device *dev, int num_clks,
int __must_check devm_clk_bulk_get_all(struct device *dev,
struct clk_bulk_data **clks);
+/**
+ * devm_clk_bulk_get_all_enable - managed get multiple clk consumers and
+ * enable all clks
+ * @dev: device for clock "consumer"
+ * @clks: pointer to the clk_bulk_data table of consumer
+ *
+ * Returns success (0) or negative errno.
+ *
+ * This helper function allows drivers to get several clk
+ * consumers and enable all of them in one operation with management.
+ * The clks will automatically be disabled and freed when the device
+ * is unbound.
+ */
+
+int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
+ struct clk_bulk_data **clks);
+
/**
* devm_clk_get - lookup and obtain a managed reference to a clock producer.
* @dev: device for clock "consumer"
@@ -960,6 +977,13 @@ static inline int __must_check devm_clk_bulk_get_all(struct device *dev,
return 0;
}
+static inline int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
+ struct clk_bulk_data **clks)
+{
+
+ return 0;
+}
+
static inline struct clk *devm_get_clk_from_child(struct device *dev,
struct device_node *np, const char *con_id)
{
--
2.17.1
There is no need to hardcode the clock info in the driver as driver can
rely on the devicetree to supply the clocks required for the functioning
of the peripheral. Get rid of the static clock info and obtain the
platform supplied clocks. All the clocks supplied is obtained and enabled
using the devm_clk_bulk_get_all_enable() API.
Signed-off-by: Shradha Todi <[email protected]>
---
drivers/pci/controller/dwc/pci-exynos.c | 54 ++-----------------------
1 file changed, 4 insertions(+), 50 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index ec5611005566..3234eb5be1fb 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -54,43 +54,11 @@
struct exynos_pcie {
struct dw_pcie pci;
void __iomem *elbi_base;
- struct clk *clk;
- struct clk *bus_clk;
+ struct clk_bulk_data *clks;
struct phy *phy;
struct regulator_bulk_data supplies[2];
};
-static int exynos_pcie_init_clk_resources(struct exynos_pcie *ep)
-{
- struct device *dev = ep->pci.dev;
- int ret;
-
- ret = clk_prepare_enable(ep->clk);
- if (ret) {
- dev_err(dev, "cannot enable pcie rc clock");
- return ret;
- }
-
- ret = clk_prepare_enable(ep->bus_clk);
- if (ret) {
- dev_err(dev, "cannot enable pcie bus clock");
- goto err_bus_clk;
- }
-
- return 0;
-
-err_bus_clk:
- clk_disable_unprepare(ep->clk);
-
- return ret;
-}
-
-static void exynos_pcie_deinit_clk_resources(struct exynos_pcie *ep)
-{
- clk_disable_unprepare(ep->bus_clk);
- clk_disable_unprepare(ep->clk);
-}
-
static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
{
writel(val, base + reg);
@@ -332,17 +300,9 @@ static int exynos_pcie_probe(struct platform_device *pdev)
if (IS_ERR(ep->elbi_base))
return PTR_ERR(ep->elbi_base);
- ep->clk = devm_clk_get(dev, "pcie");
- if (IS_ERR(ep->clk)) {
- dev_err(dev, "Failed to get pcie rc clock\n");
- return PTR_ERR(ep->clk);
- }
-
- ep->bus_clk = devm_clk_get(dev, "pcie_bus");
- if (IS_ERR(ep->bus_clk)) {
- dev_err(dev, "Failed to get pcie bus clock\n");
- return PTR_ERR(ep->bus_clk);
- }
+ ret = devm_clk_bulk_get_all_enable(dev, &ep->clks);
+ if (ret < 0)
+ return ret;
ep->supplies[0].supply = "vdd18";
ep->supplies[1].supply = "vdd10";
@@ -351,10 +311,6 @@ static int exynos_pcie_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = exynos_pcie_init_clk_resources(ep);
- if (ret)
- return ret;
-
ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
if (ret)
return ret;
@@ -369,7 +325,6 @@ static int exynos_pcie_probe(struct platform_device *pdev)
fail_probe:
phy_exit(ep->phy);
- exynos_pcie_deinit_clk_resources(ep);
regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
return ret;
@@ -383,7 +338,6 @@ static int __exit exynos_pcie_remove(struct platform_device *pdev)
exynos_pcie_assert_core_reset(ep);
phy_power_off(ep->phy);
phy_exit(ep->phy);
- exynos_pcie_deinit_clk_resources(ep);
regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
return 0;
--
2.17.1
On Wed, Jan 24, 2024 at 04:08:37PM +0530, Shradha Todi wrote:
> Provide a managed devm_clk_bulk* wrapper to get and enable all
> bulk clocks in order to simplify drivers that keeps all clocks
> enabled for the time of driver operation.
>
> Suggested-by: Marek Szyprowski <[email protected]>
> Signed-off-by: Shradha Todi <[email protected]>
> ---
> drivers/clk/clk-devres.c | 40 ++++++++++++++++++++++++++++++++++++++++
> include/linux/clk.h | 24 ++++++++++++++++++++++++
> 2 files changed, 64 insertions(+)
>
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 4fb4fd4b06bd..cbbd2cc339c3 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -182,6 +182,46 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
>
> +static void devm_clk_bulk_release_all_enable(struct device *dev, void *res)
> +{
> + struct clk_bulk_devres *devres = res;
> +
> + clk_bulk_disable_unprepare(devres->num_clks, devres->clks);
> + clk_bulk_put_all(devres->num_clks, devres->clks);
> +}
> +
> +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> + struct clk_bulk_data **clks)
> +{
> + struct clk_bulk_devres *devres;
> + int ret;
> +
> + devres = devres_alloc(devm_clk_bulk_release_all_enable,
> + sizeof(*devres), GFP_KERNEL);
> + if (!devres)
> + return -ENOMEM;
> +
> + ret = clk_bulk_get_all(dev, &devres->clks);
> + if (ret > 0) {
> + *clks = devres->clks;
> + devres->num_clks = ret;
> + } else {
> + devres_free(devres);
> + return ret;
> + }
How about:
ret = clk_bulk_get_all(dev, &devres->clks);
if (ret <= 0) {
devres_free(devres);
return ret;
}
*clks = devres->clks;
devres->num_clks = ret;
Even though this patch follows the pattern used by the rest of the APIs in the
driver, IMO above makes it more readable.
> +
> + ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> + if (!ret) {
> + devres_add(dev, devres);
> + } else {
> + clk_bulk_put_all(devres->num_clks, devres->clks);
> + devres_free(devres);
> + }
> +
Same as above:
ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
if (ret) {
clk_bulk_put_all(devres->num_clks, devres->clks);
devres_free(devres);
return ret;
}
devres_add(dev, devres);
> + return ret;
return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
> +
> static int devm_clk_match(struct device *dev, void *res, void *data)
> {
> struct clk **c = res;
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1ef013324237..a005e709b7bd 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -438,6 +438,23 @@ int __must_check devm_clk_bulk_get_optional(struct device *dev, int num_clks,
> int __must_check devm_clk_bulk_get_all(struct device *dev,
> struct clk_bulk_data **clks);
>
> +/**
> + * devm_clk_bulk_get_all_enable - managed get multiple clk consumers and
> + * enable all clks
"Get and enable all clocks of the consumer (managed)"
> + * @dev: device for clock "consumer"
> + * @clks: pointer to the clk_bulk_data table of consumer
> + *
> + * Returns success (0) or negative errno.
> + *
> + * This helper function allows drivers to get several clk
"This helper function allows drivers to get all clocks of the consumer and
enables them..."
- Mani
> + * consumers and enable all of them in one operation with management.
> + * The clks will automatically be disabled and freed when the device
> + * is unbound.
> + */
> +
> +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> + struct clk_bulk_data **clks);
> +
> /**
> * devm_clk_get - lookup and obtain a managed reference to a clock producer.
> * @dev: device for clock "consumer"
> @@ -960,6 +977,13 @@ static inline int __must_check devm_clk_bulk_get_all(struct device *dev,
> return 0;
> }
>
> +static inline int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> + struct clk_bulk_data **clks)
> +{
> +
> + return 0;
> +}
> +
> static inline struct clk *devm_get_clk_from_child(struct device *dev,
> struct device_node *np, const char *con_id)
> {
> --
> 2.17.1
>
--
மணிவண்ணன் சதாசிவம்
On Wed, Jan 24, 2024 at 04:08:38PM +0530, Shradha Todi wrote:
> There is no need to hardcode the clock info in the driver as driver can
> rely on the devicetree to supply the clocks required for the functioning
> of the peripheral. Get rid of the static clock info and obtain the
> platform supplied clocks. All the clocks supplied is obtained and enabled
> using the devm_clk_bulk_get_all_enable() API.
>
> Signed-off-by: Shradha Todi <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
> drivers/pci/controller/dwc/pci-exynos.c | 54 ++-----------------------
> 1 file changed, 4 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index ec5611005566..3234eb5be1fb 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -54,43 +54,11 @@
> struct exynos_pcie {
> struct dw_pcie pci;
> void __iomem *elbi_base;
> - struct clk *clk;
> - struct clk *bus_clk;
> + struct clk_bulk_data *clks;
> struct phy *phy;
> struct regulator_bulk_data supplies[2];
> };
>
> -static int exynos_pcie_init_clk_resources(struct exynos_pcie *ep)
> -{
> - struct device *dev = ep->pci.dev;
> - int ret;
> -
> - ret = clk_prepare_enable(ep->clk);
> - if (ret) {
> - dev_err(dev, "cannot enable pcie rc clock");
> - return ret;
> - }
> -
> - ret = clk_prepare_enable(ep->bus_clk);
> - if (ret) {
> - dev_err(dev, "cannot enable pcie bus clock");
> - goto err_bus_clk;
> - }
> -
> - return 0;
> -
> -err_bus_clk:
> - clk_disable_unprepare(ep->clk);
> -
> - return ret;
> -}
> -
> -static void exynos_pcie_deinit_clk_resources(struct exynos_pcie *ep)
> -{
> - clk_disable_unprepare(ep->bus_clk);
> - clk_disable_unprepare(ep->clk);
> -}
> -
> static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
> {
> writel(val, base + reg);
> @@ -332,17 +300,9 @@ static int exynos_pcie_probe(struct platform_device *pdev)
> if (IS_ERR(ep->elbi_base))
> return PTR_ERR(ep->elbi_base);
>
> - ep->clk = devm_clk_get(dev, "pcie");
> - if (IS_ERR(ep->clk)) {
> - dev_err(dev, "Failed to get pcie rc clock\n");
> - return PTR_ERR(ep->clk);
> - }
> -
> - ep->bus_clk = devm_clk_get(dev, "pcie_bus");
> - if (IS_ERR(ep->bus_clk)) {
> - dev_err(dev, "Failed to get pcie bus clock\n");
> - return PTR_ERR(ep->bus_clk);
> - }
> + ret = devm_clk_bulk_get_all_enable(dev, &ep->clks);
> + if (ret < 0)
> + return ret;
>
> ep->supplies[0].supply = "vdd18";
> ep->supplies[1].supply = "vdd10";
> @@ -351,10 +311,6 @@ static int exynos_pcie_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - ret = exynos_pcie_init_clk_resources(ep);
> - if (ret)
> - return ret;
> -
> ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
> if (ret)
> return ret;
> @@ -369,7 +325,6 @@ static int exynos_pcie_probe(struct platform_device *pdev)
>
> fail_probe:
> phy_exit(ep->phy);
> - exynos_pcie_deinit_clk_resources(ep);
> regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
>
> return ret;
> @@ -383,7 +338,6 @@ static int __exit exynos_pcie_remove(struct platform_device *pdev)
> exynos_pcie_assert_core_reset(ep);
> phy_power_off(ep->phy);
> phy_exit(ep->phy);
> - exynos_pcie_deinit_clk_resources(ep);
> regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
>
> return 0;
> --
> 2.17.1
>
--
மணிவண்ணன் சதாசிவம்
Hi Shradha
> -----Original Message-----
> From: Shradha Todi <[email protected]>
> Sent: Wednesday, January 24, 2024 4:09 PM
> To: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-samsung-
> [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Shradha Todi <[email protected]>
> Subject: [PATCH v4 1/2] clk: Provide managed helper to get and enable bulk
> clocks
>
> Provide a managed devm_clk_bulk* wrapper to get and enable all bulk clocks
> in order to simplify drivers that keeps all clocks enabled for the time of driver
> operation.
>
> Suggested-by: Marek Szyprowski <[email protected]>
> Signed-off-by: Shradha Todi <[email protected]>
> ---
Reviewed-by: Alim Akhtar <[email protected]>
> drivers/clk/clk-devres.c | 40
> ++++++++++++++++++++++++++++++++++++++++
> include/linux/clk.h | 24 ++++++++++++++++++++++++
> 2 files changed, 64 insertions(+)
>
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index
> 4fb4fd4b06bd..cbbd2cc339c3 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -182,6 +182,46 @@ int __must_check devm_clk_bulk_get_all(struct
> device *dev, } EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
>
> +static void devm_clk_bulk_release_all_enable(struct device *dev, void
> +*res) {
> + struct clk_bulk_devres *devres = res;
> +
> + clk_bulk_disable_unprepare(devres->num_clks, devres->clks);
> + clk_bulk_put_all(devres->num_clks, devres->clks); }
> +
> +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> + struct clk_bulk_data **clks) {
> + struct clk_bulk_devres *devres;
> + int ret;
> +
> + devres = devres_alloc(devm_clk_bulk_release_all_enable,
> + sizeof(*devres), GFP_KERNEL);
> + if (!devres)
> + return -ENOMEM;
> +
> + ret = clk_bulk_get_all(dev, &devres->clks);
> + if (ret > 0) {
> + *clks = devres->clks;
> + devres->num_clks = ret;
> + } else {
> + devres_free(devres);
> + return ret;
> + }
> +
> + ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> + if (!ret) {
> + devres_add(dev, devres);
> + } else {
> + clk_bulk_put_all(devres->num_clks, devres->clks);
> + devres_free(devres);
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
> +
> static int devm_clk_match(struct device *dev, void *res, void *data) {
> struct clk **c = res;
> diff --git a/include/linux/clk.h b/include/linux/clk.h index
> 1ef013324237..a005e709b7bd 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -438,6 +438,23 @@ int __must_check
> devm_clk_bulk_get_optional(struct device *dev, int num_clks, int
> __must_check devm_clk_bulk_get_all(struct device *dev,
> struct clk_bulk_data **clks);
>
> +/**
> + * devm_clk_bulk_get_all_enable - managed get multiple clk consumers
> and
> + * enable all clks
> + * @dev: device for clock "consumer"
> + * @clks: pointer to the clk_bulk_data table of consumer
> + *
> + * Returns success (0) or negative errno.
> + *
> + * This helper function allows drivers to get several clk
> + * consumers and enable all of them in one operation with management.
> + * The clks will automatically be disabled and freed when the device
> + * is unbound.
> + */
> +
> +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> + struct clk_bulk_data **clks);
> +
> /**
> * devm_clk_get - lookup and obtain a managed reference to a clock
> producer.
> * @dev: device for clock "consumer"
> @@ -960,6 +977,13 @@ static inline int __must_check
> devm_clk_bulk_get_all(struct device *dev,
> return 0;
> }
>
> +static inline int __must_check devm_clk_bulk_get_all_enable(struct device
> *dev,
> + struct clk_bulk_data **clks)
> +{
> +
> + return 0;
> +}
> +
> static inline struct clk *devm_get_clk_from_child(struct device *dev,
> struct device_node *np, const char *con_id)
> {
> --
> 2.17.1
Hello Shradha
> -----Original Message-----
> From: Shradha Todi <[email protected]>
> Sent: Wednesday, January 24, 2024 4:09 PM
> To: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-samsung-
> [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Shradha Todi <[email protected]>
> Subject: [PATCH v4 2/2] PCI: exynos: Adapt to clk_bulk_* APIs
>
> There is no need to hardcode the clock info in the driver as driver can rely on
> the devicetree to supply the clocks required for the functioning of the
> peripheral. Get rid of the static clock info and obtain the platform supplied
> clocks. All the clocks supplied is obtained and enabled using the
> devm_clk_bulk_get_all_enable() API.
>
> Signed-off-by: Shradha Todi <[email protected]>
> ---
Reviewed-by: Alim Akhtar <[email protected]>
> drivers/pci/controller/dwc/pci-exynos.c | 54 ++-----------------------
> 1 file changed, 4 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-exynos.c
> b/drivers/pci/controller/dwc/pci-exynos.c
> index ec5611005566..3234eb5be1fb 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -54,43 +54,11 @@
> struct exynos_pcie {
> struct dw_pcie pci;
> void __iomem *elbi_base;
> - struct clk *clk;
> - struct clk *bus_clk;
> + struct clk_bulk_data *clks;
> struct phy *phy;
> struct regulator_bulk_data supplies[2];
> };
>
> -static int exynos_pcie_init_clk_resources(struct exynos_pcie *ep) -{
> - struct device *dev = ep->pci.dev;
> - int ret;
> -
> - ret = clk_prepare_enable(ep->clk);
> - if (ret) {
> - dev_err(dev, "cannot enable pcie rc clock");
> - return ret;
> - }
> -
> - ret = clk_prepare_enable(ep->bus_clk);
> - if (ret) {
> - dev_err(dev, "cannot enable pcie bus clock");
> - goto err_bus_clk;
> - }
> -
> - return 0;
> -
> -err_bus_clk:
> - clk_disable_unprepare(ep->clk);
> -
> - return ret;
> -}
> -
> -static void exynos_pcie_deinit_clk_resources(struct exynos_pcie *ep) -{
> - clk_disable_unprepare(ep->bus_clk);
> - clk_disable_unprepare(ep->clk);
> -}
> -
> static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg) {
> writel(val, base + reg);
> @@ -332,17 +300,9 @@ static int exynos_pcie_probe(struct platform_device
> *pdev)
> if (IS_ERR(ep->elbi_base))
> return PTR_ERR(ep->elbi_base);
>
> - ep->clk = devm_clk_get(dev, "pcie");
> - if (IS_ERR(ep->clk)) {
> - dev_err(dev, "Failed to get pcie rc clock\n");
> - return PTR_ERR(ep->clk);
> - }
> -
> - ep->bus_clk = devm_clk_get(dev, "pcie_bus");
> - if (IS_ERR(ep->bus_clk)) {
> - dev_err(dev, "Failed to get pcie bus clock\n");
> - return PTR_ERR(ep->bus_clk);
> - }
> + ret = devm_clk_bulk_get_all_enable(dev, &ep->clks);
> + if (ret < 0)
> + return ret;
>
> ep->supplies[0].supply = "vdd18";
> ep->supplies[1].supply = "vdd10";
> @@ -351,10 +311,6 @@ static int exynos_pcie_probe(struct platform_device
> *pdev)
> if (ret)
> return ret;
>
> - ret = exynos_pcie_init_clk_resources(ep);
> - if (ret)
> - return ret;
> -
> ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep-
> >supplies);
> if (ret)
> return ret;
> @@ -369,7 +325,6 @@ static int exynos_pcie_probe(struct platform_device
> *pdev)
>
> fail_probe:
> phy_exit(ep->phy);
> - exynos_pcie_deinit_clk_resources(ep);
> regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
>
> return ret;
> @@ -383,7 +338,6 @@ static int __exit exynos_pcie_remove(struct
> platform_device *pdev)
> exynos_pcie_assert_core_reset(ep);
> phy_power_off(ep->phy);
> phy_exit(ep->phy);
> - exynos_pcie_deinit_clk_resources(ep);
> regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
>
> return 0;
> --
> 2.17.1
> -----Original Message-----
> From: Manivannan Sadhasivam <[email protected]>
> Sent: 29 January 2024 12:25
> To: Shradha Todi <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-samsung-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v4 1/2] clk: Provide managed helper to get and enable bulk
> clocks
>
> On Wed, Jan 24, 2024 at 04:08:37PM +0530, Shradha Todi wrote:
> > Provide a managed devm_clk_bulk* wrapper to get and enable all bulk
> > clocks in order to simplify drivers that keeps all clocks enabled for
> > the time of driver operation.
> >
> > Suggested-by: Marek Szyprowski <[email protected]>
> > Signed-off-by: Shradha Todi <[email protected]>
> > ---
> > drivers/clk/clk-devres.c | 40
> ++++++++++++++++++++++++++++++++++++++++
> > include/linux/clk.h | 24 ++++++++++++++++++++++++
> > 2 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index
> > 4fb4fd4b06bd..cbbd2cc339c3 100644
> > --- a/drivers/clk/clk-devres.c
> > +++ b/drivers/clk/clk-devres.c
> > @@ -182,6 +182,46 @@ int __must_check devm_clk_bulk_get_all(struct
> > device *dev, } EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
> >
> > +static void devm_clk_bulk_release_all_enable(struct device *dev, void
> > +*res) {
> > + struct clk_bulk_devres *devres = res;
> > +
> > + clk_bulk_disable_unprepare(devres->num_clks, devres->clks);
> > + clk_bulk_put_all(devres->num_clks, devres->clks); }
> > +
> > +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> > + struct clk_bulk_data **clks) {
> > + struct clk_bulk_devres *devres;
> > + int ret;
> > +
> > + devres = devres_alloc(devm_clk_bulk_release_all_enable,
> > + sizeof(*devres), GFP_KERNEL);
> > + if (!devres)
> > + return -ENOMEM;
> > +
> > + ret = clk_bulk_get_all(dev, &devres->clks);
> > + if (ret > 0) {
> > + *clks = devres->clks;
> > + devres->num_clks = ret;
> > + } else {
> > + devres_free(devres);
> > + return ret;
> > + }
>
> How about:
>
> ret = clk_bulk_get_all(dev, &devres->clks);
> if (ret <= 0) {
> devres_free(devres);
> return ret;
> }
>
> *clks = devres->clks;
> devres->num_clks = ret;
>
> Even though this patch follows the pattern used by the rest of the APIs in the
> driver, IMO above makes it more readable.
>
Since I have usually seen that maintainers suggest to maintain the coding style of the file, I followed the same.
If you have a stronger reason to change this, please let me know
Marek, Michael, Stephen please let us know what do you think about this?
> > +
> > + ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> > + if (!ret) {
> > + devres_add(dev, devres);
> > + } else {
> > + clk_bulk_put_all(devres->num_clks, devres->clks);
> > + devres_free(devres);
> > + }
> > +
>
> Same as above:
>
> ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> if (ret) {
> clk_bulk_put_all(devres->num_clks, devres->clks);
> devres_free(devres);
> return ret;
> }
>
> devres_add(dev, devres);
>
> > + return ret;
>
> return 0;
>
Same as above
> > +}
> > +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
> > +
> > static int devm_clk_match(struct device *dev, void *res, void *data)
> > {
> > struct clk **c = res;
> > diff --git a/include/linux/clk.h b/include/linux/clk.h index
> > 1ef013324237..a005e709b7bd 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -438,6 +438,23 @@ int __must_check
> > devm_clk_bulk_get_optional(struct device *dev, int num_clks, int
> __must_check devm_clk_bulk_get_all(struct device *dev,
> > struct clk_bulk_data **clks);
> >
> > +/**
> > + * devm_clk_bulk_get_all_enable - managed get multiple clk consumers and
> > + * enable all clks
>
> "Get and enable all clocks of the consumer (managed)"
>
Will take this up in the next patchset
> > + * @dev: device for clock "consumer"
> > + * @clks: pointer to the clk_bulk_data table of consumer
> > + *
> > + * Returns success (0) or negative errno.
> > + *
> > + * This helper function allows drivers to get several clk
>
> "This helper function allows drivers to get all clocks of the consumer and enables
> them..."
>
> - Mani
>
Will take this up. Thanks for your review Mani!
> > + * consumers and enable all of them in one operation with management.
> > + * The clks will automatically be disabled and freed when the device
> > + * is unbound.
> > + */
> > +
> > +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> > + struct clk_bulk_data **clks);
> > +
> > /**
> > * devm_clk_get - lookup and obtain a managed reference to a clock producer.
> > * @dev: device for clock "consumer"
> > @@ -960,6 +977,13 @@ static inline int __must_check
> devm_clk_bulk_get_all(struct device *dev,
> > return 0;
> > }
> >
> > +static inline int __must_check devm_clk_bulk_get_all_enable(struct device
> *dev,
> > + struct clk_bulk_data **clks)
> > +{
> > +
> > + return 0;
> > +}
> > +
> > static inline struct clk *devm_get_clk_from_child(struct device *dev,
> > struct device_node *np, const char *con_id) {
> > --
> > 2.17.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்
On 02.02.2024 12:59, Shradha Todi wrote:
>> -----Original Message-----
>> From: Manivannan Sadhasivam<[email protected]>
>> Sent: 29 January 2024 12:25
>> To: Shradha Todi<[email protected]>
>> Subject: Re: [PATCH v4 1/2] clk: Provide managed helper to get and enable bulk
>> clocks
>>
>> On Wed, Jan 24, 2024 at 04:08:37PM +0530, Shradha Todi wrote:
>>> Provide a managed devm_clk_bulk* wrapper to get and enable all bulk
>>> clocks in order to simplify drivers that keeps all clocks enabled for
>>> the time of driver operation.
>>>
>>> Suggested-by: Marek Szyprowski<[email protected]>
>>> Signed-off-by: Shradha Todi<[email protected]>
>>> ---
>>> drivers/clk/clk-devres.c | 40
>> ++++++++++++++++++++++++++++++++++++++++
>>> include/linux/clk.h | 24 ++++++++++++++++++++++++
>>> 2 files changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index
>>> 4fb4fd4b06bd..cbbd2cc339c3 100644
>>> --- a/drivers/clk/clk-devres.c
>>> +++ b/drivers/clk/clk-devres.c
>>> @@ -182,6 +182,46 @@ int __must_check devm_clk_bulk_get_all(struct
>>> device *dev, } EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
>>>
>>> +static void devm_clk_bulk_release_all_enable(struct device *dev, void
>>> +*res) {
>>> + struct clk_bulk_devres *devres = res;
>>> +
>>> + clk_bulk_disable_unprepare(devres->num_clks, devres->clks);
>>> + clk_bulk_put_all(devres->num_clks, devres->clks); }
>>> +
>>> +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
>>> + struct clk_bulk_data **clks) {
>>> + struct clk_bulk_devres *devres;
>>> + int ret;
>>> +
>>> + devres = devres_alloc(devm_clk_bulk_release_all_enable,
>>> + sizeof(*devres), GFP_KERNEL);
>>> + if (!devres)
>>> + return -ENOMEM;
>>> +
>>> + ret = clk_bulk_get_all(dev, &devres->clks);
>>> + if (ret > 0) {
>>> + *clks = devres->clks;
>>> + devres->num_clks = ret;
>>> + } else {
>>> + devres_free(devres);
>>> + return ret;
>>> + }
>> How about:
>>
>> ret = clk_bulk_get_all(dev, &devres->clks);
>> if (ret <= 0) {
>> devres_free(devres);
>> return ret;
>> }
>>
>> *clks = devres->clks;
>> devres->num_clks = ret;
>>
>> Even though this patch follows the pattern used by the rest of the APIs in the
>> driver, IMO above makes it more readable.
>>
> Since I have usually seen that maintainers suggest to maintain the coding style of the file, I followed the same.
> If you have a stronger reason to change this, please let me know
> Marek, Michael, Stephen please let us know what do you think about this?
I suggest to keep the same style as is used in the modified file (if it
doesn't conflict with the rules enforced by checkpatch and kernel's
coding style).
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland