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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 26 Feb 2025 14:22:24 +0530
Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:

> 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().

Yes. Thinking more on this it uses that parameter as a source of naming for
some of the stuff it creates.

Thanks for trying this, and no problem with sticking to you original code
given this doesn't work :(

Jonathan


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