Re: [PATCH v3 5/6] ARM: pinctrl: Add Broadcom Capri pinctrl driver

[Sherman Yin]

On 13-12-12 12:54 PM, Linus Walleij wrote:
> > +#define CAPRI_PIN_SHIFT(type, param) \
> > +       (CAPRI_ ## type ## _PIN_REG_ ## param ## _SHIFT)
> > +
> > +#define CAPRI_PIN_MASK(type, param) \
> > +       (CAPRI_ ## type ## _PIN_REG_ ## param ## _MASK)
> > +
> > +/* Macro to update reg with new pin config param */
> > +#define CAPRI_PIN_UPDATE(reg, type, param, val)                                \
> > +       (((reg) & ~CAPRI_PIN_MASK(type, param)) |                       \
> > +       (((val) << CAPRI_PIN_SHIFT(type, param)) & CAPRI_PIN_MASK(type, param)))
>
> Yuck! Are you sure you cannot convert these to static inlines and
> make them much simpler in the process?
>
> We do have an optimizing compiler, you don't need to do
> everything on one line... besides we're not on the hotpath.

If I were to convert the first 2 #defines to functions, it would either 
be a 2-level switch statement or a 2D lookup table.  IMO both of these 
options are more difficult to read than this simple concatenation, so I 
really rather keep them this way.

CAPRI_PIN_UPDATE, OTOH, doesn't require any concatenation so I can 
easily make that into an inline.

> > +/*
> > + * Write to the register using the value and mask if current value is different
> > + */
> > +static void capri_reg_write(struct pinctrl_dev *pctldev,
> > +                           void __iomem *reg,
> > +                           u32 val,
> > +                           u32 mask)
> > +{
> > +       u32 old_val;
> > +       u32 new_val;
> > +
> > +       old_val = readl(reg);
> > +       new_val = (old_val & ~mask) | (val & mask);
> > +
> > +       if (new_val == old_val) {
> > +               dev_dbg(pctldev->dev,
> > +                       "Reg 0x%p=0x%x (no change)\n",
> > +                       reg, old_val);
> > +               return;
> > +       }
> > +
> > +       dev_dbg(pctldev->dev,
> > +               "Reg 0x%p change from 0x%x to 0x%x\n",
> > +               reg, old_val, new_val);
> > +       writel(new_val, reg);
> > +}
>
> This is a reimplementation of regmap for MMIO.
> See drivers/base/regmap/regmap-mmio.c
> Notice how regmap_update_bits() is used throughout the
> kernel.
>
> If you want to do this, use regmap.

Ok.

> > +               case PIN_CONFIG_DRIVE_STRENGTH:
> > +                       /* Valid range is 2-16 mA, even numbers only */
> > +                       if ((arg < 2) || (arg > 16) || (arg % 2)) {
> > +                               dev_err(pctldev->dev,
> > +                                       "Invalid Drive Strength value (%d) for "
> > +                                       "pin %s (%d). Valid values are "
> > +                                       "(2..16) mA, even numbers only.\n",
> > +                                       arg, pdata->pins[pin].name, pin);
> > +                               return -EINVAL;
> > +                       }
> > +                       *val = CAPRI_PIN_UPDATE(*val, STD, DRV_STR, (arg/2)-1);
> > +                       *mask |= CAPRI_STD_PIN_REG_DRV_STR_MASK;
> > +                       break;
>
> Hm rather nice integer math...

Sorry, I can't tell if you are being sarcastic :)  Drive strength is 
represented by 3 bits in the register.  If the values were 2-14 I could 
have done some bit-checking instead of those 3 conditions in the if 
statement.  Or, if we use a enum of 0-7 then the check is much easier. 
But as we discussed re: pull-up resistance, we rather let users specify 
real numbers.  The (arg/2)-1 is just to convert the mA into the 3 bits.

> > +/* Goes through the configs and update register val/mask */
> > +static int capri_i2c_pin_update(struct pinctrl_dev *pctldev,
> > +                               unsigned pin,
> > +                               unsigned long *configs,
> > +                               unsigned num_configs,
> > +                               u32 *val,
> > +                               u32 *mask)
> > +{
> > +       struct capri_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
> > +       int i;
> > +       enum pin_config_param param;
> > +       u16 arg;
> > +
> > +       for (i = 0; i < num_configs; i++) {
> > +               param = pinconf_to_config_param(configs[i]);
> > +               arg = pinconf_to_config_argument(configs[i]);
> > +
> > +               switch (param) {
> > +               case PIN_CONFIG_BIAS_PULL_UP:
> > +                       if ((arg < 1) || (arg > 7)) {
> > +                               dev_err(pctldev->dev,
> > +                                       "Invalid Pull Up value (%d) for pin %s "
> > +                                       "(%d). Valid values are (1..7).\n",
> > +                                       arg, pdata->pins[pin].name, pin);
> > +                               return -EINVAL;
> > +                       }
>
> No don't do that as mentioned in the other patch. Pass pull up strength
> in Ohms.
>
> Then have a translation table here, and do some best-effort fuzzy match.

Sure.  I'm just going to error out if the user-supplied value is off, 
since it is very clear what is acceptable from the binding documentation.

> > +       /* Different pins have different configuration options */
> > +       switch (pin_type) {
> > +       case CAPRI_PIN_TYPE_STD:
> > +               rc = capri_std_pin_update(pctldev, pin, configs, num_configs,
> > +                       &cfg_val, &cfg_mask);
> > +               break;
> > +
> > +       case CAPRI_PIN_TYPE_I2C:
> > +               rc = capri_i2c_pin_update(pctldev, pin, configs, num_configs,
> > +                       &cfg_val, &cfg_mask);
> > +               break;
> > +
> > +       case CAPRI_PIN_TYPE_HDMI:
> > +               rc = capri_hdmi_pin_update(pctldev, pin, configs, num_configs,
> > +                       &cfg_val, &cfg_mask);
> > +               break;
>
> This is really nice and elegant.

Thanks. :)

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