linux-kernel - Re: [PATCH v3 4/6] reset: Add Delta TN48M CPLD reset controller

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 2 Jun 2021 13:48:23 +0200
From:   Robert Marko <robert.marko@...tura.hr>
To:     Philipp Zabel <p.zabel@...gutronix.de>
Cc:     Lee Jones <lee.jones@...aro.org>, linux-kernel@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        robh+dt@...nel.org, devicetree@...r.kernel.org,
        Luka Perkov <luka.perkov@...tura.hr>, jmp@...phyte.org,
        Paul Menzel <pmenzel@...gen.mpg.de>,
        Donald Buczek <buczek@...gen.mpg.de>
Subject: Re: [PATCH v3 4/6] reset: Add Delta TN48M CPLD reset controller

On Wed, Jun 2, 2021 at 10:47 AM Philipp Zabel <p.zabel@...gutronix.de> wrote:
>
> On Tue, 2021-06-01 at 19:09 +0200, Robert Marko wrote:
> [...]
> > Yes, it's self-clearing, per spec they will be cleared after 100ms.
>
> Can you make sure the function only returns after the reset is
> deasserted again, for example by using regmap_read_poll_timeout() on the
> reset bit?

Yes, that is simple to implement.
>
> > Will drop assert then, I saw that reset was for self-clearing, but other
> > drivers I looked for example implemented both which was confusing.
>
> If you have full control over the reset line, you can implement .reset
> by manually asserting and deasserting (possibly after a delay). But if
> the reset is self-clearing, you can't properly implement .(de)assert,
> which have an expectation about the state of the reset line after the
> function returns.

Makes sense, I will drop the assert op.
>
> > > > +}
> > > > +
> > > > +static int tn48m_control_status(struct reset_controller_dev *rcdev,
> > > > +                             unsigned long id)
> > > > +{
> > > > +     struct tn48_reset_data *data = to_tn48_reset_data(rcdev);
> > > > +     unsigned int regval;
> > > > +     int ret;
> > > > +
> > > > +     ret = regmap_read(data->regmap, TN48M_RESET_REG, &regval);
> > > > +     if (ret < 0)
> > > > +             return ret;
> > > > +
> > > > +     if (BIT(tn48m_resets[id].bit) & regval)
> > > > +             return 0;
> > > > +     else
> > > > +             return 1;
> > > > +}
> > > > +
> > > > +static const struct reset_control_ops tn48_reset_ops = {
> > > > +     .reset          = tn48m_control_reset,
> > > > +     .assert         = tn48m_control_assert,
> > > > +     .status         = tn48m_control_status,
> > > > +};
> > > > +
> > > > +static int tn48m_reset_probe(struct platform_device *pdev)
> > > > +{
> > > > +     struct tn48_reset_data *data;
> > > > +     struct regmap *regmap;
> > > > +
> > > > +     if (!pdev->dev.parent)
> > > > +             return -ENODEV;
> > >
> > > That shouldn't be necessary.
> >
> > This driver depends on having a parent as it needs to get the
> > regmap from it.
> > The parent is a CPLD using simple-i2c-mfd.
> > So it's nice to check.
>
> pdev->dev.parent is always set to &platform_bus if there is no parent.

Ok, so it's useless to check.

Will send a new version today.

Regards,
Robert
>
> regards
> Philipp



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@...tura.hr
Web: www.sartura.hr