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: <20200629163851.2130450-1-pmalani@chromium.org> <CABXOdTe-hY98YFusFkS+C8iVWZANVWcavpKSmaYtjC11yMgFrg@mail.gmail.com>
In-Reply-To: <CABXOdTe-hY98YFusFkS+C8iVWZANVWcavpKSmaYtjC11yMgFrg@mail.gmail.com>
From:   Prashant Malani <pmalani@chromium.org>
Date:   Mon, 29 Jun 2020 14:08:31 -0700
Message-ID: <CACeCKaeqw7Lj7z3DYDvR0j6x+ud3BcxVgxb9-sQ3ZS0ks_f7gg@mail.gmail.com>
Subject: Re: [PATCH v3 1/2] platform/chrome: cros_ec_typec: Use workqueue for
 port update
To:     Guenter Roeck <groeck@google.com>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Benson Leung <bleung@chromium.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        Guenter Roeck <groeck@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Guenter,

Thanks for the comments.

On Mon, Jun 29, 2020 at 2:05 PM Guenter Roeck <groeck@google.com> wrote:
>
> On Mon, Jun 29, 2020 at 9:38 AM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > Use a work queue to call the port update routines, instead of doing it
> > directly in the PD notifier callback. This will prevent other drivers
> > with PD notifier callbacks from being blocked on the port update routine
> > completing.
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes in v3:
> > - Use new 100 character line length limit.
> >
> > Changes in v2:
> > - No changes.
> >
> >  drivers/platform/chrome/cros_ec_typec.c | 25 ++++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index 0c041b79cbba..0beb62bf5adf 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -58,6 +58,7 @@ struct cros_typec_data {
> >         /* Array of ports, indexed by port number. */
> >         struct cros_typec_port *ports[EC_USB_PD_MAX_PORTS];
> >         struct notifier_block nb;
> > +       struct work_struct port_work;
> >  };
> >
> >  static int cros_typec_parse_port_props(struct typec_capability *cap,
> > @@ -619,18 +620,26 @@ static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
> >         return 0;
> >  }
> >
> > -static int cros_ec_typec_event(struct notifier_block *nb,
> > -                              unsigned long host_event, void *_notify)
> > +static void cros_typec_port_work(struct work_struct *work)
> >  {
> > -       struct cros_typec_data *typec = container_of(nb, struct cros_typec_data,
> > -                                                    nb);
> > -       int ret, i;
> > +       struct cros_typec_data *typec = container_of(work, struct cros_typec_data, port_work);
> > +       int ret;
> > +       int i;
> >
>
> I know I am getting picky here, but this seems like a personal
> preference change. There is no "one variable declaration per line"
> coding style rule.

Done.
>
> >         for (i = 0; i < typec->num_ports; i++) {
> >                 ret = cros_typec_port_update(typec, i);
> >                 if (ret < 0)
> >                         dev_warn(typec->dev, "Update failed for port: %d\n", i);
> >         }
> > +}
> > +
> > +
>
> ... but anyway, there should be no double empty lines.
>

Done.
> > +static int cros_ec_typec_event(struct notifier_block *nb,
> > +                              unsigned long host_event, void *_notify)
> > +{
> > +       struct cros_typec_data *typec = container_of(nb, struct cros_typec_data, nb);
> > +
> > +       schedule_work(&typec->port_work);
> >
> >         return NOTIFY_OK;
> >  }
> > @@ -689,6 +698,12 @@ static int cros_typec_probe(struct platform_device *pdev)
> >         if (ret < 0)
> >                 return ret;
> >
> > +       INIT_WORK(&typec->port_work, cros_typec_port_work);
> > +
> > +       /*
> > +        * Safe to call port update here, since we haven't registered the
> > +        * PD notifier yet.
> > +        */
> >         for (i = 0; i < typec->num_ports; i++) {
> >                 ret = cros_typec_port_update(typec, i);
> >                 if (ret < 0)
> > --
> > 2.27.0.212.ge8ba1cc988-goog
> >