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

[Krzysztof Kozlowski]

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.

...

>  
>  	/*
>  	 * 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.

>  	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.

>  		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