| Message ID | 20190524124841.GA25728@localhost.localdomain (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | adm1275: support PMBUS_VIRT_*_SAMPLES | expand |
Hi! On 24/05/2019 14:49, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > The device supports setting the number of samples for averaging the > measurements. There are two separate settings - PWR_AVG for averaging > PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of > PMON_CONFIG register. The values are stored as exponent of base 2 of the > actual number of samples that will be taken. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> > --- > drivers/hwmon/pmbus/adm1275.c | 68 ++++++++++++++++++++++++++++++++++- > 1 file changed, 67 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c > index f569372c9204..4efe1a9df563 100644 > --- a/drivers/hwmon/pmbus/adm1275.c > +++ b/drivers/hwmon/pmbus/adm1275.c > @@ -23,6 +23,8 @@ > #include <linux/slab.h> > #include <linux/i2c.h> > #include <linux/bitops.h> > +#include <linux/bitfield.h> > +#include <linux/log2.h> > #include "pmbus.h" > > enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; > @@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; > #define ADM1075_VAUX_OV_WARN BIT(7) > #define ADM1075_VAUX_UV_WARN BIT(6) > > +#define ADM1275_PWR_AVG_MASK GENMASK(13, 11) > +#define ADM1275_VI_AVG_MASK GENMASK(10, 8) > +#define ADM1275_SAMPLES_AVG_MAX 128 > + > struct adm1275_data { > int id; > bool have_oc_fault; > @@ -90,6 +96,7 @@ struct adm1275_data { > bool have_pin_max; > bool have_temp_max; > struct pmbus_driver_info info; > + struct mutex lock; > }; > > #define to_adm1275_data(x) container_of(x, struct adm1275_data, info) > @@ -164,6 +171,38 @@ static const struct coefficients adm1293_coefficients[] = { > [18] = { 7658, 0, -3 }, /* power, 21V, irange200 */ > }; > > +static inline int adm1275_read_pmon_config(struct i2c_client *client, u64 mask) > +{ > + int ret; > + > + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); > + if (ret < 0) > + return ret; > + > + return FIELD_GET(mask, ret); > +} > + > +static inline int adm1275_write_pmon_config(struct i2c_client *client, u64 mask, > + u16 word) > +{ > + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); > + struct adm1275_data *data = to_adm1275_data(info); > + int ret; > + > + mutex_lock(&data->lock); > + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); > + if (ret < 0) { > + mutex_unlock(&data->lock); > + return ret; > + } > + > + word = FIELD_PREP(mask, word) | (ret & ~mask); > + ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word); > + mutex_unlock(&data->lock); > + > + return ret; > +} > + > static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) > { > const struct pmbus_driver_info *info = pmbus_get_driver_info(client); > @@ -242,6 +281,19 @@ static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) > if (!data->have_temp_max) > return -ENXIO; > break; > + case PMBUS_VIRT_POWER_SAMPLES: > + ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK); > + if (ret < 0) > + break; > + ret = 1 << ret; > + break; > + case PMBUS_VIRT_IN_SAMPLES: > + case PMBUS_VIRT_CURR_SAMPLES: > + ret = adm1275_read_pmon_config(client, ADM1275_VI_AVG_MASK); > + if (ret < 0) > + break; > + ret = 1 << ret; > + break; > default: > ret = -ENODATA; > break; > @@ -286,6 +338,17 @@ static int adm1275_write_word_data(struct i2c_client *client, int page, int reg, > case PMBUS_VIRT_RESET_TEMP_HISTORY: > ret = pmbus_write_word_data(client, 0, ADM1278_PEAK_TEMP, 0); > break; > + case PMBUS_VIRT_POWER_SAMPLES: > + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX); > + ret = adm1275_write_pmon_config(client, ADM1275_PWR_AVG_MASK, > + ilog2(word)); > + break; > + case PMBUS_VIRT_IN_SAMPLES: > + case PMBUS_VIRT_CURR_SAMPLES: > + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX); > + ret = adm1275_write_pmon_config(client, ADM1275_VI_AVG_MASK, > + ilog2(word)); > + break; > default: > ret = -ENODATA; > break; > @@ -422,6 +485,8 @@ static int adm1275_probe(struct i2c_client *client, > if (!data) > return -ENOMEM; > > + mutex_init(&data->lock); > + > if (of_property_read_u32(client->dev.of_node, > "shunt-resistor-micro-ohms", &shunt)) > shunt = 1000; /* 1 mOhm if not set via DT */ > @@ -439,7 +504,8 @@ static int adm1275_probe(struct i2c_client *client, > info->format[PSC_CURRENT_OUT] = direct; > info->format[PSC_POWER] = direct; > info->format[PSC_TEMPERATURE] = direct; > - info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT; > + info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | > + PMBUS_HAVE_SAMPLES; > > info->read_word_data = adm1275_read_word_data; > info->read_byte_data = adm1275_read_byte_data;
On Fri, May 24, 2019 at 12:49:13PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > The device supports setting the number of samples for averaging the > measurements. There are two separate settings - PWR_AVG for averaging > PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of > PMON_CONFIG register. The values are stored as exponent of base 2 of the > actual number of samples that will be taken. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > --- > drivers/hwmon/pmbus/adm1275.c | 68 ++++++++++++++++++++++++++++++++++- > 1 file changed, 67 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c > index f569372c9204..4efe1a9df563 100644 > --- a/drivers/hwmon/pmbus/adm1275.c > +++ b/drivers/hwmon/pmbus/adm1275.c > @@ -23,6 +23,8 @@ > #include <linux/slab.h> > #include <linux/i2c.h> > #include <linux/bitops.h> > +#include <linux/bitfield.h> > +#include <linux/log2.h> > #include "pmbus.h" > > enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; > @@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; > #define ADM1075_VAUX_OV_WARN BIT(7) > #define ADM1075_VAUX_UV_WARN BIT(6) > > +#define ADM1275_PWR_AVG_MASK GENMASK(13, 11) > +#define ADM1275_VI_AVG_MASK GENMASK(10, 8) > +#define ADM1275_SAMPLES_AVG_MAX 128 > + > struct adm1275_data { > int id; > bool have_oc_fault; > @@ -90,6 +96,7 @@ struct adm1275_data { > bool have_pin_max; > bool have_temp_max; > struct pmbus_driver_info info; > + struct mutex lock; > }; > > #define to_adm1275_data(x) container_of(x, struct adm1275_data, info) > @@ -164,6 +171,38 @@ static const struct coefficients adm1293_coefficients[] = { > [18] = { 7658, 0, -3 }, /* power, 21V, irange200 */ > }; > > +static inline int adm1275_read_pmon_config(struct i2c_client *client, u64 mask) Why is the mask passed through as u64 ? > +{ > + int ret; > + > + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); > + if (ret < 0) > + return ret; > + > + return FIELD_GET(mask, ret); > +} > + > +static inline int adm1275_write_pmon_config(struct i2c_client *client, u64 mask, > + u16 word) > +{ > + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); > + struct adm1275_data *data = to_adm1275_data(info); > + int ret; > + > + mutex_lock(&data->lock); Why is another lock on top of the lock provided by the pmbus core required ? > + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); > + if (ret < 0) { > + mutex_unlock(&data->lock); > + return ret; > + } > + > + word = FIELD_PREP(mask, word) | (ret & ~mask); > + ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word); > + mutex_unlock(&data->lock); > + > + return ret; > +} > + > static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) > { > const struct pmbus_driver_info *info = pmbus_get_driver_info(client); > @@ -242,6 +281,19 @@ static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) > if (!data->have_temp_max) > return -ENXIO; > break; > + case PMBUS_VIRT_POWER_SAMPLES: > + ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK); > + if (ret < 0) > + break; > + ret = 1 << ret; ret = BIT(ret); > + break; > + case PMBUS_VIRT_IN_SAMPLES: > + case PMBUS_VIRT_CURR_SAMPLES: > + ret = adm1275_read_pmon_config(client, ADM1275_VI_AVG_MASK); > + if (ret < 0) > + break; > + ret = 1 << ret; ret = BIT(ret); > + break; > default: > ret = -ENODATA; > break; > @@ -286,6 +338,17 @@ static int adm1275_write_word_data(struct i2c_client *client, int page, int reg, > case PMBUS_VIRT_RESET_TEMP_HISTORY: > ret = pmbus_write_word_data(client, 0, ADM1278_PEAK_TEMP, 0); > break; > + case PMBUS_VIRT_POWER_SAMPLES: > + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX); > + ret = adm1275_write_pmon_config(client, ADM1275_PWR_AVG_MASK, > + ilog2(word)); > + break; > + case PMBUS_VIRT_IN_SAMPLES: > + case PMBUS_VIRT_CURR_SAMPLES: > + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX); > + ret = adm1275_write_pmon_config(client, ADM1275_VI_AVG_MASK, > + ilog2(word)); > + break; > default: > ret = -ENODATA; > break; > @@ -422,6 +485,8 @@ static int adm1275_probe(struct i2c_client *client, > if (!data) > return -ENOMEM; > > + mutex_init(&data->lock); > + > if (of_property_read_u32(client->dev.of_node, > "shunt-resistor-micro-ohms", &shunt)) > shunt = 1000; /* 1 mOhm if not set via DT */ > @@ -439,7 +504,8 @@ static int adm1275_probe(struct i2c_client *client, > info->format[PSC_CURRENT_OUT] = direct; > info->format[PSC_POWER] = direct; > info->format[PSC_TEMPERATURE] = direct; > - info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT; > + info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | > + PMBUS_HAVE_SAMPLES; > > info->read_word_data = adm1275_read_word_data; > info->read_byte_data = adm1275_read_byte_data; > -- > 2.20.1 >
On Tue, May 28, 2019 at 12:46:52PM -0700, Guenter Roeck wrote: >On Fri, May 24, 2019 at 12:49:13PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >> The device supports setting the number of samples for averaging the >> measurements. There are two separate settings - PWR_AVG for averaging >> PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of >> PMON_CONFIG register. The values are stored as exponent of base 2 of the >> actual number of samples that will be taken. >> >> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> >> --- >> drivers/hwmon/pmbus/adm1275.c | 68 ++++++++++++++++++++++++++++++++++- >> 1 file changed, 67 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c >> index f569372c9204..4efe1a9df563 100644 >> --- a/drivers/hwmon/pmbus/adm1275.c >> +++ b/drivers/hwmon/pmbus/adm1275.c >> @@ -23,6 +23,8 @@ >> #include <linux/slab.h> >> #include <linux/i2c.h> >> #include <linux/bitops.h> >> +#include <linux/bitfield.h> >> +#include <linux/log2.h> >> #include "pmbus.h" >> >> enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; >> @@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; >> #define ADM1075_VAUX_OV_WARN BIT(7) >> #define ADM1075_VAUX_UV_WARN BIT(6) >> >> +#define ADM1275_PWR_AVG_MASK GENMASK(13, 11) >> +#define ADM1275_VI_AVG_MASK GENMASK(10, 8) >> +#define ADM1275_SAMPLES_AVG_MAX 128 >> + >> struct adm1275_data { >> int id; >> bool have_oc_fault; >> @@ -90,6 +96,7 @@ struct adm1275_data { >> bool have_pin_max; >> bool have_temp_max; >> struct pmbus_driver_info info; >> + struct mutex lock; >> }; >> >> #define to_adm1275_data(x) container_of(x, struct adm1275_data, info) >> @@ -164,6 +171,38 @@ static const struct coefficients adm1293_coefficients[] = { >> [18] = { 7658, 0, -3 }, /* power, 21V, irange200 */ >> }; >> >> +static inline int adm1275_read_pmon_config(struct i2c_client *client, u64 mask) > >Why is the mask passed through as u64 ? Good point. I used u64 as this is the type used by bitfield machinery under the hood but I agree it doesn't make sense and is even confusing to have this in the function prototype as we are using this to mask 16 bit word anyways. I will fix that in v2. I am gonna have to cast the ret to u16 when passing to FIELD_GET() to make sure the __BF_FIELD_CHECK is not complaining (since it is signed right now), though. > >> +{ >> + int ret; >> + >> + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); >> + if (ret < 0) >> + return ret; >> + >> + return FIELD_GET(mask, ret); >> +} >> + >> +static inline int adm1275_write_pmon_config(struct i2c_client *client, u64 mask, >> + u16 word) >> +{ >> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); >> + struct adm1275_data *data = to_adm1275_data(info); >> + int ret; >> + >> + mutex_lock(&data->lock); > >Why is another lock on top of the lock provided by the pmbus core required ? > Good point, I was considering if I should instead add mutex_lock on update_lock in the pmbus_set_samples() function inside of pmbus_core.c instead (as this function is missing it) but figured that not all devices will need that (lm25066 didn't) so it might be a waste in most cases. But this may be cleaner approach indeed. Is this what you mean or there is some other lock I missed? >> + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); >> + if (ret < 0) { >> + mutex_unlock(&data->lock); >> + return ret; >> + } >> + >> + word = FIELD_PREP(mask, word) | (ret & ~mask); >> + ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word); >> + mutex_unlock(&data->lock); >> + >> + return ret; >> +} >> + >> static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) >> { >> const struct pmbus_driver_info *info = pmbus_get_driver_info(client); >> @@ -242,6 +281,19 @@ static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) >> if (!data->have_temp_max) >> return -ENXIO; >> break; >> + case PMBUS_VIRT_POWER_SAMPLES: >> + ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK); >> + if (ret < 0) >> + break; >> + ret = 1 << ret; > > ret = BIT(ret); > I intentionally used the "raw" left shift to make it more obvious this is pow2 arithmetic operation and an direct inverse to the ilog2() used on write counterpart. This is also consistent with what I used in lm25066.c driver not long time ago. I don't have strong preference but this is my reasoning. So do you still think it is better to use BIT() macro instead? >> + break; >> + case PMBUS_VIRT_IN_SAMPLES: >> + case PMBUS_VIRT_CURR_SAMPLES: >> + ret = adm1275_read_pmon_config(client, ADM1275_VI_AVG_MASK); >> + if (ret < 0) >> + break; >> + ret = 1 << ret; > > ret = BIT(ret); > >> + break; >> default: >> ret = -ENODATA; >> break; >> @@ -286,6 +338,17 @@ static int adm1275_write_word_data(struct i2c_client *client, int page, int reg, >> case PMBUS_VIRT_RESET_TEMP_HISTORY: >> ret = pmbus_write_word_data(client, 0, ADM1278_PEAK_TEMP, 0); >> break; >> + case PMBUS_VIRT_POWER_SAMPLES: >> + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX); >> + ret = adm1275_write_pmon_config(client, ADM1275_PWR_AVG_MASK, >> + ilog2(word)); >> + break; >> + case PMBUS_VIRT_IN_SAMPLES: >> + case PMBUS_VIRT_CURR_SAMPLES: >> + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX); >> + ret = adm1275_write_pmon_config(client, ADM1275_VI_AVG_MASK, >> + ilog2(word)); >> + break; >> default: >> ret = -ENODATA; >> break; >> @@ -422,6 +485,8 @@ static int adm1275_probe(struct i2c_client *client, >> if (!data) >> return -ENOMEM; >> >> + mutex_init(&data->lock); >> + >> if (of_property_read_u32(client->dev.of_node, >> "shunt-resistor-micro-ohms", &shunt)) >> shunt = 1000; /* 1 mOhm if not set via DT */ >> @@ -439,7 +504,8 @@ static int adm1275_probe(struct i2c_client *client, >> info->format[PSC_CURRENT_OUT] = direct; >> info->format[PSC_POWER] = direct; >> info->format[PSC_TEMPERATURE] = direct; >> - info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT; >> + info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | >> + PMBUS_HAVE_SAMPLES; >> >> info->read_word_data = adm1275_read_word_data; >> info->read_byte_data = adm1275_read_byte_data; >> -- >> 2.20.1 >>
On 5/29/19 12:11 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > On Tue, May 28, 2019 at 12:46:52PM -0700, Guenter Roeck wrote: >> On Fri, May 24, 2019 at 12:49:13PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>> The device supports setting the number of samples for averaging the >>> measurements. There are two separate settings - PWR_AVG for averaging >>> PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of >>> PMON_CONFIG register. The values are stored as exponent of base 2 of the >>> actual number of samples that will be taken. >>> >>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> >>> --- >>> drivers/hwmon/pmbus/adm1275.c | 68 ++++++++++++++++++++++++++++++++++- >>> 1 file changed, 67 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c >>> index f569372c9204..4efe1a9df563 100644 >>> --- a/drivers/hwmon/pmbus/adm1275.c >>> +++ b/drivers/hwmon/pmbus/adm1275.c >>> @@ -23,6 +23,8 @@ >>> #include <linux/slab.h> >>> #include <linux/i2c.h> >>> #include <linux/bitops.h> >>> +#include <linux/bitfield.h> >>> +#include <linux/log2.h> >>> #include "pmbus.h" >>> >>> enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; >>> @@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; >>> #define ADM1075_VAUX_OV_WARN BIT(7) >>> #define ADM1075_VAUX_UV_WARN BIT(6) >>> >>> +#define ADM1275_PWR_AVG_MASK GENMASK(13, 11) >>> +#define ADM1275_VI_AVG_MASK GENMASK(10, 8) >>> +#define ADM1275_SAMPLES_AVG_MAX 128 >>> + >>> struct adm1275_data { >>> int id; >>> bool have_oc_fault; >>> @@ -90,6 +96,7 @@ struct adm1275_data { >>> bool have_pin_max; >>> bool have_temp_max; >>> struct pmbus_driver_info info; >>> + struct mutex lock; >>> }; >>> >>> #define to_adm1275_data(x) container_of(x, struct adm1275_data, info) >>> @@ -164,6 +171,38 @@ static const struct coefficients adm1293_coefficients[] = { >>> [18] = { 7658, 0, -3 }, /* power, 21V, irange200 */ >>> }; >>> >>> +static inline int adm1275_read_pmon_config(struct i2c_client *client, u64 mask) >> >> Why is the mask passed through as u64 ? > > Good point. I used u64 as this is the type used by bitfield machinery > under the hood but I agree it doesn't make sense and is even confusing > to have this in the function prototype as we are using this to mask 16 > bit word anyways. I will fix that in v2. I am gonna have to cast the ret > to u16 when passing to FIELD_GET() to make sure the __BF_FIELD_CHECK is > not complaining (since it is signed right now), though. > Not sure I understand what you are talking about. FIELD_GET() uses typeof(). FIELD_GET() is used by other callers even with u8 and without any typecasts. Why would it be a problem here ? >> >>> +{ >>> + int ret; >>> + >>> + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); >>> + if (ret < 0) >>> + return ret; >>> + >>> + return FIELD_GET(mask, ret); >>> +} >>> + >>> +static inline int adm1275_write_pmon_config(struct i2c_client *client, u64 mask, >>> + u16 word) >>> +{ >>> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); >>> + struct adm1275_data *data = to_adm1275_data(info); >>> + int ret; >>> + >>> + mutex_lock(&data->lock); >> >> Why is another lock on top of the lock provided by the pmbus core required ? >> > > Good point, I was considering if I should instead add mutex_lock on > update_lock in the pmbus_set_samples() function inside of pmbus_core.c > instead (as this function is missing it) but figured that not all > devices will need that (lm25066 didn't) so it might be a waste in most > cases. But this may be cleaner approach indeed. > > Is this what you mean or there is some other lock I missed? > pmbus_set_samples() should set the pmbus lock. That was missed when the function was added. >>> + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); >>> + if (ret < 0) { >>> + mutex_unlock(&data->lock); >>> + return ret; >>> + } >>> + >>> + word = FIELD_PREP(mask, word) | (ret & ~mask); >>> + ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word); >>> + mutex_unlock(&data->lock); >>> + >>> + return ret; >>> +} >>> + >>> static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) >>> { >>> const struct pmbus_driver_info *info = pmbus_get_driver_info(client); >>> @@ -242,6 +281,19 @@ static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) >>> if (!data->have_temp_max) >>> return -ENXIO; >>> break; >>> + case PMBUS_VIRT_POWER_SAMPLES: >>> + ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK); >>> + if (ret < 0) >>> + break; >>> + ret = 1 << ret; >> >> ret = BIT(ret); >> > > I intentionally used the "raw" left shift to make it more obvious this > is pow2 arithmetic operation and an direct inverse to the ilog2() used > on write counterpart. This is also consistent with what I used in > lm25066.c driver not long time ago. > > I don't have strong preference but this is my reasoning. So do you still > think it is better to use BIT() macro instead? > I don't think that is a good rationale, but I'll let it go. Guenter >>> + break; >>> + case PMBUS_VIRT_IN_SAMPLES: >>> + case PMBUS_VIRT_CURR_SAMPLES: >>> + ret = adm1275_read_pmon_config(client, ADM1275_VI_AVG_MASK); >>> + if (ret < 0) >>> + break; >>> + ret = 1 << ret; >> >> ret = BIT(ret); >> >>> + break; >>> default: >>> ret = -ENODATA; >>> break; >>> @@ -286,6 +338,17 @@ static int adm1275_write_word_data(struct i2c_client *client, int page, int reg, >>> case PMBUS_VIRT_RESET_TEMP_HISTORY: >>> ret = pmbus_write_word_data(client, 0, ADM1278_PEAK_TEMP, 0); >>> break; >>> + case PMBUS_VIRT_POWER_SAMPLES: >>> + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX); >>> + ret = adm1275_write_pmon_config(client, ADM1275_PWR_AVG_MASK, >>> + ilog2(word)); >>> + break; >>> + case PMBUS_VIRT_IN_SAMPLES: >>> + case PMBUS_VIRT_CURR_SAMPLES: >>> + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX); >>> + ret = adm1275_write_pmon_config(client, ADM1275_VI_AVG_MASK, >>> + ilog2(word)); >>> + break; >>> default: >>> ret = -ENODATA; >>> break; >>> @@ -422,6 +485,8 @@ static int adm1275_probe(struct i2c_client *client, >>> if (!data) >>> return -ENOMEM; >>> >>> + mutex_init(&data->lock); >>> + >>> if (of_property_read_u32(client->dev.of_node, >>> "shunt-resistor-micro-ohms", &shunt)) >>> shunt = 1000; /* 1 mOhm if not set via DT */ >>> @@ -439,7 +504,8 @@ static int adm1275_probe(struct i2c_client *client, >>> info->format[PSC_CURRENT_OUT] = direct; >>> info->format[PSC_POWER] = direct; >>> info->format[PSC_TEMPERATURE] = direct; >>> - info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT; >>> + info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | >>> + PMBUS_HAVE_SAMPLES; >>> >>> info->read_word_data = adm1275_read_word_data; >>> info->read_byte_data = adm1275_read_byte_data; >>> -- >>> 2.20.1 >>> >
On Wed, May 29, 2019 at 05:17:47AM -0700, Guenter Roeck wrote: >On 5/29/19 12:11 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>On Tue, May 28, 2019 at 12:46:52PM -0700, Guenter Roeck wrote: >>>On Fri, May 24, 2019 at 12:49:13PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>>>The device supports setting the number of samples for averaging the >>>>measurements. There are two separate settings - PWR_AVG for averaging >>>>PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of >>>>PMON_CONFIG register. The values are stored as exponent of base 2 of the >>>>actual number of samples that will be taken. >>>> >>>>Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> >>>>--- >>>> drivers/hwmon/pmbus/adm1275.c | 68 ++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 67 insertions(+), 1 deletion(-) >>>> >>>>diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c >>>>index f569372c9204..4efe1a9df563 100644 >>>>--- a/drivers/hwmon/pmbus/adm1275.c >>>>+++ b/drivers/hwmon/pmbus/adm1275.c >>>>@@ -23,6 +23,8 @@ >>>> #include <linux/slab.h> >>>> #include <linux/i2c.h> >>>> #include <linux/bitops.h> >>>>+#include <linux/bitfield.h> >>>>+#include <linux/log2.h> >>>> #include "pmbus.h" >>>> >>>> enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; >>>>@@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; >>>> #define ADM1075_VAUX_OV_WARN BIT(7) >>>> #define ADM1075_VAUX_UV_WARN BIT(6) >>>> >>>>+#define ADM1275_PWR_AVG_MASK GENMASK(13, 11) >>>>+#define ADM1275_VI_AVG_MASK GENMASK(10, 8) >>>>+#define ADM1275_SAMPLES_AVG_MAX 128 >>>>+ >>>> struct adm1275_data { >>>> int id; >>>> bool have_oc_fault; >>>>@@ -90,6 +96,7 @@ struct adm1275_data { >>>> bool have_pin_max; >>>> bool have_temp_max; >>>> struct pmbus_driver_info info; >>>>+ struct mutex lock; >>>> }; >>>> >>>> #define to_adm1275_data(x) container_of(x, struct adm1275_data, info) >>>>@@ -164,6 +171,38 @@ static const struct coefficients adm1293_coefficients[] = { >>>> [18] = { 7658, 0, -3 }, /* power, 21V, irange200 */ >>>> }; >>>> >>>>+static inline int adm1275_read_pmon_config(struct i2c_client *client, u64 mask) >>> >>>Why is the mask passed through as u64 ? >> >>Good point. I used u64 as this is the type used by bitfield machinery >>under the hood but I agree it doesn't make sense and is even confusing >>to have this in the function prototype as we are using this to mask 16 >>bit word anyways. I will fix that in v2. I am gonna have to cast the ret >>to u16 when passing to FIELD_GET() to make sure the __BF_FIELD_CHECK is >>not complaining (since it is signed right now), though. >> > >Not sure I understand what you are talking about. FIELD_GET() uses typeof(). >FIELD_GET() is used by other callers even with u8 and without any typecasts. >Why would it be a problem here ? So I basically agree with you but just wanted to note why there will be additional cast needed in my code. The: return FIELD_GET(mask, ret); will be changed to: return FIELD_GET(mask, (u16)ret); And the reason for that is that the __BF_FIELD_CHECK does this check at compile time (and breaks if this is true) (_mask) > (typeof(_reg))~0ull In my case typeof(_reg) is int, so (typeof(_reg))~0ull = -1 which is signed. _mask is unsigned. Depending on the type promotion, this might or might not be true depending on the size of _mask. When _mask was u64, it always worked. For _mask being u16, it will fail. For u32, this will fail depending on if we are compiling for 32 or 64 bit architecture. All this might be obvious to you but it wasn't to me, thus this note. >>> >>>>+{ >>>>+ int ret; >>>>+ >>>>+ ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); >>>>+ if (ret < 0) >>>>+ return ret; >>>>+ >>>>+ return FIELD_GET(mask, ret); >>>>+} >>>>+ >>>>+static inline int adm1275_write_pmon_config(struct i2c_client *client, u64 mask, >>>>+ u16 word) >>>>+{ >>>>+ const struct pmbus_driver_info *info = pmbus_get_driver_info(client); >>>>+ struct adm1275_data *data = to_adm1275_data(info); >>>>+ int ret; >>>>+ >>>>+ mutex_lock(&data->lock); >>> >>>Why is another lock on top of the lock provided by the pmbus core required ? >>> >> >>Good point, I was considering if I should instead add mutex_lock on >>update_lock in the pmbus_set_samples() function inside of pmbus_core.c >>instead (as this function is missing it) but figured that not all >>devices will need that (lm25066 didn't) so it might be a waste in most >>cases. But this may be cleaner approach indeed. >> >>Is this what you mean or there is some other lock I missed? >> >pmbus_set_samples() should set the pmbus lock. That was missed when >the function was added. And by pmbus lock you mean the update_lock from the pmbus_data structure? I didn't see any other lock but wanted to double check my understanding. >>>>+ ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); >>>>+ if (ret < 0) { >>>>+ mutex_unlock(&data->lock); >>>>+ return ret; >>>>+ } >>>>+ >>>>+ word = FIELD_PREP(mask, word) | (ret & ~mask); >>>>+ ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word); >>>>+ mutex_unlock(&data->lock); >>>>+ >>>>+ return ret; >>>>+} >>>>+ >>>> static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) >>>> { >>>> const struct pmbus_driver_info *info = pmbus_get_driver_info(client); >>>>@@ -242,6 +281,19 @@ static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) >>>> if (!data->have_temp_max) >>>> return -ENXIO; >>>> break; >>>>+ case PMBUS_VIRT_POWER_SAMPLES: >>>>+ ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK); >>>>+ if (ret < 0) >>>>+ break; >>>>+ ret = 1 << ret; >>> >>> ret = BIT(ret); >>> >> >>I intentionally used the "raw" left shift to make it more obvious this >>is pow2 arithmetic operation and an direct inverse to the ilog2() used >>on write counterpart. This is also consistent with what I used in >>lm25066.c driver not long time ago. >> >>I don't have strong preference but this is my reasoning. So do you still >>think it is better to use BIT() macro instead? >> > >I don't think that is a good rationale, but I'll let it go. > If you don't think so, I'll change it in v2. As I said, I don't have a strong opinion on that. Krzysztof
On 5/29/19 5:53 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > On Wed, May 29, 2019 at 05:17:47AM -0700, Guenter Roeck wrote: >> On 5/29/19 12:11 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>> On Tue, May 28, 2019 at 12:46:52PM -0700, Guenter Roeck wrote: >>>> On Fri, May 24, 2019 at 12:49:13PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>>>> The device supports setting the number of samples for averaging the >>>>> measurements. There are two separate settings - PWR_AVG for averaging >>>>> PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of >>>>> PMON_CONFIG register. The values are stored as exponent of base 2 of the >>>>> actual number of samples that will be taken. >>>>> >>>>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> >>>>> --- >>>>> drivers/hwmon/pmbus/adm1275.c | 68 ++++++++++++++++++++++++++++++++++- >>>>> 1 file changed, 67 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c >>>>> index f569372c9204..4efe1a9df563 100644 >>>>> --- a/drivers/hwmon/pmbus/adm1275.c >>>>> +++ b/drivers/hwmon/pmbus/adm1275.c >>>>> @@ -23,6 +23,8 @@ >>>>> #include <linux/slab.h> >>>>> #include <linux/i2c.h> >>>>> #include <linux/bitops.h> >>>>> +#include <linux/bitfield.h> >>>>> +#include <linux/log2.h> >>>>> #include "pmbus.h" >>>>> >>>>> enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; >>>>> @@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; >>>>> #define ADM1075_VAUX_OV_WARN BIT(7) >>>>> #define ADM1075_VAUX_UV_WARN BIT(6) >>>>> >>>>> +#define ADM1275_PWR_AVG_MASK GENMASK(13, 11) >>>>> +#define ADM1275_VI_AVG_MASK GENMASK(10, 8) >>>>> +#define ADM1275_SAMPLES_AVG_MAX 128 >>>>> + >>>>> struct adm1275_data { >>>>> int id; >>>>> bool have_oc_fault; >>>>> @@ -90,6 +96,7 @@ struct adm1275_data { >>>>> bool have_pin_max; >>>>> bool have_temp_max; >>>>> struct pmbus_driver_info info; >>>>> + struct mutex lock; >>>>> }; >>>>> >>>>> #define to_adm1275_data(x) container_of(x, struct adm1275_data, info) >>>>> @@ -164,6 +171,38 @@ static const struct coefficients adm1293_coefficients[] = { >>>>> [18] = { 7658, 0, -3 }, /* power, 21V, irange200 */ >>>>> }; >>>>> >>>>> +static inline int adm1275_read_pmon_config(struct i2c_client *client, u64 mask) >>>> >>>> Why is the mask passed through as u64 ? >>> >>> Good point. I used u64 as this is the type used by bitfield machinery >>> under the hood but I agree it doesn't make sense and is even confusing >>> to have this in the function prototype as we are using this to mask 16 >>> bit word anyways. I will fix that in v2. I am gonna have to cast the ret >>> to u16 when passing to FIELD_GET() to make sure the __BF_FIELD_CHECK is >>> not complaining (since it is signed right now), though. >>> >> >> Not sure I understand what you are talking about. FIELD_GET() uses typeof(). >> FIELD_GET() is used by other callers even with u8 and without any typecasts. >> Why would it be a problem here ? > > So I basically agree with you but just wanted to note why there will be > additional cast needed in my code. The: > return FIELD_GET(mask, ret); > will be changed to: > return FIELD_GET(mask, (u16)ret); > > And the reason for that is that the __BF_FIELD_CHECK does this check at > compile time (and breaks if this is true) > (_mask) > (typeof(_reg))~0ull > > In my case typeof(_reg) is int, so (typeof(_reg))~0ull = -1 which is > signed. _mask is unsigned. Depending on the type promotion, this might > or might not be true depending on the size of _mask. When _mask was u64, > it always worked. For _mask being u16, it will fail. For u32, this will > fail depending on if we are compiling for 32 or 64 bit architecture. > > All this might be obvious to you but it wasn't to me, thus this note. > The problem here is that ret is an int, and integers are not well suited for mask operations. The typecast thus makes sense and is ok. >>>> >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + return FIELD_GET(mask, ret); >>>>> +} >>>>> + >>>>> +static inline int adm1275_write_pmon_config(struct i2c_client *client, u64 mask, >>>>> + u16 word) >>>>> +{ >>>>> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); >>>>> + struct adm1275_data *data = to_adm1275_data(info); >>>>> + int ret; >>>>> + >>>>> + mutex_lock(&data->lock); >>>> >>>> Why is another lock on top of the lock provided by the pmbus core required ? >>>> >>> >>> Good point, I was considering if I should instead add mutex_lock on >>> update_lock in the pmbus_set_samples() function inside of pmbus_core.c >>> instead (as this function is missing it) but figured that not all >>> devices will need that (lm25066 didn't) so it might be a waste in most >>> cases. But this may be cleaner approach indeed. >>> >>> Is this what you mean or there is some other lock I missed? >>> >> pmbus_set_samples() should set the pmbus lock. That was missed when >> the function was added. > > And by pmbus lock you mean the update_lock from the pmbus_data > structure? I didn't see any other lock but wanted to double check my > understanding. > Yes, that is the lock intended to protect write operations. Guenter >>>>> + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); >>>>> + if (ret < 0) { >>>>> + mutex_unlock(&data->lock); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + word = FIELD_PREP(mask, word) | (ret & ~mask); >>>>> + ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word); >>>>> + mutex_unlock(&data->lock); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) >>>>> { >>>>> const struct pmbus_driver_info *info = pmbus_get_driver_info(client); >>>>> @@ -242,6 +281,19 @@ static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) >>>>> if (!data->have_temp_max) >>>>> return -ENXIO; >>>>> break; >>>>> + case PMBUS_VIRT_POWER_SAMPLES: >>>>> + ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK); >>>>> + if (ret < 0) >>>>> + break; >>>>> + ret = 1 << ret; >>>> >>>> ret = BIT(ret); >>>> >>> >>> I intentionally used the "raw" left shift to make it more obvious this >>> is pow2 arithmetic operation and an direct inverse to the ilog2() used >>> on write counterpart. This is also consistent with what I used in >>> lm25066.c driver not long time ago. >>> >>> I don't have strong preference but this is my reasoning. So do you still >>> think it is better to use BIT() macro instead? >>> >> >> I don't think that is a good rationale, but I'll let it go. >> > > If you don't think so, I'll change it in v2. As I said, I don't have a > strong opinion on that. > > Krzysztof >
diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c index f569372c9204..4efe1a9df563 100644 --- a/drivers/hwmon/pmbus/adm1275.c +++ b/drivers/hwmon/pmbus/adm1275.c @@ -23,6 +23,8 @@ #include <linux/slab.h> #include <linux/i2c.h> #include <linux/bitops.h> +#include <linux/bitfield.h> +#include <linux/log2.h> #include "pmbus.h" enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; @@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 }; #define ADM1075_VAUX_OV_WARN BIT(7) #define ADM1075_VAUX_UV_WARN BIT(6) +#define ADM1275_PWR_AVG_MASK GENMASK(13, 11) +#define ADM1275_VI_AVG_MASK GENMASK(10, 8) +#define ADM1275_SAMPLES_AVG_MAX 128 + struct adm1275_data { int id; bool have_oc_fault; @@ -90,6 +96,7 @@ struct adm1275_data { bool have_pin_max; bool have_temp_max; struct pmbus_driver_info info; + struct mutex lock; }; #define to_adm1275_data(x) container_of(x, struct adm1275_data, info) @@ -164,6 +171,38 @@ static const struct coefficients adm1293_coefficients[] = { [18] = { 7658, 0, -3 }, /* power, 21V, irange200 */ }; +static inline int adm1275_read_pmon_config(struct i2c_client *client, u64 mask) +{ + int ret; + + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); + if (ret < 0) + return ret; + + return FIELD_GET(mask, ret); +} + +static inline int adm1275_write_pmon_config(struct i2c_client *client, u64 mask, + u16 word) +{ + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); + struct adm1275_data *data = to_adm1275_data(info); + int ret; + + mutex_lock(&data->lock); + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG); + if (ret < 0) { + mutex_unlock(&data->lock); + return ret; + } + + word = FIELD_PREP(mask, word) | (ret & ~mask); + ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word); + mutex_unlock(&data->lock); + + return ret; +} + static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) { const struct pmbus_driver_info *info = pmbus_get_driver_info(client); @@ -242,6 +281,19 @@ static int adm1275_read_word_data(struct i2c_client *client, int page, int reg) if (!data->have_temp_max) return -ENXIO; break; + case PMBUS_VIRT_POWER_SAMPLES: + ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK); + if (ret < 0) + break; + ret = 1 << ret; + break; + case PMBUS_VIRT_IN_SAMPLES: + case PMBUS_VIRT_CURR_SAMPLES: + ret = adm1275_read_pmon_config(client, ADM1275_VI_AVG_MASK); + if (ret < 0) + break; + ret = 1 << ret; + break; default: ret = -ENODATA; break; @@ -286,6 +338,17 @@ static int adm1275_write_word_data(struct i2c_client *client, int page, int reg, case PMBUS_VIRT_RESET_TEMP_HISTORY: ret = pmbus_write_word_data(client, 0, ADM1278_PEAK_TEMP, 0); break; + case PMBUS_VIRT_POWER_SAMPLES: + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX); + ret = adm1275_write_pmon_config(client, ADM1275_PWR_AVG_MASK, + ilog2(word)); + break; + case PMBUS_VIRT_IN_SAMPLES: + case PMBUS_VIRT_CURR_SAMPLES: + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX); + ret = adm1275_write_pmon_config(client, ADM1275_VI_AVG_MASK, + ilog2(word)); + break; default: ret = -ENODATA; break; @@ -422,6 +485,8 @@ static int adm1275_probe(struct i2c_client *client, if (!data) return -ENOMEM; + mutex_init(&data->lock); + if (of_property_read_u32(client->dev.of_node, "shunt-resistor-micro-ohms", &shunt)) shunt = 1000; /* 1 mOhm if not set via DT */ @@ -439,7 +504,8 @@ static int adm1275_probe(struct i2c_client *client, info->format[PSC_CURRENT_OUT] = direct; info->format[PSC_POWER] = direct; info->format[PSC_TEMPERATURE] = direct; - info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT; + info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | + PMBUS_HAVE_SAMPLES; info->read_word_data = adm1275_read_word_data; info->read_byte_data = adm1275_read_byte_data;
The device supports setting the number of samples for averaging the measurements. There are two separate settings - PWR_AVG for averaging PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of PMON_CONFIG register. The values are stored as exponent of base 2 of the actual number of samples that will be taken. Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> --- drivers/hwmon/pmbus/adm1275.c | 68 ++++++++++++++++++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-)