Message-ID: <ff19780e-5bbd-4074-9db3-b4f27922a093@oss.qualcomm.com>
Date: Tue, 24 Jun 2025 18:58:24 +0530
From: Jishnu Prakash <jishnu.prakash@....qualcomm.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: robh@...nel.org, krzysztof.kozlowski@...aro.org, krzk+dt@...nel.org,
        conor+dt@...nel.org, agross@...nel.org, andersson@...nel.org,
        lumag@...nel.org, dmitry.baryshkov@....qualcomm.com,
        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,
        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,
        stephan.gerhold@...aro.org
Subject: Re: [PATCH V6 4/5] iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Hi Jonathan,

On 5/11/2025 6:34 PM, Jonathan Cameron wrote:
> On Fri,  9 May 2025 16:39:58 +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.
>>

...

>> +#include <linux/delay.h>
>> +#include <linux/iio/adc/qcom-adc5-gen3-common.h>
>> +
>> +int adc5_gen3_read(struct adc5_device_data *adc, unsigned int sdam_index,
>> +		   u16 offset, u8 *data, int len)
>> +{
>> +	return regmap_bulk_read(adc->regmap,
>> +				adc->base[sdam_index].base_addr + offset,
>> +				data, len);
>> +}
>> +EXPORT_SYMBOL(adc5_gen3_read);
> 
> Consider namespacing these exports.
> 

OK, I'll add them in the existing namespace (QCOM_SPMI_ADC5_GEN3)
if there's no issue.

>> +
>> +int adc5_gen3_write(struct adc5_device_data *adc, unsigned int sdam_index,
>> +		    u16 offset, u8 *data, int len)
>> +{
>> +	return regmap_bulk_write(adc->regmap,
>> +				 adc->base[sdam_index].base_addr + offset,
>> +				 data, len);
>> +}
>> +EXPORT_SYMBOL(adc5_gen3_write);
>> +
>> +/*

...

>> +	if (status & ADC5_GEN3_STATUS1_CONV_FAULT) {
>> +		dev_err_ratelimited(adc->dev,
>> +				    "Unexpected conversion fault, status:%#x, eoc_status:%#x\n",
>> +				    status, eoc_status);
>> +		val = ADC5_GEN3_CONV_ERR_CLR_REQ;
>> +		adc5_gen3_status_clear(&adc->dev_data, ADC5_GEN3_VADC_SDAM,
>> +				       ADC5_GEN3_CONV_ERR_CLR, &val, 1);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	/* CHAN0 is the preconfigured channel for immediate conversion */
>> +	if (eoc_status & ADC5_GEN3_EOC_CHAN_0)
>> +		complete(&adc->complete);
>> +
>> +	ret = adc5_gen3_read(&adc->dev_data, ADC5_GEN3_VADC_SDAM,
>> +			     ADC5_GEN3_TM_HIGH_STS, tm_status, sizeof(tm_status));
>> +	if (ret) {
>> +		dev_err(adc->dev, "adc read TM status failed with %d\n", ret);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	if (tm_status[0] || tm_status[1]) {
>> +		adev = adc->tm_aux;
>> +		if (!adev || !adev->dev.driver) {
>> +			dev_err(adc->dev,
>> +				"adc_tm auxiliary device not initialized\n");
>> +			return IRQ_HANDLED;
>> +		}
>> +
>> +		adrv_tm = container_of(adev->dev.driver,
>> +				       struct adc_tm5_auxiliary_drv,
>> +				       adrv.driver);
>> +
>> +		if (adrv_tm && adrv_tm->tm_event_notify)
>> +			adrv_tm->tm_event_notify(adev);
>> +		else
>> +			dev_err(adc->dev,
>> +				"adc_tm auxiliary driver not initialized\n");
> 
> Not return?  Seems odd to print the dbg print only in this error path path.

In that case, I'll move the print upwards, so that it appears just before
the if() check checking tm_status[]. I think it might be useful to have this
print appear in the error paths.

> 
>> +	}
>> +
>> +	dev_dbg(adc->dev,
>> +		"Interrupt status:%#x, EOC status:%#x, high:%#x, low:%#x\n",
>> +		status, eoc_status, tm_status[0], tm_status[1]);
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
>> +static int adc5_gen3_get_fw_channel_data(struct adc5_chip *adc,
>> +					 struct adc5_channel_prop *prop,
>> +					 struct fwnode_handle *fwnode)
>> +{
>> +	const char *name = fwnode_get_name(fwnode);
>> +	const struct adc5_data *data = adc->data;

...

>> +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},
> 
> Inconsistent spacing. Should be { 85 etc
> 
>> +	.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 },
> Andy often points this out, but I'll do it this time. Fixed numbers (typically power of 2)
> elements per line make it much easier to see which element is which in these arrays.
> Reduce the indent a little to allow that here.

Does this look fine?

	.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 },


