Message-ID: <9e14f58f-e345-4bae-b14e-de25fc28d9a8@oss.qualcomm.com>
Date: Wed, 26 Feb 2025 14:22:05 +0530
From: Jishnu Prakash <jishnu.prakash@....qualcomm.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
        agross@...nel.org, andersson@...nel.org, dmitry.baryshkov@...aro.org,
        konradybcio@...nel.org, daniel.lezcano@...aro.org, sboyd@...nel.org,
        amitk@...nel.org, thara.gopinath@...il.com, lee@...nel.org,
        rafael@...nel.org, subbaraman.narayanamurthy@....qualcomm.com,
        david.collins@....qualcomm.com, anjelique.melendez@....qualcomm.com,
        quic_kamalw@...cinc.com, rui.zhang@...el.com, lukasz.luba@....com,
        lars@...afoo.de, devicetree@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        cros-qcom-dts-watchers@...omium.org, quic_skakitap@...cinc.com,
        neil.armstrong@...aro.org
Subject: Re: [PATCH V5 4/5] iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Hi Jonathan,

On 2/1/2025 5:41 PM, Jonathan Cameron wrote:
> On Sat,  1 Feb 2025 00:02:41 +0530
> Jishnu Prakash <jishnu.prakash@....qualcomm.com> wrote:
> 
>> The ADC architecture on PMIC5 Gen3 is similar to that on PMIC5 Gen2,
>> with all SW communication to ADC going through PMK8550 which
>> communicates with other PMICs through PBS.
>>
>> One major difference is that the register interface used here is that
>> of an SDAM (Shared Direct Access Memory) peripheral present on PMK8550.
>> There may be more than one SDAM used for ADC5 Gen3 and each has eight
>> channels, which may be used for either immediate reads (same functionality
>> as previous PMIC5 and PMIC5 Gen2 ADC peripherals) or recurring measurements
>> (same as ADC_TM functionality).
>>
>> By convention, we reserve the first channel of the first SDAM for all
>> immediate reads and use the remaining channels across all SDAMs for
>> ADC_TM monitoring functionality.
>>
>> Add support for PMIC5 Gen3 ADC driver for immediate read functionality.
>> ADC_TM is implemented as an auxiliary thermal driver under this ADC
>> driver.
>>
>> Signed-off-by: Jishnu Prakash <jishnu.prakash@....qualcomm.com>
> Hi,
> 
> A few minor things inline.  One general one is keep to under 80 chars
> for line wrap unless going over that makes a significant improvement
> to readability.
> 
> Jonathan
> 
>> ---
>> Changes since v4:
>> - Moved out common funtions from newly added .h file into a separate .c
>>   file to avoid duplicating them. Updated interrupt name as suggested
>>   by reviewer. Updated namespace export symbol statement to have a string
>>   as second argument to follow framework change.
>>

...

>> +
>> +			if (!conv_req)
>> +				return 0;
>> +		}
>> +
>> +		usleep_range(ADC5_GEN3_HS_DELAY_MIN_US, ADC5_GEN3_HS_DELAY_MAX_US);
> fsleep() perhaps as I doubt the extra tolerance that will give will matter
> much.
>> +	}
>> +
>> +	pr_err("Setting HS ready bit timed out, sdam_index:%d, status:%#x\n", sdam_index, status);
>> +	return -ETIMEDOUT;
>> +}
>> +EXPORT_SYMBOL(adc5_gen3_poll_wait_hs);
> 
> At some point may be worth namespacing all these exports.
> Probably not in this series though!

In the main driver file (qcom-spmi-adc5-gen3.c), I have already exported some functions to a namespace ("QCOM_SPMI_ADC5_GEN3"),
which is imported in the auxiliary driver file (qcom-spmi-adc-tm5-gen3.c).

Do you think I should export these functions to the same or a different namespace? Or should we check this later?


