Re: [PATCH] drivers: misc: ti-st: Use int instead of fuzzy char for callback status

[Samuel Ortiz]

Hi Marcel,

On Wed, Jul 13, 2016 at 11:56:02AM +0100, Marcel Holtmann wrote:
> Hi Mauro,
> 
> >> On mips and parisc:
> >> 
> >>drivers/bluetooth/btwilink.c: In function 'ti_st_open':
> >>drivers/bluetooth/btwilink.c:174:21: warning: overflow in implicit 
> >> constant conversion [-Woverflow]
> >>   hst->reg_status = -EINPROGRESS;
> >> 
> >>drivers/nfc/nfcwilink.c: In function 'nfcwilink_open':
> >>drivers/nfc/nfcwilink.c:396:31: warning: overflow in implicit constant 
> >> conversion [-Woverflow]
> >>  drv->st_register_cb_status = -EINPROGRESS;
> >> 
> >> There are actually two issues:
> >>  1. Whether "char" is signed or unsigned depends on the architecture.
> >> As the completion callback data is used to pass a (negative) error
> >> code, it should always be signed.
> >>  2. EINPROGRESS is 150 on mips, 245 on parisc.
> >> Hence -EINPROGRESS doesn't fit in a signed 8-bit number.
> >> 
> >> Change the callback status from "char" to "int" to fix these.
> >> 
> >> Signed-off-by: Geert Uytterhoeven 
> > 
> > Patch looks sane to me, but who will apply it?
> > 
> > Anyway:
> > 
> > Acked-by: Mauro Carvalho Chehab 
> 
> I can take it through bluetooth-next if there is no objection.
> 
> Samuel, are you fine with that?
Yes, please go ahead.

Cheers,
Samuel.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Doc:nfc: Fix typo in nfc-hci.txt

[Samuel Ortiz]

Hi Msanari,

On Fri, Jun 05, 2015 at 09:38:19PM +0900, Masanari Iida wrote:
 This patch fix a spelling typo in nfc-hci.txt
 
 Signed-off-by: Masanari Iida standby2...@gmail.com
 ---
  Documentation/nfc/nfc-hci.txt | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.

Cheers,
Samuel.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 00/12] Driver for Si476x series of chips

[Samuel Ortiz]

Hi Andrey,

On Thu, Apr 18, 2013 at 09:58:26AM -0700, Andrey Smirnov wrote:
 Driver for Si476x series of chips
 
 This is a eight version of the patchset originaly posted here:
 https://lkml.org/lkml/2012/9/13/590
 
 Second version of the patch was posted here:
 https://lkml.org/lkml/2012/10/5/598
 
 Third version of the patch was posted here:
 https://lkml.org/lkml/2012/10/23/510
 
 Fourth version of the patch was posted here:
 https://lkml.org/lkml/2013/2/18/572
 
 Fifth version of the patch was posted here:
 https://lkml.org/lkml/2013/2/26/45
 
 Sixth version of the patch was posted here:
 https://lkml.org/lkml/2013/2/26/257
 
 Seventh version of the patch was posted here:
 https://lkml.org/lkml/2013/2/27/22
 
 Eighth version of the patch was posted here:
 https://lkml.org/lkml/2013/3/26/891
 
 To save everyone's time I'll repost the original description of it:
 
 This patchset contains a driver for a Silicon Laboratories 476x series
 of radio tuners. The driver itself is implemented as an MFD devices
 comprised of three parts: 
  1. Core device that provides all the other devices with basic
 functionality and locking scheme.
  2. Radio device that translates between V4L2 subsystem requests into
 Core device commands.
  3. Codec device that does similar to the earlier described task, but
 for ALSA SoC subsystem.
 
 v9 of this driver has following changes:
- MFD part of the driver no longer depends on the header file added
  by the radio driver(media/si476x.h) which should potential
  restore the bisectability of the patches
I applied all the MFD patches from this patchset (All 4 first ones), plus a
follow up one for fixing the i2c related warning.
I also squashed the REGMAP_I2C dependency into patch #4.
It's all in mfd-next now, I'd appreciate if you could double check it's all
fine.

Mauro will take the rest, we made sure there won't be any merge conflict
between our trees.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 00/12] Driver for Si476x series of chips

[Samuel Ortiz]

On Thu, Apr 18, 2013 at 02:28:00PM -0300, Mauro Carvalho Chehab wrote:
 Em Thu, 18 Apr 2013 09:58:26 -0700
 Andrey Smirnov andrew.smir...@gmail.com escreveu:
 
  Driver for Si476x series of chips
  
  This is a eight version of the patchset originaly posted here:
  https://lkml.org/lkml/2012/9/13/590
  
  Second version of the patch was posted here:
  https://lkml.org/lkml/2012/10/5/598
  
  Third version of the patch was posted here:
  https://lkml.org/lkml/2012/10/23/510
  
  Fourth version of the patch was posted here:
  https://lkml.org/lkml/2013/2/18/572
  
  Fifth version of the patch was posted here:
  https://lkml.org/lkml/2013/2/26/45
  
  Sixth version of the patch was posted here:
  https://lkml.org/lkml/2013/2/26/257
  
  Seventh version of the patch was posted here:
  https://lkml.org/lkml/2013/2/27/22
  
  Eighth version of the patch was posted here:
  https://lkml.org/lkml/2013/3/26/891
  
  To save everyone's time I'll repost the original description of it:
  
  This patchset contains a driver for a Silicon Laboratories 476x series
  of radio tuners. The driver itself is implemented as an MFD devices
  comprised of three parts: 
   1. Core device that provides all the other devices with basic
  functionality and locking scheme.
   2. Radio device that translates between V4L2 subsystem requests into
  Core device commands.
   3. Codec device that does similar to the earlier described task, but
  for ALSA SoC subsystem.
  
  v9 of this driver has following changes:
 - MFD part of the driver no longer depends on the header file added
   by the radio driver(media/si476x.h) which should potential
   restore the bisectability of the patches
  
  Mauro, I am not sure if you reverted changes in patches 5 - 7, so I am
  including them just in case.
 
 No, I didn't revert all patches. I just reverted two patches: the
 last one, and the one that Samuel asked me.
Sorry I didn't have time to check your email from yesterday, but I was
actually hoping you would revert the whole patchset, then pull from my
mfd-next/topic/si476x branch to fetch the MFD bits and then apply the
v4l2/media ones (From patchset v9) on top of that.
Does that make sense to you ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/12] si476x: Fix some config dependencies and a compile warnings

[Samuel Ortiz]

On Thu, Apr 18, 2013 at 09:58:37AM -0700, Andrey Smirnov wrote:
 From: Hans Verkuil hverk...@xs4all.nl
 
 radio-si476x depends on SND and SND_SOC, the mfd driver should select
 REGMAP_I2C.
 
 Also fix a small compile warning in a debug message:
 
 drivers/mfd/si476x-i2c.c: In function ‘si476x_core_drain_rds_fifo’:
 drivers/mfd/si476x-i2c.c:391:4: warning: field width specifier ‘*’ expects 
 argument of type ‘int’, but argument 4 has type ‘long unsigned int’ [-Wformat]
 
 Acked-by: Andrey Smirnov andrew.smir...@gmail.com
 Signed-off-by: Hans Verkuil hans.verk...@cisco.com
 ---
  drivers/media/radio/Kconfig |2 +-
  drivers/mfd/Kconfig |1 +
  drivers/mfd/si476x-i2c.c|2 +-
You should have merged the MFD bits from this patch into one of the first 4
patches, that are MFD related. Or at least separated those 2 changes into 2
patches...

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 00/12] Driver for Si476x series of chips

[Samuel Ortiz]

On Thu, Apr 18, 2013 at 02:57:53PM -0300, Mauro Carvalho Chehab wrote:
 Em Thu, 18 Apr 2013 19:45:47 +0200
 Samuel Ortiz sa...@linux.intel.com escreveu:
 
  On Thu, Apr 18, 2013 at 02:28:00PM -0300, Mauro Carvalho Chehab wrote:
   Em Thu, 18 Apr 2013 09:58:26 -0700
   Andrey Smirnov andrew.smir...@gmail.com escreveu:
   
Driver for Si476x series of chips

This is a eight version of the patchset originaly posted here:
https://lkml.org/lkml/2012/9/13/590

Second version of the patch was posted here:
https://lkml.org/lkml/2012/10/5/598

Third version of the patch was posted here:
https://lkml.org/lkml/2012/10/23/510

Fourth version of the patch was posted here:
https://lkml.org/lkml/2013/2/18/572

Fifth version of the patch was posted here:
https://lkml.org/lkml/2013/2/26/45

Sixth version of the patch was posted here:
https://lkml.org/lkml/2013/2/26/257

Seventh version of the patch was posted here:
https://lkml.org/lkml/2013/2/27/22

Eighth version of the patch was posted here:
https://lkml.org/lkml/2013/3/26/891

To save everyone's time I'll repost the original description of it:

This patchset contains a driver for a Silicon Laboratories 476x series
of radio tuners. The driver itself is implemented as an MFD devices
comprised of three parts: 
 1. Core device that provides all the other devices with basic
functionality and locking scheme.
 2. Radio device that translates between V4L2 subsystem requests into
Core device commands.
 3. Codec device that does similar to the earlier described task, but
for ALSA SoC subsystem.

v9 of this driver has following changes:
   - MFD part of the driver no longer depends on the header file added
 by the radio driver(media/si476x.h) which should potential
 restore the bisectability of the patches

Mauro, I am not sure if you reverted changes in patches 5 - 7, so I am
including them just in case.
   
   No, I didn't revert all patches. I just reverted two patches: the
   last one, and the one that Samuel asked me.
  Sorry I didn't have time to check your email from yesterday, but I was
  actually hoping you would revert the whole patchset, then pull from my
  mfd-next/topic/si476x branch to fetch the MFD bits and then apply the
  v4l2/media ones (From patchset v9) on top of that.
  Does that make sense to you ?
 
 I don't rebase my tree, as this would cause troubles for everybody that
 relies on it.
 
 Reverting the entire patchset is hard, as there are lots of patches after
 them, and some patches touch at V4L2 core. Even reverting those
 two patches hit conflicts, that I needed to manage, in order to avoid
 compilation breakages.
 
 So, I really prefer to confine the patch reversion to the absolute 
 minimum.
