Date: Wed, 12 Jun 2024 02:23:00 +0000
From: "Yuxi (Yuxi) Wang" <Yuxi.Wang@...olithicpower.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        "pavel@....cz"
	<pavel@....cz>, "lee@...nel.org" <lee@...nel.org>,
        "robh+dt@...nel.org"
	<robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org"
	<krzysztof.kozlowski+dt@...aro.org>,
        "conor+dt@...nel.org"
	<conor+dt@...nel.org>,
        "wyx137120466@...il.com" <wyx137120466@...il.com>
CC: "linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V2 2/2] leds: add mp3326 driver

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Sent: Tuesday, June 11, 2024 5:22 PM
> To: Yuxi (Yuxi) Wang <Yuxi.Wang@...olithicpower.com>; pavel@....cz; lee@...nel.org; robh+dt@...nel.org;
> krzysztof.kozlowski+dt@...aro.org; conor+dt@...nel.org; wyx137120466@...il.com
> Cc: linux-leds@...r.kernel.org; devicetree@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH V2 2/2] leds: add mp3326 driver
> 
> On 11/06/2024 10:32, Yuxi Wang wrote:
> > This commit adds support for MPS MP3326 LED driver.
> 
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-
> patches.rst*L95__;Iw!!FIHMVlGrYVGa5kwGHCY!VZLXCA6uD-SR9PbzzNhDFXYNwX0LIXNJnI81FkF3VFD0K3R8zpk_o61sSGCXDa-
> mecEHFCdimzprLNeYlHJSLzaY4P_CT3YmPTXg$
> 
Sorry, it's my fault. 
I will fix it in the next version.

> >
> > Signed-off-by: Yuxi Wang <Yuxi.Wang@...olithicpower.com>
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index d721b254e1e4..3ca7be35c834 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> 
> 
> 
> > +/*
> > + * PWM in the range of [0 255]
> > + */
> > +static int led_pwm_store(struct device *dev, struct device_attribute *attr,
> > +			     const char *buf, size_t count)
> 
> Nope.
Hi Krzysztof,

What do you mean this Nope?
Is it  format or function?

> 
> ...
> 
> > +	}
> > +	r_val = r_val * 255 / 4095 + (r_val * 255 % 4095) / (4095 / 2);
> > +	g_val = g_val * 255 / 4095 + (g_val * 255 % 4095) / (4095 / 2);
> > +	b_val = b_val * 255 / 4095 + (b_val * 255 % 4095) / (4095 / 2);
> > +	if (led->num_colors == 1)
> > +		return sysfs_emit(buf, "0x%x\n", r_val);
> > +	else
> > +		return sysfs_emit(buf, "0x%x 0x%x 0x%x\n", r_val, g_val, b_val);
> > +}
> > +
> > +static int led_enable_store(struct device *dev,
> > +				struct device_attribute *attr, const char *buf,
> > +				size_t count)
> 
> Eeeee? store to enable LED? Really?
Yes. The users need this function and we provide it.


