Date:   Thu, 25 Aug 2016 17:15:36 +0800
From:   Jian Yuan <yuanjian12@...ilicon.com>
To:     Thierry Reding <thierry.reding@...il.com>
CC:     <robh+dt@...nel.org>, <mark.rutland@....com>,
        <linux-pwm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <xuejiancheng@...ilicon.com>,
        <kevin.lixu@...ilicon.com>, <jalen.hsu@...ilicon.com>
Subject: Re: [PATCH v2] pwm: add pwm driver for HiSilicon BVT SOCs



On 2016/8/24 21:05, Thierry Reding wrote:
> On Mon, Aug 22, 2016 at 03:50:13PM +0800, Jian Yuan wrote:
>> From: yuanjian <yuanjian12@...ilicon.com>
>>
>> Add pwm driver for HiSilicon BVT SOCs
> 
> pwm -> PWM, please. It'd be good to have more information here about
> what the hardware can do, where to find it, etc.
> 
Not sure what you mean? Should I describe what the PWM or the BVT SoCs can do?
>>
>> Reviewed-by: Jiancheng Xue <xuejiancheng@...ilicon.com>
>> Signed-off-by: Jian Yuan <yuanjian12@...ilicon.com>
>> ---
>> Change Log:
>> v2:
>> The number of PWMs is change to be probeable based on the compatible string.
>>
>>  .../devicetree/bindings/pwm/pwm-hibvt.txt          |  18 ++
>>  drivers/pwm/Kconfig                                |  10 +
>>  drivers/pwm/Makefile                               |   1 +
>>  drivers/pwm/pwm-hibvt.c                            | 274 +++++++++++++++++++++
>>  4 files changed, 303 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
>>  create mode 100644 drivers/pwm/pwm-hibvt.c
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
>> new file mode 100644
>> index 0000000..1274119
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
>> @@ -0,0 +1,18 @@
>> +Hisilicon PWM controller
>> +
>> +Required properties:
>> +-compatible: should contain one soc specific compatible string and one generic compatible
>> +string "hisilicon, hibvt-pwm". The soc specific strings supported including:
> 
> Why the generic compatible string? You've already shown in the driver
> that the two versions you support aren't compatible.
>
The generic compatible string should be contained in every devicetree bindings that BVT Socs support.
But I'll add another specific compatible string to distinguish it from "hisilicon,hi3516cv300-pwm".

