Re: [PATCH v3 2/2] pwm: Add Broadcom BCM7038 PWM controller support

[Florian Fainelli]

On 07/09/15 12:15, Ariel D'Alessandro wrote:
> Hi Florian,
> 
> I wrote some observations below that maybe can be useful.
> 
> El 28/08/15 a las 22:21, Florian Fainelli escribió:
>> Add support for the BCM7038-style PWM controller found in all BCM7xxx STB 
>> SoCs.
>> This controller has a hardcoded 2 channels per controller, and cascades a
>> variable frequency generator on top of a fixed frequency generator which 
>> offers
>> a range of a 148ns period all the way to ~622ms periods.
>>
>> Signed-off-by: Florian Fainelli 

NB: you can trim your replies so we do not have to skip through lengthy
uncommented parts of the patch.

[snip]

>> +
>> +static inline u32 pwm_readl(struct brcmstb_pwm_dev *p, u32 off)
> 
> The function name 'pwm_readl' sounds to be very common. It might be
> better to use a prefix here, don't you think? Maybe brcmstb_pwm_readl?

Sure, if that makes it clearer, these are local and inlined, so the
chances for getting a namespace conflict are very thin, but fair enough,
will rename to match the rest of the functions.

[snip]

>> +/*
>> + * We can be called with separate duty and period updates,
>> + * so do not reject dc == 0 right away
>> + */
>> +if (pc == PWM_PERIOD_MIN ||
>> +   (dc < PWM_ON_MIN && duty_ns))
> 
> No break needed here, this expression can be written on a single line
> increasing readability.
> 
>> +return -EINVAL;
>> +
>> +/* We converged on a calculation */
>> +if (pc <= PWM_ON_PERIOD_MAX && dc <= PWM_ON_PERIOD_MAX)
>> +break;
>> +
>> +/*
>> + * The cword needs to be a power of 2 for the variable
>> + * frequency generator to output a 50% duty cycle variable
>> + * frequency which is used as input clock to the fixed
>> + * frequency generator.
>> + */
>> +cword >>= 1;
>> +
>> +/*
>> + * Desired periods are too large, we do not have a divider
>> + * for them
>> + */
>> +if (cword < CONST_VAR_F_MIN)
>> +return -EINVAL;
>> +}
>> +
>> +done:
>> +/*
>> + * Configure the defined "cword" value to have the variable frequency
>> + * generator output a base frequency for the constant frequency
>> + * generator to derive from.
>> + */
>> +pwm_writel(b, cword >> 8, PWM_CWORD_MSB + ch * PWM_CH_SIZE);
>> +pwm_writel(b, cword & 0xff, PWM_CWORD_LSB + ch * PWM_CH_SIZE);
>> +
>> +/* Select constant frequency signal output */
>> +reg = pwm_readl(b, PWM_CTRL2);
>> +reg |= (CTRL2_OUT_SELECT << (ch * CTRL_CHAN_OFFS));
> 
> A nitpick here, outer parenthesis can be avoided.
> 
>> +pwm_writel(b, reg, PWM_CTRL2);
> 
> This read-modify-write sequence may be protected by some locking
> mechanism. Notice that, as written in the docs: "PWM core does not
> enforce any locking to pwm_enable(), pwm_disable() and pwm_config()".

Right, I will add required locking here, thanks!

[snip]

>> +r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +p->base = devm_ioremap_resource(>dev, r);
>> +if (!p->base)
>> +return -ENOMEM;
> 
> I think you're missing some cleanup routine here. You have a previous
> call to clk_prepare_enable(), so you may have a call to
> clk_disable_unprepare() here in order to exit cleanly.

Good catch yes.

> 
>> +
>> +ret = pwmchip_add(>chip);
>> +if (ret)
>> +dev_err(>dev, "failed to add PWM chip %d\n", ret);
> 
> Cleanup needed here too.
> 
>> +
>> +return ret;
>> +}
>> +
>> +static int brcmstb_pwm_remove(struct platform_device *pdev)
>> +{
>> +struct brcmstb_pwm_dev *p = platform_get_drvdata(pdev);
>> +
>> +clk_disable_unprepare(p->clk);
>> +
>> +return pwmchip_remove(>chip);
> 
> AFAIK, pwmchip_remove() may return busy if the PWM chip provides a PWM
> device that is still requested, so you shouldn't disable the clock
> before you ensure the PWM chip has been successfuly removed.

Absolutely.

> 
> It think you could do something like:
> 
>   ret = pwmchip_remove(>chip);
>   if (ret)
>   return ret;
> 
>   clk_disable_unprepare(p->clk);
>   return 0;
> 

-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 2/2] pwm: Add Broadcom BCM7038 PWM controller support

[Florian Fainelli]

On 07/09/15 12:15, Ariel D'Alessandro wrote:
> Hi Florian,
> 
> I wrote some observations below that maybe can be useful.
> 
> El 28/08/15 a las 22:21, Florian Fainelli escribió:
>> Add support for the BCM7038-style PWM controller found in all BCM7xxx STB 
>> SoCs.
>> This controller has a hardcoded 2 channels per controller, and cascades a
>> variable frequency generator on top of a fixed frequency generator which 
>> offers
>> a range of a 148ns period all the way to ~622ms periods.
>>
>> Signed-off-by: Florian Fainelli 

NB: you can trim your replies so we do not have to skip through lengthy
uncommented parts of the patch.

[snip]

>> +
>> +static inline u32 pwm_readl(struct brcmstb_pwm_dev *p, u32 off)
> 
> The function name 'pwm_readl' sounds to be very common. It might be
> better to use a prefix here, don't you think? Maybe brcmstb_pwm_readl?

Sure, if that makes it clearer, these are local and inlined, so the
chances for getting a namespace conflict are very thin, but fair enough,
will rename to match the rest of the functions.

[snip]

>> +/*
>> + * We can be called with separate duty and period updates,
>> + * so do not reject dc == 0 right away
>> + */
>> +if (pc == PWM_PERIOD_MIN ||
>> +   (dc < PWM_ON_MIN && duty_ns))
> 
> No break needed here, this expression can be written on a single line
> increasing readability.
> 
>> +return -EINVAL;
>> +
>> +/* We converged on a calculation */
>> +if (pc <= PWM_ON_PERIOD_MAX && dc <= PWM_ON_PERIOD_MAX)
>> +break;
>> +
>> +/*
>> + * The cword needs to be a power of 2 for the variable
>> + * frequency generator to output a 50% duty cycle variable
>> + * frequency which is used as input clock to the fixed
>> + * frequency generator.
>> + */
>> +cword >>= 1;
>> +
>> +/*
>> + * Desired periods are too large, we do not have a divider
>> + * for them
>> + */
>> +if (cword < CONST_VAR_F_MIN)
>> +return -EINVAL;
>> +}
>> +
>> +done:
>> +/*
>> + * Configure the defined "cword" value to have the variable frequency
>> + * generator output a base frequency for the constant frequency
>> + * generator to derive from.
>> + */
>> +pwm_writel(b, cword >> 8, PWM_CWORD_MSB + ch * PWM_CH_SIZE);
>> +pwm_writel(b, cword & 0xff, PWM_CWORD_LSB + ch * PWM_CH_SIZE);
>> +
>> +/* Select constant frequency signal output */
>> +reg = pwm_readl(b, PWM_CTRL2);
>> +reg |= (CTRL2_OUT_SELECT << (ch * CTRL_CHAN_OFFS));
> 
> A nitpick here, outer parenthesis can be avoided.
> 
>> +pwm_writel(b, reg, PWM_CTRL2);
> 
> This read-modify-write sequence may be protected by some locking
> mechanism. Notice that, as written in the docs: "PWM core does not
> enforce any locking to pwm_enable(), pwm_disable() and pwm_config()".

