linux-kernel - Re: [PATCH 3/3] regulator: Add FP9931/JD9930 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251108-nebulous-cheetah-of-prowess-964be6@kuoka>
Date: Sat, 8 Nov 2025 13:21:57 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Andreas Kemnade <andreas@...nade.info>
Cc: Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, 
	Guenter Roeck <linux@...ck-us.net>, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-hwmon@...r.kernel.org
Subject: Re: [PATCH 3/3] regulator: Add FP9931/JD9930 driver

On Fri, Nov 07, 2025 at 09:06:46PM +0100, Andreas Kemnade wrote:
> +
> +static int fp9931_probe(struct i2c_client *client)
> +{
> +	struct fp9931_data *data;
> +	struct regulator_config config = { };
> +	struct regulator_dev *rdev;
> +	int ret = 0;
> +	int i;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(struct fp9931_data), GFP_KERNEL);

sizeof(*)

> +	data->regmap = devm_regmap_init_i2c(client, &regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
> +				     "failed to allocate regmap!\n");
> +
> +	data->vin_reg = devm_regulator_get_optional(&client->dev, "vin");
> +	if (IS_ERR(data->vin_reg))
> +		return dev_err_probe(&client->dev, PTR_ERR(data->vin_reg),
> +				     "failid to get vin regulator\n");
> +
> +	data->pgood_gpio = devm_gpiod_get(&client->dev, "pg",
> +					  GPIOD_IN);

Please wrap at 80, not 60. See Linux coding style.

> +	if (IS_ERR(data->pgood_gpio))
> +		return dev_err_probe(&client->dev,
> +				     PTR_ERR(data->pgood_gpio),
> +				     "failed to get power good gpio\n");
> +
> +	data->pgood_irq = gpiod_to_irq(data->pgood_gpio);
> +	if (data->pgood_irq < 0)
> +		return data->pgood_irq;
> +
> +	data->en_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(data->en_gpio))
> +		return dev_err_probe(&client->dev,
> +				     PTR_ERR(data->en_gpio),

No need to break line.

> +				     "failed to get en gpio\n");
> +
> +	data->ts_en_gpio = devm_gpiod_get_optional(&client->dev, "ts-en", GPIOD_OUT_LOW);
> +	if (IS_ERR(data->ts_en_gpio))
> +		return dev_err_probe(&client->dev,
> +				     PTR_ERR(data->ts_en_gpio),
> +				     "failed to get en gpio\n");
> +
> +	data->dev = &client->dev;
> +	i2c_set_clientdata(client, data);
> +
> +	init_completion(&data->pgood_completion);
> +
> +	ret = devm_request_threaded_irq(&client->dev, data->pgood_irq, NULL,
> +					pgood_handler,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +					"PGOOD", data);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to request irq\n");
> +
> +	if (IS_ENABLED(CONFIG_PM)) {
> +		devm_pm_runtime_enable(&client->dev);
> +		pm_runtime_set_autosuspend_delay(&client->dev, 4000);
> +		pm_runtime_use_autosuspend(&client->dev);
> +	} else {
> +		ret = fp9931_runtime_resume(&client->dev);
> +		if (ret < 0)
> +			return ret;
> +
> +		devm_add_action_or_reset(&client->dev, disable_nopm, data);
> +	}
> +
> +	ret = setup_timings(data);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "failed to setup timings\n");
> +
> +	config.driver_data = data;
> +	config.dev = &client->dev;
> +	config.regmap = data->regmap;
> +
> +	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> +		rdev = devm_regulator_register(&client->dev,
> +					       &regulators[i],
> +					       &config);

Please wrap according to Linux coding style.

> +		if (IS_ERR(rdev))
> +			return dev_err_probe(&client->dev, PTR_ERR(rdev),
> +					     "failed to register %s regulator\n",
> +					     regulators[i].name);
> +	}
> +
> +	if (IS_REACHABLE(CONFIG_HWMON)) {
> +		struct device *hwmon_dev;
> +
> +		hwmon_dev = devm_hwmon_device_register_with_info(&client->dev, "fp9931", data,
> +								 &fp9931_chip_info, NULL);

So you use hwmon, then why binding said this is a thermal zone sensor?

> +		if (IS_ERR(hwmon_dev))
> +			dev_err(&client->dev, "failed to register hwmon\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops fp9931_pm_ops = {
> +	SET_RUNTIME_PM_OPS(fp9931_runtime_suspend, fp9931_runtime_resume, NULL)
> +};
> +
> +static const struct of_device_id fp9931_dt_ids[] = {
> +	{
> +		.compatible = "fiti,fp9931",
> +	}, {
> +		.compatible = "fiti,fp9931", /* no night mode */

Drop, it's the same compatible.

> +	}, {
> +		/* sentinel */
> +	}
> +};
> +MODULE_DEVICE_TABLE(of, fp9931_dt_ids);
> +
> +static struct i2c_driver fp9931_i2c_driver = {
> +	.driver = {
> +		   .name = "fp9931",
> +		   .owner = THIS_MODULE,

Please do not send us 12 year old code... Drop and runstandard tools
(smatch, sparse, cocci) at minimum.

> +		   .of_match_table = fp9931_dt_ids,
> +		   .pm = (&fp9931_pm_ops),

No need for ()

Best regards,
Krzysztof