Microchip USB Hub Driver Harmonization

Microchip USB Hub Driver Harmonization

[Richard Leitner]

Hello,
due to the fact (all?) the Microchip (former SMSC) USB hubs share the
same I2C configuration interface, I'm currently working on harmonizing
those USB Hub drivers. Currently this affects the usb251xb, usb3503 and
usb4604 drivers. To avoid preventable efforts (and patch versions) I
have some question on the preferred implementation:

1. Currently usb251xb uses i2c_smbus_*, usb3503 uses regmap_* and
usb4604 uses i2c_master_* functions for the hub configuration. What
would be the preferred solution?

2. What would be a good prefix for common headers/functions/macros/etc.?
I thought of "mcusbhub"... Would that be OK? Or are there any
conventions/better proposals on that?

3. Currently only usb3503 supports "platform data". Is this still needed
or may it be removed?

Thanks & kind regards,
RichardL

Re: Microchip USB Hub Driver Harmonization

[Krzysztof Kozlowski]

On Wed, May 17, 2017 at 12:58:38PM +0200, Richard Leitner wrote:
> Hello,
> due to the fact (all?) the Microchip (former SMSC) USB hubs share the
> same I2C configuration interface, I'm currently working on harmonizing
> those USB Hub drivers. Currently this affects the usb251xb, usb3503 and
> usb4604 drivers. To avoid preventable efforts (and patch versions) I
> have some question on the preferred implementation:
> 
> 1. Currently usb251xb uses i2c_smbus_*, usb3503 uses regmap_* and
> usb4604 uses i2c_master_* functions for the hub configuration. What
> would be the preferred solution?

regmap? It is already widely used for I2C drivers. I think most (or even
all?) new I2C drivers use regmap. It hides the real bus between common
regmap API.

> 2. What would be a good prefix for common headers/functions/macros/etc.?
> I thought of "mcusbhub"... Would that be OK? Or are there any
> conventions/better proposals on that?

If you are going to develop one driver for entire family, then you could
even choose just one name. Let's say the most generic.

I don't quite understand the meaning behind "harmonizing drivers".

> 3. Currently only usb3503 supports "platform data". Is this still needed
> or may it be removed?

I think it is still used, e.g. by:
arch/arm/boot/dts/exynos5250-spring.dts

Best regards,
Krzysztof

Re: Microchip USB Hub Driver Harmonization

[Richard Leitner]

On 05/17/2017 06:01 PM, Krzysztof Kozlowski wrote:
> On Wed, May 17, 2017 at 12:58:38PM +0200, Richard Leitner wrote:
>> ...
>>
>> 1. Currently usb251xb uses i2c_smbus_*, usb3503 uses regmap_* and
>> usb4604 uses i2c_master_* functions for the hub configuration. What
>> would be the preferred solution?
>
> regmap? It is already widely used for I2C drivers. I think most (or even
> all?) new I2C drivers use regmap. It hides the real bus between common
> regmap API.

Ok. Thanks for that information. Then I will go for regmap.

>
>> 2. What would be a good prefix for common headers/functions/macros/etc.?
>> I thought of "mcusbhub"... Would that be OK? Or are there any
>> conventions/better proposals on that?
>
> If you are going to develop one driver for entire family, then you could
> even choose just one name. Let's say the most generic.
>
> I don't quite understand the meaning behind "harmonizing drivers".

I'm currently evaluating if it is feasible to create one common driver 
for all USBxxxx hubs. Due to the fact there are currently only 3 hub 
types implemented mainline (the Microchip homepage lists 36 USB Hub 
products [1]) it involves quite a lot data-sheet reading ;-)

As a first step (and also the final one if implementing a common driver 
is not feasible) I thought of creating a common header/source file which 
implements all "re-useable" functions/macros/etc. Would that also be an 
accepted solution?

>
>> 3. Currently only usb3503 supports "platform data". Is this still needed
>> or may it be removed?
>
> I think it is still used, e.g. by:
> arch/arm/boot/dts/exynos5250-spring.dts

Please correct me if I'm wrong, but isn't that DT only?

The reason why I'm asking if it may be removed is because the only file 
including "linux/platform_data/usb3503.h" is the usb3503.c driver itself 
and it's also the only file using "struct usb3503_platform_data".

Thanks & kind regards,
RichardL

[1] 
https://www.microchip.com/design-centers/usb/usb-hubs-and-devices/products/overview

Re: Microchip USB Hub Driver Harmonization

[Krzysztof Kozlowski]

On Thu, May 18, 2017 at 7:33 AM, Richard Leitner <me@g0hl1n.net> wrote:
>>> 2. What would be a good prefix for common headers/functions/macros/etc.?
>>> I thought of "mcusbhub"... Would that be OK? Or are there any
>>> conventions/better proposals on that?
>>
>>
>> If you are going to develop one driver for entire family, then you could
>> even choose just one name. Let's say the most generic.
>>
>> I don't quite understand the meaning behind "harmonizing drivers".
>
>
> I'm currently evaluating if it is feasible to create one common driver for
> all USBxxxx hubs. Due to the fact there are currently only 3 hub types
> implemented mainline (the Microchip homepage lists 36 USB Hub products [1])
> it involves quite a lot data-sheet reading ;-)
>
> As a first step (and also the final one if implementing a common driver is
> not feasible) I thought of creating a common header/source file which
> implements all "re-useable" functions/macros/etc. Would that also be an
> accepted solution?

I understand. It is fine with me. You could make common part in few ways, e.g.:
1. prepare re-usable functions etc (as you said),
2. create one driver core (with init/probe/remove/exit etc) which
delegates most of the body to specific handlers around.

The benefit of the second approach is that one sees immediately that
there is one driver for entire family.


>>> 3. Currently only usb3503 supports "platform data". Is this still needed
>>> or may it be removed?
>>
>>
>> I think it is still used, e.g. by:
>> arch/arm/boot/dts/exynos5250-spring.dts
>
>
> Please correct me if I'm wrong, but isn't that DT only?
>
> The reason why I'm asking if it may be removed is because the only file
> including "linux/platform_data/usb3503.h" is the usb3503.c driver itself and
> it's also the only file using "struct usb3503_platform_data".

Ah yes, my mistake. I understood that you are asking about "platform
driver", not "platform data". From my perspective all boards use DT so
there is no platform data. In other drivers I found that some folks
from x86 world use SFI/platform_data but I do not know if it applies
here. Anyway removing platform data is fine with me.

Best regards,
Krzysztof