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