Re: [PATCH 2/4] hwmon: Add Qualcomm PMIC BCL hardware monitor driver

Manaf Meethalavalappu Pallikunhi
2026-02-13T06:21:43Z

Hi Krzysztof,

On 2/6/2026 1:42 PM, Krzysztof Kozlowski wrote:
> On Fri, Feb 06, 2026 at 02:44:06AM +0530, Manaf Meethalavalappu Pallikunhi wrote:
>> + bcl->hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
>> + bcl->hwmon_name,
>> + bcl,
>> + &bcl_hwmon_chip_info,
>> + NULL);
>> + if (IS_ERR(bcl->hwmon_dev)) {
>> + dev_err(&pdev->dev, "Failed to register hwmon device: %ld\n",
>> + PTR_ERR(bcl->hwmon_dev));
>> + return PTR_ERR(bcl->hwmon_dev);
>> + }
>> +
>> + dev_dbg(&pdev->dev, "BCL hwmon device with version: %u.%u registered\n",
>> + bcl_get_version_major(bcl), bcl_get_version_minor(bcl));
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id bcl_match[] = {
>> + {
>> + .compatible = "qcom,bcl-v1",
>> + .data = &pm7250b_data,
> And that's the proof that your binding is just repeating downstream
> bollocks pattern. Something like "v1" does not exist if you even here
> claim this is not "v1", but pm7250.
ACK, I will update to pmic names
>
>> + }, {
>> + .compatible = "qcom,bcl-v2",
>> + .data = &pm8350_data,
>> + }, {
>> + .compatible = "qcom,bcl-v3-bmx",
>> + .data = &pm8550b_data,
>> + }, {
>> + .compatible = "qcom,bcl-v3-wb",
>> + .data = &pmw5100_data,
>> + }, {
>> + .compatible = "qcom,bcl-v3-core",
>> + .data = &pm8550_data,
>> + }, {
>> + .compatible = "qcom,bcl-v4-bmx",
>> + .data = &pmih010_data,
>> + }, {
>> + .compatible = "qcom,bcl-v4-wb",
>> + .data = &pmw6100_data,
>> + }, {
>> + .compatible = "qcom,bcl-v4-core",
>> + .data = &pmh010_data,
>> + }, {
>> + .compatible = "qcom,bcl-v4-pmv010",
>> + .data = &pmv010_data,
>> + },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, bcl_match);
>> +
>> +static struct platform_driver bcl_driver = {
>> + .probe = bcl_probe,
>> + .driver = {
>> + .name = BCL_DRIVER_NAME,
> No, just use name here.
ACK
>
>> + .of_match_table = bcl_match,
>> + },
>> +};
>> +
>> +MODULE_AUTHOR("Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>");
>> +MODULE_DESCRIPTION("QCOM BCL HWMON driver");
>> +module_platform_driver(bcl_driver);
>> +MODULE_LICENSE("GPL");
> ...
>
>> +
>> +/**
>> + * bcl_disable_irq - Generic helper to disable alarm irq
>> + * @alarm: BCL level alarm data
>> + */
>> +static inline void bcl_disable_irq(struct bcl_alarm_data *alarm)
> Why do you write actual functions/code in the headers?
>
> Please follow standard C rules, which mean that the C unit contains code
> intended for execution. Headers are only for declarations.

ACK, I will move all codes from header to driver and remove header in
next revision based on this and other's comment.

Thanks,

Manaf

>
>> +{
>> + if (!alarm->irq_enabled)
>> + return;
>> + alarm->irq_enabled = false;
>> + disable_irq_nosync(alarm->irq);
>> + disable_irq_wake(alarm->irq);
>> +}
>> +
>> +#endif /* __QCOM_BCL_HWMON_H__ */
>>
>> --
>> 2.43.0
>>