From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934141Ab3GWTgE (ORCPT ); Tue, 23 Jul 2013 15:36:04 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:56843 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S933970Ab3GWTgB (ORCPT ); Tue, 23 Jul 2013 15:36:01 -0400 Date: Tue, 23 Jul 2013 15:36:00 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Tomasz Figa cc: Greg KH , Kishon Vijay Abraham I , Tomasz Figa , Laurent Pinchart , , Sylwester Nawrocki , Sascha Hauer , , , , , , , , , , , , , , , , , , , , , , , , Stephen Warren , , Daniel Lezcano Subject: Re: [PATCH 01/15] drivers: phy: add generic PHY framework In-Reply-To: <1446965.6APW5ZgLBW@amdc1227> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 23 Jul 2013, Tomasz Figa wrote: > IMHO it would be better if you provided some code example, but let's try to > check if I understood you correctly. > > 8><------------------------------------------------------------------------ > > [Board file] > > static struct phy my_phy; > > static struct platform_device phy_pdev = { > /* ... */ > .platform_data = &my_phy; > /* ... */ > }; > > static struct platform_device phy_pdev = { This should be controller_pdev, not phy_pdev, yes? > /* ... */ > .platform_data = &my_phy; > /* ... */ > }; > > [Provider driver] > > struct phy *phy = pdev->dev.platform_data; > > ret = phy_create(phy); > > [Consumer driver] > > struct phy *phy = pdev->dev.platform_data; > > ret = phy_get(&pdev->dev, phy); Or even just phy_get(&pdev->dev), because phy_get() could be smart enough to to set phy = dev->platform_data. > ------------------------------------------------------------------------><8 > > Is this what you mean? That's what I was going to suggest too. The struct phy is defined in the board file, which already knows about all the PHYs that exist in the system. (Or perhaps it is allocated dynamically, so that when many board files are present in the same kernel, only the entries listed in the board file for the current system get created.) Then the structure's address is stored in the platform data and made available to both the provider and the consumer. Even though the struct phy is defined (or allocated) in the board file, its contents don't get filled in until the PHY driver provides the details. > It's technically correct, but quality of this solution isn't really nice, > because it's a layering violation (at least if I understood what you mean). > This is because you need to have full definition of struct phy in board file > and a structure that is used as private data in PHY core comes from > platform code. You don't have to have a full definition in the board file. Just a partial definition -- most of the contents can be filled in later, when the PHY driver is ready to store the private data. It's not a layering violation for one region of the kernel to store private data in a structure defined by another part of the kernel. This happens all the time (e.g., dev_set_drvdata). Alan Stern From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alan Stern Date: Tue, 23 Jul 2013 19:36:00 +0000 Subject: Re: [PATCH 01/15] drivers: phy: add generic PHY framework Message-Id: List-Id: In-Reply-To: <1446965.6APW5ZgLBW@amdc1227> References: <1374129984-765-2-git-send-email-kishon@ti.com> In-Reply-To: <1374129984-765-2-git-send-email-kishon@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org On Tue, 23 Jul 2013, Tomasz Figa wrote: > IMHO it would be better if you provided some code example, but let's try to > check if I understood you correctly. > > 8><------------------------------------------------------------------------ > > [Board file] > > static struct phy my_phy; > > static struct platform_device phy_pdev = { > /* ... */ > .platform_data = &my_phy; > /* ... */ > }; > > static struct platform_device phy_pdev = { This should be controller_pdev, not phy_pdev, yes? > /* ... */ > .platform_data = &my_phy; > /* ... */ > }; > > [Provider driver] > > struct phy *phy = pdev->dev.platform_data; > > ret = phy_create(phy); > > [Consumer driver] > > struct phy *phy = pdev->dev.platform_data; > > ret = phy_get(&pdev->dev, phy); Or even just phy_get(&pdev->dev), because phy_get() could be smart enough to to set phy = dev->platform_data. > ------------------------------------------------------------------------><8 > > Is this what you mean? That's what I was going to suggest too. The struct phy is defined in the board file, which already knows about all the PHYs that exist in the system. (Or perhaps it is allocated dynamically, so that when many board files are present in the same kernel, only the entries listed in the board file for the current system get created.) Then the structure's address is stored in the platform data and made available to both the provider and the consumer. Even though the struct phy is defined (or allocated) in the board file, its contents don't get filled in until the PHY driver provides the details. > It's technically correct, but quality of this solution isn't really nice, > because it's a layering violation (at least if I understood what you mean). > This is because you need to have full definition of struct phy in board file > and a structure that is used as private data in PHY core comes from > platform code. You don't have to have a full definition in the board file. Just a partial definition -- most of the contents can be filled in later, when the PHY driver is ready to store the private data. It's not a layering violation for one region of the kernel to store private data in a structure defined by another part of the kernel. This happens all the time (e.g., dev_set_drvdata). Alan Stern From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alan Stern Subject: Re: [PATCH 01/15] drivers: phy: add generic PHY framework Date: Tue, 23 Jul 2013 15:36:00 -0400 (EDT) Message-ID: References: <1446965.6APW5ZgLBW@amdc1227> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Return-path: In-Reply-To: <1446965.6APW5ZgLBW@amdc1227> Sender: linux-doc-owner@vger.kernel.org To: Tomasz Figa Cc: Greg KH , Kishon Vijay Abraham I , Tomasz Figa , Laurent Pinchart , broonie@kernel.org, Sylwester Nawrocki , Sascha Hauer , kyungmin.park@samsung.com, balbi@ti.com, jg1.han@samsung.com, s.nawrocki@samsung.com, kgene.kim@samsung.com, grant.likely@linaro.org, tony@atomide.com, arnd@arndb.de, swarren@nvidia.com, devicetree@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-omap@vger.kernel.org, linux-usb@vger.kernel.org, linux-media@vger.kernel.org, linux-fbdev@vger.kernel.org, akpm@linux-foundation.org, balajitk@ti.com, george.cherian@ti.com, nsekhar@ti.com, olof@lixom.n List-Id: linux-omap@vger.kernel.org On Tue, 23 Jul 2013, Tomasz Figa wrote: > IMHO it would be better if you provided some code example, but let's try to > check if I understood you correctly. > > 8><------------------------------------------------------------------------ > > [Board file] > > static struct phy my_phy; > > static struct platform_device phy_pdev = { > /* ... */ > .platform_data = &my_phy; > /* ... */ > }; > > static struct platform_device phy_pdev = { This should be controller_pdev, not phy_pdev, yes? > /* ... */ > .platform_data = &my_phy; > /* ... */ > }; > > [Provider driver] > > struct phy *phy = pdev->dev.platform_data; > > ret = phy_create(phy); > > [Consumer driver] > > struct phy *phy = pdev->dev.platform_data; > > ret = phy_get(&pdev->dev, phy); Or even just phy_get(&pdev->dev), because phy_get() could be smart enough to to set phy = dev->platform_data. > ------------------------------------------------------------------------><8 > > Is this what you mean? That's what I was going to suggest too. The struct phy is defined in the board file, which already knows about all the PHYs that exist in the system. (Or perhaps it is allocated dynamically, so that when many board files are present in the same kernel, only the entries listed in the board file for the current system get created.) Then the structure's address is stored in the platform data and made available to both the provider and the consumer. Even though the struct phy is defined (or allocated) in the board file, its contents don't get filled in until the PHY driver provides the details. > It's technically correct, but quality of this solution isn't really nice, > because it's a layering violation (at least if I understood what you mean). > This is because you need to have full definition of struct phy in board file > and a structure that is used as private data in PHY core comes from > platform code. You don't have to have a full definition in the board file. Just a partial definition -- most of the contents can be filled in later, when the PHY driver is ready to store the private data. It's not a layering violation for one region of the kernel to store private data in a structure defined by another part of the kernel. This happens all the time (e.g., dev_set_drvdata). Alan Stern From mboxrd@z Thu Jan 1 00:00:00 1970 From: stern@rowland.harvard.edu (Alan Stern) Date: Tue, 23 Jul 2013 15:36:00 -0400 (EDT) Subject: [PATCH 01/15] drivers: phy: add generic PHY framework In-Reply-To: <1446965.6APW5ZgLBW@amdc1227> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, 23 Jul 2013, Tomasz Figa wrote: > IMHO it would be better if you provided some code example, but let's try to > check if I understood you correctly. > > 8><------------------------------------------------------------------------ > > [Board file] > > static struct phy my_phy; > > static struct platform_device phy_pdev = { > /* ... */ > .platform_data = &my_phy; > /* ... */ > }; > > static struct platform_device phy_pdev = { This should be controller_pdev, not phy_pdev, yes? > /* ... */ > .platform_data = &my_phy; > /* ... */ > }; > > [Provider driver] > > struct phy *phy = pdev->dev.platform_data; > > ret = phy_create(phy); > > [Consumer driver] > > struct phy *phy = pdev->dev.platform_data; > > ret = phy_get(&pdev->dev, phy); Or even just phy_get(&pdev->dev), because phy_get() could be smart enough to to set phy = dev->platform_data. > ------------------------------------------------------------------------><8 > > Is this what you mean? That's what I was going to suggest too. The struct phy is defined in the board file, which already knows about all the PHYs that exist in the system. (Or perhaps it is allocated dynamically, so that when many board files are present in the same kernel, only the entries listed in the board file for the current system get created.) Then the structure's address is stored in the platform data and made available to both the provider and the consumer. Even though the struct phy is defined (or allocated) in the board file, its contents don't get filled in until the PHY driver provides the details. > It's technically correct, but quality of this solution isn't really nice, > because it's a layering violation (at least if I understood what you mean). > This is because you need to have full definition of struct phy in board file > and a structure that is used as private data in PHY core comes from > platform code. You don't have to have a full definition in the board file. Just a partial definition -- most of the contents can be filled in later, when the PHY driver is ready to store the private data. It's not a layering violation for one region of the kernel to store private data in a structure defined by another part of the kernel. This happens all the time (e.g., dev_set_drvdata). Alan Stern