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.

Reply via email to