Re: [PATCH v2 1/3] usb: typec: bm92txx: add Rohm BM92TXX support

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi Alexandre,

On Mon, May 11, 2026 at 01:32:09AM +0700, Alexandre Hamamdjian via B4 Relay wrote:
> From: CTCaer <ctcaer@gmail.com>
> 
> Add a driver for the Rohm Semiconductor BM92TXX family of USB Type-C
> and Power Delivery controllers. The IC integrates an MCU that runs the
> PD state machine; the host configures it and observes status over I2C
> and reacts to a level-triggered ALERT interrupt.
> 
> The driver exposes the controller through extcon and a USB role switch,
> manages the VBUS sink, optional VBUS source and battery-charger
> regulators along with the VCONN-enable GPIO, and applies per-PDO
> charging current limits (5 V, 9 V, 12 V, 15 V) sourced from device
> tree. DisplayPort alternate-mode handling and dock LED behaviour are
> configurable through rohm,* properties so the same driver can serve
> boards that wire the part up differently. A debugfs interface under
> bm92txx/ is provided for register dumps and low-level command access
> when CONFIG_DEBUG_FS is enabled.

You need to use the USB Type-C framework for everything, not just for
the switches. The port, the partner when connected, the cable and
plugs, and all the alternate modes (the port alt modes, the plug
alt modes, and partner alt modes) need to be registered.

All that needs to be exposed properly inside kernel as well in user
space.

<snip>

> +/* VDM/VDO */
> +#define VDM_CMD_RESERVED    0x00
> +#define VDM_CMD_DISC_ID     0x01
> +#define VDM_CMD_DISC_SVID   0x02
> +#define VDM_CMD_DISC_MODE   0x03
> +#define VDM_CMD_ENTER_MODE  0x04
> +#define VDM_CMD_EXIT_MODE   0x05
> +#define VDM_CMD_ATTENTION   0x06
> +#define VDM_CMD_DP_STATUS   0x10
> +#define VDM_CMD_DP_CONFIG   0x11

Already defined in include/linux/usb/pd_vdo.h

> +#define VDM_ACK   0x40
> +#define VDM_NAK   0x80
> +#define VDM_BUSY  0xC0
> +#define VDM_UNSTRUCTURED   0x00
> +#define VDM_STRUCTURED     0x80

Ditto.

> +/* VDM Discover ID */
> +#define VDO_ID_TYPE_NONE        0
> +#define VDO_ID_TYPE_PD_HUB      1
> +#define VDO_ID_TYPE_PD_PERIPH   2
> +#define VDO_ID_TYPE_PASS_CBL    3
> +#define VDO_ID_TYPE_ACTI_CBL    4
> +#define VDO_ID_TYPE_ALTERNATE   5
> +
> +/* VDM Discover Mode Caps [From device (UFP_U) to host (DFP_U)] */
> +#define VDO_DP_UFP_D       BIT(0) /* DisplayPort Sink */
> +#define VDO_DP_DFP_D       BIT(1) /* DisplayPort Source */
> +#define VDO_DP_SUPPORT     BIT(2)
> +#define VDO_DP_RECEPTACLE  BIT(6)

include/linux/usb/typec_dp.h

> +/* VDM DP Configuration [From host (DFP_U) to device (UFP_U)] */
> +#define VDO_DP_U_DFP_D     BIT(0) /* UFP_U as DisplayPort Source */
> +#define VDO_DP_U_UFP_D     BIT(1) /* UFP_U as DisplayPort Sink */
> +#define VDO_DP_SUPPORT     BIT(2)
> +#define VDO_DP_RECEPTACLE  BIT(6)

Ditto.

> +/* VDM Mode Caps and DP Configuration pins */
> +#define VDO_DP_PIN_A   BIT(0)
> +#define VDO_DP_PIN_B   BIT(1)
> +#define VDO_DP_PIN_C   BIT(2)
> +#define VDO_DP_PIN_D   BIT(3)
> +#define VDO_DP_PIN_E   BIT(4)
> +#define VDO_DP_PIN_F   BIT(5)

Ditto.

> +/* Known VID/SVID */
> +#define VID_NINTENDO      0x057E
> +#define PID_NIN_DOCK      0x2003
> +#define PID_NIN_CHARGER   0x2004
> +
> +#define SVID_NINTENDO     VID_NINTENDO
> +#define SVID_DP           0xFF01
> 
> +/* Nintendo dock VDM Commands */
> +#define VDM_NCMD_LED_CONTROL         0x01 /* Reply size 12 */
> +#define VDM_NCMD_DEVICE_STATE        0x16 /* Reply size 12 */
> +#define VDM_NCMD_DP_SIGNAL_DISABLE   0x1C /* Reply size 8 */
> +#define VDM_NCMD_HUB_RESET           0x1E /* Reply size 8 */
> +#define VDM_NCMD_HUB_CONTROL         0x20 /* Reply size 8 */

You need a dedicated alternate mode driver for this mode.

It looks like you have a lot of duplication in this code. You really
have to refactor this whole driver. It probable does not make sense to
review this any further before that.

Please register all the Type-C (inclide/linux/usb/typec.h) and power
delivery (include/linux/usb/pd.h) components, and at least try to
handle the alternate mode VDM communication in the alt mode drivers
dedicated for each alt mode.

I also really think that that the battery charging information needs
to be exposed to user space with the power supply device class
(include/linux/power_supply.h).

Because there is a lot of stuff to be done here, please consider
splitting this into clear steps. For example, you could start by
simply registering the port and the partner, and so on.

thanks,

-- 
heikki