| Message ID | 20230327083449.1098174-1-sean@geanix.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/3] iio: adc: stm32-adc: warn if dt uses legacy channel config | expand |
On 3/27/23 10:34, Sean Nyekjaer wrote: > Since nearly all stm32 dt's are using the legacy adc channel config, > we should warn users about using it. > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > --- > drivers/iio/adc/stm32-adc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c > index 1aadb2ad2cab..d8e755d0cc52 100644 > --- a/drivers/iio/adc/stm32-adc.c > +++ b/drivers/iio/adc/stm32-adc.c > @@ -1993,6 +1993,8 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm > const struct stm32_adc_info *adc_info = adc->cfg->adc_info; > int num_channels = 0, ret; > > + dev_warn(&indio_dev->dev, "using legacy channel config\n"); > + Hi Sean, I'd recommend to avoid adding a dev_warn() on a per driver basis, for depreacted DT properties. Still I'm not sure if there's some policy in this area. IMHO, deprecated properties should be checked by using dt tools (dt_binding_check / dtbs_check or other mean?). But I have no idea if this is going to report warnings and when. Purpose would be to avoid introducing no dts files with these. As commented by Olivier on Patch 3, we've some downstream patches to adopt the generic bindings (not upstream 'yet'). Another downside is regarding backward compatibility. In case an old dtb is used to boot the kernel, as long as there's no functionality loss, I'd advise not to use any warning here. That's a valid use of an old dt. In all case, thanks for pointing issues (e.g. Patch 2 & 3), regarding this patch, I'd nack adding this warning. Please drop this change if you re-submit or turn this into a dev_dbg(). Best Regards, Fabrice > ret = device_property_count_u32(dev, "st,adc-channels"); > if (ret > adc_info->max_channels) { > dev_err(&indio_dev->dev, "Bad st,adc-channels?\n");
On Thu, 30 Mar 2023 17:30:32 +0200 Fabrice Gasnier <fabrice.gasnier@foss.st.com> wrote: > On 3/27/23 10:34, Sean Nyekjaer wrote: > > Since nearly all stm32 dt's are using the legacy adc channel config, > > we should warn users about using it. > > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > > --- > > drivers/iio/adc/stm32-adc.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c > > index 1aadb2ad2cab..d8e755d0cc52 100644 > > --- a/drivers/iio/adc/stm32-adc.c > > +++ b/drivers/iio/adc/stm32-adc.c > > @@ -1993,6 +1993,8 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm > > const struct stm32_adc_info *adc_info = adc->cfg->adc_info; > > int num_channels = 0, ret; > > > > + dev_warn(&indio_dev->dev, "using legacy channel config\n"); > > + > > Hi Sean, > > I'd recommend to avoid adding a dev_warn() on a per driver basis, for > depreacted DT properties. Still I'm not sure if there's some policy in > this area. > > IMHO, deprecated properties should be checked by using dt tools > (dt_binding_check / dtbs_check or other mean?). But I have no idea if > this is going to report warnings and when. Purpose would be to avoid > introducing no dts files with these. As commented by Olivier on Patch 3, > we've some downstream patches to adopt the generic bindings (not > upstream 'yet'). > > Another downside is regarding backward compatibility. In case an old dtb > is used to boot the kernel, as long as there's no functionality loss, > I'd advise not to use any warning here. That's a valid use of an old dt. > > In all case, thanks for pointing issues (e.g. Patch 2 & 3), regarding > this patch, I'd nack adding this warning. Please drop this change if you > re-submit or turn this into a dev_dbg(). > Agreed. Better to change to dev_dbg(). Other two patches look good to me, but will leave a bit more time for others to comment before I pick them up. As a small side note. They are fixes and this first patch is not, so they should have been before it in the series so I can route them to mainline faster than the non fix. Jonathan > Best Regards, > Fabrice > > > ret = device_property_count_u32(dev, "st,adc-channels"); > > if (ret > adc_info->max_channels) { > > dev_err(&indio_dev->dev, "Bad st,adc-channels?\n");
> On 1 Apr 2023, at 16.04, Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 30 Mar 2023 17:30:32 +0200 > Fabrice Gasnier <fabrice.gasnier@foss.st.com> wrote: > >> On 3/27/23 10:34, Sean Nyekjaer wrote: >>> Since nearly all stm32 dt's are using the legacy adc channel config, >>> we should warn users about using it. >>> >>> Signed-off-by: Sean Nyekjaer <sean@geanix.com> >>> --- >>> drivers/iio/adc/stm32-adc.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c >>> index 1aadb2ad2cab..d8e755d0cc52 100644 >>> --- a/drivers/iio/adc/stm32-adc.c >>> +++ b/drivers/iio/adc/stm32-adc.c >>> @@ -1993,6 +1993,8 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm >>> const struct stm32_adc_info *adc_info = adc->cfg->adc_info; >>> int num_channels = 0, ret; >>> >>> + dev_warn(&indio_dev->dev, "using legacy channel config\n"); >>> + >> >> Hi Sean, >> >> I'd recommend to avoid adding a dev_warn() on a per driver basis, for >> depreacted DT properties. Still I'm not sure if there's some policy in >> this area. >> >> IMHO, deprecated properties should be checked by using dt tools >> (dt_binding_check / dtbs_check or other mean?). But I have no idea if >> this is going to report warnings and when. Purpose would be to avoid >> introducing no dts files with these. As commented by Olivier on Patch 3, >> we've some downstream patches to adopt the generic bindings (not >> upstream 'yet'). >> >> Another downside is regarding backward compatibility. In case an old dtb >> is used to boot the kernel, as long as there's no functionality loss, >> I'd advise not to use any warning here. That's a valid use of an old dt. >> >> In all case, thanks for pointing issues (e.g. Patch 2 & 3), regarding >> this patch, I'd nack adding this warning. Please drop this change if you >> re-submit or turn this into a dev_dbg(). >> > > Agreed. Better to change to dev_dbg(). > > Other two patches look good to me, but will leave a bit more time for others > to comment before I pick them up. As a small side note. They are fixes and > this first patch is not, so they should have been before it in the series > so I can route them to mainline faster than the non fix. > > Jonathan Hi Jonathan, I’ll resubmit the first patch with dev_dbg() as a single commit, and then the fixes as a separate series :) /Sean > >> Best Regards, >> Fabrice >> >>> ret = device_property_count_u32(dev, "st,adc-channels"); >>> if (ret > adc_info->max_channels) { >>> dev_err(&indio_dev->dev, "Bad st,adc-channels?\n");
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c index 1aadb2ad2cab..d8e755d0cc52 100644 --- a/drivers/iio/adc/stm32-adc.c +++ b/drivers/iio/adc/stm32-adc.c @@ -1993,6 +1993,8 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm const struct stm32_adc_info *adc_info = adc->cfg->adc_info; int num_channels = 0, ret; + dev_warn(&indio_dev->dev, "using legacy channel config\n"); + ret = device_property_count_u32(dev, "st,adc-channels"); if (ret > adc_info->max_channels) { dev_err(&indio_dev->dev, "Bad st,adc-channels?\n");
Since nearly all stm32 dt's are using the legacy adc channel config, we should warn users about using it. Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- drivers/iio/adc/stm32-adc.c | 2 ++ 1 file changed, 2 insertions(+)