| Message ID | nH0fAuRxkhh0jdtlck5LucnuXiEY2wfpLF8C8spK8hebUUZ75xQOe-PjBtR7F8jEZ84l-o9rVJ3z9xvatOAJMjvbH5qCQIO5MuSOmpWr0ZQ=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] leds: lm3697: Rename struct into more appropriate name | expand |
On Fri 2020-10-09 15:51:35, Gabriel David wrote: > The mentioned struct, lm3697_led, was renamed to lm3697_bank since the > structure is representing the control banks. This name, in my opinion, > is more semantically correct. The pointers referring to it were also > renamed. > Signed-off-by: Gabriel David <ultracoolguy4@protonmail.com> > --- > Yes, this is the same Gabriel David from ultracoolguy@tutanota.org and > ultracoolguy@disroot.org. If you want me to confirm it I'll gladly do > it. No problem with that, and no need to resend. This can proably wait for 5.11... I'd like some comment from Dan... and perhaps I'd want to understand what the difference between LED and bank is. ...there can be more than one LED connected to the given bank, that's what you are pointing out? ...but these LEDs will always work in unison, and they are handled as single LED by Linux, right? Best regards, Pavel > drivers/leds/leds-lm3697.c | 90 +++++++++++++++++++------------------- > 1 file changed, 45 insertions(+), 45 deletions(-) > > diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c > index 31f5ed486839..c62f95fc17e8 100644 > --- a/drivers/leds/leds-lm3697.c > +++ b/drivers/leds/leds-lm3697.c > @@ -39,7 +39,7 @@ > #define LM3697_MAX_CONTROL_BANKS 2 > > /** > - * struct lm3697_led - > + * struct lm3697_bank - > * @hvled_strings: Array of LED strings associated with a control bank > * @label: LED label > * @led_dev: LED class device > @@ -48,7 +48,7 @@ > * @control_bank: Control bank the LED is associated to. 0 is control bank A > * 1 is control bank B > */ > -struct lm3697_led { > +struct lm3697_bank { > u32 hvled_strings[LM3697_MAX_LED_STRINGS]; > char label[LED_MAX_NAME_SIZE]; > struct led_classdev led_dev; > @@ -80,7 +80,7 @@ struct lm3697 { > int bank_cfg; > int num_banks; > > - struct lm3697_led leds[]; > + struct lm3697_bank banks[]; > }; > > static const struct reg_default lm3697_reg_defs[] = { > @@ -113,52 +113,52 @@ static const struct regmap_config lm3697_regmap_config = { > static int lm3697_brightness_set(struct led_classdev *led_cdev, > enum led_brightness brt_val) > { > - struct lm3697_led *led = container_of(led_cdev, struct lm3697_led, > + struct lm3697_bank *bank = container_of(led_cdev, struct lm3697_bank, > led_dev); > - int ctrl_en_val = (1 << led->control_bank); > + int ctrl_en_val = (1 << bank->control_bank); > int ret; > > - mutex_lock(&led->priv->lock); > + mutex_lock(&bank->priv->lock); > > if (brt_val == LED_OFF) { > - ret = regmap_update_bits(led->priv->regmap, LM3697_CTRL_ENABLE, > + ret = regmap_update_bits(bank->priv->regmap, LM3697_CTRL_ENABLE, > ctrl_en_val, ~ctrl_en_val); > if (ret) { > - dev_err(&led->priv->client->dev, "Cannot write ctrl register\n"); > + dev_err(&bank->priv->client->dev, "Cannot write ctrl register\n"); > goto brightness_out; > } > > - led->enabled = LED_OFF; > + bank->enabled = LED_OFF; > } else { > - ret = ti_lmu_common_set_brightness(&led->lmu_data, brt_val); > + ret = ti_lmu_common_set_brightness(&bank->lmu_data, brt_val); > if (ret) { > - dev_err(&led->priv->client->dev, > + dev_err(&bank->priv->client->dev, > "Cannot write brightness\n"); > goto brightness_out; > } > > - if (!led->enabled) { > - ret = regmap_update_bits(led->priv->regmap, > + if (!bank->enabled) { > + ret = regmap_update_bits(bank->priv->regmap, > LM3697_CTRL_ENABLE, > ctrl_en_val, ctrl_en_val); > if (ret) { > - dev_err(&led->priv->client->dev, > + dev_err(&bank->priv->client->dev, > "Cannot enable the device\n"); > goto brightness_out; > } > > - led->enabled = brt_val; > + bank->enabled = brt_val; > } > } > > brightness_out: > - mutex_unlock(&led->priv->lock); > + mutex_unlock(&bank->priv->lock); > return ret; > } > > static int lm3697_init(struct lm3697 *priv) > { > - struct lm3697_led *led; > + struct lm3697_bank *bank; > int i, ret; > > if (priv->enable_gpio) { > @@ -182,8 +182,8 @@ static int lm3697_init(struct lm3697 *priv) > dev_err(&priv->client->dev, "Cannot write OUTPUT config\n"); > > for (i = 0; i < priv->num_banks; i++) { > - led = &priv->leds[i]; > - ret = ti_lmu_common_set_ramp(&led->lmu_data); > + bank = &priv->banks[i]; > + ret = ti_lmu_common_set_ramp(&bank->lmu_data); > if (ret) > dev_err(&priv->client->dev, "Setting the ramp rate failed\n"); > } > @@ -194,7 +194,7 @@ static int lm3697_init(struct lm3697 *priv) > static int lm3697_probe_dt(struct lm3697 *priv) > { > struct fwnode_handle *child = NULL; > - struct lm3697_led *led; > + struct lm3697_bank *bank; > const char *name; > int control_bank; > size_t i = 0; > @@ -229,63 +229,63 @@ static int lm3697_probe_dt(struct lm3697 *priv) > goto child_out; > } > > - led = &priv->leds[i]; > + bank = &priv->banks[i]; > > ret = ti_lmu_common_get_brt_res(&priv->client->dev, > - child, &led->lmu_data); > + child, &bank->lmu_data); > if (ret) > dev_warn(&priv->client->dev, "brightness resolution property missing\n"); > > - led->control_bank = control_bank; > - led->lmu_data.regmap = priv->regmap; > - led->lmu_data.runtime_ramp_reg = LM3697_CTRL_A_RAMP + > + bank->control_bank = control_bank; > + bank->lmu_data.regmap = priv->regmap; > + bank->lmu_data.runtime_ramp_reg = LM3697_CTRL_A_RAMP + > control_bank; > - led->lmu_data.msb_brightness_reg = LM3697_CTRL_A_BRT_MSB + > - led->control_bank * 2; > - led->lmu_data.lsb_brightness_reg = LM3697_CTRL_A_BRT_LSB + > - led->control_bank * 2; > + bank->lmu_data.msb_brightness_reg = LM3697_CTRL_A_BRT_MSB + > + bank->control_bank * 2; > + bank->lmu_data.lsb_brightness_reg = LM3697_CTRL_A_BRT_LSB + > + bank->control_bank * 2; > > - led->num_leds = fwnode_property_count_u32(child, "led-sources"); > - if (led->num_leds > LM3697_MAX_LED_STRINGS) { > + bank->num_leds = fwnode_property_count_u32(child, "led-sources"); > + if (bank->num_leds > LM3697_MAX_LED_STRINGS) { > dev_err(&priv->client->dev, "Too many LED strings defined\n"); > continue; > } > > ret = fwnode_property_read_u32_array(child, "led-sources", > - led->hvled_strings, > - led->num_leds); > + bank->hvled_strings, > + bank->num_leds); > if (ret) { > dev_err(&priv->client->dev, "led-sources property missing\n"); > fwnode_handle_put(child); > goto child_out; > } > > - for (j = 0; j < led->num_leds; j++) > + for (j = 0; j < bank->num_leds; j++) > priv->bank_cfg |= > - (led->control_bank << led->hvled_strings[j]); > + (bank->control_bank << bank->hvled_strings[j]); > > ret = ti_lmu_common_get_ramp_params(&priv->client->dev, > - child, &led->lmu_data); > + child, &bank->lmu_data); > if (ret) > dev_warn(&priv->client->dev, "runtime-ramp properties missing\n"); > > fwnode_property_read_string(child, "linux,default-trigger", > - &led->led_dev.default_trigger); > + &bank->led_dev.default_trigger); > > ret = fwnode_property_read_string(child, "label", &name); > if (ret) > - snprintf(led->label, sizeof(led->label), > + snprintf(bank->label, sizeof(bank->label), > "%s::", priv->client->name); > else > - snprintf(led->label, sizeof(led->label), > + snprintf(bank->label, sizeof(bank->label), > "%s:%s", priv->client->name, name); > > - led->priv = priv; > - led->led_dev.name = led->label; > - led->led_dev.max_brightness = led->lmu_data.max_brightness; > - led->led_dev.brightness_set_blocking = lm3697_brightness_set; > + bank->priv = priv; > + bank->led_dev.name = bank->label; > + bank->led_dev.max_brightness = bank->lmu_data.max_brightness; > + bank->led_dev.brightness_set_blocking = lm3697_brightness_set; > > - ret = devm_led_classdev_register(priv->dev, &led->led_dev); > + ret = devm_led_classdev_register(priv->dev, &bank->led_dev); > if (ret) { > dev_err(&priv->client->dev, "led register err: %d\n", > ret); > @@ -313,7 +313,7 @@ static int lm3697_probe(struct i2c_client *client, > return -ENODEV; > } > > - led = devm_kzalloc(&client->dev, struct_size(led, leds, count), > + led = devm_kzalloc(&client->dev, struct_size(led, banks, count), > GFP_KERNEL); > if (!led) > return -ENOMEM; > -- > 2.28.0 > -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, 10 Oct 2020 20:57:00 +0200 Pavel Machek <pavel@ucw.cz> wrote: > On Fri 2020-10-09 15:51:35, Gabriel David wrote: > > The mentioned struct, lm3697_led, was renamed to lm3697_bank since the > > structure is representing the control banks. This name, in my opinion, > > is more semantically correct. The pointers referring to it were also > > renamed. > > > Signed-off-by: Gabriel David <ultracoolguy4@protonmail.com> > > --- > > Yes, this is the same Gabriel David from ultracoolguy@tutanota.org and > > ultracoolguy@disroot.org. If you want me to confirm it I'll gladly do > > it. > > No problem with that, and no need to resend. This can proably wait > for 5.11... > > I'd like some comment from Dan... and perhaps I'd want to understand > what the difference between LED and bank is. > > ...there can be more than one LED connected to the given bank, that's > what you are pointing out? > > ...but these LEDs will always work in unison, and they are handled as > single LED by Linux, right? > Pavel, the controller can connect 3 LED strips (to 3 different pins on the chip). There are 2 LED control banks (this is where you can set brightness). For each LED strip (each output pin) you can configure to which control bank it connects. So you have 3 LED strips and 2 control banks, that is 2^3 = 8 different configurations of connecting LED control bank to LED strip. From the perspective of Linux you see the two control banks as 2 LED class devices (because you are setting brightness for control banks, not for the LED strips). Marek
Pavel On 10/10/20 4:50 PM, Marek Behun wrote: > On Sat, 10 Oct 2020 20:57:00 +0200 > Pavel Machek <pavel@ucw.cz> wrote: > >> On Fri 2020-10-09 15:51:35, Gabriel David wrote: >>> The mentioned struct, lm3697_led, was renamed to lm3697_bank since the >>> structure is representing the control banks. This name, in my opinion, >>> is more semantically correct. The pointers referring to it were also >>> renamed. >>> Signed-off-by: Gabriel David <ultracoolguy4@protonmail.com> >>> --- >>> Yes, this is the same Gabriel David from ultracoolguy@tutanota.org and >>> ultracoolguy@disroot.org. If you want me to confirm it I'll gladly do >>> it. >> No problem with that, and no need to resend. This can proably wait >> for 5.11... >> >> I'd like some comment from Dan... and perhaps I'd want to understand >> what the difference between LED and bank is. >> >> ...there can be more than one LED connected to the given bank, that's >> what you are pointing out? >> >> ...but these LEDs will always work in unison, and they are handled as >> single LED by Linux, right? >> > Pavel, > > the controller can connect 3 LED strips (to 3 different pins on the > chip). There are 2 LED control banks (this is where you can set > brightness). For each LED strip (each output pin) you can configure to > which control bank it connects. So you have 3 LED strips and 2 control > banks, that is 2^3 = 8 different configurations of connecting LED > control bank to LED strip. > > From the perspective of Linux you see the two control banks as 2 LED > class devices (because you are setting brightness for control banks, > not for the LED strips). The way Marek explains it is correct and the way I wrote the driver intially. There is no direct control of the LEDs only controlling the 2 banks. As an example a device can put LED string 1 and 2 on a single bank to control the backlight for a display and put LED string 3 on a different bank to control the backlight of a keyboard. Like in the Droid and Droid 4 devices. 2 strings illuminate the display backlight and 1 string illuminates the keyboard the display backlight can have a independent brightness then the keyboard. To me the name of the structure does not impose any functional changes just semantic changes. And it just makes it a bit more difficult to back port functional fixes as this patch would be made mandatory for cherry picking. But I do not get many requests to back port this driver so it maybe be a moot point. Dan
Hi! > > From the perspective of Linux you see the two control banks as 2 LED > >class devices (because you are setting brightness for control banks, > >not for the LED strips). > > The way Marek explains it is correct and the way I wrote the driver > intially. There is no direct control of the LEDs only controlling the 2 > banks. > > As an example a device can put LED string 1 and 2 on a single bank to > control the backlight for a display and put LED string 3 on a different bank > to control the backlight of a keyboard. Like in the Droid and Droid 4 > devices. 2 strings illuminate the display backlight and 1 string > illuminates the keyboard the display backlight can have a independent > brightness then the keyboard. > > To me the name of the structure does not impose any functional changes just > semantic changes. And it just makes it a bit more difficult to back port > functional fixes as this patch would be made mandatory for cherry picking. > But I do not get many requests to back port this driver so it maybe be a > moot point. Ok, sorry for the confusion, and .. I believe the code can stay as-is. Bank is single entity Linux controls, and it does not need to know how many pins are really controlled on the hardware level. It will be confusing one way or another. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c index 31f5ed486839..c62f95fc17e8 100644 --- a/drivers/leds/leds-lm3697.c +++ b/drivers/leds/leds-lm3697.c @@ -39,7 +39,7 @@ #define LM3697_MAX_CONTROL_BANKS 2 /** - * struct lm3697_led - + * struct lm3697_bank - * @hvled_strings: Array of LED strings associated with a control bank * @label: LED label * @led_dev: LED class device @@ -48,7 +48,7 @@ * @control_bank: Control bank the LED is associated to. 0 is control bank A * 1 is control bank B */ -struct lm3697_led { +struct lm3697_bank { u32 hvled_strings[LM3697_MAX_LED_STRINGS]; char label[LED_MAX_NAME_SIZE]; struct led_classdev led_dev; @@ -80,7 +80,7 @@ struct lm3697 { int bank_cfg; int num_banks; - struct lm3697_led leds[]; + struct lm3697_bank banks[]; }; static const struct reg_default lm3697_reg_defs[] = { @@ -113,52 +113,52 @@ static const struct regmap_config lm3697_regmap_config = { static int lm3697_brightness_set(struct led_classdev *led_cdev, enum led_brightness brt_val) { - struct lm3697_led *led = container_of(led_cdev, struct lm3697_led, + struct lm3697_bank *bank = container_of(led_cdev, struct lm3697_bank, led_dev); - int ctrl_en_val = (1 << led->control_bank); + int ctrl_en_val = (1 << bank->control_bank); int ret; - mutex_lock(&led->priv->lock); + mutex_lock(&bank->priv->lock); if (brt_val == LED_OFF) { - ret = regmap_update_bits(led->priv->regmap, LM3697_CTRL_ENABLE, + ret = regmap_update_bits(bank->priv->regmap, LM3697_CTRL_ENABLE, ctrl_en_val, ~ctrl_en_val); if (ret) { - dev_err(&led->priv->client->dev, "Cannot write ctrl register\n"); + dev_err(&bank->priv->client->dev, "Cannot write ctrl register\n"); goto brightness_out; } - led->enabled = LED_OFF; + bank->enabled = LED_OFF; } else { - ret = ti_lmu_common_set_brightness(&led->lmu_data, brt_val); + ret = ti_lmu_common_set_brightness(&bank->lmu_data, brt_val); if (ret) { - dev_err(&led->priv->client->dev, + dev_err(&bank->priv->client->dev, "Cannot write brightness\n"); goto brightness_out; } - if (!led->enabled) { - ret = regmap_update_bits(led->priv->regmap, + if (!bank->enabled) { + ret = regmap_update_bits(bank->priv->regmap, LM3697_CTRL_ENABLE, ctrl_en_val, ctrl_en_val); if (ret) { - dev_err(&led->priv->client->dev, + dev_err(&bank->priv->client->dev, "Cannot enable the device\n"); goto brightness_out; } - led->enabled = brt_val; + bank->enabled = brt_val; } } brightness_out: - mutex_unlock(&led->priv->lock); + mutex_unlock(&bank->priv->lock); return ret; } static int lm3697_init(struct lm3697 *priv) { - struct lm3697_led *led; + struct lm3697_bank *bank; int i, ret; if (priv->enable_gpio) { @@ -182,8 +182,8 @@ static int lm3697_init(struct lm3697 *priv) dev_err(&priv->client->dev, "Cannot write OUTPUT config\n"); for (i = 0; i < priv->num_banks; i++) { - led = &priv->leds[i]; - ret = ti_lmu_common_set_ramp(&led->lmu_data); + bank = &priv->banks[i]; + ret = ti_lmu_common_set_ramp(&bank->lmu_data); if (ret) dev_err(&priv->client->dev, "Setting the ramp rate failed\n"); } @@ -194,7 +194,7 @@ static int lm3697_init(struct lm3697 *priv) static int lm3697_probe_dt(struct lm3697 *priv) { struct fwnode_handle *child = NULL; - struct lm3697_led *led; + struct lm3697_bank *bank; const char *name; int control_bank; size_t i = 0; @@ -229,63 +229,63 @@ static int lm3697_probe_dt(struct lm3697 *priv) goto child_out; } - led = &priv->leds[i]; + bank = &priv->banks[i]; ret = ti_lmu_common_get_brt_res(&priv->client->dev, - child, &led->lmu_data); + child, &bank->lmu_data); if (ret) dev_warn(&priv->client->dev, "brightness resolution property missing\n"); - led->control_bank = control_bank; - led->lmu_data.regmap = priv->regmap; - led->lmu_data.runtime_ramp_reg = LM3697_CTRL_A_RAMP + + bank->control_bank = control_bank; + bank->lmu_data.regmap = priv->regmap; + bank->lmu_data.runtime_ramp_reg = LM3697_CTRL_A_RAMP + control_bank; - led->lmu_data.msb_brightness_reg = LM3697_CTRL_A_BRT_MSB + - led->control_bank * 2; - led->lmu_data.lsb_brightness_reg = LM3697_CTRL_A_BRT_LSB + - led->control_bank * 2; + bank->lmu_data.msb_brightness_reg = LM3697_CTRL_A_BRT_MSB + + bank->control_bank * 2; + bank->lmu_data.lsb_brightness_reg = LM3697_CTRL_A_BRT_LSB + + bank->control_bank * 2; - led->num_leds = fwnode_property_count_u32(child, "led-sources"); - if (led->num_leds > LM3697_MAX_LED_STRINGS) { + bank->num_leds = fwnode_property_count_u32(child, "led-sources"); + if (bank->num_leds > LM3697_MAX_LED_STRINGS) { dev_err(&priv->client->dev, "Too many LED strings defined\n"); continue; } ret = fwnode_property_read_u32_array(child, "led-sources", - led->hvled_strings, - led->num_leds); + bank->hvled_strings, + bank->num_leds); if (ret) { dev_err(&priv->client->dev, "led-sources property missing\n"); fwnode_handle_put(child); goto child_out; } - for (j = 0; j < led->num_leds; j++) + for (j = 0; j < bank->num_leds; j++) priv->bank_cfg |= - (led->control_bank << led->hvled_strings[j]); + (bank->control_bank << bank->hvled_strings[j]); ret = ti_lmu_common_get_ramp_params(&priv->client->dev, - child, &led->lmu_data); + child, &bank->lmu_data); if (ret) dev_warn(&priv->client->dev, "runtime-ramp properties missing\n"); fwnode_property_read_string(child, "linux,default-trigger", - &led->led_dev.default_trigger); + &bank->led_dev.default_trigger); ret = fwnode_property_read_string(child, "label", &name); if (ret) - snprintf(led->label, sizeof(led->label), + snprintf(bank->label, sizeof(bank->label), "%s::", priv->client->name); else - snprintf(led->label, sizeof(led->label), + snprintf(bank->label, sizeof(bank->label), "%s:%s", priv->client->name, name); - led->priv = priv; - led->led_dev.name = led->label; - led->led_dev.max_brightness = led->lmu_data.max_brightness; - led->led_dev.brightness_set_blocking = lm3697_brightness_set; + bank->priv = priv; + bank->led_dev.name = bank->label; + bank->led_dev.max_brightness = bank->lmu_data.max_brightness; + bank->led_dev.brightness_set_blocking = lm3697_brightness_set; - ret = devm_led_classdev_register(priv->dev, &led->led_dev); + ret = devm_led_classdev_register(priv->dev, &bank->led_dev); if (ret) { dev_err(&priv->client->dev, "led register err: %d\n", ret); @@ -313,7 +313,7 @@ static int lm3697_probe(struct i2c_client *client, return -ENODEV; } - led = devm_kzalloc(&client->dev, struct_size(led, leds, count), + led = devm_kzalloc(&client->dev, struct_size(led, banks, count), GFP_KERNEL); if (!led) return -ENOMEM;
The mentioned struct, lm3697_led, was renamed to lm3697_bank since the structure is representing the control banks. This name, in my opinion, is more semantically correct. The pointers referring to it were also renamed. Signed-off-by: Gabriel David <ultracoolguy4@protonmail.com> --- Yes, this is the same Gabriel David from ultracoolguy@tutanota.org and ultracoolguy@disroot.org. If you want me to confirm it I'll gladly do it. drivers/leds/leds-lm3697.c | 90 +++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 45 deletions(-) -- 2.28.0