On Mon, Jul 25, 2022 at 4:41 PM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: ... > > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx> > > ^^^^ (Note this and read below) ... > In conjunction with above what SoB of Alice means? > > You really need to take your time and (re-)read > https://www.kernel.org/doc/html/latest/process/submitting-patches.html. Hi Andy, Thanks for your reply. We are very sorry for this mistake. We will revise it in the next patch. > > ... > > > + * Author: Alice Chen <alice_chen@xxxxxxxxxxx> > > + * Author: ChiYuan Huang <cy_huang@xxxxxxxxxxx> > > Would > * Authors: > * Name_of_Author 1 > * Name_of_Author 2 > > work for you? It looks good, thanks! We will apply this in the next patch. ... > > +struct mt6370_led { > > + union { > > + struct led_classdev isink; > > + struct led_classdev_mc mc; > > + }; > > Where is the field that makes union work? Just for saving memory space. Because these led_classdevs do not be used at the same time. Or do you think it would be better to rewrite it as follows? ------------------------------------------------------------------------------------- struct mt6370_led { struct led_classdev isink; struct led_classdev_mc mc; struct mt6370_priv *priv; u32 default_state; u32 index; }; ------------------------------------------------------------------------------------- ... > > +static int mt6370_gen_breath_pattern(struct mt6370_priv *priv, > > + struct led_pattern *pattern, u32 len, > > + u8 *pattern_val, u32 val_len) > > +{ > > + enum mt6370_led_ranges sel_range; > > + struct led_pattern *curr; > > + unsigned int sel; > > + u8 val[P_MAX_PATTERNS / 2] = {}; > > + int i; > > + > > + if (len < P_MAX_PATTERNS && val_len < P_MAX_PATTERNS / 2) > > + return -EINVAL; > > + > > + /* > > + * Pattern list > > + * tr1: byte 0, b'[7: 4] > > + * tr2: byte 0, b'[3: 0] > > + * tf1: byte 1, b'[7: 4] > > + * tf2: byte 1, b'[3: 0] > > + * ton: byte 2, b'[7: 4] > > + * toff: byte 2, b'[3: 0] > > + */ > > + for (i = 0; i < P_MAX_PATTERNS; i++) { > > + curr = pattern + i; > > + > > + sel_range = i == P_LED_TOFF ? R_LED_TOFF : R_LED_TRFON; > > + > > + linear_range_get_selector_within(priv->ranges + sel_range, > > + curr->delta_t, &sel); > > + > > + val[i / 2] |= sel << (4 * ((i + 1) % 2)); > > + } > > + > > + memcpy(pattern_val, val, 3); > > Isn't it something like put_unaligned_be24()/put_unaligned_le24()? OK, we will try to apply this method in the next patch. Thank you so much for reviewing our patches so many times and providing so many great suggestions! -- Best Regards, ChiaEn Wu
