From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932350AbdJWPXJ (ORCPT ); Mon, 23 Oct 2017 11:23:09 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:46776 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932111AbdJWPXH (ORCPT ); Mon, 23 Oct 2017 11:23:07 -0400 Subject: Re: drivers (pmbus): ir35221: Set PMBUS_PAGE before reading id and model To: Guenter Roeck Cc: jdelvare@suse.com, linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org, andrew@aj.id.au, "Edward A. James" References: <1508446628-13112-1-git-send-email-eajames@linux.vnet.ibm.com> <20171021161013.GA12794@roeck-us.net> From: Eddie James Date: Mon, 23 Oct 2017 10:23:02 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <20171021161013.GA12794@roeck-us.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-TM-AS-GCONF: 00 x-cbid: 17102315-0024-0000-0000-000017605011 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007939; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000239; SDB=6.00935339; UDB=6.00471230; IPR=6.00715605; BA=6.00005656; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017670; XFM=3.00000015; UTC=2017-10-23 15:23:06 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17102315-0025-0000-0000-00004D373CD8 Message-Id: <1911942e-f770-dbff-969c-b98ed69f68ea@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-10-23_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1710230216 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/21/2017 11:10 AM, Guenter Roeck wrote: > On Thu, Oct 19, 2017 at 03:57:08PM -0500, eajames@linux.vnet.ibm.com wrote: >> From: "Edward A. James" >> >> The MFR_ID and MFR_MODEL, which are manually read before probing the >> pmbus core, are only valid for the two pages that the ir35221 has >> available. Since we don't know the state of the device when we start >> probing, set the page number first before reading id and model. >> > If the device only has two pages, it is highly unlikely that it is possible > to select another page which is not supported; any attempts to do so should > result in a command error. > > Is this theory or based on an actual problem observed ? If so, it will require > some additional explanation. Hi, Yes, I have observed this with a physical ir35221 device off an i2c bus. It seems that there are some places in the PMBUS core that manage to set the page number to 0xFF due to pmbus_set_page being called with page=-1. pmbus_set_page doesn't check the CML register, so if the i2c op succeeds, it will be a success. I can also set it to anything with i2cset tool without error. I would also like to fix calling pmbus_set_page with -1, and I will send up a patch if/when I have time. But I felt it makes sense to also ensure the page is set correctly in the ir35221 driver, since it's an unknown state at probe time, and this device appears a bit "weird" in that it doesn't return an error if it's an unsupported page. Thanks, Eddie > > Thanks, > Guenter > >> Signed-off-by: Edward A. James >> --- >> drivers/hwmon/pmbus/ir35221.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/hwmon/pmbus/ir35221.c b/drivers/hwmon/pmbus/ir35221.c >> index 8b906b4..9a53930 100644 >> --- a/drivers/hwmon/pmbus/ir35221.c >> +++ b/drivers/hwmon/pmbus/ir35221.c >> @@ -267,6 +267,12 @@ static int ir35221_probe(struct i2c_client *client, >> | I2C_FUNC_SMBUS_READ_BLOCK_DATA)) >> return -ENODEV; >> >> + ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 0); >> + if (ret < 0) { >> + dev_err(&client->dev, "Failed to set PMBUS_PAGE\n"); >> + return ret; >> + } >> + >> ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf); >> if (ret < 0) { >> dev_err(&client->dev, "Failed to read PMBUS_MFR_ID\n");