On Wed, 26 Feb 2025 14:22:24 +0530
Jishnu Prakash <jishnu.prakash@xxxxxxxxxxxxxxxx> 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@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().
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");
> >
>
