Re: [PATCH V5 5/5] thermal: qcom: add support for PMIC5 Gen3 ADC thermal monitoring

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

Hi Jonathan,

On 2/1/2025 5:57 PM, Jonathan Cameron wrote:
> On Sat,  1 Feb 2025 00:02:42 +0530
> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> 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.
> 
> Very short wrap. For commit descriptions 75 chars is fine.
> 
>>
>> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
> Various minor comments inline.
> 
> Jonathan
> 
>> ---
>> Changes since v4:
>> - Fixed a compilation error and updated dependencies in config as suggested
>>   by reviewer.

...

>>
>> +
>> +MODULE_DEVICE_TABLE(auxiliary, adctm5_auxiliary_id_table);
>> +
>> +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);
> 
> Can use module_auxiliary_driver() to replace this boilerplate.
> The embedded adrv shouldn't stop that working that I can see.
> 

I tried to do this, but it does not work with the embedded adrv.


When I tried this change:

    -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);
    +module_auxiliary_driver(adctm5gen3_auxiliary_drv.adrv);


Ideally this should have worked as I see module_auxiliary_driver() takes a single argument of type struct auxiliary_driver. But it failed with errors like this:


    drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c:474:49: error: expected '=', ',', ';', 'asm' or '__attribute__' before '.' token
     module_auxiliary_driver(adctm5gen3_auxiliary_drv.adrv);
                                                     ^

    drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c:474:49: error: 'struct adc_tm5_auxiliary_drv' has no member named 'adrv_init'
     module_auxiliary_driver(adctm5gen3_auxiliary_drv.adrv);
                                                     ^

    drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c:474:49: error: 'struct adc_tm5_auxiliary_drv' has no member named 'adrv_exit'
     module_auxiliary_driver(adctm5gen3_auxiliary_drv.adrv);
                                                     ^


I think this happens because module_auxiliary_driver() is defined as a macro, like this:

    #define module_auxiliary_driver(__auxiliary_driver) \
        module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)

And when the text substitution for the argument is done, we would end up with lines like this in the expansion finally:

    module_init(adctm5gen3_auxiliary_drv.adrv_init);
    module_exit(adctm5gen3_auxiliary_drv.adrv_exit);


I'm facing similar issues, of the input argument being misinterpreted, if I use a pointer to the struct auxiliary_driver member (adrv), and dereference it as argument to module_auxiliary_driver().

I think module_auxiliary_driver() can only take a simple variable name as input, because in all the examples of its usage I found, I see there is a "struct auxiliary_driver" initialization just before the initialized variable is passed to module_auxiliary_driver().

In this auxiliary driver, I need to have adrv embedded within the struct adc_tm5_auxiliary_drv wrapper, as I also need to have the .tm_event_notify member, to expose a callback to the main driver, so I don't think I can change this.


I'll address your other comments in the next patch series.

Thanks,
Jishnu

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