From: anish kumar <[email protected]>
replaced kmalloc with local variable as I2C(in this case) doesn't require
kmalloc memory it can do with stack memory.
Signed-off-by: anish kumar <[email protected]>
---
drivers/staging/iio/adc/max1363_core.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
index 1037087..9462230 100644
--- a/drivers/staging/iio/adc/max1363_core.c
+++ b/drivers/staging/iio/adc/max1363_core.c
@@ -207,15 +207,12 @@ static int max1363_write_basic_config(struct i2c_client *client,
unsigned char d2)
{
int ret;
- u8 *tx_buf = kmalloc(2, GFP_KERNEL);
+ u8 tx_buf[2];
- if (!tx_buf)
- return -ENOMEM;
tx_buf[0] = d1;
tx_buf[1] = d2;
ret = i2c_master_send(client, tx_buf, 2);
- kfree(tx_buf);
return (ret > 0) ? 0 : ret;
}
--
1.7.0.4
On 06/07/11 13:39, anish wrote:
> From: anish kumar <[email protected]>
>
> replaced kmalloc with local variable as I2C(in this case) doesn't require
> kmalloc memory it can do with stack memory.
I've cc'd linux-i2c just to check I'm right about the whole i2c doesn't need
dma safe buffers bit...
>
> Signed-off-by: anish kumar <[email protected]>
Acked-by: Jonathan Cameron <[email protected]>
> ---
> drivers/staging/iio/adc/max1363_core.c | 5 +----
> 1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
> index 1037087..9462230 100644
> --- a/drivers/staging/iio/adc/max1363_core.c
> +++ b/drivers/staging/iio/adc/max1363_core.c
> @@ -207,15 +207,12 @@ static int max1363_write_basic_config(struct i2c_client *client,
> unsigned char d2)
> {
> int ret;
> - u8 *tx_buf = kmalloc(2, GFP_KERNEL);
> + u8 tx_buf[2];
>
> - if (!tx_buf)
> - return -ENOMEM;
> tx_buf[0] = d1;
> tx_buf[1] = d2;
>
> ret = i2c_master_send(client, tx_buf, 2);
> - kfree(tx_buf);
>
> return (ret > 0) ? 0 : ret;
> }
On Tue, Jun 07, 2011 at 02:00:23PM +0100, Jonathan Cameron wrote:
> On 06/07/11 13:39, anish wrote:
> > From: anish kumar <[email protected]>
> >
> > replaced kmalloc with local variable as I2C(in this case) doesn't require
> > kmalloc memory it can do with stack memory.
> I've cc'd linux-i2c just to check I'm right about the whole i2c doesn't need
> dma safe buffers bit...
No, it is down to the i2c driver, and from recollection dma from stack is
not recommended, due to things like cache line alignment. Please do not
do this.
> > Signed-off-by: anish kumar <[email protected]>
> Acked-by: Jonathan Cameron <[email protected]>
> > ---
> > drivers/staging/iio/adc/max1363_core.c | 5 +----
> > 1 files changed, 1 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
> > index 1037087..9462230 100644
> > --- a/drivers/staging/iio/adc/max1363_core.c
> > +++ b/drivers/staging/iio/adc/max1363_core.c
> > @@ -207,15 +207,12 @@ static int max1363_write_basic_config(struct i2c_client *client,
> > unsigned char d2)
> > {
> > int ret;
> > - u8 *tx_buf = kmalloc(2, GFP_KERNEL);
> > + u8 tx_buf[2];
> >
> > - if (!tx_buf)
> > - return -ENOMEM;
> > tx_buf[0] = d1;
> > tx_buf[1] = d2;
> >
> > ret = i2c_master_send(client, tx_buf, 2);
> > - kfree(tx_buf);
> >
> > return (ret > 0) ? 0 : ret;
> > }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Ben Dooks, [email protected], http://www.fluff.org/ben/
Large Hadron Colada: A large Pina Colada that makes the universe disappear.
On 06/07/11 14:41, Ben Dooks wrote:
> On Tue, Jun 07, 2011 at 02:00:23PM +0100, Jonathan Cameron wrote:
>> On 06/07/11 13:39, anish wrote:
>>> From: anish kumar <[email protected]>
>>>
>>> replaced kmalloc with local variable as I2C(in this case) doesn't require
>>> kmalloc memory it can do with stack memory.
>> I've cc'd linux-i2c just to check I'm right about the whole i2c doesn't need
>> dma safe buffers bit...
>
> No, it is down to the i2c driver, and from recollection dma from stack is
> not recommended, due to things like cache line alignment. Please do not
> do this.
Then lets drop this. Sorry Anish, seems I led you down the garden path.
I'll check all my i2c drivers don't do this...
>
>>> Signed-off-by: anish kumar <[email protected]>
>> Acked-by: Jonathan Cameron <[email protected]>
>>> ---
>>> drivers/staging/iio/adc/max1363_core.c | 5 +----
>>> 1 files changed, 1 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
>>> index 1037087..9462230 100644
>>> --- a/drivers/staging/iio/adc/max1363_core.c
>>> +++ b/drivers/staging/iio/adc/max1363_core.c
>>> @@ -207,15 +207,12 @@ static int max1363_write_basic_config(struct i2c_client *client,
>>> unsigned char d2)
>>> {
>>> int ret;
>>> - u8 *tx_buf = kmalloc(2, GFP_KERNEL);
>>> + u8 tx_buf[2];
>>>
>>> - if (!tx_buf)
>>> - return -ENOMEM;
>>> tx_buf[0] = d1;
>>> tx_buf[1] = d2;
>>>
>>> ret = i2c_master_send(client, tx_buf, 2);
>>> - kfree(tx_buf);
>>>
>>> return (ret > 0) ? 0 : ret;
>>> }
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Tue, 7 Jun 2011 14:41:35 +0100, Ben Dooks wrote:
> On Tue, Jun 07, 2011 at 02:00:23PM +0100, Jonathan Cameron wrote:
> > On 06/07/11 13:39, anish wrote:
> > > From: anish kumar <[email protected]>
> > >
> > > replaced kmalloc with local variable as I2C(in this case) doesn't require
> > > kmalloc memory it can do with stack memory.
> > I've cc'd linux-i2c just to check I'm right about the whole i2c doesn't need
> > dma safe buffers bit...
>
> No, it is down to the i2c driver, and from recollection dma from stack is
> not recommended, due to things like cache line alignment. Please do not
> do this.
To be clearer, it is down to the i2c BUS (adapter) driver, NOT the i2c
device driver. So the patch is correct.
Keep in mind that not all I2C/SMBus controllers care about DMA. In fact,
most don't (at least in the set I am maintaining - might be different
an embedded.) So calling kmalloc for every transfer in every I2C device
driver "just in case" is very much counterproductive.
And, if a controller does DMA and needs well-aligned, dynamically
allocated buffer, its driver would hopefully allocate the buffer ONCE
and keep it around, rather than reallocating it for every transfer.
--
Jean Delvare
Jonathan Cameron wrote on 2011-06-07:
> On 06/07/11 14:41, Ben Dooks wrote:
>> On Tue, Jun 07, 2011 at 02:00:23PM +0100, Jonathan Cameron wrote:
>>> On 06/07/11 13:39, anish wrote:
>>>> From: anish kumar <[email protected]>
>>>>
>>>> replaced kmalloc with local variable as I2C(in this case) doesn't
>>>> require kmalloc memory it can do with stack memory.
>>> I've cc'd linux-i2c just to check I'm right about the whole i2c
>>> doesn't need dma safe buffers bit...
>>
>> No, it is down to the i2c driver, and from recollection dma from stack
>> is not recommended, due to things like cache line alignment. Please do
>> not do this.
> Then lets drop this. Sorry Anish, seems I led you down the garden path.
> I'll check all my i2c drivers don't do this...
Can you point to a i2c bus driver that does dma and uses the buffer from the
client directly?
>>
>>>> Signed-off-by: anish kumar <[email protected]> Acked-by:
>>>> Jonathan Cameron <[email protected]> ---
>>>> drivers/staging/iio/adc/max1363_core.c | 5 +----
>>>> 1 files changed, 1 insertions(+), 4 deletions(-)
>>>> diff --git a/drivers/staging/iio/adc/max1363_core.c
>>>> b/drivers/staging/iio/adc/max1363_core.c
>>>> index 1037087..9462230 100644
>>>> --- a/drivers/staging/iio/adc/max1363_core.c
>>>> +++ b/drivers/staging/iio/adc/max1363_core.c
>>>> @@ -207,15 +207,12 @@ static int max1363_write_basic_config(struct
> i2c_client *client,
>>>> unsigned char d2)
>>>> {
>>>> int ret;
>>>> - u8 *tx_buf = kmalloc(2, GFP_KERNEL);
>>>> + u8 tx_buf[2];
>>>>
>>>> - if (!tx_buf)
>>>> - return -ENOMEM;
>>>> tx_buf[0] = d1;
>>>> tx_buf[1] = d2;
>>>>
>>>> ret = i2c_master_send(client, tx_buf, 2);
>>>> - kfree(tx_buf);
>>>>
>>>> return (ret > 0) ? 0 : ret;
>>>> }
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-i2c"
>>> in the body of a message to [email protected] More
>>> majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
Greetings,
Michael
--
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif
On Thu, 9 Jun 2011 13:53:09 +0530, anish singh wrote:
> On Wed, Jun 8, 2011 at 10:41 AM, anish singh <[email protected]>wrote:
> > On Tue, Jun 7, 2011 at 7:32 PM, Jean Delvare <[email protected]> wrote:
> >> To be clearer, it is down to the i2c BUS (adapter) driver, NOT the i2c
> >> device driver. So the patch is correct.
> >>
> > I think i can take Jean ack on this?If yes then Joanthan kindly apply
> > this patch and i think you didn't lead me in wrong way as whatever
> > said by you is corroborated by jean also i.e. it is I2C bus driver
> > responsiblity
> > to care about DMA.
> >
> Sorry to ping you again.Can i take your ack on this?
Yes of course.
Acked-by: Jean Delvare <[email protected]>
--
Jean Delvare
On 06/09/11 09:34, anish singh wrote:
>
>
> On Thu, Jun 9, 2011 at 1:59 PM, Jean Delvare <[email protected] <mailto:[email protected]>> wrote:
>
> On Thu, 9 Jun 2011 13:53:09 +0530, anish singh wrote:
> > On Wed, Jun 8, 2011 at 10:41 AM, anish singh <[email protected] <mailto:[email protected]>>wrote:
> > > On Tue, Jun 7, 2011 at 7:32 PM, Jean Delvare <[email protected] <mailto:[email protected]>> wrote:
> > >> To be clearer, it is down to the i2c BUS (adapter) driver, NOT the i2c
> > >> device driver. So the patch is correct.
> > >>
> > > I think i can take Jean ack on this?If yes then Joanthan kindly apply
> > > this patch and i think you didn't lead me in wrong way as whatever
> > > said by you is corroborated by jean also i.e. it is I2C bus driver
> > > responsiblity
> > > to care about DMA.
> > >
> > Sorry to ping you again.Can i take your ack on this?
>
> Yes of course.
>
> Acked-by: Jean Delvare <[email protected] <mailto:[email protected]>>
>
> Thanks a ton.Jonathan kindly apply it now :)
Greg, I'll send this one on to you with the set I currently have out for review.
Jonathan
On 06/09/11 09:48, Jonathan Cameron wrote:
> On 06/09/11 09:34, anish singh wrote:
>>
>>
>> On Thu, Jun 9, 2011 at 1:59 PM, Jean Delvare <[email protected] <mailto:[email protected]>> wrote:
>>
>> On Thu, 9 Jun 2011 13:53:09 +0530, anish singh wrote:
>> > On Wed, Jun 8, 2011 at 10:41 AM, anish singh <[email protected] <mailto:[email protected]>>wrote:
>> > > On Tue, Jun 7, 2011 at 7:32 PM, Jean Delvare <[email protected] <mailto:[email protected]>> wrote:
>> > >> To be clearer, it is down to the i2c BUS (adapter) driver, NOT the i2c
>> > >> device driver. So the patch is correct.
>> > >>
>> > > I think i can take Jean ack on this?If yes then Joanthan kindly apply
>> > > this patch and i think you didn't lead me in wrong way as whatever
>> > > said by you is corroborated by jean also i.e. it is I2C bus driver
>> > > responsiblity
>> > > to care about DMA.
>> > >
>> > Sorry to ping you again.Can i take your ack on this?
>>
>> Yes of course.
>>
>> Acked-by: Jean Delvare <[email protected] <mailto:[email protected]>>
>>
>> Thanks a ton.Jonathan kindly apply it now :)
> Greg, I'll send this one on to you with the set I currently have out for review.
>
Doh, after all this, I just tried to apply this to find the code in question has already
gone. Anish, what tree are you working against? Looks like I did an equivalent clean up
(with a load of others) back in May then forgot about it.
Sorry all. I should have actually have checked it was still relevant rather than reviewing
purely on basis of content of patch...
Jonathan
On 06/09/11 15:29, anish singh wrote:
>
>
> On Thu, Jun 9, 2011 at 7:46 PM, Jonathan Cameron <[email protected] <mailto:[email protected]>> wrote:
>
> On 06/09/11 09:48, Jonathan Cameron wrote:
> > On 06/09/11 09:34, anish singh wrote:
> >>
> >>
> >> On Thu, Jun 9, 2011 at 1:59 PM, Jean Delvare <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:
> >>
> >> On Thu, 9 Jun 2011 13:53:09 +0530, anish singh wrote:
> >> > On Wed, Jun 8, 2011 at 10:41 AM, anish singh <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>>wrote:
> >> > > On Tue, Jun 7, 2011 at 7:32 PM, Jean Delvare <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:
> >> > >> To be clearer, it is down to the i2c BUS (adapter) driver, NOT the i2c
> >> > >> device driver. So the patch is correct.
> >> > >>
> >> > > I think i can take Jean ack on this?If yes then Joanthan kindly apply
> >> > > this patch and i think you didn't lead me in wrong way as whatever
> >> > > said by you is corroborated by jean also i.e. it is I2C bus driver
> >> > > responsiblity
> >> > > to care about DMA.
> >> > >
> >> > Sorry to ping you again.Can i take your ack on this?
> >>
> >> Yes of course.
> >>
> >> Acked-by: Jean Delvare <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>>
> >>
> >> Thanks a ton.Jonathan kindly apply it now :)
> > Greg, I'll send this one on to you with the set I currently have out for review.
> >
> Doh, after all this, I just tried to apply this to find the code in question has already
> gone. Anish, what tree are you working against?
>
> I am using linux-next.Is it not the right tree for staging?
Something curious is going on then because this code is no longer in linux-next either.
>
> Looks like I did an equivalent clean up
> (with a load of others) back in May then forgot about it.
>
> Sorry all. I should have actually have checked it was still relevant rather than reviewing
> purely on basis of content of patch...
>
> Jonathan
>
>