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

[Manaf Meethalavalappu Pallikunhi]

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