Hi Jonathan,
Please see my comments below. (I will take out section that I already
agree with):
On Sat, 2023-02-25 at 19:27 +0000, Jonathan Cameron wrote:
>
> a representative part from the list - if no other basis to chose go
> with the
> lowest number currently supported.
>
> The custom ABI in here needs documentation before we can review it
> properly.
>
I will comment all new ABI that I have added.
> Comments inline.
>
> It's a big driver, so I'm sure more stuff will come up next round
> just due to
> mental exhaustion reviewing it in one go!
>
>
>
>
>
>
>
> > BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> > + .scan_index =
> > (_si), \
> > + .scan_type =
> > { \
> > + .sign =
> > 'u', \
> > + .realbits =
> > PAC193X_POWER_U_RES, \
> > + .storagebits =
> > 32, \
> > + .shift =
> > 4, \
> > + .endianness =
> > IIO_CPU, \
> > +
> > }
> > \
> > +}
> > +
> > +#define PAC193X_SOFT_TIMESTAMP(_index)
> > { \
> > + .type =
> > IIO_TIMESTAMP, \
> > + .channel = -
> > 1, \
>
> What is this for?
>
I will take it out. Initially I want to have also "data buffering"
into the driver, but this functionality I will add it later. Data
buffering (containing timestamp) will be useful when profiling power
consumption.
>
>
>
> > +
> > + len += sprintf(buf, "%u\n", chip_info->shunts[target]);
> > +
> > + return len;
> > +}
> > +
> > +static ssize_t channel_name_show(struct device *dev,
>
> Looks like per channel label support provided by the read_label
> iio_info callback.
>
I was trying to use the read_label iio_info callback but I end it up
into a sysfs error related to duplicated labels.
Also I'm not sure if this read_label will help me in the end. What I
was aiming to obtain here with the "channel_name_show" and the custom
IIO attribute was to have "an umbrella" name/label for multiple IIO
channels. For example on physical "channel 1" we will measure the
voltage, the current and the power because all of those IIO channels
will point to one board power supply (like VDD, VIO, V_GPU, V_DDR,
etc).
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct pac193x_chip_info *chip_info = iio_priv(indio_dev);
> > + int len = 0;
> > + int target = (int)(attr->attr.name[strlen(attr->attr.name) -
> > 1] - '0') - 1;
> > +
> > + len += sprintf(buf, "%s\n", chip_info-
> > >channel_names[target]);
> > +
> > + return len;
> > +}
> > +
> > +static ssize_t shunt_value_store(struct device *dev,
>
> Shunt values aren't normally dynamic. Why do you need to write it
> from
> userspace? I'd expect that to come from firmware.
>
> If it's needed the ext_info framework might be a better fit than
> direct implementation of the attribute.
Yes, usually the shunt aren't normally dynamic, but what I'm aiming to
get here is to let the user to overwrite the value after a calibration
of the system. This will improve the accuracy of the reading even in
the case the shunts are not of high precision ones.
>
> > + case PAC193X_VPOWER_4_ADDR:
> > + *val = chip_info-
> > >chip_reg_data.vpower[channel];
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
> > + case IIO_ENERGY:
> > + switch (chan->address) {
> > + case PAC193X_VPOWER_ACC_1_ADDR:
> > + case PAC193X_VPOWER_ACC_2_ADDR:
> > + case PAC193X_VPOWER_ACC_3_ADDR:
> > + case PAC193X_VPOWER_ACC_4_ADDR:
> > + /*
> > + * expresses the 64 bit energy value
> > as a 32 bit integer part and
> > + * 32 bits (representing 8 digits)
> > fractional value
>
> I'm lost. So it's a 64 bit value?
> We have IIO_VAL_INT_64. Does that not work here?
>
Yes, we have the IIO_VAL_INT_64 and it will work here. The "issue" was
that the IIO_VAL_INT_64 is not available before kernel v6.0 and we
already release something to customers supporting Linux kernel v4.7 and
v5.15. Also our latest release of Linux4Sam is based on the Linux
kernel v5.15 (we are working to have a release based on v6.1).
Maybe I'm totally wrong here but my idea was to have something "more
generic" that could be easily backported to an older kernel and to
change this to IIO_VAL_INT_64 later.
> > + */
> > + curr_energy = chip_info-
> > >chip_reg_data.energy_sec_acc[channel];
> > + int_part = div_s64_rem(curr_energy,
> > 100000000, &rem);
> > +
> > + /* rescale reminder to be printed as
> > "nano" value */
> > + rem = rem * 10;
> > +
> > + *val = int_part;
> > + *val2 = rem;
> > + return IIO_VAL_INT_PLUS_NANO;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
> > + case IIO_CHAN_INFO_AVERAGE_RAW:
>
> I'd forgotten we had this ABI and had to look up the docs. Hmm.
> It's a bit clunky but I guess
> still useful in this case. Longer term we should think about how to
> describe that and how it
> is different from oversampling. The problem with oversampling is it
> affects the main channel
> so we can't easily associate both an averaged value and a raw one
> like you have here.
>
> Given lots of other changes requested and early in this kernel cycle,
> perhaps it will make sense to revisit
> this question at a later version of this patch (or maybe just leave
> it alone as too hard
> to do better!)
Here I was trying to map the IIO ABI to the PAC internal registers. We
have some hardware registers that will do an average for us. Usually
with oversampling someone could increase the bit number of the
measurement, the average is just the sum of the numbers divided by how
many numbers are and it can eliminate some noise out of the
measurements.
Here the device has registers that will keep the current values (and
will calculate the power based on those values) and also some average
measurements registers (keeping the same precision) that could help the
user to identifies trends.
>
> > + ctrl_reg = PAC193X_CRTL_SAMPLE_RATE_SET(chip_info-
> > >crt_samp_spd_bitfield) |
> > + PAC193X_CHAN_ALERT_EN;
> > + ret = pac193x_i2c_write_byte(client,
> > PAC193X_CTRL_REG, ctrl_reg);
> > + if (ret) {
> > + dev_err(&client->dev, "%s - can't update
> > sample rate\n", __func__);
> > + chip_info->sample_rate_value = old_samp_rate;
> > + mutex_unlock(&chip_info->lock);
>
> Fun thing about i2c bus write failures. You have no idea what failed.
> The value might be
> the old value or the new one after the failed write... So normally we
> don't bother too much if our values are
> out of sync. and that leads to easier unwinding. However, if you
> want to keep this as it
> is then that's fine.
>
I would like to keep it, maybe there are some corner cases when it will
help debugging (good to know in case of intermittent issues)
> > +static void pac193x_work_periodic_rfsh(struct work_struct *work)
> > +{
> > + struct pac193x_chip_info *chip_info =
> > to_pac193x_chip_info(work);
> > +
> > + /* do a REFRESH, then read */
> > + pac193x_reg_snapshot(chip_info, true, false,
> > PAC193X_MIN_UPDATE_WAIT_TIME_MS);
> > +}
> > +
> > +static void pac193x_read_reg_timeout(struct timer_list *t)
> > +{
> > + struct pac193x_chip_info *chip_info = from_timer(chip_info,
> > t, tmr_forced_update);
> > +
> > + mod_timer(&chip_info->tmr_forced_update,
> > + jiffies +
> > msecs_to_jiffies(PAC193X_MAX_RFSH_LIMIT_MS));
> > +
> > + /* schedule the periodic reading from the chip */
> > + queue_work(chip_info->wq_chip, &chip_info->work_chip_rfsh);
>
> Why not queue_delayed_work()?
>
I will need to look deeper.
>
>
>
>
> > + int revision,
> > + int function)
> > +{
> > + acpi_status status;
> > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > + union acpi_object args[PAC193X_ACPI_ARG_COUNT];
> > + struct acpi_object_list args_list;
> > + uuid_t uuid;
> > +
> > + uuid_parse(PAC193X_DSM_UUID, &uuid);
> > +
> > + args[0].type = ACPI_TYPE_BUFFER;
> > + args[0].buffer.length = sizeof(struct pac193x_uuid_format);
> > + args[0].buffer.pointer = (u8 *)&uuid;
> > +
> > + args[1].type = ACPI_TYPE_INTEGER;
> > + args[1].integer.value = revision;
> > +
> > + args[2].type = ACPI_TYPE_INTEGER;
> > + args[2].integer.value = function;
> > +
> > + args[3].type = ACPI_TYPE_PACKAGE;
> > + args[3].package.count = 0;
> > +
> > + args_list.count = PAC193X_ACPI_ARG_COUNT;
> > + args_list.pointer = &args[0];
> > +
> > + status = acpi_evaluate_object(handle, "_DSM", &args_list,
> > &buffer);
> > +
> > + if (ACPI_FAILURE(status)) {
> > + kfree(buffer.pointer);
> > + return NULL;
> > + }
> > +
> > + return buffer.pointer;
> > +}
> > +
> > +static char *pac193x_acpi_get_acpi_match_entry(acpi_handle handle)
> > +{
> > + acpi_status status;
> > + union acpi_object *name_object;
> > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > +
> > + status = acpi_evaluate_object(handle, "_HID", NULL, &buffer);
> > + name_object = buffer.pointer;
> > +
> > + return name_object->string.pointer;
> > +}
> > +
> > +static const char *pac193x_match_acpi_device(struct i2c_client
> > *client,
> > + struct pac193x_chip_info
> > *chip_info)
> > +{
> > + char *name;
> > + acpi_handle handle;
> > + union acpi_object *rez;
> > + unsigned short bi_dir_mask;
> > + int idx, i;
> > +
>
> Any docs for the ACPI stuff?
> It looks plausible, but an example DSDT blob would make it easier to
> review.
>
Here we have:
https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ApplicationNotes/ApplicationNotes/PAC193X-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf
>
>
> > + /*
> > + * count how many channels are enabled and store
> > + * this information within the driver data
> > + */
> > + cnt = 0;
> > + chip_info->chip_reg_data.num_enabled_channels = 0;
> > + while (cnt < chip_info->phys_channels) {
> > + if (chip_info->active_channels[cnt])
> > + chip_info-
> > >chip_reg_data.num_enabled_channels++;
> > + cnt++;
> > + }
> > +
> > + /*
> > + * read whatever information was gathered before the driver
> > was loaded
> > + * establish which channels are enabled/disabled and then
> > establish the
> > + * information retrieval mode (using SKIP or no).
>
> It's unusual to have a driver carry on with existing values. Much
> more common
> to either have it reset the device (to known default state) or write
> all the
> registers if such a reset doesn't exist.
>
> One reason for that flow is that we really don't trust other software
> :)
> Another is that often we don't know if the device had any power until
> we turned it
> on. So it's much easier to review code where we control all the
> state.
>
> If there is a strong reason for carrying on with existing settings
> please add
> comments here and something to the patch description to point out
> this unusual
> behaviour.
>
There are some customers asking to measure the total power exchanged
from the power-up of the system (like if you are spending a lot of time
into the bootloader and drain the battery).
>
>
>
>
> > shunt_value_store, 0);
> > +static IIO_DEVICE_ATTR(shunt_value_2, 0644, shunt_value_show,
> > shunt_value_store, 0);
> > +static IIO_DEVICE_ATTR(shunt_value_3, 0644, shunt_value_show,
> > shunt_value_store, 0);
> > +static IIO_DEVICE_ATTR(shunt_value_4, 0644, shunt_value_show,
> > shunt_value_store, 0);
> > +
> > +static IIO_DEVICE_ATTR(channel_name_1, 0444, channel_name_show,
> > NULL, 0);
> > +static IIO_DEVICE_ATTR(channel_name_2, 0444, channel_name_show,
> > NULL, 0);
> > +static IIO_DEVICE_ATTR(channel_name_3, 0444, channel_name_show,
> > NULL, 0);
> > +static IIO_DEVICE_ATTR(channel_name_4, 0444, channel_name_show,
> > NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(reset_accumulators, 0200, NULL,
> > reset_accumulators_store, 0);
> > +
> > +static struct attribute *pac193x_all_attributes[] = {
> > + PAC193X_DEV_ATTR(shunt_value_1),
>
> These all need ABI documentation so that we can easily review what
> they do.
> Documenation/ABI/testing/sysfs-bus-iio-pac1931
> Note that if they overlap with ABI used elsewhere we may need to move
> it to a more
> generic place (there can't be two lots of docs for the same ABI
> element)
>
I will add comments into the code. The "shunt_value_show" will print
the shunt value used to calculate current and power.
The "channel_name_show" is the name of the device channel used to
easily identify what we are measuring (V_DDR, V_GPU, V_IO, etc) and
it's an "umbrella" for multiple IIO measurements (voltage, current and
power)
"reset_accumulators" will help the user to reinitialize the power on
all channels.
> > + if (!indio_dev) {
> > + dev_err_probe(&indio_dev->dev, PTR_ERR(indio_dev),
> > + "Can't allocate iio device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + chip_info = iio_priv(indio_dev);
> > +
> > + i2c_set_clientdata(client, indio_dev);
>
> Assuming you follow suggestion to go fully devm managed handling of
> remove, I don't htink
> you will need this.
>
> > + chip_info->client = client;
> > +
> > + memset(&chip_info->chip_reg_data, 0, sizeof(chip_info-
> > >chip_reg_data));
>
> The iio_priv space is allocated with kzalloc so no need to zero here.
>
> > +
> > + if (ACPI_HANDLE(&client->dev)) {
> > + pac193x_get_variant(chip_info);
>
> Why is this ACPI specific?
Not really. I will change it to work in both cases.
>
> If you can query the variant from the hardware, that is the right
> thing to use
> for all registration types. It can be helpful to call out if there
> is
> a mismatch though with a warning print, so comparing the result of
> the i2c call with the data is fine.
>
> > + } else {
> > + dev_id = id->driver_data;
> > +
> > + /* store the type of chip */
> > + chip_info->chip_variant = dev_id;
> > +
> > + /* get the maximum number of channels for the given
> > chip id */
> > + chip_info->phys_channels =
> > pac193x_chip_config[dev_id].phys_channels;
> > + }
> > +
> > + /*
> > + * load default settings - all channels disabled,
> > + * uni directional flow, default shunt values
>
> The concept of a default shunt value bothers me a little if this is
> an external resistor. How is there in any real sense a default?
> Can we just fail if it's not provided - or wrap the default up for
> just ACPI case where I guess there might be firmware that doesn't
> specify it?
I'm doing some math with that resistor later in code and I was just
making sure that I will not divide by 0, but indeed I could just exit
in case the value is not correctly set.
>
Thanks,
Marius
