| Message ID | 1541592640-18478-3-git-send-email-bdodge09@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | backlight/arcxcnn fix vendor prefix in driver and bindings and add support for arc1, arc3 | expand |
Hi! > The vendor-prefixes.txt file properly refers to ArcticSand > as arctic but the driver improperly abbreviated the prefix > to arc. This was a mistake in the original patch > > Signed-off-by: Brian Dodge <bdodge09@gmail.com> > --- > drivers/video/backlight/arcxcnn_bl.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > * > - * Copyright 2016 ArcticSand, Inc. > - * Author : Brian Dodge <bdodge@arcticsand.com> > + * Copyright 2018 pSemi, Inc. > + * Author : Brian Dodge <bdodge@psemi.com> Ummm. Copyright 2016-2018? > @@ -202,27 +202,27 @@ static void arcxcnn_parse_dt(struct arcxcnn *lp) > if (ret == 0) > lp->pdata->initial_brightness = prog_val; > > - ret = of_property_read_u32(node, "arc,led-config-0", &prog_val); > + ret = of_property_read_u32(node, "arctic,led-config-0", &prog_val); > if (ret == 0) > lp->pdata->led_config_0 = (u8)prog_val; > If there's a dts using this, you want to update it at the same time. You may want to support both names going forward. Pavel
Thank you Pavel, that is a good point. The chip vendor has indicated that there is no reason to maintain the old/improper prefix and wishes to go forward (only) with the "arctic" prefix and any existing dts files are or will be updated. On 11/11/18 6:30 AM, Pavel Machek wrote: > Hi! > >> The vendor-prefixes.txt file properly refers to ArcticSand >> as arctic but the driver improperly abbreviated the prefix >> to arc. This was a mistake in the original patch >> >> Signed-off-by: Brian Dodge <bdodge09@gmail.com> >> --- >> drivers/video/backlight/arcxcnn_bl.c | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) >> >> * >> - * Copyright 2016 ArcticSand, Inc. >> - * Author : Brian Dodge <bdodge@arcticsand.com> >> + * Copyright 2018 pSemi, Inc. >> + * Author : Brian Dodge <bdodge@psemi.com> > Ummm. Copyright 2016-2018? > >> @@ -202,27 +202,27 @@ static void arcxcnn_parse_dt(struct arcxcnn *lp) >> if (ret == 0) >> lp->pdata->initial_brightness = prog_val; >> >> - ret = of_property_read_u32(node, "arc,led-config-0", &prog_val); >> + ret = of_property_read_u32(node, "arctic,led-config-0", &prog_val); >> if (ret == 0) >> lp->pdata->led_config_0 = (u8)prog_val; >> > If there's a dts using this, you want to update it at the same > time. You may want to support both names going forward. > Pavel
Hi Brian On Tue, 27 Nov 2018 at 00:44, Brian Dodge <bdodge09@gmail.com> wrote: > Thank you Pavel, that is a good point. > > The chip vendor has indicated that there is no reason to maintain the > old/improper prefix and wishes to go forward (only) with the "arctic" > prefix and any existing dts files are or will be updated. > Looks like this patch series has fallen into the cracks a little. I think I assumed this info would end in the description of patch v2 1/3 (in order to answer Rob's feedback) and I sat and waited for a respin. On the other hand... I didn't actually say that explicitly anywhere! So... I'd recommend a respin perhaps with a small bit of text explaining how the vendor can state that any existing dts files will be updated. This is a peripheral device so these strings are probably embedded into OEM devicetrees rather than exclusively under the control of the vendor. Daniel. > On 11/11/18 6:30 AM, Pavel Machek wrote: > > Hi! > > > >> The vendor-prefixes.txt file properly refers to ArcticSand > >> as arctic but the driver improperly abbreviated the prefix > >> to arc. This was a mistake in the original patch > >> > >> Signed-off-by: Brian Dodge <bdodge09@gmail.com> > >> --- > >> drivers/video/backlight/arcxcnn_bl.c | 22 +++++++++++----------- > >> 1 file changed, 11 insertions(+), 11 deletions(-) > >> > >> * > >> - * Copyright 2016 ArcticSand, Inc. > >> - * Author : Brian Dodge <bdodge@arcticsand.com> > >> + * Copyright 2018 pSemi, Inc. > >> + * Author : Brian Dodge <bdodge@psemi.com> > > Ummm. Copyright 2016-2018? > > > >> @@ -202,27 +202,27 @@ static void arcxcnn_parse_dt(struct arcxcnn *lp) > >> if (ret == 0) > >> lp->pdata->initial_brightness = prog_val; > >> > >> - ret = of_property_read_u32(node, "arc,led-config-0", &prog_val); > >> + ret = of_property_read_u32(node, "arctic,led-config-0", &prog_val); > >> if (ret == 0) > >> lp->pdata->led_config_0 = (u8)prog_val; > >> > > If there's a dts using this, you want to update it at the same > > time. You may want to support both names going forward. > > > Pavel > <div dir="ltr"><div>Hi Brian</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 27 Nov 2018 at 00:44, Brian Dodge <<a href="mailto:bdodge09@gmail.com">bdodge09@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Thank you Pavel, that is a good point.<br> <br> The chip vendor has indicated that there is no reason to maintain the <br> old/improper prefix and wishes to go forward (only) with the "arctic" <br> prefix and any existing dts files are or will be updated.<br></blockquote><div><br></div><div>Looks like this patch series has fallen into the cracks a little.</div><div><br></div><div>I think I assumed this info would end in the description of patch v2 1/3 (in order to answer Rob's feedback) and I sat and waited for a respin. On the other hand... I didn't actually say that explicitly anywhere! So... I'd recommend a respin perhaps with a small bit of text explaining how the vendor can state that any existing dts files will be updated. This is a peripheral device so these strings are probably embedded into OEM devicetrees rather than exclusively under the control of the vendor.</div><div><br></div><div><br></div><div>Daniel.</div><div><br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> On 11/11/18 6:30 AM, Pavel Machek wrote:<br> > Hi!<br> ><br> >> The vendor-prefixes.txt file properly refers to ArcticSand<br> >> as arctic but the driver improperly abbreviated the prefix<br> >> to arc. This was a mistake in the original patch<br> >><br> >> Signed-off-by: Brian Dodge <<a href="mailto:bdodge09@gmail.com" target="_blank">bdodge09@gmail.com</a>><br> >> ---<br> >> drivers/video/backlight/arcxcnn_bl.c | 22 +++++++++++-----------<br> >> 1 file changed, 11 insertions(+), 11 deletions(-)<br> >><br> >> *<br> >> - * Copyright 2016 ArcticSand, Inc.<br> >> - * Author : Brian Dodge <<a href="mailto:bdodge@arcticsand.com" target="_blank">bdodge@arcticsand.com</a>><br> >> + * Copyright 2018 pSemi, Inc.<br> >> + * Author : Brian Dodge <<a href="mailto:bdodge@psemi.com" target="_blank">bdodge@psemi.com</a>><br> > Ummm. Copyright 2016-2018?<br> ><br> >> @@ -202,27 +202,27 @@ static void arcxcnn_parse_dt(struct arcxcnn *lp)<br> >> if (ret == 0)<br> >> lp->pdata->initial_brightness = prog_val;<br> >><br> >> - ret = of_property_read_u32(node, "arc,led-config-0", &prog_val);<br> >> + ret = of_property_read_u32(node, "arctic,led-config-0", &prog_val);<br> >> if (ret == 0)<br> >> lp->pdata->led_config_0 = (u8)prog_val;<br> >><br> > If there's a dts using this, you want to update it at the same<br> > time. You may want to support both names going forward.<br> > Pavel<br> </blockquote></div></div>
[Sorry to those receiving this twice... had to dig this out from the archives and sent it to the lists from the wrong mailer] On 27/11/2018 00:44, Brian Dodge wrote: > Thank you Pavel, that is a good point. > > The chip vendor has indicated that there is no reason to maintain the > old/improper prefix and wishes to go forward (only) with the "arctic" > prefix and any existing dts files are or will be updated. Looks like this patch series has fallen into the cracks a little. I think I assumed this info would end in the description of patch v2 1/3 (in order to answer Rob's feedback) and I sat and waited for a respin. On the other hand... I didn't actually say that explicitly anywhere! So... I'd recommend a respin perhaps with a small bit of text explaining how the vendor can state that any existing dts files will be updated. This is a peripheral device so these strings are probably embedded into OEM devicetrees rather than exclusively under the control of the vendor. Daniel. > > On 11/11/18 6:30 AM, Pavel Machek wrote: >> Hi! >> >>> The vendor-prefixes.txt file properly refers to ArcticSand >>> as arctic but the driver improperly abbreviated the prefix >>> to arc. This was a mistake in the original patch >>> >>> Signed-off-by: Brian Dodge <bdodge09@gmail.com> >>> --- >>> drivers/video/backlight/arcxcnn_bl.c | 22 +++++++++++----------- >>> 1 file changed, 11 insertions(+), 11 deletions(-) >>> >>> * >>> - * Copyright 2016 ArcticSand, Inc. >>> - * Author : Brian Dodge <bdodge@arcticsand.com> >>> + * Copyright 2018 pSemi, Inc. >>> + * Author : Brian Dodge <bdodge@psemi.com> >> Ummm. Copyright 2016-2018? >> >>> @@ -202,27 +202,27 @@ static void arcxcnn_parse_dt(struct arcxcnn *lp) >>> if (ret == 0) >>> lp->pdata->initial_brightness = prog_val; >>> >>> - ret = of_property_read_u32(node, "arc,led-config-0", &prog_val); >>> + ret = of_property_read_u32(node, "arctic,led-config-0", &prog_val); >>> if (ret == 0) >>> lp->pdata->led_config_0 = (u8)prog_val; >>> >> If there's a dts using this, you want to update it at the same >> time. You may want to support both names going forward. >> Pavel
On 21/06/2019 14:46, Daniel Thompson wrote: > [Sorry to those receiving this twice... had to dig this out from the > archives and sent it to the lists from the wrong mailer] > > On 27/11/2018 00:44, Brian Dodge wrote: >> Thank you Pavel, that is a good point. >> >> The chip vendor has indicated that there is no reason to maintain the >> old/improper prefix and wishes to go forward (only) with the "arctic" >> prefix and any existing dts files are or will be updated. > > Looks like this patch series has fallen into the cracks a little. > > I think I assumed this info would end in the description of patch v2 1/3 > (in order to answer Rob's feedback) and I sat and waited for a respin. > On the other hand... I didn't actually say that explicitly anywhere! > So... I'd recommend a respin perhaps with a small bit of text explaining > how the vendor can state that any existing dts files will be updated. > This is a peripheral device so these strings are probably embedded into > OEM devicetrees rather than exclusively under the control of the vendor. In fact there's a publicly available example using this binding: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/factory-gru-8652.B-chromeos-4.4/arch/arm64/boot/dts/rockchip/rk3399-gru-gru.dtsi I'm not sure it could be changed without maintaining support for old names. Daniel. > > > Daniel. > > >> >> On 11/11/18 6:30 AM, Pavel Machek wrote: >>> Hi! >>> >>>> The vendor-prefixes.txt file properly refers to ArcticSand >>>> as arctic but the driver improperly abbreviated the prefix >>>> to arc. This was a mistake in the original patch >>>> >>>> Signed-off-by: Brian Dodge <bdodge09@gmail.com> >>>> --- >>>> drivers/video/backlight/arcxcnn_bl.c | 22 +++++++++++----------- >>>> 1 file changed, 11 insertions(+), 11 deletions(-) >>>> >>>> * >>>> - * Copyright 2016 ArcticSand, Inc. >>>> - * Author : Brian Dodge <bdodge@arcticsand.com> >>>> + * Copyright 2018 pSemi, Inc. >>>> + * Author : Brian Dodge <bdodge@psemi.com> >>> Ummm. Copyright 2016-2018? >>> >>>> @@ -202,27 +202,27 @@ static void arcxcnn_parse_dt(struct arcxcnn *lp) >>>> if (ret == 0) >>>> lp->pdata->initial_brightness = prog_val; >>>> >>>> - ret = of_property_read_u32(node, "arc,led-config-0", &prog_val); >>>> + ret = of_property_read_u32(node, "arctic,led-config-0", >>>> &prog_val); >>>> if (ret == 0) >>>> lp->pdata->led_config_0 = (u8)prog_val; >>>> >>> If there's a dts using this, you want to update it at the same >>> time. You may want to support both names going forward. >>> Pavel >
Hi! > [Sorry to those receiving this twice... had to dig this out from the > archives and sent it to the lists from the wrong mailer] > > On 27/11/2018 00:44, Brian Dodge wrote: > >Thank you Pavel, that is a good point. > > > >The chip vendor has indicated that there is no reason to maintain the > >old/improper prefix and wishes to go forward (only) with the "arctic" > >prefix and any existing dts files are or will be updated. > > Looks like this patch series has fallen into the cracks a little. > > I think I assumed this info would end in the description of patch v2 1/3 (in > order to answer Rob's feedback) and I sat and waited for a respin. On the > other hand... I didn't actually say that explicitly anywhere! So... I'd > recommend a respin perhaps with a small bit of text explaining how the > vendor can state that any existing dts files will be updated. This is a > peripheral device so these strings are probably embedded into OEM > devicetrees rather than exclusively under the control of the vendor. So in next email you give good reason not to apply this :-). Anyway, this is Doc*/devicetree/, so not my area. Pavel
On Sat, Jun 22, 2019 at 12:13:25AM +0200, Pavel Machek wrote: > Hi! > > > [Sorry to those receiving this twice... had to dig this out from the > > archives and sent it to the lists from the wrong mailer] > > > > On 27/11/2018 00:44, Brian Dodge wrote: > > >Thank you Pavel, that is a good point. > > > > > >The chip vendor has indicated that there is no reason to maintain the > > >old/improper prefix and wishes to go forward (only) with the "arctic" > > >prefix and any existing dts files are or will be updated. > > > > Looks like this patch series has fallen into the cracks a little. > > > > I think I assumed this info would end in the description of patch v2 1/3 (in > > order to answer Rob's feedback) and I sat and waited for a respin. On the > > other hand... I didn't actually say that explicitly anywhere! So... I'd > > recommend a respin perhaps with a small bit of text explaining how the > > vendor can state that any existing dts files will be updated. This is a > > peripheral device so these strings are probably embedded into OEM > > devicetrees rather than exclusively under the control of the vendor. > > So in next email you give good reason not to apply this :-). Afraid so... it was on page 2 of my google search so I did a quick search, sent the first mail and then went back to my web browser. It was at that moment that I decided a quick search wasn't enough and decided to got a little deeper! Daniel.
This sure did fall through the cracks. I confirmed with the vendor that there are no existing embedded DTS with the wrong name(s) in them before submitting this patch. The new owner of this chip family, pSemi, just wanted me to wrap things up and support all of there chips (3) in a single driver and that was the extent of the work for me. Since then the manager of the program there has also changed. I assume they'd still want these changes in for completeness. AFAIK, there were just some quibbles about the copyright date range. Can you please help me push these patches in? It'll take me some time to get back in to where I left things since its been so long. I know its a bit messy but the DTS and driver changes need to be together to make sense so I couldn't really do an incremental patch sequence. What is the next step? Brian On 6/24/19 6:24 AM, Daniel Thompson wrote: > On Sat, Jun 22, 2019 at 12:13:25AM +0200, Pavel Machek wrote: >> Hi! >> >>> [Sorry to those receiving this twice... had to dig this out from the >>> archives and sent it to the lists from the wrong mailer] >>> >>> On 27/11/2018 00:44, Brian Dodge wrote: >>>> Thank you Pavel, that is a good point. >>>> >>>> The chip vendor has indicated that there is no reason to maintain the >>>> old/improper prefix and wishes to go forward (only) with the "arctic" >>>> prefix and any existing dts files are or will be updated. >>> Looks like this patch series has fallen into the cracks a little. >>> >>> I think I assumed this info would end in the description of patch v2 1/3 (in >>> order to answer Rob's feedback) and I sat and waited for a respin. On the >>> other hand... I didn't actually say that explicitly anywhere! So... I'd >>> recommend a respin perhaps with a small bit of text explaining how the >>> vendor can state that any existing dts files will be updated. This is a >>> peripheral device so these strings are probably embedded into OEM >>> devicetrees rather than exclusively under the control of the vendor. >> So in next email you give good reason not to apply this :-). > Afraid so... it was on page 2 of my google search so I did a quick > search, sent the first mail and then went back to my web browser. > > It was at that moment that I decided a quick search wasn't enough and > decided to got a little deeper! > > > Daniel.
On Mon, Jun 24, 2019 at 07:29:20AM -0400, Brian Dodge wrote: > This sure did fall through the cracks. > > I confirmed with the vendor that there are no existing embedded DTS with the > wrong name(s) in them before submitting this patch. > > The new owner of this chip family, pSemi, just wanted me to wrap things up > and support all of there chips (3) in a single driver and that was the > extent of the work for me. Since then the manager of the program there has > also changed. I assume they'd still want these changes in for completeness. > > AFAIK, there were just some quibbles about the copyright date range. Can > you please help me push these patches in? It'll take me some time to get > back in to where I left things since its been so long. I know its a bit > messy but the DTS and driver changes need to be together to make sense so I > couldn't really do an incremental patch sequence. > > What is the next step? The next step is getting the changes to DT bindings agreed. Until that happens the patchset cannot move and right now now the thread for that patch has feedback that was not replies to: https://patchwork.kernel.org/patch/10675451/ The explanation of why it is safe to accept the change to the DT bindings really should end up in the patch description rather than the mail thread. It would probably also help to have a link to https://lkml.org/lkml/2018/9/25/726 where it looks like arc versus arctic was previously discussed. The following might also be convenient for you: A quick web search for "arc,arc2c0608" suggests that the only public user is the Samsung Chromebook Plus and it is likely that this device will remain on the v4.4 kernel. For this reason we do not provide any deprecated fallback names based on "arc". Daniel. > > Brian > > On 6/24/19 6:24 AM, Daniel Thompson wrote: > > On Sat, Jun 22, 2019 at 12:13:25AM +0200, Pavel Machek wrote: > > > Hi! > > > > > > > [Sorry to those receiving this twice... had to dig this out from the > > > > archives and sent it to the lists from the wrong mailer] > > > > > > > > On 27/11/2018 00:44, Brian Dodge wrote: > > > > > Thank you Pavel, that is a good point. > > > > > > > > > > The chip vendor has indicated that there is no reason to maintain the > > > > > old/improper prefix and wishes to go forward (only) with the "arctic" > > > > > prefix and any existing dts files are or will be updated. > > > > Looks like this patch series has fallen into the cracks a little. > > > > > > > > I think I assumed this info would end in the description of patch v2 1/3 (in > > > > order to answer Rob's feedback) and I sat and waited for a respin. On the > > > > other hand... I didn't actually say that explicitly anywhere! So... I'd > > > > recommend a respin perhaps with a small bit of text explaining how the > > > > vendor can state that any existing dts files will be updated. This is a > > > > peripheral device so these strings are probably embedded into OEM > > > > devicetrees rather than exclusively under the control of the vendor. > > > So in next email you give good reason not to apply this :-). > > Afraid so... it was on page 2 of my google search so I did a quick > > search, sent the first mail and then went back to my web browser. > > > > It was at that moment that I decided a quick search wasn't enough and > > decided to got a little deeper! > > > > > > Daniel.
diff --git a/drivers/video/backlight/arcxcnn_bl.c b/drivers/video/backlight/arcxcnn_bl.c index dec790d..bebefc6 100644 --- a/drivers/video/backlight/arcxcnn_bl.c +++ b/drivers/video/backlight/arcxcnn_bl.c @@ -1,8 +1,8 @@ /* - * Backlight driver for ArcticSand ARC_X_C_0N_0N Devices + * Backlight driver for pSemi (formerly ArcticSand) ARC_X_C_0N_0N Devices * - * Copyright 2016 ArcticSand, Inc. - * Author : Brian Dodge <bdodge@arcticsand.com> + * Copyright 2018 pSemi, Inc. + * Author : Brian Dodge <bdodge@psemi.com> * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License version 2 @@ -202,27 +202,27 @@ static void arcxcnn_parse_dt(struct arcxcnn *lp) if (ret == 0) lp->pdata->initial_brightness = prog_val; - ret = of_property_read_u32(node, "arc,led-config-0", &prog_val); + ret = of_property_read_u32(node, "arctic,led-config-0", &prog_val); if (ret == 0) lp->pdata->led_config_0 = (u8)prog_val; - ret = of_property_read_u32(node, "arc,led-config-1", &prog_val); + ret = of_property_read_u32(node, "arctic,led-config-1", &prog_val); if (ret == 0) lp->pdata->led_config_1 = (u8)prog_val; - ret = of_property_read_u32(node, "arc,dim-freq", &prog_val); + ret = of_property_read_u32(node, "arctic,dim-freq", &prog_val); if (ret == 0) lp->pdata->dim_freq = (u8)prog_val; - ret = of_property_read_u32(node, "arc,comp-config", &prog_val); + ret = of_property_read_u32(node, "arctic,comp-config", &prog_val); if (ret == 0) lp->pdata->comp_config = (u8)prog_val; - ret = of_property_read_u32(node, "arc,filter-config", &prog_val); + ret = of_property_read_u32(node, "arctic,filter-config", &prog_val); if (ret == 0) lp->pdata->filter_config = (u8)prog_val; - ret = of_property_read_u32(node, "arc,trim-config", &prog_val); + ret = of_property_read_u32(node, "arctic,trim-config", &prog_val); if (ret == 0) lp->pdata->trim_config = (u8)prog_val; @@ -392,7 +392,7 @@ static int arcxcnn_remove(struct i2c_client *cl) } static const struct of_device_id arcxcnn_dt_ids[] = { - { .compatible = "arc,arc2c0608" }, + { .compatible = "arctic,arc2c0608" }, { } }; MODULE_DEVICE_TABLE(of, arcxcnn_dt_ids); @@ -415,5 +415,5 @@ static struct i2c_driver arcxcnn_driver = { module_i2c_driver(arcxcnn_driver); MODULE_LICENSE("GPL v2"); -MODULE_AUTHOR("Brian Dodge <bdodge@arcticsand.com>"); +MODULE_AUTHOR("Brian Dodge <bdodge@psemi.com>"); MODULE_DESCRIPTION("ARCXCNN Backlight driver");
The vendor-prefixes.txt file properly refers to ArcticSand as arctic but the driver improperly abbreviated the prefix to arc. This was a mistake in the original patch Signed-off-by: Brian Dodge <bdodge09@gmail.com> --- drivers/video/backlight/arcxcnn_bl.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) -- 2.7.4