Re: [PATCH v2 1/3] usb: typec: Add helper to check cable altmode support

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

Hi Andrei,

> +bool typec_cable_altmode_unsupported(struct typec_altmode *alt)
> +{
> +	struct typec_altmode *plug;
> +	struct typec_cable *cable;
> +	bool unsupported = false;
> +
> +	/*
> +	 * Check if the cable has an e-marker, supports modal operation, and the
> +	 * SOP' altmode nodes are created. If yes, then altmode is supported.
> +	 */
> +	plug = typec_altmode_get_plug(alt, TYPEC_PLUG_SOP_P);
> +	if (plug) {
> +		typec_altmode_put_plug(plug);
> +		return false;
> +	}
> +
> +	/*
> +	 * Check if the cable is registered and its identity is specified.
> +	 * If not, the cable altmode restriction cannot be checked.
> +	 */
> +	cable = typec_cable_get(typec_altmode2port(alt));
> +	if (cable && cable->identity) {
> +		const u32 id_header = cable->identity->id_header;
> +		const u32 speed = VDO_TYPEC_CABLE_SPEED(cable->identity->vdo[0]);
> +
> +		/*
> +		 * A cable lacking an ID Header indicates a non-e-marked cable,
> +		 * can only be guaranteed to have a USB 2.0 data path (D+ and D-).
> +		 */
> +		if (!id_header) {
> +			unsupported = true;
> +		} else {
> +			switch (PD_IDH_PTYPE(id_header)) {
> +			/*
> +			 * If the speed field explicitly declares it is a
> +			 * USB 2.0-only cable, altmode is unsupported.
> +			 */
> +			case IDH_PTYPE_PCABLE:
> +				unsupported = (speed == CABLE_USB2_ONLY);
> +				break;
> +			/*
> +			 * Active cables must establish an SOP' communication
> +			 * node. Since that check failed at the beginning of
> +			 * this function, this active cable does not support
> +			 * this specific altmode.
> +			 */
> +			case IDH_PTYPE_ACABLE:
> +				unsupported = true;
> +				break;
> +			}
> +		}
> +	}
> +	if (cable)
> +		typec_cable_put(cable);
> +
> +	return unsupported;

So if typec_cable_get() doesn't return a cable, this function will now
always return false - i.e. the cable is supported? Is that intentional?

This would probable be much more clear if you checked the cable only
ones, right after you take the handle.

	cable = typec_cable_get(typec_altmode2port(alt));
        if (!cable)
                return true; /* or false? */
        ...
        /* Now unconditionally */
	typec_cable_put(cable);

I think this would be even more clear if the function was called
typec_cable_altmode_supported() and you would then have a wrapper:

static inline bool typec_cable_altmode_unsupported(struct typec_altmode *alt)
{
        return !typec_cable_altmode_supported(alt);
}

Thanks,

-- 
heikki