Received-SPF: pass (google.com: domain of linux-kernel+bounces-175607-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 10 May 2024 12:54:17 +0200
From: Luca Ceresoli <luca.ceresoli@bootlin.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>,
 Conor Dooley <conor+dt@kernel.org>, Andrzej Hajda
 <andrzej.hajda@intel.com>, Neil Armstrong <neil.armstrong@linaro.org>,
 Robert Foss <rfoss@kernel.org>, Laurent Pinchart
 <Laurent.pinchart@ideasonboard.com>, Jonas Karlman <jonas@kwiboo.se>,
 Jernej Skrabec <jernej.skrabec@gmail.com>, Maarten Lankhorst
 <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>,
 Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>,
 Daniel Vetter <daniel@ffwll.ch>, Derek Kiernan <derek.kiernan@amd.com>,
 Dragan Cvetic <dragan.cvetic@amd.com>, Arnd Bergmann <arnd@arndb.de>,
 Saravana Kannan <saravanak@google.com>, Paul Kocialkowski
 <contact@paulk.fr>, =?UTF-8?Q?Herv=C3=A9?= Codina
 <herve.codina@bootlin.com>, Thomas Petazzoni
 <thomas.petazzoni@bootlin.com>, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Paul
 Kocialkowski <paul.kocialkowski@bootlin.com>
Subject: Re: [PATCH v2 5/5] misc: add ge-addon-connector driver
Message-ID: <20240510125417.1dddfd5c@booty>
In-Reply-To: <2024051039-decree-shrimp-45c6@gregkh>
References: <20240510-hotplug-drm-bridge-v2-0-ec32f2c66d56@bootlin.com>
	<20240510-hotplug-drm-bridge-v2-5-ec32f2c66d56@bootlin.com>
	<2024051039-decree-shrimp-45c6@gregkh>
Organization: Bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

Hello Greg,

thanks for reviewing.

On Fri, 10 May 2024 08:55:29 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Fri, May 10, 2024 at 09:10:41AM +0200, Luca Ceresoli wrote:
> > Add a driver to support the runtime hot-pluggable add-on connector on the
> > GE SUNH device. This connector allows connecting and disconnecting an
> > add-on to/from the main device to augment its features. Connection and
> > disconnection can happen at runtime at any moment without notice.
> > 
> > Different add-on models can be connected, and each has an EEPROM with a
> > model identifier at a fixed address.
> > 
> > The add-on hardware is added and removed using device tree overlay loading
> > and unloading.
> > 
> > Co-developed-by: Herve Codina <herve.codina@bootlin.com>
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > 
> > ---
> > 
> > This commit is new in v2.
> > ---
> >  MAINTAINERS                      |   1 +
> >  drivers/misc/Kconfig             |  15 ++
> >  drivers/misc/Makefile            |   1 +
> >  drivers/misc/ge-sunh-connector.c | 464 +++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 481 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 672c26372c92..0bdb4fc496b8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9905,6 +9905,7 @@ F:	drivers/iio/pressure/mprls0025pa*
> >  HOTPLUG CONNECTOR FOR GE SUNH ADDONS
> >  M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
> >  S:	Maintained
> > +F:	drivers/misc/ge-sunh-connector.c
> >  F:	Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
> >  
> >  HP BIOSCFG DRIVER
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 4fb291f0bf7c..99ef2eccbbaa 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -574,6 +574,21 @@ config NSM
> >  	  To compile this driver as a module, choose M here.
> >  	  The module will be called nsm.
> >  
> > +config GE_SUNH_CONNECTOR
> > +	tristate "GE SUNH hotplug add-on connector"
> > +	depends on OF
> > +	select OF_OVERLAY
> > +	select FW_LOADER
> > +	select NVMEM
> > +	select DRM_HOTPLUG_BRIDGE  
> 
> Can these be depends instead of select?  'select' causes dependencies
> that are hard, if not almost impossible, to detect at times why
> something is being enabled.

(see reply to Arnd's follow-up e-mail for this)

> > +	help
> > +	  Driver for the runtime hot-pluggable add-on connector on the GE SUNH
> > +	  device. This connector allows connecting and disconnecting an add-on
> > +	  to/from the main device to augment its features. Connection and
> > +	  disconnection can be done at runtime at any moment without
> > +	  notice. Different add-on models can be connected, and each has an EEPROM
> > +	  with a model identifier at a fixed address.  
> 
> Module name?

OK, will add.

> > +static void sunh_conn_reset(struct sunh_conn *conn, bool keep_reset)
> > +{
> > +	dev_dbg(conn->dev, "reset\n");  
> 
> ftrace is your friend.

ACK.

> > +static int sunh_conn_handle_event(struct sunh_conn *conn, bool plugged)
> > +{
> > +	int err;
> > +
> > +	if (plugged == conn->plugged)
> > +		return 0;
> > +
> > +	dev_info(conn->dev, "%s\n", plugged ? "connected" : "disconnected");  
> 
> Please remove debugging code from stuff you want to see merged.
> 
> Same for all dev_info() calls here, when drivers work properly, they are
> quiet.

While agree for other dev_info() calls, this one seems quite similar in
principle to the link up/down messages that get logged by the MII code
at [0]:

  [347229.872315] asix 1-1.3.2:1.0 enx000cf616fecb: link up, 100Mbps,
  full-duplex, lpa 0xC5E1 [347229.920449] asix 1-1.3.2:1.0 enx000cf616fecb: link down

In my case it is logging that a removable part of the hardware has been
added or removed, which appears useful. Do you think it make sense in
this scenario?

Luca

[0] https://elixir.bootlin.com/linux/v6.8.9/source/drivers/net/mii.c#L557

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com