Re: [PATCH 1/3] platform/chrome: cros_ec_charge_state: add new driver to control charge

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Mon, Nov 18, 2024 at 05:33:46PM +0800, Sung-Chi, Li wrote:
> diff --git a/drivers/platform/chrome/cros_ec_charge_state.c b/drivers/platform/chrome/cros_ec_charge_state.c
[...]
> +#define DRV_NAME "cros-ec-charge-state"
> +#define CHARGE_TYPE_CHARGE "charge"
> +#define CHARGE_TYPE_INPUT "input"

I'm not a big fan of these kind of macros and would prefer to remove them.

> +static int
> +cros_ec_charge_state_get_current_limit(struct cros_ec_device *ec_dev,
> +				       enum charge_state_params charge_type,
> +				       uint32_t *limit)
> +{
[...]
> +	*limit = cpu_to_le32(state.get_param.value);
> +	return 0;
> +}
> +
> +static int
> +cros_ec_charge_state_set_current_limit(struct cros_ec_device *ec_dev,
> +				       enum charge_state_params charge_type,
> +				       uint32_t limit)
> +{
[...]
> +	param.set_param.value = cpu_to_le32(limit);

Looks weird to me.  Both getter and setter use cpu_to_le32()?  Should one
of them be le32_to_cpu()?

> +static int
> +cros_ec_charge_state_get_cur_state(struct thermal_cooling_device *cdev,
> +				   unsigned long *state)
> +{
> +	struct cros_ec_charge_state_data *data = cdev->devdata;
> +	uint32_t limit;
> +	int ret;
> +
> +	ret = cros_ec_charge_state_get_current_limit(data->ec_dev,
> +						     data->charge_type, &limit);
> +	if (ret) {
> +		dev_err(data->dev, "failed to get current state: %d", ret);

If something went wrong, and cros_ec_charge_state_get_current_limit() keeps
returning errors, would it somehow flood the kernel logs?

> +		return ret;
> +	}
> +
> +	*state = data->max_milliamp - limit;

Would it happen: data->max_milliamp - limit < data->min_milliamp == true?

> +static int
> +cros_ec_charge_state_register_charge_chip(struct device *dev,
> +					  struct device_node *node,
> +					  struct cros_ec_device *cros_ec)
> +{
[...]
> +
> +	if (!strcmp(type_val, CHARGE_TYPE_CHARGE)) {
> +		data->charge_type = CS_PARAM_CHG_CURRENT;
> +	} else if (!strcmp(type_val, CHARGE_TYPE_INPUT)) {
> +		data->charge_type = CS_PARAM_CHG_INPUT_CURRENT;
> +	} else {
> +		dev_err(dev, "unknown charge type: %s", type_val);
> +		return -1;

How about -EINVAL?

> +	}
> +
> +	ret = of_property_read_u32(node, "min-milliamp", &data->min_milliamp);
> +	if (ret) {
> +		dev_err(dev, "failed to get min-milliamp data: %d", ret);
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "max-milliamp", &data->max_milliamp);
> +	if (ret) {
> +		dev_err(dev, "failed to get max-milliamp data: %d", ret);
> +		return ret;
> +	}

Would it happen: min-milliamp > max-milliamp == true?

> +
> +	data->ec_dev = cros_ec;
> +	data->dev = dev;
> +
> +	cdev = devm_thermal_of_cooling_device_register(
> +		dev, node, node->name, data,
> +		&cros_ec_charge_state_cooling_device_ops);
> +	if (IS_ERR_VALUE(cdev)) {

Any reasons to not use IS_ERR()?

> +static int cros_ec_charge_state_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> +	struct device_node *child;
> +
> +	for_each_available_child_of_node(cros_ec->dev->of_node, child) {
> +		if (!of_device_is_compatible(child,
> +					     "google,cros-ec-charge-state"))
> +			continue;
> +		if (cros_ec_charge_state_register_charge_chip(dev, child,
> +							      cros_ec))
> +			continue;
> +	}

There should be a way to use the compatible string in struct mfd_cell for
matching the node.  See also [1].

[1]: https://elixir.bootlin.com/linux/v6.12/source/drivers/mfd/mfd-core.c#L184

> +
> +static const struct platform_device_id cros_ec_charge_state_id[] = {
> +	{ DRV_NAME,  0 },
                    ^

> +static struct platform_driver cros_ec_chargedev_driver = {

The whole file uses "cros_ec_charge_state_" as a prefix for all symbols.  Any
reasons to not make this consistent?