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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: dmitry.torokhov@xxxxxxxxx <dmitry.torokhov@xxxxxxxxx>
> Sent: Thursday, October 22, 2020 3:56 AM
> To: Jun Li <jun.li@xxxxxxx>
> Cc: heikki.krogerus@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> rafael@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> andriy.shevchenko@xxxxxxxxxxxxxxx; hdegoede@xxxxxxxxxx;
> lee.jones@xxxxxxxxxx; mika.westerberg@xxxxxxxxxxxxxxx;
> prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx;
> laurent.pinchart+renesas@xxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Peter Chen
> <peter.chen@xxxxxxx>
> Subject: Re: [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver
> 
> 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.

Checked some gpiochips, looks like with only GPIO, yes, this lock is not required,
I will remove it.

> 
> > +
> > +	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;
> > +q
> > +	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?

My old version made it to be mandatory, I change it to be optional in this version
for possible extend to other simple typec switch design which do not use GPIO as
this is going to be a generic typec switch driver. yes, for current implementation,
this driver will only register a typec switch in sys if without GPIO provided.

Li Jun

> 
> Thanks.
> 
> --
> Dmitry




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux