linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Matthias Brugger <matthias.bgg@gmail.com>
To: Weiqing Kong <weiqing.kong@mediatek.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Mark Rutland <mark.rutland@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Kumar Gala <galak@codeaurora.org>,
	Russell King <linux@arm.linux.org.uk>,
	linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
	srv_heupstream@mediatek.com, Sascha Hauer <kernel@pengutronix.de>,
	erin.lo@mediatek.com, yingjoe.chen@mediatek.com,
	eddie.huang@mediatek.com, yh.huang@mediatek.com,
	yt.shen@mediatek.com
Subject: Re: [PATCH 2/3] pwm: Add MediaTek MT2701 display PWM driver support
Date: Fri, 3 Jun 2016 17:35:57 +0200	[thread overview]
Message-ID: <5751A3DD.7020402@gmail.com> (raw)
In-Reply-To: <1464597712-5399-3-git-send-email-weiqing.kong@mediatek.com>



On 30/05/16 10:41, Weiqing Kong wrote:
> Use the mtk_pwm_data struction to define different registers
> and add MT2701 specific register operations, such as MT2701
> doesn't have commit register, needs to disable double buffer
> before writing register, and needs to select manual mode
> and use PWM_PERIOD/PWM_HIGH_WIDTH.
>
> Signed-off-by: Weiqing Kong <weiqing.kong@mediatek.com>
> ---
>   drivers/pwm/pwm-mtk-disp.c | 89 +++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 72 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> index 0ad3385..03b9c9e 100644
> --- a/drivers/pwm/pwm-mtk-disp.c
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -18,33 +18,44 @@
>   #include <linux/io.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
> +#include <linux/of_device.h>
>   #include <linux/platform_device.h>
>   #include <linux/pwm.h>
>   #include <linux/slab.h>
>
>   #define DISP_PWM_EN		0x00
> -#define PWM_ENABLE_MASK		BIT(0)
>
>   #define DISP_PWM_COMMIT		0x08
>   #define PWM_COMMIT_MASK		BIT(0)
>
> -#define DISP_PWM_CON_0		0x10
>   #define PWM_CLKDIV_SHIFT	16
>   #define PWM_CLKDIV_MAX		0x3ff
>   #define PWM_CLKDIV_MASK		(PWM_CLKDIV_MAX << PWM_CLKDIV_SHIFT)
>
> -#define DISP_PWM_CON_1		0x14
>   #define PWM_PERIOD_BIT_WIDTH	12
>   #define PWM_PERIOD_MASK		((1 << PWM_PERIOD_BIT_WIDTH) - 1)
>
>   #define PWM_HIGH_WIDTH_SHIFT	16
>   #define PWM_HIGH_WIDTH_MASK	(0x1fff << PWM_HIGH_WIDTH_SHIFT)
>
> +#define MT2701_PWM_MANUAL_SEL_MASK	BIT(1)
> +#define MT2701_PWM_BLS_DEBUG		0xb0

Do we need to set magic 0x3 in the debug register? If so, this should be 
part of mtk_pwm_data, just as con0_sel.