> 
>> diff --git a/drivers/iio/adc/qcom-spmi-adc5-gen3.c b/drivers/iio/adc/qcom-spmi-adc5-gen3.c
>> new file mode 100644
>> index 000000000000..9cdc2d5d2671
>> --- /dev/null
>> +++ b/drivers/iio/adc/qcom-spmi-adc5-gen3.c
>> @@ -0,0 +1,724 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2025, Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/completion.h>
>> +#include <linux/err.h>
>> +#include <linux/iio/adc/qcom-adc5-gen3-common.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/log2.h>
>> +#include <linux/math64.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +#include <linux/slab.h>
>> +#include <linux/thermal.h>
>> +#include <linux/unaligned.h>
>> +
>> +#include <dt-bindings/iio/adc/qcom,spmi-vadc.h>
>> +
>> +#define ADC5_GEN3_VADC_SDAM			0x0
>> +
>> +struct adc5_chip;
>> +
>> +/*
>> + * @adc_tm: indicates TM type if the channel is used for TM measurements.
> 
> Add docs for common_props as well and might as well make this
> full kernel-doc.
> 
>> + * @chip: pointer to top-level ADC device structure.
>> + */
>> +
>> +struct adc5_channel_prop {
>> +	struct adc5_channel_common_prop common_props;
>> +	int				adc_tm;
>> +	struct adc5_chip		*chip;
>> +};
> 
> 
>> +
>> +static int adc5_gen3_read_raw(struct iio_dev *indio_dev,
>> +			      struct iio_chan_spec const *chan, int *val, int *val2,
>> +			      long mask)
>> +{
>> +	struct adc5_chip *adc = iio_priv(indio_dev);
>> +	struct adc5_channel_common_prop *prop;
>> +	u16 adc_code_volt;
>> +	int ret;
>> +
>> +	prop = &adc->chan_props[chan->address].common_props;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_PROCESSED:
>> +		ret = adc5_gen3_do_conversion(adc, prop, &adc_code_volt);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = qcom_adc5_hw_scale(prop->scale_fn_type, prop->prescale,
>> +					 adc->data, adc_code_volt, val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = adc5_gen3_do_conversion(adc, prop, &adc_code_volt);
>> +		if (ret)
>> +			return ret;
>> +		*val = (int)adc_code_volt;
> 
> Why is the cast needed?

I think it's not needed, but this IIO_CHAN_INFO_RAW case itself may not be needed...I'll remove it.

> 
>> +		return IIO_VAL_INT;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
> 
>> +static const struct adc5_data adc5_gen3_data_pmic = {
>> +	.full_scale_code_volt = 0x70e4,
>> +	.adc_chans = adc5_gen3_chans_pmic,
>> +	.info = &adc5_gen3_info,
>> +	.decimation = (unsigned int [ADC5_DECIMATION_SAMPLES_MAX])
>> +				{85, 340, 1360},
>> +	.hw_settle_1 = (unsigned int [VADC_HW_SETTLE_SAMPLES_MAX])
>> +				{15, 100, 200, 300, 400, 500, 600, 700,
>> +				1000, 2000, 4000, 8000, 16000, 32000,
>> +				64000, 128000},
> 
> Trivial but I'm trying to slowly standardize formatting of this stuff
> in IIO.  So please add space after { and before }
> Also align the the first digit of first number in each row.
> 
> 
>> +};
> 
> 
>> +static int adc5_gen3_add_aux_tm_device(struct adc5_chip *adc)
>> +{
>> +	struct tm5_aux_dev_wrapper *aux_device;
>> +	int i, ret, i_tm = 0;
>> +
>> +	aux_device = devm_kzalloc(adc->dev, sizeof(*aux_device), GFP_KERNEL);
>> +	if (!aux_device)
>> +		return -ENOMEM;
>> +
>> +	aux_device->aux_dev.name = "adc5_tm_gen3";
>> +	aux_device->aux_dev.dev.parent = adc->dev;
>> +	aux_device->aux_dev.dev.release = adc5_gen3_aux_device_release;
>> +
>> +	aux_device->tm_props = devm_kcalloc(adc->dev, adc->n_tm_channels,
>> +					    sizeof(*aux_device->tm_props), GFP_KERNEL);
>> +	if (!aux_device->tm_props)
>> +		return -ENOMEM;
>> +
>> +	aux_device->dev_data = &adc->dev_data;
>> +
>> +	for (i = 0; i < adc->nchannels; i++) {
>> +		if (!adc->chan_props[i].adc_tm)
>> +			continue;
>> +		aux_device->tm_props[i_tm] = adc->chan_props[i].common_props;
>> +		i_tm++;
>> +	}
>> +
>> +	device_set_of_node_from_dev(&aux_device->aux_dev.dev, adc->dev);
>> +
>> +	aux_device->n_tm_channels = adc->n_tm_channels;
>> +
>> +	ret = auxiliary_device_init(&aux_device->aux_dev);
>> +	if (ret) {
>> +		kfree(&aux_device->aux_dev);
>> +		return ret;
>> +	}
>> +	ret = devm_add_action_or_reset(adc->dev, adc5_gen3_uninit_aux, &aux_device->aux_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = auxiliary_device_add(&aux_device->aux_dev);
>> +	if (ret)
>> +		return ret;
>> +	ret = devm_add_action_or_reset(adc->dev, adc5_gen3_delete_aux, &aux_device->aux_dev);
>> +	if (!ret)
>> +		adc->tm_aux = &aux_device->aux_dev;
> Keep to errors out of line, even if it costs a line or two exta
> 	if (ret)
> 		return ret;
> 
> 	adc->tm_aux = &aux_device->aux_dev;
> 
> r	return 0;
>> +
>> +	return ret;
>> +}
>> +
>> +void adc5_take_mutex_lock(struct device *dev, bool lock)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev->parent);
>> +	struct adc5_chip *adc = iio_priv(indio_dev);
>> +
>> +	if (lock)
>> +		mutex_lock(&adc->lock);
>> +	else
>> +		mutex_unlock(&adc->lock);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(adc5_take_mutex_lock, "QCOM_SPMI_ADC5_GEN3");
> 
> This is potentially going to make a mess for sparse.  Might be better to split
> it in two so you can had __acquires and __releases markings.
> 
> If you don't get any warnings with sparse then I guess we are fine.
> 

I had tried building with sparse in my local workspace and I did not get any errors in this file. Do you think I can keep this unchanged?
Also, would any kernel bots run sparse later on this patch, if it's not already done?


>> +
>> +int adc5_gen3_get_scaled_reading(struct device *dev, struct adc5_channel_common_prop *common_props,
>> +				 int *val)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev->parent);
>> +	struct adc5_chip *adc = iio_priv(indio_dev);
>> +	u16 adc_code_volt;
>> +	int ret;
>> +
>> +	ret = adc5_gen3_do_conversion(adc, common_props, &adc_code_volt);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return qcom_adc5_hw_scale(common_props->scale_fn_type, common_props->prescale,
>> +		adc->data, adc_code_volt, val);
> 
> Whilst it feels like this could all have been done with generic in kernel consumer
> interfaces, I suppose that in this case the coupling is tight enough between
> the devices that there is no real purpose in doing so.
> 
>> +}
>> +EXPORT_SYMBOL_NS_GPL(adc5_gen3_get_scaled_reading, "QCOM_SPMI_ADC5_GEN3");
>> +
>> +int adc5_gen3_therm_code_to_temp(struct device *dev, struct adc5_channel_common_prop *common_props,
>> +				 u16 code, int *val)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev->parent);
>> +	struct adc5_chip *adc = iio_priv(indio_dev);
>> +
>> +	return qcom_adc5_hw_scale(common_props->scale_fn_type, common_props->prescale,
>> +		adc->data, code, val);
> 
> Where it doesn't make much difference to readablity wrap to 80 chars and align parameters after (
> 
>> +}
>> +EXPORT_SYMBOL_NS_GPL(adc5_gen3_therm_code_to_temp, "QCOM_SPMI_ADC5_GEN3");
>> +
>> +static int adc5_gen3_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct iio_dev *indio_dev;
>> +	struct adc5_chip *adc;
>> +	struct regmap *regmap;
>> +	int ret, i;
>> +	u32 *reg;
>> +
>> +	regmap = dev_get_regmap(dev->parent, NULL);
>> +	if (!regmap)
>> +		return -ENODEV;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	adc = iio_priv(indio_dev);
>> +	adc->dev_data.regmap = regmap;
>> +	adc->dev = dev;
>> +
>> +	ret = device_property_count_u32(dev, "reg");
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	adc->num_sdams = ret;
>> +	adc->dev_data.num_sdams = adc->num_sdams;
> 
> why do we need two copies?

