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?
