Re: [PATCH v2 5/7] mfd: qcom-pm8008: Add PM8010 support

[Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>]

On 10/14/2026 4:14 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] mfd: silent fallback to PM8008 on non-DT PM8010 instances
> --
> 
> commit 4e77301709b8663667033b7dfed82cf937a2d7c5
> Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> mfd: qcom-pm8008: Add PM8010 support
> 
> This commit adds support for the PM8010 to the qcom-pm8008 driver. It
> incorporates PM8010 specific IRQs, MFD cells, and match data to support the
> new device type via I2C.
> 
>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>> index 2d6c01cf54f32..4179427f38911 100644
>> --- a/drivers/mfd/qcom-pm8008.c
>> +++ b/drivers/mfd/qcom-pm8008.c
> [ ... ]
>> @@ -180,14 +218,36 @@ static void devm_irq_domain_fwnode_release(void *data)
>>  static int pm8008_probe(struct i2c_client *client)
>>  {
>>  	struct regmap_irq_chip_data *irq_data;
>> +	const struct regmap_irq_chip *irq_chip_desc;
>>  	struct device *dev = &client->dev;
>> +	const struct mfd_cell *mfd_cells;
>>  	struct regmap *regmap, *regmap2;
>>  	struct fwnode_handle *fwnode;
>>  	struct i2c_client *dummy;
>>  	struct gpio_desc *reset;
>> +	unsigned long pmic_type;
>> +	int num_mfd_cells;
>>  	char *name;
>>  	int ret;
>>  
>> +	pmic_type = (unsigned long)of_device_get_match_data(dev);
> 
> [Severity: Medium]
> Will this silently fall back to TYPE_PM8008 for non-DT devices?
> 
> of_device_get_match_data() explicitly checks for dev->of_node and returns
> NULL when it isn't present (such as devices instantiated via ACPI PRP0001
> or swnodes). A NULL return evaluates to 0, which matches TYPE_PM8008 in
> the switch below.
> 
> This means a valid qcom,pm8010-i2c device instantiated via ACPI or swnodes
> will be misidentified as TYPE_PM8008 instead of TYPE_PM8010.
> 
> Could this use i2c_get_match_data() or device_get_match_data() to ensure
> device variants are correctly extracted regardless of the underlying
> firmware representation?

This driver is intended to support DT-only instantiation right now, so
we don't expect to have instantiations via ACPI or swnodes.

But anyway, to avoid the ambiguity due to NULL return from
of_device_get_match_data evaluating to 0, I can add an explicit
check for its return value and fail probe with an error if it
ever returns NULL.

Thanks,
Jishnu


> 
>> +
>> +	switch (pmic_type) {
>> +	case TYPE_PM8008:
>> +		irq_chip_desc = &pm8008_irq_chip;
>> +		mfd_cells = pm8008_cells;
>> +		num_mfd_cells = ARRAY_SIZE(pm8008_cells);
>> +		break;
>