Re: [PATCH v3 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator

[Fenglin Wu]

On 7/25/2023 1:52 PM, Krzysztof Kozlowski wrote:
> On 25/07/2023 07:41, Fenglin Wu wrote:
> > Currently, all vibrator control register addresses are hard coded,
> > including the base address and the offset, it's not flexible to support
> > new SPMI vibrator module which is usually included in different PMICs
> > with different base address. Refactor this by introducing the HW type
> > terminology and contain the register offsets/masks/shifts in reg_filed
> > data structures corresponding to each vibrator type, and the base address
> > value defined in 'reg' property will be added for SPMI vibrators.
> >
> > Signed-off-by: Fenglin Wu <quic_fenglinw@xxxxxxxxxxx>
> > ---
> >   drivers/input/misc/pm8xxx-vibrator.c | 130 ++++++++++++++++-----------
> >   1 file changed, 77 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> > index 04cb87efd799..77bb0018d4e1 100644
> > --- a/drivers/input/misc/pm8xxx-vibrator.c
> > +++ b/drivers/input/misc/pm8xxx-vibrator.c
> > @@ -12,36 +12,36 @@
> >   #include <linux/regmap.h>
> >   #include <linux/slab.h>
> >   
> > +#define SSBI_VIB_DRV_EN_MANUAL_MASK	0xfc
> > +#define SSBI_VIB_DRV_LEVEL_MASK		0xf8
> > +#define SSBI_VIB_DRV_SHIFT		3
> > +#define SPMI_VIB_DRV_LEVEL_MASK		0xff
> > +#define SPMI_VIB_DRV_SHIFT		0
> > +
> >   #define VIB_MAX_LEVEL_mV	(3100)
> >   #define VIB_MIN_LEVEL_mV	(1200)
> >   #define VIB_MAX_LEVELS		(VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
> >   
> >   #define MAX_FF_SPEED		0xff
> >   
> > -struct pm8xxx_regs {
> > -	unsigned int enable_addr;
> > -	unsigned int enable_mask;
> > +enum pm8xxx_vib_type {
> > +	SSBI_VIB,
> > +	SPMI_VIB_GEN1,
> > +};
> >   
> > -	unsigned int drv_addr;
> > -	unsigned int drv_mask;
> > -	unsigned int drv_shift;
> > -	unsigned int drv_en_manual_mask;
> > +enum {
> > +	VIB_DRV_REG,
> > +	VIB_EN_REG,
> > +	VIB_MAX_REG,
> >   };
> >   
> > -static const struct pm8xxx_regs pm8058_regs = {
> > -	.drv_addr = 0x4A,
> > -	.drv_mask = 0xf8,
> > -	.drv_shift = 3,
> > -	.drv_en_manual_mask = 0xfc,
> > +static struct reg_field ssbi_vib_regs[VIB_MAX_REG] = {
>
> Change from const to non-const is wrong. How do you support multiple
> devices? No, this is way too fragile now.

The register definition is no longer used as the match data, hw_type is 
used.

The last suggestion was getting the register base address from the DT 
and it has to be added into the offset of SPMI vibrator registers 
(either in the previous hard-coded format or the later the reg_filed 
data structure), so it's not appropriated to make it constant.

I don't understand this question: "How do you support multiple devices?" 
For SSBI vibrator, since all the registers are fixed, and I would assume 
that there is no chance to support multiple vibrator devices on the same 
SSBI bus. If they are not on the same bus, the regmap device will be 
different while the registers definition is the same, and we are still 
able to support multiple devices, right?
The similar story for SPMI vibrators and it can support multiple devices 
if they are located on different SPMI bus, or even if they are on the 
same SPMI bus but just having different SID or PID.

> ...
>
> >   
> >   	/*
> >   	 * pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
> > @@ -168,38 +166,65 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
> >   {
> >   	struct pm8xxx_vib *vib;
> >   	struct input_dev *input_dev;
> > -	int error;
> > +	struct device *dev = &pdev->dev;
> > +	struct regmap *regmap;
> > +	struct reg_field *regs;
> > +	int error, i;
> >   	unsigned int val;
> > -	const struct pm8xxx_regs *regs;
> > +	u32 reg_base;
> >   
> > -	vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
> > +	vib = devm_kzalloc(dev, sizeof(*vib), GFP_KERNEL);
>
> Not really related. Split cleanup from new features.
Okay, will keep as it is.
> >   	if (!vib)
> >   		return -ENOMEM;
> >   
> > -	vib->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > -	if (!vib->regmap)
> > +	regmap = dev_get_regmap(dev->parent, NULL);
> > +	if (!regmap)
> >   		return -ENODEV;
> >   
> > -	input_dev = devm_input_allocate_device(&pdev->dev);
> > +	input_dev = devm_input_allocate_device(dev);
> >   	if (!input_dev)
> >   		return -ENOMEM;
> >   
> >   	INIT_WORK(&vib->work, pm8xxx_work_handler);
> >   	vib->vib_input_dev = input_dev;
> >   
> > -	regs = of_device_get_match_data(&pdev->dev);
> > +	vib->hw_type = (enum pm8xxx_vib_type)of_device_get_match_data(dev);
> >   
> > -	/* operate in manual mode */
> > -	error = regmap_read(vib->regmap, regs->drv_addr, &val);
> > +	regs = ssbi_vib_regs;
> > +	if (vib->hw_type != SSBI_VIB) {
> > +		error = fwnode_property_read_u32(dev->fwnode, "reg", &reg_base);
> > +		if (error < 0) {
> > +			dev_err(dev, "Failed to read reg address, rc=%d\n", error);
> > +			return error;
> > +		}
> > +
> > +		if (vib->hw_type == SPMI_VIB_GEN1)
> > +			regs = spmi_vib_gen1_regs;
> > +
> > +		for (i = 0; i < VIB_MAX_REG; i++)
> > +			if (regs[i].reg != 0)
> > +				regs[i].reg += reg_base;
> > +	}
> > +
> > +	error = devm_regmap_field_bulk_alloc(dev, regmap, vib->r_fields, regs, VIB_MAX_REG);
> >   	if (error < 0)
> > +	{
>
> That's not a Linux coding style.
>
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
>
> > +		dev_err(dev, "Failed to allocate regmap failed, rc=%d\n", error);
>
> No need to print errors on allocation failures.
Will fix it.
Thanks

> >   		return error;
> > +	}
> >   
> > -	val &= regs->drv_en_manual_mask;
> > -	error = regmap_write(vib->regmap, regs->drv_addr, val);
> > +	error = regmap_field_read(vib->r_fields[VIB_DRV_REG], &val);
> >   	if (error < 0)
> >   		return error;
>
>
> Best regards,
> Krzysztof