Re: [PATCH v3 5/5] pinctrl: lochnagar: Add support for the Cirrus Logic Lochnagar

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Tue, Oct 30, 2018 at 02:04:20PM +0100, Linus Walleij wrote:
> On Fri, Oct 19, 2018 at 11:50 AM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> 
> > Lochnagar is an evaluation and development board for Cirrus
> > Logic Smart CODEC and Amp devices. It allows the connection of
> > most Cirrus Logic devices on mini-cards, as well as allowing
> > connection of various application processor systems to provide a
> > full evaluation platform. This driver supports the board
> > controller chip on the Lochnagar board.
> >
> > Lochnagar provides many pins which can generally be used for an
> > audio function such as an AIF or a PDM interface, but also as
> > GPIOs.
> >
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> > +static void lochnagar_gpio_set(struct gpio_chip *chip,
> > +                              unsigned int offset, int value)
> > +{
> > +       struct lochnagar_pin_priv *priv = gpiochip_get_data(chip);
> > +       struct lochnagar *lochnagar = priv->lochnagar;
> > +       const struct lochnagar_pin *pin = priv->pins[offset].drv_data;
> > +       int ret;
> > +
> > +       value = !!value;
> 
> value = value ? BIT(pin->shift) : 0;
> 

I think this will end up more complex because...

> > +       dev_dbg(priv->dev, "Set GPIO %s to %s\n",
> > +               pin->name, value ? "high" : "low");
> > +
> > +       switch (pin->type) {
> > +       case LN_PTYPE_MUX:
> > +               value |= LN2_OP_GPIO;

I want the value to be in the lowest bit for this path.

> > +
> > +               ret = lochnagar_pin_set_mux(priv, pin, value);
> > +               break;
> > +       case LN_PTYPE_GPIO:
> > +               if (pin->invert)
> > +                       value = !value;

And then I need to invert the value here.

> > +
> > +               ret = regmap_update_bits(lochnagar->regmap, pin->reg,
> > +                                        0x1 << pin->shift,
> 
> BIT(pin->shift)
> 
> > +                                        value << pin->shift);
> 
> Just value provided you used the construction above
> to construct it.
> 

All the other comments look good will fixup for the next spin.

Thanks,
Charles