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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> 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");
>
