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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 30 Oct 2018 13:59:48 +0000
From:   Charles Keepax <ckeepax@...nsource.cirrus.com>
To:     Linus Walleij <linus.walleij@...aro.org>
CC:     Lee Jones <lee.jones@...aro.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Liam Girdwood <lgirdwood@...il.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        <patches@...nsource.cirrus.com>,
        linux-clk <linux-clk@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v3 5/5] pinctrl: lochnagar: Add support for the Cirrus
 Logic Lochnagar

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@...nsource.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@...nsource.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