Re: [PATCH 1/2] pinctrl: sh-pfc: Add drive strength support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Geert,

On Wednesday 23 Mar 2016 10:54:02 Geert Uytterhoeven wrote:
> On Mon, Mar 21, 2016 at 12:33 AM, Laurent Pinchart wrote:
> > Add support for the drive-strengh pin configuration using the generic
> > pinconf DT bindings.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> Thanks for your patch!
> 
> > diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
> > index 181ea98a63b7..73f0b33ee0a1 100644
> > --- a/drivers/pinctrl/sh-pfc/core.c
> > +++ b/drivers/pinctrl/sh-pfc/core.c
> > @@ -173,6 +173,21 @@ void sh_pfc_write_raw_reg(void __iomem *mapped_reg,
> > unsigned int reg_width,
> >         BUG();
> >  }
> > 
> > +u32 sh_pfc_read_reg(struct sh_pfc *pfc, u32 reg, unsigned int width)
> > +{
> > +       return sh_pfc_read_raw_reg(sh_pfc_phys_to_virt(pfc, reg), width);
> > +}
> > +
> > +void sh_pfc_write_reg(struct sh_pfc *pfc, u32 reg, unsigned int width,
> > u32 data)
> > +{
> > +       if (pfc->info->unlock_reg)
> > +               sh_pfc_write_raw_reg(
> > +                       sh_pfc_phys_to_virt(pfc, pfc->info->unlock_reg),
> > 32,
> > +                       ~data);
> > +
> > +       sh_pfc_write_raw_reg(sh_pfc_phys_to_virt(pfc, reg), width, data);
> > +}
> 
> I like these helpers. They may also be used by r8a7778_pinmux_[gs]et_bias()
> and r8a7790_[gs]et_io_voltage().
> 
> However, writing to the pull-up registers on r8a7778 doesn't seem to require
> writing to the unlock register first.
> 
> Should we prepare for that and add a bool parameter to sh_pfc_write_reg()?

I've thought about it, but given that we don't have an immediate issue with 
the r8a7778 implementation, I decided it would be better to wait until we have 
a couple more users for that API to decide how to implement it best.

> > --- a/drivers/pinctrl/sh-pfc/pinctrl.c
> > +++ b/drivers/pinctrl/sh-pfc/pinctrl.c
> > 
> > +static int sh_pfc_pinconf_get_drive_strength(struct sh_pfc *pfc,
> > +                                            unsigned int pin)
> > +{
> > +       unsigned long flags;
> > +       unsigned int offset;
> > +       unsigned int size;
> > +       u32 reg;
> > +       u32 val;
> > +
> > +       reg = sh_pfc_pinconf_find_drive_strength_reg(pfc, pin, &offset,
> > &size); +       if (!reg)
> > +               return -EINVAL;
> > +
> > +       spin_lock_irqsave(&pfc->lock, flags);
> > +       val = sh_pfc_read_reg(pfc, reg, 32);
> > +       spin_unlock_irqrestore(&pfc->lock, flags);
> > +
> > +       val = (val >> offset) & GENMASK(size - 1, 0);
> > +
> > +       /* Convert the value to mA based on a full drive strength value of
> > 24mA. +        * We can make the full value configurable later if needed.
> > +        */
> > +       if (size == 2)
> > +               val <<= 1;
> 
> I would write "val *= 2" here, as you're doing arithmetic, and have a "* 3"
> below anyway.

It's actually not really arithmetics, but I realize the code doesn't work 
correctly as a 2-bits drive strength set to 3 will be converted to 21 instead 
of 34. I'll fix it and resubmit.

> > +       return (val + 1) * 3;
> > +}
> > +
> > +static int sh_pfc_pinconf_set_drive_strength(struct sh_pfc *pfc,
> > +                                            unsigned int pin, u16
> > strength)
> > +{
> > +       unsigned long flags;
> > +       unsigned int offset;
> > +       unsigned int size;
> > +       u32 reg;
> > +       u32 val;
> > +
> > +       if (strength < 3 || strength > 24)
> > +               return -EINVAL;
> > +
> > +       reg = sh_pfc_pinconf_find_drive_strength_reg(pfc, pin, &offset,
> > &size); +       if (!reg)
> > +               return -EINVAL;
> > +
> > +       /* Convert the value from mA based on a full drive strength value
> > of
> > +        * 24mA. We can make the full value configurable later if needed.
> > +        */
> > +       strength = strength / 3 - 1;
> > +       if (size == 2)
> > +               strength >>= 1;
> 
> Same here: "strength / = 2".
> 
> Apart from the above, everything else looks fine to me, thx!
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

-- 
Regards,

Laurent Pinchart