Re: [PATCH 7/8] platform/chrome: cros_ec_typec: Thunderbolt support

[Abhishek Pandit-Subedi]

On Wed, Sep 25, 2024 at 2:20 PM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> On Wed, Sep 25, 2024 at 11:42:46AM GMT, Abhishek Pandit-Subedi wrote:
> > On Wed, Sep 25, 2024 at 10:13 AM Dmitry Baryshkov
> > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Sep 25, 2024 at 09:25:08AM GMT, Abhishek Pandit-Subedi wrote:
> > > > Add support for entering and exiting Thunderbolt alt-mode using AP
> > > > driven alt-mode.
> > > >
> > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> > > > ---
> > > >
> > > >  drivers/platform/chrome/Makefile              |   1 +
> > > >  drivers/platform/chrome/cros_ec_typec.c       |  29 +--
> > > >  drivers/platform/chrome/cros_typec_altmode.h  |  14 ++
> > > >  .../platform/chrome/cros_typec_thunderbolt.c  | 184 ++++++++++++++++++
> > > >  4 files changed, 216 insertions(+), 12 deletions(-)
> > > >  create mode 100644 drivers/platform/chrome/cros_typec_thunderbolt.c
> > >
> > > This patch looks like nearly exact 1:1 copy of the previous one. Please
> > > merge both altmode implementations in the same way as tcpm.c does.
> >
> > It's easier for tcpm.c to have a merged implementation because it
> > simply forwards VDMs to the internal state machine to handle without
> > doing anything with them. Our implementation is closer to
> > ucsi/displayport.c which needs to maintain an internal state machine
> > for DP and TBT VDMs and respond differently.
> >
> > I can merge the two but I'd like to understand intent (reduce code
> > duplication? reduce the number of files?). As it is, keeping the files
> > separate makes it easier to understand how each alt-mode operates in
> > my opinion.
>
> Separate common code and AltMode-specific code. This way we reduce a
> risk of errors fixed in only one of two drivers and at the same time the
> driver clearly separates common vs specific code paths (e.g. VDM
> handling is mode-specific, while the rest of the code is common).

Ack -- I'll provide a merged solution in the next version of this patch series.

>
> >
> > >
> > > --
> > > With best wishes
> > > Dmitry
>
> --
> With best wishes
> Dmitry