From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753927AbbINPIG (ORCPT ); Mon, 14 Sep 2015 11:08:06 -0400 Received: from mail-bn1bon0065.outbound.protection.outlook.com ([157.56.111.65]:3104 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751127AbbINPID (ORCPT ); Mon, 14 Sep 2015 11:08:03 -0400 X-Greylist: delayed 558 seconds by postgrey-1.27 at vger.kernel.org; Mon, 14 Sep 2015 11:08:03 EDT Authentication-Results: spf=pass (sender IP is 149.199.60.83) smtp.mailfrom=xilinx.com; rock-chips.com; dkim=none (message not signed) header.d=none;rock-chips.com; dmarc=bestguesspass action=none header.from=xilinx.com; Date: Mon, 14 Sep 2015 08:07:51 -0700 From: =?utf-8?B?U8O2cmVu?= Brinkmann To: Shawn Lin CC: Michal Simek , Ulf Hansson , , , Subject: Re: [PATCH v2 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan Message-ID: <20150914150751.GI19678@xsjsorenbubuntu> References: <1442212161-23234-1-git-send-email-shawn.lin@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1442212161-23234-1-git-send-email-shawn.lin@rock-chips.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-TM-AS-Product-Ver: IMSS-7.1.0.1224-8.0.0.1202-21812.005 X-TM-AS-User-Approved-Sender: Yes;Yes X-EOPAttributedMessage: 0 X-Microsoft-Exchange-Diagnostics: 1;BN1BFFO11FD035;1:9QGkMmWdPw+hyeRlgL1Q3MxSoqMcKPKF9ZRw1hWqYZFUE58/ACIA4tSoHwsH55GxniQl1UDVdJeiQQjS2ocFAsvsDxhKjwWNswKNQFk6/p0U/MNX/ZyG1MoqBHntGVeQ8h0QxyVq/DnyuZR867A6edpwwGR18TlIE22hWg1PK+o5+LxoZq6fakyHY8gGxZV/hsf9pMMIwD7YNb0pbw9iysSpu8RISzJX1fpe6VPB9roHS+bP+NX0c2culzUnBWzNjo0ZzVhiYYNbfMUbCy+XuOLScDGl3cnnbW/MFoVojf+/8dDqq0/PdYwjPMH9S5dgSDVxhlW2VuWeYzOz6+CbZ2c7jeyhZHiPyzjsLPtVMBw= X-Forefront-Antispam-Report: CIP:149.199.60.83;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10009020)(6009001)(2980300002)(438002)(189002)(377424004)(199003)(52044002)(24454002)(23676002)(76176999)(83506001)(5001960100002)(54356999)(19580405001)(5001860100001)(87936001)(16796002)(64706001)(6806004)(85182001)(33716001)(36386004)(50986999)(63266004)(4001350100001)(86362001)(5001830100001)(76506005)(33656002)(189998001)(81156007)(50466002)(4001540100001)(230783001)(11100500001)(19580395003)(57986006)(106466001)(110136002)(2950100001)(47776003)(62966003)(46102003)(5007970100001)(77096005)(85202003)(77156002)(92566002)(107986001);DIR:OUT;SFP:1101;SCL:1;SRVR:BN1BFFO11HUB031;H:xsj-pvapsmtpgw01;FPR:;SPF:Pass;PTR:unknown-60-83.xilinx.com;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;BN1BFFO11HUB031;2:RazgDGQoGCif20kQQyNN7MBcvJn2Tdr8etFL8gXrwUycBZI5LRYRzG44EHT+EuHtZXh38a3IzyTRMmONDCdqKhTSpPOEJ3sAh/hfXqaFcfkJJEURQJkpTVfLbmGSSwLV3GKAIMHkUIuwUmsXD7ZxUDKlLW38554Wx5h8WZEen8s=;3:C1jkbF6Hz2wqy30PFNMr1isstZqWBvzXC3fOXv3gnq+pUx2czX6HvyENgkku9BZhJa7krsj9vMp8NiXiuqXam1H7xk8CMVXdE+34DbEumvju/D0LWzFhEBy8+2wAqvKzziRN/eLZoAER2SPHgL3tnsQdEzF0IB4CrIk2fA+q6XsxNsGR6CH2eGojYWUtYqMRfaXh/C5uYLHxNuj/jRZO84qTmwmgAVYacgsv4lyjoVc+vQ5gZq2PeVKiH1VEfFaDjwTQ5H736/0B6ycy3jmn8g==;25:ECRGjZ1i4lhQYvuIN1qhAkeSGZ1py8Q1lcD9tR1eyMtpCL+UDfiscWch+3E0QXkfBqjedXgd6R72YHnPqsmWe2/WR6Y3olrygoZ7KJm6Te9b8osraxg3BGgh580DOjh27p0AjB2ii2gbSW9KiG70mqwM3PbckdyZfeKQ2nea/wjdRI02LimnqtAFiXObhxJIxqGN9ObiPgbHEH/i/kbvSFWzBLV1IZqo0bUl4K1l22RhsLo++tL7wY8+w6pOUBIddI3u/zjfL2q0hQ00bZSruA== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(8251501001);SRVR:BN1BFFO11HUB031; X-Microsoft-Exchange-Diagnostics: 1;BN1BFFO11HUB031;20:c3fxlRGVxD39yMGNkmXYki6pj7pvRRIx4MH28SHT6/P073ncNW4gnfSNkj+38PLKtUR4zAb4AFVKb/zIAAYOe85goRGIVGZ0Um3XNS/IzeKQf9sZFIpqegvQ2w3YH2Xk4QTopxAoqVEIEcsFXmP3i76dCD6OmXHNabFeMqcQX+YkEjWaYnnVtV40SI1xbTNXMdBJx1A4y/g3oGV6kKakybhiLIe8Bf0hGVJbJIozAdWAv8j2CDECyVxaNPoxpAu9Ujy13FZC8Sed6Y8OYVlIF/i8y86WMr9tYjoXZxsY2E9JFcx0yzCAqUW6DPjWUnam23AvviACDjOm5LytXJ3awKxm0qR3bO1H8JDtI5RaYfoEOmH4OYHXE8R5OKQ0LfL5v557iZL07ZFk53PdrpoPMz8XaP8iGyxzqZQfMR7DIOIMdPrcSGGKyCJPgJkKhdLkJS3DqQnT35LZ58fVpHEr7Bnk21BQSwu7eXv8TFqxO+k0G8bjdoYpvyRUta13BYJm;4:OIBg/a//HDj5VCArhqM2W53fO6/a4Bnk3r5LfcN6c/ID0rvB9zsKzpr0f9srhPlyNIDbAit5W/kgl5IBtufocFmbyRIUUuXFqpe8vPguz6o/bJVo8Clfh41jDpWxuIZZLtO2KaE7sv9Q2JNG7i7mi9KjiHnu3gzQL6WbHBuKOqrYsweMB3aSeiPwSrrNpEUeh+CrdpuHXsbhqXT2ssX8tC+xRYRt64lskSfaxvwKWM4vTGraaHCmfvvjD/B5NrXC6R/JnAn1shIMF1LUBb9WFy67fa2ziWzeAfUK0x+jwxcI1OqjulrKeom67Mo1LSHLCS+S1BWdQhibxJ3iBH+Rf7rUvny1g0K5D6U26mPVX2U= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(520029)(8121501046)(520048)(5005006)(3002001);SRVR:BN1BFFO11HUB031;BCL:0;PCL:0;RULEID:;SRVR:BN1BFFO11HUB031; X-Forefront-PRVS: 0699FCD394 X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtCTjFCRkZPMTFIVUIwMzE7MjM6dUl0UUZPZHExSzBpc2ZZbk91YmNycHJR?= =?utf-8?B?NTlYRW5jY3cyZTJNM0NZdHZ4OVZub0F1Z1lZUTV4TzB5MTBrSDNwTkNySXd2?= =?utf-8?B?SnZVVnE3bXdtUDVuOFFseStoZXRHSDhITUhMM1ZXSFdnUXQ0cVdWbE1hYzRo?= =?utf-8?B?Q2RBRVhJbGZPd0F6NjRWWWhjZTFTWTJUTTllT1c3czRUZHppblJ5V3lCVFFa?= =?utf-8?B?ZDVBTVlxeG90a3J1aUJydCtyYlN0QllPcWpFWC9SbWh5cmMwR0hndXZoSU1Q?= =?utf-8?B?STd3bW10U0huOUFmNVNIVyszaTAzZllCWWlKcWlDVmxoNi9aNVQzaUhUWldj?= =?utf-8?B?enJnWFFETjRPdzR6OWk5c05lc2dTWjMwdVZ0eE1HYWV0bmJQUngva2J1L3Ri?= =?utf-8?B?RlRVMzk1d1A3Q2U5NWJlbGYrUEFqZG1QZjhkR3I1bWs3SVB1b2RqWTI1bGsw?= =?utf-8?B?aWgvOUZKeWxVN25PTThLWFIyYWI4b1FoaFBONjVLWWlieVBoc1ZWMVBUaDdI?= =?utf-8?B?S0lreGljS2VyWlg3eWlTd3U4K0tSTmMzb0xQc0MzdTJTZlVESDYzN0lyYnF5?= =?utf-8?B?OUlCUGpBVWI2SlVzWGh1U2FQbWJpK044WGFvNlI3UnlEclhFT2ZsSmNnbVhk?= =?utf-8?B?TWVIN0NydkdLY2V2SEdZb1ZTT3E4WEdxa2tYNVU1a1RZU2xuMC9FTUJDWnlP?= =?utf-8?B?NG9Zb3Z6WDFBNjV2TUNHQktLckp4OE1jbUtFaHNtQlQrQUNWelJIUm1hZVcx?= =?utf-8?B?YUtDRlVEWUlqOTA3WmVON0RNY0tNWllOYmc3NXNPSWl0S3V1eExLa1FTeTFG?= =?utf-8?B?WCtIdCs4Y0hlZzFENEJFaUN5SDhXM1BNZWc4V1pqSTFleGJMRkdRZy9Yd2cx?= =?utf-8?B?ZkZqRVRLbHJkalREcVJiVnZ4RlNndDdhd3RYNVZqaU9qaWtxRlNMWmVqWXla?= =?utf-8?B?TSt1YnVrUW5QYnkweWZqUU42aG5xckhCODB1OTIzTnNqUDFqMllWMEdDMVg3?= =?utf-8?B?eitrQ1dLaEFzU1R3YVlsdVpCODhwL2dHdDk3eU1NbVA3NWFoUEtiRE5wOE5W?= =?utf-8?B?VENzQ3RIbmFacFB0aTNnbEs2UnZ3QmcxTHVvcS9QRTJIczgzZHVhd3dBSW1z?= =?utf-8?B?dVJzWDZGVnppSHl4OGRzVk9USTROSFRhaWZCRVBGYmNZWHlMeGwwQVRCaXQ1?= =?utf-8?B?Q00vcXhxMFFQVGdMdm1zamRwN2Nab1hGYmV5c2xiTythR2YrQXVJajRLWExz?= =?utf-8?B?TVVzUlFSRnVxeVpGdDVBNjhTaWJCNVVEa1NFcHA5emlMZG5jTXh1OU8rYzNX?= =?utf-8?B?aFB2UXkwNk1oR2lXbHp6Q1RPUjBibzZnQzRhREo2dlR6ay9kRjh2VnlicHJ3?= =?utf-8?B?eE9yczhhQVZQRTg5cVg3djVnZllxSDZBTGF5d0R1U3lEOTBySjBuNGM4VnRm?= =?utf-8?B?TUVEeXQ5MEt1RVV4Z3kvdG5FZ000TU1ZWHBBK3FGdXVEa1RyWC8yRGtpZzBJ?= =?utf-8?B?R0tFanVHZnllKytteExPdVd1SmNUSjVHM3ZTVUE2ZS9tVThBNjJ0Kzd5b0NR?= =?utf-8?B?T3FQRDExbFFjcEhUUXZwQ3MxcXBqWGNqMml4amozU0RhM0ZmUHRiRVMzZDJI?= =?utf-8?B?OEJpRnNlY0dKSU1zT3N4RVVhN0FRYmlBM1d3M2Q1bGZaTjEycXdUMmtCbzZy?= =?utf-8?B?b1FSZVYxSWQybEs4YWdNUmMrSEhFNlBkWUVBYnhJcEp1Z0lrVSs1KzREQXV4?= =?utf-8?B?QUlpMEpRUitGQmJ5RGtIQmtnPT0=?= X-Microsoft-Exchange-Diagnostics: 1;BN1BFFO11HUB031;5:ny7REq8xwH5wUjOWlAoSbCuvFFJrRao0pITeenYy1ldkZf3Jr5+kq2Q+JnZe0x0gdjVUZVaMsz2QHcLFZm0tvcirL4jmAoZ8CDQOi9TFposkbGFN7a+Pnjk52uxoahzrx/3BQ1SQ1AYEkfrdBFtSMA==;24:03iYkbbK+i7k5fXsvn8VUe2CMoVI4BDsx1b/5cYp9dgH9c3ZxiFLnFl/u+3RWKlXkWVwBcXa2mrTEOdbRMX7krvlRJEmYWw8zdYJQne20gU= SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: xilinx.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Sep 2015 15:07:58.9177 (UTC) X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.60.83];Helo=[xsj-pvapsmtpgw01] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN1BFFO11HUB031 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Shawn, overall, it looks good to me. I have some questions though. On Mon, 2015-09-14 at 02:29PM +0800, Shawn Lin wrote: > This patch adds Generic PHY access for sdhci-of-arasan. Driver > can get PHY handler from dt-binding, and power-on/init the PHY. > Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled. > Currently, it's just mandatory for arasan,sdhci-5.1. > > Signed-off-by: Shawn Lin > > Serise-changes: 2 > - Keep phy as a mandatory requirement for arasan,sdhci-5.1 > > --- > > Changes in v2: None > > drivers/mmc/host/sdhci-of-arasan.c | 97 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 97 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index 75379cb..2c13ef8 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -21,6 +21,7 @@ > > #include > #include > +#include > #include "sdhci-pltfm.h" > > #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c > @@ -35,6 +36,7 @@ > */ > struct sdhci_arasan_data { > struct clk *clk_ahb; > + struct phy *phy; > }; > > static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host) > @@ -70,6 +72,62 @@ static struct sdhci_pltfm_data sdhci_arasan_pdata = { > > #ifdef CONFIG_PM_SLEEP > /** > + * sdhci_arasan_suspend_phy - Suspend phy method for the driver > + * @phy: Handler of phy structure > + * Returns 0 on success and error value on error > + * > + * Put the phy in a deactive state. > + */ > +static int sdhci_arasan_suspend_phy(struct phy *phy) > +{ > + int ret; > + > + ret = phy_exit(phy); > + if (ret < 0) > + goto err_phy_exit; > + > + ret = phy_power_off(phy); > + if (ret < 0) > + goto err_phy_pwr_off; > + > + return 0; > + > +err_phy_pwr_off: > + phy_power_on(phy); > +err_phy_exit: > + phy_init(phy); Just to confirm, are these actions in the error path correct? E.g. if the power_off() call fails, is it safe to call power_on()? Isn't the phy still powered on? (this would apply to other error paths too) > + return ret; > +} > + > +/** > + * sdhci_arasan_resume_phy - Resume phy method for the driver > + * @phy: Handler of phy structure > + * Returns 0 on success and error value on error > + * > + * Put the phy in a active state. > + */ > +static int sdhci_arasan_resume_phy(struct phy *phy) > +{ > + int ret; > + > + ret = phy_power_on(phy); > + if (ret < 0) > + goto err_phy_pwr_on; > + > + ret = phy_init(phy); > + if (ret < 0) > + goto err_phy_init; > + > + return 0; > + > +err_phy_init: > + phy_exit(phy); > +err_phy_pwr_on: > + phy_power_off(phy); > + return ret; > +} > + > +/** > * sdhci_arasan_suspend - Suspend method for the driver > * @dev: Address of the device structure > * Returns 0 on success and error value on error > @@ -91,6 +149,12 @@ static int sdhci_arasan_suspend(struct device *dev) > clk_disable(pltfm_host->clk); > clk_disable(sdhci_arasan->clk_ahb); > > + if (sdhci_arasan->phy) { > + ret = sdhci_arasan_suspend_phy(sdhci_arasan->phy); > + if (ret < 0) > + return ret; > + } > + > return 0; > } > > @@ -122,6 +186,12 @@ static int sdhci_arasan_resume(struct device *dev) > return ret; > } > > + if (sdhci_arasan->phy) { > + ret = sdhci_arasan_resume_phy(sdhci_arasan->phy); > + if (ret < 0) > + return ret; > + } > + > return sdhci_resume_host(host); > } > #endif /* ! CONFIG_PM_SLEEP */ > @@ -166,6 +236,33 @@ static int sdhci_arasan_probe(struct platform_device *pdev) > goto clk_dis_ahb; > } > > + sdhci_arasan->phy = NULL; > + if (of_device_is_compatible(pdev->dev.of_node, > + "arasan,sdhci-5.1")) { > + sdhci_arasan->phy = devm_phy_get(&pdev->dev, > + "phy_arasan"); > + if (IS_ERR(sdhci_arasan->phy)) { > + ret = -ENODEV; > + dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n"); > + goto clk_dis_ahb; > + } > + > + ret = phy_power_on(sdhci_arasan->phy); > + if (ret < 0) { > + dev_err(&pdev->dev, "phy_power_on err.\n"); > + phy_power_off(sdhci_arasan->phy); > + goto clk_dis_ahb; > + } > + > + ret = phy_init(sdhci_arasan->phy); > + if (ret < 0) { > + dev_err(&pdev->dev, "phy_init err.\n"); > + phy_exit(sdhci_arasan->phy); > + phy_power_off(sdhci_arasan->phy); > + goto clk_dis_ahb; > + } > + } I assume you looked at options for having the error paths in a consolidated location? I guess this may be the nicest solution since all of this is in this conditional block? Feel free to add my Acked-by: Sören Brinkmann Sören