Re: [PATCH 2/2] spi: spidev: Fix OF tree warning logic

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Trent,

On Wed, Sep 19, 2018 at 11:34 PM Trent Piepho <tpiepho@xxxxxxxxxx> wrote:
> On Wed, 2018-09-19 at 23:17 +0200, Geert Uytterhoeven wrote:
> > On Wed, Sep 19, 2018 at 10:54 PM Trent Piepho <tpiepho@xxxxxxxxxx> wrote:
> > > On Wed, 2018-09-19 at 10:39 -0700, Mark Brown wrote:
> > > > On Wed, Sep 19, 2018 at 08:23:09AM +0200, Geert Uytterhoeven wrote:
> > > > > On Wed, Sep 19, 2018 at 12:50 AM Trent Piepho <tpiepho@xxxxxxxxxx> wrote:
> > > > > > +       if (spi->dev.of_node &&
> > > > > > +           of_device_is_compatible(spi->dev.of_node, "spidev")) {
> > > > > >                 dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n");
> > > > > >                 WARN_ON(spi->dev.of_node &&
> > > > > > -                       !of_match_device(spidev_dt_ids, &spi->dev));
> > > > > > +                       of_device_is_compatible(spi->dev.of_node, "spidev"));
> > > > >
> > > > > To avoid having the same conditional twice, perhaps the dev_err() and WARN_ON()
> > > > > should just be replaced by
> > > > >      WARN(1, "%pOF: buggy DT: spidev listed directly in DT\n",
> > > > > spi->dev.of_node);
> > > >
> > > > Yes, that'd be neater.
> > >
> > > How about:
> > >
> > > -       if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) {
> > > -               dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n");
> > > -               WARN_ON(spi->dev.of_node &&
> > > -                       !of_match_device(spidev_dt_ids, &spi->dev));
> > > -       }
> > > +       WARN_ON(spi->dev.of_node &&
> > > +               of_device_is_compatible(spi->dev.of_node, "spidev"),
> > > +               "%pOF: buggy DT: spidev listed directly in DT\n", spi->dev.of_node);
> >
> > s/WARN_ON/WARN/?
> >
> > The former doesn't take a message, the latter does.
>
> Yes, that's what I meant.  If the dev_err is to be removed, no reason
> to use WARN(1, ) instead of putting the condition in the WARN.

Thanks, looks fine to me!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds