Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220126012203.2979709-1-pmalani@chromium.org>
 <YfDEHoYkLc6zjSxj@google.com> <CACeCKafqQmb7jjzweaRq2ETBbPk_2HE8FbFLMdfcCD8PrdckoQ@mail.gmail.com>
In-Reply-To: <CACeCKafqQmb7jjzweaRq2ETBbPk_2HE8FbFLMdfcCD8PrdckoQ@mail.gmail.com>
From:   Guenter Roeck <groeck@google.com>
Date:   Wed, 26 Jan 2022 07:33:38 -0800
Message-ID: <CABXOdTemE9TOhOXy27beoEvj_6eTcwUj_A6=DMhEmMgCsqXRnQ@mail.gmail.com>
Subject: Re: [PATCH] platform/chrome: cros_ec_typec: Check for EC device
To:     Prashant Malani <pmalani@chromium.org>
Cc:     Tzung-Bi Shih <tzungbi@google.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Alyssa Ross <hi@alyssa.is>, Benson Leung <bleung@chromium.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        Guenter Roeck <groeck@chromium.org>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jan 25, 2022 at 8:05 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Tzung-Bi,
>
> Thanks for your review.
>
> On Tue, Jan 25, 2022 at 7:46 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> > On Wed, Jan 26, 2022 at 01:22:03AM +0000, Prashant Malani wrote:
> > > Fixes: fdc6b21e2444 ("platform/chrome: Add Type C connector class driver")
> > > Reported-by: Alyssa Ross <hi@alyssa.is>
> > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> >
> > With a minor comment,
> > Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
> >
> > > @@ -1076,6 +1076,12 @@ static int cros_typec_probe(struct platform_device *pdev)
> > >
> > >       typec->dev = dev;
> > >       typec->ec = dev_get_drvdata(pdev->dev.parent);
> > > +
> >
> > I would prefer to remove the blank line to make it look like an integrated block.
>
> I actually prefer it as it is. typec->dev is not really part of this
> "integrated block", and I don't want to add another space there.

But on the other side the check is part of the "integrated block".
Maybe add an empty line between the two assignments if you want a
separation.

> In any case, since this is a very minor style nit, I will address it
> in case there is another version required due to other comments.
>
> >
> > > +     if (!typec->ec) {
> > > +             dev_err(dev, "couldn't find parent EC device\n");
> > > +             return -ENODEV;
> > > +     }
> > > +
>
> Best,
>
> -Prashant