> 
> ...
> 
> > +{
> > +	struct led_classdev *lcdev = dev_get_drvdata(dev);
> > +	struct mp3326_led *led = container_of(lcdev, struct mp3326_led, cdev);
> > +	struct mp3326 *chip = led->private_data;
> > +	int ret;
> > +	uint val, i;
> 
> 
> > +
> > +static DEVICE_ATTR_RW(led_pwm);
> > +static DEVICE_ATTR_RW(led_enable);
> > +static DEVICE_ATTR_RO(led_short_fault);
> > +static DEVICE_ATTR_RO(led_open_fault);
> 
> No, for multiple reasons:
> 1. Where ABI documentation?
> 2. There is a standard sysfs interface. No need for most of that. Please
> explain why standard interface does not fit your needs - for each new
> interface.
Hi krzysztof,

1. Where ABI documentation?
A: 
Sorry, the abi is insufficient.

Can I add it as comment above the function?

2. There is a standard sysfs interface. No need for most of that. Please
explain why standard interface does not fit your needs - for each new
 interface.
A:
Leds has two ways to light dim. One is analog dimming, another pwm dimming.
They are different in practice. 

In RGB module, pwm dimming can control color and analog dimming can control intensity.

Mp3326 supports the two ways which can operate contemporary.

In practice, I have needs below.
1. Operate rgb color and intensity.
2. enable/disable some channel
3. short/open fault notice.

However, The standard interface only has three functions below, they are not fit my needs.
1. multi_index
2. multi_intensity(only can dim using one way, so I use it as analog dimming)
3. led_mc_calc_color


In order to fit my needs, I add the interface below.
led_pwm       
led_enable
led_short_fault
led_open_fault


>
> > +
> > +static struct attribute *led_sysfs_attrs[] = {
> > +	&dev_attr_led_pwm.attr,
> > +	&dev_attr_led_enable.attr,
> > +	&dev_attr_led_short_fault.attr,
> > +	&dev_attr_led_open_fault.attr,
> > +	NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(led_sysfs);
> > +
> > +static int mp3326_add_led(struct mp3326 *chip, struct device_node *np,
> > +			  int index)
> > +{
> > +	struct mp3326_led *led = &chip->leds[index];
> > +	struct mc_subled *info;
> > +	struct device_node *child;
> > +	struct led_classdev *cdev;
> > +	struct led_init_data init_data = {};
> > +	int ret;
> > +	int i = 0;
> > +	int count;
> > +	u32 color = 0;
> > +	u32 reg = 0;
> > +
> > +	ret = of_property_read_u32(np, "color", &color);
> > +	if (ret) {
> > +		dev_err(&chip->client->dev, "Miss color in the node\n");
> > +		return ret;
> > +	}
> 
> Blank line

Sorry, it's my fault. 
I will fix it in the next version.

> 
> > +	led->private_data = chip;
> > +	if (color == LED_COLOR_ID_RGB) {
> > +		count = of_get_child_count(np);
> > +		if (count != 3) {
> > +			dev_err(&chip->client->dev,
> > +				"RGB must have three node.\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		info = devm_kcalloc(&chip->client->dev, 3, sizeof(*info),
> > +				    GFP_KERNEL);
> > +		if (!info)
> > +			return -ENOMEM;
> > +
> > +		for_each_available_child_of_node(np, child) {
> > +			ret = of_property_read_u32(child, "reg", &reg);
> > +			if (ret || reg > MAX_CHANNEL) {
> > +				dev_err(&chip->client->dev,
> > +					"reg must less or equal than %d\n",
> > +					MAX_CHANNEL);
> > +				return -EINVAL;
> > +			}
> > +
> > +			ret = of_property_read_u32(child, "color", &color);
> > +			if (ret) {
> > +				dev_err(&chip->client->dev,
> > +					"color must have value\n");
> > +				return ret;
> > +			}
> > +
> > +			if (color > 3 || !color) {
> > +				dev_err(&chip->client->dev,
> > +					"color must be Red, Green and Blue. The color is %d\n",
> > +					color);
> > +				return ret;
> > +			}
> > +			info[i].color_index = color;
> > +			info[i].channel = reg;
> > +			info[i].brightness = 0;
> > +			i++;
> > +		}
> > +
> > +		led->subled_info = info;
> > +		led->num_colors = 3;
> > +		cdev = &led->cdev;
> > +		cdev->max_brightness = MAX_BRIGHTNESS;
> > +		cdev->brightness_set_blocking = led_brightness_set;
> > +		cdev->groups = led_sysfs_groups;
> > +		init_data.fwnode = &np->fwnode;
> > +
> > +		ret = devm_led_classdev_register_ext(&chip->client->dev,
> > +						     &led->cdev, &init_data);
> > +
> > +		if (ret) {
> > +			dev_err(&chip->client->dev,
> > +				"Unable register multicolor:%s\n", cdev->name);
> > +			return ret;
> > +		}
> > +	} else {
> > +		ret = of_property_read_u32(np, "reg", &reg);
> > +		if (ret || reg > MAX_CHANNEL) {
> > +			dev_err(&chip->client->dev,
> > +				"reg must less or equal than %d\n",
> > +				MAX_CHANNEL);
> > +			return -EINVAL;
> > +		}
> > +		info = devm_kcalloc(&chip->client->dev, 1, sizeof(*info),
> > +				    GFP_KERNEL);
> > +		led->num_colors = 1;
> > +		info[i].color_index = LED_COLOR_ID_WHITE;
> > +		info[i].channel = reg;
> > +		info[i].brightness = 0;
> > +		led->subled_info = info;
> > +		cdev = &led->cdev;
> > +		cdev->max_brightness = MAX_BRIGHTNESS;
> > +		cdev->brightness_set_blocking = led_brightness_set;
> > +		cdev->groups = led_sysfs_groups;
> > +		init_data.fwnode = &np->fwnode;
> > +		ret = devm_led_classdev_register_ext(&chip->client->dev,
> > +						     &led->cdev, &init_data);
> > +		if (ret) {
> > +			dev_err(&chip->client->dev, "Unable register led:%s\n",
> > +				cdev->name);
> > +			return ret;
> > +		}
> > +	}
> 
> Blank line
> 

Sorry, it's my fault. 
I will fix it in the next version.

> > +	return ret;
> > +}
> > +
> > +static int mp3326_parse_dt(struct mp3326 *chip)
> > +{
> > +	struct device_node *np = dev_of_node(&chip->client->dev);
> > +	struct device_node *child;
> > +	int ret;
> > +	int i = 0;
> > +
> > +	for_each_available_child_of_node(np, child) {
> > +		ret = mp3326_add_led(chip, child, i);
> > +		if (ret)
> > +			return ret;
> > +		i++;
> > +	}
> > +
> > +	ret = regmap_write(chip->regmap, MP3326_PWM_CTRL_CHANNEL_9_16, 0);
> > +	if (ret)
> > +		return ret;
> 
> Blank line
Sorry, it's my fault. 
I will fix it in the next version.


> 
> > +	ret = regmap_write(chip->regmap, MP3326_PWM_CTRL_CHANNEL_1_8, 0);
> > +	if (ret)
> > +		return ret;
> 
> Blank line

Sorry, it's my fault. 
I will fix it in the next version.
> 
> > +	return 0;
> > +}
> > +
> > +static int mp3326_leds_probe(struct i2c_client *client)
> > +{
> > +	struct mp3326 *chip;
> > +	const struct reg_field *reg_fields;
> > +	int count, i, j;
> > +
> > +	count = device_get_child_node_count(&client->dev);
> > +	if (!count) {
> 
> Drop {
> 
> Please run scripts/checkpatch.pl and fix reported warnings. Then please
> run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
> Some warnings can be ignored, especially from --strict run, but the code
> here looks like it needs a fix. Feel free to get in touch if the warning
> is not clear.
> 
Krzysztof,

Thanks your suggestion.

I run scripts/checkpatch.pl, but no warn and error occur.
I will add `--strict` and check it carefully.


> > +		return dev_err_probe(&client->dev, -EINVAL,
> > +				     "Incorrect number of leds (%d)", count);
> > +	}
> 
> blank line
> 

Sorry, it's my fault. 
I will fix it in the next version.
> > +	chip = devm_kzalloc(&client->dev, struct_size(chip, leds, count),
> > +			    GFP_KERNEL);
> > +	if (!chip)
> > +		return -ENOMEM;
> > +
> > +	chip->client = client;
> > +	chip->num_of_leds = count;
> > +	i2c_set_clientdata(client, chip);
> > +	chip->regmap = devm_regmap_init_i2c(client, &MP3326_regmap_config);
> > +	if (IS_ERR(chip->regmap))
> > +		return PTR_ERR(chip->regmap);
> > +
> > +	for (i = 0; i < MAX_CHANNEL; i++) {
> > +		reg_fields = channels_reg_fields[i];
> > +		for (j = 0; j < MAX_CTRL; j++) {
> > +			chip->regmap_fields[i][j] = devm_regmap_field_alloc(
> > +				&client->dev, chip->regmap, reg_fields[j]);
> > +			if (IS_ERR(chip->regmap_fields[i][j]))
> > +				return PTR_ERR(chip->regmap_fields[i][j]);
> > +		}
> > +	}
> 
> Blank line
> 
Sorry, it's my fault. 
I will fix it in the next version.

> > +	if (mp3326_parse_dt(chip))
> > +		return 1;
> 
> What is one? This looks like some sort of user-space or downstream
> approach. That's not how it works for upstream kernel. Do not introduce
> your downstream/user-space/other-system coding style and programming
> interface.
> 
> You must use Linux approach.
> 
> There is no way probe function returns a "1". See other files as example.
> 
> 
Sorry, it's my fault. 
I will fix it in the next version.
> > +	else
> > +		return 0;
> > +}
> > +
> > +static const struct i2c_device_id mp3326_id[] = { { "mp3326", 0 }, {} };
> 
> This must be formatted as kernel coding style. See other files as an
> example.

Ok, I use clang-format to format my code-style, maybe some incompatible.
I will correct it.
Thanks,



> 
> > +MODULE_DEVICE_TABLE(i2c, mp3326_id);
> > +
> > +static const struct of_device_id mp3326_of_match[] = { { .compatible =
> > +								 "mps,mp3326" },
> > +						       {} };
> > +MODULE_DEVICE_TABLE(of, mp3326_of_match);
> > +
> > +static struct i2c_driver mp3326_driver = {
> > +	.probe = mp3326_leds_probe,
> > +	.driver = {
> > +			.name = "mp3326_led",
> > +			.of_match_table = mp3326_of_match,
> > +		   },
> > +	.id_table = mp3326_id,
> > +};
> > +
> > +module_i2c_driver(mp3326_driver);
> > +MODULE_AUTHOR("Yuxi Wang <Yuxi.Wang@...olithicpower.com>");
> > +MODULE_DESCRIPTION("MPS MP3326 LED driver");
> > +MODULE_LICENSE("GPL");
> 
> Best regards,
> Krzysztof

Powered by blists - more mailing lists