Right, I will add required locking here, thanks!

[snip]

>> +r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +p->base = devm_ioremap_resource(>dev, r);
>> +if (!p->base)
>> +return -ENOMEM;
> 
> I think you're missing some cleanup routine here. You have a previous
> call to clk_prepare_enable(), so you may have a call to
> clk_disable_unprepare() here in order to exit cleanly.

Good catch yes.

> 
>> +
>> +ret = pwmchip_add(>chip);
>> +if (ret)
>> +dev_err(>dev, "failed to add PWM chip %d\n", ret);
> 
> Cleanup needed here too.
> 
>> +
>> +return ret;
>> +}
>> +
>> +static int brcmstb_pwm_remove(struct platform_device *pdev)
>> +{
>> +struct brcmstb_pwm_dev *p = platform_get_drvdata(pdev);
>> +
>> +clk_disable_unprepare(p->clk);
>> +
>> +return pwmchip_remove(>chip);
> 
> AFAIK, pwmchip_remove() may return busy if the PWM chip provides a PWM
> device that is still requested, so you shouldn't disable the clock
> before you ensure the PWM chip has been successfuly removed.

Absolutely.

> 
> It think you could do something like:
> 
>   ret = pwmchip_remove(>chip);
>   if (ret)
>   return ret;
> 
>   clk_disable_unprepare(p->clk);
>   return 0;
> 

-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 2/2] pwm: Add Broadcom BCM7038 PWM controller support