> 
>> +};
>> +
>> +static const struct of_device_id adc5_match_table[] = {
>> +	{
>> +		.compatible = "qcom,spmi-adc5-gen3",
>> +		.data = &adc5_gen3_data_pmic,
>> +	},
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, adc5_match_table);
>> +
>> +static int adc5_get_fw_data(struct adc5_chip *adc)

...


>> +
>> +static void adc5_gen3_uninit_aux(void *data)
>> +{
>> +	auxiliary_device_uninit(data);
>> +}
>> +
>> +static void adc5_gen3_delete_aux(void *data)
>> +{
>> +	auxiliary_device_delete(data);
>> +}
>> +
>> +static void adc5_gen3_aux_device_release(struct device *dev)
>> +{
>> +	struct auxiliary_device *aux = container_of(dev, struct auxiliary_device,
>> +						    dev);
>> +
>> +	kfree(aux);
> 
> It wasn't allocated at that granularity.
> 
>> +}
>> +
>> +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);
> 
> There is some lifetime management stuff that is going wrong here.
> Here you allocate a structure that directly contains the
> struct auxiliary_device and use devm managed allocation.
> 
> But you free the contained struct auxiliary_device via the release
> above. Firstly that's freeing at a different granularity which is going to
> go wrong.  Also, that pointer is the same as aux_device here (as first element)
> and so you free this via devm_ cleanup and on the reference count of the
> auxiliary device dropping to zero.
> 
> 
> Take a look at some other drivers that make use of auxdevs for
> how to handle this.
> 
> Key is that the devm handler should be reducing the refcount, not freeing
> the memory as the release will deal with that later.
> 

Thanks for catching it, I'll fix this in the next patch series. I think
keeping adc5_gen3_aux_device_release() empty should be the right thing
to do in this case.

I had tried removing and reloading the main and auxiliary ADC drivers
using modprobe, but somehow I did not see any issue immediately, when
I tried this only once or twice. When I repeated removal and reinsertion
several times, I eventually ran into a segmentation fault. I verified
that when I tried the same steps keeping adc5_gen3_aux_device_release()
empty, this did not happen.

>> +	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);
> 
> What is this freeing? 

This is also wrong as it frees the auxiliary device struct under the wrapper
instance aux_device, which has its memory allocated with devm_kzalloc.
I'll correct this too in the next patch series.

> 
>> +		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)
>> +		return ret;
>> +
>> +	adc->tm_aux = &aux_device->aux_dev;
>> +
>> +	return 0;
>> +}
> 
>> +static int adc5_gen3_probe(struct platform_device *pdev)
>> +{
> 
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +	init_completion(&adc->complete);
>> +	mutex_init(&adc->lock);
> If spinning again for other reasons, in new code I have slight preference for
> 	ret = devm_mutex_init(&adc->lock);
> 	if (ret)
> 		return ret;
> 
> It was never worth bothering with release until we had devm managed form but
> now we do the code complexity cost is low enough to make it reasonable.
> 
>> +	indio_dev->name = pdev->name;
> 
> Just to check.  Does that end up as a part number or similar?

I printed this name and it appeared like this:

indio_dev->name: c426000.spmi:pmic@0:adc@...0

It only gets the DT node names, which are generic, there are 
no part numbers in this name.

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

Thanks,
Jishnu

> 
>> +	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..4f476cd77b37
>> --- /dev/null
>> +++ b/include/linux/iio/adc/qcom-adc5-gen3-common.h
>> @@ -0,0 +1,193 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> 
>> +#define V_CHAN(x)		\
> 
> Might be a good idea to prefix this.  Seems likely we might end up with
> a V_CHAN macro in some generic header in future.
> 
>> +	(FIELD_PREP(ADC5_GEN3_VIRTUAL_SID_MASK, (x).sid) | (x).channel)

Powered by blists - more mailing lists