Re: [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver

[dmitry.torokhov@gmail.com]

Hi,

On Mon, Oct 19, 2020 at 05:03:15PM +0800, Li Jun wrote:
> +
> +static int typec_switch_simple_set(struct typec_switch *sw,
> +				   enum typec_orientation orientation)
> +{
> +	struct typec_switch_simple *typec_sw = typec_switch_get_drvdata(sw);
> +
> +	mutex_lock(&typec_sw->lock);

Why is this lock needed? It looks like we are passing requests directly
to gpiochip which I expect would take care of this.

> +
> +	switch (orientation) {
> +	case TYPEC_ORIENTATION_NORMAL:
> +		if (typec_sw->sel_gpio)
> +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 1);
> +		break;
> +	case TYPEC_ORIENTATION_REVERSE:
> +		if (typec_sw->sel_gpio)
> +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 0);
> +		break;
> +	case TYPEC_ORIENTATION_NONE:
> +		break;
> +	}
> +
> +	mutex_unlock(&typec_sw->lock);
> +
> +	return 0;
> +}
> +
> +static int typec_switch_simple_probe(struct platform_device *pdev)
> +{
> +	struct typec_switch_simple	*typec_sw;
> +	struct device			*dev = &pdev->dev;
> +	struct typec_switch_desc sw_desc;
> +
> +	typec_sw = devm_kzalloc(dev, sizeof(*typec_sw), GFP_KERNEL);
> +	if (!typec_sw)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, typec_sw);
> +
> +	sw_desc.drvdata = typec_sw;
> +	sw_desc.fwnode = dev->fwnode;
> +	sw_desc.set = typec_switch_simple_set;
> +	mutex_init(&typec_sw->lock);
> +
> +	/* Get the super speed active channel selection GPIO */
> +	typec_sw->sel_gpio = devm_gpiod_get_optional(dev, "switch",
> +						     GPIOD_OUT_LOW);

Does this driver make sense without the GPIO? Should it be made
mandatory?

Thanks.

-- 
Dmitry