| Message ID | 1383335158-19730-2-git-send-email-matt.porter@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Matt, On Friday 01 of November 2013 15:45:50 Matt Porter wrote: > This adds a pair of APIs that allows the generic PHY subsystem to > provide information on the PHY bus width. The PHY provider driver may > use phy_set_bus_width() to set the bus width that the PHY supports. > The controller driver may then use phy_get_bus_width() to fetch the > PHY bus width in order to properly configure the controller. I somehow does not like this. If we take this path for any further properties that we may need, we will end up with a lot of consumer specific properties stored in a PHY object having their own accessor functions. Since this is just an integration detail, what about simply adding this as a property in device tree node of the OTG controller (and pdata if considering non-DT support)? Another option would be some framework for retrieving arbitrary properties from the PHY, but I'm not really sure there is a need for such. Best regards, Tomasz
Hi Tomasz, On Saturday 02 November 2013 06:44 PM, Tomasz Figa wrote: > Hi Matt, > > On Friday 01 of November 2013 15:45:50 Matt Porter wrote: >> This adds a pair of APIs that allows the generic PHY subsystem to >> provide information on the PHY bus width. The PHY provider driver may >> use phy_set_bus_width() to set the bus width that the PHY supports. >> The controller driver may then use phy_get_bus_width() to fetch the >> PHY bus width in order to properly configure the controller. > > I somehow does not like this. If we take this path for any further > properties that we may need, we will end up with a lot of consumer > specific properties stored in a PHY object having their own accessor > functions. Only after all of us feel that a property is *generic* enough, we allow it to be added in the PHY object. > > Since this is just an integration detail, what about simply adding this as > a property in device tree node of the OTG controller (and pdata if > considering non-DT support)? We already had a discussion about this and the dt maintainers suggested the property should be in the PHY. [1] [1] -> http://www.spinics.net/lists/devicetree/msg08851.html Thanks Kishon
On Sat, Nov 02, 2013 at 10:46:55PM +0530, Kishon Vijay Abraham I wrote: > Hi Tomasz, > > On Saturday 02 November 2013 06:44 PM, Tomasz Figa wrote: > >Hi Matt, > > > >On Friday 01 of November 2013 15:45:50 Matt Porter wrote: > >>This adds a pair of APIs that allows the generic PHY subsystem to > >>provide information on the PHY bus width. The PHY provider driver may > >>use phy_set_bus_width() to set the bus width that the PHY supports. > >>The controller driver may then use phy_get_bus_width() to fetch the > >>PHY bus width in order to properly configure the controller. > > > >I somehow does not like this. If we take this path for any further > >properties that we may need, we will end up with a lot of consumer > >specific properties stored in a PHY object having their own accessor > >functions. > > Only after all of us feel that a property is *generic* enough, we > allow it to be added in the PHY object. I also want to note that this was discussed over in another thread [2] where you did consider my rough stab at a more generic attribute accessor. It was definitely my first reaction as the way to do it like Tomasz has said. The specific accessors are more readable to me besides the justification you mention above. [2] http://lkml.indiana.edu/hypermail/linux/kernel/1310.3/00673.html -Matt > >Since this is just an integration detail, what about simply adding this as > >a property in device tree node of the OTG controller (and pdata if > >considering non-DT support)? > > We already had a discussion about this and the dt maintainers > suggested the property should be in the PHY. [1] > > [1] -> http://www.spinics.net/lists/devicetree/msg08851.html > > Thanks > Kishon
On Saturday 02 of November 2013 13:47:09 Matt Porter wrote: > On Sat, Nov 02, 2013 at 10:46:55PM +0530, Kishon Vijay Abraham I wrote: > > Hi Tomasz, > > > > On Saturday 02 November 2013 06:44 PM, Tomasz Figa wrote: > > >Hi Matt, > > > > > >On Friday 01 of November 2013 15:45:50 Matt Porter wrote: > > >>This adds a pair of APIs that allows the generic PHY subsystem to > > >>provide information on the PHY bus width. The PHY provider driver > > >>may > > >>use phy_set_bus_width() to set the bus width that the PHY supports. > > >>The controller driver may then use phy_get_bus_width() to fetch the > > >>PHY bus width in order to properly configure the controller. > > > > > >I somehow does not like this. If we take this path for any further > > >properties that we may need, we will end up with a lot of consumer > > >specific properties stored in a PHY object having their own accessor > > >functions. > > > > Only after all of us feel that a property is *generic* enough, we > > allow it to be added in the PHY object. > > I also want to note that this was discussed over in another thread [2] > where you did consider my rough stab at a more generic attribute > accessor. It was definitely my first reaction as the way to do it like > Tomasz has said. The specific accessors are more readable to me besides > the justification you mention above. > > [2] http://lkml.indiana.edu/hypermail/linux/kernel/1310.3/00673.html Personally I like that version much better, but still it would need to be polished a bit. How I imagine such interface to be implemented: phy.h: struct phy { // ... const struct phy_attrs *attrs; // ... }; static inline const struct phy_attrs *phy_get_attrs(struct phy *phy) { return phy->attrs; }; phy driver: static const struct phy_attrs my_phy_attrs = { // ... }; static int my_phy_probe(...) { // ... phy = devm_phy_create_attrs(dev, &ops, &my_phy_attrs, NULL); // ... } phy consumer: // ... const struct phy_attrs *phy_attrs; phy_attrs = phy_get_attrs(phy); // ... Why I think it is better than what I've seen in this and previous instance of this thread? (in random order) a) Only the PHY driver can set the attrs. b) PHY consumer has access only to a const pointer. c) PHY attributes can be placed in a static struct inside a driver file, without the need to call any functions to set particular attributes. d) Can be extended with more attributes easily. Best regards, Tomasz
Hi, On Saturday 02 November 2013 11:28 PM, Tomasz Figa wrote: > On Saturday 02 of November 2013 13:47:09 Matt Porter wrote: >> On Sat, Nov 02, 2013 at 10:46:55PM +0530, Kishon Vijay Abraham I wrote: >>> Hi Tomasz, >>> >>> On Saturday 02 November 2013 06:44 PM, Tomasz Figa wrote: >>>> Hi Matt, >>>> >>>> On Friday 01 of November 2013 15:45:50 Matt Porter wrote: >>>>> This adds a pair of APIs that allows the generic PHY subsystem to >>>>> provide information on the PHY bus width. The PHY provider driver >>>>> may >>>>> use phy_set_bus_width() to set the bus width that the PHY supports. >>>>> The controller driver may then use phy_get_bus_width() to fetch the >>>>> PHY bus width in order to properly configure the controller. >>>> >>>> I somehow does not like this. If we take this path for any further >>>> properties that we may need, we will end up with a lot of consumer >>>> specific properties stored in a PHY object having their own accessor >>>> functions. >>> >>> Only after all of us feel that a property is *generic* enough, we >>> allow it to be added in the PHY object. >> >> I also want to note that this was discussed over in another thread [2] >> where you did consider my rough stab at a more generic attribute >> accessor. It was definitely my first reaction as the way to do it like >> Tomasz has said. The specific accessors are more readable to me besides >> the justification you mention above. >> >> [2] http://lkml.indiana.edu/hypermail/linux/kernel/1310.3/00673.html > > Personally I like that version much better, but still it would need to be > polished a bit. > > How I imagine such interface to be implemented: > > phy.h: > > struct phy { > // ... > const struct phy_attrs *attrs; > // ... > }; > > static inline const struct phy_attrs *phy_get_attrs(struct phy *phy) { > return phy->attrs; > }; API's like get_attrs is loosely defined. I'd prefer to have fully defined APIs. There might be other attributes which the consumer might not be interested in. Instead of returning the entire structure, it would be better if we have facilities for the consumer to request only the required attributes. > > phy driver: > > static const struct phy_attrs my_phy_attrs = { > // ... > }; > > static int my_phy_probe(...) > { > // ... > phy = devm_phy_create_attrs(dev, &ops, &my_phy_attrs, NULL); > // ... > } > > phy consumer: > > // ... > const struct phy_attrs *phy_attrs; > > phy_attrs = phy_get_attrs(phy); > // ... > > Why I think it is better than what I've seen in this and previous instance > of this thread? (in random order) > a) Only the PHY driver can set the attrs. > b) PHY consumer has access only to a const pointer. > c) PHY attributes can be placed in a static struct inside a driver file, > without the need to call any functions to set particular attributes. Agree with all your points for setting the attributes apart from the fact that we won't be able to add any validation criteria for the attributes while setting it if needed and also there won't be symmetric APIs for getting and setting the attributes.. Cheers Kishon
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index 6d72269..e858ce1 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -46,6 +46,7 @@ struct phy_ops { * @mutex: mutex to protect phy_ops * @init_count: used to protect when the PHY is used by multiple consumers * @power_count: used to protect when the PHY is used by multiple consumers + * @bus_width: used to specify data width of the PHY bus */ struct phy { struct device dev; @@ -55,6 +56,7 @@ struct phy { struct mutex mutex; int init_count; int power_count; + int bus_width; }; /** @@ -127,6 +129,12 @@ int phy_init(struct phy *phy); int phy_exit(struct phy *phy); int phy_power_on(struct phy *phy); int phy_power_off(struct phy *phy); +static inline int phy_get_bus_width(struct phy *phy) { + return phy->bus_width; +}; +static inline void phy_set_bus_width(struct phy *phy, int bus_width) { + phy->bus_width = bus_width; +}; struct phy *phy_get(struct device *dev, const char *string); struct phy *devm_phy_get(struct device *dev, const char *string); void phy_put(struct phy *phy); @@ -199,6 +207,14 @@ static inline int phy_power_off(struct phy *phy) return -ENOSYS; } +static inline int phy_get_bus_width(struct phy *phy) { + return -ENOSYS; +}; + +static inline void phy_set_bus_width(struct phy *phy, bus_width) { + return; +}; + static inline struct phy *phy_get(struct device *dev, const char *string) { return ERR_PTR(-ENOSYS);
This adds a pair of APIs that allows the generic PHY subsystem to provide information on the PHY bus width. The PHY provider driver may use phy_set_bus_width() to set the bus width that the PHY supports. The controller driver may then use phy_get_bus_width() to fetch the PHY bus width in order to properly configure the controller. Signed-off-by: Matt Porter <matt.porter@linaro.org> --- include/linux/phy/phy.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)