lkml.org 
[lkml]   [2022]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v1 08/14] regulator: drivers: Add TI TPS65219 PMIC regulators support
From

On 19/07/2022 15:32, Mark Brown wrote:
> On Tue, Jul 19, 2022 at 11:17:36AM +0200, Jerome Neanne wrote:
>
>> @@ -0,0 +1,414 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * tps65219-regulator.c
>> + *
> Please make the entire comment a C++ one so things look more
> intentional.
>
>> +static int tps65219_pmic_set_voltage_sel(struct regulator_dev *dev,
>> + unsigned int selector)
>> +{
>> + int ret;
>> + struct tps65219 *tps = rdev_get_drvdata(dev);
>> +
>> + /* Set the voltage based on vsel value */
>> + ret = regmap_update_bits(tps->regmap, dev->desc->vsel_reg,
>> + dev->desc->vsel_mask, selector);
>> + if (ret) {
>> + dev_dbg(tps->dev, "%s failed for regulator %s: %d ",
>> + __func__, dev->desc->name, ret);
>> + }
>> + return ret;
>> +}
> This should just be able to use the standard regmap helper, as should
> the enable and disable operations?
>
>> +static int tps65219_set_mode(struct regulator_dev *dev, unsigned int mode)
>> +{
>> + struct tps65219 *tps = rdev_get_drvdata(dev);
>> +
>> + switch (mode) {
>> + case REGULATOR_MODE_NORMAL:
>> + return regmap_set_bits(tps->regmap, TPS65219_REG_STBY_1_CONFIG,
>> + dev->desc->enable_mask);
>> +
>> + case REGULATOR_MODE_STANDBY:
>> + return regmap_clear_bits(tps->regmap,
>> + TPS65219_REG_STBY_1_CONFIG,
>> + dev->desc->enable_mask);
>> + }
>> +
>> + return -EINVAL;
> It'd be a little clearer to have that -EINVAL in a default statement.
>
>> +static irqreturn_t tps65219_regulator_irq_handler(int irq, void *data)
>> +{
>> + struct tps65219_regulator_irq_data *irq_data = data;
>> +
>> + if (irq_data->type->event_name[0] == '\0') {
>> + /* This is the timeout interrupt */
>> + dev_err(irq_data->dev, "System was put in shutdown during an active or standby transition.\n");
>> + return IRQ_HANDLED;
>> + }
>> +
>> + dev_err(irq_data->dev, "Registered %s for %s\n",
>> + irq_data->type->event_name, irq_data->type->regulator_name);
> This should be reporting the events through the notification API, see
> regulator_notifier_call_chain(). That will require a bit of refactoring
> of the way the driver is registering interrupts unfortunately, at the
> minute it doesn't have data joining them up with the
>
> I'd also reword that log message to be something more like "Error %s
> reported for %s" - at the minute it looks more like a probe message.
>
> Otherwise this looks good.
Thanks for your review.

Refactoring the code with regulator_notifier_call_chain, I realized that
some of the events in TPS65219 are not listed as standard
REGULATOR_EVENT in consumer.h

This is the case for below event list:
REGULATOR_EVENT_SCG (ShortCut to Gnd)

REGULATOR_EVENT_RV (Residual Voltage)

REGULATOR_EVENT_RV_SD (Residual Voltage ShutDown)

Should I add those events to the list of standard regulator events and
assign a code? (if yes, any rule for the values?)
Would it fit with some other predefined standard macro defined
elsewhere? (if yes, could you point me to the right location?)

> @@ -0,0 +1,414 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * tps65219-regulator.c
> + *

Please make the entire comment a C++ one so things look more
intentional.

checkpatch is complaining about that:

---------------------------------------------------------------------
v5.19-rc6-PB-MSP1/0005-mfd-drivers-Add-TI-TPS65219-PMIC-support.patch
---------------------------------------------------------------------
WARNING: Improper SPDX comment style for 'drivers/mfd/tps65219.c', please use '//' instead
#91: FILE: drivers/mfd/tps65219.c:1:
+/* SPDX-License-Identifier: GPL-2.0

Let me know if I should ignore checkpatch recommendations here.

Regards,
Jerome

\
 
 \ /
  Last update: 2022-07-22 12:13    [W:0.795 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site