Hi Siarhei,
On Sun, Feb 5, 2017 at 1:55 AM, <lis8...@gmail.com> wrote:
> From: Siarhei Volkau <lis8...@gmail.com>
>
> A31 SoC have a different map of PWM registers than others Allwinner
> SoCs, so the operation of access to the registers reworked for all
> existing in driver SoCs.
>
> Tested on Onda V972 (a31) and Marsboard A20, but only PWM
> channel 0, because other channels pins are not routed or
> have another function on these boards.
>
> Signed-off-by: Siarhei Volkau <lis8...@gmail.com>
> ---
> arch/arm/boot/dts/sun6i-a31.dtsi | 8 ++
> drivers/pwm/pwm-sun4i.c | 195
> +++++++++++++++++++++++++++++++++------
> 2 files changed, 175 insertions(+), 28 deletions(-)
You should put the dts changes in the separate patches as they go
through different trees.
Also, you haven't updated the binding documentation.
>
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index b0803f6..bd049c3 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -65,10 +72,41 @@ static const u32 prescaler_table[] = {
> 0, /* Actually 1 but tested separately */
> };
>
> +static const u32 sun6i_prescaler_table[] = {
> + 1,
> + 2,
> + 4,
> + 8,
> + 16,
> + 32,
> + 64,
> + 0,
> + 0,
> + 0,
> + 0,
> + 0,
> + 0,
> + 0,
> + 0,
> + 0,
> +};
> +
> +struct sun4i_pwm_chip;
Move the definition up here instead of doing this. (Probably should
also be in a separate patch)
> +
> +struct sunxi_reg_ops {
> + int (*ctl_rdy)(struct sun4i_pwm_chip *chip, int npwm);
> + u32 (*ctl_read)(struct sun4i_pwm_chip *chip, int npwm);
> + void (*ctl_write)(struct sun4i_pwm_chip *chip, int npwm, u32 val);
> + u32 (*prd_read)(struct sun4i_pwm_chip *chip, int npwm);
> + void (*prd_write)(struct sun4i_pwm_chip *chip, int npwm, u32 val);
> +};
> +
> struct sun4i_pwm_data {
> bool has_prescaler_bypass;
> bool has_rdy;
> unsigned int npwm;
> + const u32 *prescaler_table;
> + struct sunxi_reg_ops *ops;
> };
>
> struct sun4i_pwm_chip {
> @@ -96,10 +134,71 @@ static inline void sun4i_pwm_writel(struct
> sun4i_pwm_chip *chip,
> writel(val, chip->base + offset);
> }
>
> +static int sun4i_reg_ctl_rdy(struct sun4i_pwm_chip *chip, int npwm)
> +{
> + return PWM_RDY(npwm) & sun4i_pwm_readl(chip, PWM_CTRL_REG);
> +}
> +
> +static int sun6i_reg_ctl_rdy(struct sun4i_pwm_chip *chip, int npwm)
> +{
> + u32 val = sun4i_pwm_readl(chip, SUN6I_PWM_CH_CTL(npwm));
> +
> + return val & BIT(SUN6I_PWM_RDY_BIT);
> +}
> +
> +static u32 sun4i_reg_ctl_read(struct sun4i_pwm_chip *chip, int npwm)
> +{
> + u32 val = sun4i_pwm_readl(chip, PWM_CTRL_REG);
> +
> + return val >> (PWMCH_OFFSET * (npwm));
> +}
> +
> +static u32 sun6i_reg_ctl_read(struct sun4i_pwm_chip *chip, int npwm)
> +{
> + return sun4i_pwm_readl(chip, SUN6I_PWM_CH_CTL(npwm));
> +}
> +
> +static void sun4i_reg_ctl_write(struct sun4i_pwm_chip *chip, int npwm, u32
> val)
> +{
> + u32 rd = sun4i_pwm_readl(chip, PWM_CTRL_REG);
> +
> + rd &= ~(PWM_CHCTL_MASK << (PWMCH_OFFSET * npwm));
> + val &= (PWM_CHCTL_MASK << (PWMCH_OFFSET * npwm));
> + sun4i_pwm_writel(chip, rd | val, PWM_CTRL_REG);
> +}
> +
> +static void sun6i_reg_ctl_write(struct sun4i_pwm_chip *chip, int npwm, u32
> val)
> +{
> + return sun4i_pwm_writel(chip, val, SUN6I_PWM_CH_CTL(npwm));
> +}
> +
> +static u32 sun4i_reg_prd_read(struct sun4i_pwm_chip *chip, int npwm)
> +{
> + return sun4i_pwm_readl(chip, PWM_CH_PRD(npwm));
> +}
> +
> +static u32 sun6i_reg_prd_read(struct sun4i_pwm_chip *chip, int npwm)
> +{
> + return sun4i_pwm_readl(chip, SUN6I_PWM_CH_PRD(npwm));
> +}
> +
> +static void sun4i_reg_prd_write(struct sun4i_pwm_chip *chip, int npwm, u32
> val)
> +{
> + sun4i_pwm_writel(chip, val, PWM_CH_PRD(npwm));
> +}
> +
> +static void sun6i_reg_prd_write(struct sun4i_pwm_chip *chip, int npwm, u32
> val)
> +{
> + return sun4i_pwm_writel(chip, val, SUN6I_PWM_CH_PRD(npwm));
> +}
> +
> static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> int duty_ns, int period_ns)
> {
> struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> + const struct sun4i_pwm_data *data = sun4i_pwm->data;
I'm not sure of the value of defining a "data" variable as all it adds
a lot of churn to the patch and I believe that the compiler will do
something like this internally so there should be no performance
benefit.
> + struct sunxi_reg_ops *reg_ops = data->ops;
> + const u32 *prescaler_table = data->prescaler_table;
> u32 prd, dty, val, clk_gate;
> u64 clk_rate, div = 0;
> unsigned int prescaler = 0;
> @@ -319,6 +454,7 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> u32 val;
> int i, ret;
> const struct of_device_id *match;
> + struct sunxi_reg_ops *reg_ops = NULL;
No need to initialise this, you overwrite it unconditionally.
>
> match = of_match_device(sun4i_pwm_dt_ids, &pdev->dev);
>
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