[Ariel D'Alessandro]

Hi Florian,

I wrote some observations below that maybe can be useful.

El 28/08/15 a las 22:21, Florian Fainelli escribió:
> Add support for the BCM7038-style PWM controller found in all BCM7xxx STB 
> SoCs.
> This controller has a hardcoded 2 channels per controller, and cascades a
> variable frequency generator on top of a fixed frequency generator which 
> offers
> a range of a 148ns period all the way to ~622ms periods.
> 
> Signed-off-by: Florian Fainelli 
> ---
> Changes in v3:
> 
> - make clock mandatory
> - removed a remaining div64_u64 use
> 
> hanges in v2:
> 
> - properly format comments
> - utilize do_div instead of div64_u64
> - avoid using a "done" variable for the while loop
> - utilize a parameterized register accessor
> - remove a bunch of unnecessary assignments
> - provide a module author
> - update depends to build on BMIPS_GENERIC (the other user)
> - removed artificial padding
> - removed used only once variable: dn
> - utilize devm_ioremap_resource
> - do not print success message
> - removed THIS_MODULE from platform_driver structure
> 
>  drivers/pwm/Kconfig   |  10 ++
>  drivers/pwm/Makefile  |   1 +
>  drivers/pwm/pwm-brcmstb.c | 324 
> ++
>  3 files changed, 335 insertions(+)
>  create mode 100644 drivers/pwm/pwm-brcmstb.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f40fd8d..363c22b22071 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -111,6 +111,16 @@ config PWM_CLPS711X
> To compile this driver as a module, choose M here: the module
> will be called pwm-clps711x.
>  
> +config PWM_BRCMSTB
> + tristate "Broadcom STB PWM support"
> + depends on ARCH_BRCMSTB || BMIPS_GENERIC
> + help
> +   Generic PWM framework driver for the Broadcom Set-top-Box
> +   SoCs (BCM7xxx).
> +
> +   To compile this driver as a module, choose M Here: the module
> +   will be called pwm-brcmstb.c.
> +
>  config PWM_EP93XX
>   tristate "Cirrus Logic EP93xx PWM support"
>   depends on ARCH_EP93XX
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ec50eb5b5a8f..dc7b1b82d47e 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_PWM_ATMEL_TCB)   += pwm-atmel-tcb.o
>  obj-$(CONFIG_PWM_BCM_KONA)   += pwm-bcm-kona.o
>  obj-$(CONFIG_PWM_BCM2835)+= pwm-bcm2835.o
>  obj-$(CONFIG_PWM_BFIN)   += pwm-bfin.o
> +obj-$(CONFIG_PWM_BRCMSTB)+= pwm-brcmstb.o
>  obj-$(CONFIG_PWM_CLPS711X)   += pwm-clps711x.o
>  obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o
>  obj-$(CONFIG_PWM_FSL_FTM)+= pwm-fsl-ftm.o
> diff --git a/drivers/pwm/pwm-brcmstb.c b/drivers/pwm/pwm-brcmstb.c
> new file mode 100644
> index ..9ea73755f281
> --- /dev/null
> +++ b/drivers/pwm/pwm-brcmstb.c
> @@ -0,0 +1,324 @@
> +/*
> + * Broadcom BCM7038 PWM driver
> + * Author: Florian Fainelli
> + *
> + * Copyright (C) 2015 Broadcom Corporation
> + *
> + * 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.
> + */
> +
> +#define pr_fmt(fmt)  KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PWM_CTRL 0x00
> +#define  CTRL_START  BIT(0)
> +#define  CTRL_OEBBIT(1)
> +#define  CTRL_FORCE_HIGH BIT(2)
> +#define  CTRL_OPENDRAIN  BIT(3)
> +#define  CTRL_CHAN_OFFS  4
> +
> +#define PWM_CTRL20x04
> +#define  CTRL2_OUT_SELECTBIT(0)
> +
> +#define PWM_CWORD_MSB0x08
> +#define PWM_CWORD_LSB0x0C
> +
> +#define PWM_CH_SIZE  0x8
> +
> +/* Number of bits for the CWORD value */
> +#define CWORD_BIT_SIZE   16
> +
> +/*
> + * Maximum control word value allowed when variable-frequency PWM is used as 
> a
> + * clock for the constant-frequency PMW.
> + */
> +#define CONST_VAR_F_MAX  32768
> +#define CONST_VAR_F_MIN  1
> +
> +#define PWM_ON(ch)   (0x18 + ((ch) * PWM_CH_SIZE))
> +#define  PWM_ON_MIN  1
> +#define PWM_PERIOD(ch)   (0x1C + ((ch) * PWM_CH_SIZE))
> +#define  PWM_PERIOD_MIN  0
> +
> +#define PWM_ON_PERIOD_MAX0xff
> +
> +struct brcmstb_pwm_dev {
> + void __iomem *base;
> + struct clk *clk;
> + struct pwm_chip chip;
> +};
> +
> +static inline u32 pwm_readl(struct brcmstb_pwm_dev *p, u32 off)

The function name 'pwm_readl' sounds to be very common. It might be

Re: [PATCH v3 2/2] pwm: Add Broadcom BCM7038 PWM controller support

[Ariel D'Alessandro]

Hi Florian,

I wrote some observations below that maybe can be useful.

El 28/08/15 a las 22:21, Florian Fainelli escribió:
> Add support for the BCM7038-style PWM controller found in all BCM7xxx STB 
> SoCs.
> This controller has a hardcoded 2 channels per controller, and cascades a
> variable frequency generator on top of a fixed frequency generator which 
> offers
> a range of a 148ns period all the way to ~622ms periods.
> 
> Signed-off-by: Florian Fainelli 
> ---
> Changes in v3:
> 
> - make clock mandatory
> - removed a remaining div64_u64 use
> 
> hanges in v2:
> 
> - properly format comments
> - utilize do_div instead of div64_u64
> - avoid using a "done" variable for the while loop
> - utilize a parameterized register accessor
> - remove a bunch of unnecessary assignments
> - provide a module author
> - update depends to build on BMIPS_GENERIC (the other user)
> - removed artificial padding
> - removed used only once variable: dn
> - utilize devm_ioremap_resource
> - do not print success message
> - removed THIS_MODULE from platform_driver structure
> 
>  drivers/pwm/Kconfig   |  10 ++
>  drivers/pwm/Makefile  |   1 +
>  drivers/pwm/pwm-brcmstb.c | 324 
> ++
>  3 files changed, 335 insertions(+)
>  create mode 100644 drivers/pwm/pwm-brcmstb.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f40fd8d..363c22b22071 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -111,6 +111,16 @@ config PWM_CLPS711X
> To compile this driver as a module, choose M here: the module
> will be called pwm-clps711x.
>  
> +config PWM_BRCMSTB
> + tristate "Broadcom STB PWM support"
> + depends on ARCH_BRCMSTB || BMIPS_GENERIC
> + help
> +   Generic PWM framework driver for the Broadcom Set-top-Box
> +   SoCs (BCM7xxx).
> +
> +   To compile this driver as a module, choose M Here: the module
> +   will be called pwm-brcmstb.c.
> +
>  config PWM_EP93XX
>   tristate "Cirrus Logic EP93xx PWM support"
>   depends on ARCH_EP93XX
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ec50eb5b5a8f..dc7b1b82d47e 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_PWM_ATMEL_TCB)   += pwm-atmel-tcb.o
>  obj-$(CONFIG_PWM_BCM_KONA)   += pwm-bcm-kona.o
>  obj-$(CONFIG_PWM_BCM2835)+= pwm-bcm2835.o
>  obj-$(CONFIG_PWM_BFIN)   += pwm-bfin.o
> +obj-$(CONFIG_PWM_BRCMSTB)+= pwm-brcmstb.o
>  obj-$(CONFIG_PWM_CLPS711X)   += pwm-clps711x.o
>  obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o
>  obj-$(CONFIG_PWM_FSL_FTM)+= pwm-fsl-ftm.o
> diff --git a/drivers/pwm/pwm-brcmstb.c b/drivers/pwm/pwm-brcmstb.c
> new file mode 100644
> index ..9ea73755f281
> --- /dev/null
> +++ b/drivers/pwm/pwm-brcmstb.c
> @@ -0,0 +1,324 @@
> +/*
> + * Broadcom BCM7038 PWM driver
> + * Author: Florian Fainelli
> + *
> + * Copyright (C) 2015 Broadcom Corporation
> + *
> + * 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.
> + */
> +
> +#define pr_fmt(fmt)  KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PWM_CTRL 0x00
> +#define  CTRL_START  BIT(0)
> +#define  CTRL_OEBBIT(1)
> +#define  CTRL_FORCE_HIGH BIT(2)
> +#define  CTRL_OPENDRAIN  BIT(3)
> +#define  CTRL_CHAN_OFFS  4
> +
> +#define PWM_CTRL20x04
> +#define  CTRL2_OUT_SELECTBIT(0)
> +
> +#define PWM_CWORD_MSB0x08
> +#define PWM_CWORD_LSB0x0C
> +
> +#define PWM_CH_SIZE  0x8
> +
> +/* Number of bits for the CWORD value */
> +#define CWORD_BIT_SIZE   16
> +
> +/*
> + * Maximum control word value allowed when variable-frequency PWM is used as 
> a
> + * clock for the constant-frequency PMW.
> + */
> +#define CONST_VAR_F_MAX  32768
> +#define CONST_VAR_F_MIN  1
> +
> +#define PWM_ON(ch)   (0x18 + ((ch) * PWM_CH_SIZE))
> +#define  PWM_ON_MIN  1
> +#define PWM_PERIOD(ch)   (0x1C + ((ch) * PWM_CH_SIZE))
> +#define  PWM_PERIOD_MIN  0
> +
> +#define PWM_ON_PERIOD_MAX0xff
> +
> +struct brcmstb_pwm_dev {
> + void __iomem *base;
> + struct clk *clk;
> + struct pwm_chip chip;
> +};
> +
> +static inline u32 pwm_readl(struct brcmstb_pwm_dev *p, u32 off)

The function name 'pwm_readl' sounds to be very