mbox series

[v3,0/3] iio: adc: Add support for QCOM SPMI PMIC5 Gen3 ADC

Message ID 20231231171237.3322376-1-quic_jprakash@quicinc.com
Headers show
Series iio: adc: Add support for QCOM SPMI PMIC5 Gen3 ADC | expand

Message

Jishnu Prakash Dec. 31, 2023, 5:12 p.m. UTC
PMIC5 Gen3 has a similar ADC architecture to that on PMIC5 Gen2,
with all SW communication to ADC going through PMK8550 which
communicates with other PMICs through PBS. The major difference is
that the register interface used here is that of an SDAM present on
PMK8550, rather than a dedicated ADC peripheral. There may be more than one
SDAM used for ADC5 Gen3. Each ADC SDAM has eight channels, each of which may
be used for either immediate reads (same functionality as previous PMIC5 and
PMIC5 Gen2 ADC peripherals) or recurring measurements (same as PMIC5 and PMIC5
Gen2 ADC_TM functionality). In this case, we have VADC and ADC_TM functionality
combined into the same driver.

Patch 1 is a cleanup, to move the QCOM ADC dt-bindings files from
dt-bindings/iio to dt-bindings/iio/adc folder, as they are
specifically for ADC devices. It also fixes all compilation errors
with this change in driver and devicetree files and similar errors
in documentation for dtbinding check.

Patch 2 adds bindings for ADC5 Gen3 peripheral.

Patch 3 adds driver support for ADC5 Gen3.

Changes since v2:
- Reordered patches to keep cleanup change for ADC files first.
- Moved ADC5 Gen3 documentation into a separate file

Changes since v1:
- Dropped patches 1-5 for changing 'ADC7' peripheral name to 'ADC5 Gen2'.
- Addressed reviewer comments for binding and driver patches for ADC5 Gen3.
- Combined patches 8-11 into a single patch as requested by reviewers to make
  the change clearer and made all fixes required in same patch.

