| Message ID | 20221214080715.2700442-1-Naresh.Solanki@9elements.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RESEND,v6,1/5] hwmon: (pmbus/core): Add interrupt support | expand |
On Wed, Dec 14, 2022 at 09:07:11AM +0100, Naresh Solanki wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > Implement PMBUS irq handler. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> $ scripts/checkpatch.pl --strict index.html CHECK: Blank lines aren't necessary after an open brace '{' #131: FILE: drivers/hwmon/pmbus/pmbus_core.c:3088: + for (i = 0; i < data->info->pages; i++) { + CHECK: Alignment should match open parenthesis #183: FILE: drivers/hwmon/pmbus/pmbus_core.c:3140: + ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler, + 0, "pmbus-irq", data); CHECK: Please use a blank line after function/struct/union/enum declarations #197: FILE: drivers/hwmon/pmbus/pmbus_core.c:3154: } +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) total: 0 errors, 0 warnings, 3 checks, 109 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. index.html has style problems, please review. Please run checkpatch --strict on your patches. Also see Documentation/hwmon/submitting-patches.rst. > --- > drivers/hwmon/pmbus/pmbus.h | 2 +- > drivers/hwmon/pmbus/pmbus_core.c | 84 ++++++++++++++++++++++++++++++++ > 2 files changed, 85 insertions(+), 1 deletion(-) > > > base-commit: 364ffd2537c44cb6914ff5669153f4a86fffad29 > > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h > index 10fb17879f8e..6b2e6cf93b19 100644 > --- a/drivers/hwmon/pmbus/pmbus.h > +++ b/drivers/hwmon/pmbus/pmbus.h > @@ -26,7 +26,7 @@ enum pmbus_regs { > > PMBUS_CAPABILITY = 0x19, > PMBUS_QUERY = 0x1A, > - > + PMBUS_SMBALERT_MASK = 0x1B, > PMBUS_VOUT_MODE = 0x20, > PMBUS_VOUT_COMMAND = 0x21, > PMBUS_VOUT_TRIM = 0x22, > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 95e95783972a..244fd2597252 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data) > > return 0; > } > + > +static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val) > +{ > + return pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8)); > +} > + > +static irqreturn_t pmbus_fault_handler(int irq, void *pdata) > +{ > + struct pmbus_data *data = pdata; > + struct i2c_client *client = to_i2c_client(data->dev); > + int i, status; > + > + for (i = 0; i < data->info->pages; i++) { > + > + mutex_lock(&data->update_lock); > + status = pmbus_read_status_word(client, i); > + if (status < 0) { > + mutex_unlock(&data->update_lock); > + return status; > + } > + > + if (status & ~(PB_STATUS_OFF | PB_STATUS_BUSY | PB_STATUS_POWER_GOOD_N)) > + pmbus_clear_fault_page(client, i); > + > + mutex_unlock(&data->update_lock); > + } > + > + return IRQ_HANDLED; > +} > + > +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) > +{ > + struct device *dev = &client->dev; > + const struct pmbus_regulator_status_category *cat; > + const struct pmbus_regulator_status_assoc *bit; > + int i, j, err, ret, func; > + u8 mask; > + > + for (i = 0; i < data->info->pages; i++) { > + func = data->info->func[i]; > + > + for (j = 0; j < ARRAY_SIZE(pmbus_regulator_flag_map); j++) { > + cat = &pmbus_regulator_flag_map[j]; > + if (!(func & cat->func)) > + continue; > + mask = 0; > + for (bit = cat->bits; bit->pflag; bit++) > + mask |= bit->pflag; > + > + err = pmbus_write_smbalert_mask(client, i, cat->reg, ~mask); > + if (err) > + dev_err(dev, "Failed to set smbalert for reg 0x%02x\n", cat->reg); This concerns me. It might mean that the chip does not support PMBUS_SMBALERT_MASK. If so, there would be lots of error messages. > + } > + > + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_CML, 0xff); > + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_OTHER, 0xff); > + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_MFR_SPECIFIC, 0xff); Why check the return value from pmbus_write_smbalert_mask above but not here ? > + if (data->info->func[i] & PMBUS_HAVE_FAN12) > + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_12, 0xff); > + if (data->info->func[i] & PMBUS_HAVE_FAN34) > + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_34, 0xff); > + } > + > + /* Register notifiers - can fail if IRQ is not given */ The comment does not make sense. pmbus_irq_setup() is not called if the interrupt "is not given". > + ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler, > + 0, "pmbus-irq", data); > + if (ret) { > + dev_warn(dev, "IRQ disabled %d\n", ret); This is not a warning, it is an error. > + return ret; > + } > + > + return 0; > +} > + > #else This is still in regulator code. I said several times that this is not acceptable. > static int pmbus_regulator_register(struct pmbus_data *data) > { > return 0; > } > +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) > +{ > + return 0; > +} > #endif > > static struct dentry *pmbus_debugfs_dir; /* pmbus debugfs directory */ > @@ -3441,6 +3519,12 @@ int pmbus_do_probe(struct i2c_client *client, struct pmbus_driver_info *info) > if (ret) > return ret; > > + if (client->irq > 0) { > + ret = pmbus_irq_setup(client, data); > + if (ret) > + return ret; > + } > + > ret = pmbus_init_debugfs(client, data); > if (ret) > dev_warn(dev, "Failed to register debugfs\n");
Hi Guenter, On 29-12-2022 08:10 pm, Guenter Roeck wrote: > On Wed, Dec 14, 2022 at 09:07:11AM +0100, Naresh Solanki wrote: >> From: Patrick Rudolph <patrick.rudolph@9elements.com> >> >> Implement PMBUS irq handler. >> >> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > > $ scripts/checkpatch.pl --strict index.html > CHECK: Blank lines aren't necessary after an open brace '{' > #131: FILE: drivers/hwmon/pmbus/pmbus_core.c:3088: > + for (i = 0; i < data->info->pages; i++) { > + > > CHECK: Alignment should match open parenthesis > #183: FILE: drivers/hwmon/pmbus/pmbus_core.c:3140: > + ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler, > + 0, "pmbus-irq", data); > > CHECK: Please use a blank line after function/struct/union/enum declarations > #197: FILE: drivers/hwmon/pmbus/pmbus_core.c:3154: > } > +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) > > total: 0 errors, 0 warnings, 3 checks, 109 lines checked > > NOTE: For some of the reported defects, checkpatch may be able to > mechanically convert to the typical style using --fix or --fix-inplace. > > index.html has style problems, please review. > > Please run checkpatch --strict on your patches. > Also see Documentation/hwmon/submitting-patches.rst. I will take care of these errors in the updated version. > >> --- >> drivers/hwmon/pmbus/pmbus.h | 2 +- >> drivers/hwmon/pmbus/pmbus_core.c | 84 ++++++++++++++++++++++++++++++++ >> 2 files changed, 85 insertions(+), 1 deletion(-) >> >> >> base-commit: 364ffd2537c44cb6914ff5669153f4a86fffad29 >> >> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h >> index 10fb17879f8e..6b2e6cf93b19 100644 >> --- a/drivers/hwmon/pmbus/pmbus.h >> +++ b/drivers/hwmon/pmbus/pmbus.h >> @@ -26,7 +26,7 @@ enum pmbus_regs { >> >> PMBUS_CAPABILITY = 0x19, >> PMBUS_QUERY = 0x1A, >> - >> + PMBUS_SMBALERT_MASK = 0x1B, >> PMBUS_VOUT_MODE = 0x20, >> PMBUS_VOUT_COMMAND = 0x21, >> PMBUS_VOUT_TRIM = 0x22, >> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c >> index 95e95783972a..244fd2597252 100644 >> --- a/drivers/hwmon/pmbus/pmbus_core.c >> +++ b/drivers/hwmon/pmbus/pmbus_core.c >> @@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data) >> >> return 0; >> } >> + >> +static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val) >> +{ >> + return pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8)); >> +} >> + >> +static irqreturn_t pmbus_fault_handler(int irq, void *pdata) >> +{ >> + struct pmbus_data *data = pdata; >> + struct i2c_client *client = to_i2c_client(data->dev); >> + int i, status; >> + >> + for (i = 0; i < data->info->pages; i++) { >> + >> + mutex_lock(&data->update_lock); >> + status = pmbus_read_status_word(client, i); >> + if (status < 0) { >> + mutex_unlock(&data->update_lock); >> + return status; >> + } >> + >> + if (status & ~(PB_STATUS_OFF | PB_STATUS_BUSY | PB_STATUS_POWER_GOOD_N)) >> + pmbus_clear_fault_page(client, i); >> + >> + mutex_unlock(&data->update_lock); >> + } >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) >> +{ >> + struct device *dev = &client->dev; >> + const struct pmbus_regulator_status_category *cat; >> + const struct pmbus_regulator_status_assoc *bit; >> + int i, j, err, ret, func; >> + u8 mask; >> + >> + for (i = 0; i < data->info->pages; i++) { >> + func = data->info->func[i]; >> + >> + for (j = 0; j < ARRAY_SIZE(pmbus_regulator_flag_map); j++) { >> + cat = &pmbus_regulator_flag_map[j]; >> + if (!(func & cat->func)) >> + continue; >> + mask = 0; >> + for (bit = cat->bits; bit->pflag; bit++) >> + mask |= bit->pflag; >> + >> + err = pmbus_write_smbalert_mask(client, i, cat->reg, ~mask); >> + if (err) >> + dev_err(dev, "Failed to set smbalert for reg 0x%02x\n", cat->reg); > > This concerns me. It might mean that the chip does not support > PMBUS_SMBALERT_MASK. If so, there would be lots of error messages. After going through the PMBus specification, it appears that this should not be an issue unless there is a violation of the specification. > >> + } >> + >> + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_CML, 0xff); >> + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_OTHER, 0xff); >> + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_MFR_SPECIFIC, 0xff); > > Why check the return value from pmbus_write_smbalert_mask above but not here ? Thank you for pointing out the oversight. I will make sure to include an error check at this point. > >> + if (data->info->func[i] & PMBUS_HAVE_FAN12) >> + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_12, 0xff); >> + if (data->info->func[i] & PMBUS_HAVE_FAN34) >> + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_34, 0xff); >> + } >> + >> + /* Register notifiers - can fail if IRQ is not given */ > > The comment does not make sense. pmbus_irq_setup() is not called > if the interrupt "is not given". Yes. The comment here is not relevant and will be removed. > >> + ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler, >> + 0, "pmbus-irq", data); >> + if (ret) { >> + dev_warn(dev, "IRQ disabled %d\n", ret); > > This is not a warning, it is an error. Thank you for bringing this to my attention. I will make sure to update the code to reflect that this is an error. > >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> #else > > This is still in regulator code. I said several times that this is not > acceptable. Thank you for pointing out the mistake. I will make sure to correct this in the next revision. > >> static int pmbus_regulator_register(struct pmbus_data *data) >> { >> return 0; >> } >> +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) >> +{ >> + return 0; >> +} >> #endif >> >> static struct dentry *pmbus_debugfs_dir; /* pmbus debugfs directory */ >> @@ -3441,6 +3519,12 @@ int pmbus_do_probe(struct i2c_client *client, struct pmbus_driver_info *info) >> if (ret) >> return ret; >> >> + if (client->irq > 0) { >> + ret = pmbus_irq_setup(client, data); >> + if (ret) >> + return ret; >> + } >> + >> ret = pmbus_init_debugfs(client, data); >> if (ret) >> dev_warn(dev, "Failed to register debugfs\n"); Thanks, Naresh
On Tue, Jan 03, 2023 at 12:18:49PM +0530, Naresh Solanki wrote: > Hi Guenter, > > On 29-12-2022 08:10 pm, Guenter Roeck wrote: > > On Wed, Dec 14, 2022 at 09:07:11AM +0100, Naresh Solanki wrote: > > > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > > > > > Implement PMBUS irq handler. > > > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > > > > $ scripts/checkpatch.pl --strict index.html > > CHECK: Blank lines aren't necessary after an open brace '{' > > #131: FILE: drivers/hwmon/pmbus/pmbus_core.c:3088: > > + for (i = 0; i < data->info->pages; i++) { > > + > > > > CHECK: Alignment should match open parenthesis > > #183: FILE: drivers/hwmon/pmbus/pmbus_core.c:3140: > > + ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler, > > + 0, "pmbus-irq", data); > > > > CHECK: Please use a blank line after function/struct/union/enum declarations > > #197: FILE: drivers/hwmon/pmbus/pmbus_core.c:3154: > > } > > +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) > > > > total: 0 errors, 0 warnings, 3 checks, 109 lines checked > > > > NOTE: For some of the reported defects, checkpatch may be able to > > mechanically convert to the typical style using --fix or --fix-inplace. > > > > index.html has style problems, please review. > > > > Please run checkpatch --strict on your patches. > > Also see Documentation/hwmon/submitting-patches.rst. > I will take care of these errors in the updated version. > > > > > --- > > > drivers/hwmon/pmbus/pmbus.h | 2 +- > > > drivers/hwmon/pmbus/pmbus_core.c | 84 ++++++++++++++++++++++++++++++++ > > > 2 files changed, 85 insertions(+), 1 deletion(-) > > > > > > > > > base-commit: 364ffd2537c44cb6914ff5669153f4a86fffad29 > > > > > > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h > > > index 10fb17879f8e..6b2e6cf93b19 100644 > > > --- a/drivers/hwmon/pmbus/pmbus.h > > > +++ b/drivers/hwmon/pmbus/pmbus.h > > > @@ -26,7 +26,7 @@ enum pmbus_regs { > > > PMBUS_CAPABILITY = 0x19, > > > PMBUS_QUERY = 0x1A, > > > - > > > + PMBUS_SMBALERT_MASK = 0x1B, > > > PMBUS_VOUT_MODE = 0x20, > > > PMBUS_VOUT_COMMAND = 0x21, > > > PMBUS_VOUT_TRIM = 0x22, > > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > > > index 95e95783972a..244fd2597252 100644 > > > --- a/drivers/hwmon/pmbus/pmbus_core.c > > > +++ b/drivers/hwmon/pmbus/pmbus_core.c > > > @@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data) > > > return 0; > > > } > > > + > > > +static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val) > > > +{ > > > + return pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8)); > > > +} > > > + > > > +static irqreturn_t pmbus_fault_handler(int irq, void *pdata) > > > +{ > > > + struct pmbus_data *data = pdata; > > > + struct i2c_client *client = to_i2c_client(data->dev); > > > + int i, status; > > > + > > > + for (i = 0; i < data->info->pages; i++) { > > > + > > > + mutex_lock(&data->update_lock); > > > + status = pmbus_read_status_word(client, i); > > > + if (status < 0) { > > > + mutex_unlock(&data->update_lock); > > > + return status; > > > + } > > > + > > > + if (status & ~(PB_STATUS_OFF | PB_STATUS_BUSY | PB_STATUS_POWER_GOOD_N)) > > > + pmbus_clear_fault_page(client, i); > > > + > > > + mutex_unlock(&data->update_lock); > > > + } > > > + > > > + return IRQ_HANDLED; > > > +} > > > + > > > +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) > > > +{ > > > + struct device *dev = &client->dev; > > > + const struct pmbus_regulator_status_category *cat; > > > + const struct pmbus_regulator_status_assoc *bit; > > > + int i, j, err, ret, func; > > > + u8 mask; > > > + > > > + for (i = 0; i < data->info->pages; i++) { > > > + func = data->info->func[i]; > > > + > > > + for (j = 0; j < ARRAY_SIZE(pmbus_regulator_flag_map); j++) { > > > + cat = &pmbus_regulator_flag_map[j]; > > > + if (!(func & cat->func)) > > > + continue; > > > + mask = 0; > > > + for (bit = cat->bits; bit->pflag; bit++) > > > + mask |= bit->pflag; > > > + > > > + err = pmbus_write_smbalert_mask(client, i, cat->reg, ~mask); > > > + if (err) > > > + dev_err(dev, "Failed to set smbalert for reg 0x%02x\n", cat->reg); > > > > This concerns me. It might mean that the chip does not support > > PMBUS_SMBALERT_MASK. If so, there would be lots of error messages. > After going through the PMBus specification, it appears that this should not > be an issue unless there is a violation of the specification. PMBus chips have lots of issues which violate the specification. Have a look at the various drivers and the workarounds implemented there. You'll need to check if the command/register is supported before using it. Also, if you want to keep the error message, make it dev_err_once(). Either case, an error is an error, not to be ignored. An error here should result in an error abort. > > > > > + } > > > + > > > + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_CML, 0xff); > > > + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_OTHER, 0xff); > > > + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_MFR_SPECIFIC, 0xff); > > > > Why check the return value from pmbus_write_smbalert_mask above but not here ? > Thank you for pointing out the oversight. I will make sure to include an > error check at this point. > > > > > + if (data->info->func[i] & PMBUS_HAVE_FAN12) > > > + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_12, 0xff); > > > + if (data->info->func[i] & PMBUS_HAVE_FAN34) > > > + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_34, 0xff); > > > + } > > > + > > > + /* Register notifiers - can fail if IRQ is not given */ > > > > The comment does not make sense. pmbus_irq_setup() is not called > > if the interrupt "is not given". > Yes. The comment here is not relevant and will be removed. > > > > > + ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler, > > > + 0, "pmbus-irq", data); > > > + if (ret) { > > > + dev_warn(dev, "IRQ disabled %d\n", ret); > > > > This is not a warning, it is an error. > Thank you for bringing this to my attention. I will make sure to update the > code to reflect that this is an error. > > > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > #else > > > > This is still in regulator code. I said several times that this is not > > acceptable. > Thank you for pointing out the mistake. I will make sure to correct this in > the next revision. > > > > > static int pmbus_regulator_register(struct pmbus_data *data) > > > { > > > return 0; > > > } > > > +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) > > > +{ > > > + return 0; > > > +} > > > #endif > > > static struct dentry *pmbus_debugfs_dir; /* pmbus debugfs directory */ > > > @@ -3441,6 +3519,12 @@ int pmbus_do_probe(struct i2c_client *client, struct pmbus_driver_info *info) > > > if (ret) > > > return ret; > > > + if (client->irq > 0) { > > > + ret = pmbus_irq_setup(client, data); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > ret = pmbus_init_debugfs(client, data); > > > if (ret) > > > dev_warn(dev, "Failed to register debugfs\n"); > > Thanks, > Naresh
Hi Guenter On 03-01-2023 05:56 pm, Guenter Roeck wrote: > On Tue, Jan 03, 2023 at 12:18:49PM +0530, Naresh Solanki wrote: >> Hi Guenter, >> >> On 29-12-2022 08:10 pm, Guenter Roeck wrote: >>> On Wed, Dec 14, 2022 at 09:07:11AM +0100, Naresh Solanki wrote: >>>> From: Patrick Rudolph <patrick.rudolph@9elements.com> >>>> >>>> Implement PMBUS irq handler. >>>> >>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> >>> >>> $ scripts/checkpatch.pl --strict index.html >>> CHECK: Blank lines aren't necessary after an open brace '{' >>> #131: FILE: drivers/hwmon/pmbus/pmbus_core.c:3088: >>> + for (i = 0; i < data->info->pages; i++) { >>> + >>> >>> CHECK: Alignment should match open parenthesis >>> #183: FILE: drivers/hwmon/pmbus/pmbus_core.c:3140: >>> + ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler, >>> + 0, "pmbus-irq", data); >>> >>> CHECK: Please use a blank line after function/struct/union/enum declarations >>> #197: FILE: drivers/hwmon/pmbus/pmbus_core.c:3154: >>> } >>> +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) >>> >>> total: 0 errors, 0 warnings, 3 checks, 109 lines checked >>> >>> NOTE: For some of the reported defects, checkpatch may be able to >>> mechanically convert to the typical style using --fix or --fix-inplace. >>> >>> index.html has style problems, please review. >>> >>> Please run checkpatch --strict on your patches. >>> Also see Documentation/hwmon/submitting-patches.rst. >> I will take care of these errors in the updated version. >>> >>>> --- >>>> drivers/hwmon/pmbus/pmbus.h | 2 +- >>>> drivers/hwmon/pmbus/pmbus_core.c | 84 ++++++++++++++++++++++++++++++++ >>>> 2 files changed, 85 insertions(+), 1 deletion(-) >>>> >>>> >>>> base-commit: 364ffd2537c44cb6914ff5669153f4a86fffad29 >>>> >>>> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h >>>> index 10fb17879f8e..6b2e6cf93b19 100644 >>>> --- a/drivers/hwmon/pmbus/pmbus.h >>>> +++ b/drivers/hwmon/pmbus/pmbus.h >>>> @@ -26,7 +26,7 @@ enum pmbus_regs { >>>> PMBUS_CAPABILITY = 0x19, >>>> PMBUS_QUERY = 0x1A, >>>> - >>>> + PMBUS_SMBALERT_MASK = 0x1B, >>>> PMBUS_VOUT_MODE = 0x20, >>>> PMBUS_VOUT_COMMAND = 0x21, >>>> PMBUS_VOUT_TRIM = 0x22, >>>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c >>>> index 95e95783972a..244fd2597252 100644 >>>> --- a/drivers/hwmon/pmbus/pmbus_core.c >>>> +++ b/drivers/hwmon/pmbus/pmbus_core.c >>>> @@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data) >>>> return 0; >>>> } >>>> + >>>> +static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val) >>>> +{ >>>> + return pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8)); >>>> +} >>>> + >>>> +static irqreturn_t pmbus_fault_handler(int irq, void *pdata) >>>> +{ >>>> + struct pmbus_data *data = pdata; >>>> + struct i2c_client *client = to_i2c_client(data->dev); >>>> + int i, status; >>>> + >>>> + for (i = 0; i < data->info->pages; i++) { >>>> + >>>> + mutex_lock(&data->update_lock); >>>> + status = pmbus_read_status_word(client, i); >>>> + if (status < 0) { >>>> + mutex_unlock(&data->update_lock); >>>> + return status; >>>> + } >>>> + >>>> + if (status & ~(PB_STATUS_OFF | PB_STATUS_BUSY | PB_STATUS_POWER_GOOD_N)) >>>> + pmbus_clear_fault_page(client, i); >>>> + >>>> + mutex_unlock(&data->update_lock); >>>> + } >>>> + >>>> + return IRQ_HANDLED; >>>> +} >>>> + >>>> +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) >>>> +{ >>>> + struct device *dev = &client->dev; >>>> + const struct pmbus_regulator_status_category *cat; >>>> + const struct pmbus_regulator_status_assoc *bit; >>>> + int i, j, err, ret, func; >>>> + u8 mask; >>>> + >>>> + for (i = 0; i < data->info->pages; i++) { >>>> + func = data->info->func[i]; >>>> + >>>> + for (j = 0; j < ARRAY_SIZE(pmbus_regulator_flag_map); j++) { >>>> + cat = &pmbus_regulator_flag_map[j]; >>>> + if (!(func & cat->func)) >>>> + continue; >>>> + mask = 0; >>>> + for (bit = cat->bits; bit->pflag; bit++) >>>> + mask |= bit->pflag; >>>> + >>>> + err = pmbus_write_smbalert_mask(client, i, cat->reg, ~mask); >>>> + if (err) >>>> + dev_err(dev, "Failed to set smbalert for reg 0x%02x\n", cat->reg); >>> >>> This concerns me. It might mean that the chip does not support >>> PMBUS_SMBALERT_MASK. If so, there would be lots of error messages. >> After going through the PMBus specification, it appears that this should not >> be an issue unless there is a violation of the specification. > > PMBus chips have lots of issues which violate the specification. > Have a look at the various drivers and the workarounds implemented there. > You'll need to check if the command/register is supported before using it. > Also, if you want to keep the error message, make it dev_err_once(). > > Either case, an error is an error, not to be ignored. An error here > should result in an error abort. Yes, I agree that PMBus chips can have issues that violate the specification, and that it is important to check whether a command or register is supported before using it. I have noticed that many drivers use the PMBUS_HAVE_* flags to expose the presence of specific registers, and I think it would be a good idea to add a PMBUS_HAVE_SMBALERT flag as well, so that drivers for supported chips can use it to determine whether they should set up an IRQ handler or not. If PMBUS_HAVE_SMBALERT is set, then the IRQ handler should be set up, otherwise it should be ignored. Will this approach be right? > >>> >>>> + } >>>> + >>>> + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_CML, 0xff); >>>> + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_OTHER, 0xff); >>>> + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_MFR_SPECIFIC, 0xff); >>> >>> Why check the return value from pmbus_write_smbalert_mask above but not here ? >> Thank you for pointing out the oversight. I will make sure to include an >> error check at this point. >>> >>>> + if (data->info->func[i] & PMBUS_HAVE_FAN12) >>>> + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_12, 0xff); >>>> + if (data->info->func[i] & PMBUS_HAVE_FAN34) >>>> + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_34, 0xff); >>>> + } >>>> + >>>> + /* Register notifiers - can fail if IRQ is not given */ >>> >>> The comment does not make sense. pmbus_irq_setup() is not called >>> if the interrupt "is not given". >> Yes. The comment here is not relevant and will be removed. >>> >>>> + ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler, >>>> + 0, "pmbus-irq", data); >>>> + if (ret) { >>>> + dev_warn(dev, "IRQ disabled %d\n", ret); >>> >>> This is not a warning, it is an error. >> Thank you for bringing this to my attention. I will make sure to update the >> code to reflect that this is an error. >>> >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> #else >>> >>> This is still in regulator code. I said several times that this is not >>> acceptable. >> Thank you for pointing out the mistake. I will make sure to correct this in >> the next revision. >>> >>>> static int pmbus_regulator_register(struct pmbus_data *data) >>>> { >>>> return 0; >>>> } >>>> +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) >>>> +{ >>>> + return 0; >>>> +} >>>> #endif >>>> static struct dentry *pmbus_debugfs_dir; /* pmbus debugfs directory */ >>>> @@ -3441,6 +3519,12 @@ int pmbus_do_probe(struct i2c_client *client, struct pmbus_driver_info *info) >>>> if (ret) >>>> return ret; >>>> + if (client->irq > 0) { >>>> + ret = pmbus_irq_setup(client, data); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> ret = pmbus_init_debugfs(client, data); >>>> if (ret) >>>> dev_warn(dev, "Failed to register debugfs\n"); >> >> Thanks, >> Naresh
On Tue, Jan 03, 2023 at 08:56:59PM +0530, Naresh Solanki wrote: > Hi Guenter > > On 03-01-2023 05:56 pm, Guenter Roeck wrote: > > On Tue, Jan 03, 2023 at 12:18:49PM +0530, Naresh Solanki wrote: > > > Hi Guenter, > > > > > > On 29-12-2022 08:10 pm, Guenter Roeck wrote: > > > > On Wed, Dec 14, 2022 at 09:07:11AM +0100, Naresh Solanki wrote: > > > > > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > > > > > > > > > Implement PMBUS irq handler. > > > > > > > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > > > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > > > > > > > > $ scripts/checkpatch.pl --strict index.html > > > > CHECK: Blank lines aren't necessary after an open brace '{' > > > > #131: FILE: drivers/hwmon/pmbus/pmbus_core.c:3088: > > > > + for (i = 0; i < data->info->pages; i++) { > > > > + > > > > > > > > CHECK: Alignment should match open parenthesis > > > > #183: FILE: drivers/hwmon/pmbus/pmbus_core.c:3140: > > > > + ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler, > > > > + 0, "pmbus-irq", data); > > > > > > > > CHECK: Please use a blank line after function/struct/union/enum declarations > > > > #197: FILE: drivers/hwmon/pmbus/pmbus_core.c:3154: > > > > } > > > > +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) > > > > > > > > total: 0 errors, 0 warnings, 3 checks, 109 lines checked > > > > > > > > NOTE: For some of the reported defects, checkpatch may be able to > > > > mechanically convert to the typical style using --fix or --fix-inplace. > > > > > > > > index.html has style problems, please review. > > > > > > > > Please run checkpatch --strict on your patches. > > > > Also see Documentation/hwmon/submitting-patches.rst. > > > I will take care of these errors in the updated version. > > > > > > > > > --- > > > > > drivers/hwmon/pmbus/pmbus.h | 2 +- > > > > > drivers/hwmon/pmbus/pmbus_core.c | 84 ++++++++++++++++++++++++++++++++ > > > > > 2 files changed, 85 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > base-commit: 364ffd2537c44cb6914ff5669153f4a86fffad29 > > > > > > > > > > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h > > > > > index 10fb17879f8e..6b2e6cf93b19 100644 > > > > > --- a/drivers/hwmon/pmbus/pmbus.h > > > > > +++ b/drivers/hwmon/pmbus/pmbus.h > > > > > @@ -26,7 +26,7 @@ enum pmbus_regs { > > > > > PMBUS_CAPABILITY = 0x19, > > > > > PMBUS_QUERY = 0x1A, > > > > > - > > > > > + PMBUS_SMBALERT_MASK = 0x1B, > > > > > PMBUS_VOUT_MODE = 0x20, > > > > > PMBUS_VOUT_COMMAND = 0x21, > > > > > PMBUS_VOUT_TRIM = 0x22, > > > > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > > > > > index 95e95783972a..244fd2597252 100644 > > > > > --- a/drivers/hwmon/pmbus/pmbus_core.c > > > > > +++ b/drivers/hwmon/pmbus/pmbus_core.c > > > > > @@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data) > > > > > return 0; > > > > > } > > > > > + > > > > > +static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val) > > > > > +{ > > > > > + return pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8)); > > > > > +} > > > > > + > > > > > +static irqreturn_t pmbus_fault_handler(int irq, void *pdata) > > > > > +{ > > > > > + struct pmbus_data *data = pdata; > > > > > + struct i2c_client *client = to_i2c_client(data->dev); > > > > > + int i, status; > > > > > + > > > > > + for (i = 0; i < data->info->pages; i++) { > > > > > + > > > > > + mutex_lock(&data->update_lock); > > > > > + status = pmbus_read_status_word(client, i); > > > > > + if (status < 0) { > > > > > + mutex_unlock(&data->update_lock); > > > > > + return status; > > > > > + } > > > > > + > > > > > + if (status & ~(PB_STATUS_OFF | PB_STATUS_BUSY | PB_STATUS_POWER_GOOD_N)) > > > > > + pmbus_clear_fault_page(client, i); > > > > > + > > > > > + mutex_unlock(&data->update_lock); > > > > > + } > > > > > + > > > > > + return IRQ_HANDLED; > > > > > +} > > > > > + > > > > > +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) > > > > > +{ > > > > > + struct device *dev = &client->dev; > > > > > + const struct pmbus_regulator_status_category *cat; > > > > > + const struct pmbus_regulator_status_assoc *bit; > > > > > + int i, j, err, ret, func; > > > > > + u8 mask; > > > > > + > > > > > + for (i = 0; i < data->info->pages; i++) { > > > > > + func = data->info->func[i]; > > > > > + > > > > > + for (j = 0; j < ARRAY_SIZE(pmbus_regulator_flag_map); j++) { > > > > > + cat = &pmbus_regulator_flag_map[j]; > > > > > + if (!(func & cat->func)) > > > > > + continue; > > > > > + mask = 0; > > > > > + for (bit = cat->bits; bit->pflag; bit++) > > > > > + mask |= bit->pflag; > > > > > + > > > > > + err = pmbus_write_smbalert_mask(client, i, cat->reg, ~mask); > > > > > + if (err) > > > > > + dev_err(dev, "Failed to set smbalert for reg 0x%02x\n", cat->reg); > > > > > > > > This concerns me. It might mean that the chip does not support > > > > PMBUS_SMBALERT_MASK. If so, there would be lots of error messages. > > > After going through the PMBus specification, it appears that this should not > > > be an issue unless there is a violation of the specification. > > > > PMBus chips have lots of issues which violate the specification. > > Have a look at the various drivers and the workarounds implemented there. > > You'll need to check if the command/register is supported before using it. > > Also, if you want to keep the error message, make it dev_err_once(). > > > > Either case, an error is an error, not to be ignored. An error here > > should result in an error abort. > Yes, I agree that PMBus chips can have issues that violate the > specification, and that it is important to check whether a command or > register is supported before using it. > I have noticed that many drivers use the PMBUS_HAVE_* flags to expose the > presence of specific registers, and I think it would be a good idea to add a > PMBUS_HAVE_SMBALERT flag as well, so that drivers for supported chips can > use it to determine whether they should set up an IRQ handler or not. If > PMBUS_HAVE_SMBALERT is set, then the IRQ handler should be set up, otherwise > it should be ignored. > Will this approach be right? Not really. PMBUS_HAVE_ flags are intended to indicate sensor register support, not to indicate compliance problems. What you'd be looking for would be the flags in struct pmbus_platform_data. However, those are only intended to be used if registers/commands can not be auto-detected or if doing so causes problems. See include/linux/pmbus.h for details. Unless there is reason to believe that chips are misbehaving when trying to read from or to set PMBUS_SMBALERT_MASK, we should stick with auto- detection. After all, that is what pmbus_check_{status,byte,word}_register() functions are for. Thanks, Guenter
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h index 10fb17879f8e..6b2e6cf93b19 100644 --- a/drivers/hwmon/pmbus/pmbus.h +++ b/drivers/hwmon/pmbus/pmbus.h @@ -26,7 +26,7 @@ enum pmbus_regs { PMBUS_CAPABILITY = 0x19, PMBUS_QUERY = 0x1A, - + PMBUS_SMBALERT_MASK = 0x1B, PMBUS_VOUT_MODE = 0x20, PMBUS_VOUT_COMMAND = 0x21, PMBUS_VOUT_TRIM = 0x22, diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 95e95783972a..244fd2597252 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data) return 0; } + +static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val) +{ + return pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8)); +} + +static irqreturn_t pmbus_fault_handler(int irq, void *pdata) +{ + struct pmbus_data *data = pdata; + struct i2c_client *client = to_i2c_client(data->dev); + int i, status; + + for (i = 0; i < data->info->pages; i++) { + + mutex_lock(&data->update_lock); + status = pmbus_read_status_word(client, i); + if (status < 0) { + mutex_unlock(&data->update_lock); + return status; + } + + if (status & ~(PB_STATUS_OFF | PB_STATUS_BUSY | PB_STATUS_POWER_GOOD_N)) + pmbus_clear_fault_page(client, i); + + mutex_unlock(&data->update_lock); + } + + return IRQ_HANDLED; +} + +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) +{ + struct device *dev = &client->dev; + const struct pmbus_regulator_status_category *cat; + const struct pmbus_regulator_status_assoc *bit; + int i, j, err, ret, func; + u8 mask; + + for (i = 0; i < data->info->pages; i++) { + func = data->info->func[i]; + + for (j = 0; j < ARRAY_SIZE(pmbus_regulator_flag_map); j++) { + cat = &pmbus_regulator_flag_map[j]; + if (!(func & cat->func)) + continue; + mask = 0; + for (bit = cat->bits; bit->pflag; bit++) + mask |= bit->pflag; + + err = pmbus_write_smbalert_mask(client, i, cat->reg, ~mask); + if (err) + dev_err(dev, "Failed to set smbalert for reg 0x%02x\n", cat->reg); + } + + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_CML, 0xff); + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_OTHER, 0xff); + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_MFR_SPECIFIC, 0xff); + if (data->info->func[i] & PMBUS_HAVE_FAN12) + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_12, 0xff); + if (data->info->func[i] & PMBUS_HAVE_FAN34) + pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_34, 0xff); + } + + /* Register notifiers - can fail if IRQ is not given */ + ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler, + 0, "pmbus-irq", data); + if (ret) { + dev_warn(dev, "IRQ disabled %d\n", ret); + return ret; + } + + return 0; +} + #else static int pmbus_regulator_register(struct pmbus_data *data) { return 0; } +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data) +{ + return 0; +} #endif static struct dentry *pmbus_debugfs_dir; /* pmbus debugfs directory */ @@ -3441,6 +3519,12 @@ int pmbus_do_probe(struct i2c_client *client, struct pmbus_driver_info *info) if (ret) return ret; + if (client->irq > 0) { + ret = pmbus_irq_setup(client, data); + if (ret) + return ret; + } + ret = pmbus_init_debugfs(client, data); if (ret) dev_warn(dev, "Failed to register debugfs\n");