* [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name @ 2023-01-18 10:06 Andy Shevchenko 2023-01-18 12:35 ` Marijn Suijten 2023-01-22 17:24 ` Jonathan Cameron 0 siblings, 2 replies; 16+ messages in thread From: Andy Shevchenko @ 2023-01-18 10:06 UTC (permalink / raw) To: Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Jonathan Cameron, Lars-Peter Clausen, Andy Shevchenko, Marijn Suijten The node name can contain an address part which is unused by the driver. Moreover, this string is propagated into the userspace label, sysfs filenames *and breaking ABI*. Cut the address part out before assigning the channel name. Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") Reported-by: Marijn Suijten <marijn.suijten@somainline.org> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- v2: rephrased commit message (Marijn), fixed compilation issue (Marijin) drivers/iio/adc/qcom-spmi-adc5.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c index e90c299c913a..c2d5e06f137a 100644 --- a/drivers/iio/adc/qcom-spmi-adc5.c +++ b/drivers/iio/adc/qcom-spmi-adc5.c @@ -628,12 +628,20 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc, struct fwnode_handle *fwnode, const struct adc5_data *data) { - const char *name = fwnode_get_name(fwnode), *channel_name; + const char *channel_name; + char *name; u32 chan, value, varr[2]; u32 sid = 0; int ret; struct device *dev = adc->dev; + name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode); + if (!name) + return -ENOMEM; + + /* Cut the address part */ + name[strchrnul(name, '@') - name] = '\0'; + ret = fwnode_property_read_u32(fwnode, "reg", &chan); if (ret) { dev_err(dev, "invalid channel number %s\n", name); -- 2.39.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-18 10:06 [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name Andy Shevchenko @ 2023-01-18 12:35 ` Marijn Suijten 2023-01-18 13:22 ` Andy Shevchenko 2023-01-22 17:24 ` Jonathan Cameron 1 sibling, 1 reply; 16+ messages in thread From: Marijn Suijten @ 2023-01-18 12:35 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio, Jonathan Cameron, Lars-Peter Clausen On 2023-01-18 12:06:23, Andy Shevchenko wrote: > The node name can contain an address part which is unused > by the driver. Moreover, this string is propagated into > the userspace label, sysfs filenames *and breaking ABI*. > > Cut the address part out before assigning the channel name. > > Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") > Reported-by: Marijn Suijten <marijn.suijten@somainline.org> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> One question and suggestion below. > --- > v2: rephrased commit message (Marijn), fixed compilation issue (Marijin) > drivers/iio/adc/qcom-spmi-adc5.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c > index e90c299c913a..c2d5e06f137a 100644 > --- a/drivers/iio/adc/qcom-spmi-adc5.c > +++ b/drivers/iio/adc/qcom-spmi-adc5.c > @@ -628,12 +628,20 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc, > struct fwnode_handle *fwnode, > const struct adc5_data *data) > { > - const char *name = fwnode_get_name(fwnode), *channel_name; > + const char *channel_name; > + char *name; > u32 chan, value, varr[2]; > u32 sid = 0; > int ret; > struct device *dev = adc->dev; > > + name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode); Is this better/cleaner than copying the string from fwnode_get_name? > + if (!name) > + return -ENOMEM; > + > + /* Cut the address part */ > + name[strchrnul(name, '@') - name] = '\0'; This is the same as *strchrnul(name, '@') = '\0'; if I'm not mistaken. - Marijn > + > ret = fwnode_property_read_u32(fwnode, "reg", &chan); > if (ret) { > dev_err(dev, "invalid channel number %s\n", name); > -- > 2.39.0 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-18 12:35 ` Marijn Suijten @ 2023-01-18 13:22 ` Andy Shevchenko 2023-01-18 14:04 ` Marijn Suijten 0 siblings, 1 reply; 16+ messages in thread From: Andy Shevchenko @ 2023-01-18 13:22 UTC (permalink / raw) To: Marijn Suijten Cc: Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio, Jonathan Cameron, Lars-Peter Clausen On Wed, Jan 18, 2023 at 01:35:28PM +0100, Marijn Suijten wrote: > On 2023-01-18 12:06:23, Andy Shevchenko wrote: > > The node name can contain an address part which is unused > > by the driver. Moreover, this string is propagated into > > the userspace label, sysfs filenames *and breaking ABI*. > > > > Cut the address part out before assigning the channel name. > > > > Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") > > Reported-by: Marijn Suijten <marijn.suijten@somainline.org> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> Thank you! My answers below. ... > > + name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode); > > Is this better/cleaner than copying the string from fwnode_get_name? Coying to where? And what would be the lifetime of that string? With devm_kasprintf(): - we don't care how long the string is - we don't care about corner cases of lifetime as it's the same as device itself (i.o.w. the same as the IIO device container) ... > > + name[strchrnul(name, '@') - name] = '\0'; > > This is the same as *strchrnul(name, '@') = '\0'; if I'm not mistaken. Yes, But it's harder to read and understand. I believe the compiler has enough power to optimize this to the same assembly code. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-18 13:22 ` Andy Shevchenko @ 2023-01-18 14:04 ` Marijn Suijten 2023-01-18 14:29 ` Andy Shevchenko 0 siblings, 1 reply; 16+ messages in thread From: Marijn Suijten @ 2023-01-18 14:04 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio, Jonathan Cameron, Lars-Peter Clausen On 2023-01-18 15:22:42, Andy Shevchenko wrote: ... > > > + name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode); > > > > Is this better/cleaner than copying the string from fwnode_get_name? > > Coying to where? And what would be the lifetime of that string? > > With devm_kasprintf(): > - we don't care how long the string is > - we don't care about corner cases of lifetime as it's the same as > device itself (i.o.w. the same as the IIO device container) Curious if there isn't a devm_strdup(name) or similar? Main point is that %pfwP seems like magic when fwnode_get_name is not (but returns a const string that we cannot modify). If there is not, let's stick with devm_kasprintf(). > > > + name[strchrnul(name, '@') - name] = '\0'; > > > > This is the same as *strchrnul(name, '@') = '\0'; if I'm not mistaken. > > Yes, But it's harder to read and understand. I believe the compiler has > enough power to optimize this to the same assembly code. I find the latter clearer as it doesn't require the reader to figure out that name - name cancels itself out. Alternatively we can write strchrnul(name, '@')[0]. - Marijn ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-18 14:04 ` Marijn Suijten @ 2023-01-18 14:29 ` Andy Shevchenko 2023-01-18 15:21 ` Marijn Suijten 0 siblings, 1 reply; 16+ messages in thread From: Andy Shevchenko @ 2023-01-18 14:29 UTC (permalink / raw) To: Marijn Suijten Cc: Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio, Jonathan Cameron, Lars-Peter Clausen On Wed, Jan 18, 2023 at 03:04:23PM +0100, Marijn Suijten wrote: > On 2023-01-18 15:22:42, Andy Shevchenko wrote: ... > > > > + name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode); > > > > > > Is this better/cleaner than copying the string from fwnode_get_name? > > > > Coying to where? And what would be the lifetime of that string? > > > > With devm_kasprintf(): > > - we don't care how long the string is > > - we don't care about corner cases of lifetime as it's the same as > > device itself (i.o.w. the same as the IIO device container) > > Curious if there isn't a devm_strdup(name) or similar? Main point is > that %pfwP seems like magic when fwnode_get_name is not (but returns a > const string that we cannot modify). The devm_kstrdup(fwnode_get_name()) is an open coded variant of the above. I don't think we need to open code and produce NIH even a single API. And no, there is no magic behind that. At least from the fwnode point of view. You may very well say that > 1500 instances of "%pOF" is a magic... > If there is not, let's stick with > devm_kasprintf(). There is, but I'm against it. See above why. > > > > + name[strchrnul(name, '@') - name] = '\0'; > > > > > > This is the same as *strchrnul(name, '@') = '\0'; if I'm not mistaken. > > > > Yes, But it's harder to read and understand. I believe the compiler has > > enough power to optimize this to the same assembly code. > > I find the latter clearer as it doesn't require the reader to figure out > that name - name cancels itself out. Alternatively we can write > strchrnul(name, '@')[0]. I don't like to have Pythonisms in the C code, really. P.S. I guess this little patch already emptied my bandwidth, so I leave any further discussion to you and IIO maintainers. Thank you for the review! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-18 14:29 ` Andy Shevchenko @ 2023-01-18 15:21 ` Marijn Suijten 2023-01-18 15:57 ` Andy Shevchenko 0 siblings, 1 reply; 16+ messages in thread From: Marijn Suijten @ 2023-01-18 15:21 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio, Jonathan Cameron, Lars-Peter Clausen On 2023-01-18 16:29:24, Andy Shevchenko wrote: ... > The devm_kstrdup(fwnode_get_name()) is an open coded variant of the above. > I don't think we need to open code and produce NIH even a single API. And > no, there is no magic behind that. At least from the fwnode point of view. > > You may very well say that > 1500 instances of "%pOF" is a magic... Forgive me for not having a clear definition of "open coding" in mind (showing a different way of implementing something, compared to the "status quo" that I was not yet aware of?), nor knowing what NIH is supposed to mean in this context. We're in bike-shedding territory anyway, guess I should just bookmark the page that details all the many `%` format strings available. ... > > I find the latter clearer as it doesn't require the reader to figure out > > that name - name cancels itself out. Alternatively we can write > > strchrnul(name, '@')[0]. > > I don't like to have Pythonisms in the C code, really. > > P.S. I guess this little patch already emptied my bandwidth, so I leave > any further discussion to you and IIO maintainers. Thank you for the > review! Just soaking up kernel coding standards here :) - Marijn ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-18 15:21 ` Marijn Suijten @ 2023-01-18 15:57 ` Andy Shevchenko 0 siblings, 0 replies; 16+ messages in thread From: Andy Shevchenko @ 2023-01-18 15:57 UTC (permalink / raw) To: Marijn Suijten Cc: Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio, Jonathan Cameron, Lars-Peter Clausen On Wed, Jan 18, 2023 at 04:21:21PM +0100, Marijn Suijten wrote: > On 2023-01-18 16:29:24, Andy Shevchenko wrote: ... > > The devm_kstrdup(fwnode_get_name()) is an open coded variant of the above. > > I don't think we need to open code and produce NIH even a single API. And > > no, there is no magic behind that. At least from the fwnode point of view. > > > > You may very well say that > 1500 instances of "%pOF" is a magic... > > Forgive me for not having a clear definition of "open coding" in mind > (showing a different way of implementing something, compared to the > "status quo" that I was not yet aware of?), nor knowing what NIH is > supposed to mean in this context. "open coding" means to have a copy of the function of macro that is already implemented and available (even if it's private to some driver or module, we always can move it to the generic module and header). NIH: Not Invented Here. > We're in bike-shedding territory > anyway, guess I should just bookmark the page that details all the many > `%` format strings available. True :-) ... > > > I find the latter clearer as it doesn't require the reader to figure out > > > that name - name cancels itself out. Alternatively we can write > > > strchrnul(name, '@')[0]. > > > > I don't like to have Pythonisms in the C code, really. > > > > P.S. I guess this little patch already emptied my bandwidth, so I leave > > any further discussion to you and IIO maintainers. Thank you for the > > review! > > Just soaking up kernel coding standards here :) Right. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-18 10:06 [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name Andy Shevchenko 2023-01-18 12:35 ` Marijn Suijten @ 2023-01-22 17:24 ` Jonathan Cameron 2023-01-23 12:02 ` Andy Shevchenko 2023-01-23 16:35 ` Konrad Dybcio 1 sibling, 2 replies; 16+ messages in thread From: Jonathan Cameron @ 2023-01-22 17:24 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio, Lars-Peter Clausen, Marijn Suijten, Dmitry Baryshkov On Wed, 18 Jan 2023 12:06:23 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > The node name can contain an address part which is unused > by the driver. Moreover, this string is propagated into > the userspace label, sysfs filenames *and breaking ABI*. > > Cut the address part out before assigning the channel name. > > Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") > Reported-by: Marijn Suijten <marijn.suijten@somainline.org> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> LGTM, but given it will have ABI impact, I'd like to hear from Andy, Bjorn or Konrad as maintainers and /or Dmitry as someone who has touched this driver fairly recently. Mostly I want to be sure they know this exists before it causes surprise. Jonathan > --- > v2: rephrased commit message (Marijn), fixed compilation issue (Marijin) > drivers/iio/adc/qcom-spmi-adc5.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c > index e90c299c913a..c2d5e06f137a 100644 > --- a/drivers/iio/adc/qcom-spmi-adc5.c > +++ b/drivers/iio/adc/qcom-spmi-adc5.c > @@ -628,12 +628,20 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc, > struct fwnode_handle *fwnode, > const struct adc5_data *data) > { > - const char *name = fwnode_get_name(fwnode), *channel_name; > + const char *channel_name; > + char *name; > u32 chan, value, varr[2]; > u32 sid = 0; > int ret; > struct device *dev = adc->dev; > > + name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode); > + if (!name) > + return -ENOMEM; > + > + /* Cut the address part */ > + name[strchrnul(name, '@') - name] = '\0'; > + > ret = fwnode_property_read_u32(fwnode, "reg", &chan); > if (ret) { > dev_err(dev, "invalid channel number %s\n", name); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-22 17:24 ` Jonathan Cameron @ 2023-01-23 12:02 ` Andy Shevchenko 2023-01-23 19:30 ` Jonathan Cameron 2023-01-23 16:35 ` Konrad Dybcio 1 sibling, 1 reply; 16+ messages in thread From: Andy Shevchenko @ 2023-01-23 12:02 UTC (permalink / raw) To: Jonathan Cameron Cc: Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio, Lars-Peter Clausen, Marijn Suijten, Dmitry Baryshkov On Sun, Jan 22, 2023 at 05:24:41PM +0000, Jonathan Cameron wrote: > On Wed, 18 Jan 2023 12:06:23 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > The node name can contain an address part which is unused > > by the driver. Moreover, this string is propagated into > > the userspace label, sysfs filenames *and breaking ABI*. > > > > Cut the address part out before assigning the channel name. > > > > Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") > > Reported-by: Marijn Suijten <marijn.suijten@somainline.org> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > LGTM, but given it will have ABI impact, I'd like to hear from > Andy, Bjorn or Konrad as maintainers and /or Dmitry as someone > who has touched this driver fairly recently. Hmm... But this is to fix the ABI breakage. It means that the previous series by Nuno had broken it. > Mostly I want to be sure they know this exists before it causes surprise. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-23 12:02 ` Andy Shevchenko @ 2023-01-23 19:30 ` Jonathan Cameron 0 siblings, 0 replies; 16+ messages in thread From: Jonathan Cameron @ 2023-01-23 19:30 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio, Lars-Peter Clausen, Marijn Suijten, Dmitry Baryshkov On Mon, 23 Jan 2023 14:02:51 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sun, Jan 22, 2023 at 05:24:41PM +0000, Jonathan Cameron wrote: > > On Wed, 18 Jan 2023 12:06:23 +0200 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > The node name can contain an address part which is unused > > > by the driver. Moreover, this string is propagated into > > > the userspace label, sysfs filenames *and breaking ABI*. > > > > > > Cut the address part out before assigning the channel name. > > > > > > Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") > > > Reported-by: Marijn Suijten <marijn.suijten@somainline.org> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > LGTM, but given it will have ABI impact, I'd like to hear from > > Andy, Bjorn or Konrad as maintainers and /or Dmitry as someone > > who has touched this driver fairly recently. > > Hmm... But this is to fix the ABI breakage. It means that the previous series > by Nuno had broken it. Absolutely agree. I plan to take the change. The risk that someone needs to fix up their use of the broken ABI makes it worth a little more shouting about than if we had caught it sooner. Jonathan > > > Mostly I want to be sure they know this exists before it causes surprise. > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-22 17:24 ` Jonathan Cameron 2023-01-23 12:02 ` Andy Shevchenko @ 2023-01-23 16:35 ` Konrad Dybcio 2023-01-23 17:01 ` Marijn Suijten 2023-01-23 23:12 ` Doug Anderson 1 sibling, 2 replies; 16+ messages in thread From: Konrad Dybcio @ 2023-01-23 16:35 UTC (permalink / raw) To: Jonathan Cameron, Andy Shevchenko Cc: Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Lars-Peter Clausen, Marijn Suijten, Dmitry Baryshkov, Douglas Anderson On 22.01.2023 18:24, Jonathan Cameron wrote: > On Wed, 18 Jan 2023 12:06:23 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> The node name can contain an address part which is unused >> by the driver. Moreover, this string is propagated into >> the userspace label, sysfs filenames *and breaking ABI*. >> >> Cut the address part out before assigning the channel name. >> >> Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") >> Reported-by: Marijn Suijten <marijn.suijten@somainline.org> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > LGTM, but given it will have ABI impact, I'd like to hear from > Andy, Bjorn or Konrad as maintainers and /or Dmitry as someone > who has touched this driver fairly recently. + Doug Unless the Chromium folks relied on the old names (they're the only ones I can think of that actually could have tapped into this), I say green light! Konrad > > Mostly I want to be sure they know this exists before it causes surprise. > > Jonathan > >> --- >> v2: rephrased commit message (Marijn), fixed compilation issue (Marijin) >> drivers/iio/adc/qcom-spmi-adc5.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c >> index e90c299c913a..c2d5e06f137a 100644 >> --- a/drivers/iio/adc/qcom-spmi-adc5.c >> +++ b/drivers/iio/adc/qcom-spmi-adc5.c >> @@ -628,12 +628,20 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc, >> struct fwnode_handle *fwnode, >> const struct adc5_data *data) >> { >> - const char *name = fwnode_get_name(fwnode), *channel_name; >> + const char *channel_name; >> + char *name; >> u32 chan, value, varr[2]; >> u32 sid = 0; >> int ret; >> struct device *dev = adc->dev; >> >> + name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode); >> + if (!name) >> + return -ENOMEM; >> + >> + /* Cut the address part */ >> + name[strchrnul(name, '@') - name] = '\0'; >> + >> ret = fwnode_property_read_u32(fwnode, "reg", &chan); >> if (ret) { >> dev_err(dev, "invalid channel number %s\n", name); > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-23 16:35 ` Konrad Dybcio @ 2023-01-23 17:01 ` Marijn Suijten 2023-01-23 17:04 ` Konrad Dybcio 2023-01-23 23:12 ` Doug Anderson 1 sibling, 1 reply; 16+ messages in thread From: Marijn Suijten @ 2023-01-23 17:01 UTC (permalink / raw) To: Konrad Dybcio Cc: Jonathan Cameron, Andy Shevchenko, Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Lars-Peter Clausen, Dmitry Baryshkov, Douglas Anderson On 2023-01-23 17:35:34, Konrad Dybcio wrote: > On 22.01.2023 18:24, Jonathan Cameron wrote: > > On Wed, 18 Jan 2023 12:06:23 +0200 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > >> The node name can contain an address part which is unused > >> by the driver. Moreover, this string is propagated into > >> the userspace label, sysfs filenames *and breaking ABI*. > >> > >> Cut the address part out before assigning the channel name. > >> > >> Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") > >> Reported-by: Marijn Suijten <marijn.suijten@somainline.org> > >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > LGTM, but given it will have ABI impact, I'd like to hear from > > Andy, Bjorn or Konrad as maintainers and /or Dmitry as someone > > who has touched this driver fairly recently. > + Doug > > Unless the Chromium folks relied on the old names (they're the > only ones I can think of that actually could have tapped into > this), I say green light! Can you clarify "old names"? The ABI-broken ones after Nuno's patch with @xx suffix, or the orignal names (without @xx) before that, which this patch from Andy reverts back to? - Marijn ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-23 17:01 ` Marijn Suijten @ 2023-01-23 17:04 ` Konrad Dybcio 0 siblings, 0 replies; 16+ messages in thread From: Konrad Dybcio @ 2023-01-23 17:04 UTC (permalink / raw) To: Marijn Suijten Cc: Jonathan Cameron, Andy Shevchenko, Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Lars-Peter Clausen, Dmitry Baryshkov, Douglas Anderson On 23.01.2023 18:01, Marijn Suijten wrote: > On 2023-01-23 17:35:34, Konrad Dybcio wrote: >> On 22.01.2023 18:24, Jonathan Cameron wrote: >>> On Wed, 18 Jan 2023 12:06:23 +0200 >>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >>> >>>> The node name can contain an address part which is unused >>>> by the driver. Moreover, this string is propagated into >>>> the userspace label, sysfs filenames *and breaking ABI*. >>>> >>>> Cut the address part out before assigning the channel name. >>>> >>>> Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") >>>> Reported-by: Marijn Suijten <marijn.suijten@somainline.org> >>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>> >>> LGTM, but given it will have ABI impact, I'd like to hear from >>> Andy, Bjorn or Konrad as maintainers and /or Dmitry as someone >>> who has touched this driver fairly recently. >> + Doug >> >> Unless the Chromium folks relied on the old names (they're the >> only ones I can think of that actually could have tapped into >> this), I say green light! > > Can you clarify "old names"? The ABI-broken ones after Nuno's patch > with @xx suffix, or the orignal names (without @xx) before that, which > this patch from Andy reverts back to? Nuno's patch names, this is a fix for that but in a very unfortunate event, they might have started using sysfs paths right when the breakage happened. Konrad > > - Marijn ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-23 16:35 ` Konrad Dybcio 2023-01-23 17:01 ` Marijn Suijten @ 2023-01-23 23:12 ` Doug Anderson 2023-01-25 19:39 ` Matthias Kaehlcke 1 sibling, 1 reply; 16+ messages in thread From: Doug Anderson @ 2023-01-23 23:12 UTC (permalink / raw) To: Konrad Dybcio Cc: Jonathan Cameron, Andy Shevchenko, Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Lars-Peter Clausen, Marijn Suijten, Dmitry Baryshkov, Matthias Kaehlcke Hi, On Mon, Jan 23, 2023 at 8:35 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On 22.01.2023 18:24, Jonathan Cameron wrote: > > On Wed, 18 Jan 2023 12:06:23 +0200 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > >> The node name can contain an address part which is unused > >> by the driver. Moreover, this string is propagated into > >> the userspace label, sysfs filenames *and breaking ABI*. > >> > >> Cut the address part out before assigning the channel name. > >> > >> Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") > >> Reported-by: Marijn Suijten <marijn.suijten@somainline.org> > >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > LGTM, but given it will have ABI impact, I'd like to hear from > > Andy, Bjorn or Konrad as maintainers and /or Dmitry as someone > > who has touched this driver fairly recently. > + Doug > > Unless the Chromium folks relied on the old names (they're the > only ones I can think of that actually could have tapped into > this), I say green light! Thanks for the CC. I _think_ the only place we use these ADCs is for certain thermistors and I think that those are all just hooked up in the device tree, so the channel name doesn't matter. I'll also note that no Qualcomm Chromebooks are shipping with anything newer than kernel 5.15 right now, and (I checked) the ChromeOS 5.15 tree doesn't have commit 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties"). Thus, even if I'm wrong and the name is used someplace hidden then the "old" name would be better for us. I haven't tested the patch myself, but it sounds as if ${SUBJECT} patch is actually moving us back to the old name. +Matthias to keep me honest since he's spent more time with the ADCs. > Konrad > > > > Mostly I want to be sure they know this exists before it causes surprise. > > > > Jonathan > > > >> --- > >> v2: rephrased commit message (Marijn), fixed compilation issue (Marijin) > >> drivers/iio/adc/qcom-spmi-adc5.c | 10 +++++++++- > >> 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c > >> index e90c299c913a..c2d5e06f137a 100644 > >> --- a/drivers/iio/adc/qcom-spmi-adc5.c > >> +++ b/drivers/iio/adc/qcom-spmi-adc5.c > >> @@ -628,12 +628,20 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc, > >> struct fwnode_handle *fwnode, > >> const struct adc5_data *data) > >> { > >> - const char *name = fwnode_get_name(fwnode), *channel_name; > >> + const char *channel_name; > >> + char *name; > >> u32 chan, value, varr[2]; > >> u32 sid = 0; > >> int ret; > >> struct device *dev = adc->dev; > >> > >> + name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode); > >> + if (!name) > >> + return -ENOMEM; > >> + > >> + /* Cut the address part */ > >> + name[strchrnul(name, '@') - name] = '\0'; > >> + > >> ret = fwnode_property_read_u32(fwnode, "reg", &chan); > >> if (ret) { > >> dev_err(dev, "invalid channel number %s\n", name); > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-23 23:12 ` Doug Anderson @ 2023-01-25 19:39 ` Matthias Kaehlcke 2023-01-28 15:34 ` Jonathan Cameron 0 siblings, 1 reply; 16+ messages in thread From: Matthias Kaehlcke @ 2023-01-25 19:39 UTC (permalink / raw) To: Doug Anderson Cc: Konrad Dybcio, Jonathan Cameron, Andy Shevchenko, Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Lars-Peter Clausen, Marijn Suijten, Dmitry Baryshkov On Mon, Jan 23, 2023 at 03:12:06PM -0800, Doug Anderson wrote: > Hi, > > On Mon, Jan 23, 2023 at 8:35 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 22.01.2023 18:24, Jonathan Cameron wrote: > > > On Wed, 18 Jan 2023 12:06:23 +0200 > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > >> The node name can contain an address part which is unused > > >> by the driver. Moreover, this string is propagated into > > >> the userspace label, sysfs filenames *and breaking ABI*. > > >> > > >> Cut the address part out before assigning the channel name. > > >> > > >> Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") > > >> Reported-by: Marijn Suijten <marijn.suijten@somainline.org> > > >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > LGTM, but given it will have ABI impact, I'd like to hear from > > > Andy, Bjorn or Konrad as maintainers and /or Dmitry as someone > > > who has touched this driver fairly recently. > > + Doug > > > > Unless the Chromium folks relied on the old names (they're the > > only ones I can think of that actually could have tapped into > > this), I say green light! > > Thanks for the CC. I _think_ the only place we use these ADCs is for > certain thermistors and I think that those are all just hooked up in > the device tree, so the channel name doesn't matter. I'll also note > that no Qualcomm Chromebooks are shipping with anything newer than > kernel 5.15 right now, and (I checked) the ChromeOS 5.15 tree doesn't > have commit 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device > properties"). Thus, even if I'm wrong and the name is used someplace > hidden then the "old" name would be better for us. I haven't tested > the patch myself, but it sounds as if ${SUBJECT} patch is actually > moving us back to the old name. > > +Matthias to keep me honest since he's spent more time with the ADCs. Agreed that the channel name doesn't matter, Chrome OS currently only uses the ADCs for thermal zones controlled by the kernel. > > > Mostly I want to be sure they know this exists before it causes surprise. > > > > > > Jonathan > > > > > >> --- > > >> v2: rephrased commit message (Marijn), fixed compilation issue (Marijin) > > >> drivers/iio/adc/qcom-spmi-adc5.c | 10 +++++++++- > > >> 1 file changed, 9 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c > > >> index e90c299c913a..c2d5e06f137a 100644 > > >> --- a/drivers/iio/adc/qcom-spmi-adc5.c > > >> +++ b/drivers/iio/adc/qcom-spmi-adc5.c > > >> @@ -628,12 +628,20 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc, > > >> struct fwnode_handle *fwnode, > > >> const struct adc5_data *data) > > >> { > > >> - const char *name = fwnode_get_name(fwnode), *channel_name; > > >> + const char *channel_name; > > >> + char *name; > > >> u32 chan, value, varr[2]; > > >> u32 sid = 0; > > >> int ret; > > >> struct device *dev = adc->dev; > > >> > > >> + name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode); > > >> + if (!name) > > >> + return -ENOMEM; > > >> + > > >> + /* Cut the address part */ > > >> + name[strchrnul(name, '@') - name] = '\0'; > > >> + > > >> ret = fwnode_property_read_u32(fwnode, "reg", &chan); > > >> if (ret) { > > >> dev_err(dev, "invalid channel number %s\n", name); > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name 2023-01-25 19:39 ` Matthias Kaehlcke @ 2023-01-28 15:34 ` Jonathan Cameron 0 siblings, 0 replies; 16+ messages in thread From: Jonathan Cameron @ 2023-01-28 15:34 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Doug Anderson, Konrad Dybcio, Andy Shevchenko, Jonathan Cameron, Nuno Sá, linux-arm-msm, linux-iio, linux-kernel, Andy Gross, Bjorn Andersson, Lars-Peter Clausen, Marijn Suijten, Dmitry Baryshkov On Wed, 25 Jan 2023 19:39:46 +0000 Matthias Kaehlcke <mka@chromium.org> wrote: > On Mon, Jan 23, 2023 at 03:12:06PM -0800, Doug Anderson wrote: > > Hi, > > > > On Mon, Jan 23, 2023 at 8:35 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > > > On 22.01.2023 18:24, Jonathan Cameron wrote: > > > > On Wed, 18 Jan 2023 12:06:23 +0200 > > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > >> The node name can contain an address part which is unused > > > >> by the driver. Moreover, this string is propagated into > > > >> the userspace label, sysfs filenames *and breaking ABI*. > > > >> > > > >> Cut the address part out before assigning the channel name. > > > >> > > > >> Fixes: 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties") > > > >> Reported-by: Marijn Suijten <marijn.suijten@somainline.org> > > > >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > > > LGTM, but given it will have ABI impact, I'd like to hear from > > > > Andy, Bjorn or Konrad as maintainers and /or Dmitry as someone > > > > who has touched this driver fairly recently. > > > + Doug > > > > > > Unless the Chromium folks relied on the old names (they're the > > > only ones I can think of that actually could have tapped into > > > this), I say green light! > > > > Thanks for the CC. I _think_ the only place we use these ADCs is for > > certain thermistors and I think that those are all just hooked up in > > the device tree, so the channel name doesn't matter. I'll also note > > that no Qualcomm Chromebooks are shipping with anything newer than > > kernel 5.15 right now, and (I checked) the ChromeOS 5.15 tree doesn't > > have commit 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device > > properties"). Thus, even if I'm wrong and the name is used someplace > > hidden then the "old" name would be better for us. I haven't tested > > the patch myself, but it sounds as if ${SUBJECT} patch is actually > > moving us back to the old name. > > > > +Matthias to keep me honest since he's spent more time with the ADCs. > > Agreed that the channel name doesn't matter, Chrome OS currently only > uses the ADCs for thermal zones controlled by the kernel. Great. Thanks for all the info. Applied to the fixes-togreg branch of iio.git and marked for stable. Jonathan > > > > > Mostly I want to be sure they know this exists before it causes surprise. > > > > > > > > Jonathan > > > > > > > >> --- > > > >> v2: rephrased commit message (Marijn), fixed compilation issue (Marijin) > > > >> drivers/iio/adc/qcom-spmi-adc5.c | 10 +++++++++- > > > >> 1 file changed, 9 insertions(+), 1 deletion(-) > > > >> > > > >> diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c > > > >> index e90c299c913a..c2d5e06f137a 100644 > > > >> --- a/drivers/iio/adc/qcom-spmi-adc5.c > > > >> +++ b/drivers/iio/adc/qcom-spmi-adc5.c > > > >> @@ -628,12 +628,20 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc, > > > >> struct fwnode_handle *fwnode, > > > >> const struct adc5_data *data) > > > >> { > > > >> - const char *name = fwnode_get_name(fwnode), *channel_name; > > > >> + const char *channel_name; > > > >> + char *name; > > > >> u32 chan, value, varr[2]; > > > >> u32 sid = 0; > > > >> int ret; > > > >> struct device *dev = adc->dev; > > > >> > > > >> + name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode); > > > >> + if (!name) > > > >> + return -ENOMEM; > > > >> + > > > >> + /* Cut the address part */ > > > >> + name[strchrnul(name, '@') - name] = '\0'; > > > >> + > > > >> ret = fwnode_property_read_u32(fwnode, "reg", &chan); > > > >> if (ret) { > > > >> dev_err(dev, "invalid channel number %s\n", name); > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-01-28 15:21 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-18 10:06 [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name Andy Shevchenko 2023-01-18 12:35 ` Marijn Suijten 2023-01-18 13:22 ` Andy Shevchenko 2023-01-18 14:04 ` Marijn Suijten 2023-01-18 14:29 ` Andy Shevchenko 2023-01-18 15:21 ` Marijn Suijten 2023-01-18 15:57 ` Andy Shevchenko 2023-01-22 17:24 ` Jonathan Cameron 2023-01-23 12:02 ` Andy Shevchenko 2023-01-23 19:30 ` Jonathan Cameron 2023-01-23 16:35 ` Konrad Dybcio 2023-01-23 17:01 ` Marijn Suijten 2023-01-23 17:04 ` Konrad Dybcio 2023-01-23 23:12 ` Doug Anderson 2023-01-25 19:39 ` Matthias Kaehlcke 2023-01-28 15:34 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).