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

[Jishnu Prakash]

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@xxxxxxxxxxx> 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;
> > +}