Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
On Thu, 12 Jan 2012, Cousson, Benoit wrote:
> On 1/11/2012 4:22 PM, Paul Walmsley wrote:
> > On Wed, 11 Jan 2012, Cousson, Benoit wrote:
> >
> > > Something that puzzle me on that point is most architecture does not use
> > > plateform_device and thus does not have any pdata to hack some custom
> > > function
> > > pointers here an there.
> > > It means that there is probably some better solution to handle that.
> >
> > PCI and USB have their own bus-level device reset functions that are
> > exported from bus code that lives under drivers/. In a similar fashion,
> > we could export omap_device_reset() from our arch/arm/plat-omap code.
> > But that would require that arch/arm/plat-omap and arch/arm/mach-omap2
> > code to be compiled for any ARM build that includes i2c-omap.c. This
> > would be good to avoid, under the principle that drivers should be
> > buildable for any architecture. (This is also why we wish to remove
> > omap_{read,write}*() and cpu_is_omap() from any drivers which contain
> > those.)
> >
> > The intention a few years ago was to eventually create a a real omap_bus
> > and omap_device, with the bus/device integration code living under
> > drivers. In such a situation, we'd be able to use something like the
> > PCI/USB approach cleanly, since there wouldn't be dependencies to arch/
> > code.
>
> For reset, maybe the custom bus is a better approach, since it is something
> that is handled by the OMAP core infrastructure.
> I was referring more to all the other function pointer we have today in driver
> like MMC, DSS that does not necessarily belong to a OMAP core stuff.
Yes, that's right. In general, cross-device dependencies with no
infrastructural consequences should be handled by EXPORT_SYMBOL() between
driver code.
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
On 1/11/2012 4:22 PM, Paul Walmsley wrote:
On Wed, 11 Jan 2012, Cousson, Benoit wrote:
Something that puzzle me on that point is most architecture does not use
plateform_device and thus does not have any pdata to hack some custom function
pointers here an there.
It means that there is probably some better solution to handle that.
PCI and USB have their own bus-level device reset functions that are
exported from bus code that lives under drivers/. In a similar fashion,
we could export omap_device_reset() from our arch/arm/plat-omap code.
But that would require that arch/arm/plat-omap and arch/arm/mach-omap2
code to be compiled for any ARM build that includes i2c-omap.c. This
would be good to avoid, under the principle that drivers should be
buildable for any architecture. (This is also why we wish to remove
omap_{read,write}*() and cpu_is_omap() from any drivers which contain
those.)
The intention a few years ago was to eventually create a a real omap_bus
and omap_device, with the bus/device integration code living under
drivers. In such a situation, we'd be able to use something like the
PCI/USB approach cleanly, since there wouldn't be dependencies to arch/
code.
For reset, maybe the custom bus is a better approach, since it is
something that is handled by the OMAP core infrastructure.
I was referring more to all the other function pointer we have today in
driver like MMC, DSS that does not necessarily belong to a OMAP core stuff.
AFAIU, the way PPC is handling that is by splitting the driver in a generic
part and in a platform specific part.
You can then add any OMAP specific hacks in the OMAP specific part while
keeping most of generic code in a generic driver.
In this case, we just have to implement a small shim that will handle the OMAP
specific code for the reset. The generic driver will handle the rest.
Maybe I'm missing some important details, but that does seems easily doable
for the i2c case.
Hmm. How do those drivers call the platform-specific code in a way that
allows those drivers to be built for non-PPC platforms?
Well, the point is that a lot of that code should not be in the omap
arch directory at the first place.
For example, the MMC is using extensively function pointers because of
the control module dependency.
Having a control module driver will allow stacking the drivers without
having to get any code in mach-omap directory.
Otherwise, there is as well a dedicated drivers/platform directory that
can store some platform specific drivers. So far it looks like it is
used for x86 board kind of drivers only.
Regards,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
On Wed, 11 Jan 2012, Cousson, Benoit wrote:
> Something that puzzle me on that point is most architecture does not use
> plateform_device and thus does not have any pdata to hack some custom function
> pointers here an there.
> It means that there is probably some better solution to handle that.
PCI and USB have their own bus-level device reset functions that are
exported from bus code that lives under drivers/. In a similar fashion,
we could export omap_device_reset() from our arch/arm/plat-omap code.
But that would require that arch/arm/plat-omap and arch/arm/mach-omap2
code to be compiled for any ARM build that includes i2c-omap.c. This
would be good to avoid, under the principle that drivers should be
buildable for any architecture. (This is also why we wish to remove
omap_{read,write}*() and cpu_is_omap() from any drivers which contain
those.)
The intention a few years ago was to eventually create a a real omap_bus
and omap_device, with the bus/device integration code living under
drivers. In such a situation, we'd be able to use something like the
PCI/USB approach cleanly, since there wouldn't be dependencies to arch/
code.
> AFAIU, the way PPC is handling that is by splitting the driver in a generic
> part and in a platform specific part.
>
> You can then add any OMAP specific hacks in the OMAP specific part while
> keeping most of generic code in a generic driver.
>
> In this case, we just have to implement a small shim that will handle the OMAP
> specific code for the reset. The generic driver will handle the rest.
>
> Maybe I'm missing some important details, but that does seems easily doable
> for the i2c case.
Hmm. How do those drivers call the platform-specific code in a way that
allows those drivers to be built for non-PPC platforms?
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
On Wed, 11 Jan 2012, Shubhrajyoti wrote: > On Wednesday 11 January 2012 06:59 PM, Paul Walmsley wrote: > > > The real question is how you plan to handle the device reset function, > > given that the driver should compile on a non-OMAP build (meaning that you > > can't call omap_device*() functions from the driver directly), nor should > > the driver touch the SYSCONFIG register directly. > > Paul I thought through it myself not very clear. > - One way is to find some way for the dt to pass function pointer. Or maybe > a flag that activates a static function / hwmod func. > > Will give this a little more thought and get back. I have not any answer > right now. Well, if it makes you feel any better, no one else seems to have thought through it very much either :-( To me this is essentially implementing a bus-level (or really, integration-level) reset of the device. This seems to be a function that many other buses might support. So let's look at some of the other common Linux bus implementations. PCI has pci_reset_function() in drivers/pci/pci.c, which a handful of PCI-specific drivers call. USB has usb_reset_device() in drivers/usb/core/hub.c, also called by a handful of devices. So there's precedent for the use case. Incidentally, this is the kind of thing that ideally should be supported via a common function pointer in struct bus_type or maybe struct device. So rather than the driver calling pci_reset_function() or usb_reset_device(), it seems preferable for the driver to do something like: if (dev->bus->device_reset) dev->bus->device_reset(dev); It seems best to avoid binding a driver tightly to a particular bus whenever possible. ... Now to consider something like this from an OMAP perspective. In a rough approximation of an ideal world, much of the hwmod and omap_device code would live somewhere like drivers/omap_bus/. Our devices would be omap_device based rather than platform_device or of_device based. We could export omap_device_reset() and then the I2C driver could call this directly. We're at least a year away from this (if ever): we need PRM, CM, SCM drivers; we'd need to have a real omap_bus and omap_device that our drivers would use, unlike the faux omap_device we have now. Maybe the latter is never going to happen now due to the DT situation. So any solution that would depend on these steps as prerequisites might easily be delayed for an infinite amount of time. But let's say that a device_reset() function pointer existed inside the common struct bus_type in include/linux/device.h, as discussed above. >From an OMAP perspective, we could simply set that to point to omap_device_reset() in the OMAP-specific device creation code or DT notifier callback code. PCI and USB code could set it to point to their own reset functions. Something like the code fragment above would be recommended for all device drivers to do an integration device reset. Anyway, just an idea. - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
On 1/11/2012 2:29 PM, Paul Walmsley wrote: On Wed, 11 Jan 2012, Shubhrajyoti wrote: On Tuesday 10 January 2012 08:56 PM, Kevin Hilman wrote: "Datta, Shubhrajyoti" writes: On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti wrote: On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti wrote: On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote: This patch either needs to be acked by Ben with a note that it's okay for us to merge through the OMAP tree, or needs to be merged by Ben during the 3.4 merge window, after patches 1-3 have reached the mainline tree. I agree. Ben do you have any comments . If there are no further comments can this be merged also ? As Benoit mentioned earlier, the addition of new pdata fields will cause problems with the DT support already queued for v3.3. This series should be reworked on top of the DT support which Ben has now queued for v3.3 (my branch: for_3.3/i2c/misc) Yes will rework it. Depending on what you plan to do, you'll probably need to coordinate this with Tony also. He's already pulled some of the patches from this series into his i2c branch. The real question is how you plan to handle the device reset function, given that the driver should compile on a non-OMAP build (meaning that you can't call omap_device*() functions from the driver directly), nor should the driver touch the SYSCONFIG register directly. Something that puzzle me on that point is most architecture does not use plateform_device and thus does not have any pdata to hack some custom function pointers here an there. It means that there is probably some better solution to handle that. AFAIU, the way PPC is handling that is by splitting the driver in a generic part and in a platform specific part. You can then add any OMAP specific hacks in the OMAP specific part while keeping most of generic code in a generic driver. In this case, we just have to implement a small shim that will handle the OMAP specific code for the reset. The generic driver will handle the rest. Maybe I'm missing some important details, but that does seems easily doable for the i2c case. Regards, Benoit -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
On Wednesday 11 January 2012 06:59 PM, Paul Walmsley wrote: > On Wed, 11 Jan 2012, Shubhrajyoti wrote: > >> On Tuesday 10 January 2012 08:56 PM, Kevin Hilman wrote: >>> "Datta, Shubhrajyoti" writes: >>> On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti wrote: > On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti wrote: >> On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote: >> >>> This patch either needs to be acked by Ben with a note that it's okay >>> for >>> us to merge through the OMAP tree, or needs to be merged by Ben during >>> the >>> 3.4 merge window, after patches 1-3 have reached the mainline tree. >> I agree. >> Ben do you have any comments . If there are no further comments can this be merged also ? >>> As Benoit mentioned earlier, the addition of new pdata fields will cause >>> problems with the DT support already queued for v3.3. >>> >>> This series should be reworked on top of the DT support which Ben has >>> now queued for v3.3 (my branch: for_3.3/i2c/misc) >> Yes will rework it. > Depending on what you plan to do, you'll probably need to coordinate this > with Tony also. He's already pulled some of the patches from this series > into his i2c branch. > > The real question is how you plan to handle the device reset function, > given that the driver should compile on a non-OMAP build (meaning that you > can't call omap_device*() functions from the driver directly), nor should > the driver touch the SYSCONFIG register directly. Paul I thought through it myself not very clear. - One way is to find some way for the dt to pass function pointer. Or maybe a flag that activates a static function / hwmod func. Will give this a little more thought and get back. I have not any answer right now. > > - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
On Wed, 11 Jan 2012, Shubhrajyoti wrote: > On Tuesday 10 January 2012 08:56 PM, Kevin Hilman wrote: > > "Datta, Shubhrajyoti" writes: > > > >> On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti > >> wrote: > >>> > >>> On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti wrote: > On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote: > > > This patch either needs to be acked by Ben with a note that it's okay > > for > > us to merge through the OMAP tree, or needs to be merged by Ben during > > the > > 3.4 merge window, after patches 1-3 have reached the mainline tree. > I agree. > Ben do you have any comments . > >>> > >> If there are no further comments can this be merged also ? > > As Benoit mentioned earlier, the addition of new pdata fields will cause > > problems with the DT support already queued for v3.3. > > > > This series should be reworked on top of the DT support which Ben has > > now queued for v3.3 (my branch: for_3.3/i2c/misc) > > Yes will rework it. Depending on what you plan to do, you'll probably need to coordinate this with Tony also. He's already pulled some of the patches from this series into his i2c branch. The real question is how you plan to handle the device reset function, given that the driver should compile on a non-OMAP build (meaning that you can't call omap_device*() functions from the driver directly), nor should the driver touch the SYSCONFIG register directly. - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
On Tuesday 10 January 2012 08:56 PM, Kevin Hilman wrote: > "Datta, Shubhrajyoti" writes: > >> On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti >> wrote: >>> >>> >>> On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti wrote: Ben, On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote: > Hi > > On Tue, 13 Dec 2011, Shubhrajyoti D wrote: > >> - The reset in the driver at init is not needed anymore as the >>hwmod framework takes care of reseting it. >> - Reset is removed from omap_i2c_init, which was called >>not only during probe, but also after time out and error handling. >>device_reset were added in those places to effect the reset. >> - Earlier the hwmod SYSC settings were over-written in the driver. >>Removing the same and letting the hwmod take care of the settings. >> - Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed. >> - Clean up the SYSCONFIG SYSC bit definition macros. >> - Fix the typos in wakeup. >> >> Signed-off-by: Shubhrajyoti D >> --- >> drivers/i2c/busses/i2c-omap.c | 83 >> +++- >> 1 files changed, 23 insertions(+), 60 deletions(-) > This patch either needs to be acked by Ben with a note that it's okay for > us to merge through the OMAP tree, or needs to be merged by Ben during the > 3.4 merge window, after patches 1-3 have reached the mainline tree. I agree. Ben do you have any comments . >>> >> If there are no further comments can this be merged also ? > As Benoit mentioned earlier, the addition of new pdata fields will cause > problems with the DT support already queued for v3.3. > > This series should be reworked on top of the DT support which Ben has > now queued for v3.3 (my branch: for_3.3/i2c/misc) Yes will rework it. > Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
"Datta, Shubhrajyoti" writes: > On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti > wrote: >> >> >> >> On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti wrote: >>> >>> Ben, >>> >>> On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote: >>> > Hi >>> > >>> > On Tue, 13 Dec 2011, Shubhrajyoti D wrote: >>> > >>> >> - The reset in the driver at init is not needed anymore as the >>> >> hwmod framework takes care of reseting it. >>> >> - Reset is removed from omap_i2c_init, which was called >>> >> not only during probe, but also after time out and error handling. >>> >> device_reset were added in those places to effect the reset. >>> >> - Earlier the hwmod SYSC settings were over-written in the driver. >>> >> Removing the same and letting the hwmod take care of the settings. >>> >> - Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed. >>> >> - Clean up the SYSCONFIG SYSC bit definition macros. >>> >> - Fix the typos in wakeup. >>> >> >>> >> Signed-off-by: Shubhrajyoti D >>> >> --- >>> >> drivers/i2c/busses/i2c-omap.c | 83 >>> >> +++- >>> >> 1 files changed, 23 insertions(+), 60 deletions(-) >>> > This patch either needs to be acked by Ben with a note that it's okay for >>> > us to merge through the OMAP tree, or needs to be merged by Ben during the >>> > 3.4 merge window, after patches 1-3 have reached the mainline tree. >>> I agree. >>> Ben do you have any comments . >> >> > If there are no further comments can this be merged also ? As Benoit mentioned earlier, the addition of new pdata fields will cause problems with the DT support already queued for v3.3. This series should be reworked on top of the DT support which Ben has now queued for v3.3 (my branch: for_3.3/i2c/misc) Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti wrote: > > > > On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti wrote: >> >> Ben, >> >> On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote: >> > Hi >> > >> > On Tue, 13 Dec 2011, Shubhrajyoti D wrote: >> > >> >> - The reset in the driver at init is not needed anymore as the >> >> hwmod framework takes care of reseting it. >> >> - Reset is removed from omap_i2c_init, which was called >> >> not only during probe, but also after time out and error handling. >> >> device_reset were added in those places to effect the reset. >> >> - Earlier the hwmod SYSC settings were over-written in the driver. >> >> Removing the same and letting the hwmod take care of the settings. >> >> - Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed. >> >> - Clean up the SYSCONFIG SYSC bit definition macros. >> >> - Fix the typos in wakeup. >> >> >> >> Signed-off-by: Shubhrajyoti D >> >> --- >> >> drivers/i2c/busses/i2c-omap.c | 83 >> >> +++- >> >> 1 files changed, 23 insertions(+), 60 deletions(-) >> > This patch either needs to be acked by Ben with a note that it's okay for >> > us to merge through the OMAP tree, or needs to be merged by Ben during the >> > 3.4 merge window, after patches 1-3 have reached the mainline tree. >> I agree. >> Ben do you have any comments . > > If there are no further comments can this be merged also ? Thanks. > >> >> > >> > - Paul >> > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
Ben, On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote: > Hi > > On Tue, 13 Dec 2011, Shubhrajyoti D wrote: > >> - The reset in the driver at init is not needed anymore as the >>hwmod framework takes care of reseting it. >> - Reset is removed from omap_i2c_init, which was called >>not only during probe, but also after time out and error handling. >>device_reset were added in those places to effect the reset. >> - Earlier the hwmod SYSC settings were over-written in the driver. >>Removing the same and letting the hwmod take care of the settings. >> - Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed. >> - Clean up the SYSCONFIG SYSC bit definition macros. >> - Fix the typos in wakeup. >> >> Signed-off-by: Shubhrajyoti D >> --- >> drivers/i2c/busses/i2c-omap.c | 83 >> +++- >> 1 files changed, 23 insertions(+), 60 deletions(-) > This patch either needs to be acked by Ben with a note that it's okay for > us to merge through the OMAP tree, or needs to be merged by Ben during the > 3.4 merge window, after patches 1-3 have reached the mainline tree. I agree. Ben do you have any comments . > > - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
Hi On Tue, 13 Dec 2011, Shubhrajyoti D wrote: > - The reset in the driver at init is not needed anymore as the >hwmod framework takes care of reseting it. > - Reset is removed from omap_i2c_init, which was called >not only during probe, but also after time out and error handling. >device_reset were added in those places to effect the reset. > - Earlier the hwmod SYSC settings were over-written in the driver. >Removing the same and letting the hwmod take care of the settings. > - Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed. > - Clean up the SYSCONFIG SYSC bit definition macros. > - Fix the typos in wakeup. > > Signed-off-by: Shubhrajyoti D > --- > drivers/i2c/busses/i2c-omap.c | 83 +++- > 1 files changed, 23 insertions(+), 60 deletions(-) This patch either needs to be acked by Ben with a note that it's okay for us to merge through the OMAP tree, or needs to be merged by Ben during the 3.4 merge window, after patches 1-3 have reached the mainline tree. - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
