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

[Dmitry Baryshkov]

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).

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

-- 
With best wishes
Dmitry