Re: [PATCH 4/4] usb: roles: intel_xhci: Always allow user control

[Heikki Krogerus]

On Tue, May 01, 2018 at 11:57:55AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 30-04-18 14:41, Heikki Krogerus wrote:
> > Trying to determine the USB port type with this mux is very
> > difficult. To simplify the situation, always allowing user
> > control, even if the port is USB Type-C port.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > ---
> >   drivers/usb/roles/intel-xhci-usb-role-switch.c | 15 +--------------
> >   1 file changed, 1 insertion(+), 14 deletions(-)
> > 
> > diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > index 28102127b9d5..6482eee6fd45 100644
> > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > @@ -43,15 +43,6 @@ struct intel_xhci_acpi_match {
> >   	int hrv;
> >   };
> > -/*
> > - * ACPI IDs for PMICs which do not support separate data and power role
> > - * detection (USB ACA detection for micro USB OTG), we allow userspace to
> > - * change the role manually on these.
> > - */
> > -static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = {
> > -	{ "INT33F4",  3 }, /* X-Powers AXP288 PMIC */
> > -};
> > -
> >   static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> >   {
> >   	struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
> > @@ -137,7 +128,6 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
> >   	struct device *dev = &pdev->dev;
> >   	struct intel_xhci_usb_data *data;
> >   	struct resource *res;
> > -	int i;
> >   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >   	if (!data)
> > @@ -150,10 +140,7 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
> >   	if (!data->base)
> >   		return -ENOMEM;
> > -	for (i = 0; i < ARRAY_SIZE(allow_userspace_ctrl_ids); i++)
> > -		if (acpi_dev_present(allow_userspace_ctrl_ids[i].hid, "1",
> > -				     allow_userspace_ctrl_ids[i].hrv))
> > -			sw_desc.allow_userspace_control = true;
> > +	sw_desc.allow_userspace_control = true;
> 
> It would be better to add:
> 
> 	.allow_userspace_control = true,
> 
> To the initialization of the struct and also make the struct const since
> we are now no longer changing it and usb_role_switch_register() accepts
> it being const.

Makes sense.

> With that changed:
> 
> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>

Thanks,

-- 
heikki