> +#define MT2701_PWM_BLS_DEBUG_MASK	0x3
> +
> +struct mtk_pwm_data {
> +	unsigned int enable_bit;
> +	unsigned int con0;
> +	unsigned int con0_sel;
> +	unsigned int con1;
> +	bool have_commit_reg;
> +};
> +
>   struct mtk_disp_pwm {
>   	struct pwm_chip chip;
>   	struct clk *clk_main;
>   	struct clk *clk_mm;
>   	void __iomem *base;
> +	const struct mtk_pwm_data *data;

Couldn't that just be the offset for the commit register and set it to 
0x0 if the commit register is not present. Right now, we suppose the 
DISP_PWM_EN is at offset 0x0, so this should not be a problem.

>   };
>
>   static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)
> @@ -106,12 +117,18 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   		return err;
>   	}
>
> -	mtk_disp_pwm_update_bits(mdp, DISP_PWM_CON_0, PWM_CLKDIV_MASK,
> +	mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
> +				 PWM_CLKDIV_MASK,
>   				 clk_div << PWM_CLKDIV_SHIFT);
> -	mtk_disp_pwm_update_bits(mdp, DISP_PWM_CON_1,
> -				 PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK, value);
> -	mtk_disp_pwm_update_bits(mdp, DISP_PWM_COMMIT, PWM_COMMIT_MASK, 1);
> -	mtk_disp_pwm_update_bits(mdp, DISP_PWM_COMMIT, PWM_COMMIT_MASK, 0);
> +	mtk_disp_pwm_update_bits(mdp, mdp->data->con1,
> +				 PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK,
> +				 value);
> +	if (mdp->data->have_commit_reg) {
> +		mtk_disp_pwm_update_bits(mdp, DISP_PWM_COMMIT,
> +					 PWM_COMMIT_MASK, 0x1);
> +		mtk_disp_pwm_update_bits(mdp, DISP_PWM_COMMIT,
> +					 PWM_COMMIT_MASK, 0x0);

should be renamed to MT8173_PWM_COMMIT_MASK, although I'm not really a 
friend of mixing up SoC specific defines with a Soc specific 
mtk_disp_pwm structure.

Regards,
Matthias

> +	}
>
>   	clk_disable(mdp->clk_mm);
>   	clk_disable(mdp->clk_main);
> @@ -134,7 +151,9 @@ static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>   		return err;
>   	}
>
> -	mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, PWM_ENABLE_MASK, 1);
> +	mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN,
> +				 mdp->data->enable_bit,
> +				 mdp->data->enable_bit);
>
>   	return 0;
>   }
> @@ -143,7 +162,8 @@ static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>   {
>   	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
>
> -	mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, PWM_ENABLE_MASK, 0);
> +	mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN,
> +				 mdp->data->enable_bit, 0x0);
>
>   	clk_disable(mdp->clk_mm);
>   	clk_disable(mdp->clk_main);
> @@ -156,12 +176,41 @@ static const struct pwm_ops mtk_disp_pwm_ops = {
>   	.owner = THIS_MODULE,
>   };
>
> +static const struct mtk_pwm_data mt8173_pwm_data = {
> +	.enable_bit = BIT(0),
> +	.con0 = 0x10,
> +	.con0_sel = 0x0,
> +	.con1 = 0x14,
> +	.have_commit_reg = true,
> +};
> +
> +static const struct mtk_pwm_data mt2701_pwm_data = {
> +	.enable_bit = BIT(16),
> +	.con0 = 0xa8,
> +	.con0_sel = 0x2,
> +	.con1 = 0xac,
> +	.have_commit_reg = false,
> +};
> +
> +static const struct of_device_id mtk_disp_pwm_of_match[] = {
> +	{ .compatible = "mediatek,mt2701-disp-bls", .data = &mt2701_pwm_data},
> +	{ .compatible = "mediatek,mt6595-disp-pwm", .data = &mt8173_pwm_data},
> +	{ .compatible = "mediatek,mt8173-disp-pwm", .data = &mt8173_pwm_data},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, mtk_disp_pwm_of_match);
> +
>   static int mtk_disp_pwm_probe(struct platform_device *pdev)
>   {
> +	const struct of_device_id *id;
>   	struct mtk_disp_pwm *mdp;
>   	struct resource *r;
>   	int ret;
>
> +	id = of_match_device(mtk_disp_pwm_of_match, &pdev->dev);
> +	if (!id)
> +		return -EINVAL;
> +
>   	mdp = devm_kzalloc(&pdev->dev, sizeof(*mdp), GFP_KERNEL);
>   	if (!mdp)
>   		return -ENOMEM;
> @@ -191,6 +240,7 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev)
>   	mdp->chip.ops = &mtk_disp_pwm_ops;
>   	mdp->chip.base = -1;
>   	mdp->chip.npwm = 1;
> +	mdp->data = id->data;
>
>   	ret = pwmchip_add(&mdp->chip);
>   	if (ret < 0) {
> @@ -200,6 +250,18 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev)
>
>   	platform_set_drvdata(pdev, mdp);
>
> +	/*
> +	 * For MT2701, disable double buffer before writing register
> +	 * and select manual mode and use PWM_PERIOD/PWM_HIGH_WIDTH.
> +	 */
> +	if (!mdp->data->have_commit_reg) {
> +		mtk_disp_pwm_update_bits(mdp, MT2701_PWM_BLS_DEBUG,
> +					 MT2701_PWM_BLS_DEBUG_MASK, 0x3);
> +		mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
> +					 MT2701_PWM_MANUAL_SEL_MASK,
> +					 mdp->data->con0_sel);
> +	}
> +
>   	return 0;
>
>   disable_clk_mm:
> @@ -221,13 +283,6 @@ static int mtk_disp_pwm_remove(struct platform_device *pdev)
>   	return ret;
>   }
>
> -static const struct of_device_id mtk_disp_pwm_of_match[] = {
> -	{ .compatible = "mediatek,mt8173-disp-pwm" },
> -	{ .compatible = "mediatek,mt6595-disp-pwm" },
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(of, mtk_disp_pwm_of_match);
> -
>   static struct platform_driver mtk_disp_pwm_driver = {
>   	.driver = {
>   		.name = "mediatek-disp-pwm",
>

  reply	other threads:[~2016-06-03 15:36 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-30  8:41 [PATCH RESEND 0/3] Support Mediatek Soc MT2701 disp pwm Weiqing Kong
2016-05-30  8:41 ` [PATCH 1/3] dt-bindings: pwm: Add MediaTek display PWM bindings Weiqing Kong
2016-06-02 22:45   ` Rob Herring
2016-06-03  6:45     ` weiqing kong
2016-06-03 15:03       ` Matthias Brugger
2016-06-08  6:48         ` weiqing kong
2016-05-30  8:41 ` [PATCH 2/3] pwm: Add MediaTek MT2701 display PWM driver support Weiqing Kong
2016-06-03 15:35   ` Matthias Brugger [this message]
2016-06-08  7:21     ` weiqing kong
2016-06-08 11:22       ` Matthias Brugger
2016-06-08 12:05         ` weiqing kong
2016-05-30  8:41 ` [PATCH 3/3] dts: Add disp bls dsti and dts config for MT2701 Weiqing Kong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5751A3DD.7020402@gmail.com \
    --to=matthias.bgg@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=eddie.huang@mediatek.com \
    --cc=erin.lo@mediatek.com \
    --cc=galak@codeaurora.org \
    --cc=ijc+devicetree@hellion.org.uk \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=mark.rutland@arm.com \
    --cc=pawel.moll@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=srv_heupstream@mediatek.com \
    --cc=thierry.reding@gmail.com \
    --cc=weiqing.kong@mediatek.com \
    --cc=yh.huang@mediatek.com \
    --cc=yingjoe.chen@mediatek.com \
    --cc=yt.shen@mediatek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).