Message ID | 20221118084637.1973838-1-Naresh.Solanki@9elements.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp72337wrr; Fri, 18 Nov 2022 00:54:28 -0800 (PST) X-Google-Smtp-Source: AA0mqf5xYllV/Jt01JINgznlfQnUVcBXZTVE3hkdh9vspnWconDy97MT+PWHaizGbdFAGSwVwwM2 X-Received: by 2002:a17:907:2991:b0:782:6505:dec6 with SMTP id eu17-20020a170907299100b007826505dec6mr5074537ejc.505.1668761667941; Fri, 18 Nov 2022 00:54:27 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668761667; cv=none; d=google.com; s=arc-20160816; b=QuwvQqhDUHefbJPw7Kd2nNIRPyEtyrY4pGWdA9uEoPYQv6NhlgJxgrkEQNVA2iZTW5 gn4Bxny84vk8SZy9dJyToy9X3LwJaTtUrFdFK/y1cOOmxhyJ2eCa/OhT2jsPwRw3y/Zw E4G2wwpyrr7bQ+yRQVYXMJMw+C+vTERLmTKaaar7YFglZi3s28jR0xxafjSRrLMrpG9N bOKY3FnEJapPZIJgNGHwqnQdB0QEZ2KuiizKL0liNqTBt1cPBiC+g17NO37MkrK+jHT0 tYSS5yEwWfU2AXtpsQXYWnvc4LytZuFPoSSqzi/a5Cfo/9sp9EEwplIsRRLWBzcZTVFF M0FA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=3amH7rJnvrs/QaT/Rqb0QDnOSfc+s7uS/cghl6emmVE=; b=pT/dTBSDOvAnPe+y+9+22GMwb4J9lR6VlGN7Vrkv4IiW6uQaOHa96S6a/vFY1cxnIc VeaeyQO5f/uBZ583lh28HBPKeRtAIURQdCR4tx1UyzZ3LUH1Zoa6JxIF2k0SWnkF1OR/ E/4ux0bmP5GYNUq+k+DA9ei/dXjDhjoTOCPokGwjQSe8PQXFZEOAFWRQPlYbfmfo2ERC eDQwSTE8PLCQJIS25p3qcAudWC0S8CvP5/FRmBQjKd5zL9zgpz3bXQeDDz0wbZp/XyqC 5xp2HWkX5gPcsiPU/DQy0HlSQwPnYg2Sl30i88UHrinN2Pf1CCPuT2WdOi76uuhQGZTN 2u7Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@9elements.com header.s=google header.b=CglC4XKe; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=9elements.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id ae13-20020a17090725cd00b007994dcae65fsi2535865ejc.613.2022.11.18.00.54.02; Fri, 18 Nov 2022 00:54:27 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@9elements.com header.s=google header.b=CglC4XKe; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=9elements.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235220AbiKRIsN (ORCPT <rfc822;a1648639935@gmail.com> + 99 others); Fri, 18 Nov 2022 03:48:13 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36726 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S241632AbiKRIrs (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 18 Nov 2022 03:47:48 -0500 Received: from mail-wr1-x42d.google.com (mail-wr1-x42d.google.com [IPv6:2a00:1450:4864:20::42d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 60106273E for <linux-kernel@vger.kernel.org>; Fri, 18 Nov 2022 00:46:43 -0800 (PST) Received: by mail-wr1-x42d.google.com with SMTP id j15so7522477wrq.3 for <linux-kernel@vger.kernel.org>; Fri, 18 Nov 2022 00:46:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9elements.com; s=google; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=3amH7rJnvrs/QaT/Rqb0QDnOSfc+s7uS/cghl6emmVE=; b=CglC4XKerFt+XpCsRxGzQDfDhCRu9S4POpBLAHkc/i3X78+jF3ymvE/0LfYF1JbfxU 0kPlFg1p+lTWUDyw0yjTzCuzhOlxbZ7dWcdyb3tlSnsiwX/vmtW14j+I8J3KRgDnQCdh 8UL1gDwQeTCdTMLqj9a2NQA28esLTemjtbcTIEQW9jfTDQvVN8IyAgXDttDdJDIwkLub bBrAtmCCnwb7yfIs1/Np3dWzvi9VfMjh4qOFfdMVfmIrX8bf4HvSs1d7JHx9PsEf2I4E HutOd22xHFFcErVfveY5ZoW4fX03PxUNkigJ7ihmWzP7lNT9v/+1Y8BOVyjkc5K5bqpg c7jg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=3amH7rJnvrs/QaT/Rqb0QDnOSfc+s7uS/cghl6emmVE=; b=jHXPZuNLgOOrA42dFFAwp/2wtqLzGWsXPdqv1E8Q+Ty7i2BfPkSXI1OkVqfYgW+HQg gah1A12S/9i9bg7Vjc+HZGW31kDDe+5hK7WPoh6T5Z32/k885vLAngOmSXtfMDtdfkwI VIBHCeW5zaO+156xakJDW/gp/GRUM7r07dP257y/ColAdPu2O9WSbYwqH8TXivX6Gklw vun15e0B5PhwicBOYHr0zam7pixsIg/nkVT6+4zlm+BQLfSinWFFkn3bcxoindCK8SqB RMTndIWF3t2wsTOXP13AX/00FyXLWmTvrqO1dbS5XXYNPWj8hsDNXcHizq9y7ZwDrIeU s59g== X-Gm-Message-State: ANoB5pnFb9pxxMISXJMg/fypKAPrDGq0XZkoL1T2Bz5K23dK9Ru1E1fE e+Ygq0duc8DdGuUo2djPiId6wA== X-Received: by 2002:a5d:6d47:0:b0:230:3652:205 with SMTP id k7-20020a5d6d47000000b0023036520205mr3634228wri.322.1668761201902; Fri, 18 Nov 2022 00:46:41 -0800 (PST) Received: from stroh80.sec.9e.network (ip-078-094-000-051.um19.pools.vodafone-ip.de. [78.94.0.51]) by smtp.gmail.com with ESMTPSA id r13-20020a05600c458d00b003c7087f6c9asm8716126wmo.32.2022.11.18.00.46.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 18 Nov 2022 00:46:41 -0800 (PST) From: Naresh Solanki <naresh.solanki@9elements.com> X-Google-Original-From: Naresh Solanki <Naresh.Solanki@9elements.com> To: Guenter Roeck <linux@roeck-us.net>, linux-hwmon@vger.kernel.org, Jean Delvare <jdelvare@suse.com> Cc: Patrick Rudolph <patrick.rudolph@9elements.com>, Naresh Solanki <Naresh.Solanki@9elements.com>, linux-kernel@vger.kernel.org Subject: [PATCH v2] hwmon: pm_bus: core: Implement regulator get_status Date: Fri, 18 Nov 2022 09:46:36 +0100 Message-Id: <20221118084637.1973838-1-Naresh.Solanki@9elements.com> X-Mailer: git-send-email 2.37.3 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749823434593621703?= X-GMAIL-MSGID: =?utf-8?q?1749823434593621703?= |
Series |
[v2] hwmon: pm_bus: core: Implement regulator get_status
|
|
Commit Message
Naresh Solanki
Nov. 18, 2022, 8:46 a.m. UTC
From: Patrick Rudolph <patrick.rudolph@9elements.com> Add get_status for pmbus_regulator_ops. Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> --- drivers/hwmon/pmbus/pmbus_core.c | 72 ++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) base-commit: 27fea302952d8c90cafbdbee96bafeca03544401
Comments
On Fri, Nov 18, 2022 at 09:46:36AM +0100, Naresh Solanki wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > Add get_status for pmbus_regulator_ops. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > --- Please provide change logs. > drivers/hwmon/pmbus/pmbus_core.c | 72 ++++++++++++++++++++++++++++++++ > 1 file changed, 72 insertions(+) > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 7ec04934747e..5dde345c7679 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -2851,6 +2851,77 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned > return 0; > } > > +static int pmbus_regulator_get_status(struct regulator_dev *rdev) > +{ > + struct device *dev = rdev_get_dev(rdev); > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct pmbus_data *data = i2c_get_clientdata(client); > + u8 page = rdev_get_id(rdev); > + int status, status2; > + > + mutex_lock(&data->update_lock); > + status = pmbus_get_status(client, page, PMBUS_STATUS_WORD); > + mutex_unlock(&data->update_lock); I do not see the point of this lock here and elsewhere in this function. If you want to ensure that the status is consistent, you would need to hold the lock over the entire function, not repeatedly acquire and release it. Even then there would be no guarantee that the status is consistent because it can change anytime on the chip side. > + if (status < 0) > + return status; > + > + if (status & (PB_STATUS_VIN_UV | PB_STATUS_IOUT_OC | PB_STATUS_VOUT_OV | > + PB_STATUS_UNKNOWN)) Please align continuation lines with the matching '('. > + return REGULATOR_STATUS_ERROR; > + > + if (status & (PB_STATUS_OFF | PB_STATUS_POWER_GOOD_N)) > + return REGULATOR_STATUS_OFF; > + > + if (status & PB_STATUS_VOUT && > + data->info->func[page] & PMBUS_HAVE_STATUS_VOUT) { > + mutex_lock(&data->update_lock); > + status2 = _pmbus_read_byte_data(client, page, > + PMBUS_STATUS_VOUT); > + mutex_unlock(&data->update_lock); > + if (status2 < 0) > + return status2; > + if (status2 & (PB_VOLTAGE_OV_FAULT | PB_VOLTAGE_UV_FAULT)) > + return REGULATOR_STATUS_ERROR; > + } > + if (status & PB_STATUS_IOUT_POUT && > + data->info->func[page] & PMBUS_HAVE_STATUS_IOUT) { > + mutex_lock(&data->update_lock); > + status2 = _pmbus_read_byte_data(client, page, > + PMBUS_STATUS_IOUT); Ok to avoid continuation lines as long as the result has less then 100 columns. > + mutex_unlock(&data->update_lock); > + if (status2 < 0) > + return status2; > + if (status2 & (PB_POUT_OP_FAULT | PB_IOUT_UC_FAULT | > + PB_IOUT_OC_LV_FAULT | PB_IOUT_OC_FAULT)) > + return REGULATOR_STATUS_ERROR; > + } > + if (status & PB_STATUS_INPUT && > + data->info->func[page] & PMBUS_HAVE_STATUS_INPUT) { > + mutex_lock(&data->update_lock); > + status2 = _pmbus_read_byte_data(client, page, > + PMBUS_STATUS_INPUT); > + mutex_unlock(&data->update_lock); > + if (status2 < 0) > + return status2; > + if (status2 & (PB_IIN_OC_FAULT | PB_VOLTAGE_OV_FAULT | > + PB_VOLTAGE_UV_FAULT)) > + return REGULATOR_STATUS_ERROR; > + } > + if (status & PB_STATUS_TEMPERATURE && > + data->info->func[page] & PMBUS_HAVE_STATUS_TEMP) { > + mutex_lock(&data->update_lock); > + status2 = _pmbus_read_byte_data(client, page, > + PMBUS_STATUS_TEMPERATURE); > + mutex_unlock(&data->update_lock); > + if (status2 < 0) > + return status2; > + if (status2 & (PB_TEMP_UT_FAULT | PB_TEMP_OT_FAULT)) > + return REGULATOR_STATUS_ERROR; > + } > + > + return REGULATOR_STATUS_ON; > +} > + > static int pmbus_regulator_get_low_margin(struct i2c_client *client, int page) > { > struct pmbus_data *data = i2c_get_clientdata(client); > @@ -2991,6 +3062,7 @@ const struct regulator_ops pmbus_regulator_ops = { > .disable = pmbus_regulator_disable, > .is_enabled = pmbus_regulator_is_enabled, > .get_error_flags = pmbus_regulator_get_error_flags, > + .get_status = pmbus_regulator_get_status, > .get_voltage = pmbus_regulator_get_voltage, > .set_voltage = pmbus_regulator_set_voltage, > .list_voltage = pmbus_regulator_list_voltage, > > base-commit: 27fea302952d8c90cafbdbee96bafeca03544401 > -- > 2.37.3 >
On Fri, Nov 18, 2022 at 09:46:36AM +0100, Naresh Solanki wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > Add get_status for pmbus_regulator_ops. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> Just noticed: The subject should start with "hwmon: (pmbus/core)" Thanks, Guenter > --- > drivers/hwmon/pmbus/pmbus_core.c | 72 ++++++++++++++++++++++++++++++++ > 1 file changed, 72 insertions(+) > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 7ec04934747e..5dde345c7679 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -2851,6 +2851,77 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned > return 0; > } > > +static int pmbus_regulator_get_status(struct regulator_dev *rdev) > +{ > + struct device *dev = rdev_get_dev(rdev); > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct pmbus_data *data = i2c_get_clientdata(client); > + u8 page = rdev_get_id(rdev); > + int status, status2; > + > + mutex_lock(&data->update_lock); > + status = pmbus_get_status(client, page, PMBUS_STATUS_WORD); > + mutex_unlock(&data->update_lock); > + if (status < 0) > + return status; > + > + if (status & (PB_STATUS_VIN_UV | PB_STATUS_IOUT_OC | PB_STATUS_VOUT_OV | > + PB_STATUS_UNKNOWN)) > + return REGULATOR_STATUS_ERROR; > + > + if (status & (PB_STATUS_OFF | PB_STATUS_POWER_GOOD_N)) > + return REGULATOR_STATUS_OFF; > + > + if (status & PB_STATUS_VOUT && > + data->info->func[page] & PMBUS_HAVE_STATUS_VOUT) { > + mutex_lock(&data->update_lock); > + status2 = _pmbus_read_byte_data(client, page, > + PMBUS_STATUS_VOUT); > + mutex_unlock(&data->update_lock); > + if (status2 < 0) > + return status2; > + if (status2 & (PB_VOLTAGE_OV_FAULT | PB_VOLTAGE_UV_FAULT)) > + return REGULATOR_STATUS_ERROR; > + } > + if (status & PB_STATUS_IOUT_POUT && > + data->info->func[page] & PMBUS_HAVE_STATUS_IOUT) { > + mutex_lock(&data->update_lock); > + status2 = _pmbus_read_byte_data(client, page, > + PMBUS_STATUS_IOUT); > + mutex_unlock(&data->update_lock); > + if (status2 < 0) > + return status2; > + if (status2 & (PB_POUT_OP_FAULT | PB_IOUT_UC_FAULT | > + PB_IOUT_OC_LV_FAULT | PB_IOUT_OC_FAULT)) > + return REGULATOR_STATUS_ERROR; > + } > + if (status & PB_STATUS_INPUT && > + data->info->func[page] & PMBUS_HAVE_STATUS_INPUT) { > + mutex_lock(&data->update_lock); > + status2 = _pmbus_read_byte_data(client, page, > + PMBUS_STATUS_INPUT); > + mutex_unlock(&data->update_lock); > + if (status2 < 0) > + return status2; > + if (status2 & (PB_IIN_OC_FAULT | PB_VOLTAGE_OV_FAULT | > + PB_VOLTAGE_UV_FAULT)) > + return REGULATOR_STATUS_ERROR; > + } > + if (status & PB_STATUS_TEMPERATURE && > + data->info->func[page] & PMBUS_HAVE_STATUS_TEMP) { > + mutex_lock(&data->update_lock); > + status2 = _pmbus_read_byte_data(client, page, > + PMBUS_STATUS_TEMPERATURE); > + mutex_unlock(&data->update_lock); > + if (status2 < 0) > + return status2; > + if (status2 & (PB_TEMP_UT_FAULT | PB_TEMP_OT_FAULT)) > + return REGULATOR_STATUS_ERROR; > + } > + > + return REGULATOR_STATUS_ON; > +} > + > static int pmbus_regulator_get_low_margin(struct i2c_client *client, int page) > { > struct pmbus_data *data = i2c_get_clientdata(client); > @@ -2991,6 +3062,7 @@ const struct regulator_ops pmbus_regulator_ops = { > .disable = pmbus_regulator_disable, > .is_enabled = pmbus_regulator_is_enabled, > .get_error_flags = pmbus_regulator_get_error_flags, > + .get_status = pmbus_regulator_get_status, > .get_voltage = pmbus_regulator_get_voltage, > .set_voltage = pmbus_regulator_set_voltage, > .list_voltage = pmbus_regulator_list_voltage, > > base-commit: 27fea302952d8c90cafbdbee96bafeca03544401 > -- > 2.37.3 >
Hi Guenter, On 18-11-2022 05:40 pm, Guenter Roeck wrote: > On Fri, Nov 18, 2022 at 09:46:36AM +0100, Naresh Solanki wrote: >> From: Patrick Rudolph <patrick.rudolph@9elements.com> >> >> Add get_status for pmbus_regulator_ops. >> >> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> >> --- > > Please provide change logs. Sure > >> drivers/hwmon/pmbus/pmbus_core.c | 72 ++++++++++++++++++++++++++++++++ >> 1 file changed, 72 insertions(+) >> >> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c >> index 7ec04934747e..5dde345c7679 100644 >> --- a/drivers/hwmon/pmbus/pmbus_core.c >> +++ b/drivers/hwmon/pmbus/pmbus_core.c >> @@ -2851,6 +2851,77 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned >> return 0; >> } >> >> +static int pmbus_regulator_get_status(struct regulator_dev *rdev) >> +{ >> + struct device *dev = rdev_get_dev(rdev); >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct pmbus_data *data = i2c_get_clientdata(client); >> + u8 page = rdev_get_id(rdev); >> + int status, status2; >> + >> + mutex_lock(&data->update_lock); >> + status = pmbus_get_status(client, page, PMBUS_STATUS_WORD); >> + mutex_unlock(&data->update_lock); > > I do not see the point of this lock here and elsewhere in this function. > If you want to ensure that the status is consistent, you would need to > hold the lock over the entire function, not repeatedly acquire and release > it. Even then there would be no guarantee that the status is consistent > because it can change anytime on the chip side. Will hold the lock till end of function. Agree that chip side, status may change any moment. With this I got a question i.e., what should regulator status be reported if regulator had previously encountered some fault but currently while reading the status found that pgood bit indicate output power is good. Should we continue to still report historical fault or instead report current regulator output status based on pgood ? IMHO, it should report regulator status as ON if pgood bit is clear(i.e., output is good) & leave previously encountered error reporting to get_error_flag function. I need your Suggestion here. > >> + if (status < 0) >> + return status; >> + >> + if (status & (PB_STATUS_VIN_UV | PB_STATUS_IOUT_OC | PB_STATUS_VOUT_OV | >> + PB_STATUS_UNKNOWN)) > > Please align continuation lines with the matching '('. > Sure. >> + return REGULATOR_STATUS_ERROR; >> + >> + if (status & (PB_STATUS_OFF | PB_STATUS_POWER_GOOD_N)) >> + return REGULATOR_STATUS_OFF; >> + >> + if (status & PB_STATUS_VOUT && >> + data->info->func[page] & PMBUS_HAVE_STATUS_VOUT) { >> + mutex_lock(&data->update_lock); >> + status2 = _pmbus_read_byte_data(client, page, >> + PMBUS_STATUS_VOUT); >> + mutex_unlock(&data->update_lock); >> + if (status2 < 0) >> + return status2; >> + if (status2 & (PB_VOLTAGE_OV_FAULT | PB_VOLTAGE_UV_FAULT)) >> + return REGULATOR_STATUS_ERROR; >> + } >> + if (status & PB_STATUS_IOUT_POUT && >> + data->info->func[page] & PMBUS_HAVE_STATUS_IOUT) { >> + mutex_lock(&data->update_lock); >> + status2 = _pmbus_read_byte_data(client, page, >> + PMBUS_STATUS_IOUT); > > Ok to avoid continuation lines as long as the result has less then 100 > columns. > Sure >> + mutex_unlock(&data->update_lock); >> + if (status2 < 0) >> + return status2; >> + if (status2 & (PB_POUT_OP_FAULT | PB_IOUT_UC_FAULT | >> + PB_IOUT_OC_LV_FAULT | PB_IOUT_OC_FAULT)) >> + return REGULATOR_STATUS_ERROR; >> + } >> + if (status & PB_STATUS_INPUT && >> + data->info->func[page] & PMBUS_HAVE_STATUS_INPUT) { >> + mutex_lock(&data->update_lock); >> + status2 = _pmbus_read_byte_data(client, page, >> + PMBUS_STATUS_INPUT); >> + mutex_unlock(&data->update_lock); >> + if (status2 < 0) >> + return status2; >> + if (status2 & (PB_IIN_OC_FAULT | PB_VOLTAGE_OV_FAULT | >> + PB_VOLTAGE_UV_FAULT)) >> + return REGULATOR_STATUS_ERROR; >> + } >> + if (status & PB_STATUS_TEMPERATURE && >> + data->info->func[page] & PMBUS_HAVE_STATUS_TEMP) { >> + mutex_lock(&data->update_lock); >> + status2 = _pmbus_read_byte_data(client, page, >> + PMBUS_STATUS_TEMPERATURE); >> + mutex_unlock(&data->update_lock); >> + if (status2 < 0) >> + return status2; >> + if (status2 & (PB_TEMP_UT_FAULT | PB_TEMP_OT_FAULT)) >> + return REGULATOR_STATUS_ERROR; >> + } >> + >> + return REGULATOR_STATUS_ON; >> +} >> + >> static int pmbus_regulator_get_low_margin(struct i2c_client *client, int page) >> { >> struct pmbus_data *data = i2c_get_clientdata(client); >> @@ -2991,6 +3062,7 @@ const struct regulator_ops pmbus_regulator_ops = { >> .disable = pmbus_regulator_disable, >> .is_enabled = pmbus_regulator_is_enabled, >> .get_error_flags = pmbus_regulator_get_error_flags, >> + .get_status = pmbus_regulator_get_status, >> .get_voltage = pmbus_regulator_get_voltage, >> .set_voltage = pmbus_regulator_set_voltage, >> .list_voltage = pmbus_regulator_list_voltage, >> >> base-commit: 27fea302952d8c90cafbdbee96bafeca03544401 >> -- >> 2.37.3 >>
On Fri, Nov 18, 2022 at 08:32:26PM +0530, Naresh Solanki wrote: > > > + > > > + mutex_lock(&data->update_lock); > > > + status = pmbus_get_status(client, page, PMBUS_STATUS_WORD); > > > + mutex_unlock(&data->update_lock); > > > > I do not see the point of this lock here and elsewhere in this function. > > If you want to ensure that the status is consistent, you would need to > > hold the lock over the entire function, not repeatedly acquire and release > > it. Even then there would be no guarantee that the status is consistent > > because it can change anytime on the chip side. > Will hold the lock till end of function. Agree that chip side, status may > change any moment. > > With this I got a question i.e., what should regulator status be reported if > regulator had previously encountered some fault but currently while reading > the status found that pgood bit indicate output power is good. > Should we continue to still report historical fault or instead report > current regulator output status based on pgood ? > > IMHO, it should report regulator status as ON if pgood bit is clear(i.e., > output is good) & leave previously encountered error reporting to > get_error_flag function. > > I need your Suggestion here. > That is really a question for the regulator subsystem. Personally I think the current status should be reported, but that is just me. Guenter
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 7ec04934747e..5dde345c7679 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -2851,6 +2851,77 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned return 0; } +static int pmbus_regulator_get_status(struct regulator_dev *rdev) +{ + struct device *dev = rdev_get_dev(rdev); + struct i2c_client *client = to_i2c_client(dev->parent); + struct pmbus_data *data = i2c_get_clientdata(client); + u8 page = rdev_get_id(rdev); + int status, status2; + + mutex_lock(&data->update_lock); + status = pmbus_get_status(client, page, PMBUS_STATUS_WORD); + mutex_unlock(&data->update_lock); + if (status < 0) + return status; + + if (status & (PB_STATUS_VIN_UV | PB_STATUS_IOUT_OC | PB_STATUS_VOUT_OV | + PB_STATUS_UNKNOWN)) + return REGULATOR_STATUS_ERROR; + + if (status & (PB_STATUS_OFF | PB_STATUS_POWER_GOOD_N)) + return REGULATOR_STATUS_OFF; + + if (status & PB_STATUS_VOUT && + data->info->func[page] & PMBUS_HAVE_STATUS_VOUT) { + mutex_lock(&data->update_lock); + status2 = _pmbus_read_byte_data(client, page, + PMBUS_STATUS_VOUT); + mutex_unlock(&data->update_lock); + if (status2 < 0) + return status2; + if (status2 & (PB_VOLTAGE_OV_FAULT | PB_VOLTAGE_UV_FAULT)) + return REGULATOR_STATUS_ERROR; + } + if (status & PB_STATUS_IOUT_POUT && + data->info->func[page] & PMBUS_HAVE_STATUS_IOUT) { + mutex_lock(&data->update_lock); + status2 = _pmbus_read_byte_data(client, page, + PMBUS_STATUS_IOUT); + mutex_unlock(&data->update_lock); + if (status2 < 0) + return status2; + if (status2 & (PB_POUT_OP_FAULT | PB_IOUT_UC_FAULT | + PB_IOUT_OC_LV_FAULT | PB_IOUT_OC_FAULT)) + return REGULATOR_STATUS_ERROR; + } + if (status & PB_STATUS_INPUT && + data->info->func[page] & PMBUS_HAVE_STATUS_INPUT) { + mutex_lock(&data->update_lock); + status2 = _pmbus_read_byte_data(client, page, + PMBUS_STATUS_INPUT); + mutex_unlock(&data->update_lock); + if (status2 < 0) + return status2; + if (status2 & (PB_IIN_OC_FAULT | PB_VOLTAGE_OV_FAULT | + PB_VOLTAGE_UV_FAULT)) + return REGULATOR_STATUS_ERROR; + } + if (status & PB_STATUS_TEMPERATURE && + data->info->func[page] & PMBUS_HAVE_STATUS_TEMP) { + mutex_lock(&data->update_lock); + status2 = _pmbus_read_byte_data(client, page, + PMBUS_STATUS_TEMPERATURE); + mutex_unlock(&data->update_lock); + if (status2 < 0) + return status2; + if (status2 & (PB_TEMP_UT_FAULT | PB_TEMP_OT_FAULT)) + return REGULATOR_STATUS_ERROR; + } + + return REGULATOR_STATUS_ON; +} + static int pmbus_regulator_get_low_margin(struct i2c_client *client, int page) { struct pmbus_data *data = i2c_get_clientdata(client); @@ -2991,6 +3062,7 @@ const struct regulator_ops pmbus_regulator_ops = { .disable = pmbus_regulator_disable, .is_enabled = pmbus_regulator_is_enabled, .get_error_flags = pmbus_regulator_get_error_flags, + .get_status = pmbus_regulator_get_status, .get_voltage = pmbus_regulator_get_voltage, .set_voltage = pmbus_regulator_set_voltage, .list_voltage = pmbus_regulator_list_voltage,