| Message ID | 1562868255-31467-1-git-send-email-prime.zeng@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | phy: Change the configuration interface param to void* to make it more general | expand |
On Fri, Jul 12, 2019 at 02:04:08AM +0800, Zeng Tao wrote: > The phy framework now allows runtime configurations, but only limited > to mipi now, and it's not reasonable to introduce user specified > configurations into the union phy_configure_opts structure. An simple > way is to replace with a void *. I'm not sure why it's unreasonable? > We have already got some phy drivers which introduce private phy API > for runtime configurations, and with this patch, they can switch to > the phy_configure as a replace. If you have a custom mode of operation, then you'll need a custom phy_mode as well, and surely you can have a custom set of parameters. Since those functions are meant to provide a two-way negotiation of the various parameters, you'll have to have that structure shared between the two either way, so the only thing required in addition to what you would have passing a void is one line to add that structure in the union. That's barely unreasonable. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Maxime: Thanks for your reply. >-----Original Message----- >From: Maxime Ripard [mailto:maxime.ripard@bootlin.com] >Sent: Thursday, July 11, 2019 7:21 PM >To: Zengtao (B) <prime.zeng@hisilicon.com> >Cc: kishon@ti.com; Chen-Yu Tsai <wens@csie.org>; Paul Kocialkowski ><paul.kocialkowski@bootlin.com>; Sakari Ailus <sakari.ailus@linux.intel.com>; >linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org >Subject: Re: [PATCH] phy: Change the configuration interface param to void* >to make it more general > >* PGP Signed by an unknown key > >On Fri, Jul 12, 2019 at 02:04:08AM +0800, Zeng Tao wrote: >> The phy framework now allows runtime configurations, but only limited >> to mipi now, and it's not reasonable to introduce user specified >> configurations into the union phy_configure_opts structure. An simple >> way is to replace with a void *. > >I'm not sure why it's unreasonable? > The phy.h will need to include vendor specific phy headers, and the union phy_configure_opts will become more complex. I don't think this is a good solution to include all vendor specific phy configs into a single union structure. >> We have already got some phy drivers which introduce private phy API >> for runtime configurations, and with this patch, they can switch to >> the phy_configure as a replace. > >If you have a custom mode of operation, then you'll need a custom >phy_mode as well, and surely you can have a custom set of parameters. > >Since those functions are meant to provide a two-way negotiation of the >various parameters, you'll have to have that structure shared between the >two either way, so the only thing required in addition to what you would have >passing a void is one line to add that structure in the union. > >That's barely unreasonable. > >Maxime > >-- >Maxime Ripard, Bootlin >Embedded Linux and Kernel engineering >https://bootlin.com > >* Unknown Key >* 0x671851C5
Hi, On Wed, Jul 17, 2019 at 06:36:09AM +0000, Zengtao (B) wrote: > Hi Maxime: > > Thanks for your reply. > > >-----Original Message----- > >From: Maxime Ripard [mailto:maxime.ripard@bootlin.com] > >Sent: Thursday, July 11, 2019 7:21 PM > >To: Zengtao (B) <prime.zeng@hisilicon.com> > >Cc: kishon@ti.com; Chen-Yu Tsai <wens@csie.org>; Paul Kocialkowski > ><paul.kocialkowski@bootlin.com>; Sakari Ailus <sakari.ailus@linux.intel.com>; > >linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org > >Subject: Re: [PATCH] phy: Change the configuration interface param to void* > >to make it more general > > > >* PGP Signed by an unknown key > > > >On Fri, Jul 12, 2019 at 02:04:08AM +0800, Zeng Tao wrote: > >> The phy framework now allows runtime configurations, but only limited > >> to mipi now, and it's not reasonable to introduce user specified > >> configurations into the union phy_configure_opts structure. An simple > >> way is to replace with a void *. > > > >I'm not sure why it's unreasonable? > > > > The phy.h will need to include vendor specific phy headers I'm not sure this is an issue. > and the union phy_configure_opts will become more complex. And this was the plan all along. > I don't think this is a good solution to include all vendor specific > phy configs into a single union structure. The thing is, as Sakari have stated, this interface was meant as a generic way to negotiate a configuration between a PHY consumer and a PHY provider (ie, whatever sends data to the phy and the phy itself). If you remove the explicit type check, then you need to have prior knowledge (and agreement) on both sides, which breaks the initial intent. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi maxime: >-----Original Message----- >From: Maxime Ripard [mailto:maxime.ripard@bootlin.com] >Sent: Thursday, July 18, 2019 12:38 AM >To: Zengtao (B) <prime.zeng@hisilicon.com> >Cc: kishon@ti.com; Chen-Yu Tsai <wens@csie.org>; Paul Kocialkowski ><paul.kocialkowski@bootlin.com>; Sakari Ailus <sakari.ailus@linux.intel.com>; >linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org >Subject: Re: [PATCH] phy: Change the configuration interface param to void* >to make it more general > >Hi, > >On Wed, Jul 17, 2019 at 06:36:09AM +0000, Zengtao (B) wrote: >> Hi Maxime: >> >> Thanks for your reply. >> >> >-----Original Message----- >> >From: Maxime Ripard [mailto:maxime.ripard@bootlin.com] >> >Sent: Thursday, July 11, 2019 7:21 PM >> >To: Zengtao (B) <prime.zeng@hisilicon.com> >> >Cc: kishon@ti.com; Chen-Yu Tsai <wens@csie.org>; Paul Kocialkowski >> ><paul.kocialkowski@bootlin.com>; Sakari Ailus >> ><sakari.ailus@linux.intel.com>; linux-kernel@vger.kernel.org; >> >linux-arm-kernel@lists.infradead.org >> >Subject: Re: [PATCH] phy: Change the configuration interface param to >> >void* to make it more general >> > >> >* PGP Signed by an unknown key >> > >> >On Fri, Jul 12, 2019 at 02:04:08AM +0800, Zeng Tao wrote: >> >> The phy framework now allows runtime configurations, but only >> >> limited to mipi now, and it's not reasonable to introduce user >> >> specified configurations into the union phy_configure_opts >> >> structure. An simple way is to replace with a void *. >> > >> >I'm not sure why it's unreasonable? >> > >> >> The phy.h will need to include vendor specific phy headers > >I'm not sure this is an issue. > >> and the union phy_configure_opts will become more complex. > >And this was the plan all along. > >> I don't think this is a good solution to include all vendor specific >> phy configs into a single union structure. > >The thing is, as Sakari have stated, this interface was meant as a generic way >to negotiate a configuration between a PHY consumer and a PHY provider (ie, >whatever sends data to the phy and the phy itself). > >If you remove the explicit type check, then you need to have prior knowledge >(and agreement) on both sides, which breaks the initial intent. I get your point, so if we follow your design, it will look this: union phy_configure_opts { struct xxxx1_phy phy1_opts; struct xxxx1_phy phy2_opts; struct xxxx1_phy phy3_opts; ..... } And the general phy framework needn't to know about the type but just pass through, the caller and the phy driver definitely need to know what they are doing. So I suggest a more general type void *, otherwise the general phy will need to see a lot of details which is not that general. Zengtao > >Maxime > >-- >Maxime Ripard, Bootlin >Embedded Linux and Kernel engineering >https://bootlin.com
Hi, On Sat, Jul 20, 2019 at 03:03:20AM +0000, Zengtao (B) wrote: > >-----Original Message----- > >From: Maxime Ripard [mailto:maxime.ripard@bootlin.com] > >Sent: Thursday, July 18, 2019 12:38 AM > >To: Zengtao (B) <prime.zeng@hisilicon.com> > >Cc: kishon@ti.com; Chen-Yu Tsai <wens@csie.org>; Paul Kocialkowski > ><paul.kocialkowski@bootlin.com>; Sakari Ailus <sakari.ailus@linux.intel.com>; > >linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org > >Subject: Re: [PATCH] phy: Change the configuration interface param to void* > >to make it more general > > > >Hi, > > > >On Wed, Jul 17, 2019 at 06:36:09AM +0000, Zengtao (B) wrote: > >> Hi Maxime: > >> > >> Thanks for your reply. > >> > >> >-----Original Message----- > >> >From: Maxime Ripard [mailto:maxime.ripard@bootlin.com] > >> >Sent: Thursday, July 11, 2019 7:21 PM > >> >To: Zengtao (B) <prime.zeng@hisilicon.com> > >> >Cc: kishon@ti.com; Chen-Yu Tsai <wens@csie.org>; Paul Kocialkowski > >> ><paul.kocialkowski@bootlin.com>; Sakari Ailus > >> ><sakari.ailus@linux.intel.com>; linux-kernel@vger.kernel.org; > >> >linux-arm-kernel@lists.infradead.org > >> >Subject: Re: [PATCH] phy: Change the configuration interface param to > >> >void* to make it more general > >> > > >> >* PGP Signed by an unknown key > >> > > >> >On Fri, Jul 12, 2019 at 02:04:08AM +0800, Zeng Tao wrote: > >> >> The phy framework now allows runtime configurations, but only > >> >> limited to mipi now, and it's not reasonable to introduce user > >> >> specified configurations into the union phy_configure_opts > >> >> structure. An simple way is to replace with a void *. > >> > > >> >I'm not sure why it's unreasonable? > >> > > >> > >> The phy.h will need to include vendor specific phy headers > > > >I'm not sure this is an issue. > > > >> and the union phy_configure_opts will become more complex. > > > >And this was the plan all along. > > > >> I don't think this is a good solution to include all vendor specific > >> phy configs into a single union structure. > > > >The thing is, as Sakari have stated, this interface was meant as a generic way > >to negotiate a configuration between a PHY consumer and a PHY provider (ie, > >whatever sends data to the phy and the phy itself). > > > >If you remove the explicit type check, then you need to have prior knowledge > >(and agreement) on both sides, which breaks the initial intent. > > I get your point, so if we follow your design, it will look this: > > union phy_configure_opts { > struct xxxx1_phy phy1_opts; > struct xxxx1_phy phy2_opts; > struct xxxx1_phy phy3_opts; > ..... > } > > And the general phy framework needn't to know about the type but > just pass through, the caller and the phy driver definitely need to > know what they are doing. I'm not quite sure what you mean here. The configuration only applies to the current PHY mode. So the phy consumer will have changed the mode, and the PHY will have accepted it. That change is also doable from the framework. Then, which part of the union is being used is easy to figure out for both parties, since they agree on it already. > So I suggest a more general type void *, otherwise the general phy > will need to see a lot of details which is not that general. Except that this effectively becomes a black box that the framework has no control and / or knowledge about. Which means that it cannot do any kind of checks on it anymore, and again, that the consumer and driver need to have prior knowledge of what is being passed, without any way to check whether it's actually what needs to be passed. To some extent, the union also allows that, but it's just less explicit and in general worse, to just pass void* here. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c index 79c8af5..6a60473 100644 --- a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c +++ b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c @@ -105,12 +105,12 @@ static int sun6i_dphy_init(struct phy *phy) return 0; } -static int sun6i_dphy_configure(struct phy *phy, union phy_configure_opts *opts) +static int sun6i_dphy_configure(struct phy *phy, void *opts) { struct sun6i_dphy *dphy = phy_get_drvdata(phy); int ret; - ret = phy_mipi_dphy_config_validate(&opts->mipi_dphy); + ret = phy_mipi_dphy_config_validate(opts); if (ret) return ret; diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c index 90c4e9b..0ec5013 100644 --- a/drivers/phy/cadence/cdns-dphy.c +++ b/drivers/phy/cadence/cdns-dphy.c @@ -233,23 +233,23 @@ static int cdns_dphy_config_from_opts(struct phy *phy, } static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode, - union phy_configure_opts *opts) + void *opts) { struct cdns_dphy_cfg cfg = { 0 }; if (mode != PHY_MODE_MIPI_DPHY) return -EINVAL; - return cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg); + return cdns_dphy_config_from_opts(phy, opts, &cfg); } -static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts) +static int cdns_dphy_configure(struct phy *phy, void *opts) { struct cdns_dphy *dphy = phy_get_drvdata(phy); struct cdns_dphy_cfg cfg = { 0 }; int ret; - ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg); + ret = cdns_dphy_config_from_opts(phy, opts, &cfg); if (ret) return ret; diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index e3880c4a1..048d4d6 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -420,7 +420,7 @@ EXPORT_SYMBOL_GPL(phy_calibrate); * * Returns: 0 if successful, an negative error code otherwise */ -int phy_configure(struct phy *phy, union phy_configure_opts *opts) +int phy_configure(struct phy *phy, void *opts) { int ret; @@ -455,7 +455,7 @@ EXPORT_SYMBOL_GPL(phy_configure); * Returns: 0 if successful, an negative error code otherwise */ int phy_validate(struct phy *phy, enum phy_mode mode, int submode, - union phy_configure_opts *opts) + void *opts) { int ret; diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index 15032f14..8948f94 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -16,8 +16,6 @@ #include <linux/pm_runtime.h> #include <linux/regulator/consumer.h> -#include <linux/phy/phy-mipi-dphy.h> - struct phy; enum phy_mode { @@ -41,15 +39,6 @@ enum phy_mode { PHY_MODE_SATA }; -/** - * union phy_configure_opts - Opaque generic phy configuration - * - * @mipi_dphy: Configuration set applicable for phys supporting - * the MIPI_DPHY phy mode. - */ -union phy_configure_opts { - struct phy_configure_opts_mipi_dphy mipi_dphy; -}; /** * struct phy_ops - set of function pointers for performing phy operations @@ -80,7 +69,7 @@ struct phy_ops { * * Returns: 0 if successful, an negative error code otherwise */ - int (*configure)(struct phy *phy, union phy_configure_opts *opts); + int (*configure)(struct phy *phy, void *opts); /** * @validate: @@ -99,7 +88,7 @@ struct phy_ops { * error code otherwise */ int (*validate)(struct phy *phy, enum phy_mode mode, int submode, - union phy_configure_opts *opts); + void *opts); int (*reset)(struct phy *phy); int (*calibrate)(struct phy *phy); void (*release)(struct phy *phy); @@ -207,9 +196,9 @@ int phy_power_off(struct phy *phy); int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode); #define phy_set_mode(phy, mode) \ phy_set_mode_ext(phy, mode, 0) -int phy_configure(struct phy *phy, union phy_configure_opts *opts); +int phy_configure(struct phy *phy, void *opts); int phy_validate(struct phy *phy, enum phy_mode mode, int submode, - union phy_configure_opts *opts); + void *opts); static inline enum phy_mode phy_get_mode(struct phy *phy) { @@ -354,8 +343,7 @@ static inline int phy_calibrate(struct phy *phy) return -ENOSYS; } -static inline int phy_configure(struct phy *phy, - union phy_configure_opts *opts) +static inline int phy_configure(struct phy *phy, void *opts) { if (!phy) return 0; @@ -364,7 +352,7 @@ static inline int phy_configure(struct phy *phy, } static inline int phy_validate(struct phy *phy, enum phy_mode mode, int submode, - union phy_configure_opts *opts) + void *opts) { if (!phy) return 0;
The phy framework now allows runtime configurations, but only limited to mipi now, and it's not reasonable to introduce user specified configurations into the union phy_configure_opts structure. An simple way is to replace with a void *. We have already got some phy drivers which introduce private phy API for runtime configurations, and with this patch, they can switch to the phy_configure as a replace. Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com> --- drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 4 ++-- drivers/phy/cadence/cdns-dphy.c | 8 ++++---- drivers/phy/phy-core.c | 4 ++-- include/linux/phy/phy.h | 24 ++++++------------------ 4 files changed, 14 insertions(+), 26 deletions(-)