Am 16.01.2017 um 15:05 schrieb Felipe Balbi:
>
> Hi,
>
> Heiner Kallweit <hkallweit1@xxxxxxxxx> writes:
>> Set the iomem parameters in the usb_hcd to fix this misleading
>> message during driver load:
>> dwc2 c9100000.usb: irq 22, io mem 0x00000000
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> ---
>> drivers/usb/dwc2/core.h | 3 ++-
>> drivers/usb/dwc2/hcd.c | 5 ++++-
>> drivers/usb/dwc2/hcd.h | 3 ++-
>> drivers/usb/dwc2/platform.c | 2 +-
>> 4 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 9548d3e0..b66eaeea 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -1229,7 +1229,8 @@ static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
>> static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
>> static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>> static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
>> -static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>> +static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>> + struct resource *res)
>> { return 0; }
>> static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
>> { return 0; }
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 911c3b36..2cfbd10e 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -4964,7 +4964,7 @@ static void dwc2_hcd_release(struct dwc2_hsotg *hsotg)
>> * USB bus with the core and calls the hc_driver->start() function. It returns
>> * a negative error on failure.
>> */
>> -int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>> +int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, struct resource *res)
>> {
>> struct usb_hcd *hcd;
>> struct dwc2_host_chan *channel;
>> @@ -5021,6 +5021,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>
>> hcd->has_tt = 1;
>>
>> + hcd->rsrc_start = res->start;
>> + hcd->rsrc_len = resource_size(res);
>> +
>> ((struct wrapper_priv_data *) &hcd->hcd_priv)->hsotg = hsotg;
>> hsotg->priv = hcd;
>>
>> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
>> index 1ed5fa2b..2305b5fb 100644
>> --- a/drivers/usb/dwc2/hcd.h
>> +++ b/drivers/usb/dwc2/hcd.h
>> @@ -521,7 +521,8 @@ static inline u8 dwc2_hcd_is_pipe_out(struct dwc2_hcd_pipe_info *pipe)
>> return !dwc2_hcd_is_pipe_in(pipe);
>> }
>>
>> -extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq);
>> +extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>> + struct resource *res);
>> extern void dwc2_hcd_remove(struct dwc2_hsotg *hsotg);
>>
>> /* Transaction Execution Functions */
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index 4fc8c603..5ddc8860 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -445,7 +445,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
>> }
>>
>> if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) {
>> - retval = dwc2_hcd_init(hsotg, hsotg->irq);
>> + retval = dwc2_hcd_init(hsotg, hsotg->irq, res);
>
> This is a good idea, but there's more work that can come out of this, if
> you're interested:
>
> i) devm_ioremap_resource() could be called by the generic layer
> ii) devm_request_irq() could be move to the generic layer
> iii) dwc2_hcd_init() could also be called generically as long as dr_mode
> is set properly.
> iv) dwc2_debugfs_init() could be called generically as well
>
> IOW, dwc2_driver_probe() could be as minimal as:
>
> static int dwc2_driver_probe(struct platform_device *dev)
> {
> struct dwc2_hsotg *hsotg;
> struct resource *res;
> int retval;
>
> hsotg = devm_kzalloc(&dev->dev, sizeof(*hsotg), GFP_KERNEL);
> if (!hsotg)
> return -ENOMEM;
>
> hsotg->dev = &dev->dev;
>
> if (!dev->dev.dma_mask)
> dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
>
> retval = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
> if (retval)
> return retval;
>
> hsotg->res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> hsotg->irq = platform_get_irq(dev, 0);
>
> retval = dwc2_get_dr_mode(hsotg);
> if (retval)
> return retval;
>
> retval = dwc2_get_hwparams(hsotg);
> if (retval)
> return retval;
>
> platform_set_drvdata(dev, hsotg);
>
> retval = dwc2_core_init(hsotg);
> if (retval)
> return retval;
>
> return 0;
> }
>
> dwc2_core_init() needs to implemented, of course, but it could hide all
> details about what to do with IRQs and resources and what not. Assuming
> you can properly initialize clocks, it could even handle clocks
> generically for you.
>
I see what you mean. I'm just a little confused about the term "generic" here:
For me something generic has more than one user. Here we seem to speak just
about factoring out some code from the probe function, or?
Your proposal for reducing the probe function would mean to reorder some calls
like the ones to dwc2_lowlevel_hw_init and dwc2_lowlevel_hw_enable.
At a first glance this seems to be ok, but I'm not 100% sure.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