In that case we're left with only one solution: Leave your tree as it is (with
both patches reverted) and push the mfd/Kconfig and mfd/Makefile changes as a
3.10 fix. radio/radio-si476x.c should not build without the MFD Kconfig symbol
so we should be safe. The radio/radio-si476x.c Kconfig dependency is not
correct btw, I'll send you a patch for that.

This is an ugly solution, but the only one I can think about. I would have
appreciated some sync before you merged this jumbo patch, especially since the
bulk of it is an MFD driver.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 00/12] Driver for Si476x series of chips

[Samuel Ortiz]

On Thu, Apr 18, 2013 at 08:17:18PM +0200, Samuel Ortiz wrote:
 On Thu, Apr 18, 2013 at 02:57:53PM -0300, Mauro Carvalho Chehab wrote:
  Em Thu, 18 Apr 2013 19:45:47 +0200
  Samuel Ortiz sa...@linux.intel.com escreveu:
  
   On Thu, Apr 18, 2013 at 02:28:00PM -0300, Mauro Carvalho Chehab wrote:
Em Thu, 18 Apr 2013 09:58:26 -0700
Andrey Smirnov andrew.smir...@gmail.com escreveu:

 Driver for Si476x series of chips
 
 This is a eight version of the patchset originaly posted here:
 https://lkml.org/lkml/2012/9/13/590
 
 Second version of the patch was posted here:
 https://lkml.org/lkml/2012/10/5/598
 
 Third version of the patch was posted here:
 https://lkml.org/lkml/2012/10/23/510
 
 Fourth version of the patch was posted here:
 https://lkml.org/lkml/2013/2/18/572
 
 Fifth version of the patch was posted here:
 https://lkml.org/lkml/2013/2/26/45
 
 Sixth version of the patch was posted here:
 https://lkml.org/lkml/2013/2/26/257
 
 Seventh version of the patch was posted here:
 https://lkml.org/lkml/2013/2/27/22
 
 Eighth version of the patch was posted here:
 https://lkml.org/lkml/2013/3/26/891
 
 To save everyone's time I'll repost the original description of it:
 
 This patchset contains a driver for a Silicon Laboratories 476x series
 of radio tuners. The driver itself is implemented as an MFD devices
 comprised of three parts: 
  1. Core device that provides all the other devices with basic
 functionality and locking scheme.
  2. Radio device that translates between V4L2 subsystem requests into
 Core device commands.
  3. Codec device that does similar to the earlier described task, but
 for ALSA SoC subsystem.
 
 v9 of this driver has following changes:
- MFD part of the driver no longer depends on the header file added
  by the radio driver(media/si476x.h) which should potential
  restore the bisectability of the patches
 
 Mauro, I am not sure if you reverted changes in patches 5 - 7, so I am
 including them just in case.

No, I didn't revert all patches. I just reverted two patches: the
last one, and the one that Samuel asked me.
   Sorry I didn't have time to check your email from yesterday, but I was
   actually hoping you would revert the whole patchset, then pull from my
   mfd-next/topic/si476x branch to fetch the MFD bits and then apply the
   v4l2/media ones (From patchset v9) on top of that.
   Does that make sense to you ?
  
  I don't rebase my tree, as this would cause troubles for everybody that
  relies on it.
  
  Reverting the entire patchset is hard, as there are lots of patches after
  them, and some patches touch at V4L2 core. Even reverting those
  two patches hit conflicts, that I needed to manage, in order to avoid
  compilation breakages.
  
  So, I really prefer to confine the patch reversion to the absolute 
  minimum.
 In that case we're left with only one solution: Leave your tree as it is (with
 both patches reverted) and push the mfd/Kconfig and mfd/Makefile changes as a
 3.10 fix. radio/radio-si476x.c should not build without the MFD Kconfig symbol
 so we should be safe. 
You reverted that one, I think this was not needed as it would not build
without the MFD symbol. Any other reason why you reverted it ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/12] v4l2: Add a V4L2 driver for SI476X MFD

[Samuel Ortiz]

Hi Andrey,

On Thu, Apr 18, 2013 at 09:58:35AM -0700, Andrey Smirnov wrote:
 diff --git a/drivers/media/radio/Kconfig b/drivers/media/radio/Kconfig
 index ead9928..170460d 100644
 --- a/drivers/media/radio/Kconfig
 +++ b/drivers/media/radio/Kconfig
 @@ -18,6 +18,23 @@ config RADIO_SI470X
  
  source drivers/media/radio/si470x/Kconfig
  
 +config RADIO_SI476X
 + tristate Silicon Laboratories Si476x I2C FM Radio
 + depends on I2C  VIDEO_V4L2
 + select MFD_CORE
 + select MFD_SI476X_CORE
This is wrong. You want depends on MFD_SI476X_CORE, you should not select a
symbol that has other dependencies. It also would allow us to carry the
v4l2/media parts of your patchset independently from the MFD ones as the radio
driver will not be buildable on a tree where the MFD part is not present.
We'll try to figure something out with Mauro.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 1/9] mfd: Add commands abstraction layer for SI476X MFD

[Samuel Ortiz]

On Mon, Apr 08, 2013 at 01:40:40PM -0700, Andrey Smirnov wrote:
 On Mon, Apr 8, 2013 at 1:09 PM, Samuel Ortiz sa...@linux.intel.com wrote:
  On Mon, Apr 08, 2013 at 11:34:43AM -0700, Andrey Smirnov wrote:
   On Mon, Apr 8, 2013 at 3:16 AM, Samuel Ortiz sa...@linux.intel.com
  wrote:
This file doesn't exist yet, which breaks bisectability.
I'm fine with you including it with the first patch. I will prepare a
   branch
with the mfd patches from your serie for Mauro to pull from.
   
  
   It was initially one single patch(in v1), and I split it in three upon
   Hans' request(for ease of reviewing).
  It probably made sense then, but now, as I said, it breaks bisectability.
  So
  I'd appreciate if you could add this header file to this first patch so
  that I
  can merge the MFD parts independently.
  Again, I will provide a branch for Mauro to pull from and apply the
  remaining
  patches on top of it.
 
 
 OK, I will squash the commits and make another version of the patches.
The separation of the patches as it is now looks good to me. But you need to
add the media header to the first MFD patches, no need to squash commits
together.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/10] drivers: mfd: use module_platform_driver_probe()

[Samuel Ortiz]

Hi Fabio,

On Thu, Mar 14, 2013 at 02:11:29PM +0100, Fabio Porcedda wrote:
 This patch converts the drivers to use the
 module_platform_driver_probe() macro which makes the code smaller and
 a bit simpler.
 
 Signed-off-by: Fabio Porcedda fabio.porce...@gmail.com
 Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
 Cc: Linus Walleij linus.wall...@linaro.org
 Cc: Samuel Ortiz sa...@linux.intel.com
 Cc: linux-arm-ker...@lists.infradead.org
 ---
  drivers/mfd/davinci_voicecodec.c | 12 +---
  drivers/mfd/htc-pasic3.c | 13 +
  2 files changed, 2 insertions(+), 23 deletions(-)
Jingoo Han sent a larger patchset to convert many MFD drivers to
module_platform_driver_probe(), including htc-pasic3 and davinci_voicecodec.

See my mfd-next tree for more details.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 1/9] mfd: Add commands abstraction layer for SI476X MFD

[Samuel Ortiz]

Hi Andrey,

On Tue, Mar 26, 2013 at 07:47:18PM -0700, Andrey Smirnov wrote:
 From: Andrey Smirnov andreysm@charmander.(none)
 
 This patch adds all the functions used for exchanging commands with
 the chip.
 
 Acked-by: Hans Verkuil hans.verk...@cisco.com
 Signed-off-by: Andrey Smirnov andrew.smir...@gmail.com
 ---
  drivers/mfd/si476x-cmd.c | 1554 
 ++
  1 file changed, 1554 insertions(+)
  create mode 100644 drivers/mfd/si476x-cmd.c
The patchset looks good to me, and I was willing to merge it but:


 diff --git a/drivers/mfd/si476x-cmd.c b/drivers/mfd/si476x-cmd.c
 new file mode 100644
 index 000..71ac2e8
 --- /dev/null
 +++ b/drivers/mfd/si476x-cmd.c
 @@ -0,0 +1,1554 @@
 +/*
 + * drivers/mfd/si476x-cmd.c -- Subroutines implementing command
 + * protocol of si476x series of chips
 + *
 + * Copyright (C) 2012 Innovative Converged Devices(ICD)
 + * Copyright (C) 2013 Andrey Smirnov
 + *
 + * Author: Andrey Smirnov andrew.smir...@gmail.com
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; version 2 of the License.
 + *
 + * This program is distributed in the hope that it will be useful, but
 + * WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 + * General Public License for more details.
 + *
 + */
 +
 +#include linux/module.h
 +#include linux/completion.h
 +#include linux/delay.h
 +#include linux/atomic.h
 +#include linux/i2c.h
 +#include linux/device.h
 +#include linux/gpio.h
 +#include linux/videodev2.h
 +
 +#include media/si476x.h
This file doesn't exist yet, which breaks bisectability.
I'm fine with you including it with the first patch. I will prepare a branch
with the mfd patches from your serie for Mauro to pull from.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v2 1/2] max77693: added device tree support

[Samuel Ortiz]

Hi Andrzej,

On Tue, Feb 19, 2013 at 04:36:16PM +0100, Andrzej Hajda wrote:
 max77693 mfd main device uses only wakeup field
 from max77693_platform_data. This field is mapped
 to wakeup-source property in device tree.
 Device tree bindings doc will be added in max77693-led patch.
 
 Signed-off-by: Andrzej Hajda a.ha...@samsung.com
 Reviewed-by: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  drivers/mfd/max77693.c |   40 +---
  1 file changed, 33 insertions(+), 7 deletions(-)
This patch looks good to me, but doesn't apply to mfd-next. Would you mind
rebasing it ?

Cheers,
Samuel.


-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 1/9] mfd: Add commands abstraction layer for SI476X MFD

[Samuel Ortiz]

Hi Andrey,

