On Tue, Nov 12, 2019 at 05:37:57PM +0000, Jonathan Cameron wrote: > On Tue, 12 Nov 2019 17:35:50 +0200 > Beniamin Bia <beniamin.bia@xxxxxxxxxx> wrote: > > > From: Michael Hennerich <michael.hennerich@xxxxxxxxxx> > > > > ADM1177 is a Hot Swap Controller and Digital Power Monitor with > > Soft Start Pin. > > > > Datasheet: > > Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1177.pdf > > > > Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx> > > Co-developed-by: Beniamin Bia <beniamin.bia@xxxxxxxxxx> > > Signed-off-by: Beniamin Bia <beniamin.bia@xxxxxxxxxx> > > Hi Beniamin, > > A couple immediate thoughts. > > 1. That cc list has some rather non obvious people on it. Unless something > fairly surprising is going on, probably better to cut it back a bit. > > 2. Why IIO? Not entirely obvious to me. From first glance this is definitely > doing hardware monitoring. If there is a reason there should be a clear > statement here on why. > I don't see why this is implemented as iio driver. I think it should be implemented as hardware monitoring driver. Some more comments below. Guenter > +CC Guenter and Jean as hwmon maintainers. > > Whilst I'm here, a very quick review below. > > > --- > > drivers/iio/adc/Kconfig | 12 ++ > > drivers/iio/adc/Makefile | 1 + > > drivers/iio/adc/adm1177.c | 257 ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 270 insertions(+) > > create mode 100644 drivers/iio/adc/adm1177.c > > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > index 9554890a3200..4db8c6dcb595 100644 > > --- a/drivers/iio/adc/Kconfig > > +++ b/drivers/iio/adc/Kconfig > > @@ -228,6 +228,18 @@ config ASPEED_ADC > > To compile this driver as a module, choose M here: the module will be > > called aspeed_adc. > > > > +config ADM1177 > > + tristate "Analog Devices ADM1177 Digital Power Monitor driver" > > + depends on I2C > > + help > > + Say yes here to build support for Analog Devices: > > + ADM1177 Hot Swap Controller and Digital Power Monitor > > + with Soft Start Pin. Provides direct access > > + via sysfs. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called adm1177. > > + > > config AT91_ADC > > tristate "Atmel AT91 ADC" > > depends on ARCH_AT91 > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > > index 5ecc481c2967..7899d6a158f3 100644 > > --- a/drivers/iio/adc/Makefile > > +++ b/drivers/iio/adc/Makefile > > @@ -24,6 +24,7 @@ obj-$(CONFIG_AD7887) += ad7887.o > > obj-$(CONFIG_AD7949) += ad7949.o > > obj-$(CONFIG_AD799X) += ad799x.o > > obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o > > +obj-$(CONFIG_ADM1177) += adm1177.o > > obj-$(CONFIG_AT91_ADC) += at91_adc.o > > obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o > > obj-$(CONFIG_AXP20X_ADC) += axp20x_adc.o > > diff --git a/drivers/iio/adc/adm1177.c b/drivers/iio/adc/adm1177.c > > new file mode 100644 > > index 000000000000..daec34566a65 > > --- /dev/null > > +++ b/drivers/iio/adc/adm1177.c > > @@ -0,0 +1,257 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * ADM1177 Hot Swap Controller and Digital Power Monitor with Soft Start Pin > > + * > > + * Copyright 2015-2019 Analog Devices Inc. > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/err.h> > > +#include <linux/i2c.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/regulator/consumer.h> > > +#include <linux/slab.h> > > +#include <linux/sysfs.h> > > + > > +#include <linux/iio/iio.h> > > +#include <linux/iio/sysfs.h> > > +#include <linux/iio/events.h> > > + > > +/* Command Byte Operations */ > > +#define ADM1177_CMD_V_CONT BIT(0) > > +#define ADM1177_CMD_V_ONCE BIT(1) > > +#define ADM1177_CMD_I_CONT BIT(2) > > +#define ADM1177_CMD_I_ONCE BIT(3) > > +#define ADM1177_CMD_VRANGE BIT(4) > > +#define ADM1177_CMD_STATUS_RD BIT(6) > > + > > +/* Extended Register */ > > +#define ADM1177_REG_ALERT_EN 1 > > +#define ADM1177_REG_ALERT_TH 2 > > +#define ADM1177_REG_CONTROL 3 > > + > > +/* ADM1177_REG_ALERT_EN */ > > +#define ADM1177_EN_ADC_OC1 BIT(0) > > +#define ADM1177_EN_ADC_OC4 BIT(1) > > +#define ADM1177_EN_HS_ALERT BIT(2) > > +#define ADM1177_EN_OFF_ALERT BIT(3) > > +#define ADM1177_CLEAR BIT(4) > > + > > +/* ADM1177_REG_CONTROL */ > > +#define ADM1177_SWOFF BIT(0) > > + > > +#define ADM1177_BITS 12 > > + > > +/** > > + * struct adm1177_state - driver instance specific data > > + * @client pointer to i2c client > > + * @reg regulator info for the the power supply of the device > > + * @command internal control register > > + * @r_sense_uohm current sense resistor value > > + * @alert_threshold_ua current limit for shutdown > > + * @vrange_high internal voltage divider > > + */ > > +struct adm1177_state { > > + struct i2c_client *client; > > + struct regulator *reg; > > + u8 command; > > + u32 r_sense_uohm; > > + u32 alert_threshold_ua; > > + bool vrange_high; > > +}; > > + > > +static int adm1177_read(struct adm1177_state *st, u8 num, u8 *data) > > +{ > > + struct i2c_client *client = st->client; > > + int ret = 0; > > + > > + ret = i2c_master_recv(client, data, num); > > + if (ret < 0) { > > + dev_err(&client->dev, "I2C read error\n"); Seems a bit noisy (and inconsistent). It would make much more sense to report the error to userspace instead of ignoring it and reporting a random value (ie the stack content). > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int adm1177_write_cmd(struct adm1177_state *st, u8 cmd) > > +{ > > + st->command = cmd; > > + return i2c_smbus_write_byte(st->client, cmd); > > +} > > + > > +static int adm1177_write_reg(struct adm1177_state *st, u8 reg, u8 val) > > +{ > > + return i2c_smbus_write_byte_data(st->client, reg, val); > > These wrappers don't really add anything. I'd just make the i2c > calls directly inline. They will be self explanatory. > Plus, their return value is never checked. > > +} > > + > > +static int adm1177_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, > > + int *val2, > > + long mask) > > +{ > > + struct adm1177_state *st = iio_priv(indio_dev); > > + u8 data[3]; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + adm1177_read(st, 3, data); > > + switch (chan->type) { > > + case IIO_VOLTAGE: > > + *val = (data[0] << 4) | (data[2] >> 4); > > + return IIO_VAL_INT; > > + case IIO_CURRENT: > > + *val = (data[1] << 4) | (data[2] & 0xF); > > + return IIO_VAL_INT; > > + default: > > + return -EINVAL; > > + } > > + > > + case IIO_CHAN_INFO_SCALE: > > + switch (chan->type) { > > + case IIO_VOLTAGE: > > + if (st->command & ADM1177_CMD_VRANGE) > > + *val = 6650; > > + else > > + *val = 26350; > > + > > + *val2 = ADM1177_BITS; > > + return IIO_VAL_FRACTIONAL_LOG2; > > + case IIO_CURRENT: > > + *val = 105840 / st->r_sense_uohm; > > + > > + *val2 = ADM1177_BITS; > > + return IIO_VAL_FRACTIONAL_LOG2; > > + default: > > + return -EINVAL; > > + } > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static const struct iio_chan_spec adm1177_channels[] = { > > + { > > + .type = IIO_VOLTAGE, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE), > > + .indexed = 1, > > + .channel = 0, > > + }, > > + { > > + .type = IIO_CURRENT, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE), > > + .indexed = 1, > > + .channel = 0, > > + }, > > +}; > > + > > +static const struct iio_info adm1177_info = { > > + .read_raw = &adm1177_read_raw, > > +}; > > blank line here. > > > +static void adm1177_remove(void *data) > > +{ > > + struct adm1177_state *st = data; > > + > > + if (st->reg) > > + regulator_disable(st->reg); > > Probe and remove should be mirror images of each other. > As you have a mixture of managed and non managed calls that > isn't the case here. > > devm_add_action_or_reset will sort this out for you if > used to turn off the regulator at the right point on removal. > On top of that, this function is added with devm_add_action_or_reset(), and it is only ever called if st->reg is not NULL. > > +} > > + > > +static int adm1177_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + int ret; > > + struct device_node *np; > > + struct adm1177_state *st; > > + struct iio_dev *indio_dev; > > + > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + st = iio_priv(indio_dev); > > + i2c_set_clientdata(client, indio_dev); > > + st->client = client; > > + > > + np = client->dev.of_node; > > + > > + st->reg = devm_regulator_get_optional(&client->dev, "vref"); > > + if (IS_ERR(st->reg)) { > > + if (PTR_ERR(st->reg) == EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + > > + st->reg = NULL; > > + } else { > > + ret = regulator_enable(st->reg); > > + if (ret) > > + return ret; > > + ret = devm_add_action_or_reset(&client->dev, adm1177_remove, > > + st); > > + if (ret) > > + return ret; > > + } > > + > > + of_property_read_u32(np, "adi,r-sense-micro-ohms", &st->r_sense_uohm); shunt-resistor-micro-ohms > > + of_property_read_u32(np, "adi,shutdown-threshold-microamp", > > + &st->alert_threshold_ua); > > + st->vrange_high = of_property_read_bool(np, > > + "adi,vrange-high-enable"); > > + if (st->alert_threshold_ua) { > > + unsigned int val; > > + > > + val = (st->alert_threshold_ua * st->r_sense_uohm * 0xFF); > > + val /= 105840000U; > > + if (val > 0xFF) { > > + dev_err(&client->dev, > > + "Requested shutdown current %d uA above limit\n", > > + st->alert_threshold_ua); > > + > > + val = 0xFF; > > + } > > + adm1177_write_reg(st, ADM1177_REG_ALERT_TH, val); > > + } > > + > > + adm1177_write_cmd(st, ADM1177_CMD_V_CONT | > > + ADM1177_CMD_I_CONT | > > + (st->vrange_high ? 0 : ADM1177_CMD_VRANGE)); > > + > > + indio_dev->name = id->name; > > + indio_dev->channels = adm1177_channels; > > + indio_dev->num_channels = ARRAY_SIZE(adm1177_channels); > > + > > + indio_dev->dev.parent = &client->dev; > > + indio_dev->info = &adm1177_info; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + > > + return devm_iio_device_register(&client->dev, indio_dev); > > +} > > + > > +static const struct i2c_device_id adm1177_ids[] = { > > + { "adm1177", 0 }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(i2c, adm1177_ids); > > + > > +static const struct of_device_id adm1177_dt_ids[] = { > > + { .compatible = "adi,adm1177" }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, adm1177_dt_ids); > > + > > +static struct i2c_driver adm1177_driver = { > > + .driver = { > > + .name = "adm1177", > > + .of_match_table = adm1177_dt_ids, > > + }, > > + .probe = adm1177_probe, > > + .id_table = adm1177_ids, > > +}; > > +module_i2c_driver(adm1177_driver); > > + > > +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@xxxxxxxxxx>"); > > +MODULE_DESCRIPTION("Analog Devices ADM1177 ADC driver"); > > +MODULE_LICENSE("GPL v2"); > >
