Hi all,
the main goal of this patchset is to simplify clk management code in
exynos5433 drivers by using clk bulk API. In order to achieve that,
patch #1 adds a new function to clk core, which dynamically allocates
clk_bulk_data array and fills its id fields.
Best regards,
Maciej Purski
Maciej Purski (8):
clk: Add clk_bulk_alloc functions
media: s5p-jpeg: Use bulk clk API
drm/exynos/decon: Use clk bulk API
drm/exynos/dsi: Use clk bulk API
drm/exynos: mic: Use clk bulk API
drm/exynos/hdmi: Use clk bulk API
[media] exynos-gsc: Use clk bulk API
[media] s5p-mfc: Use clk bulk API
drivers/clk/clk-bulk.c | 16 +++++
drivers/clk/clk-devres.c | 37 +++++++++--
drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 50 +++++----------
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 68 +++++++++-----------
drivers/gpu/drm/exynos/exynos_drm_mic.c | 44 +++++--------
drivers/gpu/drm/exynos/exynos_hdmi.c | 85 ++++++++-----------------
drivers/media/platform/exynos-gsc/gsc-core.c | 55 ++++++----------
drivers/media/platform/exynos-gsc/gsc-core.h | 2 +-
drivers/media/platform/s5p-jpeg/jpeg-core.c | 45 ++++++-------
drivers/media/platform/s5p-jpeg/jpeg-core.h | 2 +-
drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 6 +-
drivers/media/platform/s5p-mfc/s5p_mfc_pm.c | 41 +++++-------
include/linux/clk.h | 64 +++++++++++++++++++
13 files changed, 263 insertions(+), 252 deletions(-)
--
2.7.4
When a driver is going to use clk_bulk_get() function, it has to
initialize an array of clk_bulk_data, by filling its id fields.
Add a new function to the core, which dynamically allocates
clk_bulk_data array and fills its id fields. Add clk_bulk_free()
function, which frees the array allocated by clk_bulk_alloc() function.
Add a managed version of clk_bulk_alloc().
Signed-off-by: Maciej Purski <[email protected]>
---
drivers/clk/clk-bulk.c | 16 ++++++++++++
drivers/clk/clk-devres.c | 37 +++++++++++++++++++++++++---
include/linux/clk.h | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 113 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/clk-bulk.c b/drivers/clk/clk-bulk.c
index 4c10456..2f16941 100644
--- a/drivers/clk/clk-bulk.c
+++ b/drivers/clk/clk-bulk.c
@@ -19,6 +19,22 @@
#include <linux/clk.h>
#include <linux/device.h>
#include <linux/export.h>
+#include <linux/slab.h>
+
+struct clk_bulk_data *clk_bulk_alloc(int num_clocks, const char *const *clk_ids)
+{
+ struct clk_bulk_data *ptr;
+ int i;
+
+ ptr = kcalloc(num_clocks, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ for (i = 0; i < num_clocks; i++)
+ ptr[i].id = clk_ids[i];
+
+ return ptr;
+}
void clk_bulk_put(int num_clks, struct clk_bulk_data *clks)
{
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index d854e26..2115b97 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -9,6 +9,39 @@
#include <linux/export.h>
#include <linux/gfp.h>
+struct clk_bulk_devres {
+ struct clk_bulk_data *clks;
+ int num_clks;
+};
+
+static void devm_clk_alloc_release(struct device *dev, void *res)
+{
+ struct clk_bulk_devres *devres = res;
+
+ clk_bulk_free(devres->clks);
+}
+
+struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev, int num_clks,
+ const char *const *clk_ids)
+{
+ struct clk_bulk_data **ptr, *clk_bulk;
+
+ ptr = devres_alloc(devm_clk_alloc_release,
+ num_clks * sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ clk_bulk = clk_bulk_alloc(num_clks, clk_ids);
+ if (clk_bulk) {
+ *ptr = clk_bulk;
+ devres_add(dev, ptr);
+ } else {
+ devres_free(ptr);
+ }
+
+ return clk_bulk;
+}
+
static void devm_clk_release(struct device *dev, void *res)
{
clk_put(*(struct clk **)res);
@@ -34,10 +67,6 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
}
EXPORT_SYMBOL(devm_clk_get);
-struct clk_bulk_devres {
- struct clk_bulk_data *clks;
- int num_clks;
-};
static void devm_clk_bulk_release(struct device *dev, void *res)
{
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 4c4ef9f..7d66f41 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -15,6 +15,7 @@
#include <linux/err.h>
#include <linux/kernel.h>
#include <linux/notifier.h>
+#include <linux/slab.h>
struct device;
struct clk;
@@ -240,6 +241,52 @@ static inline void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
#endif
#ifdef CONFIG_HAVE_CLK
+
+/**
+ * clk_bulk_alloc - allocates an array of clk_bulk_data and fills their
+ * id field
+ * @num_clks: number of clk_bulk_data
+ * @clk_ids: array of clock consumer ID's
+ *
+ * This function allows drivers to dynamically create an array of clk_bulk_data
+ * and fill their id field in one operation. If successful, it allows calling
+ * clk_bulk_get on the pointer returned by this function.
+ *
+ * Returns a pointer to a clk_bulk_data array, or valid IS_ERR() condition
+ * containing errno.
+ */
+struct clk_bulk_data *clk_bulk_alloc(int num_clks, const char *const *clk_ids);
+
+/**
+ * devm_clk_bulk_alloc - allocates an array of clk_bulk_data and fills their
+ * id field
+ * @dev: device for clock "consumer"
+ * @num_clks: number of clk_bulk_data
+ * @clk_ids: array of clock consumer ID's
+ *
+ * This function allows drivers to dynamically create an array of clk_bulk_data
+ * and fill their id field in one operation with management, the array will
+ * automatically be freed when the device is unbound. If successful, it allows
+ * calling clk_bulk_get on the pointer returned by this function.
+ *
+ * Returns a pointer to a clk_bulk_data array, or valid IS_ERR() condition
+ * containing errno.
+ */
+struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev, int num_clks,
+ const char * const *clk_ids);
+
+/**
+ * clk_bulk_free - frees the array of clk_bulk_data
+ * @clks: pointer to clk_bulk_data array
+ *
+ * This function frees the array allocated by clk_bulk_data. It must be called
+ * when all clks are freed.
+ */
+static inline void clk_bulk_free(struct clk_bulk_data *clks)
+{
+ kfree(clks);
+}
+
/**
* clk_get - lookup and obtain a reference to a clock producer.
* @dev: device for clock "consumer"
@@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id);
#else /* !CONFIG_HAVE_CLK */
+static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks,
+ const char **clk_ids)
+{
+ return NULL;
+}
+
+static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev,
+ int num_clks,
+ const char **clk_ids)
+{
+ return NULL;
+}
+
+static inline void clk_bulk_free(struct clk_bulk_data *clks)
+{
+}
+
static inline struct clk *clk_get(struct device *dev, const char *id)
{
return NULL;
--
2.7.4
Using bulk clk functions simplifies the driver's code. Use devm_clk_bulk
functions instead of iterating over an array of clks.
Signed-off-by: Maciej Purski <[email protected]>
---
drivers/media/platform/s5p-jpeg/jpeg-core.c | 45 ++++++++++++-----------------
drivers/media/platform/s5p-jpeg/jpeg-core.h | 2 +-
2 files changed, 20 insertions(+), 27 deletions(-)
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 79b63da..681a515 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2903,7 +2903,7 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
{
struct s5p_jpeg *jpeg;
struct resource *res;
- int i, ret;
+ int ret;
/* JPEG IP abstraction struct */
jpeg = devm_kzalloc(&pdev->dev, sizeof(struct s5p_jpeg), GFP_KERNEL);
@@ -2938,15 +2938,16 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
}
/* clocks */
- for (i = 0; i < jpeg->variant->num_clocks; i++) {
- jpeg->clocks[i] = devm_clk_get(&pdev->dev,
- jpeg->variant->clk_names[i]);
- if (IS_ERR(jpeg->clocks[i])) {
- dev_err(&pdev->dev, "failed to get clock: %s\n",
- jpeg->variant->clk_names[i]);
- return PTR_ERR(jpeg->clocks[i]);
- }
- }
+ jpeg->clocks = devm_clk_bulk_alloc(&pdev->dev,
+ jpeg->variant->num_clocks,
+ jpeg->variant->clk_names);
+ if (IS_ERR(jpeg->clocks))
+ return PTR_ERR(jpeg->clocks);
+
+ ret = devm_clk_bulk_get(&pdev->dev, jpeg->variant->num_clocks,
+ jpeg->clocks);
+ if (ret < 0)
+ return ret;
/* v4l2 device */
ret = v4l2_device_register(&pdev->dev, &jpeg->v4l2_dev);
@@ -3047,7 +3048,6 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
static int s5p_jpeg_remove(struct platform_device *pdev)
{
struct s5p_jpeg *jpeg = platform_get_drvdata(pdev);
- int i;
pm_runtime_disable(jpeg->dev);
@@ -3058,8 +3058,8 @@ static int s5p_jpeg_remove(struct platform_device *pdev)
v4l2_device_unregister(&jpeg->v4l2_dev);
if (!pm_runtime_status_suspended(&pdev->dev)) {
- for (i = jpeg->variant->num_clocks - 1; i >= 0; i--)
- clk_disable_unprepare(jpeg->clocks[i]);
+ clk_bulk_disable_unprepare(jpeg->variant->num_clocks,
+ jpeg->clocks);
}
return 0;
@@ -3069,10 +3069,8 @@ static int s5p_jpeg_remove(struct platform_device *pdev)
static int s5p_jpeg_runtime_suspend(struct device *dev)
{
struct s5p_jpeg *jpeg = dev_get_drvdata(dev);
- int i;
- for (i = jpeg->variant->num_clocks - 1; i >= 0; i--)
- clk_disable_unprepare(jpeg->clocks[i]);
+ clk_bulk_disable_unprepare(jpeg->variant->num_clocks, jpeg->clocks);
return 0;
}
@@ -3081,16 +3079,11 @@ static int s5p_jpeg_runtime_resume(struct device *dev)
{
struct s5p_jpeg *jpeg = dev_get_drvdata(dev);
unsigned long flags;
- int i, ret;
-
- for (i = 0; i < jpeg->variant->num_clocks; i++) {
- ret = clk_prepare_enable(jpeg->clocks[i]);
- if (ret) {
- while (--i >= 0)
- clk_disable_unprepare(jpeg->clocks[i]);
- return ret;
- }
- }
+ int ret;
+
+ ret = clk_bulk_prepare_enable(jpeg->variant->num_clocks, jpeg->clocks);
+ if (ret)
+ return ret;
spin_lock_irqsave(&jpeg->slock, flags);
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.h b/drivers/media/platform/s5p-jpeg/jpeg-core.h
index a46465e..dc6ed98 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.h
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.h
@@ -133,7 +133,7 @@ struct s5p_jpeg {
void __iomem *regs;
unsigned int irq;
enum exynos4_jpeg_result irq_ret;
- struct clk *clocks[JPEG_MAX_CLOCKS];
+ struct clk_bulk_data *clocks;
struct device *dev;
struct s5p_jpeg_variant *variant;
u32 irq_status;
--
2.7.4
Using bulk clk functions simplifies the driver's code. Use devm_clk_bulk
functions instead of iterating over an array of clks.
Signed-off-by: Maciej Purski <[email protected]>
---
drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 6 ++--
drivers/media/platform/s5p-mfc/s5p_mfc_pm.c | 41 +++++++++----------------
2 files changed, 18 insertions(+), 29 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 76119a8..da3f0b3 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -192,9 +192,9 @@ struct s5p_mfc_buf {
* struct s5p_mfc_pm - power management data structure
*/
struct s5p_mfc_pm {
- struct clk *clock_gate;
- const char * const *clk_names;
- struct clk *clocks[MFC_MAX_CLOCKS];
+ struct clk *clock_gate;
+ const char * const *clk_names;
+ struct clk_bulk_data *clocks;
int num_clocks;
bool use_clock_gating;
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c b/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
index eb85ced..857f6ea 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
@@ -24,7 +24,7 @@ static atomic_t clk_ref;
int s5p_mfc_init_pm(struct s5p_mfc_dev *dev)
{
- int i;
+ int ret;
pm = &dev->pm;
p_dev = dev;
@@ -35,17 +35,17 @@ int s5p_mfc_init_pm(struct s5p_mfc_dev *dev)
pm->clock_gate = NULL;
/* clock control */
- for (i = 0; i < pm->num_clocks; i++) {
- pm->clocks[i] = devm_clk_get(pm->device, pm->clk_names[i]);
- if (IS_ERR(pm->clocks[i])) {
- mfc_err("Failed to get clock: %s\n",
- pm->clk_names[i]);
- return PTR_ERR(pm->clocks[i]);
- }
- }
+ pm->clocks = devm_clk_bulk_alloc(pm->device, pm->num_clocks,
+ pm->clk_names);
+ if (IS_ERR(pm->clocks))
+ return PTR_ERR(pm->clocks);
+
+ ret = devm_clk_bulk_get(pm->device, pm->num_clocks, pm->clocks);
+ if (ret < 0)
+ return ret;
if (dev->variant->use_clock_gating)
- pm->clock_gate = pm->clocks[0];
+ pm->clock_gate = pm->clocks[0].clk;
pm_runtime_enable(pm->device);
atomic_set(&clk_ref, 0);
@@ -75,43 +75,32 @@ void s5p_mfc_clock_off(void)
int s5p_mfc_power_on(void)
{
- int i, ret = 0;
+ int ret = 0;
ret = pm_runtime_get_sync(pm->device);
if (ret < 0)
return ret;
/* clock control */
- for (i = 0; i < pm->num_clocks; i++) {
- ret = clk_prepare_enable(pm->clocks[i]);
- if (ret < 0) {
- mfc_err("clock prepare failed for clock: %s\n",
- pm->clk_names[i]);
- i++;
- goto err;
- }
- }
+ ret = clk_bulk_prepare_enable(pm->num_clocks, pm->clocks);
+ if (ret < 0)
+ goto err;
/* prepare for software clock gating */
clk_disable(pm->clock_gate);
return 0;
err:
- while (--i > 0)
- clk_disable_unprepare(pm->clocks[i]);
pm_runtime_put(pm->device);
return ret;
}
int s5p_mfc_power_off(void)
{
- int i;
-
/* finish software clock gating */
clk_enable(pm->clock_gate);
- for (i = 0; i < pm->num_clocks; i++)
- clk_disable_unprepare(pm->clocks[i]);
+ clk_bulk_disable_unprepare(pm->num_clocks, pm->clocks);
return pm_runtime_put_sync(pm->device);
}
--
2.7.4
Using bulk clk functions simplifies the driver's code. Use devm_clk_bulk
functions instead of iterating over an array of clks.
Signed-off-by: Maciej Purski <[email protected]>
---
drivers/gpu/drm/exynos/exynos_hdmi.c | 97 ++++++++++--------------------------
1 file changed, 27 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index a4b75a4..6c208f7 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -136,8 +136,8 @@ struct hdmi_context {
int irq;
struct regmap *pmureg;
struct regmap *sysreg;
- struct clk **clk_gates;
- struct clk **clk_muxes;
+ struct clk_bulk_data *clk_gates;
+ struct clk_bulk_data *clk_muxes;
struct regulator_bulk_data regul_bulk[ARRAY_SIZE(supply)];
struct regulator *reg_hdmi_en;
struct exynos_drm_clk phy_clk;
@@ -739,43 +739,16 @@ static int hdmiphy_reg_write_buf(struct hdmi_context *hdata,
}
}
-static int hdmi_clk_enable_gates(struct hdmi_context *hdata)
-{
- int i, ret;
-
- for (i = 0; i < hdata->drv_data->clk_gates.count; ++i) {
- ret = clk_prepare_enable(hdata->clk_gates[i]);
- if (!ret)
- continue;
-
- dev_err(hdata->dev, "Cannot enable clock '%s', %d\n",
- hdata->drv_data->clk_gates.data[i], ret);
- while (i--)
- clk_disable_unprepare(hdata->clk_gates[i]);
- return ret;
- }
-
- return 0;
-}
-
-static void hdmi_clk_disable_gates(struct hdmi_context *hdata)
-{
- int i = hdata->drv_data->clk_gates.count;
-
- while (i--)
- clk_disable_unprepare(hdata->clk_gates[i]);
-}
-
static int hdmi_clk_set_parents(struct hdmi_context *hdata, bool to_phy)
{
struct device *dev = hdata->dev;
int ret = 0;
+ struct clk_bulk_data *clk_muxes = hdata->clk_muxes;
int i;
for (i = 0; i < hdata->drv_data->clk_muxes.count; i += 3) {
- struct clk **c = &hdata->clk_muxes[i];
-
- ret = clk_set_parent(c[2], c[to_phy]);
+ ret = clk_set_parent(clk_muxes[i + 2].clk,
+ clk_muxes[i + to_phy].clk);
if (!ret)
continue;
@@ -1655,54 +1628,36 @@ static irqreturn_t hdmi_irq_thread(int irq, void *arg)
return IRQ_HANDLED;
}
-static int hdmi_clks_get(struct hdmi_context *hdata,
- const struct string_array_spec *names,
- struct clk **clks)
+static struct clk_bulk_data *hdmi_clks_alloc_get(struct hdmi_context *hdata,
+ const struct string_array_spec *names)
{
- struct device *dev = hdata->dev;
- int i;
-
- for (i = 0; i < names->count; ++i) {
- struct clk *clk = devm_clk_get(dev, names->data[i]);
-
- if (IS_ERR(clk)) {
- int ret = PTR_ERR(clk);
+ struct clk_bulk_data *ptr;
+ int ret;
- dev_err(dev, "Cannot get clock %s, %d\n",
- names->data[i], ret);
+ ptr = devm_clk_bulk_alloc(hdata->dev, names->count, names->data);
+ if (IS_ERR(ptr))
+ return ptr;
- return ret;
- }
-
- clks[i] = clk;
- }
+ ret = devm_clk_bulk_get(hdata->dev, names->count, ptr);
+ if (ret < 0)
+ return ERR_PTR(ret);
- return 0;
+ return ptr;
}
static int hdmi_clk_init(struct hdmi_context *hdata)
{
const struct hdmi_driver_data *drv_data = hdata->drv_data;
- int count = drv_data->clk_gates.count + drv_data->clk_muxes.count;
- struct device *dev = hdata->dev;
- struct clk **clks;
- int ret;
- if (!count)
- return 0;
-
- clks = devm_kzalloc(dev, sizeof(*clks) * count, GFP_KERNEL);
- if (!clks)
- return -ENOMEM;
+ hdata->clk_muxes = hdmi_clks_alloc_get(hdata, &drv_data->clk_muxes);
+ if (IS_ERR(hdata->clk_muxes))
+ return PTR_ERR(hdata->clk_muxes);
- hdata->clk_gates = clks;
- hdata->clk_muxes = clks + drv_data->clk_gates.count;
+ hdata->clk_gates = hdmi_clks_alloc_get(hdata, &drv_data->clk_gates);
+ if (IS_ERR(hdata->clk_gates))
+ return PTR_ERR(hdata->clk_gates);
- ret = hdmi_clks_get(hdata, &drv_data->clk_gates, hdata->clk_gates);
- if (ret)
- return ret;
-
- return hdmi_clks_get(hdata, &drv_data->clk_muxes, hdata->clk_muxes);
+ return 0;
}
@@ -2073,7 +2028,8 @@ static int __maybe_unused exynos_hdmi_suspend(struct device *dev)
{
struct hdmi_context *hdata = dev_get_drvdata(dev);
- hdmi_clk_disable_gates(hdata);
+ clk_bulk_disable_unprepare(hdata->drv_data->clk_gates.count,
+ hdata->clk_gates);
return 0;
}
@@ -2083,7 +2039,8 @@ static int __maybe_unused exynos_hdmi_resume(struct device *dev)
struct hdmi_context *hdata = dev_get_drvdata(dev);
int ret;
- ret = hdmi_clk_enable_gates(hdata);
+ ret = clk_bulk_prepare_enable(hdata->drv_data->clk_gates.count,
+ hdata->clk_gates);
if (ret < 0)
return ret;
--
2.7.4
Using bulk clk functions simplifies the driver's code. Use devm_clk_bulk
functions instead of iterating over an array of clks.
Signed-off-by: Maciej Purski <[email protected]>
---
drivers/media/platform/exynos-gsc/gsc-core.c | 55 ++++++++++------------------
drivers/media/platform/exynos-gsc/gsc-core.h | 2 +-
2 files changed, 20 insertions(+), 37 deletions(-)
diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 17854a3..fa7e993 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1149,7 +1149,6 @@ static int gsc_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
const struct gsc_driverdata *drv_data = of_device_get_match_data(dev);
int ret;
- int i;
gsc = devm_kzalloc(dev, sizeof(struct gsc_dev), GFP_KERNEL);
if (!gsc)
@@ -1187,25 +1186,19 @@ static int gsc_probe(struct platform_device *pdev)
return -ENXIO;
}
- for (i = 0; i < gsc->num_clocks; i++) {
- gsc->clock[i] = devm_clk_get(dev, drv_data->clk_names[i]);
- if (IS_ERR(gsc->clock[i])) {
- dev_err(dev, "failed to get clock: %s\n",
- drv_data->clk_names[i]);
- return PTR_ERR(gsc->clock[i]);
- }
- }
+ gsc->clocks = devm_clk_bulk_alloc(dev, gsc->num_clocks,
+ drv_data->clk_names);
+ if (IS_ERR(gsc->clocks))
+ return PTR_ERR(gsc->clocks);
- for (i = 0; i < gsc->num_clocks; i++) {
- ret = clk_prepare_enable(gsc->clock[i]);
- if (ret) {
- dev_err(dev, "clock prepare failed for clock: %s\n",
- drv_data->clk_names[i]);
- while (--i >= 0)
- clk_disable_unprepare(gsc->clock[i]);
- return ret;
- }
- }
+ ret = devm_clk_bulk_get(dev, gsc->num_clocks,
+ gsc->clocks);
+ if (ret)
+ return ret;
+
+ ret = clk_bulk_prepare_enable(gsc->num_clocks, gsc->clocks);
+ if (ret)
+ return ret;
ret = devm_request_irq(dev, res->start, gsc_irq_handler,
0, pdev->name, gsc);
@@ -1239,15 +1232,14 @@ static int gsc_probe(struct platform_device *pdev)
err_v4l2:
v4l2_device_unregister(&gsc->v4l2_dev);
err_clk:
- for (i = gsc->num_clocks - 1; i >= 0; i--)
- clk_disable_unprepare(gsc->clock[i]);
+ clk_bulk_disable_unprepare(gsc->num_clocks, gsc->clocks);
+
return ret;
}
static int gsc_remove(struct platform_device *pdev)
{
struct gsc_dev *gsc = platform_get_drvdata(pdev);
- int i;
pm_runtime_get_sync(&pdev->dev);
@@ -1255,8 +1247,7 @@ static int gsc_remove(struct platform_device *pdev)
v4l2_device_unregister(&gsc->v4l2_dev);
vb2_dma_contig_clear_max_seg_size(&pdev->dev);
- for (i = 0; i < gsc->num_clocks; i++)
- clk_disable_unprepare(gsc->clock[i]);
+ clk_bulk_disable_unprepare(gsc->num_clocks, gsc->clocks);
pm_runtime_put_noidle(&pdev->dev);
pm_runtime_disable(&pdev->dev);
@@ -1307,18 +1298,12 @@ static int gsc_runtime_resume(struct device *dev)
{
struct gsc_dev *gsc = dev_get_drvdata(dev);
int ret = 0;
- int i;
pr_debug("gsc%d: state: 0x%lx\n", gsc->id, gsc->state);
- for (i = 0; i < gsc->num_clocks; i++) {
- ret = clk_prepare_enable(gsc->clock[i]);
- if (ret) {
- while (--i >= 0)
- clk_disable_unprepare(gsc->clock[i]);
- return ret;
- }
- }
+ ret = clk_bulk_prepare_enable(gsc->num_clocks, gsc->clocks);
+ if (ret)
+ return ret;
gsc_hw_set_sw_reset(gsc);
gsc_wait_reset(gsc);
@@ -1331,14 +1316,12 @@ static int gsc_runtime_suspend(struct device *dev)
{
struct gsc_dev *gsc = dev_get_drvdata(dev);
int ret = 0;
- int i;
ret = gsc_m2m_suspend(gsc);
if (ret)
return ret;
- for (i = gsc->num_clocks - 1; i >= 0; i--)
- clk_disable_unprepare(gsc->clock[i]);
+ clk_bulk_disable_unprepare(gsc->num_clocks, gsc->clocks);
pr_debug("gsc%d: state: 0x%lx\n", gsc->id, gsc->state);
return ret;
diff --git a/drivers/media/platform/exynos-gsc/gsc-core.h b/drivers/media/platform/exynos-gsc/gsc-core.h
index 715d9c9d..08ff7b9 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.h
+++ b/drivers/media/platform/exynos-gsc/gsc-core.h
@@ -334,7 +334,7 @@ struct gsc_dev {
struct gsc_variant *variant;
u16 id;
int num_clocks;
- struct clk *clock[GSC_MAX_CLOCKS];
+ struct clk_bulk_data *clocks;
void __iomem *regs;
wait_queue_head_t irq_queue;
struct gsc_m2m_device m2m;
--
2.7.4
Using bulk clk functions simplifies the driver's code. Use devm_clk_bulk
functions instead of iterating over an array of clks.
In order to achieve consistency with other drivers, define clock names
in driver's variants structures.
Signed-off-by: Maciej Purski <[email protected]>
---
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 68 +++++++++++++++------------------
1 file changed, 30 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 7904ffa..46a8b5c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -209,11 +209,7 @@
#define DSI_XFER_TIMEOUT_MS 100
#define DSI_RX_FIFO_EMPTY 0x30800002
-#define OLD_SCLK_MIPI_CLK_NAME "pll_clk"
-
-static char *clk_names[5] = { "bus_clk", "sclk_mipi",
- "phyclk_mipidphy0_bitclkdiv8", "phyclk_mipidphy0_rxclkesc0",
- "sclk_rgb_vclk_to_dsim0" };
+#define DSI_MAX_CLOCKS 5
enum exynos_dsi_transfer_type {
EXYNOS_DSI_TX,
@@ -243,6 +239,7 @@ struct exynos_dsi_driver_data {
unsigned int plltmr_reg;
unsigned int has_freqband:1;
unsigned int has_clklane_stop:1;
+ const char *clock_names[DSI_MAX_CLOCKS];
unsigned int num_clks;
unsigned int max_freq;
unsigned int wait_for_reset;
@@ -259,7 +256,7 @@ struct exynos_dsi {
void __iomem *reg_base;
struct phy *phy;
- struct clk **clks;
+ struct clk_bulk_data *clks;
struct regulator_bulk_data supplies[2];
int irq;
int te_gpio;
@@ -453,6 +450,7 @@ static const struct exynos_dsi_driver_data exynos3_dsi_driver_data = {
.plltmr_reg = 0x50,
.has_freqband = 1,
.has_clklane_stop = 1,
+ .clock_names = {"bus_clk", "pll_clk"},
.num_clks = 2,
.max_freq = 1000,
.wait_for_reset = 1,
@@ -465,6 +463,7 @@ static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
.plltmr_reg = 0x50,
.has_freqband = 1,
.has_clklane_stop = 1,
+ .clock_names = {"bus_clk", "sclk_mipi"},
.num_clks = 2,
.max_freq = 1000,
.wait_for_reset = 1,
@@ -475,6 +474,7 @@ static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
.reg_ofs = exynos_reg_ofs,
.plltmr_reg = 0x58,
+ .clock_names = {"bus_clk", "pll_clk"},
.num_clks = 2,
.max_freq = 1000,
.wait_for_reset = 1,
@@ -486,6 +486,10 @@ static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
.reg_ofs = exynos5433_reg_ofs,
.plltmr_reg = 0xa0,
.has_clklane_stop = 1,
+ .clock_names = {"bus_clk", "phyclk_mipidphy0_bitclkdiv8",
+ "phyclk_mipidphy0_rxclkesc0",
+ "sclk_rgb_vclk_to_dsim0",
+ "sclk_mipi"},
.num_clks = 5,
.max_freq = 1500,
.wait_for_reset = 0,
@@ -497,6 +501,7 @@ static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {
.reg_ofs = exynos5433_reg_ofs,
.plltmr_reg = 0xa0,
.has_clklane_stop = 1,
+ .clock_names = {"bus_clk", "pll_clk"},
.num_clks = 2,
.max_freq = 1500,
.wait_for_reset = 1,
@@ -1711,7 +1716,7 @@ static int exynos_dsi_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct resource *res;
struct exynos_dsi *dsi;
- int ret, i;
+ int ret;
dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
if (!dsi)
@@ -1743,26 +1748,15 @@ static int exynos_dsi_probe(struct platform_device *pdev)
return -EPROBE_DEFER;
}
- dsi->clks = devm_kzalloc(dev,
- sizeof(*dsi->clks) * dsi->driver_data->num_clks,
- GFP_KERNEL);
- if (!dsi->clks)
- return -ENOMEM;
+ dsi->clks = devm_clk_bulk_alloc(dev, dsi->driver_data->num_clks,
+ dsi->driver_data->clock_names);
+ if (IS_ERR(dsi->clks))
+ return PTR_ERR(dsi->clks);
- for (i = 0; i < dsi->driver_data->num_clks; i++) {
- dsi->clks[i] = devm_clk_get(dev, clk_names[i]);
- if (IS_ERR(dsi->clks[i])) {
- if (strcmp(clk_names[i], "sclk_mipi") == 0) {
- strcpy(clk_names[i], OLD_SCLK_MIPI_CLK_NAME);
- i--;
- continue;
- }
-
- dev_info(dev, "failed to get the clock: %s\n",
- clk_names[i]);
- return PTR_ERR(dsi->clks[i]);
- }
- }
+ ret = devm_clk_bulk_get(dev, dsi->driver_data->num_clks,
+ dsi->clks);
+ if (ret < 0)
+ return ret;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dsi->reg_base = devm_ioremap_resource(dev, res);
@@ -1817,7 +1811,7 @@ static int __maybe_unused exynos_dsi_suspend(struct device *dev)
struct drm_encoder *encoder = dev_get_drvdata(dev);
struct exynos_dsi *dsi = encoder_to_dsi(encoder);
const struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
- int ret, i;
+ int ret;
usleep_range(10000, 20000);
@@ -1833,8 +1827,7 @@ static int __maybe_unused exynos_dsi_suspend(struct device *dev)
phy_power_off(dsi->phy);
- for (i = driver_data->num_clks - 1; i > -1; i--)
- clk_disable_unprepare(dsi->clks[i]);
+ clk_bulk_disable_unprepare(driver_data->num_clks, dsi->clks);
ret = regulator_bulk_disable(ARRAY_SIZE(dsi->supplies), dsi->supplies);
if (ret < 0)
@@ -1848,7 +1841,7 @@ static int __maybe_unused exynos_dsi_resume(struct device *dev)
struct drm_encoder *encoder = dev_get_drvdata(dev);
struct exynos_dsi *dsi = encoder_to_dsi(encoder);
const struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
- int ret, i;
+ int ret;
ret = regulator_bulk_enable(ARRAY_SIZE(dsi->supplies), dsi->supplies);
if (ret < 0) {
@@ -1856,23 +1849,22 @@ static int __maybe_unused exynos_dsi_resume(struct device *dev)
return ret;
}
- for (i = 0; i < driver_data->num_clks; i++) {
- ret = clk_prepare_enable(dsi->clks[i]);
- if (ret < 0)
- goto err_clk;
- }
+ ret = clk_bulk_prepare_enable(driver_data->num_clks, dsi->clks);
+ if (ret < 0)
+ goto err_clk;
ret = phy_power_on(dsi->phy);
if (ret < 0) {
dev_err(dsi->dev, "cannot enable phy %d\n", ret);
- goto err_clk;
+ goto err_phy;
}
return 0;
+err_phy:
+ clk_bulk_disable_unprepare(driver_data->num_clks, dsi->clks);
+
err_clk:
- while (--i > -1)
- clk_disable_unprepare(dsi->clks[i]);
regulator_bulk_disable(ARRAY_SIZE(dsi->supplies), dsi->supplies);
return ret;
--
2.7.4
Using bulk clk functions simplifies the driver's code. Use devm_clk_bulk
functions instead of iterating over an array of clks.
Signed-off-by: Maciej Purski <[email protected]>
---
drivers/gpu/drm/exynos/exynos_drm_mic.c | 41 +++++++++++----------------------
1 file changed, 14 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c
index 2174814..276558a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_mic.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
@@ -88,7 +88,7 @@
#define MIC_BS_SIZE_2D(x) ((x) & 0x3fff)
-static char *clk_names[] = { "pclk_mic0", "sclk_rgb_vclk_to_mic0" };
+static const char *const clk_names[] = { "pclk_mic0", "sclk_rgb_vclk_to_mic0" };
#define NUM_CLKS ARRAY_SIZE(clk_names)
static DEFINE_MUTEX(mic_mutex);
@@ -96,7 +96,7 @@ struct exynos_mic {
struct device *dev;
void __iomem *reg;
struct regmap *sysreg;
- struct clk *clks[NUM_CLKS];
+ struct clk_bulk_data *clks;
bool i80_mode;
struct videomode vm;
@@ -338,10 +338,8 @@ static const struct component_ops exynos_mic_component_ops = {
static int exynos_mic_suspend(struct device *dev)
{
struct exynos_mic *mic = dev_get_drvdata(dev);
- int i;
- for (i = NUM_CLKS - 1; i > -1; i--)
- clk_disable_unprepare(mic->clks[i]);
+ clk_bulk_disable_unprepare(NUM_CLKS, mic->clks);
return 0;
}
@@ -349,19 +347,8 @@ static int exynos_mic_suspend(struct device *dev)
static int exynos_mic_resume(struct device *dev)
{
struct exynos_mic *mic = dev_get_drvdata(dev);
- int ret, i;
-
- for (i = 0; i < NUM_CLKS; i++) {
- ret = clk_prepare_enable(mic->clks[i]);
- if (ret < 0) {
- DRM_ERROR("Failed to enable clock (%s)\n",
- clk_names[i]);
- while (--i > -1)
- clk_disable_unprepare(mic->clks[i]);
- return ret;
- }
- }
- return 0;
+
+ return clk_bulk_prepare_enable(NUM_CLKS, mic->clks);
}
#endif
@@ -374,7 +361,7 @@ static int exynos_mic_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct exynos_mic *mic;
struct resource res;
- int ret, i;
+ int ret;
mic = devm_kzalloc(dev, sizeof(*mic), GFP_KERNEL);
if (!mic) {
@@ -405,16 +392,16 @@ static int exynos_mic_probe(struct platform_device *pdev)
goto err;
}
- for (i = 0; i < NUM_CLKS; i++) {
- mic->clks[i] = devm_clk_get(dev, clk_names[i]);
- if (IS_ERR(mic->clks[i])) {
- DRM_ERROR("mic: Failed to get clock (%s)\n",
- clk_names[i]);
- ret = PTR_ERR(mic->clks[i]);
- goto err;
- }
+ mic->clks = devm_clk_bulk_alloc(dev, NUM_CLKS, clk_names);
+ if (IS_ERR(mic->clks)) {
+ ret = PTR_ERR(mic->clks);
+ goto err;
}
+ ret = devm_clk_bulk_get(dev, NUM_CLKS, mic->clks);
+ if (ret < 0)
+ goto err;
+
platform_set_drvdata(pdev, mic);
mic->bridge.funcs = &mic_bridge_funcs;
--
2.7.4
Using bulk clk functions simplifies the driver's code. Use devm_clk_bulk
functions instead of iterating over an array of clks.
Signed-off-by: Maciej Purski <[email protected]>
---
drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 50 ++++++++-------------------
1 file changed, 15 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index 1c330f2..1760fcb 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -55,7 +55,7 @@ struct decon_context {
struct exynos_drm_plane_config configs[WINDOWS_NR];
void __iomem *addr;
struct regmap *sysreg;
- struct clk *clks[ARRAY_SIZE(decon_clks_name)];
+ struct clk_bulk_data *clks;
unsigned int irq;
unsigned int irq_vsync;
unsigned int irq_lcd_sys;
@@ -485,15 +485,13 @@ static irqreturn_t decon_te_irq_handler(int irq, void *dev_id)
static void decon_clear_channels(struct exynos_drm_crtc *crtc)
{
struct decon_context *ctx = crtc->ctx;
- int win, i, ret;
+ int win, ret;
DRM_DEBUG_KMS("%s\n", __FILE__);
- for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
- ret = clk_prepare_enable(ctx->clks[i]);
- if (ret < 0)
- goto err;
- }
+ ret = clk_bulk_prepare_enable(ARRAY_SIZE(decon_clks_name), ctx->clks);
+ if (ret < 0)
+ return;
decon_shadow_protect(ctx, true);
for (win = 0; win < WINDOWS_NR; win++)
@@ -504,10 +502,6 @@ static void decon_clear_channels(struct exynos_drm_crtc *crtc)
/* TODO: wait for possible vsync */
msleep(50);
-
-err:
- while (--i >= 0)
- clk_disable_unprepare(ctx->clks[i]);
}
static enum drm_mode_status decon_mode_valid(struct exynos_drm_crtc *crtc,
@@ -638,10 +632,8 @@ static irqreturn_t decon_irq_handler(int irq, void *dev_id)
static int exynos5433_decon_suspend(struct device *dev)
{
struct decon_context *ctx = dev_get_drvdata(dev);
- int i = ARRAY_SIZE(decon_clks_name);
- while (--i >= 0)
- clk_disable_unprepare(ctx->clks[i]);
+ clk_bulk_disable_unprepare(ARRAY_SIZE(decon_clks_name), ctx->clks);
return 0;
}
@@ -649,19 +641,9 @@ static int exynos5433_decon_suspend(struct device *dev)
static int exynos5433_decon_resume(struct device *dev)
{
struct decon_context *ctx = dev_get_drvdata(dev);
- int i, ret;
-
- for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
- ret = clk_prepare_enable(ctx->clks[i]);
- if (ret < 0)
- goto err;
- }
-
- return 0;
+ int ret;
-err:
- while (--i >= 0)
- clk_disable_unprepare(ctx->clks[i]);
+ ret = clk_bulk_prepare_enable(ARRAY_SIZE(decon_clks_name), ctx->clks);
return ret;
}
@@ -719,7 +701,6 @@ static int exynos5433_decon_probe(struct platform_device *pdev)
struct decon_context *ctx;
struct resource *res;
int ret;
- int i;
ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -732,15 +713,14 @@ static int exynos5433_decon_probe(struct platform_device *pdev)
if (ctx->out_type & IFTYPE_HDMI)
ctx->first_win = 1;
- for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
- struct clk *clk;
-
- clk = devm_clk_get(ctx->dev, decon_clks_name[i]);
- if (IS_ERR(clk))
- return PTR_ERR(clk);
+ ctx->clks = devm_clk_bulk_alloc(dev, ARRAY_SIZE(decon_clks_name),
+ decon_clks_name);
+ if (IS_ERR(ctx->clks))
+ return PTR_ERR(ctx->clks);
- ctx->clks[i] = clk;
- }
+ ret = devm_clk_bulk_get(dev, ARRAY_SIZE(decon_clks_name), ctx->clks);
+ if (ret < 0)
+ return ret;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
ctx->addr = devm_ioremap_resource(dev, res);
--
2.7.4
Hi Maciej,
On 19/02/18 15:43, Maciej Purski wrote:
> When a driver is going to use clk_bulk_get() function, it has to
> initialize an array of clk_bulk_data, by filling its id fields.
>
> Add a new function to the core, which dynamically allocates
> clk_bulk_data array and fills its id fields. Add clk_bulk_free()
> function, which frees the array allocated by clk_bulk_alloc() function.
> Add a managed version of clk_bulk_alloc().
Seeing how every subsequent patch ends up with the roughly this same stanza:
x = devm_clk_bulk_alloc(dev, num, names);
if (IS_ERR(x)
return PTR_ERR(x);
ret = devm_clk_bulk_get(dev, x, num);
I wonder if it might be better to simply implement:
int devm_clk_bulk_alloc_get(dev, &x, num, names)
that does the whole lot in one go, and let drivers that want to do more
fiddly things continue to open-code the allocation.
But perhaps that's an abstraction too far... I'm not all that familiar
with the lie of the land here.
> Signed-off-by: Maciej Purski <[email protected]>
> ---
> drivers/clk/clk-bulk.c | 16 ++++++++++++
> drivers/clk/clk-devres.c | 37 +++++++++++++++++++++++++---
> include/linux/clk.h | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 113 insertions(+), 4 deletions(-)
>
[...]
> @@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id);
>
> #else /* !CONFIG_HAVE_CLK */
>
> +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks,
> + const char **clk_ids)
> +{
> + return NULL;
Either way, is it intentional not returning an ERR_PTR() value in this
case? Since NULL will pass an IS_ERR() check, it seems a bit fragile for
an allocation call to apparently succeed when the whole API is
configured out (and I believe introducing new uses of IS_ERR_OR_NULL()
is in general strongly discouraged.)
Robin.
> +}
> +
> +static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev,
> + int num_clks,
> + const char **clk_ids)
> +{
> + return NULL;
> +}
> +
> +static inline void clk_bulk_free(struct clk_bulk_data *clks)
> +{
> +}
> +
> static inline struct clk *clk_get(struct device *dev, const char *id)
> {
> return NULL;
>
HI Maciej,
Just sharing a couple of fly-by ideas - please don't read too much into them.
On 19 February 2018 at 15:43, Maciej Purski <[email protected]> wrote:
> When a driver is going to use clk_bulk_get() function, it has to
> initialize an array of clk_bulk_data, by filling its id fields.
>
> Add a new function to the core, which dynamically allocates
> clk_bulk_data array and fills its id fields. Add clk_bulk_free()
> function, which frees the array allocated by clk_bulk_alloc() function.
> Add a managed version of clk_bulk_alloc().
>
Most places use a small fixed number of struct clk pointers.
Using devres + kalloc to allocate 1-4 pointers feels a bit strange.
Quick grep shows over 150 instances that could be updated to use the new API.
Adding a cocci script to simplify the transition would be a good idea.
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -15,6 +15,7 @@
> #include <linux/err.h>
> #include <linux/kernel.h>
> #include <linux/notifier.h>
> +#include <linux/slab.h>
>
The extra header declaration should not be needed. One should be able
to forward declare any undefined structs.
HTH
Emil
Hi Maciej,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.16-rc2 next-20180219]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maciej-Purski/Use-clk-bulk-API-in-exynos5433-drivers/20180220-054431
base: git://linuxtv.org/media_tree.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64
All warnings (new ones prefixed by >>):
drivers/media/platform/exynos-gsc/gsc-core.c: In function 'gsc_probe':
>> drivers/media/platform/exynos-gsc/gsc-core.c:1190:8: warning: passing argument 3 of 'devm_clk_bulk_alloc' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
drv_data->clk_names);
^~~~~~~~
In file included from drivers/media/platform/exynos-gsc/gsc-core.c:25:0:
include/linux/clk.h:654:37: note: expected 'const char **' but argument is of type 'const char * const*'
static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev,
^~~~~~~~~~~~~~~~~~~
vim +1190 drivers/media/platform/exynos-gsc/gsc-core.c
1144
1145 static int gsc_probe(struct platform_device *pdev)
1146 {
1147 struct gsc_dev *gsc;
1148 struct resource *res;
1149 struct device *dev = &pdev->dev;
1150 const struct gsc_driverdata *drv_data = of_device_get_match_data(dev);
1151 int ret;
1152
1153 gsc = devm_kzalloc(dev, sizeof(struct gsc_dev), GFP_KERNEL);
1154 if (!gsc)
1155 return -ENOMEM;
1156
1157 ret = of_alias_get_id(pdev->dev.of_node, "gsc");
1158 if (ret < 0)
1159 return ret;
1160
1161 if (drv_data == &gsc_v_100_drvdata)
1162 dev_info(dev, "compatible 'exynos5-gsc' is deprecated\n");
1163
1164 gsc->id = ret;
1165 if (gsc->id >= drv_data->num_entities) {
1166 dev_err(dev, "Invalid platform device id: %d\n", gsc->id);
1167 return -EINVAL;
1168 }
1169
1170 gsc->num_clocks = drv_data->num_clocks;
1171 gsc->variant = drv_data->variant[gsc->id];
1172 gsc->pdev = pdev;
1173
1174 init_waitqueue_head(&gsc->irq_queue);
1175 spin_lock_init(&gsc->slock);
1176 mutex_init(&gsc->lock);
1177
1178 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
1179 gsc->regs = devm_ioremap_resource(dev, res);
1180 if (IS_ERR(gsc->regs))
1181 return PTR_ERR(gsc->regs);
1182
1183 res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
1184 if (!res) {
1185 dev_err(dev, "failed to get IRQ resource\n");
1186 return -ENXIO;
1187 }
1188
1189 gsc->clocks = devm_clk_bulk_alloc(dev, gsc->num_clocks,
> 1190 drv_data->clk_names);
1191 if (IS_ERR(gsc->clocks))
1192 return PTR_ERR(gsc->clocks);
1193
1194 ret = devm_clk_bulk_get(dev, gsc->num_clocks,
1195 gsc->clocks);
1196 if (ret)
1197 return ret;
1198
1199 ret = clk_bulk_prepare_enable(gsc->num_clocks, gsc->clocks);
1200 if (ret)
1201 return ret;
1202
1203 ret = devm_request_irq(dev, res->start, gsc_irq_handler,
1204 0, pdev->name, gsc);
1205 if (ret) {
1206 dev_err(dev, "failed to install irq (%d)\n", ret);
1207 goto err_clk;
1208 }
1209
1210 ret = v4l2_device_register(dev, &gsc->v4l2_dev);
1211 if (ret)
1212 goto err_clk;
1213
1214 ret = gsc_register_m2m_device(gsc);
1215 if (ret)
1216 goto err_v4l2;
1217
1218 platform_set_drvdata(pdev, gsc);
1219
1220 gsc_hw_set_sw_reset(gsc);
1221 gsc_wait_reset(gsc);
1222
1223 vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
1224
1225 dev_dbg(dev, "gsc-%d registered successfully\n", gsc->id);
1226
1227 pm_runtime_set_active(dev);
1228 pm_runtime_enable(dev);
1229
1230 return 0;
1231
1232 err_v4l2:
1233 v4l2_device_unregister(&gsc->v4l2_dev);
1234 err_clk:
1235 clk_bulk_disable_unprepare(gsc->num_clocks, gsc->clocks);
1236
1237 return ret;
1238 }
1239
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Maciej,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.16-rc2 next-20180219]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maciej-Purski/Use-clk-bulk-API-in-exynos5433-drivers/20180220-054431
base: git://linuxtv.org/media_tree.git master
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
>> ERROR: "devm_clk_bulk_alloc" [drivers/media/platform/s5p-jpeg/s5p-jpeg.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Maciej,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.16-rc2 next-20180219]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maciej-Purski/Use-clk-bulk-API-in-exynos5433-drivers/20180220-054431
base: git://linuxtv.org/media_tree.git master
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
ERROR: "devm_clk_bulk_alloc" [drivers/media/platform/s5p-jpeg/s5p-jpeg.ko] undefined!
>> ERROR: "devm_clk_bulk_alloc" [drivers/gpu/drm/exynos/exynosdrm.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Maciej,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.16-rc2 next-20180219]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maciej-Purski/Use-clk-bulk-API-in-exynos5433-drivers/20180220-054431
base: git://linuxtv.org/media_tree.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64
All warnings (new ones prefixed by >>):
drivers/media/platform/s5p-mfc/s5p_mfc_pm.c: In function 's5p_mfc_init_pm':
>> drivers/media/platform/s5p-mfc/s5p_mfc_pm.c:39:7: warning: passing argument 3 of 'devm_clk_bulk_alloc' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
pm->clk_names);
^~
In file included from drivers/media/platform/s5p-mfc/s5p_mfc_pm.c:13:0:
include/linux/clk.h:654:37: note: expected 'const char **' but argument is of type 'const char * const*'
static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev,
^~~~~~~~~~~~~~~~~~~
vim +39 drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
24
25 int s5p_mfc_init_pm(struct s5p_mfc_dev *dev)
26 {
27 int ret;
28
29 pm = &dev->pm;
30 p_dev = dev;
31
32 pm->num_clocks = dev->variant->num_clocks;
33 pm->clk_names = dev->variant->clk_names;
34 pm->device = &dev->plat_dev->dev;
35 pm->clock_gate = NULL;
36
37 /* clock control */
38 pm->clocks = devm_clk_bulk_alloc(pm->device, pm->num_clocks,
> 39 pm->clk_names);
40 if (IS_ERR(pm->clocks))
41 return PTR_ERR(pm->clocks);
42
43 ret = devm_clk_bulk_get(pm->device, pm->num_clocks, pm->clocks);
44 if (ret < 0)
45 return ret;
46
47 if (dev->variant->use_clock_gating)
48 pm->clock_gate = pm->clocks[0].clk;
49
50 pm_runtime_enable(pm->device);
51 atomic_set(&clk_ref, 0);
52 return 0;
53 }
54
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Robin,
On 2018-02-19 17:29, Robin Murphy wrote:
> Hi Maciej,
>
> On 19/02/18 15:43, Maciej Purski wrote:
>> When a driver is going to use clk_bulk_get() function, it has to
>> initialize an array of clk_bulk_data, by filling its id fields.
>>
>> Add a new function to the core, which dynamically allocates
>> clk_bulk_data array and fills its id fields. Add clk_bulk_free()
>> function, which frees the array allocated by clk_bulk_alloc() function.
>> Add a managed version of clk_bulk_alloc().
>
> Seeing how every subsequent patch ends up with the roughly this same
> stanza:
>
> x = devm_clk_bulk_alloc(dev, num, names);
> if (IS_ERR(x)
> return PTR_ERR(x);
> ret = devm_clk_bulk_get(dev, x, num);
>
> I wonder if it might be better to simply implement:
>
> int devm_clk_bulk_alloc_get(dev, &x, num, names)
>
> that does the whole lot in one go, and let drivers that want to do
> more fiddly things continue to open-code the allocation.
>
> But perhaps that's an abstraction too far... I'm not all that familiar
> with the lie of the land here.
Hmmm. This patchset clearly shows, that it would be much simpler if we
get rid of clk_bulk_data structure at all and let clk_bulk_* functions
to operate on struct clk *array[]. Typically the list of clock names
is already in some kind of array (taken from driver data or statically
embedded into driver), so there is little benefit from duplicating it
in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe
there are other benefits from this approach.
If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data
structure and switching to clock ptr array:
int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[],
const char *clk_names[]);
int clk_bulk_prepare(int num_clks, struct clk *clks[]);
int clk_bulk_enable(int num_clks, struct clk *clks[]);
...
>
>> Signed-off-by: Maciej Purski <[email protected]>
>> ---
>> drivers/clk/clk-bulk.c | 16 ++++++++++++
>> drivers/clk/clk-devres.c | 37 +++++++++++++++++++++++++---
>> include/linux/clk.h | 64
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 113 insertions(+), 4 deletions(-)
>>
>
> [...]
>> @@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id,
>> const char *con_id);
>> #else /* !CONFIG_HAVE_CLK */
>> +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks,
>> + const char **clk_ids)
>> +{
>> + return NULL;
>
> Either way, is it intentional not returning an ERR_PTR() value in this
> case? Since NULL will pass an IS_ERR() check, it seems a bit fragile
> for an allocation call to apparently succeed when the whole API is
> configured out (and I believe introducing new uses of IS_ERR_OR_NULL()
> is in general strongly discouraged.)
>
> Robin.
>
>> +}
>> +
>> +static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct
>> device *dev,
>> + int num_clks,
>> + const char **clk_ids)
>> +{
>> + return NULL;
>> +}
>> +
>> +static inline void clk_bulk_free(struct clk_bulk_data *clks)
>> +{
>> +}
>> +
>> static inline struct clk *clk_get(struct device *dev, const char *id)
>> {
>> return NULL;
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Hi Marek,
On 20/02/18 09:36, Marek Szyprowski wrote:
> Hi Robin,
>
> On 2018-02-19 17:29, Robin Murphy wrote:
>> Hi Maciej,
>>
>> On 19/02/18 15:43, Maciej Purski wrote:
>>> When a driver is going to use clk_bulk_get() function, it has to
>>> initialize an array of clk_bulk_data, by filling its id fields.
>>>
>>> Add a new function to the core, which dynamically allocates
>>> clk_bulk_data array and fills its id fields. Add clk_bulk_free()
>>> function, which frees the array allocated by clk_bulk_alloc() function.
>>> Add a managed version of clk_bulk_alloc().
>>
>> Seeing how every subsequent patch ends up with the roughly this same
>> stanza:
>>
>> x = devm_clk_bulk_alloc(dev, num, names);
>> if (IS_ERR(x)
>> return PTR_ERR(x);
>> ret = devm_clk_bulk_get(dev, x, num);
>>
>> I wonder if it might be better to simply implement:
>>
>> int devm_clk_bulk_alloc_get(dev, &x, num, names)
>>
>> that does the whole lot in one go, and let drivers that want to do
>> more fiddly things continue to open-code the allocation.
>>
>> But perhaps that's an abstraction too far... I'm not all that familiar
>> with the lie of the land here.
>
> Hmmm. This patchset clearly shows, that it would be much simpler if we
> get rid of clk_bulk_data structure at all and let clk_bulk_* functions
> to operate on struct clk *array[]. Typically the list of clock names
> is already in some kind of array (taken from driver data or statically
> embedded into driver), so there is little benefit from duplicating it
> in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe
> there are other benefits from this approach.
>
> If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data
> structure and switching to clock ptr array:
>
> int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[],
> const char *clk_names[]);
> int clk_bulk_prepare(int num_clks, struct clk *clks[]);
> int clk_bulk_enable(int num_clks, struct clk *clks[]);
> ...
Yes, that's certainly a possibility; if on the other hand there are
desirable reasons for the encapsulation (personally, I do think it's
quite neat), then maybe num_clocks should get pushed down into
clk_bulk_data as well - then with dedicated alloc/free functions as
proposed here it could become a simple opaque cookie as far as callers
are concerned.
I also haven't looked into the origins of the bulk API design, though;
I've just been familiarising myself from the perspective of reviewing
general clk API usage in drivers.
Robin.
>>> Signed-off-by: Maciej Purski <[email protected]>
>>> ---
>>> drivers/clk/clk-bulk.c | 16 ++++++++++++
>>> drivers/clk/clk-devres.c | 37 +++++++++++++++++++++++++---
>>> include/linux/clk.h | 64
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 113 insertions(+), 4 deletions(-)
>>>
>>
>> [...]
>>> @@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id,
>>> const char *con_id);
>>> #else /* !CONFIG_HAVE_CLK */
>>> +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks,
>>> + const char **clk_ids)
>>> +{
>>> + return NULL;
>>
>> Either way, is it intentional not returning an ERR_PTR() value in this
>> case? Since NULL will pass an IS_ERR() check, it seems a bit fragile
>> for an allocation call to apparently succeed when the whole API is
>> configured out (and I believe introducing new uses of IS_ERR_OR_NULL()
>> is in general strongly discouraged.)
>>
>> Robin.
>>
>>> +}
>>> +
>>> +static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct
>>> device *dev,
>>> + int num_clks,
>>> + const char **clk_ids)
>>> +{
>>> + return NULL;
>>> +}
>>> +
>>> +static inline void clk_bulk_free(struct clk_bulk_data *clks)
>>> +{
>>> +}
>>> +
>>> static inline struct clk *clk_get(struct device *dev, const char *id)
>>> {
>>> return NULL;
>>
> Best regards
Quoting Marek Szyprowski (2018-02-20 01:36:03)
> Hi Robin,
>
> On 2018-02-19 17:29, Robin Murphy wrote:
> >
> > Seeing how every subsequent patch ends up with the roughly this same
> > stanza:
> >
> > x = devm_clk_bulk_alloc(dev, num, names);
> > if (IS_ERR(x)
> > return PTR_ERR(x);
> > ret = devm_clk_bulk_get(dev, x, num);
> >
> > I wonder if it might be better to simply implement:
> >
> > int devm_clk_bulk_alloc_get(dev, &x, num, names)
> >
> > that does the whole lot in one go, and let drivers that want to do
> > more fiddly things continue to open-code the allocation.
> >
> > But perhaps that's an abstraction too far... I'm not all that familiar
> > with the lie of the land here.
>
> Hmmm. This patchset clearly shows, that it would be much simpler if we
> get rid of clk_bulk_data structure at all and let clk_bulk_* functions
> to operate on struct clk *array[]. Typically the list of clock names
> is already in some kind of array (taken from driver data or statically
> embedded into driver), so there is little benefit from duplicating it
> in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe
> there are other benefits from this approach.
>
> If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data
> structure and switching to clock ptr array:
>
> int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[],
> const char *clk_names[]);
> int clk_bulk_prepare(int num_clks, struct clk *clks[]);
> int clk_bulk_enable(int num_clks, struct clk *clks[]);
> ...
>
If you have an array of pointers to names of clks then we can put the
struct clk pointers adjacent to them at the same time. I suppose the
problem there is that some drivers want to mark that array of pointers
to names as const. And then we can't update the clk pointers next to
them.
This is the same design that regulators has done so that's why it's
written like this for clks. Honestly, we're talking about a handful of
pointers here so I'm not sure it really matters much. Just duplicate the
pointer and be done if you want to mark the array of names as const or
have your const 'setup' structure point to the bulk_data array that you
define statically non-const somewhere globally.