* [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes
From: Tomeu Vizoso @ 2015-07-01 9:41 UTC (permalink / raw)
To: linux-kernel
Cc: devicetree, linux-fbdev, Tomeu Vizoso, linux-gpio, Liam Girdwood,
Stephen Warren, Rafael J. Wysocki, alsa-devel, dri-devel,
Jaroslav Kysela, linux-acpi, Mark Brown, linux-pwm, linux-tegra,
Alexandre Courbot
In-Reply-To: <1435743667-11987-1-git-send-email-tomeu.vizoso@collabora.com>
So others can find out what dependencies a nvidia,tegra-audio-max98090
device has, as specified in
bindings/sound/nvidia,tegra-audio-max98090.txt.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
Changes in v2: None
sound/soc/tegra/tegra_max98090.c | 42 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/sound/soc/tegra/tegra_max98090.c b/sound/soc/tegra/tegra_max98090.c
index 902da36..0f7cbf3 100644
--- a/sound/soc/tegra/tegra_max98090.c
+++ b/sound/soc/tegra/tegra_max98090.c
@@ -316,7 +316,47 @@ static struct platform_driver tegra_max98090_driver = {
.probe = tegra_max98090_probe,
.remove = tegra_max98090_remove,
};
-module_platform_driver(tegra_max98090_driver);
+
+static void add_dependency(struct fwnode_handle *fwnode,
+ const char *property,
+ struct list_head *deps)
+{
+ struct device_node *np;
+
+ np = of_parse_phandle(to_of_node(fwnode), property, 0);
+ if (!np)
+ return;
+
+ fwnode_add_dependency(&np->fwnode, deps);
+}
+
+static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
+ struct list_head *deps)
+{
+ add_dependency(fwnode, "nvidia,i2s-controller", deps);
+ add_dependency(fwnode, "nvidia,audio-codec", deps);
+}
+
+static int __init tegra_max98090_init(void)
+{
+ int err;
+
+ err = platform_driver_register(&tegra_max98090_driver);
+ if (err < 0)
+ return err;
+
+ fwnode_add_dependency_parser(tegra_max98090_get_dependencies);
+
+ return 0;
+}
+module_init(tegra_max98090_init);
+
+static void __exit tegra_max98090_exit(void)
+{
+ fwnode_remove_dependency_parser(tegra_max98090_get_dependencies);
+ platform_driver_unregister(&tegra_max98090_driver);
+}
+module_exit(tegra_max98090_exit);
MODULE_AUTHOR("Stephen Warren <swarren@nvidia.com>");
MODULE_DESCRIPTION("Tegra max98090 machine ASoC driver");
--
2.4.1
^ permalink raw reply related
* [PATCH v2 12/12] driver-core: probe dependencies before probing
From: Tomeu Vizoso @ 2015-07-01 9:41 UTC (permalink / raw)
To: linux-kernel
Cc: devicetree, linux-fbdev, Tomeu Vizoso, linux-gpio,
Greg Kroah-Hartman, Rafael J. Wysocki, alsa-devel, dri-devel,
linux-acpi, Mark Brown, linux-pwm
In-Reply-To: <1435743667-11987-1-git-send-email-tomeu.vizoso@collabora.com>
Before actually probing a device, find out what dependencies it has and
do our best to ensure that they are available at this point.
This is accomplished by finding out what platform devices need to be
probed and probing them. Non-platform devices will be probed when the
closest ancestor that is a platform device is probed.
If any dependencies are still unavailable after that (most probably a
missing driver or an error in the HW description from the firmware), we
print a nice error message so that people don't have to add a zillion of
printks to find out why a device asked for its probe to be deferred.
Dependencies are discovered with the help of the code that is already
implementing the specification of the firmware bindings, via the
callbacks registered with fwnode_add_dependency_parser().
Currently the dependencies list is discarded but it could be stored for
later usage.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
tegra, kernel, usb
---
Changes in v2:
- Allocate the list of dependencies and pass it to the function that
fills it.
drivers/base/dd.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 139 insertions(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index a638bbb..c8a1aff 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -25,6 +25,9 @@
#include <linux/async.h>
#include <linux/pm_runtime.h>
#include <linux/pinctrl/devinfo.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
#include "base.h"
#include "power/power.h"
@@ -54,6 +57,140 @@ static LIST_HEAD(deferred_probe_active_list);
static struct workqueue_struct *deferred_wq;
static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
+static bool device_is_bound(struct device *dev)
+{
+ return klist_node_attached(&dev->p->knode_driver);
+}
+
+static int fwnode_match(struct device *dev, void *data)
+{
+ return dev->fwnode = data;
+}
+
+static bool fwnode_is_bound(struct fwnode_handle *fwnode)
+{
+ struct device *dev;
+
+ dev = bus_find_device(&platform_bus_type, NULL, fwnode, fwnode_match);
+
+ /* Check whether device is bound or is being probed right now */
+ return dev ? dev->driver : false;
+}
+
+static bool fwnode_is_platform(struct fwnode_handle *fwnode)
+{
+ struct fwnode_handle *parent;
+ const char *compatible;
+ int count;
+
+ count = fwnode_property_read_string_array(fwnode, "compatible", NULL,
+ 0);
+
+ /* The node has to have a compatible string */
+ if (!count)
+ return false;
+
+ /* But it cannot be only simple-bus */
+ if ((count = 1) &&
+ !fwnode_property_read_string(fwnode, "compatible", &compatible) &&
+ !strcmp(compatible, "simple-bus"))
+ return false;
+
+ parent = fwnode_get_parent(fwnode);
+
+ /* Node is immediately below root */
+ if (!fwnode_get_parent(parent))
+ return true;
+
+ /* If its parent is a simple-bus */
+ if (fwnode_is_compatible(parent, "simple-bus"))
+ return true;
+
+ return false;
+}
+
+static struct fwnode_handle *get_enclosing_platform_dev(
+ struct fwnode_handle *fwnode)
+{
+ struct fwnode_handle *iter, *node = NULL;
+
+ for (iter = fwnode;
+ iter && fwnode_get_parent(iter);
+ iter = fwnode_get_parent(iter)) {
+
+ /*
+ * If we already have a platform device and an ancestor is
+ * already bound, the first is the one we want to probe.
+ */
+ if (node && fwnode_is_bound(iter))
+ break;
+
+ if (fwnode_is_platform(iter))
+ node = iter;
+ }
+
+ return node;
+}
+
+static bool check_dependency(struct fwnode_handle *fwnode)
+{
+ struct fwnode_handle *target;
+ struct device *dev;
+
+ if (!fwnode)
+ return true;
+
+ target = get_enclosing_platform_dev(fwnode);
+ if (!target)
+ return true;
+
+ dev = bus_find_device(&platform_bus_type, NULL, target, fwnode_match);
+ if (!dev) {
+ pr_debug("Couldn't find device for %s\n",
+ fwnode_get_name(fwnode));
+ return false;
+ }
+
+ /*
+ * Device is bound or is being probed right now. If we have bad luck
+ * and the dependency isn't ready when it's needed, deferred probe
+ * will save us.
+ */
+ if (dev->driver)
+ return true;
+
+ bus_probe_device(dev);
+
+ /* If the dependency hasn't finished probing, we'll want a warning */
+ return device_is_bound(dev);
+}
+
+static void check_dependencies(struct device *dev)
+{
+ struct fwnode_dependency *dep, *tmp;
+ LIST_HEAD(deps);
+
+ if (dev->parent && !check_dependency(dev->parent->fwnode))
+ pr_debug("Parent '%s' of device '%s' not available\n",
+ dev_name(dev->parent), dev_name(dev));
+
+ if (!dev->fwnode) {
+ pr_debug("Device '%s' doesn't have a fwnode\n", dev_name(dev));
+ return;
+ }
+
+ fwnode_get_dependencies(dev->fwnode, &deps);
+
+ list_for_each_entry_safe(dep, tmp, &deps, dependency) {
+ if (!check_dependency(dep->fwnode))
+ pr_debug("Dependency '%s' not available\n",
+ fwnode_get_name(dep->fwnode));
+
+ list_del(&dep->dependency);
+ kfree(dep);
+ }
+}
+
/*
* deferred_probe_work_func() - Retry probing devices in the active list.
*/
@@ -287,6 +424,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
dev->driver = drv;
+ check_dependencies(dev);
+
/* If using pinctrl, bind pins now before probing */
ret = pinctrl_bind_pins(dev);
if (ret)
--
2.4.1
^ permalink raw reply related
* [PATCH v3] video-lp8788: Delete a check before backlight_device_unregister()
From: SF Markus Elfring @ 2015-07-01 10:30 UTC (permalink / raw)
To: Lee Jones, Jingoo Han, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev
Cc: Linux Kernel Mailing List, kernel-janitors, Julia Lawall
In-Reply-To: <20150701080612.GG3210@x1>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 1 Jul 2015 12:08:31 +0200
The backlight_device_unregister() function tests whether its argument
is NULL and then returns immediately.
Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/video/backlight/lp8788_bl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/video/backlight/lp8788_bl.c b/drivers/video/backlight/lp8788_bl.c
index e418d5b..5d583d7 100644
--- a/drivers/video/backlight/lp8788_bl.c
+++ b/drivers/video/backlight/lp8788_bl.c
@@ -221,8 +221,7 @@ static void lp8788_backlight_unregister(struct lp8788_bl *bl)
{
struct backlight_device *bl_dev = bl->bl_dev;
- if (bl_dev)
- backlight_device_unregister(bl_dev);
+ backlight_device_unregister(bl_dev);
}
static ssize_t lp8788_get_bl_ctl_mode(struct device *dev,
--
2.4.5
^ permalink raw reply related
* Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods
From: Heiko Stübner @ 2015-07-01 11:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-15-git-send-email-boris.brezillon@free-electrons.com>
Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
> Implement the ->enable(), ->disable() and ->is_enabled methods and remove
> the PWM call in ->set_voltage_sel().
> This is particularly important for critical regulators tagged as always-on,
> because not claiming the PWM (and its dependencies) might lead to
> unpredictable behavior (like a system hang because the PWM clk is only
> claimed when the PWM device is enabled).
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/regulator/pwm-regulator.c
> b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct
> regulator_dev *rdev,
>
> drvdata->state = selector;
>
> - ret = pwm_enable(drvdata->pwm);
> - if (ret) {
> - dev_err(&rdev->dev, "Failed to enable PWM\n");
> - return ret;
> - }
> -
> return 0;
> }
>
> @@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct
> regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV;
> }
>
> +static int pwm_regulator_enable(struct regulator_dev *dev)
> +{
> + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> +
> + return pwm_enable(drvdata->pwm);
> +}
> +
> +static int pwm_regulator_disable(struct regulator_dev *dev)
> +{
> + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> +
> + pwm_disable(drvdata->pwm);
> +
> + return 0;
> +}
> +
> +static int pwm_regulator_is_enabled(struct regulator_dev *dev)
> +{
> + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> +
> + return pwm_is_enabled(drvdata->pwm);
> +}
nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of tabs)
^ permalink raw reply
* Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods
From: Boris Brezillon @ 2015-07-01 12:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3495901.qMzLCVWCDl@diego>
Hi Heiko,
On Wed, 01 Jul 2015 13:58:09 +0200
Heiko Stübner <heiko@sntech.de> wrote:
> Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
> > Implement the ->enable(), ->disable() and ->is_enabled methods and remove
> > the PWM call in ->set_voltage_sel().
> > This is particularly important for critical regulators tagged as always-on,
> > because not claiming the PWM (and its dependencies) might lead to
> > unpredictable behavior (like a system hang because the PWM clk is only
> > claimed when the PWM device is enabled).
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
> > 1 file changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/regulator/pwm-regulator.c
> > b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
> > --- a/drivers/regulator/pwm-regulator.c
> > +++ b/drivers/regulator/pwm-regulator.c
> > @@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct
> > regulator_dev *rdev,
> >
> > drvdata->state = selector;
> >
> > - ret = pwm_enable(drvdata->pwm);
> > - if (ret) {
> > - dev_err(&rdev->dev, "Failed to enable PWM\n");
> > - return ret;
> > - }
> > -
> > return 0;
> > }
> >
> > @@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct
> > regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV;
> > }
> >
> > +static int pwm_regulator_enable(struct regulator_dev *dev)
> > +{
> > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > +
> > + return pwm_enable(drvdata->pwm);
> > +}
> > +
> > +static int pwm_regulator_disable(struct regulator_dev *dev)
> > +{
> > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > +
> > + pwm_disable(drvdata->pwm);
> > +
> > + return 0;
> > +}
> > +
> > +static int pwm_regulator_is_enabled(struct regulator_dev *dev)
> > +{
> > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > +
> > + return pwm_is_enabled(drvdata->pwm);
> > +}
>
> nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of tabs)
Yep, I noticed checkpatch warnings/errors before sending the patch, but
since this is just an RFC I decided to fix them for the next version ;-)
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods
From: Heiko Stübner @ 2015-07-01 12:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150701140531.2041b05c@bbrezillon>
Am Mittwoch, 1. Juli 2015, 14:05:31 schrieb Boris Brezillon:
> Hi Heiko,
>
> On Wed, 01 Jul 2015 13:58:09 +0200
>
> Heiko Stübner <heiko@sntech.de> wrote:
> > Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
> > > Implement the ->enable(), ->disable() and ->is_enabled methods and
> > > remove
> > > the PWM call in ->set_voltage_sel().
> > > This is particularly important for critical regulators tagged as
> > > always-on,
> > > because not claiming the PWM (and its dependencies) might lead to
> > > unpredictable behavior (like a system hang because the PWM clk is only
> > > claimed when the PWM device is enabled).
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > ---
> > >
> > > drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
> > > 1 file changed, 26 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/regulator/pwm-regulator.c
> > > b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
> > > --- a/drivers/regulator/pwm-regulator.c
> > > +++ b/drivers/regulator/pwm-regulator.c
> > > @@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct
> > > regulator_dev *rdev,
> > >
> > > drvdata->state = selector;
> > >
> > > - ret = pwm_enable(drvdata->pwm);
> > > - if (ret) {
> > > - dev_err(&rdev->dev, "Failed to enable PWM\n");
> > > - return ret;
> > > - }
> > > -
> > >
> > > return 0;
> > >
> > > }
> > >
> > > @@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct
> > > regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV;
> > >
> > > }
> > >
> > > +static int pwm_regulator_enable(struct regulator_dev *dev)
> > > +{
> > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > +
> > > + return pwm_enable(drvdata->pwm);
> > > +}
> > > +
> > > +static int pwm_regulator_disable(struct regulator_dev *dev)
> > > +{
> > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > +
> > > + pwm_disable(drvdata->pwm);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int pwm_regulator_is_enabled(struct regulator_dev *dev)
> > > +{
> > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > +
> > > + return pwm_is_enabled(drvdata->pwm);
> > > +}
> >
> > nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of
> > tabs)
> Yep, I noticed checkpatch warnings/errors before sending the patch, but
> since this is just an RFC I decided to fix them for the next version ;-)
ok, so I'll just skip over any more style issues for now. Making my way
through your series and trying it on my veyron right now :-)
Heiko
^ permalink raw reply
* Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods
From: Boris Brezillon @ 2015-07-01 12:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3530235.H11CYEfk28@diego>
On Wed, 01 Jul 2015 14:08:18 +0200
Heiko Stübner <heiko@sntech.de> wrote:
> Am Mittwoch, 1. Juli 2015, 14:05:31 schrieb Boris Brezillon:
> > Hi Heiko,
> >
> > On Wed, 01 Jul 2015 13:58:09 +0200
> >
> > Heiko Stübner <heiko@sntech.de> wrote:
> > > Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
> > > > Implement the ->enable(), ->disable() and ->is_enabled methods and
> > > > remove
> > > > the PWM call in ->set_voltage_sel().
> > > > This is particularly important for critical regulators tagged as
> > > > always-on,
> > > > because not claiming the PWM (and its dependencies) might lead to
> > > > unpredictable behavior (like a system hang because the PWM clk is only
> > > > claimed when the PWM device is enabled).
> > > >
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > ---
> > > >
> > > > drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
> > > > 1 file changed, 26 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/regulator/pwm-regulator.c
> > > > b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
> > > > --- a/drivers/regulator/pwm-regulator.c
> > > > +++ b/drivers/regulator/pwm-regulator.c
> > > > @@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct
> > > > regulator_dev *rdev,
> > > >
> > > > drvdata->state = selector;
> > > >
> > > > - ret = pwm_enable(drvdata->pwm);
> > > > - if (ret) {
> > > > - dev_err(&rdev->dev, "Failed to enable PWM\n");
> > > > - return ret;
> > > > - }
> > > > -
> > > >
> > > > return 0;
> > > >
> > > > }
> > > >
> > > > @@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct
> > > > regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV;
> > > >
> > > > }
> > > >
> > > > +static int pwm_regulator_enable(struct regulator_dev *dev)
> > > > +{
> > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > > +
> > > > + return pwm_enable(drvdata->pwm);
> > > > +}
> > > > +
> > > > +static int pwm_regulator_disable(struct regulator_dev *dev)
> > > > +{
> > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > > +
> > > > + pwm_disable(drvdata->pwm);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int pwm_regulator_is_enabled(struct regulator_dev *dev)
> > > > +{
> > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > > +
> > > > + return pwm_is_enabled(drvdata->pwm);
> > > > +}
> > >
> > > nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of
> > > tabs)
> > Yep, I noticed checkpatch warnings/errors before sending the patch, but
> > since this is just an RFC I decided to fix them for the next version ;-)
>
> ok, so I'll just skip over any more style issues for now. Making my way
> through your series and trying it on my veyron right now :-)
Also note that I haven't tested the series on a real board (just compile
tested) because I don't have the board with me right now, but I wanted
to post the RFC early so that we can discuss the concepts.
Anyway, any feedback on the implementation (including bug reports) is
welcome.
This is the version I actually tested on the veyron board:
https://github.com/bbrezillon/linux-rk/tree/rk-3.14
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes
From: Mark Brown @ 2015-07-01 17:38 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-kernel, linux-acpi, dri-devel, linux-fbdev, linux-gpio,
devicetree, linux-pwm, Rafael J. Wysocki, alsa-devel,
Jaroslav Kysela, Thierry Reding, Takashi Iwai, Stephen Warren,
Liam Girdwood, linux-tegra, Alexandre Courbot
In-Reply-To: <1435743667-11987-12-git-send-email-tomeu.vizoso@collabora.com>
[-- Attachment #1: Type: text/plain, Size: 552 bytes --]
On Wed, Jul 01, 2015 at 11:41:06AM +0200, Tomeu Vizoso wrote:
> +static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
> + struct list_head *deps)
> +{
> + add_dependency(fwnode, "nvidia,i2s-controller", deps);
> + add_dependency(fwnode, "nvidia,audio-codec", deps);
> +}
Why is this all being open coded in an individual driver (we already
know about and manage all these dependencies in the core...)? If we're
going to do this I'd expect the interface for specifying DT nodes to the
core to be changed to support this.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 12/15] pwm: rockchip: add initial state retrieval
From: Heiko Stübner @ 2015-07-01 21:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-13-git-send-email-boris.brezillon@free-electrons.com>
Hi Boris,
Am Mittwoch, 1. Juli 2015, 10:21:58 schrieb Boris Brezillon:
> Implement the ->init_state() function to expose initial state.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
[...]
> @@ -98,6 +110,36 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> }
>
> +static void rockchip_pwm_init_state(struct pwm_chip *chip,
> + struct pwm_device *pwm)
> +{
> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> + unsigned long clk_rate;
> + u64 tmp;
> + int ret;
> +
> + ret = clk_enable(pc->clk);
> + if (ret)
> + return;
> +
> + clk_rate = clk_get_rate(pc->clk);
> +
> + tmp = readl(pc->base + pc->data->regs.period);
> + tmp *= pc->data->prescaler * NSEC_PER_SEC;
> + tmp = do_div(tmp, clk_rate);
I guess you want to have the division result here and not the remainder, so
- tmp = do_div(tmp, clk_rate);
+ do_div(tmp, clk_rate);
> + pwm->state.period = tmp;
> +
> + tmp = readl(pc->base + pc->data->regs.duty);
> + tmp *= pc->data->prescaler * NSEC_PER_SEC;
> + tmp = do_div(tmp, clk_rate);
ditto
> + pwm->state.duty_cycle = tmp;
> +
> + pc->data->init(chip, chip->pwms);
> +
> + if (!pwm_is_enabled(pwm))
> + clk_disable(pc->clk);
> +}
> +
> static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device
> *pwm, int duty_ns, int period_ns)
> {
> @@ -171,6 +213,7 @@ static void rockchip_pwm_disable(struct pwm_chip *chip,
> struct pwm_device *pwm) }
>
> static const struct pwm_ops rockchip_pwm_ops_v1 = {
> + .init_state = rockchip_pwm_init_state,
> .config = rockchip_pwm_config,
> .enable = rockchip_pwm_enable,
> .disable = rockchip_pwm_disable,
> @@ -178,6 +221,7 @@ static const struct pwm_ops rockchip_pwm_ops_v1 = {
> };
>
> static const struct pwm_ops rockchip_pwm_ops_v2 = {
> + .init_state = rockchip_pwm_init_state,
> .config = rockchip_pwm_config,
> .set_polarity = rockchip_pwm_set_polarity,
> .enable = rockchip_pwm_enable,
> @@ -195,6 +239,7 @@ static const struct rockchip_pwm_data pwm_data_v1 = {
> .prescaler = 2,
> .ops = &rockchip_pwm_ops_v1,
> .set_enable = rockchip_pwm_set_enable_v1,
> + .init = rockchip_pwm_init_v1,
> };
>
> static const struct rockchip_pwm_data pwm_data_v2 = {
> @@ -207,6 +252,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
> .prescaler = 1,
> .ops = &rockchip_pwm_ops_v2,
> .set_enable = rockchip_pwm_set_enable_v2,
> + .init = rockchip_pwm_init_v2,
you're referencing the v2 init here, but only add it in the next patch?
[pwm: rockchip: add support for atomic update]
> };
>
> static const struct rockchip_pwm_data pwm_data_vop = {
> @@ -219,6 +265,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
> .prescaler = 1,
> .ops = &rockchip_pwm_ops_v2,
> .set_enable = rockchip_pwm_set_enable_v2,
> + .init = rockchip_pwm_init_v2,
ditto
Heiko
^ permalink raw reply
* Re: [RFC PATCH 13/15] pwm: rockchip: add support for atomic update
From: Heiko Stübner @ 2015-07-01 21:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-14-git-send-email-boris.brezillon@free-electrons.com>
Hi Boris,
Am Mittwoch, 1. Juli 2015, 10:21:59 schrieb Boris Brezillon:
> Implement the ->apply() function to add support for atomic update.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> @@ -110,6 +113,26 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> }
>
> +static void rockchip_pwm_init_v2(struct pwm_chip *chip, struct pwm_device
> *pwm) +{
> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> + u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
> + PWM_CONTINUOUS;
> + u32 val;
> +
> + val = readl(pc->base + pc->data->regs.ctrl);
> +
> + if ((val & enable_conf) != enable_conf)
> + return;
> +
> + pwm->state.enabled = true;
> +
> + enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
> +
> + if ((val & enable_conf) = enable_conf)
> + pwm->state.polarity = PWM_POLARITY_INVERSED;
the inactive setting does not affect the polarity of the running pwm, only what
to do when it gets turned off. Also PWM_DUTY_NEGATIVE is the "0" value for the
bit so also is bad to compare against (and results in wrong readings). So I
would suggest changing this like
- enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
+ enable_conf = PWM_DUTY_POSITIVE;
- if ((val & enable_conf) = enable_conf)
+ if ((val & enable_conf) != enable_conf)
> +}
> +
> static void rockchip_pwm_init_state(struct pwm_chip *chip,
> struct pwm_device *pwm)
> {
Heiko
^ permalink raw reply
* [RFC PATCH 16/15] pwm: add informations about polarity, duty cycle and period to debugfs
From: Heiko Stübner @ 2015-07-01 21:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-1-git-send-email-boris.brezillon@free-electrons.com>
The pwm-states make it possible to also output the polarity, duty cycle
and period information in the debugfs pwm summary-outout.
This makes it easier to gather overview information about pwms without
needing to walk through the sysfs attributes of every pwm.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
might be nice to have too ;-)
drivers/pwm/core.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6dafd8e..79037a2 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -951,9 +951,18 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
if (test_bit(PWMF_REQUESTED, &pwm->flags))
seq_puts(s, " requested");
- if (pwm_is_enabled(pwm))
+ if (pwm_is_enabled(pwm)) {
seq_puts(s, " enabled");
+ seq_printf(s, " period:%uns",
+ pwm_get_period(pwm));
+ seq_printf(s, " duty:%uns",
+ pwm_get_duty_cycle(pwm));
+ seq_printf(s, " polarity:%s",
+ pwm_get_polarity(pwm) ? "inverse"
+ : "normal");
+ }
+
seq_puts(s, "\n");
}
}
--
2.1.4
^ permalink raw reply related
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Heiko Stübner @ 2015-07-01 21:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-1-git-send-email-boris.brezillon@free-electrons.com>
Hi Boris,
Am Mittwoch, 1. Juli 2015, 10:21:46 schrieb Boris Brezillon:
> Hello Thierry,
>
> This series adds support for atomic PWM update, or ITO, the capability
> to update all the parameters of a PWM device (enabled/disabled, period,
> duty and polarity) in one go.
>
> This implementation is still experimental, and I may have missed some key
> aspect, so any feedback are welcome.
>
> Also note that I haven't protected the state update with any locking.
> That's because the existing config does not protect against concurrent
> access to a requested PWM device (see the pwm_config implementation).
> I guess the PWM framework assume the user will implement the proper locking
> scheme if it has to concurrently access the device.
>
> The 5 first patches prepare the addition of the pwm_state concept, which
> will be used to allow atomic updates.
> The following patches introduce the pwm_state struct, initial state
> retrieval and atomic update concepts.
>
> Patches 12 and 13 are showing how one can implement the initial state
> retrieval and atomic update features in a PWM driver (in this specific
> case I implemented it in the rockchip driver).
>
> The last 2 patches are making use of those changes to improve the
> pwm-regulator driver (initializing the regulator state based on the
> initial PWM state).
at first I got very strange readings (very wrong values and wrong polarity),
which resulted from the issues I pointed out in the replies to individual
patches. After fixing these, the pwm read-back now returns exactly the expected
values :-) .
And with the original voltage table from the Chromeos-devicetrees, the pwm-
regulator also returns the expected 1.2V that coreboot initially set.
Heiko
^ permalink raw reply
* Re: [PATCH v2 01/12] device: property: delay device-driver matches
From: Rafael J. Wysocki @ 2015-07-01 23:29 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-kernel, Mark Brown, linux-acpi, dri-devel, linux-fbdev,
linux-gpio, devicetree, linux-pwm, alsa-devel, Greg Kroah-Hartman
In-Reply-To: <1435743667-11987-2-git-send-email-tomeu.vizoso@collabora.com>
On Wednesday, July 01, 2015 11:40:56 AM Tomeu Vizoso wrote:
> Delay matches of platform devices until late_initcall, when we are sure
> that all built-in drivers have been registered already. This is needed
> to prevent deferred probes because of some dependencies' drivers not
> having registered yet.
>
> This reduces the total amount of work that the kernel does during boot
> because it won't try to match devices to drivers when built-in drivers
> are still registering but also reduces some parallelism, so total boot
> time might slightly increase or decrease depending on the platform and
> kernel configuration.
>
> This change will make make possible to prevent any deferred probes once
> devices are probed in dependency order.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
> Changes in v2:
> - Instead of delaying all probes until late_initcall, only delay matches
> of platform devices that have a firmware node attached.
>
> drivers/base/property.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 8528eb9..8ead1ba 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -16,8 +16,11 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> +#include <linux/platform_device.h>
> #include <linux/property.h>
>
> +static bool fwnode_match_enable = false;
> +
> /**
> * device_add_property_set - Add a collection of properties to a device object.
> * @dev: Device to add properties to.
> @@ -604,6 +607,15 @@ EXPORT_SYMBOL_GPL(fwnode_is_compatible);
> bool fwnode_driver_match_device(struct device *dev,
> const struct device_driver *drv)
> {
> + /*
> + * Delay matches of platform devices until late_initcall, when we are
> + * sure that all built-in drivers have been registered already. This
> + * is needed to prevent deferred probes because of some drivers
> + * not having registered yet.
> + */
> + if(dev->bus = &platform_bus_type && !fwnode_match_enable)
> + return false;
I'm not particularly enthusiastic about referring to specific bus types in
generic code like that.
What about having a special version of fwnode_driver_match_device() specifically
for the platform bus type that will do the check?
> +
> if (is_of_node(dev->fwnode))
> return of_driver_match_device(dev, drv);
> else if (is_acpi_node(dev->fwnode))
> @@ -612,3 +624,20 @@ bool fwnode_driver_match_device(struct device *dev,
> return false;
> }
> EXPORT_SYMBOL_GPL(fwnode_driver_match_device);
> +
> +static int __device_attach(struct device *dev, void *data)
> +{
> + device_initial_probe(dev);
> +
> + return 0;
> +}
> +
> +static int fwnode_match_initcall(void)
> +{
> + fwnode_match_enable = true;
> +
> + bus_for_each_dev(&platform_bus_type, NULL, NULL, __device_attach);
> +
> + return 0;
> +}
> +late_initcall(fwnode_match_initcall);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH v2 02/12] device: property: find dependencies of a firmware node
From: Rafael J. Wysocki @ 2015-07-01 23:36 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-kernel, Mark Brown, linux-acpi, dri-devel, linux-fbdev,
linux-gpio, devicetree, linux-pwm, alsa-devel, Greg Kroah-Hartman
In-Reply-To: <1435743667-11987-3-git-send-email-tomeu.vizoso@collabora.com>
On Wednesday, July 01, 2015 11:40:57 AM Tomeu Vizoso wrote:
> Adds API that allows callers to find out what other firmware nodes a
> node depends on.
>
> Implementors of bindings documentation can register callbacks that
> return the dependencies of a node.
>
> Dependency information can be used to change the order in which devices
> are probed, or to print a warning when a device node is going to be
> probed without all its dependencies fulfilled.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
I'd like to see a description of the new API in English in the changelog.
> ---
>
> Changes in v2:
> - Allow bindings implementations register a function instead of using
> class callbacks, as not only subsystems implement firmware bindings.
>
> drivers/base/property.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/fwnode.h | 5 +++
> include/linux/property.h | 12 +++++++
> 3 files changed, 108 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 8ead1ba..9d38ede 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -19,7 +19,13 @@
> #include <linux/platform_device.h>
> #include <linux/property.h>
>
Please add a comment describing this structure. In particular, what it is
supposed to be used for and how it is supposed to be used.
> +struct dependency_parser {
> + struct list_head parser;
I'd rather call this "list_node" or something like that.
> + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps);
> +};
> +
> static bool fwnode_match_enable = false;
> +static LIST_HEAD(dependency_parsers);
>
> /**
> * device_add_property_set - Add a collection of properties to a device object.
> @@ -553,6 +559,27 @@ bool device_dma_is_coherent(struct device *dev)
> EXPORT_SYMBOL_GPL(device_dma_is_coherent);
>
> /**
> + * fwnode_add_dependency - add firmware node to the passed dependency list
> + * @fwnode: Firmware node to add to dependency list
> + * @list: Dependency list to add the fwnode to
> + */
> +void fwnode_add_dependency(struct fwnode_handle *fwnode,
> + struct list_head *list)
> +{
> + struct fwnode_dependency *dep;
> +
> + dep = kzalloc(sizeof(*dep), GFP_KERNEL);
> + if (!dep)
> + return;
> +
> + INIT_LIST_HEAD(&dep->dependency);
> + dep->fwnode = fwnode;
> +
> + list_add_tail(&dep->dependency, list);
> +}
> +EXPORT_SYMBOL_GPL(fwnode_add_dependency);
> +
> +/**
> * fwnode_get_parent - return the parent node of a device node
> * @fwnode: Device node to find the parent node of
> */
> @@ -600,6 +627,70 @@ bool fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compatible)
> EXPORT_SYMBOL_GPL(fwnode_is_compatible);
>
> /**
> + * fwnode_add_dependency_parser - register dependency parser
> + * @func: Function that will be called to find out dependencies of a node
> + *
> + * Registers a callback that will be called when collecting the dependencies
> + * of a firmware node. The callback should inspect the properties of the node
> + * and call fwnode_add_dependency() for each dependency it recognizes, from
> + * the bindings documentation.
> + */
> +void fwnode_add_dependency_parser(
> + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps))
> +{
> + struct dependency_parser *parser;
> +
> + parser = kzalloc(sizeof(*parser), GFP_KERNEL);
> + if (!parser)
> + return;
> +
> + INIT_LIST_HEAD(&parser->parser);
> + parser->func = func;
> +
> + list_add_tail(&parser->parser, &dependency_parsers);
We're modifying a global list here. Any locking needed? RCU? Whatever?
> +}
> +EXPORT_SYMBOL_GPL(fwnode_add_dependency_parser);
> +
> +/**
> + * fwnode_remove_dependency_parser - unregister dependency parser
> + * @func: Function that was to be called to find out dependencies of a node
> + */
> +void fwnode_remove_dependency_parser(
> + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps))
> +{
> + struct dependency_parser *parser, *tmp;
> +
> + list_for_each_entry_safe(parser, tmp, &dependency_parsers, parser) {
> + if (parser->func = func) {
> + list_del(&parser->parser);
> + kfree(parser);
> + return;
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(fwnode_remove_dependency_parser);
> +
> +/**
> + * fwnode_get_dependencies - find out what dependencies a firmware node has
> + * @fwnode: firmware node to find its dependencies
> + * @deps: list of struct fwnode_dependency in which dependencies will be placed
> + */
> +void fwnode_get_dependencies(struct fwnode_handle *fwnode,
> + struct list_head *deps)
> +{
> + struct dependency_parser *parser;
> + struct fwnode_handle *child;
> +
> + list_for_each_entry(parser, &dependency_parsers, parser)
> + parser->func(fwnode, deps);
> +
> + /* Some device nodes will have dependencies in non-device sub-nodes */
> + fwnode_for_each_child_node(fwnode, child)
> + if (!fwnode_property_present(child, "compatible"))
This is a blatant OF-ism. We need to think about a generic way to express that.
> + fwnode_get_dependencies(child, deps);
> +}
> +
> +/**
> * fwnode_driver_match_device - Tell if a driver matches a device.
> * @drv: the device_driver structure to test
> * @dev: the device structure to match against
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 0408545..68ab558 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -24,4 +24,9 @@ struct fwnode_handle {
> struct fwnode_handle *secondary;
> };
>
> +struct fwnode_dependency {
> + struct fwnode_handle *fwnode;
> + struct list_head dependency;
So this is a node in the list of dependencies, right?
I'd call that field "list_node", then.
> +};
And fwnode is a firmware node that the owner of the list depends on, right?
> +
> #endif
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 4e453c4..b8b86ea 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -86,6 +86,18 @@ bool fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compatible);
> bool fwnode_driver_match_device(struct device *dev,
> const struct device_driver *drv);
>
> +void fwnode_add_dependency(struct fwnode_handle *fwnode,
> + struct list_head *list);
> +
> +void fwnode_add_dependency_parser(
> + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps));
> +
> +void fwnode_remove_dependency_parser(
> + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps));
> +
> +void fwnode_get_dependencies(struct fwnode_handle *fwnode,
> + struct list_head *list);
> +
> unsigned int device_get_child_node_count(struct device *dev);
>
> static inline bool device_property_read_bool(struct device *dev,
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH v3] video-lp8788: Delete a check before backlight_device_unregister()
From: Jingoo Han @ 2015-07-02 4:44 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Lee Jones, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev@vger.kernel.org, Linux Kernel Mailing List,
kernel-janitors@vger.kernel.org, Julia Lawall, jingoo1han
In-Reply-To: <5593C14A.8020700@users.sourceforge.net>
> On 2015. 7. 1., at PM 7:30, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
>
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 1 Jul 2015 12:08:31 +0200
>
> The backlight_device_unregister() function tests whether its argument
> is NULL and then returns immediately.
> Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
It looks good.
Acked-by: Jingoo Han <jingoohan1@gmail.com>
> ---
> drivers/video/backlight/lp8788_bl.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/lp8788_bl.c b/drivers/video/backlight/lp8788_bl.c
> index e418d5b..5d583d7 100644
> --- a/drivers/video/backlight/lp8788_bl.c
> +++ b/drivers/video/backlight/lp8788_bl.c
> @@ -221,8 +221,7 @@ static void lp8788_backlight_unregister(struct lp8788_bl *bl)
> {
> struct backlight_device *bl_dev = bl->bl_dev;
>
> - if (bl_dev)
> - backlight_device_unregister(bl_dev);
> + backlight_device_unregister(bl_dev);
> }
>
> static ssize_t lp8788_get_bl_ctl_mode(struct device *dev,
> --
> 2.4.5
>
^ permalink raw reply
* Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts
From: Uwe Kleine-König @ 2015-07-02 6:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-6-git-send-email-boris.brezillon@free-electrons.com>
On Wed, Jul 01, 2015 at 10:21:51AM +0200, Boris Brezillon wrote:
> When requested by a user, the PWM is assigned a default period and polarity
> extracted from the DT, the platform data or statically set by the driver.
> Those default values are currently stored in the period and polarity
> fields of the pwm_device struct, but they will be stored somewhere else
> once we have introduced the architecture allowing for hardware state
> retrieval.
>
> The pwm_set_default_polarity and pwm_set_default_period should only be
> used by PWM drivers or the PWM core infrastructure to specify the
> default period and polarity values.
Would it make sense to put the prototypes of
pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h
then?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Uwe Kleine-König @ 2015-07-02 7:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-1-git-send-email-boris.brezillon@free-electrons.com>
Hello Boris,
On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
> This series adds support for atomic PWM update, or ITO, the capability
> to update all the parameters of a PWM device (enabled/disabled, period,
> duty and polarity) in one go.
on first reading the subject of your series I thought it was about
asserting that the newly set config is active before the call to
pwm_config (et al) returns. That's a problem I addressed a few times in
the past. I wonder if it's only me or if a different wording should be
used for "update all parameters with a single function call".
But other than that I like the series.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Tomi Valkeinen @ 2015-07-02 7:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150702070343.GD11824@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 822 bytes --]
On 02/07/15 10:03, Uwe Kleine-König wrote:
> Hello Boris,
>
> On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
>> This series adds support for atomic PWM update, or ITO, the capability
>> to update all the parameters of a PWM device (enabled/disabled, period,
>> duty and polarity) in one go.
> on first reading the subject of your series I thought it was about
> asserting that the newly set config is active before the call to
> pwm_config (et al) returns. That's a problem I addressed a few times in
> the past. I wonder if it's only me or if a different wording should be
> used for "update all parameters with a single function call".
In my vocabulary "blocking" means that the work is done before the
function returns, and "atomic" means the work is done in one step.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Boris Brezillon @ 2015-07-02 7:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150702070343.GD11824@pengutronix.de>
Hi Uwe,
On Thu, 2 Jul 2015 09:03:43 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> Hello Boris,
>
> On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
> > This series adds support for atomic PWM update, or ITO, the capability
> > to update all the parameters of a PWM device (enabled/disabled, period,
> > duty and polarity) in one go.
> on first reading the subject of your series I thought it was about
> asserting that the newly set config is active before the call to
> pwm_config (et al) returns. That's a problem I addressed a few times in
> the past. I wonder if it's only me or if a different wording should be
> used for "update all parameters with a single function call".
Yep, I can reword it differently.
>
> But other than that I like the series.
Great!
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Uwe Kleine-König @ 2015-07-02 7:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5594E56C.3010908@ti.com>
On Thu, Jul 02, 2015 at 10:17:00AM +0300, Tomi Valkeinen wrote:
>
>
> On 02/07/15 10:03, Uwe Kleine-König wrote:
> > Hello Boris,
> >
> > On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
> >> This series adds support for atomic PWM update, or ITO, the capability
> >> to update all the parameters of a PWM device (enabled/disabled, period,
> >> duty and polarity) in one go.
> > on first reading the subject of your series I thought it was about
> > asserting that the newly set config is active before the call to
> > pwm_config (et al) returns. That's a problem I addressed a few times in
> > the past. I wonder if it's only me or if a different wording should be
> > used for "update all parameters with a single function call".
>
> In my vocabulary "blocking" means that the work is done before the
> function returns, and "atomic" means the work is done in one step.
blocking is IMHO something slightly different, maybe "synchronous" is a
good term for "done when the call returns".
For write(2) I'd say
- blocking means to only return when the write request has reached the
kernel, but not necessarily the medium. I.e. the caller doesn't need
to care further; and
- atomic means that the contents of two concurrent writers don't mix in
the resulting file content; and
- synchronous means that once write() returns the data is on the
medium.
So atomic seems to be fine to use here.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [RFC PATCH 13/15] pwm: rockchip: add support for atomic update
From: Boris Brezillon @ 2015-07-02 7:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1709826.dVqTms8REP@diego>
Hi Heiko,
On Wed, 01 Jul 2015 23:48:31 +0200
Heiko Stübner <heiko@sntech.de> wrote:
> Hi Boris,
>
>
> Am Mittwoch, 1. Juli 2015, 10:21:59 schrieb Boris Brezillon:
> > Implement the ->apply() function to add support for atomic update.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > @@ -110,6 +113,26 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> > *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> > }
> >
> > +static void rockchip_pwm_init_v2(struct pwm_chip *chip, struct pwm_device
> > *pwm) +{
> > + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> > + u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
> > + PWM_CONTINUOUS;
> > + u32 val;
> > +
> > + val = readl(pc->base + pc->data->regs.ctrl);
> > +
> > + if ((val & enable_conf) != enable_conf)
> > + return;
> > +
> > + pwm->state.enabled = true;
> > +
> > + enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
> > +
> > + if ((val & enable_conf) = enable_conf)
> > + pwm->state.polarity = PWM_POLARITY_INVERSED;
>
> the inactive setting does not affect the polarity of the running pwm, only what
> to do when it gets turned off. Also PWM_DUTY_NEGATIVE is the "0" value for the
> bit so also is bad to compare against (and results in wrong readings). So I
> would suggest changing this like
Indeed
>
> - enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
> + enable_conf = PWM_DUTY_POSITIVE;
>
> - if ((val & enable_conf) = enable_conf)
> + if ((val & enable_conf) != enable_conf)
Or just:
if(val & PWM_DUTY_POSITIVE)
Thanks for the fix.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 12/15] pwm: rockchip: add initial state retrieval
From: Boris Brezillon @ 2015-07-02 7:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3258341.kaLi8uDfLr@diego>
On Wed, 01 Jul 2015 23:44:46 +0200
Heiko Stübner <heiko@sntech.de> wrote:
> Hi Boris,
>
> Am Mittwoch, 1. Juli 2015, 10:21:58 schrieb Boris Brezillon:
> > Implement the ->init_state() function to expose initial state.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
>
> [...]
>
> > @@ -98,6 +110,36 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> > *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> > }
> >
> > +static void rockchip_pwm_init_state(struct pwm_chip *chip,
> > + struct pwm_device *pwm)
> > +{
> > + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> > + unsigned long clk_rate;
> > + u64 tmp;
> > + int ret;
> > +
> > + ret = clk_enable(pc->clk);
> > + if (ret)
> > + return;
> > +
> > + clk_rate = clk_get_rate(pc->clk);
> > +
> > + tmp = readl(pc->base + pc->data->regs.period);
> > + tmp *= pc->data->prescaler * NSEC_PER_SEC;
> > + tmp = do_div(tmp, clk_rate);
>
> I guess you want to have the division result here and not the remainder, so
> - tmp = do_div(tmp, clk_rate);
> + do_div(tmp, clk_rate);
>
Oh crap. I make the same mistake over and over again.
[...]
> > static const struct rockchip_pwm_data pwm_data_v2 = {
> > @@ -207,6 +252,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
> > .prescaler = 1,
> > .ops = &rockchip_pwm_ops_v2,
> > .set_enable = rockchip_pwm_set_enable_v2,
> > + .init = rockchip_pwm_init_v2,
>
> you're referencing the v2 init here, but only add it in the next patch?
> [pwm: rockchip: add support for atomic update]
Yep, this function should be added in this patch.
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts
From: Boris Brezillon @ 2015-07-02 7:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150702064445.GC11824@pengutronix.de>
On Thu, 2 Jul 2015 08:44:45 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> On Wed, Jul 01, 2015 at 10:21:51AM +0200, Boris Brezillon wrote:
> > When requested by a user, the PWM is assigned a default period and polarity
> > extracted from the DT, the platform data or statically set by the driver.
> > Those default values are currently stored in the period and polarity
> > fields of the pwm_device struct, but they will be stored somewhere else
> > once we have introduced the architecture allowing for hardware state
> > retrieval.
> >
> > The pwm_set_default_polarity and pwm_set_default_period should only be
> > used by PWM drivers or the PWM core infrastructure to specify the
> > default period and polarity values.
> Would it make sense to put the prototypes of
> pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h
> then?
>
Yes, definitely. I was thinking about moving those functions/prototypes
into include/linux/pwm-provider.h, but I'm fine with
drivers/pwm/pwm-private.h too.
Thierry, any opinion ?
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Boris Brezillon @ 2015-07-02 7:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3250976.mr9RMtjrdC@diego>
On Wed, 01 Jul 2015 23:57:57 +0200
Heiko Stübner <heiko@sntech.de> wrote:
> Hi Boris,
>
> Am Mittwoch, 1. Juli 2015, 10:21:46 schrieb Boris Brezillon:
> > Hello Thierry,
> >
> > This series adds support for atomic PWM update, or ITO, the capability
> > to update all the parameters of a PWM device (enabled/disabled, period,
> > duty and polarity) in one go.
> >
> > This implementation is still experimental, and I may have missed some key
> > aspect, so any feedback are welcome.
> >
> > Also note that I haven't protected the state update with any locking.
> > That's because the existing config does not protect against concurrent
> > access to a requested PWM device (see the pwm_config implementation).
> > I guess the PWM framework assume the user will implement the proper locking
> > scheme if it has to concurrently access the device.
> >
> > The 5 first patches prepare the addition of the pwm_state concept, which
> > will be used to allow atomic updates.
> > The following patches introduce the pwm_state struct, initial state
> > retrieval and atomic update concepts.
> >
> > Patches 12 and 13 are showing how one can implement the initial state
> > retrieval and atomic update features in a PWM driver (in this specific
> > case I implemented it in the rockchip driver).
> >
> > The last 2 patches are making use of those changes to improve the
> > pwm-regulator driver (initializing the regulator state based on the
> > initial PWM state).
>
> at first I got very strange readings (very wrong values and wrong polarity),
> which resulted from the issues I pointed out in the replies to individual
> patches. After fixing these, the pwm read-back now returns exactly the expected
> values :-) .
Sorry about that, as I said I only compile tested the series :-/.
Anyway, thanks for providing fixes for these bugs, they'll be applied
in the next version.
>
> And with the original voltage table from the Chromeos-devicetrees, the pwm-
> regulator also returns the expected 1.2V that coreboot initially set.
Great! And thanks for testing the patches.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 16/15] pwm: add informations about polarity, duty cycle and period to debugfs
From: Boris Brezillon @ 2015-07-02 13:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3365087.n2eEES60JK@diego>
On Wed, 01 Jul 2015 23:50:46 +0200
Heiko Stübner <heiko@sntech.de> wrote:
> The pwm-states make it possible to also output the polarity, duty cycle
> and period information in the debugfs pwm summary-outout.
> This makes it easier to gather overview information about pwms without
> needing to walk through the sysfs attributes of every pwm.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
> might be nice to have too ;-)
Yes.
>
> drivers/pwm/core.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6dafd8e..79037a2 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -951,9 +951,18 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
> if (test_bit(PWMF_REQUESTED, &pwm->flags))
> seq_puts(s, " requested");
>
> - if (pwm_is_enabled(pwm))
> + if (pwm_is_enabled(pwm)) {
> seq_puts(s, " enabled");
>
> + seq_printf(s, " period:%uns",
> + pwm_get_period(pwm));
> + seq_printf(s, " duty:%uns",
> + pwm_get_duty_cycle(pwm));
> + seq_printf(s, " polarity:%s",
> + pwm_get_polarity(pwm) ? "inverse"
> + : "normal");
I would print those values even if the PWM is not enabled.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox