Re: [PATCH v5 2/2] iio: adc: adding support for PAC193x

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 22 Feb 2024 18:42:06 +0200
<marius.cristea@microchip.com> wrote:

> From: Marius Cristea <marius.cristea@microchip.com>
> 
> This is the iio driver for Microchip
> PAC193X series of Power Monitor with Accumulator chip family.
> 
> Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
So I had a few comments on this, but nothing that can't be cleaned up later.
+ I'll fix the thing the bots didn't like on the bindings.

Series applied to the togreg branch of iio.git and pushed out
as testing for 0-day to take a look at it.

Thanks,

Jonathan


> diff --git a/drivers/iio/adc/pac1934.c b/drivers/iio/adc/pac1934.c
> new file mode 100644
> index 000000000000..a67fdaba940d
> --- /dev/null
> +++ b/drivers/iio/adc/pac1934.c


> +
> +/*
> + * documentation related to the ACPI device definition
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ApplicationNotes/ApplicationNotes/PAC1934-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf
> + */
> +static bool pac1934_acpi_parse_channel_config(struct i2c_client *client,
> +					      struct pac1934_chip_info *info)
> +{
This should probably also return an int with 0 good and anything else having
a more meaningful error value if we can come up with one.

> +static bool pac1934_of_parse_channel_config(struct i2c_client *client,
> +					    struct pac1934_chip_info *info)

pac1934_fw_parse_channel_config seeing as code isn't of specific now.

Also return negative error or zero so that you can provide a more
meaningful return value in probe + it will be handy when
we make use of the device_for_each_child_node_scoped() in here
(that's so new I'm not asking people to use it this cycle).

Can make these changes later as part of the changes to use
new infrastructure I mention below.

> +{
> +	struct fwnode_handle *node, *fwnode;
> +	struct device *dev = &client->dev;
> +	unsigned int current_channel;
> +	int idx, ret;
> +
> +	info->sample_rate_value = 1024;
> +	current_channel = 1;
> +
> +	fwnode = dev_fwnode(dev);
> +	fwnode_for_each_available_child_node(fwnode, node) {

Ah this. Did we discuss this in earlier versions?
Been a while so I've forgotten.

	device_for_each_child_node() is actually implemented to only
allow for available nodes.  So you can use that directly here.
It's weird, and we've raised it several times with the fwnode
folk.

Seeing as we'll probably convert this shortly to
device_for_each_child_node_scoped() we can deal with that detail
as part of that patch.


> +		ret = fwnode_property_read_u32(node, "reg", &idx);
> +		if (ret) {
> +			dev_err_probe(dev, ret,
> +				      "reading invalid channel index\n");
> +			goto err_fwnode;
> +		}
> +		/* adjust idx to match channel index (1 to 4) from the datasheet */
> +		idx--;
> +
> +		if (current_channel >= (info->phys_channels + 1) ||
> +		    idx >= info->phys_channels || idx < 0) {
> +			dev_err_probe(dev, -EINVAL,
> +				      "%s: invalid channel_index %d value\n",
> +				      fwnode_get_name(node), idx);
> +			goto err_fwnode;
> +		}
> +
> +		/* enable channel */
> +		info->active_channels[idx] = true;
> +
> +		ret = fwnode_property_read_u32(node, "shunt-resistor-micro-ohms",
> +					       &info->shunts[idx]);
> +		if (ret) {
> +			dev_err_probe(dev, ret,
> +				      "%s: invalid shunt-resistor value: %d\n",
> +				      fwnode_get_name(node), info->shunts[idx]);
> +			goto err_fwnode;
> +		}
> +
> +		if (fwnode_property_present(node, "label")) {
> +			ret = fwnode_property_read_string(node, "label",
> +							  (const char **)&info->labels[idx]);
> +			if (ret) {
> +				dev_err_probe(dev, ret,
> +					      "%s: invalid rail-name value\n",
> +					      fwnode_get_name(node));
> +				goto err_fwnode;
> +			}
> +		}
> +
> +		info->bi_dir[idx] = fwnode_property_read_bool(node, "bipolar");
> +
> +		current_channel++;
> +	}
> +
> +	return true;
> +
> +err_fwnode:
> +	fwnode_handle_put(node);
> +
> +	return false;
> +}

> +static int pac1934_prep_iio_channels(struct pac1934_chip_info *info, struct iio_dev *indio_dev)
> +{
> +	struct iio_chan_spec *ch_sp;
> +	int channel_size, attribute_count, cnt;
> +	void *dyn_ch_struct, *tmp_data;
> +	struct device *dev = &info->client->dev;
> +
> +	/* find out dynamically how many IIO channels we need */
> +	attribute_count = 0;
> +	channel_size = 0;
> +	for (cnt = 0; cnt < info->phys_channels; cnt++) {
> +		if (!info->active_channels[cnt])
> +			continue;
> +
> +		/* add the size of the properties of one chip physical channel */
> +		channel_size += sizeof(pac1934_single_channel);
> +		/* count how many enabled channels we have */
> +		attribute_count += ARRAY_SIZE(pac1934_single_channel);
> +		dev_info(dev, ":%s: Channel %d active\n",
dev_dbg()
This is too noisy really given I'd assume that's easy to establish
after the driver is loaded. If nothing else comes up I'll make this dev_dbg
whilst applying.
> +			 __func__, cnt + 1);
> +	}