dt-bindings: pwm: hibvt: Add hi3559v100 support
Subsystems (5)
PATCH 1/2 dt-bindings: pwm: hibvt: Add hi3559v100 support
Add support for hi3559v100-shub-pwm and hisilicon,hi3559v100-pwm platforms.
--- Documentation/devicetree/bindings/pwm/pwm-hibvt.txt | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/pwm/pwm-hibvt.txt +2 -0
@@ -5,6 +5,8 @@ Required properties: The SoC specific strings supported including: "hisilicon,hi3516cv300-pwm" "hisilicon,hi3519v100-pwm" + "hisilicon,hi3559v100-shub-pwm" + "hisilicon,hi3559v100-pwm - reg: physical base address and length of the controller's registers. - clocks: phandle and clock specifier of the PWM reference clock. - resets: phandle and reset specifier for the PWM controller reset.
Signature · 1 line
2.17.1
PATCH 2/2 pwm: hibvt: Add hi3559v100 support
Add support for hi3559v100-shub-pwm and hisilicon,hi3559v100-pwm platforms. They require a special quirk: pwm has to be enabled again to force duty_cycle refresh.
--- drivers/pwm/pwm-hibvt.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)
drivers/pwm/pwm-hibvt.c +23 -3
@@ -49,6 +49,7 @@ struct hibvt_pwm_chip {struct clk *clk;void __iomem *base;struct reset_control *rstc; +bool quirk_force_enable; };struct hibvt_pwm_soc {
@@ -56,6 +57,7 @@ struct hibvt_pwm_soc { };static const struct hibvt_pwm_soc pwm_soc[2 ] = { + { .num_pwms =2 }, { .num_pwms =4 }, { .num_pwms =8 }, };
@@ -148,13 +150,19 @@ static void hibvt_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,static int hibvt_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm,struct pwm_state *state) { +struct hibvt_pwm_chip *hi_pwm_chip =to_hibvt_pwm_chip (chip); +if (state->polarity != pwm->state.polarity)hibvt_pwm_set_polarity (chip, pwm, state->polarity);if (state->period != pwm->state.period || - state->duty_cycle != pwm->state.duty_cycle) + state->duty_cycle != pwm->state.duty_cycle) {hibvt_pwm_config (chip, pwm, state->duty_cycle, state->period); +if (hi_pwm_chip->quirk_force_enable && state->enabled) +hibvt_pwm_enable (chip, pwm); + } +if (state->enabled != pwm->state.enabled) {if (state->enabled)hibvt_pwm_enable (chip, pwm);
@@ -176,6 +184,7 @@ static int hibvt_pwm_probe(struct platform_device *pdev) {const struct hibvt_pwm_soc *soc =of_device_get_match_data (&pdev->dev); +struct device_node *np = pdev->dev.of_node;struct hibvt_pwm_chip *pwm_chip;struct resource *res;int ret;
@@ -199,6 +208,15 @@ static int hibvt_pwm_probe(struct platform_device *pdev) pwm_chip->chip.of_xlate = of_pwm_xlate_with_flags; pwm_chip->chip.of_pwm_n_cells =3 ; +/* +* On those platforms, it is required to enable the +* pwm again each time we want to refresh the duty +* cycle. +*/ +if (of_device_is_compatible (np,"hisilicon,hi3559v100-shub-pwm" ) || +of_device_is_compatible (np,"hisilicon,hi3559v100-pwm" )) + pwm_chip->quirk_force_enable = true; + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pwm_chip->base =devm_ioremap_resource (&pdev->dev, res);if (IS_ERR (pwm_chip->base))
@@ -250,8 +268,10 @@ static int hibvt_pwm_remove(struct platform_device *pdev) }static const struct of_device_id hibvt_pwm_of_match[] = { - { .compatible ="hisilicon,hi3516cv300-pwm" , .data = &pwm_soc[0 ] }, - { .compatible ="hisilicon,hi3519v100-pwm" , .data = &pwm_soc[1 ] }, + { .compatible ="hisilicon,hi3516cv300-pwm" , .data = &pwm_soc[1 ] }, + { .compatible ="hisilicon,hi3519v100-pwm" , .data = &pwm_soc[2 ] }, + { .compatible ="hisilicon,hi3559v100-shub-pwm" , .data = &pwm_soc[2 ] }, + { .compatible ="hisilicon,hi3559v100-pwm" , .data = &pwm_soc[0 ] }, { } };MODULE_DEVICE_TABLE (of, hibvt_pwm_of_match);
Signature · 1 line
2.17.1
Re: [PATCH 2/2] pwm: hibvt: Add hi3559v100 support
Quoted 22 lines · Mathieu Othacehe
Add support for hi3559v100-shub-pwm and hisilicon,hi3559v100-pwm platforms. They require a special quirk: pwm has to be enabled again to force
duty_cyclerefresh.Signed-off-by Mathieu Othacehe <[email protected]>--- drivers/pwm/pwm-hibvt.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)drivers/pwm/pwm-hibvt.c +1
-0@@ -49,6 +49,7 @@ struct hibvt_pwm_chip {struct clk *clk;void __iomem *base;struct reset_control *rstc; +bool quirk_force_enable; };struct hibvt_pwm_soc {
drivers/pwm/pwm-hibvt.c struct hibvt_pwm_soc { · line 62
Quoted 6 lines
drivers/pwm/pwm-hibvt.c +1
-0@@ -56,6 +57,7 @@ struct hibvt_pwm_soc { };static const struct hibvt_pwm_soc pwm_soc[2 ] = { + { .num_pwms =2 }, { .num_pwms =4 }, { .num_pwms =8 },
The members of this struct are used as of-data (in struct of_device_id::data below). When looking at the usage:
{ .compatible = "hisilicon,hi3516cv300-pwm", .data = &pwm_soc[1] },
{ .compatible = "hisilicon,hi3519v100-pwm", .data = &pwm_soc[2] },
{ .compatible = "hisilicon,hi3559v100-shub-pwm", .data = &pwm_soc[2] },
{ .compatible = "hisilicon,hi3559v100-pwm", .data = &pwm_soc[0] },this isn't exactly easy to understand. I would prefer to do it as follows:
static const struct hibvt_pwm_soc hi3516cv300_soc_info = {
.num_pwms = 2,
};
...
static const struct of_device_id hibvt_pwm_of_match[] = {
{ .compatible = "hisilicon,hi3516cv300-pwm", .data = &hi3516cv300_soc_info },
...
};Then you could also add a member to hibvt_pwm_soc to signal if that force_enable quirk is necessary and would not need to use of_device_is_compatible() to determine this. The result is that you have a description of all relevant differences in a single place.
@Thierry: Also this is yet another driver instance where a num-pwms property would simplify the driver because up to before this patch this was the only difference between the different variants.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH 2/2] pwm: hibvt: Add hi3559v100 support
Quoted 51 lines · Uwe Kleine-König
Quoted 22 lines · Mathieu Othacehe
Add support for hi3559v100-shub-pwm and hisilicon,hi3559v100-pwm platforms. They require a special quirk: pwm has to be enabled again to force
duty_cyclerefresh.Signed-off-by Mathieu Othacehe <[email protected]>--- drivers/pwm/pwm-hibvt.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)drivers/pwm/pwm-hibvt.c +1
-0@@ -49,6 +49,7 @@ struct hibvt_pwm_chip {struct clk *clk;void __iomem *base;struct reset_control *rstc; +bool quirk_force_enable; };struct hibvt_pwm_soc {drivers/pwm/pwm-hibvt.c struct hibvt_pwm_soc { · line 62
Quoted 6 lines
drivers/pwm/pwm-hibvt.c +1
-0@@ -56,6 +57,7 @@ struct hibvt_pwm_soc { };static const struct hibvt_pwm_soc pwm_soc[2 ] = { + { .num_pwms =2 }, { .num_pwms =4 }, { .num_pwms =8 },The members of this struct are used as of-data (in struct
of_device_id::databelow). When looking at the usage:{ .compatible = "hisilicon,hi3516cv300-pwm", .data = &pwm_soc[1] }, { .compatible = "hisilicon,hi3519v100-pwm", .data = &pwm_soc[2] }, { .compatible = "hisilicon,hi3559v100-shub-pwm", .data = &pwm_soc[2] }, { .compatible = "hisilicon,hi3559v100-pwm", .data = &pwm_soc[0] },this isn't exactly easy to understand. I would prefer to do it as follows:
static const struct hibvt_pwm_soc hi3516cv300_soc_info = { .num_pwms = 2, }; ... static const struct of_device_id hibvt_pwm_of_match[] = { { .compatible = "hisilicon,hi3516cv300-pwm", .data = &hi3516cv300_soc_info }, ... };
Quoted 8 lines
Then you could also add a member to
hibvt_pwm_socto signal if thatforce_enablequirk is necessary and would not need to useof_device_is_compatible()to determine this. The result is that you have a description of all relevant differences in a single place.@Thierry: Also this is yet another driver instance where a num-pwms property would simplify the driver because up to before this patch this was the only difference between the different variants.
We don't need the num-pwms in device tree if it can be derived from the compatible string.
Thierry
Re: [PATCH 2/2] pwm: hibvt: Add hi3559v100 support
Quoted 57 lines · Thierry Reding
Quoted 56 lines · Uwe Kleine-König
Quoted 22 lines · Mathieu Othacehe
Add support for hi3559v100-shub-pwm and hisilicon,hi3559v100-pwm platforms. They require a special quirk: pwm has to be enabled again to force
duty_cyclerefresh.Signed-off-by Mathieu Othacehe <[email protected]>--- drivers/pwm/pwm-hibvt.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)drivers/pwm/pwm-hibvt.c +1
-0@@ -49,6 +49,7 @@ struct hibvt_pwm_chip {struct clk *clk;void __iomem *base;struct reset_control *rstc; +bool quirk_force_enable; };struct hibvt_pwm_soc {drivers/pwm/pwm-hibvt.c struct hibvt_pwm_soc { · line 62
Quoted 6 lines
drivers/pwm/pwm-hibvt.c +1
-0@@ -56,6 +57,7 @@ struct hibvt_pwm_soc { };static const struct hibvt_pwm_soc pwm_soc[2 ] = { + { .num_pwms =2 }, { .num_pwms =4 }, { .num_pwms =8 },The members of this struct are used as of-data (in struct
of_device_id::databelow). When looking at the usage:{ .compatible = "hisilicon,hi3516cv300-pwm", .data = &pwm_soc[1] }, { .compatible = "hisilicon,hi3519v100-pwm", .data = &pwm_soc[2] }, { .compatible = "hisilicon,hi3559v100-shub-pwm", .data = &pwm_soc[2] }, { .compatible = "hisilicon,hi3559v100-pwm", .data = &pwm_soc[0] },this isn't exactly easy to understand. I would prefer to do it as follows:
static const struct hibvt_pwm_soc hi3516cv300_soc_info = { .num_pwms = 2, }; ... static const struct of_device_id hibvt_pwm_of_match[] = { { .compatible = "hisilicon,hi3516cv300-pwm", .data = &hi3516cv300_soc_info }, ... };Then you could also add a member to
hibvt_pwm_socto signal if thatforce_enablequirk is necessary and would not need to useof_device_is_compatible()to determine this. The result is that you have a description of all relevant differences in a single place.
Quoted 6 lines
Quoted 3 lines
@Thierry: Also this is yet another driver instance where a num-pwms property would simplify the driver because up to before this patch this was the only difference between the different variants.
We don't need the num-pwms in device tree if it can be derived from the compatible string.
Right, this not about a must. It can be done without this property. This is just about putting the number of pwms in another place to describe the pwm differently. Then you'd put
num-pwms = <2>;
in the device tree and then don't need to bother if which hisilicon SoC this is in the driver. Sure, you can implicitly know this using the compatible. This is just another shade of gray. If put to an extreme you can also claim your machine is completely defined by just
/ {
compatible = "technexion,imx6ul-pico-hobbit";
}and determine all other relevant stuff from the compatible.
A similar case is the i2c rtc ds1307. This is described in the device tree as:
rtc@68 {
compatible = "dallas,ds1307";
reg = <0x68>;
}even though the ds1307 can only ever be used at address 0x68. So you could argue there is no good reason to specify the reg property because that could be determined from the compatible.
Still for the sake of a generic way to describe i2c devices it was agreed on that it should be there and generic i2c code can benefit from it.
If we decided to put num-pwms consistenly into the device trees (and we would have done so earlier and not have to care to handle old device trees) the following patch would be possible:
drivers/pwm/pwm-hibvt.c +3 -14
@@ -51,15 +51,6 @@ struct hibvt_pwm_chip {struct reset_control *rstc; }; -struct hibvt_pwm_soc { -u32 num_pwms; -}; - -static const struct hibvt_pwm_soc pwm_soc[2 ] = { - { .num_pwms =4 }, - { .num_pwms =8 }, -}; -static inline struct hibvt_pwm_chip *to_hibvt_pwm_chip (struct pwm_chip *chip) { return container_of(chip, struct hibvt_pwm_chip, chip);
@@ -174,8 +165,6 @@ static const struct pwm_ops hibvt_pwm_ops = {static int hibvt_pwm_probe (struct platform_device *pdev) { -const struct hibvt_pwm_soc *soc = -of_device_get_match_data (&pdev->dev);struct hibvt_pwm_chip *pwm_chip;struct resource *res;int ret;
@@ -195,7 +184,7 @@ static int hibvt_pwm_probe(struct platform_device *pdev) pwm_chip->chip.ops = &hibvt_pwm_ops; pwm_chip->chip.dev = &pdev->dev; pwm_chip->chip.base =-1 ; - pwm_chip->chip.npwm = soc->num_pwms; + pwm_chip->chip.npwm =pwm_get_npwm_from_dt (); pwm_chip->chip.of_xlate = of_pwm_xlate_with_flags; pwm_chip->chip.of_pwm_n_cells =3 ;
@@ -250,8 +239,8 @@ static int hibvt_pwm_remove(struct platform_device *pdev) }static const struct of_device_id hibvt_pwm_of_match[] = { - { .compatible ="hisilicon,hi3516cv300-pwm" , .data = &pwm_soc[0 ] }, - { .compatible ="hisilicon,hi3519v100-pwm" , .data = &pwm_soc[1 ] }, + { .compatible ="hisilicon,hi3516cv300-pwm" , }, + { .compatible ="hisilicon,hi3519v100-pwm" , }, { } };MODULE_DEVICE_TABLE (of, hibvt_pwm_of_match);
drivers/pwm/pwm-mediatek.c +1 -6
@@ -55,7 +55,6 @@ static const char * const mtk_pwm_clk_name[MTK_CLK_MAX] = { };struct mtk_pwm_platform_data { -unsigned int num_pwms;bool pwm45_fixup;bool has_clks; };
@@ -260,7 +259,7 @@ static int mtk_pwm_probe(struct platform_device *pdev) pc->chip.dev = &pdev->dev; pc->chip.ops = &mtk_pwm_ops; pc->chip.base =-1 ; - pc->chip.npwm = data->num_pwms; + pc->chip.npwm =pwm_get_npwm_from_dt (); ret =pwmchip_add (&pc->chip);if (ret <0 ) {
@@ -279,25 +278,21 @@ static int mtk_pwm_remove(struct platform_device *pdev) }static const struct mtk_pwm_platform_data mt2712_pwm_data = { - .num_pwms =8 , .pwm45_fixup = false, .has_clks = true, };static const struct mtk_pwm_platform_data mt7622_pwm_data = { - .num_pwms =6 , .pwm45_fixup = false, .has_clks = true, };static const struct mtk_pwm_platform_data mt7623_pwm_data = { - .num_pwms =5 , .pwm45_fixup = true, .has_clks = true, };static const struct mtk_pwm_platform_data mt7628_pwm_data = { - .num_pwms =4 , .pwm45_fixup = true, .has_clks = false, };
drivers/pwm/pwm-sun4i.c +1 -5
@@ -73,7 +73,6 @@ static const u32 prescaler_table[] = {struct sun4i_pwm_data {bool has_prescaler_bypass; -unsigned int npwm; };struct sun4i_pwm_chip {
@@ -314,17 +313,14 @@ static const struct pwm_ops sun4i_pwm_ops = {static const struct sun4i_pwm_data sun4i_pwm_dual_nobypass = { .has_prescaler_bypass = false, - .npwm =2 , };static const struct sun4i_pwm_data sun4i_pwm_dual_bypass = { .has_prescaler_bypass = true, - .npwm =2 , };static const struct sun4i_pwm_data sun4i_pwm_single_bypass = { .has_prescaler_bypass = true, - .npwm =1 , };static const struct of_device_id sun4i_pwm_dt_ids[] = {
@@ -375,7 +371,7 @@ static int sun4i_pwm_probe(struct platform_device *pdev) pwm->chip.dev = &pdev->dev; pwm->chip.ops = &sun4i_pwm_ops; pwm->chip.base =-1 ; - pwm->chip.npwm = pwm->data->npwm; + pwm->chip.npwm =pwm_get_npwm_from_dt (); pwm->chip.of_xlate = of_pwm_xlate_with_flags; pwm->chip.of_pwm_n_cells =3 ;
drivers/pwm/pwm-tegra.c +1 -5
@@ -40,8 +40,6 @@#define PWM_SCALE_SHIFT 0struct tegra_pwm_soc { -unsigned int num_channels; -/* Maximum IP frequency for given SoCs */ unsigned long max_frequency; };
@@ -230,7 +228,7 @@ static int tegra_pwm_probe(struct platform_device *pdev) pwm->chip.dev = &pdev->dev; pwm->chip.ops = &tegra_pwm_ops; pwm->chip.base =-1 ; - pwm->chip.npwm = pwm->soc->num_channels; + pwm->chip.npwm =pwm_get_npwm_from_dt (); ret =pwmchip_add (&pwm->chip);if (ret <0 ) {
@@ -286,12 +284,10 @@ static int tegra_pwm_resume(struct device *dev)#endif static const struct tegra_pwm_soc tegra20_pwm_soc = { - .num_channels =4 , .max_frequency =48000000UL , };static const struct tegra_pwm_soc tegra186_pwm_soc = { - .num_channels =1 , .max_frequency =102000000UL , };
together with a generic implementation of the function to get the number of pwms from the device tree. This looks like a good change and that's why I'm convinced it is sensible to put this into the device trees.
And if you ask me having num-pwms in the device tree like:
bls: pwm@1400a000 {
compatible = "mediatek,mt2701-disp-pwm";
reg = <..>;
#pwm-cells = <2>;
clocks = <&mmsys CLK_MM_MDP_BLS_26M>, <&mmsys CLK_MM_DISP_BLS>;
clock-names = "main", "mm";
num-pwms = <8>;
}is also a nice addition because then the author of a machine device tree (or a reviewer, or even a generic dtb linter) would easier spot how many pwms there are and that
pwms = <&bls 6 200000>
can be used without looking into the reference manual or the driver.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |