* [PATCH v2 01/10] pwm: introduce default period and polarity concepts
From: Boris Brezillon @ 2015-07-20 15:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1437406327-6207-1-git-send-email-boris.brezillon@free-electrons.com>
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.
PWM users might call the pwm_get_default_period to query the default
period value. There is currently no helper to query the default
polarity, but it might be added later on if there is a need for it.
This patch also modifies all the places where the default helpers should
be used in place of the standard ones.
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
drivers/leds/leds-pwm.c | 2 +-
drivers/pwm/core.c | 14 +++++++-------
drivers/pwm/pwm-pxa.c | 2 +-
drivers/pwm/pwm-sun4i.c | 3 ++-
drivers/regulator/pwm-regulator.c | 2 +-
drivers/video/backlight/lm3630a_bl.c | 4 ++--
drivers/video/backlight/pwm_bl.c | 2 +-
drivers/video/fbdev/ssd1307fb.c | 2 +-
include/linux/pwm.h | 17 +++++++++++++++++
9 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 1d07e3e..2c564d1 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -125,7 +125,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
if (led_data->can_sleep)
INIT_WORK(&led_data->work, led_pwm_work);
- led_data->period = pwm_get_period(led_data->pwm);
+ led_data->period = pwm_get_default_period(led_data->pwm);
if (!led_data->period && (led->pwm_period_ns > 0))
led_data->period = led->pwm_period_ns;
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index f7c11d2..7ffad2b 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -146,12 +146,12 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
if (IS_ERR(pwm))
return pwm;
- pwm_set_period(pwm, args->args[1]);
+ pwm_set_default_period(pwm, args->args[1]);
if (args->args[2] & PWM_POLARITY_INVERTED)
- pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
+ pwm_set_default_polarity(pwm, PWM_POLARITY_INVERSED);
else
- pwm_set_polarity(pwm, PWM_POLARITY_NORMAL);
+ pwm_set_default_polarity(pwm, PWM_POLARITY_NORMAL);
return pwm;
}
@@ -172,7 +172,7 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
if (IS_ERR(pwm))
return pwm;
- pwm_set_period(pwm, args->args[1]);
+ pwm_set_default_period(pwm, args->args[1]);
return pwm;
}
@@ -262,7 +262,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
pwm->chip = chip;
pwm->pwm = chip->base + i;
pwm->hwpwm = i;
- pwm->polarity = polarity;
+ pwm_set_default_polarity(pwm, polarity);
radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
}
@@ -704,8 +704,8 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
if (IS_ERR(pwm))
goto out;
- pwm_set_period(pwm, chosen->period);
- pwm_set_polarity(pwm, chosen->polarity);
+ pwm_set_default_period(pwm, chosen->period);
+ pwm_set_default_polarity(pwm, chosen->polarity);
out:
mutex_unlock(&pwm_lookup_lock);
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index cb2f702..65b80aa 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -160,7 +160,7 @@ pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
if (IS_ERR(pwm))
return pwm;
- pwm_set_period(pwm, args->args[0]);
+ pwm_set_default_period(pwm, args->args[0]);
return pwm;
}
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index cd9dde5..a364fb7 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -333,7 +333,8 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
val = sun4i_pwm_readl(pwm, PWM_CTRL_REG);
for (i = 0; i < pwm->chip.npwm; i++)
if (!(val & BIT_CH(PWM_ACT_STATE, i)))
- pwm->chip.pwms[i].polarity = PWM_POLARITY_INVERSED;
+ pwm_set_default_polarity(&pwm->chip.pwms[i],
+ PWM_POLARITY_INVERSED);
clk_disable_unprepare(pwm->clk);
return 0;
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index ffa9612..12b4d9d 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -46,7 +46,7 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
int dutycycle;
int ret;
- pwm_reg_period = pwm_get_period(drvdata->pwm);
+ pwm_reg_period = pwm_get_default_period(drvdata->pwm);
dutycycle = (pwm_reg_period *
drvdata->duty_cycle_table[selector].dutycycle) / 100;
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 35fe482..449ebc3 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -162,7 +162,7 @@ static int lm3630a_intr_config(struct lm3630a_chip *pchip)
static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
{
- unsigned int period = pwm_get_period(pchip->pwmd);
+ unsigned int period = pwm_get_default_period(pchip->pwmd);
unsigned int duty = br * period / br_max;
pwm_config(pchip->pwmd, duty, period);
@@ -425,7 +425,7 @@ static int lm3630a_probe(struct i2c_client *client,
return PTR_ERR(pchip->pwmd);
}
}
- pchip->pwmd->period = pdata->pwm_period;
+ pwm_set_default_period(pchip->pwmd, pdata->pwm_period);
/* interrupt enable : irq 0 is not allowed */
pchip->irq = client->irq;
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index eff379b..ae498c1 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -294,7 +294,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
* set the period from platform data if it has not already been set
* via the PWM lookup table.
*/
- pb->period = pwm_get_period(pb->pwm);
+ pb->period = pwm_get_default_period(pb->pwm);
if (!pb->period && (data->pwm_period_ns > 0)) {
pb->period = data->pwm_period_ns;
pwm_set_period(pb->pwm, data->pwm_period_ns);
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 3e153c0..6949626 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -294,7 +294,7 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
return PTR_ERR(par->pwm);
}
- par->pwm_period = pwm_get_period(par->pwm);
+ par->pwm_period = pwm_get_default_period(par->pwm);
/* Enable the PWM */
pwm_config(par->pwm, par->pwm_period / 2, par->pwm_period);
pwm_enable(par->pwm);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 6f286df..ba4b7ed 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -103,11 +103,22 @@ static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
pwm->period = period;
}
+static inline void pwm_set_default_period(struct pwm_device *pwm,
+ unsigned int period)
+{
+ pwm_set_period(pwm, period);
+}
+
static inline unsigned int pwm_get_period(const struct pwm_device *pwm)
{
return pwm ? pwm->period : 0;
}
+static inline unsigned int pwm_get_default_period(const struct pwm_device *pwm)
+{
+ return pwm_get_period(pwm);
+}
+
static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)
{
if (pwm)
@@ -124,6 +135,12 @@ static inline unsigned int pwm_get_duty_cycle(const struct pwm_device *pwm)
*/
int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity);
+static inline void pwm_set_default_polarity(struct pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ pwm_set_polarity(pwm, polarity);
+}
+
static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
{
return pwm ? pwm->polarity : PWM_POLARITY_NORMAL;
--
1.9.1
^ permalink raw reply related
* [PATCH v2 00/10] pwm: add support for atomic update
From: Boris Brezillon @ 2015-07-20 15:31 UTC (permalink / raw)
To: linux-arm-kernel
Hello Thierry,
This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
Best Regards,
Boris
Changes since v1:
- dropped applied patches
- squashed Heiko's fixes into the rockchip driver changes
- made a few cosmetic changes
- added kerneldoc comments
- added Heiko's patch to display more information in debugfs
- dropped pwm-regulator patches (should be submitted separately)
Boris Brezillon (9):
pwm: introduce default period and polarity concepts
pwm: define a new pwm_state struct
pwm: move the enabled/disabled info to pwm_state struct
backlight: pwm_bl: remove useless call to pwm_set_period
pwm: declare a default PWM state
pwm: add the PWM initial state retrieval infra
pwm: add the core infrastructure to allow atomic update
pwm: rockchip: add initial state retrieval
pwm: rockchip: add support for atomic update
Heiko Stübner (1):
pwm: add information about polarity, duty cycle and period to debugfs
drivers/leds/leds-pwm.c | 2 +-
drivers/pwm/core.c | 169 +++++++++++++++++++++++++++++++----
drivers/pwm/pwm-pxa.c | 2 +-
drivers/pwm/pwm-rockchip.c | 119 +++++++++++++++++++-----
drivers/pwm/pwm-sun4i.c | 3 +-
drivers/regulator/pwm-regulator.c | 2 +-
drivers/video/backlight/lm3630a_bl.c | 4 +-
drivers/video/backlight/pwm_bl.c | 10 ++-
drivers/video/fbdev/ssd1307fb.c | 2 +-
include/linux/pwm.h | 86 +++++++++++++++---
10 files changed, 336 insertions(+), 63 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH] backlight: pm8941-wled: Move PM8941 WLED driver to backlight
From: Jingoo Han @ 2015-07-20 14:22 UTC (permalink / raw)
To: 'Bjorn Andersson'
Cc: 'Rob Herring', 'Pawel Moll',
'Mark Rutland', 'Ian Campbell',
'Kumar Gala', 'Bryan Wu',
'Richard Purdie', 'Jacek Anaszewski',
'Lee Jones', 'Jean-Christophe Plagniol-Villard',
'Tomi Valkeinen', 'Rob Clark', devicetree,
linux-kernel, linux-leds, linux-fbdev, linux-arm-msm,
'Jingoo Han'
In-Reply-To: <1436990540-23354-1-git-send-email-bjorn.andersson@sonymobile.com>
On Thursday, July 16, 2015 5:02 AM, Bjorn Andersson wrote:
>
> The Qualcomm PM8941 WLED block is used for backlight and should therefor
> be in the backlight framework and not in the LED framework. This moves
> the driver and adapts to the backlight api instead.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> .../devicetree/bindings/leds/leds-pm8941-wled.txt | 5 +-
> drivers/leds/Kconfig | 8 ---
> drivers/leds/Makefile | 1 -
> drivers/video/backlight/Kconfig | 8 +++
> drivers/video/backlight/Makefile | 1 +
> .../backlight/pm8941-wled.c} | 59 ++++++++++------------
> 6 files changed, 36 insertions(+), 46 deletions(-)
> rename drivers/{leds/leds-pm8941-wled.c => video/backlight/pm8941-wled.c} (90%)
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt
> b/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt
> index a85a964d61f5..424f8444a6cd 100644
> --- a/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt
Please move this txt file to 'backlight' directory.
> @@ -5,10 +5,7 @@ Required properties:
> - reg: slave address
>
> Optional properties:
> -- label: The label for this led
> - See Documentation/devicetree/bindings/leds/common.txt
> -- linux,default-trigger: Default trigger assigned to the LED
> - See Documentation/devicetree/bindings/leds/common.txt
> +- label: The name of the backlight device
> - qcom,cs-out: bool; enable current sink output
> - qcom,cabc: bool; enable content adaptive backlight control
> - qcom,ext-gen: bool; use externally generated modulator signal to dim
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9ad35f72ab4c..b8d4b965ca2a 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -578,14 +578,6 @@ config LEDS_VERSATILE
> This option enabled support for the LEDs on the ARM Versatile
> and RealView boards. Say Y to enabled these.
>
> -config LEDS_PM8941_WLED
> - tristate "LED support for the Qualcomm PM8941 WLED block"
> - depends on LEDS_CLASS
> - select REGMAP
> - help
> - This option enables support for the 'White' LED block
> - on Qualcomm PM8941 PMICs.
> -
> comment "LED Triggers"
> source "drivers/leds/trigger/Kconfig"
>
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 8d6a24a2f513..abe96d960ebe 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -63,7 +63,6 @@ obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
> obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o
> -obj-$(CONFIG_LEDS_PM8941_WLED) += leds-pm8941-wled.o
> obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o
>
> # LED SPI Drivers
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 0505b796d743..6c67c5430933 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -299,6 +299,14 @@ config BACKLIGHT_TOSA
> If you have an Sharp SL-6000 Zaurus say Y to enable a driver
> for its backlight
>
> +config BACKLIGHT_PM8941_WLED
> + tristate "Qualcomm PM8941 WLED Driver"
> + depends on LEDS_CLASS
LEDS_CLASS?
Please check your patch carefully.
One more thing,
Did you test this patch with the board?
If not, I will not accept this patch, unless other people send 'Tested-by'.
Best regards,
Jingoo Han
> + select REGMAP
> + help
> + If you have the Qualcomm PM8941, say Y to enable a driver for the
> + WLED block.
> +
> config BACKLIGHT_SAHARA
> tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
> depends on X86
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index d67073f9d421..16ec534cff30 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_BACKLIGHT_OMAP1) += omap1_bl.o
> obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o
> obj-$(CONFIG_BACKLIGHT_PANDORA) += pandora_bl.o
> obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
> +obj-$(CONFIG_BACKLIGHT_PM8941_WLED) += pm8941-wled.o
> obj-$(CONFIG_BACKLIGHT_PWM) += pwm_bl.o
> obj-$(CONFIG_BACKLIGHT_SAHARA) += kb3886_bl.o
> obj-$(CONFIG_BACKLIGHT_SKY81452) += sky81452-backlight.o
> diff --git a/drivers/leds/leds-pm8941-wled.c b/drivers/video/backlight/pm8941-wled.c
> similarity index 90%
> rename from drivers/leds/leds-pm8941-wled.c
> rename to drivers/video/backlight/pm8941-wled.c
> index bf64a593fbf1..4f5ae95331a1 100644
> --- a/drivers/leds/leds-pm8941-wled.c
> +++ b/drivers/video/backlight/pm8941-wled.c
> @@ -11,7 +11,7 @@
> */
>
> #include <linux/kernel.h>
> -#include <linux/leds.h>
> +#include <linux/backlight.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> @@ -76,30 +76,29 @@ struct pm8941_wled_config {
> };
>
> struct pm8941_wled {
> + const char *name;
> struct regmap *regmap;
> u16 addr;
>
> - struct led_classdev cdev;
> -
> struct pm8941_wled_config cfg;
> };
>
> -static int pm8941_wled_set(struct led_classdev *cdev,
> - enum led_brightness value)
> +static int pm8941_wled_update_status(struct backlight_device *bl)
> {
> - struct pm8941_wled *wled;
> + struct pm8941_wled *wled = bl_get_data(bl);
> + u16 val = bl->props.brightness;
> u8 ctrl = 0;
> - u16 val;
> int rc;
> int i;
>
> - wled = container_of(cdev, struct pm8941_wled, cdev);
> + if (bl->props.power != FB_BLANK_UNBLANK ||
> + bl->props.fb_blank != FB_BLANK_UNBLANK ||
> + bl->props.state & BL_CORE_FBBLANK)
> + val = 0;
>
> - if (value != 0)
> + if (val != 0)
> ctrl = PM8941_WLED_REG_MOD_EN_BIT;
>
> - val = value * PM8941_WLED_REG_VAL_MAX / LED_FULL;
> -
> rc = regmap_update_bits(wled->regmap,
> wled->addr + PM8941_WLED_REG_MOD_EN,
> PM8941_WLED_REG_MOD_EN_MASK, ctrl);
> @@ -128,16 +127,6 @@ static int pm8941_wled_set(struct led_classdev *cdev,
> return rc;
> }
>
> -static void pm8941_wled_set_brightness(struct led_classdev *cdev,
> - enum led_brightness value)
> -{
> - if (pm8941_wled_set(cdev, value)) {
> - dev_err(cdev->dev, "Unable to set brightness\n");
> - return;
> - }
> - cdev->brightness = value;
> -}
> -
> static int pm8941_wled_setup(struct pm8941_wled *wled)
> {
> int rc;
> @@ -336,12 +325,9 @@ static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
> }
> wled->addr = val;
>
> - rc = of_property_read_string(dev->of_node, "label", &wled->cdev.name);
> + rc = of_property_read_string(dev->of_node, "label", &wled->name);
> if (rc)
> - wled->cdev.name = dev->of_node->name;
> -
> - wled->cdev.default_trigger = of_get_property(dev->of_node,
> - "linux,default-trigger", NULL);
> + wled->name = dev->of_node->name;
>
> *cfg = pm8941_wled_config_defaults;
> for (i = 0; i < ARRAY_SIZE(u32_opts); ++i) {
> @@ -377,8 +363,14 @@ static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
> return 0;
> }
>
> +static const struct backlight_ops pm8941_wled_ops = {
> + .update_status = pm8941_wled_update_status,
> +};
> +
> static int pm8941_wled_probe(struct platform_device *pdev)
> {
> + struct backlight_properties props;
> + struct backlight_device *bl;
> struct pm8941_wled *wled;
> struct regmap *regmap;
> int rc;
> @@ -403,13 +395,14 @@ static int pm8941_wled_probe(struct platform_device *pdev)
> if (rc)
> return rc;
>
> - wled->cdev.brightness_set = pm8941_wled_set_brightness;
> -
> - rc = devm_led_classdev_register(&pdev->dev, &wled->cdev);
> - if (rc)
> - return rc;
> -
> - platform_set_drvdata(pdev, wled);
> + memset(&props, 0, sizeof(struct backlight_properties));
> + props.type = BACKLIGHT_RAW;
> + props.max_brightness = PM8941_WLED_REG_VAL_MAX;
> + bl = devm_backlight_device_register(&pdev->dev, wled->name,
> + &pdev->dev, wled,
> + &pm8941_wled_ops, &props);
> + if (IS_ERR(bl))
> + return PTR_ERR(bl);
>
> return 0;
> };
> --
> 1.8.2.2
^ permalink raw reply
* Re: [PATCH] backlight: pm8941-wled: Move PM8941 WLED driver to backlight
From: Jingoo Han @ 2015-07-20 14:15 UTC (permalink / raw)
To: 'Jacek Anaszewski', 'Bjorn Andersson'
Cc: 'Rob Herring', 'Pawel Moll',
'Mark Rutland', 'Ian Campbell',
'Kumar Gala', 'Bryan Wu',
'Richard Purdie', 'Lee Jones',
'Jean-Christophe Plagniol-Villard',
'Tomi Valkeinen', 'Rob Clark', devicetree,
linux-kernel, linux-leds, linux-fbdev, linux-arm-msm,
'Jingoo Han'
In-Reply-To: <55A764C5.5060205@samsung.com>
On Thursday, July 16, 2015 5:01 PM, Jacek Anaszewski wrote:
> On 07/15/2015 10:02 PM, Bjorn Andersson wrote:
> > The Qualcomm PM8941 WLED block is used for backlight and should therefor
> > be in the backlight framework and not in the LED framework. This moves
> > the driver and adapts to the backlight api instead.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > ---
> > .../devicetree/bindings/leds/leds-pm8941-wled.txt | 5 +-
> > drivers/leds/Kconfig | 8 ---
> > drivers/leds/Makefile | 1 -
> > drivers/video/backlight/Kconfig | 8 +++
> > drivers/video/backlight/Makefile | 1 +
> > .../backlight/pm8941-wled.c} | 59 ++++++++++------------
> > 6 files changed, 36 insertions(+), 46 deletions(-)
> > rename drivers/{leds/leds-pm8941-wled.c => video/backlight/pm8941-wled.c} (90%)
>
> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
If you are not an author of this driver or a maintainer affected by this patch,
Just 'Reviewed-by' looks good.
Best regards,
Jingoo Han
>
> --
> Best Regards,
> Jacek Anaszewski
^ permalink raw reply
* Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct
From: Boris Brezillon @ 2015-07-20 10:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720100925.GX29614@ulmo>
On Mon, 20 Jul 2015 12:09:26 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Jul 20, 2015 at 12:01:16PM +0200, Boris Brezillon wrote:
> > On Mon, 20 Jul 2015 10:04:59 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
> > > [...]
> > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > [...]
> > > > +struct pwm_state {
> > > > + unsigned int period; /* in nanoseconds */
> > > > + unsigned int duty_cycle; /* in nanoseconds */
> > > > + enum pwm_polarity polarity;
> > > > +};
> > >
> > > No need for the extra padding here.
> >
> > What do you mean by "extra padding" ?
> > I just reused the indentation used in the pwm_device struct.
>
> Yeah, I have a local patch to fix that up. I find it useless to pad
> things like this, and it has the downside that it will become totally
> inconsistent (or cause a lot of churn by reformatting) if ever you add a
> field that extends beyond the padding. Single spaces don't have any such
> drawbacks and, in my opinion, look just as good.
I prefer the single space approach too, so I won't complain ;-).
>
> > Would you prefer something like that ?
> >
> > struct pwm_state {
> > unsigned int period; /* in nanoseconds */
> > unsigned int duty_cycle; /* in nanoseconds */
> > enum pwm_polarity polarity;
> > };
>
> Yeah. I'd say even the comments would be more suited in a kerneldoc-
> style comment:
>
> /**
> * struct pwm_state - state of a PWM channel
> * @period: PWM period (in nanoseconds)
> * @duty_cycle: PWM duty cycle (in nanoseconds)
> * @polarity: PWM polarity
> */
> struct pwm_state {
> unsigned int period;
> unsigned int duty_cycle;
> enum pwm_polarity polarity;
> };
>
> This is something that users will need to deal with, so eventually
> somebody might look at this via some DocBook generated HTML or PDF.
I agree.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct
From: Thierry Reding @ 2015-07-20 10:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720120116.2358b829@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]
On Mon, Jul 20, 2015 at 12:01:16PM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:04:59 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
> > [...]
> > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > [...]
> > > +struct pwm_state {
> > > + unsigned int period; /* in nanoseconds */
> > > + unsigned int duty_cycle; /* in nanoseconds */
> > > + enum pwm_polarity polarity;
> > > +};
> >
> > No need for the extra padding here.
>
> What do you mean by "extra padding" ?
> I just reused the indentation used in the pwm_device struct.
Yeah, I have a local patch to fix that up. I find it useless to pad
things like this, and it has the downside that it will become totally
inconsistent (or cause a lot of churn by reformatting) if ever you add a
field that extends beyond the padding. Single spaces don't have any such
drawbacks and, in my opinion, look just as good.
> Would you prefer something like that ?
>
> struct pwm_state {
> unsigned int period; /* in nanoseconds */
> unsigned int duty_cycle; /* in nanoseconds */
> enum pwm_polarity polarity;
> };
Yeah. I'd say even the comments would be more suited in a kerneldoc-
style comment:
/**
* struct pwm_state - state of a PWM channel
* @period: PWM period (in nanoseconds)
* @duty_cycle: PWM duty cycle (in nanoseconds)
* @polarity: PWM polarity
*/
struct pwm_state {
unsigned int period;
unsigned int duty_cycle;
enum pwm_polarity polarity;
};
This is something that users will need to deal with, so eventually
somebody might look at this via some DocBook generated HTML or PDF.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 11/15] pwm: add the core infrastructure to allow atomic update
From: Thierry Reding @ 2015-07-20 10:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720114827.2e5d52a5@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 2001 bytes --]
On Mon, Jul 20, 2015 at 11:48:27AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:59:40 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
[...]
> > > +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> > > +{
> > > + int err = 0;
> > > +
> > > + if (!pwm)
> > > + return -EINVAL;
> > > +
> > > + if (!memcmp(state, &pwm->state, sizeof(*state)))
> > > + return 0;
> > > +
> > > + if (pwm->chip->ops->apply) {
> > > + err = pwm->chip->ops->apply(pwm->chip, pwm, state);
> > > + if (!err)
> > > + pwm->state = *state;
> >
> > Maybe we want pwm_set_state() for this?
>
> I'm not opposed to the addition of the pwm_set_state() function as long
> as it's a private one: I don't want to let PMW drivers or users mess up
> with the current PWM state.
Yeah, it could be a static function in core.c. What I want to avoid is
having to change a bunch of code if ever state assignment becomes
something other than merely copying a structure.
[...]
> > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > index b47244a..7e99679 100644
> > > --- a/include/linux/pwm.h
> > > +++ b/include/linux/pwm.h
> > > @@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
> > > return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
> > > }
> > >
> > > +/*
> > > + * pwm_apply_state - apply a new state to the PWM device
> > > + */
> > > +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
> >
> > If you add kerneldoc, please add it properly. It should start with /**
> > and you need to list at least the parameters and return value.
>
> Yes, I'll fix that.
> BTW, I remember that you were expecting another name for this function
> (pwm_update IIRC).
I don't mind the pwm_apply_state() name very much. It's pretty accurate
with regards to what it does.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct
From: Boris Brezillon @ 2015-07-20 10:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720080458.GG29614@ulmo>
On Mon, 20 Jul 2015 10:04:59 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
> [...]
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> [...]
> > +struct pwm_state {
> > + unsigned int period; /* in nanoseconds */
> > + unsigned int duty_cycle; /* in nanoseconds */
> > + enum pwm_polarity polarity;
> > +};
>
> No need for the extra padding here.
What do you mean by "extra padding" ?
I just reused the indentation used in the pwm_device struct.
Would you prefer something like that ?
struct pwm_state {
unsigned int period; /* in nanoseconds */
unsigned int duty_cycle; /* in nanoseconds */
enum pwm_polarity polarity;
};
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Thierry Reding @ 2015-07-20 10:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720115704.0c64d070@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 4089 bytes --]
On Mon, Jul 20, 2015 at 11:57:04AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 11:10:04 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Mon, Jul 20, 2015 at 10:50:03AM +0200, Boris Brezillon wrote:
> > > On Mon, 20 Jul 2015 10:36:50 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > > On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > > > > On Mon, 20 Jul 2015 10:16:00 +0200
> > > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > >
> > > > > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > > > > > > The PWM period will be set when calling pwm_config. Remove this useless
> > > > > > > call to pwm_set_period, which might mess up with the initial PWM state
> > > > > > > once we have added proper support for PWM init state retrieval.
> > > > > > >
> > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > > > ---
> > > > > > > drivers/video/backlight/pwm_bl.c | 4 +---
> > > > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > > > index ae498c1..fe5597c 100644
> > > > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > > > > * via the PWM lookup table.
> > > > > > > */
> > > > > > > pb->period = pwm_get_default_period(pb->pwm);
> > > > > > > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > > > > > > + if (!pb->period && (data->pwm_period_ns > 0))
> > > > > > > pb->period = data->pwm_period_ns;
> > > > > > > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > > > > > > - }
> > > > > > >
> > > > > > > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
> > > > > >
> > > > > > As far as I remember this line is there in order to pass in a period if
> > > > > > the backlight driver is initialized from board setup files. In such a
> > > > > > case there won't be an period associated with the PWM channel in the
> > > > > > first place.
> > > > > >
> > > > > > I think even with the introduction of a default period, we'd be missing
> > > > > > out on the board setup case because there is no standard place where it
> > > > > > is being set, so it must come from the platform data.
> > > > >
> > > > > AFAICT, we don't need to explicitly set the period when probing the
> > > > > backlight device, because it will be set next time we call
> > > > > pwm_config(), and since we're passing pb->period when calling
> > > > > pwm_config() everything should be fine.
> > > >
> > > > Calling pwm_set_period() is still good for consistency. Consider for
> > > > example what happens if after the driver were to call pwm_get_period().
> > > > It would return some more or less random value (likely 0 or whatever it
> > > > had been set to by an earlier user).
> > >
> > > Yes, that's true in general, but in this specific driver
> > > pwm_get_period() is never called, and the driver only relies on the
> > > pb->period value.
> >
> > Perhaps that's something that should change. If the PWM core has all
> > this infrastructure there should be no need for the backlight driver to
> > keep it's own copy of that variable.
>
> Yes, probably. In any case, I don't think we want PWM users to be able
> to mess up with the current or default PWM state, that's why I was
> planning on making the pwm_set_default_xxx helpers private to PWM
> drivers and core infrastructure.
>
> Also note that if we keep this assignment it should at least be changed
> to a pwm_set_default_period() so that it does not override the current
> PWM state.
I think we should be able to live without the assignment. Perhaps when
replacing it, add a comment saying that this is for very legacy cases
only and that PWM lookup tables are the right way to fix this.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Boris Brezillon @ 2015-07-20 9:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720091003.GN29614@ulmo>
On Mon, 20 Jul 2015 11:10:04 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Jul 20, 2015 at 10:50:03AM +0200, Boris Brezillon wrote:
> > On Mon, 20 Jul 2015 10:36:50 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > > > On Mon, 20 Jul 2015 10:16:00 +0200
> > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > > > > > The PWM period will be set when calling pwm_config. Remove this useless
> > > > > > call to pwm_set_period, which might mess up with the initial PWM state
> > > > > > once we have added proper support for PWM init state retrieval.
> > > > > >
> > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > > ---
> > > > > > drivers/video/backlight/pwm_bl.c | 4 +---
> > > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > > index ae498c1..fe5597c 100644
> > > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > > > * via the PWM lookup table.
> > > > > > */
> > > > > > pb->period = pwm_get_default_period(pb->pwm);
> > > > > > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > > > > > + if (!pb->period && (data->pwm_period_ns > 0))
> > > > > > pb->period = data->pwm_period_ns;
> > > > > > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > > > > > - }
> > > > > >
> > > > > > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
> > > > >
> > > > > As far as I remember this line is there in order to pass in a period if
> > > > > the backlight driver is initialized from board setup files. In such a
> > > > > case there won't be an period associated with the PWM channel in the
> > > > > first place.
> > > > >
> > > > > I think even with the introduction of a default period, we'd be missing
> > > > > out on the board setup case because there is no standard place where it
> > > > > is being set, so it must come from the platform data.
> > > >
> > > > AFAICT, we don't need to explicitly set the period when probing the
> > > > backlight device, because it will be set next time we call
> > > > pwm_config(), and since we're passing pb->period when calling
> > > > pwm_config() everything should be fine.
> > >
> > > Calling pwm_set_period() is still good for consistency. Consider for
> > > example what happens if after the driver were to call pwm_get_period().
> > > It would return some more or less random value (likely 0 or whatever it
> > > had been set to by an earlier user).
> >
> > Yes, that's true in general, but in this specific driver
> > pwm_get_period() is never called, and the driver only relies on the
> > pb->period value.
>
> Perhaps that's something that should change. If the PWM core has all
> this infrastructure there should be no need for the backlight driver to
> keep it's own copy of that variable.
Yes, probably. In any case, I don't think we want PWM users to be able
to mess up with the current or default PWM state, that's why I was
planning on making the pwm_set_default_xxx helpers private to PWM
drivers and core infrastructure.
Also note that if we keep this assignment it should at least be changed
to a pwm_set_default_period() so that it does not override the current
PWM state.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 11/15] pwm: add the core infrastructure to allow atomic update
From: Boris Brezillon @ 2015-07-20 9:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720085939.GL29614@ulmo>
On Mon, 20 Jul 2015 10:59:40 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
> > Add an ->apply() method to the pwm_ops struct to allow PWM drivers to
> > implement atomic update.
> > This method will be prefered over the ->enable(), ->disable() and
> > ->config() methods if available.
> >
> > Add the pwm_get_state(), pwm_get_default_state() and pwm_apply_state()
> > functions for PWM users to be able to use the atomic update feature.
> >
> > Note that the pwm_apply_state() does not guarantee the atomicity of the
> > update operation, it all depends on the availability and implementation
> > of the ->apply() method.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > drivers/pwm/core.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
> > include/linux/pwm.h | 26 +++++++++++++
> > 2 files changed, 124 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 30631f5..6dafd8e 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -238,8 +238,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > unsigned int i;
> > int ret;
> >
> > - if (!chip || !chip->dev || !chip->ops || !chip->ops->config ||
> > - !chip->ops->enable || !chip->ops->disable || !chip->npwm)
> > + if (!chip || !chip->dev || !chip->ops || (!chip->ops->apply &&
> > + (!chip->ops->config || !chip->ops->enable ||
> > + !chip->ops->disable)) || !chip->npwm)
> > return -EINVAL;
>
> This is becoming really unreadable, perhaps split it into two checks, or
> even split out the sanity check on the ops into a separate function to
> make the negations easier to read:
>
> static bool pwm_ops_check(const struct pwm_ops *ops)
> {
> /* driver supports legacy, non-atomic operation */
> if (ops->config && ops->enable && ops->disable)
> return true;
>
> /* driver supports atomic operation */
> if (ops->apply)
> return true;
>
> return false;
> }
>
> and then use this:
>
> if (!chip || !chip->dev || !chip->ops || !chip->npwm)
> return -EINVAL;
>
> if (!pwm_ops_check(chip->ops))
> return -EINVAL;
>
Sure, I'll change that to make it more readable.
> > mutex_lock(&pwm_lock);
> > @@ -430,7 +431,17 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> > if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
> > return -EINVAL;
> >
> > - err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> > + if (pwm->chip->ops->apply) {
> > + struct pwm_state state = pwm->state;
>
> Shouldn't this use pwm_get_state()?
Yes, I'll fix all of them
[...]
>
> > +
> > + state.enabled = true;
> > + err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
>
> There should be a space between the above two lines.
I'll add an empty line.
>
> >
> > +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> > +{
> > + int err = 0;
> > +
> > + if (!pwm)
> > + return -EINVAL;
> > +
> > + if (!memcmp(state, &pwm->state, sizeof(*state)))
> > + return 0;
> > +
> > + if (pwm->chip->ops->apply) {
> > + err = pwm->chip->ops->apply(pwm->chip, pwm, state);
> > + if (!err)
> > + pwm->state = *state;
>
> Maybe we want pwm_set_state() for this?
I'm not opposed to the addition of the pwm_set_state() function as long
as it's a private one: I don't want to let PMW drivers or users mess up
with the current PWM state.
>
> > + } else {
> > + /*
> > + * FIXME: restore the initial state in case of error.
> > + */
> > + if (state->polarity != pwm->state.polarity) {
> > + pwm_disable(pwm);
> > + err = pwm_set_polarity(pwm, state->polarity);
> > + if (err)
> > + goto out;
> > + }
> > +
> > + if (state->period != pwm->state.period ||
> > + state->duty_cycle != pwm->state.duty_cycle) {
> > + err = pwm_config(pwm, state->period, state->duty_cycle);
> > + if (err)
> > + goto out;
> > + }
> > +
> > + if (state->enabled != pwm->state.enabled) {
> > + if (state->enabled)
> > + err = pwm_enable(pwm);
> > + else
> > + pwm_disable(pwm);
> > + }
> > + }
> > +
> > +out:
> > + return err;
> > +}
> > +EXPORT_SYMBOL_GPL(pwm_apply_state);
> > +
> > static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
> > {
> > struct pwm_chip *chip;
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index b47244a..7e99679 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
> > return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
> > }
> >
> > +/*
> > + * pwm_apply_state - apply a new state to the PWM device
> > + */
> > +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
>
> If you add kerneldoc, please add it properly. It should start with /**
> and you need to list at least the parameters and return value.
Yes, I'll fix that.
BTW, I remember that you were expecting another name for this function
(pwm_update IIRC).
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 10/15] pwm: add the PWM initial state retrieval infra
From: Boris Brezillon @ 2015-07-20 9:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720090123.GM29614@ulmo>
On Mon, 20 Jul 2015 11:01:24 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Jul 01, 2015 at 10:21:56AM +0200, Boris Brezillon wrote:
> > Add a ->init_state() function to the pwm_ops struct to let PWM drivers
> > initialize the PWM state attached to a PWM device.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > drivers/pwm/core.c | 3 +++
> > include/linux/pwm.h | 2 ++
> > 2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 3e830ce..30631f5 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -264,6 +264,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > pwm->hwpwm = i;
> > pwm_set_default_polarity(pwm, polarity);
> >
> > + if (chip->ops->init_state)
> > + chip->ops->init_state(chip, pwm);
> > +
> > radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
> > }
> >
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index 0f36a06..b47244a 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -177,6 +177,8 @@ struct pwm_ops {
> > struct pwm_device *pwm);
> > void (*disable)(struct pwm_chip *chip,
> > struct pwm_device *pwm);
> > + void (*init_state)(struct pwm_chip *chip,
> > + struct pwm_device *pwm);
>
> I think I'd call this reset_state. init has this connotation of setting
> a set of default values. For reset it's clearer in my opinion that it's
> resetting to the hardware state.
I'm fine with the reset_state name, I'll change that in my v2.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Thierry Reding @ 2015-07-20 9:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720105003.29b5813a@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 4159 bytes --]
On Mon, Jul 20, 2015 at 10:50:03AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:36:50 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > > On Mon, 20 Jul 2015 10:16:00 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > > > > The PWM period will be set when calling pwm_config. Remove this useless
> > > > > call to pwm_set_period, which might mess up with the initial PWM state
> > > > > once we have added proper support for PWM init state retrieval.
> > > > >
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > ---
> > > > > drivers/video/backlight/pwm_bl.c | 4 +---
> > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > index ae498c1..fe5597c 100644
> > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > > * via the PWM lookup table.
> > > > > */
> > > > > pb->period = pwm_get_default_period(pb->pwm);
> > > > > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > > > > + if (!pb->period && (data->pwm_period_ns > 0))
> > > > > pb->period = data->pwm_period_ns;
> > > > > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > > > > - }
> > > > >
> > > > > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
> > > >
> > > > As far as I remember this line is there in order to pass in a period if
> > > > the backlight driver is initialized from board setup files. In such a
> > > > case there won't be an period associated with the PWM channel in the
> > > > first place.
> > > >
> > > > I think even with the introduction of a default period, we'd be missing
> > > > out on the board setup case because there is no standard place where it
> > > > is being set, so it must come from the platform data.
> > >
> > > AFAICT, we don't need to explicitly set the period when probing the
> > > backlight device, because it will be set next time we call
> > > pwm_config(), and since we're passing pb->period when calling
> > > pwm_config() everything should be fine.
> >
> > Calling pwm_set_period() is still good for consistency. Consider for
> > example what happens if after the driver were to call pwm_get_period().
> > It would return some more or less random value (likely 0 or whatever it
> > had been set to by an earlier user).
>
> Yes, that's true in general, but in this specific driver
> pwm_get_period() is never called, and the driver only relies on the
> pb->period value.
Perhaps that's something that should change. If the PWM core has all
this infrastructure there should be no need for the backlight driver to
keep it's own copy of that variable.
> > Technically I think the most proper equivalent here would be to set the
> > default state's period to data->pwm_period_ns, but I don't think that's
> > proper to do. Perhaps since this is only relevant to boards where the
> > backlight device is created from board setup code we don't have to care
> > so much about messing up the initial state because either the board
> > setup code has been carefully written to match what the bootloader set
> > up, or because they don't match at all, in which case we don't have to
> > worry anyway.
>
> IMHO, if we had to support default period values for non DT boards, the
> proper way would be to pass something in the PWM platform data and let
> the PWM driver (or PWM core) initialize the default PWM state.
> This way the PWM user could rely on the pwm_get_default_period() helper
> to extract the default period value.
Yes, that's what PWM lookup tables are meant to address. I tried to
convert existing users a number of times, but never got any replies and
since it's board code I couldn't merge this through the PWM tree...
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 10/15] pwm: add the PWM initial state retrieval infra
From: Thierry Reding @ 2015-07-20 9:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-11-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]
On Wed, Jul 01, 2015 at 10:21:56AM +0200, Boris Brezillon wrote:
> Add a ->init_state() function to the pwm_ops struct to let PWM drivers
> initialize the PWM state attached to a PWM device.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/pwm/core.c | 3 +++
> include/linux/pwm.h | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3e830ce..30631f5 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -264,6 +264,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> pwm->hwpwm = i;
> pwm_set_default_polarity(pwm, polarity);
>
> + if (chip->ops->init_state)
> + chip->ops->init_state(chip, pwm);
> +
> radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
> }
>
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 0f36a06..b47244a 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -177,6 +177,8 @@ struct pwm_ops {
> struct pwm_device *pwm);
> void (*disable)(struct pwm_chip *chip,
> struct pwm_device *pwm);
> + void (*init_state)(struct pwm_chip *chip,
> + struct pwm_device *pwm);
I think I'd call this reset_state. init has this connotation of setting
a set of default values. For reset it's clearer in my opinion that it's
resetting to the hardware state.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 11/15] pwm: add the core infrastructure to allow atomic update
From: Thierry Reding @ 2015-07-20 8:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-12-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 6611 bytes --]
On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
> Add an ->apply() method to the pwm_ops struct to allow PWM drivers to
> implement atomic update.
> This method will be prefered over the ->enable(), ->disable() and
> ->config() methods if available.
>
> Add the pwm_get_state(), pwm_get_default_state() and pwm_apply_state()
> functions for PWM users to be able to use the atomic update feature.
>
> Note that the pwm_apply_state() does not guarantee the atomicity of the
> update operation, it all depends on the availability and implementation
> of the ->apply() method.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/pwm/core.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
> include/linux/pwm.h | 26 +++++++++++++
> 2 files changed, 124 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 30631f5..6dafd8e 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -238,8 +238,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> unsigned int i;
> int ret;
>
> - if (!chip || !chip->dev || !chip->ops || !chip->ops->config ||
> - !chip->ops->enable || !chip->ops->disable || !chip->npwm)
> + if (!chip || !chip->dev || !chip->ops || (!chip->ops->apply &&
> + (!chip->ops->config || !chip->ops->enable ||
> + !chip->ops->disable)) || !chip->npwm)
> return -EINVAL;
This is becoming really unreadable, perhaps split it into two checks, or
even split out the sanity check on the ops into a separate function to
make the negations easier to read:
static bool pwm_ops_check(const struct pwm_ops *ops)
{
/* driver supports legacy, non-atomic operation */
if (ops->config && ops->enable && ops->disable)
return true;
/* driver supports atomic operation */
if (ops->apply)
return true;
return false;
}
and then use this:
if (!chip || !chip->dev || !chip->ops || !chip->npwm)
return -EINVAL;
if (!pwm_ops_check(chip->ops))
return -EINVAL;
> mutex_lock(&pwm_lock);
> @@ -430,7 +431,17 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
> return -EINVAL;
>
> - err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> + if (pwm->chip->ops->apply) {
> + struct pwm_state state = pwm->state;
Shouldn't this use pwm_get_state()?
> +
> + state.period = period_ns;
> + state.duty_cycle = duty_ns;
> +
> + err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
> + } else {
> + err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> + }
> +
> if (err)
> return err;
>
> @@ -455,6 +466,17 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
> if (!pwm || !pwm->chip->ops)
> return -EINVAL;
>
> + if (pwm->chip->ops->apply) {
> + struct pwm_state state = pwm->state;
Same here.
> +
> + state.polarity = polarity;
> + err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
> + if (!err)
> + pwm->state.polarity = polarity;
> +
> + return err;
> + }
> +
> if (!pwm->chip->ops->set_polarity)
> return -ENOSYS;
>
> @@ -477,17 +499,27 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
> */
> int pwm_enable(struct pwm_device *pwm)
> {
> - if (pwm && !pwm_is_enabled(pwm)) {
> - int err;
> + int err;
>
> - err = pwm->chip->ops->enable(pwm->chip, pwm);
> - if (!err)
> - pwm->state.enabled = true;
> + if (!pwm)
> + return -EINVAL;
>
> - return err;
> + if (pwm_is_enabled(pwm))
> + return 0;
> +
> + if (pwm->chip->ops->apply) {
> + struct pwm_state state = pwm->state;
And here.
> +
> + state.enabled = true;
> + err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
There should be a space between the above two lines.
> + } else {
> + err = pwm->chip->ops->enable(pwm->chip, pwm);
> }
>
> - return pwm ? 0 : -EINVAL;
> + if (!err)
> + pwm->state.enabled = true;
> +
> + return err;
> }
> EXPORT_SYMBOL_GPL(pwm_enable);
>
> @@ -497,13 +529,67 @@ EXPORT_SYMBOL_GPL(pwm_enable);
> */
> void pwm_disable(struct pwm_device *pwm)
> {
> - if (pwm && pwm_is_enabled(pwm)) {
> + if (!pwm || !pwm_is_enabled(pwm))
> + return;
> +
> + if (pwm->chip->ops->apply) {
> + struct pwm_state state = pwm->state;
> +
> + state.enabled = false;
> + pwm->chip->ops->apply(pwm->chip, pwm, &state);
> + } else {
> pwm->chip->ops->disable(pwm->chip, pwm);
> - pwm->state.enabled = false;
> }
> +
> + pwm->state.enabled = false;
> }
> EXPORT_SYMBOL_GPL(pwm_disable);
Same comments as for pwm_enable().
>
> +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> +{
> + int err = 0;
> +
> + if (!pwm)
> + return -EINVAL;
> +
> + if (!memcmp(state, &pwm->state, sizeof(*state)))
> + return 0;
> +
> + if (pwm->chip->ops->apply) {
> + err = pwm->chip->ops->apply(pwm->chip, pwm, state);
> + if (!err)
> + pwm->state = *state;
Maybe we want pwm_set_state() for this?
> + } else {
> + /*
> + * FIXME: restore the initial state in case of error.
> + */
> + if (state->polarity != pwm->state.polarity) {
> + pwm_disable(pwm);
> + err = pwm_set_polarity(pwm, state->polarity);
> + if (err)
> + goto out;
> + }
> +
> + if (state->period != pwm->state.period ||
> + state->duty_cycle != pwm->state.duty_cycle) {
> + err = pwm_config(pwm, state->period, state->duty_cycle);
> + if (err)
> + goto out;
> + }
> +
> + if (state->enabled != pwm->state.enabled) {
> + if (state->enabled)
> + err = pwm_enable(pwm);
> + else
> + pwm_disable(pwm);
> + }
> + }
> +
> +out:
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(pwm_apply_state);
> +
> static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
> {
> struct pwm_chip *chip;
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index b47244a..7e99679 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
> return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
> }
>
> +/*
> + * pwm_apply_state - apply a new state to the PWM device
> + */
> +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
If you add kerneldoc, please add it properly. It should start with /**
and you need to list at least the parameters and return value.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Boris Brezillon @ 2015-07-20 8:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720083648.GK29614@ulmo>
On Mon, 20 Jul 2015 10:36:50 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > On Mon, 20 Jul 2015 10:16:00 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > > > The PWM period will be set when calling pwm_config. Remove this useless
> > > > call to pwm_set_period, which might mess up with the initial PWM state
> > > > once we have added proper support for PWM init state retrieval.
> > > >
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > ---
> > > > drivers/video/backlight/pwm_bl.c | 4 +---
> > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > index ae498c1..fe5597c 100644
> > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > * via the PWM lookup table.
> > > > */
> > > > pb->period = pwm_get_default_period(pb->pwm);
> > > > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > > > + if (!pb->period && (data->pwm_period_ns > 0))
> > > > pb->period = data->pwm_period_ns;
> > > > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > > > - }
> > > >
> > > > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
> > >
> > > As far as I remember this line is there in order to pass in a period if
> > > the backlight driver is initialized from board setup files. In such a
> > > case there won't be an period associated with the PWM channel in the
> > > first place.
> > >
> > > I think even with the introduction of a default period, we'd be missing
> > > out on the board setup case because there is no standard place where it
> > > is being set, so it must come from the platform data.
> >
> > AFAICT, we don't need to explicitly set the period when probing the
> > backlight device, because it will be set next time we call
> > pwm_config(), and since we're passing pb->period when calling
> > pwm_config() everything should be fine.
>
> Calling pwm_set_period() is still good for consistency. Consider for
> example what happens if after the driver were to call pwm_get_period().
> It would return some more or less random value (likely 0 or whatever it
> had been set to by an earlier user).
Yes, that's true in general, but in this specific driver
pwm_get_period() is never called, and the driver only relies on the
pb->period value.
>
> Technically I think the most proper equivalent here would be to set the
> default state's period to data->pwm_period_ns, but I don't think that's
> proper to do. Perhaps since this is only relevant to boards where the
> backlight device is created from board setup code we don't have to care
> so much about messing up the initial state because either the board
> setup code has been carefully written to match what the bootloader set
> up, or because they don't match at all, in which case we don't have to
> worry anyway.
IMHO, if we had to support default period values for non DT boards, the
proper way would be to pass something in the PWM platform data and let
the PWM driver (or PWM core) initialize the default PWM state.
This way the PWM user could rely on the pwm_get_default_period() helper
to extract the default period value.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Thierry Reding @ 2015-07-20 8:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720102143.05949ca2@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]
On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:16:00 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > > The PWM period will be set when calling pwm_config. Remove this useless
> > > call to pwm_set_period, which might mess up with the initial PWM state
> > > once we have added proper support for PWM init state retrieval.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > ---
> > > drivers/video/backlight/pwm_bl.c | 4 +---
> > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > index ae498c1..fe5597c 100644
> > > --- a/drivers/video/backlight/pwm_bl.c
> > > +++ b/drivers/video/backlight/pwm_bl.c
> > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > * via the PWM lookup table.
> > > */
> > > pb->period = pwm_get_default_period(pb->pwm);
> > > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > > + if (!pb->period && (data->pwm_period_ns > 0))
> > > pb->period = data->pwm_period_ns;
> > > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > > - }
> > >
> > > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
> >
> > As far as I remember this line is there in order to pass in a period if
> > the backlight driver is initialized from board setup files. In such a
> > case there won't be an period associated with the PWM channel in the
> > first place.
> >
> > I think even with the introduction of a default period, we'd be missing
> > out on the board setup case because there is no standard place where it
> > is being set, so it must come from the platform data.
>
> AFAICT, we don't need to explicitly set the period when probing the
> backlight device, because it will be set next time we call
> pwm_config(), and since we're passing pb->period when calling
> pwm_config() everything should be fine.
Calling pwm_set_period() is still good for consistency. Consider for
example what happens if after the driver were to call pwm_get_period().
It would return some more or less random value (likely 0 or whatever it
had been set to by an earlier user).
Technically I think the most proper equivalent here would be to set the
default state's period to data->pwm_period_ns, but I don't think that's
proper to do. Perhaps since this is only relevant to boards where the
backlight device is created from board setup code we don't have to care
so much about messing up the initial state because either the board
setup code has been carefully written to match what the bootloader set
up, or because they don't match at all, in which case we don't have to
worry anyway.
Of course the right thing to do would be to replace all initialization
of the data->pwm_period_ns by proper PWM lookup tables, but that's
proven difficult in the past since very few people still have access to
hardware where that code gets executed.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts
From: Boris Brezillon @ 2015-07-20 8:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720082241.GJ29614@ulmo>
On Mon, 20 Jul 2015 10:22:42 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Jul 20, 2015 at 10:14:43AM +0200, Boris Brezillon wrote:
> > Hi Thierry,
> >
> > On Mon, 20 Jul 2015 10:03:14 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> > > > 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 ?
> > >
> > > I'm not sure I see the need for this. If they are the default values and
> > > drivers have no need to change them, then storing them in the regular
> > > period and polarity fields seems just fine (they'll be propagated into
> > > new state objects as they get created).
> > >
> > > And if the driver has a need to change them, then why would it ever care
> > > about the default values?
> >
> > Because the period is often directly extracted from the DT, and this
> > extracted period may not match the one configured by the bootloader.
> >
> > If the driver wants to display the current status without changing the
> > PWM state, then the driver will use the current state. ITOH, if it
> > has to apply a new config, the driver will use the default period
> > value (extracted from the DT) and change the duty-cycle depending on its
> > needs.
> > This is the case we have with the pwm-regulator driver: we want to
> > display the initial voltage value without changing the PWM config, and
> > when someone decides to change the voltage, we want to use the default
> > period instead of keeping the one configured by the bootloader.
>
> Wouldn't it make more sense to postpone this until the introduction of
> the default state, then? That way we'd be getting a more consistent way
> of dealing with default vs. initial by looking only at state objects.
Hm, I was trying to keep the series bisectable. If we do that
after introducing the default state concept, then some drivers will
retrieve invalid values until the patches introducing the default
helpers and changing the different drivers to call the default helpers
where appropriate are introduced.
>
> Ideally initial state should be the same as the default state. Except
> maybe for the duty-cycle, which won't be encoded in the default state
> anyway.
Yes, but we don't live in an ideal world ;-), and the value set in an
old bootloaders might be considered wrong at some point, and new dts
versions might decide to change a bit the period value.
--
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: Thierry Reding @ 2015-07-20 8:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720101443.180ebddb@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]
On Mon, Jul 20, 2015 at 10:14:43AM +0200, Boris Brezillon wrote:
> Hi Thierry,
>
> On Mon, 20 Jul 2015 10:03:14 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> > > 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 ?
> >
> > I'm not sure I see the need for this. If they are the default values and
> > drivers have no need to change them, then storing them in the regular
> > period and polarity fields seems just fine (they'll be propagated into
> > new state objects as they get created).
> >
> > And if the driver has a need to change them, then why would it ever care
> > about the default values?
>
> Because the period is often directly extracted from the DT, and this
> extracted period may not match the one configured by the bootloader.
>
> If the driver wants to display the current status without changing the
> PWM state, then the driver will use the current state. ITOH, if it
> has to apply a new config, the driver will use the default period
> value (extracted from the DT) and change the duty-cycle depending on its
> needs.
> This is the case we have with the pwm-regulator driver: we want to
> display the initial voltage value without changing the PWM config, and
> when someone decides to change the voltage, we want to use the default
> period instead of keeping the one configured by the bootloader.
Wouldn't it make more sense to postpone this until the introduction of
the default state, then? That way we'd be getting a more consistent way
of dealing with default vs. initial by looking only at state objects.
Ideally initial state should be the same as the default state. Except
maybe for the duty-cycle, which won't be encoded in the default state
anyway.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Boris Brezillon @ 2015-07-20 8:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720081559.GI29614@ulmo>
On Mon, 20 Jul 2015 10:16:00 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > The PWM period will be set when calling pwm_config. Remove this useless
> > call to pwm_set_period, which might mess up with the initial PWM state
> > once we have added proper support for PWM init state retrieval.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > drivers/video/backlight/pwm_bl.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index ae498c1..fe5597c 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > * via the PWM lookup table.
> > */
> > pb->period = pwm_get_default_period(pb->pwm);
> > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > + if (!pb->period && (data->pwm_period_ns > 0))
> > pb->period = data->pwm_period_ns;
> > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > - }
> >
> > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
>
> As far as I remember this line is there in order to pass in a period if
> the backlight driver is initialized from board setup files. In such a
> case there won't be an period associated with the PWM channel in the
> first place.
>
> I think even with the introduction of a default period, we'd be missing
> out on the board setup case because there is no standard place where it
> is being set, so it must come from the platform data.
AFAICT, we don't need to explicitly set the period when probing the
backlight device, because it will be set next time we call
pwm_config(), and since we're passing pb->period when calling
pwm_config() everything should be fine.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Thierry Reding @ 2015-07-20 8:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-9-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]
On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> The PWM period will be set when calling pwm_config. Remove this useless
> call to pwm_set_period, which might mess up with the initial PWM state
> once we have added proper support for PWM init state retrieval.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/video/backlight/pwm_bl.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index ae498c1..fe5597c 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> * via the PWM lookup table.
> */
> pb->period = pwm_get_default_period(pb->pwm);
> - if (!pb->period && (data->pwm_period_ns > 0)) {
> + if (!pb->period && (data->pwm_period_ns > 0))
> pb->period = data->pwm_period_ns;
> - pwm_set_period(pb->pwm, data->pwm_period_ns);
> - }
>
> pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
As far as I remember this line is there in order to pass in a period if
the backlight driver is initialized from board setup files. In such a
case there won't be an period associated with the PWM channel in the
first place.
I think even with the introduction of a default period, we'd be missing
out on the board setup case because there is no standard place where it
is being set, so it must come from the platform data.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts
From: Boris Brezillon @ 2015-07-20 8:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720080313.GF29614@ulmo>
Hi Thierry,
On Mon, 20 Jul 2015 10:03:14 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> > 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 ?
>
> I'm not sure I see the need for this. If they are the default values and
> drivers have no need to change them, then storing them in the regular
> period and polarity fields seems just fine (they'll be propagated into
> new state objects as they get created).
>
> And if the driver has a need to change them, then why would it ever care
> about the default values?
Because the period is often directly extracted from the DT, and this
extracted period may not match the one configured by the bootloader.
If the driver wants to display the current status without changing the
PWM state, then the driver will use the current state. ITOH, if it
has to apply a new config, the driver will use the default period
value (extracted from the DT) and change the duty-cycle depending on its
needs.
This is the case we have with the pwm-regulator driver: we want to
display the initial voltage value without changing the PWM config, and
when someone decides to change the voltage, we want to use the default
period instead of keeping the one configured by the bootloader.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 07/15] pwm: move the enabled/disabled info to pwm_state struct
From: Thierry Reding @ 2015-07-20 8:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-8-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1521 bytes --]
On Wed, Jul 01, 2015 at 10:21:53AM +0200, Boris Brezillon wrote:
> Prepare the transition to PWM atomic update by moving the enabled/disabled
> state into the pwm_state struct. This way we can easily update the whole
> PWM state by copying the new state in the ->state field.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/pwm/core.c | 15 ++++++++++++---
> include/linux/pwm.h | 6 +++---
> 2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index a6bc8e6..3e830ce 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -474,8 +474,15 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
> */
> int pwm_enable(struct pwm_device *pwm)
> {
> - if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
> - return pwm->chip->ops->enable(pwm->chip, pwm);
> + if (pwm && !pwm_is_enabled(pwm)) {
> + int err;
> +
> + err = pwm->chip->ops->enable(pwm->chip, pwm);
> + if (!err)
> + pwm->state.enabled = true;
> +
> + return err;
> + }
Technically there's now a race between the pwm_is_enabled() and
pwm->state.enabled = true; statements, but as discussed in the cover
letter I think that's fine because of the assumptions about concurrent
usage of PWMs.
The most important check (PWMF_REQUESTED) is still atomic, so it is
still up to drivers to properly lock concurrent access to a PWM device
and the core will make sure that a device can only be requested once.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct
From: Thierry Reding @ 2015-07-20 8:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-7-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 339 bytes --]
On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
[...]
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
[...]
> +struct pwm_state {
> + unsigned int period; /* in nanoseconds */
> + unsigned int duty_cycle; /* in nanoseconds */
> + enum pwm_polarity polarity;
> +};
No need for the extra padding here.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts
From: Thierry Reding @ 2015-07-20 8:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150702094955.4b1c9254@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]
On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> 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 ?
I'm not sure I see the need for this. If they are the default values and
drivers have no need to change them, then storing them in the regular
period and polarity fields seems just fine (they'll be propagated into
new state objects as they get created).
And if the driver has a need to change them, then why would it ever care
about the default values?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ 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