Re: [PATCH RESEND v6 2/5] hwmon: (pmbus/core): Notify hwmon events

[Naresh Solanki <naresh.solanki@xxxxxxxxxxxxx>]

Hi Guenter,

On 29-12-2022 08:16 pm, Guenter Roeck wrote:
> On Wed, Dec 14, 2022 at 09:07:12AM +0100, Naresh Solanki wrote:
> > Notify hwmon events using the pmbus irq handler.
> >
> > Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> > ---
> >   drivers/hwmon/pmbus/pmbus_core.c | 95 ++++++++++++++++++++++++--------
> >   1 file changed, 72 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index 244fd2597252..b005a1c8ad7e 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -2781,18 +2781,43 @@ static const struct pmbus_regulator_status_category pmbus_regulator_flag_map[] =
> >   	},
> >   };
> >   
> > -static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
> > +#define to_dev_attr(_dev_attr) \
> > +	container_of(_dev_attr, struct device_attribute, attr)
> > +
> > +static void pmbus_notify(struct pmbus_data *data, int page, int reg, int flags)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < data->num_attributes; i++) {
> > +		struct device_attribute *da = to_dev_attr(data->group.attrs[i]);
> > +		struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> > +		int index = attr->index;
> > +		u16 smask = pb_index_to_mask(index);
> > +		u8 spage = pb_index_to_page(index);
> > +		u16 sreg = pb_index_to_reg(index);
> > +
> > +		if (reg == sreg && page == spage && (smask & flags)) {
> > +			dev_dbg(data->dev, "sysfs notify: %s", da->attr.name);
> > +			sysfs_notify(&data->dev->kobj, NULL, da->attr.name);
> > +			kobject_uevent(&data->dev->kobj, KOBJ_CHANGE);
> > +			flags &= ~smask;
> > +		}
> > +
> > +		if (!flags)
> > +			break;
> > +	}
> > +}
>
> Interrupt aupport as well as sysfs and kobject notifications are not
> regulator specific and do not depend on regulator support. It has to be
> independent of regulator support, meaning it must also work if regulator
> support is disabled.
>
> I seem to have trouble expressing myself, but I don't know how else to say
> it, sorry.
>
> It doesn't make sense to review the series further until this is addressed.
I understand your concern about the independence of the interrupt 
support, sysfs, and kobject notifications from the regulator support. I 
will make sure to address this issue and ensure that these features work 
regardless of whether regulator support is enabled or disabled in the 
next revision. Thank you for your patience and for your feedback.
I will make sure to address your concerns.
> Guenter
>
> > +
> > +static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error,
> > +				    bool notify)
> >   {
> > -	int i, status;
> >   	const struct pmbus_regulator_status_category *cat;
> >   	const struct pmbus_regulator_status_assoc *bit;
> > -	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);
> > +	struct i2c_client *client = to_i2c_client(data->dev);
> >   	int func = data->info->func[page];
> > +	int i, status, ret;
> >   
> > -	*flags = 0;
> > +	*error = 0;
> >   
> >   	mutex_lock(&data->update_lock);
> >   
> > @@ -2803,14 +2828,17 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
> >   
> >   		status = _pmbus_read_byte_data(client, page, cat->reg);
> >   		if (status < 0) {
> > -			mutex_unlock(&data->update_lock);
> > -			return status;
> > +			ret = status;
> > +			goto unlock;
> >   		}
> >   
> >   		for (bit = cat->bits; bit->pflag; bit++) {
> >   			if (status & bit->pflag)
> > -				*flags |= bit->rflag;
> > +				*error |= bit->rflag;
> >   		}
> > +
> > +		if (notify && status)
> > +			pmbus_notify(data, page, cat->reg, status);
> >   	}
> >   
> >   	/*
> > @@ -2823,36 +2851,53 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
> >   	 * REGULATOR_ERROR_<foo>_WARN.
> >   	 */
> >   	status = pmbus_get_status(client, page, PMBUS_STATUS_WORD);
> > -	mutex_unlock(&data->update_lock);
> > -	if (status < 0)
> > -		return status;
> >   
> > -	if (pmbus_regulator_is_enabled(rdev)) {
> > +	if (status < 0) {
> > +		ret = status;
> > +		goto unlock;
> > +	}
> > +
> > +	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> > +	if (ret < 0)
> > +		goto unlock;
> > +
> > +	if (ret & PB_OPERATION_CONTROL_ON) {
> >   		if (status & PB_STATUS_OFF)
> > -			*flags |= REGULATOR_ERROR_FAIL;
> > +			*error |= REGULATOR_ERROR_FAIL;
> >   
> >   		if (status & PB_STATUS_POWER_GOOD_N)
> > -			*flags |= REGULATOR_ERROR_REGULATION_OUT;
> > +			*error |= REGULATOR_ERROR_REGULATION_OUT;
> >   	}
> >   	/*
> >   	 * Unlike most other status bits, PB_STATUS_{IOUT_OC,VOUT_OV} are
> >   	 * defined strictly as fault indicators (not warnings).
> >   	 */
> >   	if (status & PB_STATUS_IOUT_OC)
> > -		*flags |= REGULATOR_ERROR_OVER_CURRENT;
> > +		*error |= REGULATOR_ERROR_OVER_CURRENT;
> >   	if (status & PB_STATUS_VOUT_OV)
> > -		*flags |= REGULATOR_ERROR_REGULATION_OUT;
> > +		*error |= REGULATOR_ERROR_REGULATION_OUT;
> >   
> >   	/*
> >   	 * If we haven't discovered any thermal faults or warnings via
> >   	 * PMBUS_STATUS_TEMPERATURE, map PB_STATUS_TEMPERATURE to a warning as
> >   	 * a (conservative) best-effort interpretation.
> >   	 */
> > -	if (!(*flags & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN)) &&
> > +	if (!(*error & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN)) &&
> >   	    (status & PB_STATUS_TEMPERATURE))
> > -		*flags |= REGULATOR_ERROR_OVER_TEMP_WARN;
> > +		*error |= REGULATOR_ERROR_OVER_TEMP_WARN;
> >   
> > -	return 0;
> > +unlock:
> > +	mutex_unlock(&data->update_lock);
> > +	return ret;
> > +}
> > +
> > +static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
> > +{
> > +	struct device *dev = rdev_get_dev(rdev);
> > +	struct i2c_client *client = to_i2c_client(dev->parent);
> > +	struct pmbus_data *data = i2c_get_clientdata(client);
> > +
> > +	return pmbus_get_flags(data, rdev_get_id(rdev), flags, false);
> >   }
> >   
> >   static int pmbus_regulator_get_status(struct regulator_dev *rdev)
> > @@ -3082,10 +3127,14 @@ 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;
> > +	int i, status, ret;
> >   
> >   	for (i = 0; i < data->info->pages; i++) {
> >   
> > +		ret = pmbus_get_flags(data, i, &status, true);
> > +		if (ret)
> > +			return ret;
> > +
> >   		mutex_lock(&data->update_lock);
> >   		status = pmbus_read_status_word(client, i);
> >   		if (status < 0) {
> > @@ -3099,7 +3148,7 @@ static irqreturn_t pmbus_fault_handler(int irq, void *pdata)
> >   		mutex_unlock(&data->update_lock);
> >   	}
> >   
> > -	return IRQ_HANDLED;
> > +	return ret;
> >   }
> >   
> >   static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)
Regards,
Naresh