Re: [PATCH V8 4/4] thermal: qcom: add support for PMIC5 Gen3 ADC thermal monitoring

[Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>]

Hi Dmitry,

On 12/6/2025 7:54 AM, Dmitry Baryshkov wrote:
> On Thu, Nov 27, 2025 at 07:10:36PM +0530, Jishnu Prakash wrote:
>> Add support for ADC_TM part of PMIC5 Gen3.
>>
>> This is an auxiliary driver under the Gen3 ADC driver, which implements the
>> threshold setting and interrupt generating functionalities of QCOM ADC_TM
>> drivers, used to support thermal trip points.
>>
>> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
>> ---

...

>>
>>  drivers/thermal/qcom/Kconfig                  |   9 +
>>  drivers/thermal/qcom/Makefile                 |   1 +
>>  drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c | 530 ++++++++++++++++++
>>  3 files changed, 540 insertions(+)
>>  create mode 100644 drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
>>
>> diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
>> index a6bb01082ec6..1acb11e4ac80 100644
>> --- a/drivers/thermal/qcom/Kconfig
>> +++ b/drivers/thermal/qcom/Kconfig
>> @@ -21,6 +21,15 @@ config QCOM_SPMI_ADC_TM5
>>  	  Thermal client sets threshold temperature for both warm and cool and
>>  	  gets updated when a threshold is reached.
>>  
>> +config QCOM_SPMI_ADC_TM5_GEN3
>> +	tristate "Qualcomm SPMI PMIC Thermal Monitor ADC5 Gen3"
>> +	depends on QCOM_SPMI_ADC5_GEN3
> 
> This module depends directly on the Gen3 ADC driver. I think you can
> drop a separate "common" submodule.
> 

Yes, I can do this in the next patch series.

>> +	help
>> +	  This enables the auxiliary thermal driver for the ADC5 Gen3 thermal
>> +	  monitoring device. It shows up as a thermal zone with multiple trip points.
>> +	  Thermal client sets threshold temperature for both warm and cool and
>> +	  gets updated when a threshold is reached.
>> +
>>  config QCOM_SPMI_TEMP_ALARM
>>  	tristate "Qualcomm SPMI PMIC Temperature Alarm"
>>  	depends on OF && SPMI && IIO
> 
> 
>> +
>> +static struct adc_tm5_auxiliary_drv adctm5gen3_auxiliary_drv = {
>> +	.adrv = {
>> +		.id_table = adctm5_auxiliary_id_table,
>> +		.probe = adc_tm5_probe,
>> +	},
>> +	.tm_event_notify = adctm_event_handler,
>> +};
>> +
>> +static int __init adctm5_init_module(void)
>> +{
>> +	return auxiliary_driver_register(&adctm5gen3_auxiliary_drv.adrv);
>> +}
>> +
>> +static void __exit adctm5_exit_module(void)
>> +{
>> +	auxiliary_driver_unregister(&adctm5gen3_auxiliary_drv.adrv);
>> +}
>> +
>> +module_init(adctm5_init_module);
>> +module_exit(adctm5_exit_module);
> 
> We really need to make this work with module_auxiliary_driver-like
> macro.
> 

I tried doing this again now, but I'm not sure if the way I found is fine.


Just to recap, the main issue with using module_auxiliary_driver() directly,
which I discussed with Jonathan here earlier: (https://lore.kernel.org/all/20250301032901.7b38fed4@jic23-huawei/)
was that it is a macro definition which uses its input argument to
generate function names. So if I have a line like this:

    module_auxiliary_driver(adctm5gen3_auxiliary_drv.adrv);

it will generate function definitions like this, due to text substitutions:

    static int __init adctm5gen3_auxiliary_drv.adrv_init(void)

which will fail compilation.



I see that in other drivers where module_auxiliary_driver() is used, there is a 
"struct auxiliary_driver" initialization just before that initialized variable 
is passed to module_auxiliary_driver(). I tried making a similar change here now:


    -static struct adc_tm5_auxiliary_drv adctm5gen3_auxiliary_drv = {
    -       .adrv = {
    -               .id_table = adctm5_auxiliary_id_table,
    -               .probe = adc_tm5_probe,
    -       },
    -       .tm_event_notify = adctm_event_handler,
    +static struct auxiliary_driver adctm5gen3_auxiliary_driver = {
    +    .id_table = adctm5_auxiliary_id_table,
    +    .probe = adc_tm5_probe,
     };
    
    -static int __init adctm5_init_module(void)
    -{
    -       return auxiliary_driver_register(&adctm5gen3_auxiliary_drv.adrv);
    -}
    -
    -static void __exit adctm5_exit_module(void)
    -{
    -       auxiliary_driver_unregister(&adctm5gen3_auxiliary_drv.adrv);
    -}
    +struct adc_tm5_auxiliary_drv adctm5gen3_auxiliary_drv = {
    +       .adrv = adctm5gen3_auxiliary_driver,
    +       .tm_event_notify = adctm_event_handler,
    +};
    
    -module_init(adctm5_init_module);
    -module_exit(adctm5_exit_module);
    +module_auxiliary_driver(adctm5gen3_auxiliary_driver);

With this, I get the following error:

    drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c:513:10: error: initializer element is not constant
      513 |  .adrv = adctm5gen3_auxiliary_driver,
          |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~


It looks like the above definiton of adctm5gen3_auxiliary_driver is not considered
a compile-time constant. I made the following modification to fix this:

    -static struct auxiliary_driver adctm5gen3_auxiliary_driver = {
    +static const struct auxiliary_driver adctm5gen3_auxiliary_driver = {


And with this, the code does get built, but with warnings like this:


    drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c: In function ‘adctm5gen3_auxiliary_driver_init’:
    ./include/linux/device/driver.h:260:20: warning: passing argument 1 of ‘__auxiliary_driver_register’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
      260 |  return __register(&(__driver) , ##__VA_ARGS__); \
          |                    ^~~~~~~~~~~
    ./include/linux/auxiliary_bus.h:253:30: note: in definition of macro ‘auxiliary_driver_register’
      253 |  __auxiliary_driver_register(auxdrv, THIS_MODULE, KBUILD_MODNAME)
          |                              ^~~~~~
    ./include/linux/auxiliary_bus.h:287:2: note: in expansion of macro ‘module_driver’
      287 |  module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
          |  ^~~~~~~~~~~~~
    drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c:517:1: note: in expansion of macro ‘module_auxiliary_driver’
      517 | module_auxiliary_driver(adctm5gen3_auxiliary_driver);
          | ^~~~~~~~~~~~~~~~~~~~~~~
    ./include/linux/auxiliary_bus.h:250:58: note: expected ‘struct auxiliary_driver *’ but argument is of type ‘const struct auxiliary_driver *’
      250 | int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct module *owner,
          |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~


Is it acceptable to have the above code changes to use module_auxiliary_driver(),
even with the warnings generated? 

Or do you have any other suggestions?

Thanks,
Jishnu


>> +
>> +MODULE_DESCRIPTION("SPMI PMIC Thermal Monitor ADC driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_IMPORT_NS("QCOM_SPMI_ADC5_GEN3");
>> -- 
>> 2.25.1
>>
>