Re: [PATCH v4 1/6] drivers: phy: add generic PHY framework

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi,

On Tuesday 02 April 2013 01:04 AM, Sylwester Nawrocki wrote:
> Just couple minor comments...
>
> On 03/28/2013 06:43 AM, Kishon Vijay Abraham I wrote:
>> The PHY framework provides a set of APIs for the PHY drivers to
>> create/destroy a PHY and APIs for the PHY users to obtain a reference
>> to the
>> PHY with or without using phandle. To obtain a reference to the PHY
>> without
>> using phandle, the platform specfic intialization code (say from board
>> file)
>> should have already called phy_bind with the binding information. The
>> binding
>> information consists of phy's device name, phy user device name and an
>> index.
>> The index is used when the same phy user binds to mulitple phys.
>>
>> PHY drivers should create the PHY by passing phy_descriptor that has
>> describes the PHY (label, type etc..) and ops like init, exit,
>> suspend, resume,
>> poweron, shutdown.
>>
>> The documentation for the generic PHY framework is added in
>> Documentation/phy.txt and the documentation for the sysfs entry is added
>> in Documentation/ABI/testing/sysfs-class-phy and the documentation for
>> dt binding is can be found at
>> Documentation/devicetree/bindings/phy/phy-bindings.txt
>>
>> Signed-off-by: Kishon Vijay Abraham I<kishon@ti.com>
>> ---
> [...]
>> +/**
>> + * phy_put - release the PHY
>
> nit: According to kernel-doc documentation function names should have
> parentheses appended to the name. So it would need to be
>
> + * phy_put() - release the PHY
>
> in this case and it applies to multiple places elsewhere in this patch.

Will fix it.
>
>> + * @phy: the phy returned by phy_get()
>> + *
>> + * Releases a refcount the caller received from phy_get().
>> + */
>> +void phy_put(struct phy *phy)
>> +{
>> +    if (phy) {
>> +        module_put(phy->ops->owner);
>> +        put_device(&phy->dev);
>> +    }
>> +}
>> +EXPORT_SYMBOL_GPL(phy_put);
> [...]
>> +/**
>> + * devm_of_phy_get_byname - lookup and obtain a reference to a phy by
>> name
>> + * @dev: device that requests this phy
>> + * @string - the phy name as given in the dt data
>
> s/ -/:

Ok.
>
>> + *
>> + * Calls devm_of_phy_get (which associates the device with the phy
>> using devres
>> + * on successful phy get) and passes on the return value of
>> devm_of_phy_get.
>> + */
>> +struct phy *devm_of_phy_get_byname(struct device *dev, const char
>> *string)
>> +{
>> +    int index;
>> +
>> +    if (!dev->of_node) {
>> +        dev_dbg(dev, "device does not have a device node entry\n");
>> +        return ERR_PTR(-EINVAL);
>> +    }
>> +
>> +    index = of_property_match_string(dev->of_node, "phy-names", string);
>> +
>> +    return devm_of_phy_get(dev, index);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_of_phy_get_byname);
>> +
>> +/**
>> + * phy_get - lookup and obtain a reference to a phy.
>> + * @dev: device that requests this phy
>> + * @index: the index of the phy
>> + *
>> + * Returns the phy driver, after getting a refcount to it; or
>> + * -ENODEV if there is no such phy.  The caller is responsible for
>> + * calling phy_put() to release that count.
>> + */
>> +struct phy *phy_get(struct device *dev, int index)
>> +{
>> +    struct phy *phy = NULL;
>
> Unneeded initialization.
>
>> +    phy = phy_lookup(dev, index);
>> +    if (IS_ERR(phy)) {
>> +        dev_err(dev, "unable to find phy\n");
>> +        goto err0;
>
> Wouldn't it be better to just do:

Indeed.
>
>          return phy;
>> +    }
>> +
>> +    if (!try_module_get(phy->ops->owner)) {
>> +        phy = ERR_PTR(-EPROBE_DEFER);
>
> and
>          return ERR_PTR(-EPROBE_DEFER);
>
>> +        goto err0;
>
> and to drop this goto and the label below ?
>
>> +    }
>> +
>> +    get_device(&phy->dev);
>> +
>> +err0:
>> +    return phy;
>> +}
>> +EXPORT_SYMBOL_GPL(phy_get);
>
>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>> new file mode 100644
>> index 0000000..0cb2298
>> --- /dev/null
>> +++ b/include/linux/phy/phy.h
>> @@ -0,0 +1,237 @@
>> +/*
>> + * phy.h -- generic phy header file
> [...]
>> +#ifndef __DRIVERS_PHY_H
>> +#define __DRIVERS_PHY_H
>> +
>> +#include<linux/err.h>
>> +#include<linux/of.h>
>> +
>> +struct phy
>
> I think you also need to add either
>
> #include <linux/device.h>
>
> or
>
> struct device;
>
> struct device * is used further in this file.

Ok.
>
>> +/**
>> + * struct phy_ops - set of function pointers for performing phy
>> operations
>> + * @init: operation to be performed for initializing phy
>> + * @exit: operation to be performed while exiting
>> + * @suspend: suspending the phy
>> + * @resume: resuming the phy
>
>> + * @poweron: powering on the phy
>> + * @shutdown: shutting down the phy
>
> Could these be named power_on/power_off ? Looks a bit more symmetric
> to me that way.

Ok. Will have it that way.

Thanks
Kishon