Re: [PATCH v4] PHY: sunxi: Add driver for sunxi usb phy

[Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

Hi,

On 03/01/2014 06:37 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Saturday 01 March 2014 10:39 PM, Hans de Goede wrote:
>> The Allwinner A1x / A2x SoCs have 2 or 3 usb phys which are all accessed
>> through a single set of registers. Besides this there are also some other
>> phy related bits which need poking, which are per phy, but shared between the
>> ohci and ehci controllers, so these are also controlled from this new phy
>> driver.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> ---
>>   .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  26 ++
>>   drivers/phy/Kconfig                                |  11 +
>>   drivers/phy/Makefile                               |   1 +
>>   drivers/phy/phy-sun4i-usb.c                        | 331 +++++++++++++++++++++
>>   4 files changed, 369 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
>>   create mode 100644 drivers/phy/phy-sun4i-usb.c
>>
> ..
> <snip>
> .
> .
> 
>> +static int sun4i_usb_phy_init(struct phy *_phy)
>> +{
>> +    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
>> +    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
>> +    int ret;
>> +
>> +    ret = clk_prepare_enable(data->clk);
>> +    if (ret)
> dev_err here?

I would expect the clk subsys to print an error if anything goes
wrong here, repeating that error in the device driver seems
not useful to me.

Note that this practice of simply propagating the error is a common
pattern in many many users of the clk / reset / regulator framework.

Also can I please get one final review of this patch rather then
a round of comments ending with:

"If you can fix these minor comments while changing the $subject (PHY: sunxi) it
should be good to get merged."

Only to get more review comments when I've already fixed the minor
comments ?

>> +        return ret;
>> +
>> +    ret = reset_control_deassert(phy->reset);
>> +    if (ret) {
> 
> here too..

idem.

>> +        clk_disable_unprepare(data->clk);
>> +        return ret;
>> +    }
>> +
>> +    /* Adjust PHY's magnitude and rate */
>> +    sun4i_usb_phy_write(phy, PHY_TX_AMPLITUDE_TUNE, 0x14, 5);
>> +
>> +    /* Disconnect threshold adjustment */
>> +    sun4i_usb_phy_write(phy, PHY_DISCON_TH_SEL, data->disc_thresh, 2);
>> +
>> +    sun4i_usb_phy_passby(phy, 1);
>> +
>> +    return 0;
>> +}
>> +
>> +static int sun4i_usb_phy_exit(struct phy *_phy)
>> +{
>> +    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
>> +    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
>> +
>> +    sun4i_usb_phy_passby(phy, 0);
>> +    reset_control_assert(phy->reset);
>> +    clk_disable_unprepare(data->clk);
>> +
>> +    return 0;
>> +}
>> +
>> +static int sun4i_usb_phy_power_on(struct phy *_phy)
>> +{
>> +    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
>> +    int ret = 0;
>> +
>> +    if (phy->vbus)
>> +        ret = regulator_enable(phy->vbus);
> dev_err here too..

idem.

>> +
>> +    return ret;
>> +}
>> +
>> +static int sun4i_usb_phy_power_off(struct phy *_phy)
>> +{
>> +    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
>> +
>> +    if (phy->vbus)
>> +        regulator_disable(phy->vbus);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct phy_ops sun4i_usb_phy_ops = {
>> +    .init        = sun4i_usb_phy_init,
>> +    .exit        = sun4i_usb_phy_exit,
>> +    .power_on    = sun4i_usb_phy_power_on,
>> +    .power_off    = sun4i_usb_phy_power_off,
>> +    .owner        = THIS_MODULE,
>> +};
>> +
>> +static struct phy *sun4i_usb_phy_xlate(struct device *dev,
>> +                    struct of_phandle_args *args)
>> +{
>> +    struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
>> +
>> +    if (WARN_ON(args->args[0] == 0 || args->args[0] >= data->num_phys))
>> +        return ERR_PTR(-ENODEV);
>> +
>> +    return data->phys[args->args[0]].phy;
>> +}
>> +
>> +static int sun4i_usb_phy_probe(struct platform_device *pdev)
>> +{
>> +    struct sun4i_usb_phy_data *data;
>> +    struct device *dev = &pdev->dev;
>> +    struct device_node *np = dev->of_node;
>> +    void __iomem *pmu = NULL;
>> +    struct phy_provider *phy_provider;
>> +    struct reset_control *reset;
>> +    struct regulator *vbus;
>> +    struct resource *res;
>> +    struct phy *phy;
>> +    char name[16];
>> +    int i;
>> +
>> +    data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +    if (!data)
>> +        return -ENOMEM;
>> +
>> +    mutex_init(&data->mutex);
>> +
>> +    if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
>> +        data->num_phys = 2;
>> +    else
>> +        data->num_phys = 3;
>> +
>> +    if (of_device_is_compatible(np, "allwinner,sun4i-a10-usb-phy"))
>> +        data->disc_thresh = 3;
>> +    else
>> +        data->disc_thresh = 2;
> 
> These 'data' can actually be part of 'of_device_id' table and can be obtained by using 'of_match_device'.

This was already suggested by Maxime around the time of v2, and
I responded with this:

"The problem with using the of_device_id .data field is that I can only
store a single integer there. To store 2 I need to: define a struct,
create an array of these structs with initialization. Create an enum for
indexing the array which must be kept in sync with the initializers
manually, store either the index, or a direct pointer to the correct array
entry into the .data field, add code to get the of_device_id from the
compatible string, and then finally extract the settings from the struct
again.

See IE how this is done in drivers/ata/ahci_platform.c, I've tried
to come up with a simpler way and failed, for ahci_platform.c the
struct with per compatible-string data is quite big so it makes some
sense to use this construction. Here however not so much, this adds a
whole lot of unnecessary extra code + indirection. I esp. object against
the indirection as that unnecessarily makes it harder to follow whats
going on."

Which is still valid.

Regards,

Hans