Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1571349 > unrolled thread
| Started by | Peter Meerwald-Stadler <pmeerw@pmeerw.net> |
|---|---|
| First post | 2017-02-01 10:20 +0100 |
| Last post | 2017-02-01 19:30 +0100 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering Peter Meerwald-Stadler <pmeerw@pmeerw.net> - 2017-02-01 10:20 +0100
Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering Peter Meerwald-Stadler <pmeerw@pmeerw.net> - 2017-02-01 10:40 +0100
Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering Peter Meerwald-Stadler <pmeerw@pmeerw.net> - 2017-02-01 11:00 +0100
RE: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering "Ken.Lin" <ken.lin@advantech.com> - 2017-02-01 19:30 +0100
| From | Peter Meerwald-Stadler <pmeerw@pmeerw.net> |
|---|---|
| Date | 2017-02-01 10:20 +0100 |
| Subject | Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering |
| Message-ID | <t5Zf4-41c-17@gated-at.bofh.it> |
Hello,
> Fixes a regression triggered by a change in the layout of
> struct iio_chan_spec, but the real bug is in the driver which assumed
> a specific structure layout in the first place.
I don't think that this is a proper fix
maybe the driver is unique in that it uses mask_separate for INFO_SCALE
and not by_type, but since there is just one PRESSURE channel, it should
be equivalent
what do you mean by 'driver which assumed a specific structure'?
thanks, p.
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index cc3f84139157..525644a7442d 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -190,7 +190,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> {
> .type = IIO_PRESSURE,
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> - BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> .scan_index = 0,
> .scan_type = {
> .sign = 'u',
> @@ -203,7 +203,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> {
> .type = IIO_TEMP,
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> - BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> .scan_index = 1,
> .scan_type = {
> .sign = 's',
>
--
Peter Meerwald-Stadler
+43-664-2444418 (mobile)
[toc] | [next] | [standalone]
| From | Peter Meerwald-Stadler <pmeerw@pmeerw.net> |
|---|---|
| Date | 2017-02-01 10:40 +0100 |
| Message-ID | <t5Zyq-4a5-17@gated-at.bofh.it> |
| In reply to | #1571349 |
> >> Fixes a regression triggered by a change in the layout of
> >> struct iio_chan_spec, but the real bug is in the driver which assumed
> >> a specific structure layout in the first place.
> > what do you mean by 'driver which assumed a specific structure'?
>
> Look again, the two bits are not OR:ed together as implied by the
> indentation. There is a comma between them, which put the ..._SCALE
> bit in the next field. That next field was .info_mask_shared_by_type
> before the patch adding the available attribute that triggered the
> regression and .info_mask_separate_available after it.
wow, now that you say it :)
since I wrote these drivers I can assure you that this is just a typo, use
of the 'specific structure' is a coincident
maybe adding your explanation regarding not ORing to the patch description
would be a good idea
I was confused by the change from
.info_mask_separate to .info_mask_shared_by_type
a fix could have just changed
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
as originally intended
> Cheers,
> peda
>
> > thanks, p.
> >
> >> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> >> index cc3f84139157..525644a7442d 100644
> >> --- a/drivers/iio/pressure/mpl3115.c
> >> +++ b/drivers/iio/pressure/mpl3115.c
> >> @@ -190,7 +190,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> >> {
> >> .type = IIO_PRESSURE,
> >> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> - BIT(IIO_CHAN_INFO_SCALE),
> >> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >> .scan_index = 0,
> >> .scan_type = {
> >> .sign = 'u',
> >> @@ -203,7 +203,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> >> {
> >> .type = IIO_TEMP,
> >> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> - BIT(IIO_CHAN_INFO_SCALE),
> >> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >> .scan_index = 1,
> >> .scan_type = {
> >> .sign = 's',
> >>
> >
>
--
Peter Meerwald-Stadler
+43-664-2444418 (mobile)
[toc] | [prev] | [next] | [standalone]
| From | Peter Meerwald-Stadler <pmeerw@pmeerw.net> |
|---|---|
| Date | 2017-02-01 11:00 +0100 |
| Message-ID | <t5ZRN-4h5-49@gated-at.bofh.it> |
| In reply to | #1571368 |
> > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > BIT(IIO_CHAN_INFO_SCALE),
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > BIT(IIO_CHAN_INFO_SCALE),
> > as originally intended
>
> I considered that option, but the code in mpl3115_read_raw (and
> mpl115_read_raw for that matter) return constants fro these values which
> to me indicated that they were not "separate" and as that would also be
> the change which replicated the exact behavior from before the regression
> I went with that. But I don't care either way, so I can re-spin if you
> want me to? (But don't blame me if that regresses in some other
> interesting way).
no, all good; shared_by_type is the way to go
I'd rather respin for the not ORed comment in the patch
> >> Cheers,
> >> peda
> >>
> >>> thanks, p.
> >>>
> >>>> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> >>>> index cc3f84139157..525644a7442d 100644
> >>>> --- a/drivers/iio/pressure/mpl3115.c
> >>>> +++ b/drivers/iio/pressure/mpl3115.c
> >>>> @@ -190,7 +190,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> >>>> {
> >>>> .type = IIO_PRESSURE,
> >>>> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >>>> - BIT(IIO_CHAN_INFO_SCALE),
> >>>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >>>> .scan_index = 0,
> >>>> .scan_type = {
> >>>> .sign = 'u',
> >>>> @@ -203,7 +203,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> >>>> {
> >>>> .type = IIO_TEMP,
> >>>> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >>>> - BIT(IIO_CHAN_INFO_SCALE),
> >>>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >>>> .scan_index = 1,
> >>>> .scan_type = {
> >>>> .sign = 's',
> >>>>
> >>>
> >>
> >
>
--
Peter Meerwald-Stadler
+43-664-2444418 (mobile)
[toc] | [prev] | [next] | [standalone]
| From | "Ken.Lin" <ken.lin@advantech.com> |
|---|---|
| Date | 2017-02-01 19:30 +0100 |
| Message-ID | <t67Pj-W0-11@gated-at.bofh.it> |
| In reply to | #1571413 |
> -----Original Message----- > From: Peter Rosin [mailto:peda@axentia.se] > Sent: Wednesday, February 1, 2017 2:18 AM > To: Peter Meerwald-Stadler > Cc: linux-kernel@vger.kernel.org; Jonathan Cameron; Hartmut Knaack; Lars- > Peter Clausen; Alison Schofield; Gregor Boirie; Sanchayan Maity; Ken.Lin; linux- > iio@vger.kernel.org > Subject: Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field > ordering > > On 2017-02-01 10:57, Peter Meerwald-Stadler wrote: > >>> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > >>> BIT(IIO_CHAN_INFO_SCALE), > >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > >>> BIT(IIO_CHAN_INFO_SCALE), > >>> as originally intended > >> > >> I considered that option, but the code in mpl3115_read_raw (and > >> mpl115_read_raw for that matter) return constants fro these values > >> which to me indicated that they were not "separate" and as that would > >> also be the change which replicated the exact behavior from before > >> the regression I went with that. But I don't care either way, so I > >> can re-spin if you want me to? (But don't blame me if that regresses > >> in some other interesting way). > > > > no, all good; shared_by_type is the way to go I'd rather respin for > > the not ORed comment in the patch > > Ok, but I think I'll wait a bit so that Ken Lin gets some time to verify that it > actually solves the original problem. It should, but... > The patch test result looks good to me and functions well as before. # ls /sys/bus/iio/devices/iio\:device1/ buffer current_timestamp_clock dev in_pressure_raw in_pressure_scale in_temp_raw in_temp_scale name of_node power scan_elements subsystem trigger uevent # cat /sys/bus/iio/devices/iio\:device1/in_pressure_raw 403668 # cat /sys/bus/iio/devices/iio\:device1/in_pressure_scale 0.000250 # cat /sys/bus/iio/devices/iio\:device1/in_temp_scale 0.062500 # cat /sys/bus/iio/devices/iio\:device1/in_temp_raw 480 Thanks > Cheers, > peda > > > -- > This message has been scanned for viruses and dangerous content by > MailScanner, and is believed to be clean. -- This message has been scanned for viruses and dangerous content by MailScanner, and is believed to be clean.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web