Re: [PATCH v3 4/4] USB: OMAP1: Tahvo USB transceiver driver
Hi,
On Mon, Jul 08, 2013 at 10:44:02PM +0300, Aaro Koskinen wrote:
> > > +static void tahvo_usb_become_host(struct tahvo_usb *tu)
> > > +{
> > > + struct retu_dev *rdev = dev_get_drvdata(tu->pt_dev->dev.parent);
> > > +
> > > + extcon_set_cable_state(&tu->extcon, "USB-HOST", true);
> > > +
> > > + /* Power up the transceiver in USB host mode */
> > > + retu_write(rdev, TAHVO_REG_USBR, USBR_REGOUT | USBR_NSUSPEND |
> > > +USBR_MASTER_SW2 | USBR_MASTER_SW1);
> > > + tu->phy.state = OTG_STATE_A_IDLE;
> > > +
> > > + check_vbus_state(tu);
> > > +}
> > > +
> > > +static void tahvo_usb_stop_host(struct tahvo_usb *tu)
> > > +{
> > > + tu->phy.state = OTG_STATE_A_IDLE;
> >
> > shouldn't you undo tahvo_usb_become_host() here ? I mean, set the
> > transceiver to SLAVE ?
>
> tahvo_usb_become_peripheral() (or power_off) is always called after this
> function. Actually, these stop functions can be eliminated altogether
> as they are only called from a single place...
alright, I guess GCC is already inlining them anyway, your choice.
> > > +static int tahvo_usb_set_host(struct usb_otg *otg, struct usb_bus *host)
> > > +{
> > > + struct tahvo_usb *tu = container_of(otg->phy, struct tahvo_usb, phy);
> > > +
> > > + dev_dbg(&tu->pt_dev->dev, "%s %p\n", __func__, host);
> > > +
> > > + if (otg == NULL)
> > > + return -ENODEV;
> > > +
> > > + mutex_lock(&tu->serialize);
> > > +
> > > + if (host == NULL) {
> > > + if (tu->tahvo_mode == TAHVO_MODE_HOST)
> > > + tahvo_usb_power_off(tu);
> > > + otg->host = NULL;
> > > + mutex_unlock(&tu->serialize);
> > > + return 0;
> > > + }
> > > +
> > > + if (tu->tahvo_mode == TAHVO_MODE_HOST) {
> > > + otg->host = NULL;
> > > + tahvo_usb_become_host(tu);
> > > + }
> > > +
> > > + otg->host = host;
> > > +
> > > + mutex_unlock(&tu->serialize);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int tahvo_usb_set_peripheral(struct usb_otg *otg,
> > > + struct usb_gadget *gadget)
> >
> > I want to get rid of the extra 'gadget' and 'host' parameters on
> > ->set_host() and ->set_peripheral(). Nobody really uses them other than:
> >
> > my_phy->phy.otg->{gadget,host} = {gadget,host};
> >
> > For that, I need all other PHYs to stop relying on those parameters and
> > I'll cook patches for that for v3.12 merge window.
>
> How will the PHY know if there is a host/gadget driver present? I guess
> I will need to wait for those patches...
It won't. We're assuming that if the link asks to become a host, it
should already know that there is a host side available :-)
> > > +static int tahvo_usb_probe(struct platform_device *pdev)
> > > +{
> > > + struct retu_dev *rdev = dev_get_drvdata(pdev->dev.parent);
> > > + struct tahvo_usb *tu;
> > > + int ret;
> > > +
> > > + tu = devm_kzalloc(&pdev->dev, sizeof(*tu), GFP_KERNEL);
> > > + if (!tu)
> > > + return -ENOMEM;
> > > +
> > > + tu->phy.otg = devm_kzalloc(&pdev->dev, sizeof(*tu->phy.otg),
> > > +GFP_KERNEL);
> > > + if (!tu->phy.otg)
> > > + return -ENOMEM;
> > > +
> > > + tu->pt_dev = pdev;
> > > +
> > > + /* Default mode */
> > > +#ifdef CONFIG_TAHVO_USB_HOST_BY_DEFAULT
> > > + tu->tahvo_mode = TAHVO_MODE_HOST;
> > > +#else
> > > + tu->tahvo_mode = TAHVO_MODE_PERIPHERAL;
> > > +#endif
> >
> > should you call become_host()/become_peripheral() here ?
>
> Not needed. Once the PHY is registered, the framework will call set_host /
> set_peripheral and the HW gets set to correct state.
ok good, thanks
> > /Could also use devm_request_threaded_irq()
>
> Does not really help much, since the IRQ has to be freed manually anyway
> (to ensure it's disabled before usb_remove_phy(); also looks like it's
> currently enabled too early...).
you miss a free() in the error path. Switching to
devm_request_threaded_irq() would save you the trouble of having to call
that explicitly.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v3 4/4] USB: OMAP1: Tahvo USB transceiver driver
Hi,
On Mon, Jul 08, 2013 at 10:21:09AM +0300, Felipe Balbi wrote:
> On Tue, Jun 18, 2013 at 08:07:01PM +0300, Aaro Koskinen wrote:
> > +static ssize_t vbus_state_show(struct device *device,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct tahvo_usb *tu = dev_get_drvdata(device);
> > + return sprintf(buf, "%d\n", tu->vbus_state);
> > +}
> > +static DEVICE_ATTR(vbus_state, 0444, vbus_state_show, NULL);
>
> VBUS status sounds generic enough, also, you should be printing on/off
> here instead of tahvo-specific values. Some users might not have access
> to the docs, or even the kernel sources, to understand what that integer
> means.
Ok, I'll fix.
> > +static void tahvo_usb_become_host(struct tahvo_usb *tu)
> > +{
> > + struct retu_dev *rdev = dev_get_drvdata(tu->pt_dev->dev.parent);
> > +
> > + extcon_set_cable_state(&tu->extcon, "USB-HOST", true);
> > +
> > + /* Power up the transceiver in USB host mode */
> > + retu_write(rdev, TAHVO_REG_USBR, USBR_REGOUT | USBR_NSUSPEND |
> > + USBR_MASTER_SW2 | USBR_MASTER_SW1);
> > + tu->phy.state = OTG_STATE_A_IDLE;
> > +
> > + check_vbus_state(tu);
> > +}
> > +
> > +static void tahvo_usb_stop_host(struct tahvo_usb *tu)
> > +{
> > + tu->phy.state = OTG_STATE_A_IDLE;
>
> shouldn't you undo tahvo_usb_become_host() here ? I mean, set the
> transceiver to SLAVE ?
tahvo_usb_become_peripheral() (or power_off) is always called after this
function. Actually, these stop functions can be eliminated altogether
as they are only called from a single place...
> > +static int tahvo_usb_set_host(struct usb_otg *otg, struct usb_bus *host)
> > +{
> > + struct tahvo_usb *tu = container_of(otg->phy, struct tahvo_usb, phy);
> > +
> > + dev_dbg(&tu->pt_dev->dev, "%s %p\n", __func__, host);
> > +
> > + if (otg == NULL)
> > + return -ENODEV;
> > +
> > + mutex_lock(&tu->serialize);
> > +
> > + if (host == NULL) {
> > + if (tu->tahvo_mode == TAHVO_MODE_HOST)
> > + tahvo_usb_power_off(tu);
> > + otg->host = NULL;
> > + mutex_unlock(&tu->serialize);
> > + return 0;
> > + }
> > +
> > + if (tu->tahvo_mode == TAHVO_MODE_HOST) {
> > + otg->host = NULL;
> > + tahvo_usb_become_host(tu);
> > + }
> > +
> > + otg->host = host;
> > +
> > + mutex_unlock(&tu->serialize);
> > +
> > + return 0;
> > +}
> > +
> > +static int tahvo_usb_set_peripheral(struct usb_otg *otg,
> > + struct usb_gadget *gadget)
>
> I want to get rid of the extra 'gadget' and 'host' parameters on
> ->set_host() and ->set_peripheral(). Nobody really uses them other than:
>
> my_phy->phy.otg->{gadget,host} = {gadget,host};
>
> For that, I need all other PHYs to stop relying on those parameters and
> I'll cook patches for that for v3.12 merge window.
How will the PHY know if there is a host/gadget driver present? I guess
I will need to wait for those patches...
> > +static ssize_t otg_mode_store(struct device *device,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct tahvo_usb *tu = dev_get_drvdata(device);
> > + int r;
> > +
> > + mutex_lock(&tu->serialize);
> > + if (count >= 4 && strncmp(buf, "host", 4) == 0) {
> > + if (tu->tahvo_mode == TAHVO_MODE_PERIPHERAL)
> > + tahvo_usb_stop_peripheral(tu);
> > + tu->tahvo_mode = TAHVO_MODE_HOST;
> > + if (tu->phy.otg->host) {
> > + dev_info(device, "HOST mode: host controller
> > present\n");
> > + tahvo_usb_become_host(tu);
> > + } else {
> > + dev_info(device, "HOST mode: no host controller,
> > powering off\n");
> > + tahvo_usb_power_off(tu);
> > + }
> > + r = strlen(buf);
> > + } else if (count >= 10 && strncmp(buf, "peripheral", 10) == 0) {
> > + if (tu->tahvo_mode == TAHVO_MODE_HOST)
> > + tahvo_usb_stop_host(tu);
> > + tu->tahvo_mode = TAHVO_MODE_PERIPHERAL;
> > + if (tu->phy.otg->gadget) {
> > + dev_info(device, "PERIPHERAL mode: gadget driver
> > present\n");
> > + tahvo_usb_become_peripheral(tu);
> > + } else {
> > + dev_info(device, "PERIPHERAL mode: no gadget driver,
> > powering off\n");
> > + tahvo_usb_power_off(tu);
> > + }
> > + r = strlen(buf);
> > + } else {
> > + r = -EINVAL;
> > + }
> > + mutex_unlock(&tu->serialize);
> > +
> > + return r;
> > +}
> > +
> > +static DEVICE_ATTR(otg_mode, 0644, otg_mode_show, otg_mode_store);
>
> this looks like something which would be very nice if implemented
> generically. Since you, anyway miss the documentation in
> Documentation/ABI/ and need to respin the patch, how about making this
> generic ?
Ok, I'
Re: [PATCH v3 4/4] USB: OMAP1: Tahvo USB transceiver driver
Hi,
On Tue, Jun 18, 2013 at 08:07:01PM +0300, Aaro Koskinen wrote:
> +static ssize_t vbus_state_show(struct device *device,
> +struct device_attribute *attr, char *buf)
> +{
> + struct tahvo_usb *tu = dev_get_drvdata(device);
> + return sprintf(buf, "%d\n", tu->vbus_state);
> +}
> +static DEVICE_ATTR(vbus_state, 0444, vbus_state_show, NULL);
VBUS status sounds generic enough, also, you should be printing on/off
here instead of tahvo-specific values. Some users might not have access
to the docs, or even the kernel sources, to understand what that integer
means.
> +static void tahvo_usb_become_host(struct tahvo_usb *tu)
> +{
> + struct retu_dev *rdev = dev_get_drvdata(tu->pt_dev->dev.parent);
> +
> + extcon_set_cable_state(&tu->extcon, "USB-HOST", true);
> +
> + /* Power up the transceiver in USB host mode */
> + retu_write(rdev, TAHVO_REG_USBR, USBR_REGOUT | USBR_NSUSPEND |
> +USBR_MASTER_SW2 | USBR_MASTER_SW1);
> + tu->phy.state = OTG_STATE_A_IDLE;
> +
> + check_vbus_state(tu);
> +}
> +
> +static void tahvo_usb_stop_host(struct tahvo_usb *tu)
> +{
> + tu->phy.state = OTG_STATE_A_IDLE;
shouldn't you undo tahvo_usb_become_host() here ? I mean, set the
transceiver to SLAVE ?
> +static int tahvo_usb_set_host(struct usb_otg *otg, struct usb_bus *host)
> +{
> + struct tahvo_usb *tu = container_of(otg->phy, struct tahvo_usb, phy);
> +
> + dev_dbg(&tu->pt_dev->dev, "%s %p\n", __func__, host);
> +
> + if (otg == NULL)
> + return -ENODEV;
> +
> + mutex_lock(&tu->serialize);
> +
> + if (host == NULL) {
> + if (tu->tahvo_mode == TAHVO_MODE_HOST)
> + tahvo_usb_power_off(tu);
> + otg->host = NULL;
> + mutex_unlock(&tu->serialize);
> + return 0;
> + }
> +
> + if (tu->tahvo_mode == TAHVO_MODE_HOST) {
> + otg->host = NULL;
> + tahvo_usb_become_host(tu);
> + }
> +
> + otg->host = host;
> +
> + mutex_unlock(&tu->serialize);
> +
> + return 0;
> +}
> +
> +static int tahvo_usb_set_peripheral(struct usb_otg *otg,
> + struct usb_gadget *gadget)
I want to get rid of the extra 'gadget' and 'host' parameters on
->set_host() and ->set_peripheral(). Nobody really uses them other than:
my_phy->phy.otg->{gadget,host} = {gadget,host};
For that, I need all other PHYs to stop relying on those parameters and
I'll cook patches for that for v3.12 merge window.
> +static ssize_t otg_mode_store(struct device *device,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct tahvo_usb *tu = dev_get_drvdata(device);
> + int r;
> +
> + mutex_lock(&tu->serialize);
> + if (count >= 4 && strncmp(buf, "host", 4) == 0) {
> + if (tu->tahvo_mode == TAHVO_MODE_PERIPHERAL)
> + tahvo_usb_stop_peripheral(tu);
> + tu->tahvo_mode = TAHVO_MODE_HOST;
> + if (tu->phy.otg->host) {
> + dev_info(device, "HOST mode: host controller
> present\n");
> + tahvo_usb_become_host(tu);
> + } else {
> + dev_info(device, "HOST mode: no host controller,
> powering off\n");
> + tahvo_usb_power_off(tu);
> + }
> + r = strlen(buf);
> + } else if (count >= 10 && strncmp(buf, "peripheral", 10) == 0) {
> + if (tu->tahvo_mode == TAHVO_MODE_HOST)
> + tahvo_usb_stop_host(tu);
> + tu->tahvo_mode = TAHVO_MODE_PERIPHERAL;
> + if (tu->phy.otg->gadget) {
> + dev_info(device, "PERIPHERAL mode: gadget driver
> present\n");
> + tahvo_usb_become_peripheral(tu);
> + } else {
> + dev_info(device, "PERIPHERAL mode: no gadget driver,
> powering off\n");
> + tahvo_usb_power_off(tu);
> + }
> + r = strlen(buf);
> + } else {
> + r = -EINVAL;
> + }
> + mutex_unlock(&tu->serialize);
> +
> + return r;
> +}
> +
> +static DEVICE_ATTR(otg_mode, 0644, otg_mode_show, otg_mode_store);
this looks like something which would be very nice if implemented
generically. Since you, anyway miss the documentation in
Documentation/ABI/ and need to respin the patch, how about making this
generic ?
> +static int tahvo_usb_probe(struct platform_device *pdev)
> +{
> + struct retu_dev *rdev = dev_get_drvdata(pdev->dev.parent);
> + struct tahvo_usb *tu;
> + int ret;
> +
> + tu = devm_kzalloc(&pdev->dev, sizeof(*tu), GFP_KERNEL);
> + if (!tu)
> + return -ENOMEM;
> +
> + tu->phy.otg = devm_kzalloc(&pdev->dev, sizeof(*tu->phy.otg),
> +GFP_KERNEL);
> + if (!tu->phy.otg)
> + return -ENOMEM;
> +
> + tu->pt_dev = pdev;