On Mon, Apr 08, 2013 at 11:34:43AM -0700, Andrey Smirnov wrote:
 On Mon, Apr 8, 2013 at 3:16 AM, Samuel Ortiz sa...@linux.intel.com wrote:
  This file doesn't exist yet, which breaks bisectability.
  I'm fine with you including it with the first patch. I will prepare a
 branch
  with the mfd patches from your serie for Mauro to pull from.
 
 
 It was initially one single patch(in v1), and I split it in three upon
 Hans' request(for ease of reviewing).
It probably made sense then, but now, as I said, it breaks bisectability. So
I'd appreciate if you could add this header file to this first patch so that I
can merge the MFD parts independently.
Again, I will provide a branch for Mauro to pull from and apply the remaining
patches on top of it.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mfd: remove CONFIG_MFD_SUPPORT

[Samuel Ortiz]

Hi Arnd,

On Fri, Sep 02, 2011 at 04:43:36PM +0200, Arnd Bergmann wrote:
 We currently have two symbols to control compilation the MFD subsystem,
 MFD_SUPPORT and MFD_CORE. The MFD_SUPPORT is actually not required
 at all, it only hides the submenu when not set, with the effect that
 Kconfig warns about missing dependencies when another driver selects
 an MFD driver while MFD_SUPPORT is disabled. Turning the MFD submenu
 back from menuconfig into a plain menu simplifies the Kconfig syntax
 for those kinds of users and avoids the surprise when the menu
 suddenly appears because another driver was enabled that selects this
 symbol.
So I applied this one, thanks a lot.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Thu, Apr 07, 2011 at 07:35:15AM -0700, Grant Likely wrote:
  Below is a patch for the Xilinx SPI example. Although this would fix the
  issue, we'd still have to do that on device per device basis. I had a 
  similar
  solution where MFD drivers would set a flag for sub drivers that don't need
  any of the MFD bits. In that case the MFD core code would just forward the
  platform data, instead of embedding it through an MFD cell.
 
 platform_data is already a fiddly bit where you don't know what
 structure type platform_data points at; it is implicitly known and
 easy to get wrong.  This solution makes me *very* nervous
 because it would become even easier to get a mismatch on the
 platform_data pointer type.
How would that be more error prone than say a board file instantiating a
platform device after having set the platform_data pointer to point to an
implicitely know structure reference ?

Cheers,
Samuel.

P.S.: Would you be ok with something like the patch below ?

  ---
   drivers/mfd/timberdale.c |8 
   drivers/spi/xilinx_spi.c |   19 ++-
   include/linux/mfd/core.h |3 +++
   3 files changed, 25 insertions(+), 5 deletions(-)
  
  diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
  index 94c6c8a..c9220ce 100644
  --- a/drivers/mfd/timberdale.c
  +++ b/drivers/mfd/timberdale.c
  @@ -416,7 +416,7 @@ static __devinitdata struct mfd_cell 
  timberdale_cells_bar0_cfg0[] = {
  .mfd_data = timberdale_radio_platform_data,
  },
  {
  -   .name = xilinx_spi,
  +   .name = mfd_xilinx_spi,
  .num_resources = ARRAY_SIZE(timberdale_spi_resources),
  .resources = timberdale_spi_resources,
  .mfd_data = timberdale_xspi_platform_data,
  @@ -476,7 +476,7 @@ static __devinitdata struct mfd_cell 
  timberdale_cells_bar0_cfg1[] = {
  .mfd_data = timberdale_radio_platform_data,
  },
  {
  -   .name = xilinx_spi,
  +   .name = mfd_xilinx_spi,
  .num_resources = ARRAY_SIZE(timberdale_spi_resources),
  .resources = timberdale_spi_resources,
  .mfd_data = timberdale_xspi_platform_data,
  @@ -526,7 +526,7 @@ static __devinitdata struct mfd_cell 
  timberdale_cells_bar0_cfg2[] = {
  .mfd_data = timberdale_radio_platform_data,
  },
  {
  -   .name = xilinx_spi,
  +   .name = mfd_xilinx_spi,
  .num_resources = ARRAY_SIZE(timberdale_spi_resources),
  .resources = timberdale_spi_resources,
  .mfd_data = timberdale_xspi_platform_data,
  @@ -570,7 +570,7 @@ static __devinitdata struct mfd_cell 
  timberdale_cells_bar0_cfg3[] = {
  .mfd_data = timberdale_radio_platform_data,
  },
  {
  -   .name = xilinx_spi,
  +   .name = mfd_xilinx_spi,
  .num_resources = ARRAY_SIZE(timberdale_spi_resources),
  .resources = timberdale_spi_resources,
  .mfd_data = timberdale_xspi_platform_data,
  diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
  index c69c6f2..3287b84 100644
  --- a/drivers/spi/xilinx_spi.c
  +++ b/drivers/spi/xilinx_spi.c
  @@ -471,7 +471,11 @@ static int __devinit xilinx_spi_probe(struct 
  platform_device *dev)
  struct spi_master *master;
  u8 i;
   
  -   pdata = mfd_get_data(dev);
  +   if (platform_get_device_id(dev) 
  +   platform_get_device_id(dev)-driver_data  MFD_PLATFORM_DEVICE)
  +   pdata = mfd_get_data(dev);
  +   else
  +   pdata = dev-dev.platform_data;
  if (pdata) {
  num_cs = pdata-num_chipselect;
  little_endian = pdata-little_endian;
  @@ -530,6 +534,18 @@ static int __devexit xilinx_spi_remove(struct 
  platform_device *dev)
   /* work with hotplug and coldplug */
   MODULE_ALIAS(platform: XILINX_SPI_NAME);
   
  +static const struct platform_device_id xilinx_spi_id_table[] = {
  +   {
  +   .name   = XILINX_SPI_NAME,
  +   },
  +   {
  +   .name   = mfd_xilinx_spi,
  +   .driver_data = MFD_PLATFORM_DEVICE,
  +   },
  +   {  },   /* Terminating Entry */
  +};
  +MODULE_DEVICE_TABLE(platform, xilinx_spi_id_table);
  +
   static struct platform_driver xilinx_spi_driver = {
  .probe = xilinx_spi_probe,
  .remove = __devexit_p(xilinx_spi_remove),
  @@ -538,6 +554,7 @@ static struct platform_driver xilinx_spi_driver = {
  .owner = THIS_MODULE,
  .of_match_table = xilinx_spi_of_match,
  },
  +   .id_table   = xilinx_spi_id_table,
   };
   
   static int __init xilinx_spi_pltfm_init(void)
  diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
  index ad1b19a..13f31f4 100644
  --- a/include/linux/mfd/core.h
  +++ b/include/linux/mfd/core.h
  @@ -89,6 +89,9 @@ static inline const struct mfd_cell *mfd_get_cell(struct 
  platform_device *pdev)
  return pdev-dev.platform_data;
   }
   
  +/* */
  +#define MFD_PLATFORM_DEVICE BIT(0)
  +
   

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Mon, Apr 04, 2011 at 09:04:29PM -0600, Grant Likely wrote:
  The second step would be to get rid of mfd_get_data() and have all 
  subdrivers
  going back to the regular platform_data way. They would no longer be 
  dependant
  on the MFD code except for those who really need it. In that case they could
  just call mfd_get_cell() and get full access to their MFD cell.
 
 The revert to platform_data needs to happen ASAP though.  If this
 second step isn't ready really quickly, then the current patches
 should be reverted to give some breathing room for creating the
 replacement patches.  However, it's not such a rush if the below
 patch really does eliminate all of the nastiness of the original
 series. (I haven't looked and a rolled up diff of the first series and
 this change, so I don't know for sure).
I am done reverting these changes, with a final patch getting rid of
mfd_get_data. See
git://git.kernel.org/pub/scm/linux/kernel/git/sameo/mfd-2.6.git for-linus

I still need to give it a second review before pushing it to lkml for
comments. It's 20 patches long, so I'm not entirely sure Linus would take that
at that point.
Pushing patch #1 would be enough for fixing the issues introduced by the
original patchset, so I'm leaning toward pushing it and leaving the 19 other
patches for the next merge window.


 In principle I agree with this patch.  Some comments below.
Thanks for the comments. I think I addressed all of them in patch #1:


---
 drivers/base/platform.c  |1 +
 drivers/mfd/mfd-core.c   |   15 +--
 include/linux/device.h   |3 +++
 include/linux/mfd/core.h |7 +--
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f051cff..bde6b97 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -149,6 +149,7 @@ static void platform_device_release(struct device *dev)
 
of_device_node_put(pa-pdev.dev);
kfree(pa-pdev.dev.platform_data);
+   kfree(pa-pdev.dev.mfd_cell);
kfree(pa-pdev.resource);
kfree(pa);
 }
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d01574d..99b0d6d 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -18,6 +18,18 @@
 #include linux/pm_runtime.h
 #include linux/slab.h
 
+static int mfd_platform_add_cell(struct platform_device *pdev, const struct 
mfd_cell *cell)
+{
+   if (!cell)
+   return 0;
+
+   pdev-dev.mfd_cell = kmemdup(cell, sizeof(*cell), GFP_KERNEL);
+   if (!pdev-dev.mfd_cell)
+   return -ENOMEM;
+
+   return 0;
+}
+
 int mfd_cell_enable(struct platform_device *pdev)
 {
const struct mfd_cell *cell = mfd_get_cell(pdev);
@@ -75,7 +87,7 @@ static int mfd_add_device(struct device *parent, int id,
 
pdev-dev.parent = parent;
 
-   ret = platform_device_add_data(pdev, cell, sizeof(*cell));
+   ret = mfd_platform_add_cell(pdev, cell);
if (ret)
goto fail_res;
 
@@ -123,7 +135,6 @@ static int mfd_add_device(struct device *parent, int id,
 
return 0;
 
-/* platform_device_del(pdev); */
 fail_res:
kfree(res);
 fail_device:
diff --git a/include/linux/device.h b/include/linux/device.h
index ab8dfc0..cf353cf 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -33,6 +33,7 @@ struct class;
 struct subsys_private;
 struct bus_type;
 struct device_node;
+struct mfd_cell;
 
 struct bus_attribute {
struct attributeattr;
@@ -444,6 +445,8 @@ struct device {
struct device_node  *of_node; /* associated device tree node */
const struct of_device_id *of_match; /* matching of_device_id from 
driver */
 
+   struct mfd_cell *mfd_cell; /* MFD cell pointer */
+
dev_t   devt;   /* dev_t, creates the sysfs dev */
 
spinlock_t  devres_lock;
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index ad1b19a..28f81cf 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -86,7 +86,7 @@ extern int mfd_clone_cell(const char *cell, const char 
**clones,
  */
 static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
 {
-   return pdev-dev.platform_data;
+   return pdev-dev.mfd_cell;
 }
 
 /*
@@ -95,7 +95,10 @@ static inline const struct mfd_cell *mfd_get_cell(struct 
platform_device *pdev)
  */
 static inline void *mfd_get_data(struct platform_device *pdev)
 {
-   return mfd_get_cell(pdev)-mfd_data;
+   if (pdev-dev.mfd_cell)
+   return mfd_get_cell(pdev)-mfd_data;
+   else
+   return pdev-dev.platform_data;
 }
 
 extern int mfd_add_devices(struct device *parent, int id,
-- 
1.7.2.3

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

Hi Greg,

On Wed, Apr 06, 2011 at 08:58:05AM -0700, Greg KH wrote:
 On Wed, Apr 06, 2011 at 05:23:23PM +0200, Samuel Ortiz wrote:
  --- a/include/linux/device.h
  +++ b/include/linux/device.h
  @@ -33,6 +33,7 @@ struct class;
   struct subsys_private;
   struct bus_type;
   struct device_node;
  +struct mfd_cell;
   
   struct bus_attribute {
  struct attributeattr;
  @@ -444,6 +445,8 @@ struct device {
  struct device_node  *of_node; /* associated device tree node */
  const struct of_device_id *of_match; /* matching of_device_id from 
  driver */
   
  +   struct mfd_cell *mfd_cell; /* MFD cell pointer */
  +
 
 What is a MFD cell pointer and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.

We've been faced with the problem of being able to pass both MFD related data
and a platform_data pointer to some of those drivers. Squeezing the MFD bits
in the sub driver platform_data pointer doesn't work for drivers that know
nothing about MFDs. It also adds an additional dependency on the MFD API to
all MFD sub drivers. That prevents any of those drivers to eventually be used
as plain platform device drivers.
So, adding an MFD cell pointer to the device structure allows us to cleanly
pass both pieces of information, while keeping all the MFD sub drivers
independant from the MFD core if they want/can.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

Hi Ben,

On Wed, Apr 06, 2011 at 06:16:49PM +0100, Ben Hutchings wrote:
  So, adding an MFD cell pointer to the device structure allows us to cleanly
  pass both pieces of information, while keeping all the MFD sub drivers
  independant from the MFD core if they want/can.
 
 Why isn't an MFD the parent of its component devices?
It actually is. How would that help here ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Wed, Apr 06, 2011 at 10:56:47AM -0700, Greg KH wrote:
 On Wed, Apr 06, 2011 at 07:05:38PM +0200, Samuel Ortiz wrote:
  Hi Greg,
  
  On Wed, Apr 06, 2011 at 08:58:05AM -0700, Greg KH wrote:
   On Wed, Apr 06, 2011 at 05:23:23PM +0200, Samuel Ortiz wrote:
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -33,6 +33,7 @@ struct class;
 struct subsys_private;
 struct bus_type;
 struct device_node;
+struct mfd_cell;
 
 struct bus_attribute {
struct attributeattr;
@@ -444,6 +445,8 @@ struct device {
struct device_node  *of_node; /* associated device tree 
node */
const struct of_device_id *of_match; /* matching of_device_id 
from driver */
 
+   struct mfd_cell *mfd_cell; /* MFD cell pointer */
+
   
   What is a MFD cell pointer and why is it needed in struct device?
  An MFD cell is an MFD instantiated device.
  MFD (Multi Function Device) drivers instantiate platform devices. Those
  devices drivers sometimes need a platform data pointer, sometimes an MFD
  specific pointer, and sometimes both. Also, some of those drivers have been
  implemented as MFD sub drivers, while others know nothing about MFD and just
  expect a plain platform_data pointer.
 
 That sounds like a bug in those drivers, why not fix them to properly
 pass in the correct pointer?
Because they're drivers for generic IPs, not MFD ones. By forcing them to use
MFD specific structure and APIs, we make it more difficult for platform code
to instantiate them.
The timberdale MFD for example is built with a Xilinx SPI controller, and a
Micrel ks8842 ethernet switch IP. Forcing those devices into being MFD devices
would mean any platform willing to instantiate them would have to use the MFD
APIs. That sounds a bit artificial to me.
Although there is currently no drivers instantiated by both an MFD driver
and some platform code, Grant complaint about the Xilinx SPI driver moving
from a platform driver to an MFD one makes sense to me. 

  So, adding an MFD cell pointer to the device structure allows us to cleanly
  pass both pieces of information, while keeping all the MFD sub drivers
  independant from the MFD core if they want/can.
 
 They shouldn't be independant, 
Excuse my poor spelling.

 make them dependant and go from there.
That's what the code currently does.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Fri, Apr 01, 2011 at 05:58:44PM -0600, Grant Likely wrote:
 On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz sa...@linux.intel.com wrote:
  On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
  On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon dilin...@queued.net 
  wrote:
   On Fri, 1 Apr 2011 13:20:31 +0200
   Samuel Ortiz sa...@linux.intel.com wrote:
  
   Hi Grant,
  
   On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
   [...]
Gah.  Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable).  For instance, the xilinx ip
cores are used by more than just mfd.
   I agree. Since the vast majority of the MFD subdevices are MFD
   specific IPs, I overlooked that part. The impacted drivers are the
   timberdale and the DaVinci voice codec ones.
 
  Another option is you could do this for MFD devices:
 
  struct mfd_device {
          struct platform_devce pdev;
          struct mfd_cell *cell;
  };
 
  However, that requires that drivers using the mfd_cell will *never*
  get instantiated outside of the mfd infrastructure, and there is no
  way to protect against this so it is probably a bad idea.
 
  Or, mfd_cell could be added to platform_device directly which would
  *by far* be the safest option at the cost of every platform_device
  having a mostly unused mfd_cell pointer.  Not a significant cost in my
  opinion.
  I thought about this one, but I had the impression people would want to kill
  me for adding an MFD specific pointer to platform_device. I guess it's worth
  giving it a try since it would be a simple and safe solution.
  I'll look at it later this weekend.
 
  Thanks for the input.
 
 [cc'ing gregkh because we're talking about modifying struct platform_device]
 
 I'll back you up on this one.  It is a far better solution than the
 alternatives.  At least with mfd, it covers a large set of devices.  I
 think there is a strong argument for doing this.  Or alternatively,
 the particular interesting fields from mfd_cell could be added to
 platform_device.  What information do child devices need access to?
In some cases, they need the whole cell to clone it. So I'm up for adding an
mfd_cell pointer to the platform_device structure.
Below is a tentative patch. This is a first step and would fix all
regressions. I tried to keep the MFD dependencies as small as possible, which
is why I placed the pdev-mfd_cell building code in mfd-core.c
The second step would be to get rid of mfd_get_data() and have all subdrivers
going back to the regular platform_data way. They would no longer be dependant
on the MFD code except for those who really need it. In that case they could
just call mfd_get_cell() and get full access to their MFD cell.

--- 
 drivers/mfd/mfd-core.c  |   27 ++-
 include/linux/mfd/core.h|7 +--
 include/linux/platform_device.h |5 +
 3 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d01574d..c0fc1c0 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -18,6 +18,21 @@
 #include linux/pm_runtime.h
 #include linux/slab.h
 
+static int mfd_platform_add_cell(struct platform_device *pdev, const struct 
mfd_cell *cell)
+{
+   struct mfd_cell *c;
+
+   if (cell == NULL)
+   return 0;
+
+   c = kmemdup(cell, sizeof(struct mfd_cell), GFP_KERNEL);
+   if (c == NULL)
+   return -ENOMEM;
+
+   pdev-mfd_cell = c;
+   return 0;
+}
+
 int mfd_cell_enable(struct platform_device *pdev)
 {
const struct mfd_cell *cell = mfd_get_cell(pdev);
@@ -75,7 +90,7 @@ static int mfd_add_device(struct device *parent, int id,
 
pdev-dev.parent = parent;
 
-   ret = platform_device_add_data(pdev, cell, sizeof(*cell));
+   ret = mfd_platform_add_cell(pdev, cell);
if (ret)
goto fail_res;
 
@@ -104,17 +119,17 @@ static int mfd_add_device(struct device *parent, int id,
if (!cell-ignore_resource_conflicts) {
ret = acpi_check_resource_conflict(res);
if (ret)
-   goto fail_res;
+   goto fail_cell;
}
}
 
ret = platform_device_add_resources(pdev, res, cell-num_resources);
if (ret)
-   goto fail_res;
+   goto fail_cell;
 
ret = platform_device_add(pdev);
if (ret)
-   goto fail_res;
+   goto fail_cell;
 
if (cell-pm_runtime_no_callbacks)
pm_runtime_no_callbacks(pdev-dev);
@@ -123,7 +138,8 @@ static int mfd_add_device(struct device *parent, int id,
 
return 0

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

Hi Grant,

On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
 On Wed, Feb 02, 2011 at 08:08:12PM -0800, Andres Salomon wrote:
  
  No need to explicitly set the cell's platform_data/data_size.
  
  In this case, move the various platform_data pointers
  to driver_data.  All of the clients which make use of it
  are also changed.
  
  Signed-off-by: Andres Salomon dilin...@queued.net
  ---
   drivers/dma/timb_dma.c   |2 +-
   drivers/gpio/timbgpio.c  |5 +-
   drivers/i2c/busses/i2c-ocores.c  |2 +-
   drivers/i2c/busses/i2c-xiic.c|2 +-
   drivers/media/radio/radio-timb.c |2 +-
   drivers/media/video/timblogiw.c  |2 +-
   drivers/mfd/timberdale.c |   81 
  +-
   drivers/net/ks8842.c |2 +-
   drivers/spi/xilinx_spi.c |2 +-
   9 files changed, 36 insertions(+), 64 deletions(-)
  
  diff --git a/drivers/dma/timb_dma.c b/drivers/dma/timb_dma.c
  index 3b88a4e..aa06ca4 100644
  --- a/drivers/dma/timb_dma.c
  +++ b/drivers/dma/timb_dma.c
  @@ -684,7 +684,7 @@ static irqreturn_t td_irq(int irq, void *devid)
   
   static int __devinit td_probe(struct platform_device *pdev)
   {
  -   struct timb_dma_platform_data *pdata = pdev-dev.platform_data;
  +   struct timb_dma_platform_data *pdata = platform_get_drvdata(pdev);
 
 Hold the phone.  I know this has already been merged, but this isn't correct.
 
 drvdata is under the ownership of the driver, which means it should
 *not* be set when .probe() gets called.  It is for driver private data
 to do with as it needs, usually for internal state.
We didn't merge that version of the patchset, but one getting the
platform_data pointer from mfd_get_data(). That introduces the issue you're
talking about below.


 Gah.  Not all devices instantiated via mfd will be an mfd device,
 which means that the driver may very well expect an *entirely
 different* platform_device pointer; which further means a very high
 potential of incorrectly dereferenced structures (as evidenced by a
 patch series that is not bisectable).  For instance, the xilinx ip
 cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD specific IPs, I
overlooked that part. The impacted drivers are the timberdale and the DaVinci
voice codec ones.
To fix that problem I propose 2 alternatives:

1) When declaring the sub devices cells, the MFD driver should specify an
mfd_data_size value for sub devices that are not MFD specific. It's the MFD
driver responsibility to set the cell properly, and the non MFD specific
drivers are kept MFD agnostic.
See my patch below for the timberdale case.

2) Revert the mfd_get_data() call for getting sub devices platform data
pointers. That was introduced to ease the MFD cell sharing work, so if we take
this route we'll need the cs5535 MFD driver to pass its cells as platform_data
pointer. Andres, can you confirm that this would be fine for the
mfd_clone_cell() routine to keep working ?

Patch for solution 1:


 drivers/mfd/mfd-core.c  |   13 ++---
 drivers/mfd/timberdale.c|   11 +++
 include/linux/mfd/core.h|1 +
 drivers/i2c/busses/i2c-ocores.c |3 +--
 drivers/i2c/busses/i2c-xiic.c   |3 +--
 drivers/net/ks8842.c|3 +--
 drivers/spi/xilinx_spi.c|3 +--
 7 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d01574d..8abe510 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -75,9 +75,16 @@ static int mfd_add_device(struct device *parent, int id,
 
pdev-dev.parent = parent;
 
-   ret = platform_device_add_data(pdev, cell, sizeof(*cell));
-   if (ret)
-   goto fail_res;
+   if (cell-mfd_data_size  0) {
+   ret = platform_device_add_data(pdev,
+   cell-mfd_data, cell-mfd_data_size);
+   if (ret)
+   goto fail_res;
+   } else {
+   ret = platform_device_add_data(pdev, cell, sizeof(*cell));
+   if (ret)
+   goto fail_res;
+   }
 
for (r = 0; r  cell-num_resources; r++) {
res[r].name = cell-resources[r].name;
diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
index 94c6c8a..b4d2d09 100644
--- a/drivers/mfd/timberdale.c
+++ b/drivers/mfd/timberdale.c
@@ -396,6 +396,7 @@ static __devinitdata struct mfd_cell 
timberdale_cells_bar0_cfg0[] = {
.num_resources = ARRAY_SIZE(timberdale_xiic_resources),
.resources = timberdale_xiic_resources,
.mfd_data = timberdale_xiic_platform_data,
+   .mfd_data_size = sizeof(timberdale_xiic_platform_data)
},
{
.name = timb-gpio,
@@ -420,12 +421,14 @@ static __devinitdata struct mfd_cell 
timberdale_cells_bar0_cfg0[] = {
.num_resources 

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Fri, Apr 01, 2011 at 10:47:56AM -0700, Andres Salomon wrote:
 On Fri, 1 Apr 2011 13:20:31 +0200
 Samuel Ortiz sa...@linux.intel.com wrote:
 
  Hi Grant,
  
  On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
 [...]
   Gah.  Not all devices instantiated via mfd will be an mfd device,
   which means that the driver may very well expect an *entirely
   different* platform_device pointer; which further means a very high
   potential of incorrectly dereferenced structures (as evidenced by a
   patch series that is not bisectable).  For instance, the xilinx ip
   cores are used by more than just mfd.
  I agree. Since the vast majority of the MFD subdevices are MFD
  specific IPs, I overlooked that part. The impacted drivers are the
  timberdale and the DaVinci voice codec ones.
 
 Can you please provide pointers to what you're referring to?  The only
 code that I could find that created platform devices prefixed with
 'timb-' or named 'xilinx_spi' was drivers/mfd/timberdale.c.
The xilinx-spi, ocores-i2c, i2c-xiic drivers and to some extend the
ks8842 ethernet driver are generic IPs that the timberdale SOC happens to
use. So I agree it's extremely unlikely that anyone could come up with a
platform that would be re-using e.g. the timb-radio IP, but I think it's less
unikely for more generic IPs such as the xilinx-spi one.


  To fix that problem I propose 2 alternatives:
  
  1) When declaring the sub devices cells, the MFD driver should
  specify an mfd_data_size value for sub devices that are not MFD
  specific. It's the MFD driver responsibility to set the cell
  properly, and the non MFD specific drivers are kept MFD agnostic.
  See my patch below for the timberdale case.
  
  2) Revert the mfd_get_data() call for getting sub devices platform
  data pointers. That was introduced to ease the MFD cell sharing work,
  so if we take this route we'll need the cs5535 MFD driver to pass its
  cells as platform_data pointer. Andres, can you confirm that this
  would be fine for the mfd_clone_cell() routine to keep working ?
 
 It would break mfd_clone_cell, as it uses mfd_get_cell to grab the one
 to clone.  We could change it to accept the cell as an argument.  It
 would also break mfd_cell_enable/disable, of course.
I'm talking about reverting the default behaviour of passing the MFD cell as
the platform data, and going back to the cell definitions setting their
platform_data pointer explicitely. In that case, the cs5535 driver would have
to do something like:

diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
index 155fa04..3e3841d 100644
--- a/drivers/mfd/cs5535-mfd.c
+++ b/drivers/mfd/cs5535-mfd.c
@@ -106,6 +106,7 @@ static __devinitdata struct mfd_cell cs5535_mfd_cells[] =
{
.name = cs5535-acpi,
.num_resources = 1,
.resources = cs5535_mfd_resources[ACPI_BAR],
+   .platform_data = cs5535_mfd_cells[ACPI_BAR],
 
.enable = cs5535_mfd_res_enable,
.disable = cs5535_mfd_res_disable,

mfd_get_cell would then return cs5535_mfd_cells[ACPI_BAR].

This fix would put all sub devices drivers back to an MFD agnostic state,
although the vast majority of them will certainly never be found anywhere else
than in their current MFD SoC. That's why I'm still not sure which way to go
to fix that problem.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
 On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon dilin...@queued.net wrote:
  On Fri, 1 Apr 2011 13:20:31 +0200
  Samuel Ortiz sa...@linux.intel.com wrote:
 
  Hi Grant,
 
  On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
  [...]
   Gah.  Not all devices instantiated via mfd will be an mfd device,
   which means that the driver may very well expect an *entirely
   different* platform_device pointer; which further means a very high
   potential of incorrectly dereferenced structures (as evidenced by a
   patch series that is not bisectable).  For instance, the xilinx ip
   cores are used by more than just mfd.
  I agree. Since the vast majority of the MFD subdevices are MFD
  specific IPs, I overlooked that part. The impacted drivers are the
  timberdale and the DaVinci voice codec ones.
 
 Another option is you could do this for MFD devices:
 
 struct mfd_device {
 struct platform_devce pdev;
 struct mfd_cell *cell;
 };
 
 However, that requires that drivers using the mfd_cell will *never*
 get instantiated outside of the mfd infrastructure, and there is no
 way to protect against this so it is probably a bad idea.
 
 Or, mfd_cell could be added to platform_device directly which would
 *by far* be the safest option at the cost of every platform_device
 having a mostly unused mfd_cell pointer.  Not a significant cost in my
 opinion.
I thought about this one, but I had the impression people would want to kill
me for adding an MFD specific pointer to platform_device. I guess it's worth
giving it a try since it would be a simple and safe solution.
I'll look at it later this weekend.

Thanks for the input.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v21 1/3] MFD: WL1273 FM Radio: MFD driver for the FM radio.

[Samuel Ortiz]

Hi Matti,

On Tue, Mar 01, 2011 at 10:00:48AM +0200, Matti J. Aaltonen wrote:
 This is the core of the WL1273 FM radio driver, it connects
 the two child modules. The two child drivers are
 drivers/media/radio/radio-wl1273.c and sound/soc/codecs/wl1273.c.
 
 The radio-wl1273 driver implements the V4L2 interface and communicates
 with the device. The ALSA codec offers digital audio, without it only
 analog audio is available.

Acked-by: Samuel Ortiz sa...@linux.intel.com

Mauro, I suppose you're taking this one ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v22 3/3] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Samuel Ortiz]

On Tue, Mar 01, 2011 at 03:44:45PM +0200, Matti J. Aaltonen wrote:
 On Tue, 2011-03-01 at 13:23 +, ext Mark Brown wrote:
  On Tue, Mar 01, 2011 at 03:10:37PM +0200, Matti J. Aaltonen wrote:
   These changes are needed to keep up with the changes in the
   MFD core and V4L2 parts of the wl1273 FM radio driver.
   
   Use function pointers instead of exported functions for I2C IO.
   Also move all preprocessor constants from the wl1273.h to
   include/linux/mfd/wl1273-core.h.
   
   Also update the year in the copyright statement.
   
   Signed-off-by: Matti J. Aaltonen matti.j.aalto...@nokia.com
  
  Acked-by: Mark Brown broo...@opensource.wolfsonmicro.com
  
  *Please* keep acks unless you're making substantial changes to
  repostings.
 
 OK, I see, I should have added the ACKs to the relevant driver files
 instead of copying them to the cover letter...
Yes, it makes it easier for the maintainer taking your patches.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v20 1/3] MFD: Wl1273 FM radio core: Add I2C IO functions.

[Samuel Ortiz]

Hi Matti,

On Mon, Feb 28, 2011 at 01:02:29PM +0200, Matti J. Aaltonen wrote:
 Add I2C IO functions.
 Change the IO operation from read to write in wl1273_fm_set_volume.
 Update the year of the copyright statement.
 Remove two unnecessary calls to i2c_set_clientdata.
Provided that you add a changelog relevant to the patch itself, and not to the
v1-v2 diff:
Acked-by: Samuel Ortiz sa...@linux.intel.com

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v19 1/3] MFD: WL1273 FM Radio: MFD driver for the FM radio.

[Samuel Ortiz]

Hi Matti

On Tue, Feb 15, 2011 at 10:13:44AM +0200, Matti J. Aaltonen wrote:
 This is the core of the WL1273 FM radio driver, it connects
 the two child modules. The two child drivers are
 drivers/media/radio/radio-wl1273.c and sound/soc/codecs/wl1273.c.
 
 The radio-wl1273 driver implements the V4L2 interface and communicates
 with the device. The ALSA codec offers digital audio, without it only
 analog audio is available.
The driver looks fine, but for Mauro to take this one you'd have to provide a
diff against the already existing wl1273-core.

I have some minor comments:

 diff --git a/drivers/mfd/wl1273-core.c b/drivers/mfd/wl1273-core.c
 new file mode 100644
 index 000..66e0ac9
 --- /dev/null
 +++ b/drivers/mfd/wl1273-core.c
 @@ -0,0 +1,295 @@
 +/*
 + * MFD driver for wl1273 FM radio and audio codec submodules.
 + *
 + * Copyright (C) 2010 Nokia Corporation
2011.


 +/**
 + * wl1273_fm_set_volume() -  Set volume.
 + * @core:A pointer to the device struct.
 + * @volume:  The new volume value.
 + */
 +static int wl1273_fm_set_volume(struct wl1273_core *core, unsigned int 
 volume)
 +{
 + u16 val;
 + int r;
 +
 + if (volume  WL1273_MAX_VOLUME)
 + return -EINVAL;
 +
 + if (core-volume == volume)
 + return 0;
 +
 + val = volume;
 + r = wl1273_fm_read_reg(core, WL1273_VOLUME_SET, val);
 + if (r)
 + return r;
 +
 + core-volume = volume;
 + return 0;
 +}
I'm confused with this one: Isn't WL1273_VOLUME_SET a command ? Also, how can
reading from it set the volume ?


 +static int wl1273_core_remove(struct i2c_client *client)
 +{
 + struct wl1273_core *core = i2c_get_clientdata(client);
 +
 + dev_dbg(client-dev, %s\n, __func__);
 +
 + mfd_remove_devices(client-dev);
 + i2c_set_clientdata(client, NULL)
Not needed.

 +err:
 + i2c_set_clientdata(client, NULL);
Ditto.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v19 0/3] TI Wl1273 FM radio driver.

[Samuel Ortiz]

Hi Mauro,

On Tue, Feb 15, 2011 at 05:59:52PM -0200, Mauro Carvalho Chehab wrote:
 Em 15-02-2011 06:13, Matti J. Aaltonen escreveu:
  Hello.
  
  Now I've refactored the code so that the I2C I/O functions are in the 
  MFD core. Also now the codec can be compiled without compiling the V4L2
  driver.
  
  I haven't implemented the audio routing (yet), but I've added a TODO
  comment about it in the codec file.
 
 Matti,
 
 It looks ok on my eyes. As most of the changes is at the V4L part, it is
 probably better to merge this patch via my tree.
I'm fine with that, yes. I'll add my Acked-by once Matti has fixed the minor
issues I found.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: WL1273 FM Radio driver...

[Samuel Ortiz]

Hi Mauro,

On Wed, Feb 02, 2011 at 01:35:01PM -0200, Mauro Carvalho Chehab wrote:
 Em 30-01-2011 21:23, Samuel Ortiz escreveu:
  Hi Matti,
  
  On Tue, Jan 18, 2011 at 05:04:23PM +0200, Matti J. Aaltonen wrote:
  Hello
 
  I have been trying to get the WL1273 FM radio driver into the kernel for
  some time. It has been kind of difficult, one of the reasons is that I
  didn't realize I should have tried to involve all relevant maintainers
  to the discussion form the beginning (AsoC, Media and MFD). At Mark's
  suggestion I'm trying to reopen the discussion now.
 
  The driver consists of an MFD core and two child drivers (the audio
  codec and the V4L2 driver). And the question is mainly about the role of
  the MFD driver: the original design had the IO functions in the core.
  Currently the core is practically empty mainly because Mauro very
  strongly wanted to have “everything” in the V4L2 driver.
  What was Mauro main concerns with having the IO part in the core ?
  A lot of MFD drivers are going that path already.
 
 My concerns is that the V4L2-specific part of the code should be at 
 drivers/media.
 I prefer that the specific MFD I/O part to be at drivers/mfd, just like
 the other drivers.
Agreed, but it seems that's not the case currently. Would you be ok with Matti
refactoring those 2 drivers a bit so that the actual core I/O parts should be
handled by the MFD driver ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: WL1273 FM Radio driver...

[Samuel Ortiz]

Hi Matti,

On Tue, Jan 18, 2011 at 05:04:23PM +0200, Matti J. Aaltonen wrote:
 Hello
 
 I have been trying to get the WL1273 FM radio driver into the kernel for
 some time. It has been kind of difficult, one of the reasons is that I
 didn't realize I should have tried to involve all relevant maintainers
 to the discussion form the beginning (AsoC, Media and MFD). At Mark's
 suggestion I'm trying to reopen the discussion now.
 
 The driver consists of an MFD core and two child drivers (the audio
 codec and the V4L2 driver). And the question is mainly about the role of
 the MFD driver: the original design had the IO functions in the core.
 Currently the core is practically empty mainly because Mauro very
 strongly wanted to have “everything” in the V4L2 driver.
What was Mauro main concerns with having the IO part in the core ?
A lot of MFD drivers are going that path already.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v18 1/2] MFD: WL1273 FM Radio: MFD driver for the FM radio.

[Samuel Ortiz]

Hi Matti,

On Fri, Dec 10, 2010 at 04:41:33PM +0200, Matti J. Aaltonen wrote:
 This is the core of the WL1273 FM radio driver, it connects
 the two child modules. The two child drivers are
 drivers/media/radio/radio-wl1273.c and sound/soc/codecs/wl1273.c.
 
 The radio-wl1273 driver implements the V4L2 interface and communicates
 with the device. The ALSA codec offers digital audio, without it only
 analog audio is available.
 
 Signed-off-by: Matti J. Aaltonen matti.j.aalto...@nokia.com
Acked-by: Samuel Ortiz sa...@linux.intel.com

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 1/2] MFD: WL1273 FM Radio: MFD driver for the FM radio.

[Samuel Ortiz]

Hi Matti,

On Fri, Dec 03, 2010 at 05:02:27PM +0200, Matti J. Aaltonen wrote:
 This is the core of the WL1273 FM radio driver, it connects
 the two child modules. The two child drivers are
 drivers/media/radio/radio-wl1273.c and sound/soc/codecs/wl1273.c.
 
 The radio-wl1273 driver implements the V4L2 interface and communicates
 with the device. The ALSA codec offers digital audio, without it only
 analog audio is available.
The driver looks much better now. If that goes through Mauro's tree, please
add my: Acked-by: Samuel Ortiz sa...@linux.intel.com

I just have one nitpick comment here:

 diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
 index f54b365..b59be8b 100644
 --- a/drivers/mfd/Makefile
 +++ b/drivers/mfd/Makefile
 @@ -81,3 +81,4 @@ obj-$(CONFIG_MFD_JANZ_CMODIO)   += janz-cmodio.o
  obj-$(CONFIG_MFD_JZ4740_ADC) += jz4740-adc.o
  obj-$(CONFIG_MFD_TPS6586X)   += tps6586x.o
  obj-$(CONFIG_MFD_VX855)  += vx855.o
 +obj-$(CONFIG_MFD_WL1273_CORE)+= wl1273-core.o
 \ No newline at end of file
Please add a new line here.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v16 1/2] MFD: WL1273 FM Radio: MFD driver for the FM radio.

[Samuel Ortiz]

Hi Matti,

On Wed, Nov 17, 2010 at 03:42:00PM +0200, Matti J. Aaltonen wrote:
 This is the core of the WL1273 FM radio driver, it connects
 the two child modules.
 
 This is a parent for two child drivers:
 drivers/media/radio/radio-wl1273.c and sound/soc/codecs/wl1273.c
 
 Radio-wl1273 implements the V4L2 interface and the communication to the 
 device.
 The ALSA codec is for digital audio. Without the codec only analog audio
 is available.
 
 Signed-off-by: Matti J. Aaltonen matti.j.aalto...@nokia.com
My review:

 +config WL1273_CORE
 + tristate
 + depends on I2C
 + select MFD_CORE
 + default n
 +
You need to be a lot more verbose here. Nobody knows what this wl1273 core
driver is for.
Also, please rename your config to MFD_WL1273_CORE.


 +
 +#include linux/delay.h
Not needed.


 +#include linux/mfd/wl1273-core.h
 +#include linux/slab.h
 +#include media/v4l2-common.h
Not needed neither.


 +#define DRIVER_DESC WL1273 FM Radio Core
 +
 +static struct i2c_device_id wl1273_driver_id_table[] = {
 + { WL1273_FM_DRIVER_NAME, 0 },
 + { }
 +};
 +MODULE_DEVICE_TABLE(i2c, wl1273_driver_id_table);
 +
 +static int wl1273_core_remove(struct i2c_client *client)
 +{
 + struct wl1273_core *core = i2c_get_clientdata(client);
 +
 + dev_dbg(client-dev, %s\n, __func__);
 +
 + mfd_remove_devices(client-dev);
 + i2c_set_clientdata(client, NULL);
 + kfree(core);
 +
 + return 0;
 +}
 +
 +static int __devinit wl1273_core_probe(struct i2c_client *client,
 +const struct i2c_device_id *id)
 +{
 + struct wl1273_fm_platform_data *pdata = client-dev.platform_data;
 + int r = 0;
 + struct wl1273_core *core;
 + int children = 0;
 +
 + dev_dbg(client-dev, %s\n, __func__);
 +
 + if (!pdata) {
 + dev_err(client-dev, No platform data.\n);
 + return -EINVAL;
 + }
 +
 + core = kzalloc(sizeof(*core), GFP_KERNEL);
 + if (!core)
 + return -ENOMEM;
 +
 + core-pdata = pdata;
 + core-client = client;
 + mutex_init(core-lock);
 +
 + i2c_set_clientdata(client, core);
 +
 + /* the radio child must be first */
 + if (pdata-children  WL1273_RADIO_CHILD) {
 + struct mfd_cell *cell = core-cells[children];
Please add a line between your variable declarations and the rest of the code.


 + dev_dbg(client-dev, %s: Have V4L2.\n, __func__);
 +
 + cell-name = wl1273_fm_radio;
 + cell-platform_data = core;
 + cell-data_size = sizeof(core);
 + children++;
 + } else {
 + dev_err(client-dev, Cannot function without radio child.\n);
 + r = -EINVAL;
 + goto err_radio_child;
 + }
So I'd prefer something like:

if (!pdata-children  WL1273_RADIO_CHILD) {
dev_err(...);
return -EINVAL;
}

before you actually allocate the core pointer.

 + if (pdata-children  WL1273_CODEC_CHILD) {
 + struct mfd_cell *cell = core-cells[children];
A new line here as well, please.


 + dev_dbg(client-dev, %s: Have codec.\n, __func__);
 + cell-name = wl1273-codec;
 + cell-platform_data = core;
 + cell-data_size = sizeof(core);
 + children++;
 + }
 +
 + if (children) {
 + dev_dbg(client-dev, %s: Have children.\n, __func__);
 + r = mfd_add_devices(client-dev, -1, core-cells,
 + children, NULL, 0);
 + } else {
 + dev_err(client-dev, No platform data found for children.\n);
 + r = -ENODEV;
 + }
 +
 + if (!r)
 + return 0;
I'd prefere:

if (children == 0) {
dev_err();
r = -ENODEV;
goto err;
}


dev_dbg();

return mfd_add_devices();

err:



 +err_radio_child:
 + i2c_set_clientdata(client, NULL);
 + pdata-free_resources();
 + kfree(core);
 +
 + dev_dbg(client-dev, %s\n, __func__);
 +
 + return r;
 +}
 +
 +static struct i2c_driver wl1273_core_driver = {
 + .driver = {
 + .name = WL1273_FM_DRIVER_NAME,
 + },
 + .probe = wl1273_core_probe,
 + .id_table = wl1273_driver_id_table,
 + .remove = __devexit_p(wl1273_core_remove),
 +};
 +
 +static int __init wl1273_core_init(void)
 +{
 + int r;
 +
 + r = i2c_add_driver(wl1273_core_driver);
 + if (r) {
 + pr_err(WL1273_FM_DRIVER_NAME
 +: driver registration failed\n);
 + return r;
 + }
 +
 + return 0;
Here you can just return r.



 +}
 +
 +static void __exit wl1273_core_exit(void)
 +{
 + flush_scheduled_work();
What is that for ?



 + i2c_del_driver(wl1273_core_driver);
 +}
 +late_initcall(wl1273_core_init);
 +module_exit(wl1273_core_exit);
 +
 +MODULE_AUTHOR(Matti Aaltonen matti.j.aalto...@nokia.com);
 +MODULE_DESCRIPTION(DRIVER_DESC);
 +MODULE_LICENSE(GPL);
 diff --git 

Re: Remaining BKL users, what to do

[Samuel Ortiz]

On Thu, 2010-09-16 at 16:32 +0200, Arnd Bergmann wrote:
 net/appletalk:
 net/ipx/af_ipx.c:
 net/irda/af_irda.c:
   Can probably be saved from retirement in drivers/staging if the
   maintainers still care.
I'll take care of the IrDA part.

Cheers,
Samuel.


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND][PATCH 0/2] media, mfd: Add timberdale video-in driver

[Samuel Ortiz]

Hi Mauro,

On Wed, Sep 08, 2010 at 04:39:42PM -0300, Mauro Carvalho Chehab wrote:
 Em 02-09-2010 08:56, Richard Röjfors escreveu:
  To follow are two patches.
  
  The first adds the timberdale video-in driver to the media tree.
  
  The second adds it to the timberdale MFD driver.
  
  Samuel and Mauro hope you can support and solve the potential merge
  issue between your two trees.
 
 If the conflicts are trivial, I can handle when merging upstream.
Since the mfd part of this patchset has a build time dependency on the media
part, would you mind pushing both patches upstream through your tree ?
I'll ACK the mfd part.

Cheers,
Samuel.


  
  Thanks
  --Richard
  
 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v2] mfd: Add timberdale video-in driver to timberdale

[Samuel Ortiz]

Hi Richard,

On Fri, Sep 10, 2010 at 11:29:35AM +0200, Richard Röjfors wrote:
 This patch defines platform data for the video-in driver
 and adds it to all configurations of timberdale.
 
 Signed-off-by: Richard Röjfors richard.rojf...@pelagicore.com
Acked-by: Samuel Ortiz sa...@linux.intel.com

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] media, mfd: Add timberdale video-in driver

[Samuel Ortiz]

Hi Richard,

On Fri, Apr 16, 2010 at 06:27:54PM +0200, Richard Röjfors wrote:
 To follow are two patches.
 
 The first one adds the timberdale video-in driver to the media tree.
 
 The second one adds it to the timberdale MFD driver.
 
 The Kconfig of the media patch selects TIMB_DMA which is introduced
 in the DMA tree, that's why I cc:d in Dan.
 
 Samuel and Mauro hope you can support and solve the potential merge
 issue between your two trees.
Mauro, the mfd part of this patch depends on the video one. Do you mind if I
take both through my tree, after getting your Acked-by ?

Cheers,
Samuel.


 Thanks
 --Richard
 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/1] mfd: Add support for the timberdale FPGA.

[Samuel Ortiz]

Hi Richard,

On Thu, Feb 04, 2010 at 12:18:52PM +0100, Richard Röjfors wrote:
 The timberdale FPGA is found on the Intel in-Vehicle Infotainment reference 
 board
 russelville.
 
 The driver is a PCI driver which chunks up the I/O memory and distributes 
 interrupts
 to a number of platform devices for each IP inside the FPGA.
 
 Signed-off-by: Richard Röjfors richard.rojf...@pelagicore.com
The patch looks fine to me:
Signed-off-by: Samuel Ortiz sa...@linux.intel.com

Cheers,
Samuel.


 ---
 diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
 index 8782978..f92673b 100644
 --- a/drivers/mfd/Kconfig
 +++ b/drivers/mfd/Kconfig
 @@ -348,6 +348,17 @@ config AB4500_CORE
 read/write functions for the devices to get access to this chip.
 This chip embeds various other multimedia funtionalities as well.
 
 +config MFD_TIMBERDALE
 + tristate Support for the Timberdale FPGA
 + select MFD_CORE
 + depends on PCI
 + ---help---
 + This is the core driver for the timberdale FPGA. This device is a
 + multifunction device which exposes numerous platform devices.
 +
 + The timberdale FPGA can be found on the Intel Atom development board
 + for in-vehicle infontainment, called Russellville.
 +
endmenu
 
menu Multimedia Capabilities Port drivers
 diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
 index e09eb48..53375ac 100644
 --- a/drivers/mfd/Makefile
 +++ b/drivers/mfd/Makefile
 @@ -55,4 +55,6 @@ obj-$(CONFIG_AB3100_CORE)   += ab3100-core.o
obj-$(CONFIG_AB3100_OTP)   += ab3100-otp.o
obj-$(CONFIG_AB4500_CORE)  += ab4500-core.o
obj-$(CONFIG_MFD_88PM8607) += 88pm8607.o
 -obj-$(CONFIG_PMIC_ADP5520)   += adp5520.o
 \ No newline at end of file
 +obj-$(CONFIG_PMIC_ADP5520)   += adp5520.o
 +
 +obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
 diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
 new file mode 100644
 index 000..603cf06
 --- /dev/null
 +++ b/drivers/mfd/timberdale.c
 @@ -0,0 +1,663 @@
 +/*
 + * timberdale.c timberdale FPGA MFD driver
 + * Copyright (c) 2009 Intel Corporation
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 + */
 +
 +/* Supports:
 + * Timberdale FPGA
 + */
 +
 +#include linux/kernel.h
 +#include linux/module.h
 +#include linux/pci.h
 +#include linux/msi.h
 +#include linux/mfd/core.h
 +
 +#include linux/timb_gpio.h
 +
 +#include linux/i2c.h
 +#include linux/i2c-ocores.h
 +#include linux/i2c/tsc2007.h
 +
 +#include linux/spi/spi.h
 +#include linux/spi/xilinx_spi.h
 +#include linux/spi/max7301.h
 +#include linux/spi/mc33880.h
 +
 +#include timberdale.h
 +
 +#define DRIVER_NAME timberdale
 +
 +struct timberdale_device {
 + resource_size_t ctl_mapbase;
 + unsigned char __iomem   *ctl_membase;
 + struct {
 + u32 major;
 + u32 minor;
 + u32 config;
 + } fw;
 +};
 +
 +/*--*/
 +
 +static struct tsc2007_platform_data timberdale_tsc2007_platform_data = {
 + .model = 2003,
 + .x_plate_ohms = 100
 +};
 +
 +static struct i2c_board_info timberdale_i2c_board_info[] = {
 + {
 + I2C_BOARD_INFO(tsc2007, 0x48),
 + .platform_data = timberdale_tsc2007_platform_data,
 + .irq = IRQ_TIMBERDALE_TSC_INT
 + },
 +};
 +
 +static __devinitdata struct ocores_i2c_platform_data
 +timberdale_ocores_platform_data = {
 + .regstep = 4,
 + .clock_khz = 62500,
 + .devices = timberdale_i2c_board_info,
 + .num_devices = ARRAY_SIZE(timberdale_i2c_board_info)
 +};
 +
 +const static __devinitconst struct resource timberdale_ocores_resources[] = {
 + {
 + .start  = OCORESOFFSET,
 + .end= OCORESEND,
 + .flags  = IORESOURCE_MEM,
 + },
 + {
 + .start  = IRQ_TIMBERDALE_I2C,
 + .end= IRQ_TIMBERDALE_I2C,
 + .flags  = IORESOURCE_IRQ,
 + },
 +};
 +
 +const struct max7301_platform_data timberdale_max7301_platform_data = {
 + .base = 200
 +};
 +
 +const struct mc33880_platform_data timberdale_mc33880_platform_data = {
 + .base = 100
 +};
 +
 +static struct spi_board_info timberdale_spi_16bit_board_info[] = {
 + {
 + .modalias = max7301,
 + .max_speed_hz = 26000,
 + .chip_select = 2,
 + .mode = SPI_MODE_0

Re: [PATCH] mfd: Add support for the timberdale FPGA.

[Samuel Ortiz]

Hi Mauro,

On Wed, Feb 03, 2010 at 09:16:07AM +, Mauro Carvalho Chehab wrote:
 Hi Richard,
 
 Richard Röjfors wrote:
  The timberdale FPGA is found on the Intel in-Vehicle Infotainment reference 
  board
  russelville.
 
  The driver is a PCI driver which chunks up the I/O memory and distributes 
  interrupts
  to a number of platform devices for each IP inside the FPGA.
 
  Signed-off-by: Richard Röjfors richard.rojf...@pelagicore.com
 
 I'm not sure how to deal with this patch. It doesn't contain anything related
 to V4L2 inside it, nor it applies to drivers/media, but it depends on the 
 radio-timb
 driver that you submitted us.
Actually, there are no build dependencies between those 2 drivers, which is
very good.


 As this patch will be committed at mfd tree, the better is if Samuel can 
 review
 this patch and give his ack. I'll add it together with the V4L2 driver and 
 submit them
 via my tree.
I'm going to review this patch right now. Typically, mfd core drivers and
their subdevices are submitted as a patchset via my tree, because the
subdevices drivers have dependencies with the core. The mfd core driver is
submitted without any dependencies, and then when the subdevices drivers are
submitted, we add relevant code to the mfd driver. This way we prevent any
build breakage or bisection mess.
The timberdale chip right now doesnt depend on anything, but will soon depend
on the radio driver that's on your tree, but also on a sound and on a network
driver. You'd have to take all those if Richard wants to push them right now.
So, what I propose is to take the timberdale mfd core driver and the radio
one, with your SOB. Then when Richard wants to submit additional subdevices
drivers I'll be able to take them and we'll avoid polluting your tree with non
media related drivers. Does that make sense to you ?

Cheers,
Samuel.

 
 Samuel,
 
 Would this way work for you?
 
 Cheers,
 Mauro.
 
  ---
  diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
  index 8782978..f92673b 100644
  --- a/drivers/mfd/Kconfig
  +++ b/drivers/mfd/Kconfig
  @@ -348,6 +348,17 @@ config AB4500_CORE
  read/write functions for the devices to get access to this chip.
  This chip embeds various other multimedia funtionalities as well.
 
  +config MFD_TIMBERDALE
  + tristate Support for the Timberdale FPGA
  + select MFD_CORE
  + depends on PCI
  + ---help---
  + This is the core driver for the timberdale FPGA. This device is a
  + multifunction device which exposes numerous platform devices.
  +
  + The timberdale FPGA can be found on the Intel Atom development board
  + for in-vehicle infontainment, called Russellville.
  +
   endmenu
 
   menu Multimedia Capabilities Port drivers
  diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
  index e09eb48..53375ac 100644
  --- a/drivers/mfd/Makefile
  +++ b/drivers/mfd/Makefile
  @@ -55,4 +55,6 @@ obj-$(CONFIG_AB3100_CORE)   += ab3100-core.o
   obj-$(CONFIG_AB3100_OTP) += ab3100-otp.o
   obj-$(CONFIG_AB4500_CORE)+= ab4500-core.o
   obj-$(CONFIG_MFD_88PM8607)   += 88pm8607.o
  -obj-$(CONFIG_PMIC_ADP5520)   += adp5520.o
  \ No newline at end of file
  +obj-$(CONFIG_PMIC_ADP5520)   += adp5520.o
  +
  +obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
  diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
  new file mode 100644
  index 000..90f745b
  --- /dev/null
  +++ b/drivers/mfd/timberdale.c
  @@ -0,0 +1,667 @@
  +/*
  + * timberdale.c timberdale FPGA MFD driver
  + * Copyright (c) 2009 Intel Corporation
  + *
  + * This program is free software; you can redistribute it and/or modify
  + * it under the terms of the GNU General Public License version 2 as
  + * published by the Free Software Foundation.
  + *
  + * This program is distributed in the hope that it will be useful,
  + * but WITHOUT ANY WARRANTY; without even the implied warranty of
  + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  + * GNU General Public License for more details.
  + *
  + * You should have received a copy of the GNU General Public License
  + * along with this program; if not, write to the Free Software
  + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  + */
  +
  +/* Supports:
  + * Timberdale FPGA
  + */
  +
  +#include linux/kernel.h
  +#include linux/module.h
  +#include linux/pci.h
  +#include linux/msi.h
  +#include linux/mfd/core.h
  +
  +#include linux/timb_gpio.h
  +
  +#include linux/i2c.h
  +#include linux/i2c-ocores.h
  +#include linux/i2c/tsc2007.h
  +
  +#include linux/spi/spi.h
  +#include linux/spi/xilinx_spi.h
  +#include linux/spi/max7301.h
  +#include linux/spi/mc33880.h
  +
  +#include timberdale.h
  +
  +#define DRIVER_NAME timberdale
  +
  +struct timberdale_device {
  + resource_size_t ctl_mapbase;
  + unsigned char __iomem   *ctl_membase;
  + struct {
  + u32 major;
  + u32 minor;
  + u32 config;
  + } fw;
 

Re: [PATCH] mfd: Add support for the timberdale FPGA.

[Samuel Ortiz]

On Wed, Feb 03, 2010 at 10:18:17AM +, Mauro Carvalho Chehab wrote:
 Samuel Ortiz wrote:
  I'm going to review this patch right now. Typically, mfd core drivers and
  their subdevices are submitted as a patchset via my tree, because the
  subdevices drivers have dependencies with the core. The mfd core driver is
  submitted without any dependencies, and then when the subdevices drivers are
  submitted, we add relevant code to the mfd driver. This way we prevent any
  build breakage or bisection mess.
 
 The drivers at media are generally very complex with dependencies on several 
 other
 subsystems. In general, almost all depends on i2c, alsa and input, and an 
 increasing
 number of drivers has also dependencies with platform_data added at 
 arch-dependent
 includes/drivers. Yet, this specific driver is simple.
 
 I generally tend to add those drivers via my tree, since it is generally 
 simpler to
 prevent breakage/bisection troubles, 
I see that we have similar issues :)



 but it is also ok for me if you want to add
 them via your tree, after I get my ack.
Great, let's to that then.


  The timberdale chip right now doesnt depend on anything, but will soon 
  depend
  on the radio driver that's on your tree, but also on a sound and on a 
  network
  driver. You'd have to take all those if Richard wants to push them right 
  now.
 
 There were one or two minor changes requested on radio-timb patchset. After 
 that
 changes, we're ready to merge it.
All right, I'd appreciate if you could cc me on the relevant thread.



  So, what I propose is to take the timberdale mfd core driver and the radio
  one, with your SOB. Then when Richard wants to submit additional subdevices
  drivers I'll be able to take them and we'll avoid polluting your tree with 
  non
  media related drivers. Does that make sense to you ?
 
 Yes, it does. I don't think Richard is urging with those patches, so my idea 
 is
 to keep them for linux-next. It would equally work for me if you add the 
 patches
 on your tree after my ack. The only merge conflicts we may expect from V4L 
 side
 are related to Kconfig/Makefile, and those will be easy to fix during the 
 merge
 period.
Ok, thanks again for your understanding. This is definitely material for the
next merge window, so I'll merge it into my for-next branch.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
-
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: Les Montalets- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] dvb/dvb-usb: prepare for FIRMWARE_NAME_MAX removal

[Samuel Ortiz]

On Tue, May 26, 2009 at 02:32:45PM -0400, Michael Krufky wrote:
 On Tue, May 26, 2009 at 1:40 PM, Samuel Ortiz sa...@linux.intel.com wrote:
  From: Samuel Ortiz sa...@linux.intel.com
  To: Mauro Carvalho Chehab mche...@infradead.org
 
  We're going to remove the FIRMWARE_NAME_MAX definition in order to avoid any
  firmware name length restriction.
  This patch changes the dvb_usb_device_properties firmware field accordingly.
 
  Signed-off-by: Samuel Ortiz sa...@linux.intel.com
 
  ---
   drivers/media/dvb/dvb-usb/dvb-usb.h |    2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
 
  Index: iwm-2.6/drivers/media/dvb/dvb-usb/dvb-usb.h
  ===
  --- iwm-2.6.orig/drivers/media/dvb/dvb-usb/dvb-usb.h    2009-05-26 
  17:24:36.0 +0200
  +++ iwm-2.6/drivers/media/dvb/dvb-usb/dvb-usb.h 2009-05-26 
  17:25:19.0 +0200
  @@ -196,7 +196,7 @@ struct dvb_usb_device_properties {
   #define CYPRESS_FX2     3
         int        usb_ctrl;
         int        (*download_firmware) (struct usb_device *, const struct 
  firmware *);
  -       const char firmware[FIRMWARE_NAME_MAX];
  +       const char *firmware;
         int        no_reconnect;
 
         int size_of_priv;
 
  --
  Intel Open Source Technology Centre
  http://oss.intel.com/
  --
  To unsubscribe from this list: send the line unsubscribe linux-kernel in
  the body of a message to majord...@vger.kernel.org
  More majordomo info at  http://vger.kernel.org/majordomo-info.html
  Please read the FAQ at  http://www.tux.org/lkml/
 
 
 Samuel,
 
 Your patch makes the following change:
 
 -   const char firmware[FIRMWARE_NAME_MAX];
 +   const char *firmware;
 
 Before your change, struct dvb_usb_device_properties actually contains
 memory allocated for the firmware filename.  After your change, this
 is nothing but a pointer.
 
 This will cause an OOPS.
No, not if it's correctly initialized, as it seems to be for all the
dvb_usb_device_properties users right now.
Typically, you'd initialize your dvb_usb_device_properties like this:

static struct dvb_usb_device_properties a800_properties = {
.caps = DVB_USB_IS_AN_I2C_ADAPTER,

.usb_ctrl = CYPRESS_FX2,
.firmware = dvb-usb-avertv-a800-02.fw,
[...]

And that's fine.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html