Re: [PATCH v4 1/9] pwm: sunxi: Use regmap API for register access.

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3807 bytes --]

On Mon, Feb 27, 2017 at 02:22:02PM +0300, Siarhei Volkau wrote:
> Hi, Maxime
> 
> 2017-02-27 12:17 GMT+03:00 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > Hi Siarhei,
> >
> > On Fri, Feb 24, 2017 at 08:41:08AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >> From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>
> >> The patch replaces iomem register access routines to regmap
> >> equivalents.
> >>
> >> Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  drivers/pwm/Kconfig     |   2 +-
> >>  drivers/pwm/pwm-sun4i.c | 143 ++++++++++++++++++++++++++++++++++++------------
> >>  2 files changed, 110 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index 2d0cfaa..6b4dc1a 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -416,7 +416,7 @@ config PWM_STMPE
> >>  config PWM_SUN4I
> >>       tristate "Allwinner PWM support"
> >>       depends on ARCH_SUNXI || COMPILE_TEST
> >> -     depends on HAS_IOMEM && COMMON_CLK
> >> +     depends on REGMAP_MMIO && COMMON_CLK
> >>       help
> >>         Generic PWM framework driver for Allwinner SoCs.
> >>
> >> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> >> index b0803f6..5565f03 100644
> >> --- a/drivers/pwm/pwm-sun4i.c
> >> +++ b/drivers/pwm/pwm-sun4i.c
> >> @@ -9,7 +9,7 @@
> >>  #include <linux/bitops.h>
> >>  #include <linux/clk.h>
> >>  #include <linux/err.h>
> >> -#include <linux/io.h>
> >> +#include <linux/regmap.h>
> >>  #include <linux/module.h>
> >>  #include <linux/of.h>
> >>  #include <linux/of_device.h>
> >> @@ -74,7 +74,7 @@ struct sun4i_pwm_data {
> >>  struct sun4i_pwm_chip {
> >>       struct pwm_chip chip;
> >>       struct clk *clk;
> >> -     void __iomem *base;
> >> +     struct regmap *regmap;
> >>       spinlock_t ctrl_lock;
> >>       const struct sun4i_pwm_data *data;
> >>  };
> >> @@ -84,18 +84,6 @@ static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)
> >>       return container_of(chip, struct sun4i_pwm_chip, chip);
> >>  }
> >>
> >> -static inline u32 sun4i_pwm_readl(struct sun4i_pwm_chip *chip,
> >> -                               unsigned long offset)
> >> -{
> >> -     return readl(chip->base + offset);
> >> -}
> >> -
> >> -static inline void sun4i_pwm_writel(struct sun4i_pwm_chip *chip,
> >> -                                 u32 val, unsigned long offset)
> >> -{
> >> -     writel(val, chip->base + offset);
> >> -}
> >> -
> >>  static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >>                           int duty_ns, int period_ns)
> >>  {
> >> @@ -152,7 +140,11 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >>       }
> >>
> >>       spin_lock(&sun4i_pwm->ctrl_lock);
> >> -     val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> >> +     err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
> >> +     if (err) {
> >> +             dev_err(chip->dev, "failed to read from CTL register\n");
> >> +             goto err_cleanup;
> >> +     }
> >
> > I'm not sure you need those error checks. If there's an error when you
> > write to an MMIO bus, you have much more important issues than your
> > return code there.
> 
> This probably overkill for MMIO, but it helps when wrong parameters
> passed to regmap functions - mostly during debugging stage.

There's no way you can get it wrong with regmap_read / write here. And
what's useful during debugging may not be fit for real world use
case. We never had error checking before, it was working just fine,
this is just as true here.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com