Re: [PATCH v3 3/3] iio: adc: Add support for QCOM PMIC5 Gen3 ADC

[Jishnu Prakash <quic_jprakash@quicinc.com>]

Hi Jonathan,

(Resending this mail for tracking on mailing lists, as it got rejected 
from lists the first time due to HTML)

On 1/1/2024 11:24 PM, Jonathan Cameron wrote:
> On Sun, 31 Dec 2023 22:42:37 +0530
> Jishnu Prakash <quic_jprakash@quicinc.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.
>>


>> +
>> +	for (i = 0; i < adc->nchannels; i++) {
>> +		bool upper_set = false, lower_set = false;
>> +		int temp, offset;
>> +		u16 code = 0;
>> +
>> +		chan_prop = &adc->chan_props[i];
>> +		offset = chan_prop->tm_chan_index;
>> +
>> +		if (!chan_prop->adc_tm)
>> +			continue;
>> +
>> +		mutex_lock(&adc->lock);
>> +		if (chan_prop->sdam_index != sdam_index) {
> 
> Perhaps factor this block out as indent already high and adding scoped_guard would
> make it worse.

I don't think I can completely factor it out, as we need to update 
several local variables here (sdam_index, tm_status, buf, also chan_prop 
above), but I'll try to reduce it as much as possible.

> 
>> +			sdam_index = chan_prop->sdam_index;
>> +			ret = adc5_gen3_read(adc, sdam_index, ADC5_GEN3_TM_HIGH_STS,
>> +					tm_status, 2);
>> +			if (ret) {
>> +				dev_err(adc->dev, "adc read TM status failed with %d\n", ret);
>> +				goto out;
>> +			}


>> +
>> +static void adc5_gen3_disable(void *data)
>> +{
>> +	struct adc5_chip *adc = data;
>> +	int i;
>> +
>> +	if (adc->n_tm_channels)
>> +		cancel_work_sync(&adc->tm_handler_work);
> If this is required before the place where a simple
> devm_request_irq() will result in the irqs being cleaned up
> them register this callback earlier to avoid problems there.
> 

On checking again, it looks like I can just use devm_request_irq() and 
avoid having to free irqs explicitly here and elsewhere. I'll  still 
need to call cancel_work_sync() and I think you have also asked me to 
keep this call in another comment below. I have another question for it 
below.

>> +
>> +	for (i = 0; i < adc->num_sdams; i++)
>> +		free_irq(adc->base[i].irq, adc);
>> +
>> +	mutex_lock(&adc->lock);
>> +	/* Disable all available TM channels */
>> +	for (i = 0; i < adc->nchannels; i++) {
>> +		if (!adc->chan_props[i].adc_tm)
>> +			continue;
>> +		adc5_gen3_poll_wait_hs(adc, adc->chan_props[i].sdam_index);
>> +		_adc_tm5_gen3_disable_channel(&adc->chan_props[i]);
>> +	}
>> +
>> +	mutex_unlock(&adc->lock);
>> +}
> 


>> +
>> +	prop->hw_settle_time = VADC_DEF_HW_SETTLE_TIME;
> 
> I'd prefer to see you has through the value that maps to this after qcom_adc5_hw_settle_time_from_dt
> so then you can just set a default in value and call the rest of the code unconditionally.
> Same for the cases that follow.

I can remove the return check for fwnode_property_read_u32() as you 
suggested, but I think we still need to keep the return check for 
qcom_adc5_hw_settle_time_from_dt(), to check in case values unsupported 
in this ADC HW are set in DT. Same for the other properties.

> 
>> +	ret = fwnode_property_read_u32(fwnode, "qcom,hw-settle-time", &value);
>> +	if (!ret) {
>> +		ret = qcom_adc5_hw_settle_time_from_dt(value,
>> +						data->hw_settle_1);
>> +		if (ret < 0)
>> +			return dev_err_probe(dev, ret, "%#x invalid hw-settle-time %d us\n",
>> +				chan, value);
>> +		prop->hw_settle_time = ret;
>> +	}
>> +


>> +
>> +	chan_props = adc->chan_props;
>> +	adc->n_tm_channels = 0;
>> +	iio_chan = adc->iio_chans;
>> +	adc->data = device_get_match_data(adc->dev);
>> +	if (!adc->data)
>> +		adc->data = &adc5_gen3_data_pmic;
> 
> Why do you need a default?  Add a comment so we remember the reasoning.

On second thought, this may not be needed, I'll remove this.

> 
> 
>> +
>> +	device_for_each_child_node(adc->dev, child) {
>> +		ret = adc5_gen3_get_fw_channel_data(adc, chan_props, child, adc->data);
>> +		if (ret < 0) {


>> +
>> +		ret = platform_get_irq_byname(pdev, adc->base[i].irq_name);
>> +		if (ret < 0) {
>> +			kfree(reg);
>> +			dev_err(dev, "Getting IRQ %d by name failed, ret = %d\n",
>> +					adc->base[i].irq, ret);
>> +			goto err_irq;
>> +		}
>> +		adc->base[i].irq = ret;
>> +
>> +		ret = request_irq(adc->base[i].irq, adc5_gen3_isr, 0, adc->base[i].irq_name, adc);
> 
> Don't mix devm and non dev calls.  And don't group up multiple things in one devm callback
> as it almost always leads to bugs where for example only some irqs are allocated.

I can replace request_irq() with devm_request_irq(). But when you say 
not to group up multiple things in one devm callback, do you mean the 
devm_add_action() callback I added below or something else right here?



> 
>> +		if (ret < 0) {
>> +			kfree(reg);
>> +			dev_err(dev, "Failed to request SDAM%d irq, ret = %d\n", i, ret);
>> +			goto err_irq;
>> +		}
>> +	}
>> +	kfree(reg);
> 
> I would factor out this code and allocation of reg so you can easily use scope
> based cleanup (see linux/cleanup.h) to avoid the kfree(reg) entries that
> make for awkward code flow.
> 

The kfrees are not really needed, I'll just use devm_kcalloc to allocate 
memory for the "reg" variable. With this and devm_request_irq, I think a 
scoped guard would not be needed here.


> 
> 
>> +
>> +	ret = devm_add_action(dev, adc5_gen3_disable, adc);
> As above, this action does multiple things. Also use devm_add_action_or_reset() to cleanup
> if the devm registration fails without needing to do it manually.

I'll change it to devm_add_action_or_reset(), but do you mean I should 
call devm_add_action_or_reset() twice to register two separate callbacks 
instead of just adc5_gen3_disable? Like one for calling 
cancel_work_sync() alone and the other for the loop where we disable all 
TM channels?


> 
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to register adc disablement devm action, %d\n", ret);
>> +		goto err_irq;
>> +	}
>> +


>> +
>> +	if (adc->n_tm_channels)
>> +		INIT_WORK(&adc->tm_handler_work, tm_handler_work);
> 
> Until this init work seems unlikely you should be calling the cancel
> work in gen3_disable()

We are already calling cancel_work_sync() in adc5_gen3_disable....is 
there any change needed?


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


Thanks,

Jishnu

> 
> 
>> +
>> +	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;
>> +
>> +	ret = devm_iio_device_register(dev, indio_dev);
>> +	if (!ret)
>> +		return 0;
> Please keep error conditions as the out of line path.
> 
> 	if (ret)
> 		goto err_irq;
> 
> 	return 0;
> 
> 
>> +
>> +err_irq:
>> +	for (i = 0; i < adc->num_sdams; i++)
>> +		free_irq(adc->base[i].irq, adc);
> 
> Already freed by a devm cleanup handler.
> 
>> +
>> +	return ret;
>> +}
>