Re: [PATCH] phy: Change the configuration interface param to void* to make it more general

[Maxime Ripard <maxime.ripard@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 3795 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]