You're right, adc->num_sdams is not needed, I'll remove it.

> 
>> +
>> +	reg = devm_kcalloc(dev, adc->num_sdams, sizeof(u32), GFP_KERNEL);
>> +	if (!reg)
>> +		return -ENOMEM;
>> +
>> +	ret = device_property_read_u32_array(dev, "reg", reg, adc->num_sdams);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to read reg property, ret = %d\n", ret);
> 
> Look at how dev_err_probe works. You should not be explicitly printing ret.
> Fix all instances of this (and if copied from another driver, feel free to fix that too!)
> 
>> +
>> +	adc->dev_data.base = devm_kcalloc(dev, adc->num_sdams, sizeof(*adc->dev_data.base),
>> +					  GFP_KERNEL);
>> +	if (!adc->dev_data.base)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +	init_completion(&adc->complete);
>> +	mutex_init(&adc->lock);
>> +
>> +	for (i = 0; i < adc->num_sdams; i++) {
>> +		adc->dev_data.base[i].base_addr = reg[i];
>> +
>> +		adc->dev_data.base[i].irq_name = devm_kasprintf(dev, GFP_KERNEL, "sdam%d", i);
>> +		if (!adc->dev_data.base[i].irq_name)
>> +			return -ENOMEM;
>> +
>> +		ret = platform_get_irq_byname(pdev, adc->dev_data.base[i].irq_name);
>> +		if (ret < 0)
>> +			return dev_err_probe(dev, ret, "Getting IRQ %d by name failed, ret = %d\n",
>> +					adc->dev_data.base[i].irq, ret);
>> +		adc->dev_data.base[i].irq = ret;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, adc->dev_data.base[ADC5_GEN3_VADC_SDAM].irq, adc5_gen3_isr,
>> +			       0, adc->dev_data.base[ADC5_GEN3_VADC_SDAM].irq_name, adc);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "Failed to request SDAM%d irq, ret = %d\n",
>> +				ADC5_GEN3_VADC_SDAM, ret);
>> +
>> +	ret = adc5_get_fw_data(adc);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (adc->n_tm_channels > 0)
>> +		adc5_gen3_add_aux_tm_device(adc);
>> +
>> +	indio_dev->name = pdev->name;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->info = &adc5_gen3_info;
>> +	indio_dev->channels = adc->iio_chans;
>> +	indio_dev->num_channels = adc->nchannels;
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>> +}
> 
>> diff --git a/include/linux/iio/adc/qcom-adc5-gen3-common.h b/include/linux/iio/adc/qcom-adc5-gen3-common.h
>> new file mode 100644
>> index 000000000000..66edbf0ae137
>> --- /dev/null
>> +++ b/include/linux/iio/adc/qcom-adc5-gen3-common.h
>> @@ -0,0 +1,164 @@
> ...
> 
> 
>> +#define ADC5_GEN3_VIRTUAL_SID_MASK			GENMASK(15, 8)
>> +#define ADC5_GEN3_CHANNEL_MASK			GENMASK(7, 0)
>> +#define V_CHAN(x)		\
>> +	(FIELD_PREP(ADC5_GEN3_VIRTUAL_SID_MASK, (x).sid) | (x).channel)	\
> 
> That trailing \ makes little sense and will be fragile to white space
> changes.
> 
>> +
>> +enum adc5_cal_method {
>> +	ADC5_NO_CAL = 0,
>> +	ADC5_RATIOMETRIC_CAL,
>> +	ADC5_ABSOLUTE_CAL
> 
> Add a comma on trailing item.  Not immediately obvious we will never
> get anything after this so convention is to have that comma.
> That doesn't apply for terminating entries that we must not add
> antyhing after.
> 
>> +};
> ...
>> +/*
> 
> Looks like valid kernel doc, so /** and check it builds fine
> with the kernel-doc script.
> 
>> + * struct adc5_channel_prop - ADC channel property.
>> + * @channel: channel number, refer to the channel list.
>> + * @cal_method: calibration method.
>> + * @decimation: sampling rate supported for the channel.
>> + * @sid: slave id of PMIC owning the channel.
> 
> In common with most of the kernel, if there is another name that
> can be used, I'd prefer avoiding that term.
> ID probably fine for example or leave it ambiguous as SID
> 

Just to be sure, does this look fine?

@sid: ID of PMIC owning the channel.

I'll address all your other comments in the next patch series.

Thanks,
Jishnu


>> + * @label: Channel name used in device tree.
>> + * @prescale: channel scaling performed on the input signal.
>> + * @hw_settle_time: the time between AMUX being configured and the
>> + *	start of conversion.
> 
> Good to include units in the docs and maybe the field name.
> 
>> + * @avg_samples: ability to provide single result from the ADC
>> + *	that is an average of multiple measurements.
>> + * @scale_fn_type: Represents the scaling function to convert voltage
>> + *	physical units desired by the client for the channel.
>> + */
>> +struct adc5_channel_common_prop {
>> +	unsigned int			channel;
>> +	enum adc5_cal_method		cal_method;
>> +	unsigned int			decimation;
>> +	unsigned int			sid;
>> +	const char			*label;
>> +	unsigned int			prescale;
>> +	unsigned int			hw_settle_time;
>> +	unsigned int			avg_samples;
>> +	enum vadc_scale_fn_type		scale_fn_type;
>> +};
>> +
>> +struct tm5_aux_dev_wrapper {
>> +	struct auxiliary_device aux_dev;
>> +	struct adc5_device_data *dev_data;
>> +	struct adc5_channel_common_prop *tm_props;
>> +	unsigned int			n_tm_channels;
> 
> Odd indent on that last item.  Just stick to one space.
> I'd do that for all structures as trying to align has a nasty habit of
> needing noisy changes as a driver evolves in a desperate attempt to keep
> things looking pretty.
> 
>> +};
>> +
>> +struct adc_tm5_auxiliary_drv {
>> +	struct auxiliary_driver adrv;
>> +	void (*tm_event_notify)(struct auxiliary_device *adev);
>> +};
>> +
>> +int adc5_gen3_read(struct adc5_device_data *adc, unsigned int sdam_index,
>> +		   u16 offset, u8 *data, int len);
>> +
>> +int adc5_gen3_write(struct adc5_device_data *adc, unsigned int sdam_index,
>> +		    u16 offset, u8 *data, int len);
>> +
>> +int adc5_gen3_poll_wait_hs(struct adc5_device_data *adc, unsigned int sdam_index);
>> +
>> +void adc5_gen3_update_dig_param(struct adc5_channel_common_prop *prop, u8 *data);
>> +
>> +int adc5_gen3_status_clear(struct adc5_device_data *adc,
>> +			   int sdam_index, u16 offset, u8 *val, int len);
>> +
>> +void adc5_take_mutex_lock(struct device *dev, bool lock);
>> +int adc5_gen3_get_scaled_reading(struct device *dev, struct adc5_channel_common_prop *common_props,
> 
> Very long lines. Please add a break after dev,
> 
> Generally I prefer that we still aim for 80 chars in IIO, but a bit
> over is fine if useful for readability.
> 
>> +				 int *val);
>> +int adc5_gen3_therm_code_to_temp(struct device *dev, struct adc5_channel_common_prop *common_props,
>> +				 u16 code, int *val);
>> +
>> +#endif /* QCOM_ADC5_GEN3_COMMON_H */
> 

Powered by blists - more mailing lists