Jishnu Prakash (3):
  dt-bindings: iio/adc: Move QCOM ADC bindings to iio/adc folder
  dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC
  iio: adc: Add support for QCOM PMIC5 Gen3 ADC

 .../bindings/iio/adc/qcom,spmi-adc5-gen3.yaml |  212 +++
 .../bindings/iio/adc/qcom,spmi-vadc.yaml      |    4 +-
 .../bindings/mfd/qcom,spmi-pmic.yaml          |    2 +-
 .../bindings/thermal/qcom-spmi-adc-tm-hc.yaml |    2 +-
 .../bindings/thermal/qcom-spmi-adc-tm5.yaml   |    6 +-
 arch/arm64/boot/dts/qcom/pm2250.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm6125.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm6150.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm6150l.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pm660.dtsi           |    2 +-
 arch/arm64/boot/dts/qcom/pm660l.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm7250b.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pm8150.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm8150b.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pm8150l.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pm8916.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm8950.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm8953.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm8994.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm8998.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pmi632.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pmi8950.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pmm8155au_1.dtsi     |    2 +-
 arch/arm64/boot/dts/qcom/pmp8074.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pms405.dtsi          |    2 +-
 .../boot/dts/qcom/qcm6490-fairphone-fp5.dts   |    4 +-
 arch/arm64/boot/dts/qcom/sc7280-idp.dts       |    2 +-
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi      |    2 +-
 arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi    |    4 +-
 arch/arm64/boot/dts/qcom/sc8180x-pmics.dtsi   |    2 +-
 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    |    6 +-
 .../boot/dts/qcom/sm7225-fairphone-fp4.dts    |    2 +-
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts       |    8 +-
 drivers/iio/adc/Kconfig                       |   25 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/qcom-spmi-adc5-gen3.c         | 1198 +++++++++++++++++
 drivers/iio/adc/qcom-spmi-adc5.c              |    2 +-
 drivers/iio/adc/qcom-spmi-vadc.c              |    2 +-
 .../iio/adc/qcom,spmi-adc5-gen3-pm8550.h      |   50 +
 .../iio/adc/qcom,spmi-adc5-gen3-pm8550b.h     |   89 ++
 .../iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h    |   22 +
 .../iio/adc/qcom,spmi-adc5-gen3-pmk8550.h     |   56 +
 .../iio/{ => adc}/qcom,spmi-adc7-pm7325.h     |    2 +-
 .../iio/{ => adc}/qcom,spmi-adc7-pm8350.h     |    2 +-
 .../iio/{ => adc}/qcom,spmi-adc7-pm8350b.h    |    2 +-
 .../iio/{ => adc}/qcom,spmi-adc7-pmk8350.h    |    2 +-
 .../iio/{ => adc}/qcom,spmi-adc7-pmr735a.h    |    2 +-
 .../iio/{ => adc}/qcom,spmi-adc7-pmr735b.h    |    2 +-
 .../iio/{ => adc}/qcom,spmi-adc7-smb139x.h    |    2 +-
 .../iio/{ => adc}/qcom,spmi-vadc.h            |   81 ++
 50 files changed, 1785 insertions(+), 51 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml
 create mode 100644 drivers/iio/adc/qcom-spmi-adc5-gen3.c
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm7325.h (98%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350.h (98%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350b.h (99%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmk8350.h (97%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735a.h (95%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735b.h (95%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-smb139x.h (93%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-vadc.h (77%)

Comments

Jishnu Prakash Feb. 14, 2024, 1:58 p.m. UTC | #1
Hi Dmitry,

On 12/31/2023 11:16 PM, Dmitry Baryshkov wrote:
> On Sun, 31 Dec 2023 at 19:13, 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.

>> +static int adc_tm_register_tzd(struct adc5_chip *adc)
>> +{
>> +       unsigned int i, channel;
>> +       struct thermal_zone_device *tzd;
>> +
>> +       for (i = 0; i < adc->nchannels; i++) {
>> +               channel = V_CHAN(adc->chan_props[i]);
>> +
>> +               if (!adc->chan_props[i].adc_tm)
>> +                       continue;
>> +               tzd = devm_thermal_of_zone_register(adc->dev, channel,
>> +                       &adc->chan_props[i], &adc_tm_ops);
> It is _very_ useful to register a hwmon too by calling
> devm_thermal_add_hwmon_sysfs(). However this becomes tricky, as this
> function is not defined in one of the global headers.
>
> This actually points out an issue. You have the ADC driver fused
> together with the thermal driver. Can I suggest using the aux device
> to split the thermal functionality to the separate driver?
>
> This way it would be possible to use the ADC without any thermal
> monitoring in place.


There are a couple of issues which may make it harder to split the 
thermal functionality from this driver into an auxiliary driver as you 
mentioned.

For one, we use the same set of registers (offsets 0x4f-0x55) for both 
VADC function(requesting an immediate channel reading) and ADC_TM 
function (setting upper/lower thermal thresholds on a channel). To avoid 
any race conditions, we would need to share a mutex between the 
top-level ADC driver and the auxiliary ADC_TM thermal driver to avoid 
concurrently accessing these or any other shared registers.

In addition, the device has only one interrupt with one interrupt 
handler, and it gets triggered for both VADC and ADC_TM  events (end of 
conversion and threshold violation, respectively). The handler checks 
for both types of event and handles it as required.

For the shared interrupt, we may be able to keep the interrupt handler 
in the top-level driver and just notify the auxiliary TM driver if a 
threshold violation is detected. For the shared mutex, I think the 
auxiliary driver may be able to access the parent driver's mutex, but 
I'll need to check more for the implementation in both of these cases.

Please let me know if you see any problems with this kind of 
implementation or if you have any additional comments.

Thanks,

Jishnu

>> +
>> +               if (IS_ERR(tzd)) {
>> +                       if (PTR_ERR(tzd) == -ENODEV) {
>> +                               dev_warn(adc->dev, "thermal sensor on channel %d is not used\n",
>> +                                        channel);
>> +                               continue;
>> +                       }
>> +
>>
>
Dmitry Baryshkov Feb. 16, 2024, 10:52 a.m. UTC | #2
On Wed, 14 Feb 2024 at 15:58, Jishnu Prakash <quic_jprakash@quicinc.com> wrote:
>
> Hi Dmitry,
>
> On 12/31/2023 11:16 PM, Dmitry Baryshkov wrote:
> > On Sun, 31 Dec 2023 at 19:13, 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.
>
> >> +static int adc_tm_register_tzd(struct adc5_chip *adc)
> >> +{
> >> +       unsigned int i, channel;
> >> +       struct thermal_zone_device *tzd;
> >> +
> >> +       for (i = 0; i < adc->nchannels; i++) {
> >> +               channel = V_CHAN(adc->chan_props[i]);
> >> +
> >> +               if (!adc->chan_props[i].adc_tm)
> >> +                       continue;
> >> +               tzd = devm_thermal_of_zone_register(adc->dev, channel,
> >> +                       &adc->chan_props[i], &adc_tm_ops);
> > It is _very_ useful to register a hwmon too by calling
> > devm_thermal_add_hwmon_sysfs(). However this becomes tricky, as this
> > function is not defined in one of the global headers.
> >
> > This actually points out an issue. You have the ADC driver fused
> > together with the thermal driver. Can I suggest using the aux device
> > to split the thermal functionality to the separate driver?
> >
> > This way it would be possible to use the ADC without any thermal
> > monitoring in place.
>
>
> There are a couple of issues which may make it harder to split the
> thermal functionality from this driver into an auxiliary driver as you
> mentioned.
>
> For one, we use the same set of registers (offsets 0x4f-0x55) for both
> VADC function(requesting an immediate channel reading) and ADC_TM
> function (setting upper/lower thermal thresholds on a channel). To avoid
> any race conditions, we would need to share a mutex between the
> top-level ADC driver and the auxiliary ADC_TM thermal driver to avoid
> concurrently accessing these or any other shared registers.

Just export an API performing this access. No need to export data (aka mutex).

>
> In addition, the device has only one interrupt with one interrupt
> handler, and it gets triggered for both VADC and ADC_TM  events (end of
> conversion and threshold violation, respectively). The handler checks
> for both types of event and handles it as required.

You can extend auxiliary drivers with the custom callbacks, see
drivers/base/auxiliary.c .
I think you can call a callback from ADC_TM driver from your ADC driver.

>
> For the shared interrupt, we may be able to keep the interrupt handler
> in the top-level driver and just notify the auxiliary TM driver if a
> threshold violation is detected. For the shared mutex, I think the
> auxiliary driver may be able to access the parent driver's mutex, but
> I'll need to check more for the implementation in both of these cases.
>
> Please let me know if you see any problems with this kind of
> implementation or if you have any additional comments.
>
> Thanks,
>
> Jishnu
>
> >> +
> >> +               if (IS_ERR(tzd)) {
> >> +                       if (PTR_ERR(tzd) == -ENODEV) {
> >> +                               dev_warn(adc->dev, "thermal sensor on channel %d is not used\n",
> >> +                                        channel);
> >> +                               continue;
> >> +                       }
> >> +
> >>
> >
Jishnu Prakash Feb. 16, 2024, 11:44 a.m. UTC | #3
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;
>> +}
>
Jonathan Cameron Feb. 16, 2024, 1:54 p.m. UTC | #4
On Fri, 16 Feb 2024 16:10:18 +0530
Jishnu Prakash <quic_jprakash@quicinc.com> wrote:

> Hi Jonathan,
> 
> 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.

Keeping it is fine, just make sure it's registered in the right location
to ensure it is taken down after we are sure it can't get scheduled again
(I think that is what I was getting at - been a while!)
> 
> 
> >> +
> >> +	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.

Sure, you can check for errors in using the property. Just do it unconditionally
so that you call the same code for the default (even though you know that will
be fine).  Should simplify the code.

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

> >  
> >> +
> >> +	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?

Yes, I meant the devm_add_action() callback.

> 
> 
> >> +		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.

If you don't need it after this function, then better to clean it up.

> 
> 
> >
> >  
> >> +
> >> +	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?

yes

> 
> 
> >> +	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?
Yes - add a devm_add_action() here (not need for reset in this case as nothing
can have queued any work yet) and handle this on it's own.

Each cleanup action should match with a setup action.

Jonathan