> Also soc -> SoC, please.
> 
>> +	"hisilicon,hi3516cv300-pwm"
>> +- reg: physical base address and length of the controller's registers.
>> +- clocks: phandle and clock specifier of the PWM reference clock.
>> +- resets: offset address and offset bit for reset or unreset of the controller.
> 
> That's weird. Usually this should say just that the property should
> contain the phandle plus a reset specifier for the controller reset. The
> above description already defines what the specifier looks like, which
> may not be true on all SoCs that this hardware gets integrated into.
> 
Ok.
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index c182efc..3c48768 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -158,6 +158,15 @@ config PWM_FSL_FTM
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-fsl-ftm.
>>  
>> +config PWM_HIBVT
>> +	tristate "HiSilicon BVT PWM support"
>> +	depends on ARCH_HISI
>> +	help
>> +	  Generic PWM framework driver for hisilicon BVT SOCs.
> 
> Please use a consistent spelling for HiSilicon. Also: SOC -> SoC.
> 
Ok.
>> @@ -438,4 +447,5 @@ config PWM_VT8500
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-vt8500.
>>  
>> +
> 
> Please don't add this extra blank line.
> 
Ok.
>> diff --git a/drivers/pwm/pwm-hibvt.c b/drivers/pwm/pwm-hibvt.c
> [...]
>> new file mode 100644
>> index 0000000..84f617e
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-hibvt.c
>> @@ -0,0 +1,274 @@
>> +/*
>> + * PWM Controller Driver for HiSilicon BVT SOCs
>> + *
>> + * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/reset.h>
>> +
>> +#define PWM_CFG0_ADDR(x)    (((x) * 0x20) + 0x0)
>> +#define PWM_CFG1_ADDR(x)    (((x) * 0x20) + 0x4)
>> +#define PWM_CFG2_ADDR(x)    (((x) * 0x20) + 0x8)
>> +#define PWM_CTRL_ADDR(x)    (((x) * 0x20) + 0xC)
>> +
>> +#define PWM_ENABLE_SHIFT    0
>> +#define PWM_ENABLE_MASK		BIT(0)
>> +
>> +#define PWM_POLARITY_SHIFT	1
>> +#define PWM_POLARITY_MASK	BIT(1)
>> +
>> +#define PWM_KEEP_SHIFT	    2
>> +#define PWM_KEEP_MASK	    BIT(2)
>> +
>> +#define PWM_PERIOD_MASK	    GENMASK(31, 0)
>> +#define PWM_DUTY_MASK	    GENMASK(31, 0)
> 
> There's inconsistent padding in the above. I thought checkpatch warned
> about this nowadays.
> 
>> +
>> +struct hibvt_pwm_chip {
>> +	struct pwm_chip	chip;
>> +	struct clk	*clk;
>> +	void __iomem	*mmio_base;
>> +	struct reset_control *rstc;
>> +};
> 
> No artificial padding with tabs above. A single space between type and
> variable name is good enough. Also why the extra mmio_ prefix for the
> I/O memory base address? You use the much shorter "base" for variables
> elsewhere, why not in this structure?
> 
Ok.
>> +
>> +static int pwm_num_array[] = {8, 4};
> 
> static const unsigned, please. Or better yet, introduce a separate
> structure for this type of SoC-specific data. Something like:
> 
> 	struct hibvt_pwm_soc {
> 		unsigned int num_pwms;
> 	};
> 
Good idea.
>> +
>> +static inline
>> +struct hibvt_pwm_chip *to_hibvt_pwm_chip(struct pwm_chip *chip)
> 
> It's more idiomatic to put the return type on the first line as well.
> 
Ok.
>> +{
>> +	return container_of(chip, struct hibvt_pwm_chip, chip);
>> +}
>> +
>> +static void hibvt_pwm_set_bits(void __iomem *base, unsigned int offset,
>> +					unsigned int mask, unsigned int data)
> 
> Please align arguments on subsequent lines with the first argument on
> the first line. And use u32 for mask and data since that's what readl()
> and writel() use.
> 
Ok.
>> +{
>> +	void __iomem *address = base + offset;
>> +	unsigned int value;
> 
> u32, please.
> 
Ok.
>> +
>> +	value = readl(address);
>> +	value &= ~mask;
>> +	value |= (data & mask);
>> +	writel(value, address);
>> +}
>> +
>> +static int hibvt_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
>> +	unsigned int offset;
>> +
>> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
>> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
> 
> I don't think the temporary variable "offset" is useful here.
> 
>> +			PWM_ENABLE_MASK, 0x1);
> 
> Also, please align this properly.
> 
Ok.
>> +
>> +	return 0;
>> +}
>> +
>> +static void hibvt_pwm_disable(struct pwm_chip *chip,
>> +					struct pwm_device *pwm)
> 
> The above will fit on a single line.
> 
Ok.
>> +{
>> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
>> +	unsigned int offset;
>> +
>> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
>> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
>> +			PWM_ENABLE_MASK, 0x0);
> 
> Again, I don't think the offset variable is any good here.
> 
Ok.
>> +}
>> +
>> +static int hibvt_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> +					int duty_cycle_ns, int period_ns)
>> +{
>> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
>> +	unsigned int offset;
>> +	unsigned int freq;
>> +	unsigned int period_num, duty_num;
> 
> Why the _num suffix? You could just name them "period" and "duty". Also
> you can put the last four variables above on the same line.
> 
Ok.
>> +
>> +	freq = div_u64(clk_get_rate(hi_pwm_chip->clk), 1000000);
>> +
>> +	period_num = div_u64(freq * period_ns, 1000);
>> +	duty_num = div_u64(period_num * duty_cycle_ns, period_ns);
>> +
>> +	offset = PWM_CFG0_ADDR(pwm->hwpwm);
>> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
>> +			PWM_PERIOD_MASK, period_num);
>> +
>> +	offset = PWM_CFG1_ADDR(pwm->hwpwm);
>> +	hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
>> +			PWM_DUTY_MASK, duty_num);
> 
> offset can be dropped here as well, in my opinion.
> 
Ok.
>> +
>> +	return 0;
>> +}
>> +
>> +static int hibvt_pwm_set_polarity(struct pwm_chip *chip,
>> +					struct pwm_device *pwm,
>> +					enum pwm_polarity polarity)
>> +{
>> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
>> +	unsigned int offset;
>> +
>> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
>> +	if (polarity == PWM_POLARITY_INVERSED)
>> +		hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
>> +				PWM_POLARITY_MASK, (0x1 << PWM_POLARITY_SHIFT));
>> +	else
>> +		hibvt_pwm_set_bits(hi_pwm_chip->mmio_base, offset,
>> +				PWM_POLARITY_MASK, (0x0 << PWM_POLARITY_SHIFT));
>> +
>> +	return 0;
>> +}
>> +
>> +void hibvt_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> +				struct pwm_state *state)
>> +{
>> +	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
>> +	void __iomem *base;
>> +	unsigned int offset;
>> +	unsigned int freq;
>> +	unsigned int period_num, duty_num;
>> +	unsigned int enable;
> 
> I think you could trim the list of variables down a little. offset can
> go away for reasons given above. And of the other four, the only one
> that you need to reuse is freq, so period_num, duty_num and enable can
> all be replaced by a "value" variable, for example. Also, make that an
> u32, because that's what readl() returns.
> 
Fine, it looks like a better solution.
>> +
>> +	freq = div_u64(clk_get_rate(hi_pwm_chip->clk), 1000000);
>> +	base = hi_pwm_chip->mmio_base;
>> +
>> +	offset = PWM_CFG0_ADDR(pwm->hwpwm);
>> +	period_num = readl(base + offset);
>> +	state->period = div_u64(period_num * 1000, freq);
>> +
>> +	offset = PWM_CFG1_ADDR(pwm->hwpwm);
>> +	duty_num = readl(base + offset);
>> +	state->duty_cycle = div_u64(duty_num * 1000, freq);
>> +
>> +	offset = PWM_CTRL_ADDR(pwm->hwpwm);
>> +	enable = readl(base + offset);
>> +	state->enabled = (PWM_ENABLE_MASK & enable);
>> +}
>> +
>> +static struct pwm_ops hibvt_pwm_ops = {
>> +	.enable       = hibvt_pwm_enable,
>> +	.disable      = hibvt_pwm_disable,
>> +	.config       = hibvt_pwm_config,
>> +	.set_polarity = hibvt_pwm_set_polarity,
>> +	.get_state    = hibvt_pwm_get_state,
> 
> Please don't mix legacy and atomic support. If you support .get_state(),
> you should implement .apply() instead of .enable()/.disable()/.config().
> Also, please remove the artificial padding.
> 
I agree to implement .apply() function.
>> +
>> +	.owner        = THIS_MODULE,
>> +};
>> +
>> +static const struct of_device_id hibvt_pwm_of_match[];
> 
> No need for this...
> 
Ok.
>> +
>> +static int hibvt_pwm_probe(struct platform_device *pdev)
>> +{
>> +	struct hibvt_pwm_chip *pwm_chip;
>> +	const struct of_device_id *of_id =
>> +				of_match_device(hibvt_pwm_of_match, &pdev->dev);
> 
> ... if you use of_device_get_match_data() here.
> 
>> +	struct resource *res;
>> +	int ret = 0;
> 
> No need to initialize to 0 here.
> 
>> +	int i;
> 
> unsigned int, please.
> 
>> +	int offset;
>> +	int pwm_nums;
> 
> I think both of these can be dropped. See below.
> 
>> +
>> +	if (!of_id)
>> +		return -ENODEV;
>> +
>> +	pwm_chip = devm_kzalloc(&pdev->dev, sizeof(*pwm_chip), GFP_KERNEL);
>> +	if (pwm_chip == NULL)
>> +		return -ENOMEM;
>> +
>> +	pwm_chip->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(pwm_chip->clk)) {
>> +		dev_err(&pdev->dev, "getting clock failed with %ld\n",
>> +				PTR_ERR(pwm_chip->clk));
>> +		return PTR_ERR(pwm_chip->clk);
>> +	}
>> +
>> +	pwm_nums = *((int *)of_id->data);
> 
> This becomes a lot easier to read if you use a proper structure for the
> SoC specific data:
> 
> 	const struct hibvt_pwm_soc *soc = of_device_get_match_data(&pdev->dev);
> 
> 	pwm_chip->chip.npwm = soc->num_pwms;
> 
That's right.
>> +
>> +	pwm_chip->chip.ops = &hibvt_pwm_ops;
>> +	pwm_chip->chip.dev = &pdev->dev;
>> +	pwm_chip->chip.base = -1;
>> +	pwm_chip->chip.npwm = pwm_nums;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	pwm_chip->mmio_base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(pwm_chip->mmio_base))
>> +		return PTR_ERR(pwm_chip->mmio_base);
>> +
>> +	ret = clk_prepare_enable(pwm_chip->clk);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	pwm_chip->rstc = devm_reset_control_get(&pdev->dev, NULL);
>> +	if (IS_ERR(pwm_chip->rstc))
>> +		return PTR_ERR(pwm_chip->rstc);
>> +
> 
> You might want to move clk_prepare_enable() below this so you don't keep
> it enabled when the reset control can't be requested.
> 
Ok, I'll add it.
>> +	reset_control_assert(pwm_chip->rstc);
>> +	msleep(30);
>> +	reset_control_deassert(pwm_chip->rstc);
>> +
>> +	ret = pwmchip_add(&pwm_chip->chip);
>> +	if (ret < 0)
>> +		return ret;
> 
> You might want to assert the reset and disable and unprepare the clock
> when this fails.
> 
>> +
>> +	for (i = 0; i < pwm_nums; i++) {
>> +		offset = PWM_CTRL_ADDR(i);
>> +		hibvt_pwm_set_bits(pwm_chip->mmio_base, offset,
> 
> Things will still fit on one line if you use PWM_CTRL_ADDR(i) in place
> above, so I don't think you gain anything by adding the temporary
> variable.
> 
>> +				PWM_KEEP_MASK, (0x1 << PWM_KEEP_SHIFT));
>> +	}
>> +
>> +	platform_set_drvdata(pdev, pwm_chip);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hibvt_pwm_remove(struct platform_device *pdev)
>> +{
>> +	struct hibvt_pwm_chip *pwm_chip;
>> +
>> +	pwm_chip = platform_get_drvdata(pdev);
>> +	if (pwm_chip == NULL)
>> +		return -ENODEV;
> 
> There's no way this will ever happen.
> 
>> +
>> +	clk_disable_unprepare(pwm_chip->clk);
> 
> What about the reset?
> 
Ok.
>> +
>> +	return pwmchip_remove(&pwm_chip->chip);
>> +}
>> +
>> +static const struct of_device_id hibvt_pwm_of_match[] = {
>> +	{.compatible = "hisilicon,hibvt-pwm", .data = &pwm_num_array[0]},
>> +	{.compatible = "hisilicon,hi3516cv300-pwm", .data = &pwm_num_array[1]},
> 
> Spaces after { and before }, please.
> 
Ok.
>> +	{  }
>> +};
>> +MODULE_DEVICE_TABLE(of, hibvt_pwm_of_match);
>> +
>> +static struct platform_driver hibvt_pwm_driver = {
>> +	.driver		= {
>> +		.name	= "hibvt-pwm",
>> +		.of_match_table = hibvt_pwm_of_match,
>> +	},
>> +	.probe		= hibvt_pwm_probe,
>> +	.remove		= hibvt_pwm_remove,
>> +};
> 
> No need for the artificial padding, it's not working correctly for the
> .of_match_table field anyway.
> 
>> +
>> +module_platform_driver(hibvt_pwm_driver);
> 
> No blank line between the "};" and "module_platform_driver(...);" line,
> please.
> 
>> +
>> +MODULE_AUTHOR("yuanjian12@...ilicon.com");
> 
> It's customary to put your real name here.
> 
>> +MODULE_DESCRIPTION("Hisilicon BVT SOCs PWM driver");
> 
> Again, please use consistent spelling for HiSilicon.
> 
>> +MODULE_LICENSE("GPL v2");
> 
> The header comment says "GPL v2 or later", but "GPL v2" here means GPL
> v2 only.
> 
Ok.
> Thierry
> 

Thanks.
Jian Yuan

Powered by blists - more mailing lists