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

["Kaustabh Chakraborty" <kauschluss@disroot.org>]

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@disroot.org>
>> ---
>>  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.

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