| Message ID | 20190912144310.7458-3-andrea.merello@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for ad7949 | expand |
On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: > [External] > > This driver supports 14-bits and 16-bits devices. All of them have a 14-bit > configuration registers. All SPI trasfers, for reading AD conversion > results and for writing the configuration register, fit in two bytes. > > The driver always uses 4-bytes xfers which seems at least pointless (maybe > even harmful). This patch trims the SPI xfer len and the buffer size to > two bytes. > The length reduction proposal is fine. But, this patch raises a question about endianess. I'm actually wondering here if we need to see about maybe using a __be16 vs u16. I'm not that kernel-savy yet about some of these low-level things to be completely sure here. So, I'd let someone else maybe handle it. Thanks Alex > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > --- > drivers/iio/adc/ad7949.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > index 518044c31a73..5c2b3446fa4a 100644 > --- a/drivers/iio/adc/ad7949.c > +++ b/drivers/iio/adc/ad7949.c > @@ -54,7 +54,7 @@ struct ad7949_adc_chip { > u8 resolution; > u16 cfg; > unsigned int current_channel; > - u32 buffer ____cacheline_aligned; > + u16 buffer ____cacheline_aligned; > }; > > static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > struct spi_transfer tx[] = { > { > .tx_buf = &ad7949_adc->buffer, > - .len = 4, > + .len = 2, > .bits_per_word = bits_per_word, > }, > }; > @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, > struct spi_transfer tx[] = { > { > .rx_buf = &ad7949_adc->buffer, > - .len = 4, > + .len = 2, > .bits_per_word = bits_per_word, > }, > };
Il giorno ven 13 set 2019 alle ore 08:46 Ardelean, Alexandru <alexandru.Ardelean@analog.com> ha scritto: > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: > > [External] > > > > This driver supports 14-bits and 16-bits devices. All of them have a 14-bit > > configuration registers. All SPI trasfers, for reading AD conversion > > results and for writing the configuration register, fit in two bytes. > > > > The driver always uses 4-bytes xfers which seems at least pointless (maybe > > even harmful). This patch trims the SPI xfer len and the buffer size to > > two bytes. > > > > The length reduction proposal is fine. > > But, this patch raises a question about endianess. > I'm actually wondering here if we need to see about maybe using a __be16 vs u16. > > I'm not that kernel-savy yet about some of these low-level things to be completely sure here. > So, I'd let someone else maybe handle it. Good point.. It seems that indeed not much care has been taken about endianess here.. Probably we need also some le16_to_cpu() and firends.. Maybe another separate patch can be made to take care about endianess later on? BTW Also, the ____cacheline_aligned is a bit scaring :) I don't know what is that for... > > Thanks > Alex > > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > > --- > > drivers/iio/adc/ad7949.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > > index 518044c31a73..5c2b3446fa4a 100644 > > --- a/drivers/iio/adc/ad7949.c > > +++ b/drivers/iio/adc/ad7949.c > > @@ -54,7 +54,7 @@ struct ad7949_adc_chip { > > u8 resolution; > > u16 cfg; > > unsigned int current_channel; > > - u32 buffer ____cacheline_aligned; > > + u16 buffer ____cacheline_aligned; > > }; > > > > static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > > @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > > struct spi_transfer tx[] = { > > { > > .tx_buf = &ad7949_adc->buffer, > > - .len = 4, > > + .len = 2, > > .bits_per_word = bits_per_word, > > }, > > }; > > @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, > > struct spi_transfer tx[] = { > > { > > .rx_buf = &ad7949_adc->buffer, > > - .len = 4, > > + .len = 2, > > .bits_per_word = bits_per_word, > > }, > > };
On Fri, 2019-09-13 at 09:56 +0200, Andrea Merello wrote: > Il giorno ven 13 set 2019 alle ore 08:46 Ardelean, Alexandru > <alexandru.Ardelean@analog.com> ha scritto: > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: > > > [External] > > > > > > This driver supports 14-bits and 16-bits devices. All of them have a 14-bit > > > configuration registers. All SPI trasfers, for reading AD conversion > > > results and for writing the configuration register, fit in two bytes. > > > > > > The driver always uses 4-bytes xfers which seems at least pointless (maybe > > > even harmful). This patch trims the SPI xfer len and the buffer size to > > > two bytes. > > > > > > > The length reduction proposal is fine. > > > > But, this patch raises a question about endianess. > > I'm actually wondering here if we need to see about maybe using a __be16 vs u16. > > > > I'm not that kernel-savy yet about some of these low-level things to be completely sure here. > > So, I'd let someone else maybe handle it. > > Good point.. It seems that indeed not much care has been taken about > endianess here.. Probably we need also some le16_to_cpu() and > firends.. > > Maybe another separate patch can be made to take care about endianess later on? > Sure. Another patch makes sense. But, I am bit vague here about what to do. I would copy examples from other drivers about how things were done. There seem to be others that did this already somehow. > BTW Also, the ____cacheline_aligned is a bit scaring :) I don't know > what is that for... I'm still not completely clear on ___cacheline_aligned. Mostly because I did not find any issues that needed it. I just used it based on how others used it and all was fine. From older notes, it looks helpful with DMA and some memory allocation stuff. But this also depends on arch ; for some of them it's empty. > > > Thanks > > Alex > > > > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > > > --- > > > drivers/iio/adc/ad7949.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > > > index 518044c31a73..5c2b3446fa4a 100644 > > > --- a/drivers/iio/adc/ad7949.c > > > +++ b/drivers/iio/adc/ad7949.c > > > @@ -54,7 +54,7 @@ struct ad7949_adc_chip { > > > u8 resolution; > > > u16 cfg; > > > unsigned int current_channel; > > > - u32 buffer ____cacheline_aligned; > > > + u16 buffer ____cacheline_aligned; > > > }; > > > > > > static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > > > @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > > > struct spi_transfer tx[] = { > > > { > > > .tx_buf = &ad7949_adc->buffer, > > > - .len = 4, > > > + .len = 2, > > > .bits_per_word = bits_per_word, > > > }, > > > }; > > > @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, > > > struct spi_transfer tx[] = { > > > { > > > .rx_buf = &ad7949_adc->buffer, > > > - .len = 4, > > > + .len = 2, > > > .bits_per_word = bits_per_word, > > > }, > > > };
On Fri, 13 Sep 2019 09:56:56 +0200 Andrea Merello <andrea.merello@gmail.com> wrote: > Il giorno ven 13 set 2019 alle ore 08:46 Ardelean, Alexandru > <alexandru.Ardelean@analog.com> ha scritto: > > > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: > > > [External] > > > > > > This driver supports 14-bits and 16-bits devices. All of them have a 14-bit > > > configuration registers. All SPI trasfers, for reading AD conversion > > > results and for writing the configuration register, fit in two bytes. > > > > > > The driver always uses 4-bytes xfers which seems at least pointless (maybe > > > even harmful). This patch trims the SPI xfer len and the buffer size to > > > two bytes. > > > > > > > The length reduction proposal is fine. > > > > But, this patch raises a question about endianess. > > I'm actually wondering here if we need to see about maybe using a __be16 vs u16. > > > > I'm not that kernel-savy yet about some of these low-level things to be completely sure here. > > So, I'd let someone else maybe handle it. > > Good point.. It seems that indeed not much care has been taken about > endianess here.. Probably we need also some le16_to_cpu() and > firends.. More complexity here :) So a lot of earlier SPI drivers didn't set bits_per_word, the result of this is that a read had no way to know how to unwind the endian nature of the data. If you do a 4 byte read, is that 4x 1 byte, 2x 2 bytes or 1x 4 bytes. Thus the SPI subsystem had no way of knowing how to convert from wire order of big endian to cpu endianness. This is particularly fun as it is common to have variable length registers on SPI devices (be it described on the datasheet as some registers have high and low byte addresses). In drivers where this can be set to one consistent value, then the SPI subsystem should do the work for us. Hence this one should be fine. ( I think :) > > Maybe another separate patch can be made to take care about endianess later on? > > BTW Also, the ____cacheline_aligned is a bit scaring :) I don't know > what is that for... > > > > > Thanks > > Alex > > > > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > > > --- > > > drivers/iio/adc/ad7949.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > > > index 518044c31a73..5c2b3446fa4a 100644 > > > --- a/drivers/iio/adc/ad7949.c > > > +++ b/drivers/iio/adc/ad7949.c > > > @@ -54,7 +54,7 @@ struct ad7949_adc_chip { > > > u8 resolution; > > > u16 cfg; > > > unsigned int current_channel; > > > - u32 buffer ____cacheline_aligned; > > > + u16 buffer ____cacheline_aligned; > > > }; > > > > > > static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > > > @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > > > struct spi_transfer tx[] = { > > > { > > > .tx_buf = &ad7949_adc->buffer, > > > - .len = 4, > > > + .len = 2, > > > .bits_per_word = bits_per_word, > > > }, > > > }; > > > @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, > > > struct spi_transfer tx[] = { > > > { > > > .rx_buf = &ad7949_adc->buffer, > > > - .len = 4, > > > + .len = 2, > > > .bits_per_word = bits_per_word, > > > }, > > > };
On Sun, 2019-09-15 at 11:36 +0100, Jonathan Cameron wrote: > On Fri, 13 Sep 2019 09:56:56 +0200 > Andrea Merello <andrea.merello@gmail.com> wrote: > > > Il giorno ven 13 set 2019 alle ore 08:46 Ardelean, Alexandru > > <alexandru.Ardelean@analog.com> ha scritto: > > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: > > > > [External] > > > > > > > > This driver supports 14-bits and 16-bits devices. All of them have a 14-bit > > > > configuration registers. All SPI trasfers, for reading AD conversion > > > > results and for writing the configuration register, fit in two bytes. > > > > > > > > The driver always uses 4-bytes xfers which seems at least pointless (maybe > > > > even harmful). This patch trims the SPI xfer len and the buffer size to > > > > two bytes. > > > > > > > > > > The length reduction proposal is fine. > > > > > > But, this patch raises a question about endianess. > > > I'm actually wondering here if we need to see about maybe using a __be16 vs u16. > > > > > > I'm not that kernel-savy yet about some of these low-level things to be completely sure here. > > > So, I'd let someone else maybe handle it. > > > > Good point.. It seems that indeed not much care has been taken about > > endianess here.. Probably we need also some le16_to_cpu() and > > firends.. > > More complexity here :) So a lot of earlier SPI drivers didn't set bits_per_word, > the result of this is that a read had no way to know how to unwind the endian > nature of the data. If you do a 4 byte read, is that 4x 1 byte, 2x 2 bytes or > 1x 4 bytes. Thus the SPI subsystem had no way of knowing how to convert from > wire order of big endian to cpu endianness. This is particularly fun as it > is common to have variable length registers on SPI devices (be it described > on the datasheet as some registers have high and low byte addresses). > > In drivers where this can be set to one consistent value, then the SPI subsystem > should do the work for us. Hence this one should be fine. ( I think :) > Based on other input: Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > Maybe another separate patch can be made to take care about endianess later on? > > > > BTW Also, the ____cacheline_aligned is a bit scaring :) I don't know > > what is that for... > > > > > Thanks > > > Alex > > > > > > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > > > > --- > > > > drivers/iio/adc/ad7949.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > > > > index 518044c31a73..5c2b3446fa4a 100644 > > > > --- a/drivers/iio/adc/ad7949.c > > > > +++ b/drivers/iio/adc/ad7949.c > > > > @@ -54,7 +54,7 @@ struct ad7949_adc_chip { > > > > u8 resolution; > > > > u16 cfg; > > > > unsigned int current_channel; > > > > - u32 buffer ____cacheline_aligned; > > > > + u16 buffer ____cacheline_aligned; > > > > }; > > > > > > > > static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > > > > @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > > > > struct spi_transfer tx[] = { > > > > { > > > > .tx_buf = &ad7949_adc->buffer, > > > > - .len = 4, > > > > + .len = 2, > > > > .bits_per_word = bits_per_word, > > > > }, > > > > }; > > > > @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, > > > > struct spi_transfer tx[] = { > > > > { > > > > .rx_buf = &ad7949_adc->buffer, > > > > - .len = 4, > > > > + .len = 2, > > > > .bits_per_word = bits_per_word, > > > > }, > > > > };
On Mon, 16 Sep 2019 07:51:30 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Sun, 2019-09-15 at 11:36 +0100, Jonathan Cameron wrote: > > On Fri, 13 Sep 2019 09:56:56 +0200 > > Andrea Merello <andrea.merello@gmail.com> wrote: > > > > > Il giorno ven 13 set 2019 alle ore 08:46 Ardelean, Alexandru > > > <alexandru.Ardelean@analog.com> ha scritto: > > > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: > > > > > [External] > > > > > > > > > > This driver supports 14-bits and 16-bits devices. All of them have a 14-bit > > > > > configuration registers. All SPI trasfers, for reading AD conversion > > > > > results and for writing the configuration register, fit in two bytes. > > > > > > > > > > The driver always uses 4-bytes xfers which seems at least pointless (maybe > > > > > even harmful). This patch trims the SPI xfer len and the buffer size to > > > > > two bytes. > > > > > > > > > > > > > The length reduction proposal is fine. > > > > > > > > But, this patch raises a question about endianess. > > > > I'm actually wondering here if we need to see about maybe using a __be16 vs u16. > > > > > > > > I'm not that kernel-savy yet about some of these low-level things to be completely sure here. > > > > So, I'd let someone else maybe handle it. > > > > > > Good point.. It seems that indeed not much care has been taken about > > > endianess here.. Probably we need also some le16_to_cpu() and > > > firends.. > > > > More complexity here :) So a lot of earlier SPI drivers didn't set bits_per_word, > > the result of this is that a read had no way to know how to unwind the endian > > nature of the data. If you do a 4 byte read, is that 4x 1 byte, 2x 2 bytes or > > 1x 4 bytes. Thus the SPI subsystem had no way of knowing how to convert from > > wire order of big endian to cpu endianness. This is particularly fun as it > > is common to have variable length registers on SPI devices (be it described > > on the datasheet as some registers have high and low byte addresses). > > > > In drivers where this can be set to one consistent value, then the SPI subsystem > > should do the work for us. Hence this one should be fine. ( I think :) > > > > Based on other input: > > Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com> I've applied this to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Note, as we don't have a proven case in which it causes actual harm, I haven't marked it for stable. Thanks, Jonathan > > > > Maybe another separate patch can be made to take care about endianess later on? > > > > > > BTW Also, the ____cacheline_aligned is a bit scaring :) I don't know > > > what is that for... > > > > > > > Thanks > > > > Alex > > > > > > > > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > > > > > --- > > > > > drivers/iio/adc/ad7949.c | 6 +++--- > > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > > > > > index 518044c31a73..5c2b3446fa4a 100644 > > > > > --- a/drivers/iio/adc/ad7949.c > > > > > +++ b/drivers/iio/adc/ad7949.c > > > > > @@ -54,7 +54,7 @@ struct ad7949_adc_chip { > > > > > u8 resolution; > > > > > u16 cfg; > > > > > unsigned int current_channel; > > > > > - u32 buffer ____cacheline_aligned; > > > > > + u16 buffer ____cacheline_aligned; > > > > > }; > > > > > > > > > > static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > > > > > @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > > > > > struct spi_transfer tx[] = { > > > > > { > > > > > .tx_buf = &ad7949_adc->buffer, > > > > > - .len = 4, > > > > > + .len = 2, > > > > > .bits_per_word = bits_per_word, > > > > > }, > > > > > }; > > > > > @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, > > > > > struct spi_transfer tx[] = { > > > > > { > > > > > .rx_buf = &ad7949_adc->buffer, > > > > > - .len = 4, > > > > > + .len = 2, > > > > > .bits_per_word = bits_per_word, > > > > > }, > > > > > };
diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c index 518044c31a73..5c2b3446fa4a 100644 --- a/drivers/iio/adc/ad7949.c +++ b/drivers/iio/adc/ad7949.c @@ -54,7 +54,7 @@ struct ad7949_adc_chip { u8 resolution; u16 cfg; unsigned int current_channel; - u32 buffer ____cacheline_aligned; + u16 buffer ____cacheline_aligned; }; static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, struct spi_transfer tx[] = { { .tx_buf = &ad7949_adc->buffer, - .len = 4, + .len = 2, .bits_per_word = bits_per_word, }, }; @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, struct spi_transfer tx[] = { { .rx_buf = &ad7949_adc->buffer, - .len = 4, + .len = 2, .bits_per_word = bits_per_word, }, };
This driver supports 14-bits and 16-bits devices. All of them have a 14-bit configuration registers. All SPI trasfers, for reading AD conversion results and for writing the configuration register, fit in two bytes. The driver always uses 4-bytes xfers which seems at least pointless (maybe even harmful). This patch trims the SPI xfer len and the buffer size to two bytes. Signed-off-by: Andrea Merello <andrea.merello@gmail.com> --- drivers/iio/adc/ad7949.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)