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

From: "Zengtao (B)" <prime.zeng@hisilicon.com>
To: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kishon@ti.com" <kishon@ti.com>,
	Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
	Chen-Yu Tsai <wens@csie.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH] phy: Change the configuration interface param to void* to make it more general
Date: Sat, 20 Jul 2019 03:03:20 +0000	[thread overview]
Message-ID: <678F3D1BB717D949B966B68EAEB446ED2FF5D942@DGGEMM506-MBX.china.huawei.com> (raw)
In-Reply-To: <20190717163753.ti6swjfhm7fczcn4@flea>

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel