Re: [PATCH v2 07/12] mfd: sec: store hardware revision in sec_pmic_dev and add S2MU005 support

[Kaustabh Chakraborty]

On 2026-02-04 20:35 +05:30, Kaustabh Chakraborty wrote:
> On 2026-02-04 14:17 +00:00, André Draszik wrote:
>> Hi Kaustabh,
>>
>> On Mon, 2026-01-26 at 00:37 +0530, Kaustabh Chakraborty wrote:
>>> The device revision matters in cases when in some PMICs, the correct
>>> register offsets very in different revisions. Instead of just debug
>>
>> s/very/vary
>>
>>> printing the value, store it in the driver data struct.
>>
>> Please mention that you're not doing that for s2mpg1x, though.
>>
>>> 
>>> Unlike other devices, S2MU005 has its hardware revision ID in register
>>> offset 0x73. Allow handling different devices and add support for S2MU005.
>>> 
>>> Signed-off-by: Kaustabh Chakraborty <kauschluss@xxxxxxxxxxx>
>>> ---
>>>  drivers/mfd/sec-common.c         | 41 ++++++++++++++++++++++++++++++----------
>>>  include/linux/mfd/samsung/core.h |  1 +
>>>  2 files changed, 32 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/drivers/mfd/sec-common.c b/drivers/mfd/sec-common.c
>>> index bc2a1f2c6dc7a..069a1ba9aa1f1 100644
>>> --- a/drivers/mfd/sec-common.c
>>> +++ b/drivers/mfd/sec-common.c
>>> @@ -16,6 +16,7 @@
>>>  #include <linux/mfd/samsung/irq.h>
>>>  #include <linux/mfd/samsung/s2mps11.h>
>>>  #include <linux/mfd/samsung/s2mps13.h>
>>> +#include <linux/mfd/samsung/s2mu005.h>
>>>  #include <linux/module.h>
>>>  #include <linux/of.h>
>>>  #include <linux/pm.h>
>>> @@ -111,17 +112,38 @@ static const struct mfd_cell s2mu005_devs[] = {
>>>  	MFD_CELL_OF("s2mu005-rgb", NULL, NULL, 0, 0, "samsung,s2mu005-rgb"),
>>>  };
>>>  
>>> -static void sec_pmic_dump_rev(struct sec_pmic_dev *sec_pmic)
>>> +static int sec_pmic_store_rev(struct sec_pmic_dev *sec_pmic)
>>>  {
>>> -	unsigned int val;
>>> +	unsigned int reg, mask, shift;
>>> +	int ret;
>>>  
>>> -	/* For s2mpg1x, the revision is in a different regmap */
>>> -	if (sec_pmic->device_type == S2MPG10)
>>> -		return;
>>> +	switch (sec_pmic->device_type) {
>>> +	case S2MPG10:
>>> +		/* For s2mpg1x, the revision is in a different regmap */
>>> +		return 0;
>>> +	case S2MU005:
>>> +		reg = S2MU005_REG_ID;
>>> +		mask = S2MU005_ID_MASK;
>>> +		shift = S2MU005_ID_SHIFT;
>>> +		break;
>>> +	default:
>>> +		/* For other device types, the REG_ID is always the first register. */
>>> +		reg = S2MPS11_REG_ID;
>>> +		mask = ~0;
>>> +		shift = 0;
>>> +	}
>>> +
>>> +	ret = regmap_read(sec_pmic->regmap_pmic, reg, &sec_pmic->revision);
>>> +	if (ret) {
>>> +		dev_err(sec_pmic->dev, "Failed to read PMIC revision (%d)\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	sec_pmic->revision &= mask;
>>> +	sec_pmic->revision >>= shift;
>>>  
>>> -	/* For each device type, the REG_ID is always the first register */
>>> -	if (!regmap_read(sec_pmic->regmap_pmic, S2MPS11_REG_ID, &val))
>>> -		dev_dbg(sec_pmic->dev, "Revision: 0x%x\n", val);
>>> +	dev_dbg(sec_pmic->dev, "Revision: 0x%x\n", sec_pmic->revision);
>>> +	return 0;
>>>  }
>>>  
>>>  static void sec_pmic_configure(struct sec_pmic_dev *sec_pmic)
>>> @@ -262,9 +284,8 @@ int sec_pmic_probe(struct device *dev, int device_type, unsigned int irq,
>>>  		return ret;
>>>  
>>>  	sec_pmic_configure(sec_pmic);
>>> -	sec_pmic_dump_rev(sec_pmic);
>>>  
>>> -	return ret;
>>> +	return sec_pmic_store_rev(sec_pmic);
>>>  }
>>>  EXPORT_SYMBOL_GPL(sec_pmic_probe);
>>>  
>>> diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h
>>> index 43e0c5e55f5d3..56aa33d7e3d60 100644
>>> --- a/include/linux/mfd/samsung/core.h
>>> +++ b/include/linux/mfd/samsung/core.h
>>> @@ -70,6 +70,7 @@ struct sec_pmic_dev {
>>>  
>>>  	int device_type;
>>>  	int irq;
>>> +	unsigned int revision;
>>
>> kerneldoc needs to be updated.
>
> Seems like it needs cleanup anyway, I will send a patch for that
> separately (if this patch gets dropped in the next rev, see below).
>
>>
>> Given the LED driver is the only driver & device so far which needs the
>> PMIC revision, maybe for now that driver could determine the revision
>> itself instead of adding this new member for everybody?
>
> Hmm, implementing that would make this patch redundant. I'll do so.

It however seems weird to not handle it here; we're already reading the
revision value; it also makes sense to store it in the MFD driver
itself.

Either way, the patch won't be redundant, because the reading part at
least needs to be implemented here.

>
>>
>> Cheers,
>> Andre'
>>
>>>  };
>>>  
>>>  struct sec_platform_data {