Hello Greg,
thanks for reviewing.
On Fri, 10 May 2024 08:55:29 +0100
Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > Signed-off-by: Herve Codina <herve.codina@xxxxxxxxxxx>
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
> >
> > ---
> >
> > 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@xxxxxxxxxxx>
> > 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
