Re: [PATCH 1/4] sysfs: Support is_visible() on binary attributes
On Mon, Sep 21, 2015 at 10:38:20AM -0300, Emilio López wrote: > According to the sysfs header file: > > "The returned value will replace static permissions defined in > struct attribute or struct bin_attribute." > > but this isn't the case, as is_visible is only called on struct attribute > only. This patch introduces a new is_bin_visible() function to implement > the same functionality for binary attributes, and updates documentation > accordingly. > > Note that to keep functionality and code similar to that of normal > attributes, the mode is now checked as well to ensure it contains only > read/write permissions or SYSFS_PREALLOC. > > Reviewed-by: Guenter Roeck > Signed-off-by: Emilio López As this should probably go through the "platform drivers" maintainer, I'll just give you this: Acked-by: Greg Kroah-Hartman So it can go through their tree and not require me to just take this one. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes
On Wed, Sep 09, 2015 at 10:14:46AM -0300, Emilio López wrote: > On 09/09/15 01:12, Guenter Roeck wrote: > >On 09/08/2015 08:58 PM, Greg KH wrote: > >>On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote: > >>>Hi Emilio, > >>> > >>>On 09/08/2015 05:51 PM, Emilio López wrote: > >>>>Hi Greg & Guenter, > >>>> > >>>[ ... ] > >>>>>>> > >>>>>>>Unless I am missing something, this is not explained anywhere, > >>>>>>>but it is > >>>>>>>not entirely trivial to understand. I think it should be documented. > >>>> > >>>>I agree. I couldn't find any mention of what this int was supposed > >>>>to be by looking at Documentation/ (is_visible is not even mentioned > >>>>:/) or include/linux/sysfs.h. Once we settle on something I'll > >>>>document it before sending a v2. > >>>> > >>>In the include file ? No strong preference, though. > >>> > >>>>By the way, I wrote a quick coccinelle script to match is_visible() > >>>>users which reference the index (included below), and it found > >>>>references to drivers which do not seem to use any binary > >>>>attributes, so I believe changing the index meaning shouldn't be an > >>>>issue. > >>>> > >>>Good. > >>> > >>>>>>I agree, make i the number of the bin attribute and that should solve > >>>>>>this issue. > >>>>>> > >>>>>No, that would conflict with the "normal" use of is_visible for > >>>>>non-binary > >>>>>attributes, and make the index all but useless, since the > >>>>>is_visible function > >>>>>would have to search through all the attributes anyway to figure > >>>>>out which one > >>>>>is being checked. > >>>> > >>>>Yeah, using the same indexes would be somewhat pointless, although > >>>>not many seem to be using it anyway (only 14 files matched). Others > >>>>seem to be comparing the attr* instead. An alternative would be to > >>>>use negative indexes for binary attributes and positive indexes for > >>>>normal attributes. > >>>> > >>>... and I probably wrote or reviewed a significant percentage of > >>>those ;-). > >>> > >>>Using negative numbers for binary attributes is an interesting idea. > >>>Kind of unusual, though. Greg, any thoughts on that ? > >> > >>Ick, no, that's a mess, maybe we just could drop the index alltogether? > >> > > > >No, please don't. Having to manually compare dozens of index pointers > >would be > >even more of a mess. > > So, what about keeping it the way it is in the patch, and documenting it > thoroughly? Otherwise, we could introduce another "is_bin_visible" function > to do this same thing but just on binary attributes, but I'd rather not add > a new function pointer if possible. is_bin_visiable makes sense to me instead of trying to overload the index number in some crazy way. There's no issue with adding another function pointer. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes
On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote: > Hi Emilio, > > On 09/08/2015 05:51 PM, Emilio López wrote: > >Hi Greg & Guenter, > > > [ ... ] > > Unless I am missing something, this is not explained anywhere, but it is > not entirely trivial to understand. I think it should be documented. > > > >I agree. I couldn't find any mention of what this int was supposed to be by > >looking at Documentation/ (is_visible is not even mentioned :/) or > >include/linux/sysfs.h. Once we settle on something I'll document it before > >sending a v2. > > > In the include file ? No strong preference, though. > > >By the way, I wrote a quick coccinelle script to match is_visible() users > >which reference the index (included below), and it found references to > >drivers which do not seem to use any binary attributes, so I believe > >changing the index meaning shouldn't be an issue. > > > Good. > > >>>I agree, make i the number of the bin attribute and that should solve > >>>this issue. > >>> > >>No, that would conflict with the "normal" use of is_visible for non-binary > >>attributes, and make the index all but useless, since the is_visible > >>function > >>would have to search through all the attributes anyway to figure out which > >>one > >>is being checked. > > > >Yeah, using the same indexes would be somewhat pointless, although not many > >seem to be using it anyway (only 14 files matched). Others seem to be > >comparing the attr* instead. An alternative would be to use negative indexes > >for binary attributes and positive indexes for normal attributes. > > > ... and I probably wrote or reviewed a significant percentage of those ;-). > > Using negative numbers for binary attributes is an interesting idea. > Kind of unusual, though. Greg, any thoughts on that ? Ick, no, that's a mess, maybe we just could drop the index alltogether? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes
On Tue, Sep 08, 2015 at 08:30:13AM -0700, Guenter Roeck wrote:
> Emilio,
>
> On Tue, Sep 08, 2015 at 09:07:44AM -0300, Emilio López wrote:
> > According to the sysfs header file:
> >
> > "The returned value will replace static permissions defined in
> > struct attribute or struct bin_attribute."
> >
> > but this isn't the case, as is_visible is only called on
> > struct attribute only. This patch adds the code paths required
> > to support is_visible() on binary attributes.
> >
> > Signed-off-by: Emilio López
> > ---
> > fs/sysfs/group.c | 22 ++
> > 1 file changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > index 39a0199..eb6996a 100644
> > --- a/fs/sysfs/group.c
> > +++ b/fs/sysfs/group.c
> > @@ -37,10 +37,10 @@ static int create_files(struct kernfs_node *parent,
> > struct kobject *kobj,
> > {
> > struct attribute *const *attr;
> > struct bin_attribute *const *bin_attr;
> > - int error = 0, i;
> > + int error = 0, i = 0;
> >
> > if (grp->attrs) {
> > - for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> > + for (attr = grp->attrs; *attr && !error; i++, attr++) {
> > umode_t mode = (*attr)->mode;
> >
> > /*
> > @@ -73,13 +73,27 @@ static int create_files(struct kernfs_node *parent,
> > struct kobject *kobj,
> > }
> >
> > if (grp->bin_attrs) {
> > - for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> > + for (bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
> > + umode_t mode = (*bin_attr)->attr.mode;
> > +
> > if (update)
> > kernfs_remove_by_name(parent,
> > (*bin_attr)->attr.name);
> > + if (grp->is_visible) {
> > + mode = grp->is_visible(kobj,
> > + &(*bin_attr)->attr, i);
>
> With this, if 'n' is the number of non-binary attributes,
>
> for i < n:
> The index passed to is_visible points to a non-binary attribute.
> for i >= n:
> The index passed to is_visible points to the (index - n)th binary
> attribute.
>
> Unless I am missing something, this is not explained anywhere, but it is
> not entirely trivial to understand. I think it should be documented.
I agree, make i the number of the bin attribute and that should solve
this issue.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH stable-4.0] ARM: EXYNOS: Fix failed second suspend on Exynos4
On Sat, Jun 20, 2015 at 06:32:46PM +0900, Krzysztof Kozlowski wrote: > Hi Greg, > > I backported the patch below for stable 4.0. It seems it was missed or > maybe I sent it the wrong way? The backport was mailed to > sta...@vger.kernel.org > > Is the patch queued for next 4.0 stable release? Should I resent it? It's still in the to-apply queue, which is a few hundred patches long. Don't worry, it's not lost, just in good company with other patches I'll get to eventually. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
Re: [PATCH v2] ARM: dts: fix the clock-frequency of rinato board's panel
On Fri, Jun 12, 2015 at 07:06:50PM +0900, Hyungwon Hwang wrote:
> After the commit abc0b1447d4974963548777a5ba4a4457c82c426
> ("drm: Perform basic sanity checks on probed modes"), proper
> clock-frequency becomes mandatory for validating the mode of panel.
> The display does not work if there is no mode validated. Also, this
> clock-frequency must be set appropriately for getting required frame
> rate.
>
> Signed-off-by: Hyungwon Hwang
> ---
> arch/arm/boot/dts/exynos3250-rinato.dts | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND] serial: samsung: Fix serial config dependencies for exynos7
On Mon, Nov 17, 2014 at 10:14:51AM +0530, Abhilash Kesavan wrote: > From: Pankaj Dubey > > Exynos7 has a similar serial controller to that present in older Samsung > SoCs. To re-use the existing serial driver on Exynos7 we need to have > SERIAL_SAMSUNG_UARTS_4 and SERIAL_SAMSUNG_UARTS selected. This is not > possible because these symbols are dependent on PLAT_SAMSUNG which is > not present for the ARMv8 based exynos7. > > Change the dependency of these symbols from PLAT_SAMSUNG to the serial > driver thus making it available on exynos7. As the existing platform > specific code making use of these symbols is related to uart driver this > change in dependency should not cause any issues. > > Signed-off-by: Pankaj Dubey > Signed-off-by: Naveen Krishna Chatradhi > Signed-off-by: Abhilash Kesavan > Cc: Greg Kroah-Hartman > --- Acked-by: Greg Kroah-Hartman -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb: Remove references to non-existent PLAT_S5P symbol
On Wed, Nov 05, 2014 at 11:23:36AM +0100, Sylwester Nawrocki wrote:
> On 04/11/14 20:52, Paul Bolle wrote:
> > On Tue, 2014-11-04 at 11:42 -0800, Greg KH wrote:
> >> > As it's something that no one seemed to ever need before (i.e. it's not
> >> > a regression fix), but it would be a "new feature", I don't think it's
> >> > really a stable fix.
> >> >
> >> > But feel free to convince me otherwise :)
> >
> > Sylwester, was I right in thinking that users of PLAT_S5P, who could set
> > USB_EHCI_EXYNOS or USB_OHCI_EXYNOS pre v3.17, got, well, transferred to
> > ARCH_S5PV210 and lost the ability to set one of those symbols in v3.17?
>
> Yes, after commit d78c16ccde96 ("ARM: SAMSUNG: Remove remaining legacy code")
> we lost the ability to enable USB OHCI and EHCI on S5PV210 SoC.
> Thus for those who use the mainline kernel (might be rare) with S5PV210 SoC
> there is obviously a regression in USB subsystem in v3.17.
Ok, I'll add this to 3.17-stable when it hits Linus's tree, thanks.
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb: Remove references to non-existent PLAT_S5P symbol
On Tue, Nov 04, 2014 at 08:21:04PM +0100, Paul Bolle wrote: > On Tue, 2014-11-04 at 19:33 +0100, Sylwester Nawrocki wrote: > > On 04/11/14 00:24, Greg KH wrote: > > > This isn't a stable issue... > > > > Sorry for disturbing then, let me go and read the documentation again. > > If I remember correctly, I asked Sylwester to mark this for stable. So > it's me that should be educated here. > > Why is a patch that allows the users of ARCH_S5PV210 to set > USB_EHCI_EXYNOS or USB_OHCI_EXYNOS - which they apparently need - for a > v3.17.y kernel, not a stable issue? As it's something that no one seemed to ever need before (i.e. it's not a regression fix), but it would be a "new feature", I don't think it's really a stable fix. But feel free to convince me otherwise :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb: Remove references to non-existent PLAT_S5P symbol
On Tue, Oct 07, 2014 at 11:12:07AM +0200, Sylwester Nawrocki wrote:
> The PLAT_S5P Kconfig symbol was removed in commit d78c16ccde96
> ("ARM: SAMSUNG: Remove remaining legacy code"). There are still
> some references left, fix that by replacing them with ARCH_S5PV210.
>
> Fixes: d78c16ccde96 ("ARM: SAMSUNG: Remove remaining legacy code")
> Reported-by: Paul Bolle
> Acked-by: Jingoo Han
> Cc: [3.17+]
This isn't a stable issue...
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/2] usb: host: ehci-exynos: Remove unnecessary usb-phy support
On Thu, Sep 25, 2014 at 10:50:22AM +0530, Vivek Gautam wrote: > Hi Greg, > > > On Mon, Sep 22, 2014 at 11:15 AM, Vivek Gautam > wrote: > > Now that we have completely moved from older USB-PHY drivers > > to newer GENERIC-PHY drivers for PHYs available with USB controllers > > on Exynos series of SoCs, we can remove the support for the same > > in our host drivers too. > > > > We also defer the probe for our host in case we end up getting > > EPROBE_DEFER error when getting PHYs. > > > > Signed-off-by: Vivek Gautam > > Acked-by: Jingoo Han > > Acked-by: Alan Stern > > --- > > Did this one got missed for usb-next ? > I can only see "usb: host: ohci-exynos: Remove unnecessary usb-phy support" > in the next branch. > > Ignore me if you have already taken care of this, and plan to queue it up. If it's not in my tree already, please resend as I don't have this in my queue anymore. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] misc: add sii9234 driver
On Fri, Apr 11, 2014 at 01:48:29PM +0200, Tomasz Stanislawski wrote: > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 1cb7408..3b7f266 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -515,6 +515,14 @@ config SRAM > the genalloc API. It is supposed to be used for small on-chip SRAM > areas found on many SoCs. > > +config SII9234 > + tristate "Silicon Image SII9234 Driver" > + depends on I2C I doubt this is the only dependency, right? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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] Add support for sii9234 chip
On Fri, Apr 11, 2014 at 01:48:28PM +0200, Tomasz Stanislawski wrote: > Hi everyone, > This patchset adds support for sii9234 HD Mobile Link Bridge. The chip is > used > to convert HDMI signal into MHL. The driver enables HDMI output on Trats and > Trats2 boards. > > The code is based on the driver [1] developed by: >Adam Hampson >Erik Gilling > with additional contributions from: >Shankar Bandal >Dharam Kumar > > The drivers architecture was greatly simplified and transformed into a form > accepted (hopefully) by opensource community. The main differences from > original code are: > * using single I2C client instead of 4 subclients > * remove all logic non-related to establishing HDMI link > * simplify error handling > * rewrite state machine in interrupt handler > * wakeup and discovery triggered by an extcon event > * integrate with Device Tree > > For now, the driver is added to drivers/misc/ directory because it has neigher > userspace nor kernel interface. The chip is capable of receiving and > processing CEC events, so the driver may export an input device in /dev/ in > the > future. However CEC could be also handled by HDMI driver. > > I kindly ask for suggestions about the best location for this driver. It really is an extcon driver, so why not put it in drivers/extcon? And that might solve any build issues you have if you don't select extcon in your .config file and try to build this code :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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] serial: pl011: Move uart_register_driver call to device probe
On Fri, Feb 14, 2014 at 12:07:17AM +, Russell King - ARM Linux wrote: > On Thu, Feb 13, 2014 at 03:26:06PM -0800, Greg KH wrote: > > On Thu, Feb 13, 2014 at 06:42:49PM +, Russell King - ARM Linux wrote: > > > We went through this before, and I stated the paths, and no one disagreed > > > with that. > > > > > > It /is/ racy. > > > > Ok, I just went and looked at the uart driver register path, and I don't > > see the race (note, if there is one, it's there today, regardless of > > this patch). > > The race isn't the uart code, it's the driver model. > > Consider what happens when this happens: > > * Two pl011 devices get registered at the same time by two different > threads. How? What two different busses will see this same device? The amba bus code should prevent that from happening, right? If not, there's bigger problems in that bus code :) That's where this problem should be fixed, if there is one, otherwise this same issue would be there for any type of driver that calles into the uart core, right? > * Both devices have a lock taken on the _device_ itself before matching > against the driver. > > * Both devices get matched to the same driver. > > * Both devices are passed into the driver's probe function. > > * Both check uart_reg.state, both call uart_register_driver() on that > at the same time, which results in two allocations inside > uart_register_driver(), one gets overwritten... > > So, the /only/ thing which stops this happening is that the devices > are generally available before the driver is registered, and driver > registration results in devices being probed serially. Moreover, both > attempt to call tty_register_driver()... one succeeds, the other fails. > > However, what about the userspace bind/unbind methods. Yes, userspace > can ask the driver core to unbind devices from a driver or bind - and > again, there's no per-driver locking here. So, if you can trigger two > concurrent binds from userspace, you hit the same race as above. > > So, if you want to accept these patches, go ahead, introduce races, but > personally I'd recommend plugging these races. The only way to solve this would be to do it in the bus, I don't see anything here that makes it any "racier" than it currently is. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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] serial: pl011: Move uart_register_driver call to device probe
On Thu, Feb 13, 2014 at 06:42:49PM +, Russell King - ARM Linux wrote: > On Thu, Feb 13, 2014 at 10:27:01AM -0800, Greg KH wrote: > > On Thu, Feb 13, 2014 at 06:15:59PM +, Russell King - ARM Linux wrote: > > > On Thu, Feb 13, 2014 at 10:12:16AM -0800, Greg KH wrote: > > > > On Mon, Jan 20, 2014 at 10:04:15AM +, Russell King - ARM Linux > > > > wrote: > > > > > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera wrote: > > > > > > uart_register_driver call binds the driver to a specific device > > > > > > node through tty_register_driver call. This should typically happen > > > > > > during device probe call. > > > > > > > > > > > > In a multiplatform scenario, it is possible that multiple serial > > > > > > drivers are part of the kernel. Currently the driver registration > > > > > > fails > > > > > > if multiple serial drivers with same default major/minor numbers are > > > > > > included in the kernel. > > > > > > > > > > > > A typical case is observed with amba-pl011 and samsung-uart drivers. > > > > > > > > > > NAK. There should not be any other driver using amba-pl011's device > > > > > numbers. > > > > > > > > I agree, but there is. And because of that, moving the registration to > > > > the probe call fixes the issue with building a kernel with all of the > > > > drivers built into them, so I'm going to take both of these patches, as > > > > it does solve that problem, while still allowing the device number > > > > collision to happen. > > > > > > So what happens when two _devices_ are probed by this driver at the same > > > time? > > > > The bus that the driver is on will not allow that to happen, I thought > > we went through this before... > > > > And yes, devices on different busses could cause problems, but is that > > the case here for these devices? At first glance, I don't think that > > can happen for these drivers. > > We went through this before, and I stated the paths, and no one disagreed > with that. > > It /is/ racy. Ok, I just went and looked at the uart driver register path, and I don't see the race (note, if there is one, it's there today, regardless of this patch). uart_register_driver() fils in a bunch of fields, and passes control off to tty_register_driver(). If the minor/major number allocation here fails, due to collisions, then an error occurs, and things back out safely. If the allocation succeeds, then we lock the list of drivers, add them to the list, then register the tty device with the subsystem for how ever many tty devices were asked for. Yes, the locking might be wrong here, in the tty layer, but again, that's nothing new created by this patch. So I fail to see the problem with applying this patch, as it solves a problem people are having, and should be fine given that no hardware with both of these devices will ever be present at the same time. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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] serial: pl011: Move uart_register_driver call to device probe
On Thu, Feb 13, 2014 at 06:15:59PM +, Russell King - ARM Linux wrote: > On Thu, Feb 13, 2014 at 10:12:16AM -0800, Greg KH wrote: > > On Mon, Jan 20, 2014 at 10:04:15AM +, Russell King - ARM Linux wrote: > > > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera wrote: > > > > uart_register_driver call binds the driver to a specific device > > > > node through tty_register_driver call. This should typically happen > > > > during device probe call. > > > > > > > > In a multiplatform scenario, it is possible that multiple serial > > > > drivers are part of the kernel. Currently the driver registration fails > > > > if multiple serial drivers with same default major/minor numbers are > > > > included in the kernel. > > > > > > > > A typical case is observed with amba-pl011 and samsung-uart drivers. > > > > > > NAK. There should not be any other driver using amba-pl011's device > > > numbers. > > > > I agree, but there is. And because of that, moving the registration to > > the probe call fixes the issue with building a kernel with all of the > > drivers built into them, so I'm going to take both of these patches, as > > it does solve that problem, while still allowing the device number > > collision to happen. > > So what happens when two _devices_ are probed by this driver at the same > time? The bus that the driver is on will not allow that to happen, I thought we went through this before... And yes, devices on different busses could cause problems, but is that the case here for these devices? At first glance, I don't think that can happen for these drivers. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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] serial: pl011: Move uart_register_driver call to device probe
On Mon, Jan 20, 2014 at 10:04:15AM +, Russell King - ARM Linux wrote: > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera wrote: > > uart_register_driver call binds the driver to a specific device > > node through tty_register_driver call. This should typically happen > > during device probe call. > > > > In a multiplatform scenario, it is possible that multiple serial > > drivers are part of the kernel. Currently the driver registration fails > > if multiple serial drivers with same default major/minor numbers are > > included in the kernel. > > > > A typical case is observed with amba-pl011 and samsung-uart drivers. > > NAK. There should not be any other driver using amba-pl011's device numbers. I agree, but there is. And because of that, moving the registration to the probe call fixes the issue with building a kernel with all of the drivers built into them, so I'm going to take both of these patches, as it does solve that problem, while still allowing the device number collision to happen. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] serial: samsung: Move uart_register_driver call to device probe
On Tue, Jan 21, 2014 at 12:07:06AM +, Russell King - ARM Linux wrote: > On Mon, Jan 20, 2014 at 03:51:28PM -0800, Greg KH wrote: > > On Mon, Jan 20, 2014 at 11:16:03PM +, Russell King - ARM Linux wrote: > > > I don't believe the driver model has any locking to prevent a drivers > > > ->probe function running concurrently with it's ->remove function for > > > two (or more) devices. > > > > The bus prevents this from happening. > > > > > The locking against this is done on a per-device basis, not a per-driver > > > basis. > > > > No, on a per-bus basis. > > I don't see it. > > Let's start from driver_register(). Which happens from module probing, which is single-threaded, right? Or from module_init callbacks, which is single-threaded. Normally, busses never add devices (which is what drivers bind to), except in a single-at-a-time fashion, unless they really know what they are doing (i.e. PCI had multi-threaded device probing for a while, don't remember if it still does...) > This takes no locks and calls bus_add_driver(). > This also takes no locks and calls driver_attach(). > This walks the list of devices calling __driver_attach() for each. > __driver_attach() tries to match the device against the driver, > locks the parent device if one exists, and the device which is about > to be probed. It then calls driver_probe_device(). > driver_probe_device() inserts a runtime barrier and calls really_probe(). > really_probe() ultimately calls either the bus ->probe method or the > driver ->probe method. > > At no point in that sequence do I see anything which does any locking > on a per-driver basis. Let's look at device_add(). > > device_add() calls bus_probe_device(), which then calls device_attach(). > device_attach() takes the device's lock, and walks the list of drivers > calling __device_attach() on each driver. This then calls down into > driver_probe_device(), and the path is the same as the above. > > I don't see any per-driver locking here either. > > I've checked the klist stuff, don't see anything there. Ditto for > bus_for_each_drv(). > > If you think there's a per-driver lock that's held over probes or removes, > please point it out. I'm fairly certain that there isn't, because we have > to be able to deal with recursive probes (yes, we've had to deal with > those in the past.) Hm, you are right, I think that's why we had to remove the locks. The klist stuff handles us getting the needed locks for managing our internal lists of devices and drivers, and those should be fine. So, let's go back to your original worry, what are you concerned about? A device being removed while probe() is called? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] serial: samsung: Move uart_register_driver call to device probe
On Mon, Jan 20, 2014 at 11:16:03PM +, Russell King - ARM Linux wrote: > On Mon, Jan 20, 2014 at 03:11:41PM -0800, Greg KH wrote: > > On Mon, Jan 20, 2014 at 09:32:06PM +, Russell King - ARM Linux wrote: > > > On Mon, Jan 20, 2014 at 01:16:01PM -0800, Greg KH wrote: > > > > On Mon, Jan 20, 2014 at 10:05:30AM +, Russell King - ARM Linux > > > > wrote: > > > > > On Mon, Jan 20, 2014 at 02:32:34PM +0530, Tushar Behera wrote: > > > > > > uart_register_driver call binds the driver to a specific device > > > > > > node through tty_register_driver call. This should typically happen > > > > > > during device probe call. > > > > > > > > > > > > In a multiplatform scenario, it is possible that multiple serial > > > > > > drivers are part of the kernel. Currently the driver registration > > > > > > fails > > > > > > if multiple serial drivers with same default major/minor numbers are > > > > > > included in the kernel. > > > > > > > > > > > > A typical case is observed with amba-pl011 and samsung-uart drivers. > > > > > > > > > > The samsung-uart driver is at fault here - the major/minor numbers > > > > > were > > > > > officially registered to amba-pl011. Samsung needs to be fixed > > > > > properly. > > > > > > > > I agree, the Samsung driver is "broken" here, but that's no reason why > > > > these two drivers can't register with the tty layer _after_ the hardware > > > > is detected, and not before. > > > > > > > > That saves resources on systems that build the drivers in, yet do not > > > > have the hardware present, which is always a good thing. > > > > > > Great, so what you're saying is that we need to wait until the first > > > device calls into the probe function. What about removal... how does > > > a driver know when it's last device has been removed to de-register > > > that? > > > > The "bus" that the device is on handles that, right? > > > > > I guess it needs the driver model to provide some way to know when a > > > driver is completely unbound - but isn't that racy? > > > > How is it racy? That's how the driver model works... > > Think about what happens when the last device unregisters, but a new > device comes along and is probed. > > I don't believe the driver model has any locking to prevent a drivers > ->probe function running concurrently with it's ->remove function for > two (or more) devices. The bus prevents this from happening. > The locking against this is done on a per-device basis, not a per-driver > basis. No, on a per-bus basis. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] serial: samsung: Move uart_register_driver call to device probe
On Mon, Jan 20, 2014 at 09:32:06PM +, Russell King - ARM Linux wrote: > On Mon, Jan 20, 2014 at 01:16:01PM -0800, Greg KH wrote: > > On Mon, Jan 20, 2014 at 10:05:30AM +, Russell King - ARM Linux wrote: > > > On Mon, Jan 20, 2014 at 02:32:34PM +0530, Tushar Behera wrote: > > > > uart_register_driver call binds the driver to a specific device > > > > node through tty_register_driver call. This should typically happen > > > > during device probe call. > > > > > > > > In a multiplatform scenario, it is possible that multiple serial > > > > drivers are part of the kernel. Currently the driver registration fails > > > > if multiple serial drivers with same default major/minor numbers are > > > > included in the kernel. > > > > > > > > A typical case is observed with amba-pl011 and samsung-uart drivers. > > > > > > The samsung-uart driver is at fault here - the major/minor numbers were > > > officially registered to amba-pl011. Samsung needs to be fixed properly. > > > > I agree, the Samsung driver is "broken" here, but that's no reason why > > these two drivers can't register with the tty layer _after_ the hardware > > is detected, and not before. > > > > That saves resources on systems that build the drivers in, yet do not > > have the hardware present, which is always a good thing. > > Great, so what you're saying is that we need to wait until the first > device calls into the probe function. What about removal... how does > a driver know when it's last device has been removed to de-register > that? The "bus" that the device is on handles that, right? > I guess it needs the driver model to provide some way to know when a > driver is completely unbound - but isn't that racy? How is it racy? That's how the driver model works... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] serial: samsung: Move uart_register_driver call to device probe
On Mon, Jan 20, 2014 at 10:05:30AM +, Russell King - ARM Linux wrote: > On Mon, Jan 20, 2014 at 02:32:34PM +0530, Tushar Behera wrote: > > uart_register_driver call binds the driver to a specific device > > node through tty_register_driver call. This should typically happen > > during device probe call. > > > > In a multiplatform scenario, it is possible that multiple serial > > drivers are part of the kernel. Currently the driver registration fails > > if multiple serial drivers with same default major/minor numbers are > > included in the kernel. > > > > A typical case is observed with amba-pl011 and samsung-uart drivers. > > The samsung-uart driver is at fault here - the major/minor numbers were > officially registered to amba-pl011. Samsung needs to be fixed properly. I agree, the Samsung driver is "broken" here, but that's no reason why these two drivers can't register with the tty layer _after_ the hardware is detected, and not before. That saves resources on systems that build the drivers in, yet do not have the hardware present, which is always a good thing. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tty: Fallback to use dynamic major number
On Thu, Jan 16, 2014 at 05:08:14PM +, Mark Brown wrote: > On Thu, Jan 16, 2014 at 08:18:41AM -0800, Greg KH wrote: > > On Thu, Jan 16, 2014 at 10:33:22AM +0530, Tushar Behera wrote: > > > In a multi-platform scenario, the hard-coded major/minor numbers in > > > serial drivers may conflict with each other. A typical scenario is > > > observed with amba-pl011 and samsung-uart drivers, both of these > > > drivers use same set of major/minor numbers. If both of these drivers > > > are enabled, probe of samsung-uart driver fails because the desired > > > node is busy. > > > Why would both drivers ever be loaded at the same time? Don't they bind > > to different hardware devices, and as such, never will be in the same > > system? > > No, the issue is happening because the drivers are registering things > at module load time and not at device instantiation time. That's a driver bug that could easily be fixed, instead of hacking up the tty core :) > While the drivers won't actually be run together things like the > multiplatform defconfig and distro configs will build them in since > people tend to like to get serial early on. Build them into the kernel and not as modules? > > The driver shouldn't be registering it's tty devices if the hardware > > isn't present in the system, so how can this codepath ever happen? > > A quick and unscientific poll of serial drivers seems to suggest that > uart_register_driver() is normally called when the module is loaded (if > it shouldn't be this is at least a very common error pattern). It's a common error pattern :) > > > The issue is fixed by adding a fallback in driver core, so that we can > > > use dynamic major number in case device node allocation fails for > > > hard-coded major/minor number. > > > Did you test this out? You still get userspace breakage with it :( > > Yes. I should put my hands up and say that this was my idea. The > theory is that any system using static allocation for a device shouldn't > be affected (modulo races, it's not perfect obviously), any currently > broken system with a userspace with a dynamic dev will start working and > any breakage is confined to drivers which duplicated major numbers (and > even then only on systems with static /dev). > > The other solutions I can think of are moving tty_register_driver() to > device probe time, allowing tty_register_driver() to accept duplicate > majors and the complaining when the devices actually get instantiated > or changing major numbers where there is a duplicate which is guaranteed > to break at least some userspaces with static devices (which was the > original patch you complained about). The first two solutions look a > bit fun though perhaps they fall out more easily than I suspect. How about fixing the drivers as I mentioned above, to not register with the uart or tty core until they know that their hardware is actually present in the system? That would keep any tty core "hacks" from being needed. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tty: Fallback to use dynamic major number
On Thu, Jan 16, 2014 at 10:33:22AM +0530, Tushar Behera wrote: > In a multi-platform scenario, the hard-coded major/minor numbers in > serial drivers may conflict with each other. A typical scenario is > observed with amba-pl011 and samsung-uart drivers, both of these > drivers use same set of major/minor numbers. If both of these drivers > are enabled, probe of samsung-uart driver fails because the desired > node is busy. Why would both drivers ever be loaded at the same time? Don't they bind to different hardware devices, and as such, never will be in the same system? The driver shouldn't be registering it's tty devices if the hardware isn't present in the system, so how can this codepath ever happen? > The issue is fixed by adding a fallback in driver core, so that we can > use dynamic major number in case device node allocation fails for > hard-coded major/minor number. Did you test this out? You still get userspace breakage with it :( > Signed-off-by: Tushar Behera > --- > Hi Greg, > > This is my second attempt at getting this fixed. Let me know if you are > okay with this. > > Initial approach to fix this problem was by forcing samsung-uart driver > to use dynamic major number, but it was rejected [1] because of possible > user-space breakage. "possible"? Heh, no, that should be "real". Sorry, but I think you are trying to fix a problem that never actually happens in real life, or in valid configurations... greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/2] dwc2/s3c-hsotg: Initial steps to combine the 2 driver
On Tue, Jan 14, 2014 at 08:57:12PM +, Paul Zimmerman wrote: > > From: Dinh Nguyen [mailto:dingu...@altera.com] > > Sent: Tuesday, January 14, 2014 12:46 PM > > > > On Tue, 2014-01-14 at 06:21 -0800, Greg KH wrote: > > > On Tue, Jan 14, 2014 at 05:01:00AM -0600, dingu...@altera.com wrote: > > > > From: Dinh Nguyen > > > > > > > > Hi, > > > > > > > > I'm starting work downstream on combining the DWC2 host driver and the > > > > s3c-hsotg > > > > gadget driver into a dual-role OTG driver. Before I go further, I was > > > > hoping to > > > > solicit comments on whether or not my initial approach is correct? I > > > > know there > > > > are plans to combine the 2, so would like to solicit > > > > comments/suggestions so > > > > that I can also upstream it as well. > > > > > > > > These 2 patches: > > > > > > > > * Moves the DWC2 driver out of drivers/staging into drivers/usb/dwc2/ > > > > > > This already happened yesterday in my tree, so you should see this in > > > linux-next by now, no need to do it again :) > > > > > > > I see it now. Thanks for the pointer. > > > > > > * Moves the s3c-hsotg driver into drivers/usb/dwc2/ > > > > * Delete s3c-hsotg.h > > > > * Make the s3c-hsotg.c file use the defines in hw.h from the DWC2 > > > > driver. > > > > > > > > This initial patch has been tested on the SOCFPGA platform only in > > > > Host-only > > > > and Gadget-only mode. > > > > > > > > The next step would be to do the combining of the driver into a > > > > dual-role OTG > > > > driver. > > > > > > I was told that merging the two of these isn't going to work as the > > > silicon is just too different, which is why I allowed the code to move > > > out of staging. If you feel differently, and think you can combine the > > > two drivers, that's wonderful, I'll gladly take patches to do so, but be > > > sure to test on the proper platforms to make sure nothing breaks. > > > > > > > I wasn't aware of the silicon differences. I just took the s3c-hsotg > > driver as is and it worked fine on my version 2.93a of the USB IP. I'll > > search the ML for information, or perhaps Paul can comment? > > I think Greg is thinking of the octeon-usb driver in staging [1], not > the s3c-hsotg driver. The plan was always to eventually merge dwc2 with > s3c-hsotg. Yes, I'm totally confused, you are right. Nevermind then, Dinh, if you want to redo your patch after 3.14-rc1 is out, that would be great as merging the drivers together can be done easier after that development point. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/2] dwc2/s3c-hsotg: Initial steps to combine the 2 driver
On Tue, Jan 14, 2014 at 05:01:00AM -0600, dingu...@altera.com wrote: > From: Dinh Nguyen > > Hi, > > I'm starting work downstream on combining the DWC2 host driver and the > s3c-hsotg > gadget driver into a dual-role OTG driver. Before I go further, I was hoping > to > solicit comments on whether or not my initial approach is correct? I know > there > are plans to combine the 2, so would like to solicit comments/suggestions so > that I can also upstream it as well. > > These 2 patches: > > * Moves the DWC2 driver out of drivers/staging into drivers/usb/dwc2/ This already happened yesterday in my tree, so you should see this in linux-next by now, no need to do it again :) > * Moves the s3c-hsotg driver into drivers/usb/dwc2/ > * Delete s3c-hsotg.h > * Make the s3c-hsotg.c file use the defines in hw.h from the DWC2 driver. > > This initial patch has been tested on the SOCFPGA platform only in Host-only > and Gadget-only mode. > > The next step would be to do the combining of the driver into a dual-role OTG > driver. I was told that merging the two of these isn't going to work as the silicon is just too different, which is why I allowed the code to move out of staging. If you feel differently, and think you can combine the two drivers, that's wonderful, I'll gladly take patches to do so, but be sure to test on the proper platforms to make sure nothing breaks. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: samsung: Remove hard-coded major/minor numbers
On Fri, Dec 27, 2013 at 03:47:31PM +0530, Tushar Behera wrote:
> On 27 December 2013 12:08, Greg KH wrote:
> > On Fri, Dec 27, 2013 at 12:00:20PM +0530, Tushar Behera wrote:
> >> On 27 December 2013 10:48, Greg KH wrote:
> >> > On Fri, Dec 27, 2013 at 10:37:28AM +0530, Tushar Behera wrote:
>
> [ ... ]
>
> >> >> @@ -951,8 +949,6 @@ static struct uart_driver s3c24xx_uart_drv = {
> >> >> .nr = CONFIG_SERIAL_SAMSUNG_UARTS,
> >> >> .cons = S3C24XX_SERIAL_CONSOLE,
> >> >> .dev_name = S3C24XX_SERIAL_NAME,
> >> >> - .major = S3C24XX_SERIAL_MAJOR,
> >> >> - .minor = S3C24XX_SERIAL_MINOR,
> >> >
> >> > Doesn't this break existing systems and configurations that are
> >> > expecting 204:64 as the location of this serial port?
> >> >
> >>
> >> I tested this on Exynos4210-Origen, Exynos5250-Arndale board, it works
> >> fine there. I haven't tested on any older boards.
> >
> > How did it work? You are relying on some userspace tools to do this
> > properly, right? What about systems without those specific tools?
> >
>
> Enabling CONFIG_DEVTMPFS, all the /dev/ttySAC nodes are generated
> and the appropriate console is specified through command line
> argument.
But what about systems that rely on a hard-coded /dev?
Look, I'm all for making everyone use devtmpfs, but just changing
major:minor numbers for drivers isn't ok, as you are changing the
userspace ABI for the device.
Please realize what you are asking for here, I really don't think you
grasp it given that you didn't ask any of the maintainers of this driver
about the change in the first place.
Please get approval for this patch from others within Linaro before
sending it out again. Linaro has a process in place for this type of
thing, please use it, otherwise it makes people like me really grumpy
and upset and causes me to yell at people at their conferences.
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: samsung: Remove hard-coded major/minor numbers
On Fri, Dec 27, 2013 at 10:43:23AM +0400, Alexander Shiyan wrote:
> Hello.
> > On Fri, Dec 27, 2013 at 12:00:20PM +0530, Tushar Behera wrote:
> > > On 27 December 2013 10:48, Greg KH wrote:
> > > > On Fri, Dec 27, 2013 at 10:37:28AM +0530, Tushar Behera wrote:
> > > >> The hard-coded values clash with the values set for amba-pl011 serial
> > > >> driver. Because of this there is no serial output on Samsung boards
> > > >> if amba-pl011 is enabled alongwith samsung-serial driver.
> > > >>
> > > >> Remove the hardcoded values and let the framework decide on
> > > >> appropriate major/minor number. This is required for multi-platform
> > > >> development work on Exynos platform.
> > > >>
> > > >> Signed-off-by: Tushar Behera
> ...
> > > >> #define S3C24XX_SERIAL_NAME "ttySAC"
> > > >> -#define S3C24XX_SERIAL_MAJOR 204
> > > >> -#define S3C24XX_SERIAL_MINOR 64
> > > >>
> > > >> /* macros to change one thing to another */
> > > >>
> > > >> @@ -951,8 +949,6 @@ static struct uart_driver s3c24xx_uart_drv = {
> > > >> .nr = CONFIG_SERIAL_SAMSUNG_UARTS,
> > > >> .cons = S3C24XX_SERIAL_CONSOLE,
> > > >> .dev_name = S3C24XX_SERIAL_NAME,
> > > >> - .major = S3C24XX_SERIAL_MAJOR,
> > > >> - .minor = S3C24XX_SERIAL_MINOR,
> > > >
> > > > Doesn't this break existing systems and configurations that are
> > > > expecting 204:64 as the location of this serial port?
> > > >
> > >
> > > I tested this on Exynos4210-Origen, Exynos5250-Arndale board, it works
> > > fine there. I haven't tested on any older boards.
> >
> > How did it work? You are relying on some userspace tools to do this
> > properly, right? What about systems without those specific tools?
>
> Can this issue be resolved by using MODULE_ALIAS_CHARDEV()
> in the driver code?
How exactly would that work?
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: samsung: Remove hard-coded major/minor numbers
On Fri, Dec 27, 2013 at 12:00:20PM +0530, Tushar Behera wrote:
> On 27 December 2013 10:48, Greg KH wrote:
> > On Fri, Dec 27, 2013 at 10:37:28AM +0530, Tushar Behera wrote:
> >> The hard-coded values clash with the values set for amba-pl011 serial
> >> driver. Because of this there is no serial output on Samsung boards
> >> if amba-pl011 is enabled alongwith samsung-serial driver.
> >>
> >> Remove the hardcoded values and let the framework decide on
> >> appropriate major/minor number. This is required for multi-platform
> >> development work on Exynos platform.
> >>
> >> Signed-off-by: Tushar Behera
> >> ---
> >> drivers/tty/serial/samsung.c |4
> >> 1 file changed, 4 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> >> index c1af04d..9c20543 100644
> >> --- a/drivers/tty/serial/samsung.c
> >> +++ b/drivers/tty/serial/samsung.c
> >> @@ -56,8 +56,6 @@
> >> /* UART name and device definitions */
> >>
> >> #define S3C24XX_SERIAL_NAME "ttySAC"
> >> -#define S3C24XX_SERIAL_MAJOR 204
> >> -#define S3C24XX_SERIAL_MINOR 64
> >>
> >> /* macros to change one thing to another */
> >>
> >> @@ -951,8 +949,6 @@ static struct uart_driver s3c24xx_uart_drv = {
> >> .nr = CONFIG_SERIAL_SAMSUNG_UARTS,
> >> .cons = S3C24XX_SERIAL_CONSOLE,
> >> .dev_name = S3C24XX_SERIAL_NAME,
> >> - .major = S3C24XX_SERIAL_MAJOR,
> >> - .minor = S3C24XX_SERIAL_MINOR,
> >
> > Doesn't this break existing systems and configurations that are
> > expecting 204:64 as the location of this serial port?
> >
>
> I tested this on Exynos4210-Origen, Exynos5250-Arndale board, it works
> fine there. I haven't tested on any older boards.
How did it work? You are relying on some userspace tools to do this
properly, right? What about systems without those specific tools?
> > Why change this one and not the amba-pl011 driver?
> >
>
> I could only test this driver, so thought of changing this rather than
> modifying amba-pl011 driver. I don't have any other reason.
Please get the samsung driver maintainer to agree with this and sign off
on it before trying to get it merged again.
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: ohci-exynos: Change to use phy provided by the generic phy framework
On Wed, Nov 06, 2013 at 10:27:49AM +0900, Jingoo Han wrote: > Change the phy provider used from the old usb phy specific to a new one > using the generic phy framework. > > Signed-off-by: Jingoo Han > Cc: Kamil Debski > --- > Exynos OHCI driver also uses Exynos USB2.0 PHY. Thus, I make this > patch based-on Kamil Debski's patchset for adding Exynos USB 2.0 PHY > driver. > (http://www.spinics.net/lists/linux-samsung-soc/msg24104.html) > > drivers/usb/host/ohci-exynos.c | 28 > 1 file changed, 8 insertions(+), 20 deletions(-) This has some fuzz when applied to my tree, care to redo it against my usb-next branch of the usb.git tree on git.kernel.org? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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/7] video phy's adaptation to *generic phy framework*
On Wed, Oct 16, 2013 at 09:58:09PM +0530, Kishon Vijay Abraham I wrote: > Hi Greg, > > This series includes video PHY adaptation to Generic PHY Framework. > With the adaptation they were able to get rid of plat data callbacks. > > Since you've taken the Generic PHY Framework, I think this series should > also go into your tree. All now applied, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 2/8] usb: phy: omap-usb2: use the new generic PHY framework
On Wed, Aug 21, 2013 at 11:16:07AM +0530, Kishon Vijay Abraham I wrote:
> Used the generic PHY framework API to create the PHY. Now the power off and
> power on are done in omap_usb_power_off and omap_usb_power_on respectively.
> The omap-usb2 driver is also moved to driver/phy.
>
> However using the old USB PHY library cannot be completely removed
> because OTG is intertwined with PHY and moving to the new framework
> will break OTG. Once we have a separate OTG state machine, we
> can get rid of the USB PHY library.
>
> Signed-off-by: Kishon Vijay Abraham I
> Reviewed-by: Sylwester Nawrocki
> Acked-by: Felipe Balbi
> ---
> drivers/phy/Kconfig | 12 +
> drivers/phy/Makefile |1 +
> drivers/{usb => }/phy/phy-omap-usb2.c | 45
> ++---
> drivers/usb/phy/Kconfig | 10
> drivers/usb/phy/Makefile |1 -
> 5 files changed, 54 insertions(+), 15 deletions(-)
> rename drivers/{usb => }/phy/phy-omap-usb2.c (88%)
I tried to apply this to my USB branch, but it fails.
Kishon, you were going to refresh this patch series, right? Please do,
because as-is, I can't take it.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 0/8] PHY framework
On Fri, Sep 20, 2013 at 11:04:40AM +0530, Kishon Vijay Abraham I wrote: > Hi Greg, > > On Tuesday 17 September 2013 09:11 PM, Felipe Balbi wrote: > > On Wed, Sep 04, 2013 at 02:27:06PM +0530, Kishon Vijay Abraham I wrote: > >> On Tuesday 03 September 2013 09:20 PM, Greg KH wrote: > >>> On Tue, Sep 03, 2013 at 08:55:23PM +0530, Kishon Vijay Abraham I wrote: > >>>> Hi Greg, > >>>> > >>>> On Wednesday 28 August 2013 12:50 AM, Felipe Balbi wrote: > >>>>> Hi, > >>>>> > >>>>> On Mon, Aug 26, 2013 at 01:44:49PM +0530, Kishon Vijay Abraham I wrote: > >>>>>> On Wednesday 21 August 2013 11:16 AM, Kishon Vijay Abraham I wrote: > >>>>>>> Added a generic PHY framework that provides a set of APIs for the PHY > >>>>>>> drivers > >>>>>>> to create/destroy a PHY and APIs for the PHY users to obtain a > >>>>>>> reference to > >>>>>>> the PHY with or without using phandle. > >>>>>>> > >>>>>>> This framework will be of use only to devices that uses external PHY > >>>>>>> (PHY > >>>>>>> functionality is not embedded within the controller). > >>>>>>> > >>>>>>> The intention of creating this framework is to bring the phy drivers > >>>>>>> spread > >>>>>>> all over the Linux kernel to drivers/phy to increase code re-use and > >>>>>>> to > >>>>>>> increase code maintainability. > >>>>>>> > >>>>>>> Comments to make PHY as bus wasn't done because PHY devices can be > >>>>>>> part of > >>>>>>> other bus and making a same device attached to multiple bus leads to > >>>>>>> bad > >>>>>>> design. > >>>>>>> > >>>>>>> If the PHY driver has to send notification on connect/disconnect, the > >>>>>>> PHY > >>>>>>> driver should make use of the extcon framework. Using this susbsystem > >>>>>>> to use extcon framwork will have to be analysed. > >>>>>>> > >>>>>>> You can find this patch series @ > >>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git > >>>>>>> testing > >>>>>> > >>>>>> Looks like there are not further comments on this series. Can you take > >>>>>> this in > >>>>>> your misc tree? > >>>>> > >>>>> Do you want me to queue these for you ? There are quite a few users for > >>>>> this framework already and I know of at least 2 others which will show > >>>>> up for v3.13. > >>>> > >>>> Can you queue this patch series? There are quite a few users already for > >>>> this > >>>> framework. > >>> > >>> It will have to wait for 3.13 as the merge window for new features has > >>> been closed for a week or so. Sorry, I'll queue this up after 3.12-rc1 > >>> is out. > >> > >> Alright, thanks. > > > > Just a gentle ping on this one... > > Let me know if you want me to rebase this patch series on the latest mainline > HEAD. Yes please. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 0/8] PHY framework
On Tue, Sep 03, 2013 at 08:55:23PM +0530, Kishon Vijay Abraham I wrote: > Hi Greg, > > On Wednesday 28 August 2013 12:50 AM, Felipe Balbi wrote: > > Hi, > > > > On Mon, Aug 26, 2013 at 01:44:49PM +0530, Kishon Vijay Abraham I wrote: > >> On Wednesday 21 August 2013 11:16 AM, Kishon Vijay Abraham I wrote: > >>> Added a generic PHY framework that provides a set of APIs for the PHY > >>> drivers > >>> to create/destroy a PHY and APIs for the PHY users to obtain a reference > >>> to > >>> the PHY with or without using phandle. > >>> > >>> This framework will be of use only to devices that uses external PHY (PHY > >>> functionality is not embedded within the controller). > >>> > >>> The intention of creating this framework is to bring the phy drivers > >>> spread > >>> all over the Linux kernel to drivers/phy to increase code re-use and to > >>> increase code maintainability. > >>> > >>> Comments to make PHY as bus wasn't done because PHY devices can be part of > >>> other bus and making a same device attached to multiple bus leads to bad > >>> design. > >>> > >>> If the PHY driver has to send notification on connect/disconnect, the PHY > >>> driver should make use of the extcon framework. Using this susbsystem > >>> to use extcon framwork will have to be analysed. > >>> > >>> You can find this patch series @ > >>> git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git testing > >> > >> Looks like there are not further comments on this series. Can you take > >> this in > >> your misc tree? > > > > Do you want me to queue these for you ? There are quite a few users for > > this framework already and I know of at least 2 others which will show > > up for v3.13. > > Can you queue this patch series? There are quite a few users already for this > framework. It will have to wait for 3.13 as the merge window for new features has been closed for a week or so. Sorry, I'll queue this up after 3.12-rc1 is out. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 6/8] usb: host: ohci-s3c2410 Use clk_prepare_enable/clk_disable_unprepare
On Wed, Jul 31, 2013 at 04:44:43PM -0400, Alan Stern wrote:
> On Wed, 31 Jul 2013, Tomasz Figa wrote:
>
> > Alan, Greg,
> >
> > On Tuesday 23 of July 2013 01:49:23 Tomasz Figa wrote:
> > > This patch modifies the ohci-s3c2410 driver to prepare and unprepare
> > > clocks in addition to enabling and disabling, since it is required
> > > by common clock framework.
> > >
> > > Signed-off-by: Tomasz Figa
> > > ---
> > > drivers/usb/host/ohci-s3c2410.c | 8
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/ohci-s3c2410.c
> > > b/drivers/usb/host/ohci-s3c2410.c index e125770..db096bf 100644
> > > --- a/drivers/usb/host/ohci-s3c2410.c
> > > +++ b/drivers/usb/host/ohci-s3c2410.c
> > > @@ -47,10 +47,10 @@ static void s3c2410_start_hc(struct platform_device
> > > *dev, struct usb_hcd *hcd)
> > >
> > > dev_dbg(&dev->dev, "s3c2410_start_hc:\n");
> > >
> > > - clk_enable(usb_clk);
> > > + clk_prepare_enable(usb_clk);
> > > mdelay(2); /* let the bus clock stabilise */
> > >
> > > - clk_enable(clk);
> > > + clk_prepare_enable(clk);
> > >
> > > if (info != NULL) {
> > > info->hcd = hcd;
> > > @@ -75,8 +75,8 @@ static void s3c2410_stop_hc(struct platform_device
> > > *dev) (info->enable_oc)(info, 0);
> > > }
> > >
> > > - clk_disable(clk);
> > > - clk_disable(usb_clk);
> > > + clk_disable_unprepare(clk);
> > > + clk_disable_unprepare(usb_clk);
> > > }
> > >
> > > /* ohci_s3c2410_hub_status_data
> >
> > Any chance to get your ack on this?
>
> Sorry, this must have slipped past. It's fine with me.
Acked-by: Greg Kroah-Hartman
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 v10 1/8] drivers: phy: add generic PHY framework
On Fri, Jul 26, 2013 at 06:19:16PM +0530, Kishon Vijay Abraham I wrote:
> +static int phy_get_id(void)
> +{
> + int ret;
> + int id;
> +
> + ret = ida_pre_get(&phy_ida, GFP_KERNEL);
> + if (!ret)
> + return -ENOMEM;
> +
> + ret = ida_get_new(&phy_ida, &id);
> + if (ret < 0)
> + return ret;
> +
> + return id;
> +}
ida_simple_get() instead? And if you do that, you can get rid of this
function entirely.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, Jul 23, 2013 at 11:05:48PM +0200, Tomasz Figa wrote: > > That's not so bad, as long as you let the phy core use whatever name it > > wants for the device when it registers it with sysfs. > > Yes, in regulator core consumer names are completely separated from this. > Regulator core simply assigns a sequential integer ID to each regulator > and registers /sys/class/regulator/regulator.ID for each regulator. Yes, that's fine. > > Use the name you > > are requesting as a "tag" or some such "hint" as to what the phy can be > > looked up by. > > > > Good luck handling duplicate "tags" :) > > The tag alone is not a key. Lookup key consists of two components, > consumer device name and consumer tag. What kind of duplicate tags can be > a problem here? Ok, I didn't realize it looked at both parts, that makes sense, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, Jul 23, 2013 at 10:07:52PM +0200, Tomasz Figa wrote:
> On Tuesday 23 of July 2013 12:44:23 Greg KH wrote:
> > On Tue, Jul 23, 2013 at 08:31:05PM +0100, Mark Brown wrote:
> > > > You don't "know" the id of the device you are looking up, due to
> > > > multiple devices being in the system (dynamic ids, look back earlier
> > > > in
> > > > this thread for details about that.)
> > >
> > > I got copied in very late so don't have most of the thread I'm afraid,
> > > I did try looking at web archives but didn't see a clear problem
> > > statement. In any case this is why the APIs doing lookups do the
> > > lookups in the context of the requesting device - devices ask for
> > > whatever name they use locally.
> >
> > What do you mean by "locally"?
> >
> > The problem with the api was that the phy core wanted a id and a name to
> > create a phy, and then later other code was doing a "lookup" based on
> > the name and id (mushed together), because it "knew" that this device
> > was the one it wanted.
> >
> > Just like the clock api, which, for multiple devices, has proven to
> > cause problems. I don't want to see us accept an api that we know has
> > issues in it now, I'd rather us fix it up properly.
> >
> > Subsystems should be able to create ids how ever they want to, and not
> > rely on the code calling them to specify the names of the devices that
> > way, otherwise the api is just too fragile.
> >
> > I think, that if you create a device, then just carry around the pointer
> > to that device (in this case a phy) and pass it to whatever other code
> > needs it. No need to do lookups on "known names" or anything else,
> > just normal pointers, with no problems for multiple devices, busses, or
> > naming issues.
>
> PHY object is not a device, it is something that a device driver creates
> (one or more instances of) when it is being probed.
But you created a 'struct device' for it, so I think of it as a "device"
be it "virtual" or "real" :)
> You don't have a clean way to export this PHY object to other driver,
> other than keeping this PHY on a list inside PHY core with some
> well-known ID (e.g. device name + consumer port name/index, like in
> regulator core) and then to use this well-known ID inside consumer
> driver as a lookup key passed to phy_get();
>
> Actually I think for PHY case, exactly the same way as used for
> regulators might be completely fine:
>
> 1. Each PHY would have some kind of platform, non-unique name, that is
> just used to print some messages (like the platform/board name of a
> regulator).
> 2. Each PHY would have an array of consumers. Consumer specifier would
> consist of consumer device name and consumer port name - just like in
> regulator subsystem.
> 3. PHY driver receives an array of, let's say, phy_init_data inside its
> platform data that it would use to register its PHYs.
> 4. Consumer drivers would have constant consumer port names and wouldn't
> receive any information about PHYs from platform code.
>
> Code example:
>
> [Board file]
>
> static const struct phy_consumer_data usb_20_phy0_consumers[] = {
> {
> .devname = "foo-ehci",
> .port = "usbphy",
> },
> };
>
> static const struct phy_consumer_data usb_20_phy1_consumers[] = {
> {
> .devname = "foo-otg",
> .port = "otgphy",
> },
> };
>
> static const struct phy_init_data my_phys[] = {
> {
> .name = "USB 2.0 PHY 0",
> .consumers = usb_20_phy0_consumers,
> .num_consumers = ARRAY_SIZE(usb_20_phy0_consumers),
> },
> {
> .name = "USB 2.0 PHY 1",
> .consumers = usb_20_phy1_consumers,
> .num_consumers = ARRAY_SIZE(usb_20_phy1_consumers),
> },
> { }
> };
>
> static const struct platform_device usb_phy_pdev = {
> .name = "foo-usbphy",
> .id = -1,
> .dev = {
> .platform_data = my_phys,
> },
> };
>
> [PHY driver]
>
> static int foo_usbphy_probe(pdev)
> {
> struct foo_usbphy *foo;
> struct phy_init_data *init_data = pdev->dev.platform_data;
> /* ... */
> // for each PHY in init_data {
> phy_register(&foo->phy[i], &init_data[i]);
> // }
> /* ... */
&
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, Jul 23, 2013 at 08:31:05PM +0100, Mark Brown wrote: > > You don't "know" the id of the device you are looking up, due to > > multiple devices being in the system (dynamic ids, look back earlier in > > this thread for details about that.) > > I got copied in very late so don't have most of the thread I'm afraid, > I did try looking at web archives but didn't see a clear problem > statement. In any case this is why the APIs doing lookups do the > lookups in the context of the requesting device - devices ask for > whatever name they use locally. What do you mean by "locally"? The problem with the api was that the phy core wanted a id and a name to create a phy, and then later other code was doing a "lookup" based on the name and id (mushed together), because it "knew" that this device was the one it wanted. Just like the clock api, which, for multiple devices, has proven to cause problems. I don't want to see us accept an api that we know has issues in it now, I'd rather us fix it up properly. Subsystems should be able to create ids how ever they want to, and not rely on the code calling them to specify the names of the devices that way, otherwise the api is just too fragile. I think, that if you create a device, then just carry around the pointer to that device (in this case a phy) and pass it to whatever other code needs it. No need to do lookups on "known names" or anything else, just normal pointers, with no problems for multiple devices, busses, or naming issues. > > > Having to write platform data for everything gets old fast and the code > > > duplication is pretty tedious... > > > Adding a single pointer is "tedious"? Where is the "name" that you are > > going to lookup going to come from? That code doesn't write itself... > > It's adding platform data in the first place that gets tedious - and of > course there's also DT and ACPI to worry about, it's not just a case of > platform data and then you're done. Pushing the lookup into library > code means that drivers don't have to worry about any of this stuff. I agree, so just pass around the pointer to the phy and all is good. No need to worry about DT or ACPI or anything else. > For most of the APIs doing this there is a clear and unambiguous name in > the hardware that can be used (and for hardware process reasons is > unlikely to get changed). The major exception to this is the clock API > since it is relatively rare to have clear, segregated IP level > information for IPs baked into larger chips. The other APIs tend to be > establishing chip to chip links. The clock api is having problems with multiple "names" due to dynamic devices from what I was told. I want to prevent the PHY interface from having that same issue. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, Jul 23, 2013 at 07:48:11PM +0200, Tomasz Figa wrote:
> On Tuesday 23 of July 2013 10:37:11 Greg KH wrote:
> > On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote:
> > > > Ick, no. Why can't you just pass the pointer to the phy itself? If
> > > > you
> > > > had a "priv" pointer to search from, then you could have just passed
> > > > the
> > > > original phy pointer in the first place, right?
> > >
> > > IMHO it would be better if you provided some code example, but let's
> > > try to check if I understood you correctly.
> >
> > It's not my code that I want to have added, so I don't have to write
> > examples, I just get to complain about the existing stuff :)
>
> Still, I think that some small code snippets illustrating the idea are
> really helpful.
>
> > > 8><
> > >
> > >
> > > [Board file]
> > >
> > > static struct phy my_phy;
> > >
> > > static struct platform_device phy_pdev = {
> > >
> > > /* ... */
> > > .platform_data = &my_phy;
> > > /* ... */
> > >
> > > };
> > >
> > > static struct platform_device phy_pdev = {
> > >
> > > /* ... */
> > > .platform_data = &my_phy;
> > > /* ... */
> > >
> > > };
> > >
> > > [Provider driver]
> > >
> > > struct phy *phy = pdev->dev.platform_data;
> > >
> > > ret = phy_create(phy);
> > >
> > > [Consumer driver]
> > >
> > > struct phy *phy = pdev->dev.platform_data;
> > >
> > > ret = phy_get(&pdev->dev, phy);
> > >
> > > ---
> > > -><8
> > >
> > > Is this what you mean?
> >
> > No. Well, kind of. What's wrong with using the platform data structure
> > unique to the board to have the pointer?
> >
> > For example (just randomly picking one), the ata-pxa driver would change
> > include/linux/platform_data/ata-pxa.h to have a phy pointer in it:
> >
> > struct phy;
> >
> > struct pata_pxa_pdata {
> > /* PXA DMA DREQ<0:2> pin */
> > uint32_tdma_dreq;
> > /* Register shift */
> > uint32_treg_shift;
> > /* IRQ flags */
> > uint32_tirq_flags;
> > /* PHY */
> > struct phy *phy;
> > };
> >
> > Then, when you create the platform, set the phy* pointer with a call to
> > phy_create(). Then you can use that pointer wherever that plaform data
> > is available (i.e. whereever platform_data is at).
>
> Hmm? So, do you suggest to call phy_create() from board file? What phy_ops
> struct and other hardware parameters would it take?
>
> > > > The issue is that a string "name" is not going to scale at all, as it
> > > > requires hard-coded information that will change over time (as the
> > > > existing clock interface is already showing.)
> > >
> > > I fully agree that a simple, single string will not scale even in some,
> > > not so uncommon cases, but there is already a lot of existing lookup
> > > solutions over the kernel and so there is no point in introducing
> > > another one.
> > I'm trying to get _rid_ of lookup "solutions" and just use a real
> > pointer, as you should. I'll go tackle those other ones after this one
> > is taken care of, to show how the others should be handled as well.
>
> There was a reason for introducing lookup solutions. The reason was that in
> board file there is no way to get a pointer to something that is going to be
> created much later in time. We don't do time travel ;-).
>
> > > > Please just pass the real "phy" pointer around, that's what it is
> > > > there
> > > > for. Your "board binding" logic/code should be able to handle this,
> > > > as
> > > > it somehow was going to do the same thing with a "name".
> > >
> > > It's technically correct, but quality of this solution isn't really
> > > nice, because it's a layering violation (at least if I understood what
> > > you mean). This is because you need to have full definition of struct
> > > phy in board file and a structure that is used as p
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, Jul 23, 2013 at 06:44:56PM +0100, Mark Brown wrote: > On Tue, Jul 23, 2013 at 10:37:11AM -0700, Greg KH wrote: > > On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote: > > > > I fully agree that a simple, single string will not scale even in some, > > > not > > > so uncommon cases, but there is already a lot of existing lookup > > > solutions > > > over the kernel and so there is no point in introducing another one. > > > I'm trying to get _rid_ of lookup "solutions" and just use a real > > pointer, as you should. I'll go tackle those other ones after this one > > is taken care of, to show how the others should be handled as well. > > What are the problems you are seeing with doing things with lookups? You don't "know" the id of the device you are looking up, due to multiple devices being in the system (dynamic ids, look back earlier in this thread for details about that.) > Having to write platform data for everything gets old fast and the code > duplication is pretty tedious... Adding a single pointer is "tedious"? Where is the "name" that you are going to lookup going to come from? That code doesn't write itself... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote:
> > Ick, no. Why can't you just pass the pointer to the phy itself? If you
> > had a "priv" pointer to search from, then you could have just passed the
> > original phy pointer in the first place, right?
>
> IMHO it would be better if you provided some code example, but let's try to
> check if I understood you correctly.
It's not my code that I want to have added, so I don't have to write
examples, I just get to complain about the existing stuff :)
> 8><
>
> [Board file]
>
> static struct phy my_phy;
>
> static struct platform_device phy_pdev = {
> /* ... */
> .platform_data = &my_phy;
> /* ... */
> };
>
> static struct platform_device phy_pdev = {
> /* ... */
> .platform_data = &my_phy;
> /* ... */
> };
>
> [Provider driver]
>
> struct phy *phy = pdev->dev.platform_data;
>
> ret = phy_create(phy);
>
> [Consumer driver]
>
> struct phy *phy = pdev->dev.platform_data;
>
> ret = phy_get(&pdev->dev, phy);
>
> ><8
>
> Is this what you mean?
No. Well, kind of. What's wrong with using the platform data structure
unique to the board to have the pointer?
For example (just randomly picking one), the ata-pxa driver would change
include/linux/platform_data/ata-pxa.h to have a phy pointer in it:
struct phy;
struct pata_pxa_pdata {
/* PXA DMA DREQ<0:2> pin */
uint32_tdma_dreq;
/* Register shift */
uint32_treg_shift;
/* IRQ flags */
uint32_tirq_flags;
/* PHY */
struct phy *phy;
};
Then, when you create the platform, set the phy* pointer with a call to
phy_create(). Then you can use that pointer wherever that plaform data
is available (i.e. whereever platform_data is at).
> > The issue is that a string "name" is not going to scale at all, as it
> > requires hard-coded information that will change over time (as the
> > existing clock interface is already showing.)
>
> I fully agree that a simple, single string will not scale even in some, not
> so uncommon cases, but there is already a lot of existing lookup solutions
> over the kernel and so there is no point in introducing another one.
I'm trying to get _rid_ of lookup "solutions" and just use a real
pointer, as you should. I'll go tackle those other ones after this one
is taken care of, to show how the others should be handled as well.
> > Please just pass the real "phy" pointer around, that's what it is there
> > for. Your "board binding" logic/code should be able to handle this, as
> > it somehow was going to do the same thing with a "name".
>
> It's technically correct, but quality of this solution isn't really nice,
> because it's a layering violation (at least if I understood what you mean).
> This is because you need to have full definition of struct phy in board file
> and a structure that is used as private data in PHY core comes from
> platform code.
No, just a pointer, you don't need the "full" structure until you get to
some .c code that actually manipulates the phy itself, for all other
places, you are just dealing with a pointer and a structure you never
reference.
Does that make more sense?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, Jul 23, 2013 at 09:58:34PM +0530, Kishon Vijay Abraham I wrote:
> Hi Greg,
>
> On Tuesday 23 July 2013 09:48 PM, Greg KH wrote:
> > On Tue, Jul 23, 2013 at 08:48:24PM +0530, Kishon Vijay Abraham I wrote:
> >> Hi,
> >>
> >> On Tuesday 23 July 2013 08:07 PM, Alan Stern wrote:
> >>> On Tue, 23 Jul 2013, Tomasz Figa wrote:
> >>>
> >>>> On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
> >>>>> Hi Alan,
> >>>
> >>> Thanks for helping to clarify the issues here.
> >>>
> >>>>>> Okay. Are PHYs _always_ platform devices?
> >>>>>
> >>>>> They can be i2c, spi or any other device types as well.
> >>>
> >>> In those other cases, presumably there is no platform data associated
> >>> with the PHY since it isn't a platform device. Then how does the
> >>> kernel know which controller is attached to the PHY? Is this spelled
> >>> out in platform data associated with the PHY's i2c/spi/whatever parent?
> .
> .
>
> .
> .
> >>
> >>static struct phy *phy_lookup(void *priv) {
> >>.
> >>.
> >>if (phy->priv==priv) //instead of string comparison, we'll use
> >> pointer
> >>return phy;
> >>}
> >>
> >> PHY driver should be like
> >>phy_create((dev, ops, pdata->info);
> >>
> >> The controller driver would do
> >>phy_get(dev, NULL, pdata->info);
> >>
> >> Now the PHY framework will check for a match of *priv* pointer and return
> >> the PHY.
> >>
> >> I think this should be possible?
> >
> > Ick, no. Why can't you just pass the pointer to the phy itself? If you
> > had a "priv" pointer to search from, then you could have just passed the
> > original phy pointer in the first place, right?
> >
> > The issue is that a string "name" is not going to scale at all, as it
> > requires hard-coded information that will change over time (as the
> > existing clock interface is already showing.)
> >
> > Please just pass the real "phy" pointer around, that's what it is there
> > for. Your "board binding" logic/code should be able to handle this, as
> > it somehow was going to do the same thing with a "name".
>
> The problem is the board file won't have the *phy* pointer. *phy* pointer is
> created at a much later time when the phy driver is probed.
Ok, then save it then, as no one could have used it before then, right?
All I don't want to see is any "get by name/void *" functions in the
api, as that way is fragile and will break, as people have already
shown.
Just pass the real pointer around. If that is somehow a problem, then
something larger is a problem with how board devices are tied together :)
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, Jul 23, 2013 at 08:48:24PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 23 July 2013 08:07 PM, Alan Stern wrote:
> > On Tue, 23 Jul 2013, Tomasz Figa wrote:
> >
> >> On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
> >>> Hi Alan,
> >
> > Thanks for helping to clarify the issues here.
> >
> Okay. Are PHYs _always_ platform devices?
> >>>
> >>> They can be i2c, spi or any other device types as well.
> >
> > In those other cases, presumably there is no platform data associated
> > with the PHY since it isn't a platform device. Then how does the
> > kernel know which controller is attached to the PHY? Is this spelled
> > out in platform data associated with the PHY's i2c/spi/whatever parent?
>
> Yes. I think we could use i2c_board_info for passing platform data.
> >
> >>PHY. Currently this information is represented by name or
> >> ID
> >>strings embedded in platform data.
> >
> > right. It's embedded in the platform data of the controller.
>
> It must also be embedded in the PHY's platform data somehow.
> Otherwise, how would the kernel know which PHY to use?
> >>>
> >>> By using a PHY lookup as Stephen and I suggested in our previous
> >>> replies. Without any extra data in platform data. (I have even posted a
> >>> code example.)
> >
> > I don't understand, because I don't know what "a PHY lookup" does.
>
> It is how the PHY framework finds a PHY, when the controller (say USB)requests
> a PHY from the PHY framework.
> >
> In this case, it doesn't matter where the platform_device structures
> are created or where the driver source code is. Let's take a simple
> example. Suppose the system design includes a PHY named "foo". Then
> the board file could contain:
>
> struct phy_info { ... } phy_foo;
> EXPORT_SYMBOL_GPL(phy_foo);
>
> and a header file would contain:
>
> extern struct phy_info phy_foo;
>
> The PHY supplier could then call phy_create(&phy_foo), and the PHY
> client could call phy_find(&phy_foo). Or something like that; make up
> your own structure tags and function names.
>
> It's still possible to have conflicts, but now two PHYs with the same
> name (or a misspelled name somewhere) will cause an error at link
> time.
> >>>
> >>> This is incorrect, sorry. First of all it's a layering violation - you
> >>> export random driver-specific symbols from one driver to another. Then
> >
> > No, that's not what I said. Neither the PHY driver nor the controller
> > driver exports anything to the other. Instead, both drivers use data
> > exported by the board file.
>
> I think instead we can use the same data while creating the platform data of
> the controller and the PHY.
> The PHY driver while creating the PHY (using PHY framework) will also pass the
> *data* it actually got from the platform data to the framework.
> The PHY user driver (USB), while requesting for the PHY (from the PHY
> framework) will pass the *data* it got from its platform data.
> The PHY framework can do a comparison of the *data* pointers it has and return
> the appropriate PHY to the controller.
> >
> >>> imagine 4 SoCs - A, B, C, D. There are two PHY types PHY1 and PHY2 and
> >>> there are two types of consumer drivers (e.g. USB host controllers). Now
> >>> consider following mapping:
> >>>
> >>> SoC PHY consumer
> >>> A PHY1HOST1
> >>> B PHY1HOST2
> >>> C PHY2HOST1
> >>> D PHY2HOST2
> >>>
> >>> So we have to be able to use any of the PHYs with any of the host
> >>> drivers. This means you would have to export symbol with the same name
> >>> from both PHY drivers, which obviously would not work in this case,
> >>> because having both drivers enabled (in a multiplatform aware
> >>> configuration) would lead to linking conflict.
> >
> > You're right; the scheme was too simple. Instead, the board file must
> > export two types of data structures, one for PHYs and one for
> > controllers. Like this:
> >
> > struct phy_info {
> > /* Info for the controller attached to this PHY */
> > struct controller_info *hinfo;
> > };
> >
> > struct controller_info {
> > /* Info for the PHY which this controller is attached to */
> > struct phy_info *pinfo;
> > };
> >
> > The board file for SoC A would contain:
> >
> > struct phy_info phy1 = {&host1);
> > EXPORT_SYMBOL(phy1);
> > struct controller_info host1 = {&phy1};
> > EXPORT_SYMBOL(host1);
> >
> > The board file for SoC B would contain:
> >
> > struct phy_info phy1 = {&host2);
> > EXPORT_SYMBOL(phy1);
> > struct controller_info host2 = {&phy1};
> > EXPORT_SYMBOL(host2);
>
> I meant something like this
> struct phy_info {
> const char *name;
> };
>
> struct phy_platform_data {
> .
> .
> struct phy_info *info;
> };
>
> struct usb_controller_platform_data {
> .
> .
> struct phy_info *info;
> };
>
>
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Mon, Jul 22, 2013 at 12:55:18PM +0530, Kishon Vijay Abraham I wrote: > > The issue (or one of the issues) in this discussion is that > > Greg does not like the idea of using names or IDs to associate > > PHYs with controllers, because they are too prone to > > duplications or other errors. Pointers are more reliable. > > > > But pointers to what? Since the only data known to be > > available to both the PHY driver and controller driver is the > > platform data, the obvious answer is a pointer to platform data > > (either for the PHY or for the controller, or maybe both). > > hmm.. it's not going to be simple though as the platform device for the PHY > and > controller can be created in entirely different places. e.g., in some cases > the > PHY device is a child of some mfd core device (the device will be created in > drivers/mfd) and the controller driver (usually) is created in board file. I > guess then we have to come up with something to share a pointer in two > different files. What's wrong with using the platform_data structure that is unique to all boards (see include/platform_data/ for examples)? Isn't that what this structure is there for? greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Sun, Jul 21, 2013 at 12:22:48PM +0200, Sascha Hauer wrote: > On Sat, Jul 20, 2013 at 07:59:10PM -0700, Greg KH wrote: > > On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote: > > > On Sat, 20 Jul 2013, Greg KH wrote: > > > > > > > > >>That should be passed using platform data. > > > > > > > > > > > >Ick, don't pass strings around, pass pointers. If you have platform > > > > > >data you can get to, then put the pointer there, don't use a "name". > > > > > > > > > > I don't think I understood you here :-s We wont have phy pointer > > > > > when we create the device for the controller no?(it'll be done in > > > > > board file). Probably I'm missing something. > > > > > > > > Why will you not have that pointer? You can't rely on the "name" as the > > > > device id will not match up, so you should be able to rely on the > > > > pointer being in the structure that the board sets up, right? > > > > > > > > Don't use names, especially as ids can, and will, change, that is going > > > > to cause big problems. Use pointers, this is C, we are supposed to be > > > > doing that :) > > > > > > Kishon, I think what Greg means is this: The name you are using must > > > be stored somewhere in a data structure constructed by the board file, > > > right? Or at least, associated with some data structure somehow. > > > Otherwise the platform code wouldn't know which PHY hardware > > > corresponded to a particular name. > > > > > > Greg's suggestion is that you store the address of that data structure > > > in the platform data instead of storing the name string. Have the > > > consumer pass the data structure's address when it calls phy_create, > > > instead of passing the name. Then you don't have to worry about two > > > PHYs accidentally ending up with the same name or any other similar > > > problems. > > > > Close, but the issue is that whatever returns from phy_create() should > > then be used, no need to call any "find" functions, as you can just use > > the pointer that phy_create() returns. Much like all other class api > > functions in the kernel work. > > I think the problem here is to connect two from the bus structure > completely independent devices. Several frameworks (ASoC, soc-camera) > had this problem and this wasn't solved until the advent of devicetrees > and their phandles. > phy_create might be called from the probe function of some i2c device > (the phy device) and the resulting pointer is then needed in some other > platform devices (the user of the phy) probe function. > The best solution we have right now is implemented in the clk framework > which uses a string matching of the device names in clk_get() (at least > in the non-dt case). I would argue that clocks are wrong here as well, as others have already pointed out. What's wrong with the platform_data structure, why can't that be used for this? Or, if not, we can always add pointers to the platform device structure, or even the main 'struct device' as well, that's what it is there for. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Sun, Jul 21, 2013 at 01:12:07PM +0200, Tomasz Figa wrote:
> On Sunday 21 of July 2013 16:37:33 Kishon Vijay Abraham I wrote:
> > Hi,
> >
> > On Sunday 21 July 2013 04:01 PM, Tomasz Figa wrote:
> > > Hi,
> > >
> > > On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
> > >> On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
> > >>> On Sat, 20 Jul 2013, Greg KH wrote:
> > >>>>>>> That should be passed using platform data.
> > >>>>>>
> > >>>>>> Ick, don't pass strings around, pass pointers. If you have
> > >>>>>> platform
> > >>>>>> data you can get to, then put the pointer there, don't use a
> > >>>>>> "name".
> > >>>>>
> > >>>>> I don't think I understood you here :-s We wont have phy pointer
> > >>>>> when we create the device for the controller no?(it'll be done in
> > >>>>> board file). Probably I'm missing something.
> > >>>>
> > >>>> Why will you not have that pointer? You can't rely on the "name"
> > >>>> as
> > >>>> the device id will not match up, so you should be able to rely on
> > >>>> the pointer being in the structure that the board sets up, right?
> > >>>>
> > >>>> Don't use names, especially as ids can, and will, change, that is
> > >>>> going
> > >>>> to cause big problems. Use pointers, this is C, we are supposed to
> > >>>> be
> > >>>> doing that :)
> > >>>
> > >>> Kishon, I think what Greg means is this: The name you are using
> > >>> must
> > >>> be stored somewhere in a data structure constructed by the board
> > >>> file,
> > >>> right? Or at least, associated with some data structure somehow.
> > >>> Otherwise the platform code wouldn't know which PHY hardware
> > >>> corresponded to a particular name.
> > >>>
> > >>> Greg's suggestion is that you store the address of that data
> > >>> structure
> > >>> in the platform data instead of storing the name string. Have the
> > >>> consumer pass the data structure's address when it calls phy_create,
> > >>> instead of passing the name. Then you don't have to worry about two
> > >>> PHYs accidentally ending up with the same name or any other similar
> > >>> problems.
> > >>
> > >> Close, but the issue is that whatever returns from phy_create()
> > >> should
> > >> then be used, no need to call any "find" functions, as you can just
> > >> use
> > >> the pointer that phy_create() returns. Much like all other class api
> > >> functions in the kernel work.
> > >
> > > I think there is a confusion here about who registers the PHYs.
> > >
> > > All platform code does is registering a platform/i2c/whatever device,
> > > which causes a driver (located in drivers/phy/) to be instantiated.
> > > Such drivers call phy_create(), usually in their probe() callbacks,
> > > so platform_code has no way (and should have no way, for the sake of
> > > layering) to get what phy_create() returns.
Why not put pointers in the platform data structure that can hold these
pointers? I thought that is why we created those structures in the
first place. If not, what are they there for?
> > > IMHO we need a lookup method for PHYs, just like for clocks,
> > > regulators, PWMs or even i2c busses because there are complex cases
> > > when passing just a name using platform data will not work. I would
> > > second what Stephen said [1] and define a structure doing things in a
> > > DT-like way.
> > >
> > > Example;
> > >
> > > [platform code]
> > >
> > > static const struct phy_lookup my_phy_lookup[] = {
> > >
> > > PHY_LOOKUP("s3c-hsotg.0", "otg", "samsung-usbphy.1", "phy.2"),
> >
> > The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
> > creating the device, the ids in the device name would change and
> > PHY_LOOKUP wont be useful.
>
> I don't think this is a problem. All the existing lookup methods already
> use ID to identify devices (see regulators,
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote: > On Sat, 20 Jul 2013, Greg KH wrote: > > > > >>That should be passed using platform data. > > > > > > > >Ick, don't pass strings around, pass pointers. If you have platform > > > >data you can get to, then put the pointer there, don't use a "name". > > > > > > I don't think I understood you here :-s We wont have phy pointer > > > when we create the device for the controller no?(it'll be done in > > > board file). Probably I'm missing something. > > > > Why will you not have that pointer? You can't rely on the "name" as the > > device id will not match up, so you should be able to rely on the > > pointer being in the structure that the board sets up, right? > > > > Don't use names, especially as ids can, and will, change, that is going > > to cause big problems. Use pointers, this is C, we are supposed to be > > doing that :) > > Kishon, I think what Greg means is this: The name you are using must > be stored somewhere in a data structure constructed by the board file, > right? Or at least, associated with some data structure somehow. > Otherwise the platform code wouldn't know which PHY hardware > corresponded to a particular name. > > Greg's suggestion is that you store the address of that data structure > in the platform data instead of storing the name string. Have the > consumer pass the data structure's address when it calls phy_create, > instead of passing the name. Then you don't have to worry about two > PHYs accidentally ending up with the same name or any other similar > problems. Close, but the issue is that whatever returns from phy_create() should then be used, no need to call any "find" functions, as you can just use the pointer that phy_create() returns. Much like all other class api functions in the kernel work. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Sat, Jul 20, 2013 at 08:49:32AM +0530, Kishon Vijay Abraham I wrote: > Hi, > > On Saturday 20 July 2013 05:20 AM, Greg KH wrote: > >On Fri, Jul 19, 2013 at 12:06:01PM +0530, Kishon Vijay Abraham I wrote: > >>Hi, > >> > >>On Friday 19 July 2013 11:59 AM, Greg KH wrote: > >>>On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote: > >>>>Hi, > >>>> > >>>>On Friday 19 July 2013 11:13 AM, Greg KH wrote: > >>>>>On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote: > >>>>>>>>>>+ ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id); > >>>>>>>>> > >>>>>>>>>Your naming is odd, no "phy" anywhere in it? You rely on the sender > >>>>>>>>>to > >>>>>>>>>never send a duplicate name.id pair? Why not create your own ids > >>>>>>>>>based > >>>>>>>>>on the number of phys in the system, like almost all other classes > >>>>>>>>>and > >>>>>>>>>subsystems do? > >>>>>>>> > >>>>>>>>hmm.. some PHY drivers use the id they provide to perform some of > >>>>>>>>their > >>>>>>>>internal operation as in [1] (This is used only if a single PHY > >>>>>>>>provider > >>>>>>>>implements multiple PHYS). Probably I'll add an option like > >>>>>>>>PLATFORM_DEVID_AUTO > >>>>>>>>to give the PHY drivers an option to use auto id. > >>>>>>>> > >>>>>>>>[1] -> > >>>>>>>>http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html > >>>>>>> > >>>>>>>No, who cares about the id? No one outside of the phy core ever > >>>>>>>should, > >>>>>>>because you pass back the only pointer that they really do care about, > >>>>>>>if they need to do anything with the device. Use that, and then you > >>>>>>>can > >>>>>> > >>>>>>hmm.. ok. > >>>>>> > >>>>>>>rip out all of the "search for a phy by a string" logic, as that's not > >>>>>> > >>>>>>Actually this is needed for non-dt boot case. In the case of dt boot, > >>>>>>we use a > >>>>>>phandle by which the controller can get a reference to the phy. But in > >>>>>>the case > >>>>>>of non-dt boot, the controller can get a reference to the phy only by > >>>>>>label. > >>>>> > >>>>>I don't understand. They registered the phy, and got back a pointer to > >>>>>it. Why can't they save it in their local structure to use it again > >>>>>later if needed? They should never have to "ask" for the device, as the > >>>> > >>>>One is a *PHY provider* driver which is a driver for some PHY device. > >>>>This will > >>>>use phy_create to create the phy. > >>>>The other is a *PHY consumer* driver which might be any controller driver > >>>>(can > >>>>be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference > >>>>to the > >>>>phy (by *phandle* in the case of dt boot and *label* in the case of > >>>>non-dt boot). > >>>>>device id might be unknown if there are multiple devices in the system. > >>>> > >>>>I agree with you on the device id part. That need not be known to the PHY > >>>>driver. > >>> > >>>How does a consumer know which "label" to use in a non-dt system if > >>>there are multiple PHYs in the system? > >> > >>That should be passed using platform data. > > > >Ick, don't pass strings around, pass pointers. If you have platform > >data you can get to, then put the pointer there, don't use a "name". > > I don't think I understood you here :-s We wont have phy pointer > when we create the device for the controller no?(it'll be done in > board file). Probably I'm missing something. Why will you not have that pointer? You can't rely on the "name" as the device id will not match up, so you should be able to rely on the pointer being in the structure that the board sets up, right? Don't use names, especially as ids can, and will, change, that is going to cause big problems. Use pointers, this is C, we are supposed to be doing that :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Fri, Jul 19, 2013 at 12:06:01PM +0530, Kishon Vijay Abraham I wrote: > Hi, > > On Friday 19 July 2013 11:59 AM, Greg KH wrote: > > On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote: > >> Hi, > >> > >> On Friday 19 July 2013 11:13 AM, Greg KH wrote: > >>> On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote: > >>>>>>>> +ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id); > >>>>>>> > >>>>>>> Your naming is odd, no "phy" anywhere in it? You rely on the sender > >>>>>>> to > >>>>>>> never send a duplicate name.id pair? Why not create your own ids > >>>>>>> based > >>>>>>> on the number of phys in the system, like almost all other classes and > >>>>>>> subsystems do? > >>>>>> > >>>>>> hmm.. some PHY drivers use the id they provide to perform some of their > >>>>>> internal operation as in [1] (This is used only if a single PHY > >>>>>> provider > >>>>>> implements multiple PHYS). Probably I'll add an option like > >>>>>> PLATFORM_DEVID_AUTO > >>>>>> to give the PHY drivers an option to use auto id. > >>>>>> > >>>>>> [1] -> > >>>>>> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html > >>>>> > >>>>> No, who cares about the id? No one outside of the phy core ever should, > >>>>> because you pass back the only pointer that they really do care about, > >>>>> if they need to do anything with the device. Use that, and then you can > >>>> > >>>> hmm.. ok. > >>>> > >>>>> rip out all of the "search for a phy by a string" logic, as that's not > >>>> > >>>> Actually this is needed for non-dt boot case. In the case of dt boot, we > >>>> use a > >>>> phandle by which the controller can get a reference to the phy. But in > >>>> the case > >>>> of non-dt boot, the controller can get a reference to the phy only by > >>>> label. > >>> > >>> I don't understand. They registered the phy, and got back a pointer to > >>> it. Why can't they save it in their local structure to use it again > >>> later if needed? They should never have to "ask" for the device, as the > >> > >> One is a *PHY provider* driver which is a driver for some PHY device. This > >> will > >> use phy_create to create the phy. > >> The other is a *PHY consumer* driver which might be any controller driver > >> (can > >> be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to > >> the > >> phy (by *phandle* in the case of dt boot and *label* in the case of non-dt > >> boot). > >>> device id might be unknown if there are multiple devices in the system. > >> > >> I agree with you on the device id part. That need not be known to the PHY > >> driver. > > > > How does a consumer know which "label" to use in a non-dt system if > > there are multiple PHYs in the system? > > That should be passed using platform data. Ick, don't pass strings around, pass pointers. If you have platform data you can get to, then put the pointer there, don't use a "name". thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote: > Hi, > > On Friday 19 July 2013 11:13 AM, Greg KH wrote: > > On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote: > >>>>>> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id); > >>>>> > >>>>> Your naming is odd, no "phy" anywhere in it? You rely on the sender to > >>>>> never send a duplicate name.id pair? Why not create your own ids based > >>>>> on the number of phys in the system, like almost all other classes and > >>>>> subsystems do? > >>>> > >>>> hmm.. some PHY drivers use the id they provide to perform some of their > >>>> internal operation as in [1] (This is used only if a single PHY provider > >>>> implements multiple PHYS). Probably I'll add an option like > >>>> PLATFORM_DEVID_AUTO > >>>> to give the PHY drivers an option to use auto id. > >>>> > >>>> [1] -> > >>>> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html > >>> > >>> No, who cares about the id? No one outside of the phy core ever should, > >>> because you pass back the only pointer that they really do care about, > >>> if they need to do anything with the device. Use that, and then you can > >> > >> hmm.. ok. > >> > >>> rip out all of the "search for a phy by a string" logic, as that's not > >> > >> Actually this is needed for non-dt boot case. In the case of dt boot, we > >> use a > >> phandle by which the controller can get a reference to the phy. But in the > >> case > >> of non-dt boot, the controller can get a reference to the phy only by > >> label. > > > > I don't understand. They registered the phy, and got back a pointer to > > it. Why can't they save it in their local structure to use it again > > later if needed? They should never have to "ask" for the device, as the > > One is a *PHY provider* driver which is a driver for some PHY device. This > will > use phy_create to create the phy. > The other is a *PHY consumer* driver which might be any controller driver (can > be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to the > phy (by *phandle* in the case of dt boot and *label* in the case of non-dt > boot). > > device id might be unknown if there are multiple devices in the system. > > I agree with you on the device id part. That need not be known to the PHY > driver. How does a consumer know which "label" to use in a non-dt system if there are multiple PHYs in the system? Do you have any drivers that are non-dt using this yet? greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote: > +ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id); > >>> > >>> Your naming is odd, no "phy" anywhere in it? You rely on the sender to > >>> never send a duplicate name.id pair? Why not create your own ids based > >>> on the number of phys in the system, like almost all other classes and > >>> subsystems do? > >> > >> hmm.. some PHY drivers use the id they provide to perform some of their > >> internal operation as in [1] (This is used only if a single PHY provider > >> implements multiple PHYS). Probably I'll add an option like > >> PLATFORM_DEVID_AUTO > >> to give the PHY drivers an option to use auto id. > >> > >> [1] -> > >> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html > > > > No, who cares about the id? No one outside of the phy core ever should, > > because you pass back the only pointer that they really do care about, > > if they need to do anything with the device. Use that, and then you can > > hmm.. ok. > > > rip out all of the "search for a phy by a string" logic, as that's not > > Actually this is needed for non-dt boot case. In the case of dt boot, we use a > phandle by which the controller can get a reference to the phy. But in the > case > of non-dt boot, the controller can get a reference to the phy only by label. I don't understand. They registered the phy, and got back a pointer to it. Why can't they save it in their local structure to use it again later if needed? They should never have to "ask" for the device, as the device id might be unknown if there are multiple devices in the system. Or am I missing something? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Thu, Jul 18, 2013 at 02:29:52PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 18 July 2013 12:50 PM, Greg KH wrote:
> > On Thu, Jul 18, 2013 at 12:16:10PM +0530, Kishon Vijay Abraham I wrote:
> >> +struct phy_provider *__of_phy_provider_register(struct device *dev,
> >> + struct module *owner, struct phy * (*of_xlate)(struct device *dev,
> >> + struct of_phandle_args *args));
> >> +struct phy_provider *__devm_of_phy_provider_register(struct device *dev,
> >> + struct module *owner, struct phy * (*of_xlate)(struct device *dev,
> >> + struct of_phandle_args *args))
> >> +
> >> +__of_phy_provider_register and __devm_of_phy_provider_register can be
> >> used to
> >> +register the phy_provider and it takes device, owner and of_xlate as
> >> +arguments. For the dt boot case, all PHY providers should use one of the
> >> above
> >> +2 APIs to register the PHY provider.
> >
> > Why do you have __ for the prefix of a public function? Is that really
> > the way that OF handles this type of thing?
>
> I have a macro of_phy_provider_register/devm_of_phy_provider_register that
> calls these functions and should be used by the PHY drivers. Probably I should
> make a mention of it in the Documentation.
Yes, mention those as you never want to be calling __* functions
directly, right?
> >> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
> >
> > Your naming is odd, no "phy" anywhere in it? You rely on the sender to
> > never send a duplicate name.id pair? Why not create your own ids based
> > on the number of phys in the system, like almost all other classes and
> > subsystems do?
>
> hmm.. some PHY drivers use the id they provide to perform some of their
> internal operation as in [1] (This is used only if a single PHY provider
> implements multiple PHYS). Probably I'll add an option like
> PLATFORM_DEVID_AUTO
> to give the PHY drivers an option to use auto id.
>
> [1] ->
> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html
No, who cares about the id? No one outside of the phy core ever should,
because you pass back the only pointer that they really do care about,
if they need to do anything with the device. Use that, and then you can
rip out all of the "search for a phy by a string" logic, as that's not
needed either. Just stick to the pointer, it's easier, and safer that
way.
> >> +static inline int phy_pm_runtime_get(struct phy *phy)
> >> +{
> >> + if (WARN(IS_ERR(phy), "Invalid PHY reference\n"))
> >> + return -EINVAL;
> >
> > Why would phy ever not be valid and a error pointer? And why dump the
> > stack if that happens, that seems really extreme.
>
> hmm.. there might be cases where the same controller in one soc needs PHY
> control and in some other soc does not need PHY control. In such cases, we
> might get error pointer here.
> I'll change WARN to dev_err.
I still don't understand. You have control over the code that calls
these functions, just ensure that they pass in a valid pointer, it's
that simple. Or am I missing something?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/15] usb: phy: omap-usb2: use the new generic PHY framework
On Thu, Jul 18, 2013 at 12:16:11PM +0530, Kishon Vijay Abraham I wrote:
> Used the generic PHY framework API to create the PHY. Now the power off and
> power on are done in omap_usb_power_off and omap_usb_power_on respectively.
>
> However using the old USB PHY library cannot be completely removed
> because OTG is intertwined with PHY and moving to the new framework
> will break OTG. Once we have a separate OTG state machine, we
> can get rid of the USB PHY library.
>
> Signed-off-by: Kishon Vijay Abraham I
> Reviewed-by: Sylwester Nawrocki
> Acked-by: Felipe Balbi
> ---
> drivers/usb/phy/Kconfig |1 +
> drivers/usb/phy/phy-omap-usb2.c | 45
> +++
> 2 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index 3622fff..cc55993 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -75,6 +75,7 @@ config OMAP_CONTROL_USB
> config OMAP_USB2
> tristate "OMAP USB2 PHY Driver"
> depends on ARCH_OMAP2PLUS
> + depends on GENERIC_PHY
> select OMAP_CONTROL_USB
> help
> Enable this to support the transceiver that is part of SOC. This
> diff --git a/drivers/usb/phy/phy-omap-usb2.c b/drivers/usb/phy/phy-omap-usb2.c
> index 844ab68..751b30f 100644
> --- a/drivers/usb/phy/phy-omap-usb2.c
> +++ b/drivers/usb/phy/phy-omap-usb2.c
> @@ -28,6 +28,7 @@
> #include
> #include
> #include
> +#include
>
> /**
> * omap_usb2_set_comparator - links the comparator present in the sytem with
> @@ -119,10 +120,36 @@ static int omap_usb2_suspend(struct usb_phy *x, int
> suspend)
> return 0;
> }
>
> +static int omap_usb_power_off(struct phy *x)
> +{
> + struct omap_usb *phy = phy_get_drvdata(x);
> +
> + omap_control_usb_phy_power(phy->control_dev, 0);
> +
> + return 0;
> +}
> +
> +static int omap_usb_power_on(struct phy *x)
> +{
> + struct omap_usb *phy = phy_get_drvdata(x);
> +
> + omap_control_usb_phy_power(phy->control_dev, 1);
> +
> + return 0;
> +}
> +
> +static struct phy_ops ops = {
> + .power_on = omap_usb_power_on,
> + .power_off = omap_usb_power_off,
> + .owner = THIS_MODULE,
> +};
> +
> static int omap_usb2_probe(struct platform_device *pdev)
> {
> struct omap_usb *phy;
> + struct phy *generic_phy;
> struct usb_otg *otg;
> + struct phy_provider *phy_provider;
>
> phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
> if (!phy) {
> @@ -144,6 +171,11 @@ static int omap_usb2_probe(struct platform_device *pdev)
> phy->phy.otg= otg;
> phy->phy.type = USB_PHY_TYPE_USB2;
>
> + phy_provider = devm_of_phy_provider_register(phy->dev,
> + of_phy_simple_xlate);
> + if (IS_ERR(phy_provider))
> + return PTR_ERR(phy_provider);
> +
> phy->control_dev = omap_get_control_dev();
> if (IS_ERR(phy->control_dev)) {
> dev_dbg(&pdev->dev, "Failed to get control device\n");
> @@ -159,6 +191,15 @@ static int omap_usb2_probe(struct platform_device *pdev)
> otg->start_srp = omap_usb_start_srp;
> otg->phy= &phy->phy;
>
> + platform_set_drvdata(pdev, phy);
> + pm_runtime_enable(phy->dev);
> +
> + generic_phy = devm_phy_create(phy->dev, 0, &ops, "omap-usb2");
> + if (IS_ERR(generic_phy))
> + return PTR_ERR(generic_phy);
So, if I have two of these controllers in my system, I can't create the
second phy because the name for it will be identical to the first?
That's why the phy core should handle the id, and not rely on the
drivers to set it, as they have no idea how many they have in the
system.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Thu, Jul 18, 2013 at 12:16:10PM +0530, Kishon Vijay Abraham I wrote:
> +struct phy_provider *__of_phy_provider_register(struct device *dev,
> + struct module *owner, struct phy * (*of_xlate)(struct device *dev,
> + struct of_phandle_args *args));
> +struct phy_provider *__devm_of_phy_provider_register(struct device *dev,
> + struct module *owner, struct phy * (*of_xlate)(struct device *dev,
> + struct of_phandle_args *args))
> +
> +__of_phy_provider_register and __devm_of_phy_provider_register can be used to
> +register the phy_provider and it takes device, owner and of_xlate as
> +arguments. For the dt boot case, all PHY providers should use one of the
> above
> +2 APIs to register the PHY provider.
Why do you have __ for the prefix of a public function? Is that really
the way that OF handles this type of thing?
> --- /dev/null
> +++ b/drivers/phy/Kconfig
> @@ -0,0 +1,13 @@
> +#
> +# PHY
> +#
> +
> +menuconfig GENERIC_PHY
> + tristate "PHY Subsystem"
> + help
> + Generic PHY support.
> +
> + This framework is designed to provide a generic interface for PHY
> + devices present in the kernel. This layer will have the generic
> + API by which phy drivers can create PHY using the phy framework and
> + phy users can obtain reference to the PHY.
Again, please reverse this. The drivers that use it should select it,
not depend on it, which will then enable this option. I will never know
if I need to enable it, and based on your follow-on patches, if I don't,
drivers that were working just fine, now disappeared from my build,
which isn't nice, and a pain to notice and fix up.
> +/**
> + * phy_create() - create a new phy
> + * @dev: device that is creating the new phy
> + * @id: id of the phy
> + * @ops: function pointers for performing phy operations
> + * @label: label given to the phy
> + *
> + * Called to create a phy using phy framework.
> + */
> +struct phy *phy_create(struct device *dev, u8 id, const struct phy_ops *ops,
> + const char *label)
> +{
> + int ret;
> + struct phy *phy;
> +
> + if (!dev) {
> + dev_WARN(dev, "no device provided for PHY\n");
> + ret = -EINVAL;
> + goto err0;
> + }
> +
> + phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> + if (!phy) {
> + ret = -ENOMEM;
> + goto err0;
> + }
> +
> + device_initialize(&phy->dev);
> + mutex_init(&phy->mutex);
> +
> + phy->dev.class = phy_class;
> + phy->dev.parent = dev;
> + phy->dev.of_node = dev->of_node;
> + phy->id = id;
> + phy->ops = ops;
> + phy->label = kstrdup(label, GFP_KERNEL);
> +
> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
Your naming is odd, no "phy" anywhere in it? You rely on the sender to
never send a duplicate name.id pair? Why not create your own ids based
on the number of phys in the system, like almost all other classes and
subsystems do?
> +static inline int phy_pm_runtime_get(struct phy *phy)
> +{
> + if (WARN(IS_ERR(phy), "Invalid PHY reference\n"))
> + return -EINVAL;
Why would phy ever not be valid and a error pointer? And why dump the
stack if that happens, that seems really extreme.
> +
> + if (!pm_runtime_enabled(&phy->dev))
> + return -ENOTSUPP;
> +
> + return pm_runtime_get(&phy->dev);
> +}
This, and the other inline functions in this .h file seem huge, why are
they inline and not in the .c file? There's no speed issues, and it
should save space overall in the .c file. Please move them.
> +static inline int phy_init(struct phy *phy)
> +{
> + int ret;
> +
> + ret = phy_pm_runtime_get_sync(phy);
> + if (ret < 0 && ret != -ENOTSUPP)
> + return ret;
> +
> + mutex_lock(&phy->mutex);
> + if (phy->init_count++ == 0 && phy->ops->init) {
> + ret = phy->ops->init(phy);
> + if (ret < 0) {
> + dev_err(&phy->dev, "phy init failed --> %d\n", ret);
> + goto out;
> + }
> + }
> +
> +out:
> + mutex_unlock(&phy->mutex);
> + phy_pm_runtime_put(phy);
> + return ret;
> +}
> +
> +static inline int phy_exit(struct phy *phy)
> +{
> + int ret;
> +
> + ret = phy_pm_runtime_get_sync(phy);
> + if (ret < 0 && ret != -ENOTSUPP)
> + return ret;
> +
> + mutex_lock(&phy->mutex);
> + if (--phy->init_count == 0 && phy->ops->exit) {
> + ret = phy->ops->exit(phy);
> + if (ret < 0) {
> + dev_err(&phy->dev, "phy exit failed --> %d\n", ret);
> + goto out;
> + }
> + }
> +
> +out:
> + mutex_unlock(&phy->mutex);
> + phy_pm_runtime_put(phy);
> + return ret;
> +}
> +
> +static inline int phy_power_on(struct phy *phy)
> +{
> + int ret = -ENOTSUPP;
> +
> + ret = phy_pm_runtime_get_sync(phy);
> + if (ret < 0 && ret != -ENOTSUPP)
> + return ret;
> +
> +
Re: [PATCH v2 2/2] usb: dwc3-exynos: Fix compatible strings for the device
On Thu, Jan 24, 2013 at 07:15:30PM +0530, Vivek Gautam wrote: > Using specific chip in compatible strings. Newer SOCs can claim > device by using older string in the compatible list. > > Signed-off-by: Vivek Gautam > Acked-by: Grant Likely > Reviewed-by: Doug Anderson > --- > drivers/usb/dwc3/dwc3-exynos.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) Felipe, want me to take this one, or will you? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: samsung: protect NULL dereference of clock name
On Wed, Jun 20, 2012 at 01:28:47PM +0900, Kukjin Kim wrote: > Kyoungil Kim wrote: > > > > From: KeyYoung Park > > > > When priting the serial clock source, if clock source name is null, > > kernel reference NULL point. > > > > Signed-off-by: KeyYoung Park > > Signed-off-by: Huisung Kang > > Signed-off-by: Kyoungil Kim > > (Cc'ed Greg) > > Acked-by: Kukjin Kim > > Greg, could you please pick this up? Already have, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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] serial: samsung: Fixed wrong comparison for baudclk_rate
On Wed, Jun 20, 2012 at 01:28:40PM +0900, Kukjin Kim wrote: > Kyoungil Kim wrote: > > > > port->baudclk_rate should be compared to the rate of port->baudclk, > > because port->baudclk_rate was assigned as the rate of port->baudclk > > previously. > > So to check that the current baudclk rate is same as previous rate, > > the target of comparison sholud be the rate of port->baudclk. > > > > Signed-off-by: Jun-Ho, Yoon > > Signed-off-by: Kyoungil Kim > > (Cc'ed Greg) > > Acked-by: Kukjin Kim > > Greg, this is needed to fix the wrong comparison for baudclk_rate for > Samsung Serial. > Could you please pick this up in your fix tree? Already there, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] serial: samsung: Remove NULL checking for baud clock
On Wed, Jun 20, 2012 at 01:28:36PM +0900, Kukjin Kim wrote:
> Kukjin Kim wrote:
> >
> > Kyoungil Kim wrote:
> > >
> > > Kyoungil Kim wrote:
> > >
> > > I missed following.
> > >
> > > Russell King suggested:
> > >
> > > As I said, drivers have no business interpreting anything but
> IS_ERR(clk)
> > > as being an error. They should not make any other
> > > assumptions.
> > >
> > > Suggested-by: Russell King
> > >
> > > > Signed-off-by: Kyoungil Kim
> > > > ---
> > > > drivers/tty/serial/samsung.c | 21 -
> > > > 1 files changed, 12 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/samsung.c
> > b/drivers/tty/serial/samsung.c
> > > > index d8b0aee..5668538 100644
> > > > --- a/drivers/tty/serial/samsung.c
> > > > +++ b/drivers/tty/serial/samsung.c
> > > > @@ -529,7 +529,7 @@ static void s3c24xx_serial_pm(struct uart_port
> > *port,
> > > unsigned int level,
> > > >
> > > > switch (level) {
> > > > case 3:
> > > > - if (!IS_ERR(ourport->baudclk) && ourport->baudclk !=
> > > > NULL)
> > > > + if (!IS_ERR(ourport->baudclk))
> > > > clk_disable(ourport->baudclk);
> > > >
> > > > clk_disable(ourport->clk);
> > > > @@ -538,7 +538,7 @@ static void s3c24xx_serial_pm(struct uart_port
> > *port,
> > > unsigned int level,
> > > > case 0:
> > > > clk_enable(ourport->clk);
> > > >
> > > > - if (!IS_ERR(ourport->baudclk) && ourport->baudclk !=
> > > > NULL)
> > > > + if (!IS_ERR(ourport->baudclk))
> > > > clk_enable(ourport->baudclk);
> > > >
> > > > break;
> > > > @@ -604,7 +604,6 @@ static unsigned int s3c24xx_serial_getclk(struct
> > > s3c24xx_uart_port *ourport,
> > > > char clkname[MAX_CLK_NAME_LENGTH];
> > > > int calc_deviation, deviation = (1 << 30) - 1;
> > > >
> > > > - *best_clk = NULL;
> > > > clk_sel = (ourport->cfg->clk_sel) ? ourport->cfg->clk_sel :
> > > > ourport->info->def_clk_sel;
> > > > for (cnt = 0; cnt < info->num_clks; cnt++) {
> > > > @@ -613,7 +612,7 @@ static unsigned int s3c24xx_serial_getclk(struct
> > > s3c24xx_uart_port *ourport,
> > > >
> > > > sprintf(clkname, "clk_uart_baud%d", cnt);
> > > > clk = clk_get(ourport->port.dev, clkname);
> > > > - if (IS_ERR_OR_NULL(clk))
> > > > + if (IS_ERR(clk))
> > > > continue;
> > > >
> > > > rate = clk_get_rate(clk);
> > > > @@ -684,7 +683,7 @@ static void s3c24xx_serial_set_termios(struct
> > > uart_port *port,
> > > > {
> > > > struct s3c2410_uartcfg *cfg = s3c24xx_port_to_cfg(port);
> > > > struct s3c24xx_uart_port *ourport = to_ourport(port);
> > > > - struct clk *clk = NULL;
> > > > + struct clk *clk = ERR_PTR(-EINVAL);
> > > > unsigned long flags;
> > > > unsigned int baud, quot, clk_sel = 0;
> > > > unsigned int ulcon;
> > > > @@ -705,7 +704,7 @@ static void s3c24xx_serial_set_termios(struct
> > > uart_port *port,
> > > > quot = s3c24xx_serial_getclk(ourport, baud, &clk, &clk_sel);
> > > > if (baud == 38400 && (port->flags & UPF_SPD_MASK) ==
> > > > UPF_SPD_CUST)
> > > > quot = port->custom_divisor;
> > > > - if (!clk)
> > > > + if (IS_ERR(clk))
> > > > return;
> > > >
> > > > /* check to see if we need to change clock source */
> > > > @@ -713,9 +712,9 @@ static void s3c24xx_serial_set_termios(struct
> > > uart_port *port,
> > > > if (ourport->baudclk != clk) {
> > > > s3c24xx_serial_setsource(port, clk_sel);
> > > >
> > > > - if (ourport->baudclk != NULL &&
> > > > !IS_ERR(ourport->baudclk)) {
> > > > + if (!IS_ERR(ourport->baudclk)) {
> > > > clk_disable(ourport->baudclk);
> > > > - ourport->baudclk = NULL;
> > > > + ourport->baudclk = ERR_PTR(-EINVAL);
> > > > }
> > > >
> > > > clk_enable(clk);
> > > > @@ -1160,6 +1159,9 @@ static ssize_t s3c24xx_serial_show_clksrc(struct
> > > device *dev,
> > > > struct uart_port *port = s3c24xx_dev_to_port(dev);
> > > > struct s3c24xx_uart_port *ourport = to_ourport(port);
> > > >
> > > > + if (IS_ERR(ourport->baudclk))
> > > > + return -EINVAL;
> > > > +
> > > > return snprintf(buf, PAGE_SIZE, "* %s\n",
> > > > ourport->baudclk->name);
> > > > }
> > > >
> > > > @@ -1200,6 +1202,7 @@ static int s3c24xx_serial_probe(struct
> > > platform_device *pdev)
> > > > return -ENODEV;
> > > > }
> > > >
> > > > + ourport->baudclk = ERR_PTR(-EINVAL);
> > > > ourport->info = ourport->drv_data->info;
> > > > ourport->cfg = (pdev->dev.platform_data) ?
> > > >
Re: [GIT PULL] Samsung usb stuff for v3.5
On Thu, May 17, 2012 at 03:45:55PM +0900, Kukjin Kim wrote: > Hi Greg and Felipe, > > Please pull Samsung 's3c-hsotg' UDC support for EXYNOS4210 and S5PV210 from: > git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git > v3.5-for-usb > > Since it has a dependency on 'usb: hsotg: samsung ...' patches which have > been already in usb tree now, so would be better if you could pull this > series in your tree. Or this can be sent to upstream in the end of upcoming > merge window after pulling usb tree. But I think usb tree is better in this > case. > > If any problems, please kindly let me know. All pulled into my usb-next branch now, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Samsung usb stuff for v3.5
On Thu, May 17, 2012 at 03:45:55PM +0900, Kukjin Kim wrote: > Hi Greg and Felipe, > > Please pull Samsung 's3c-hsotg' UDC support for EXYNOS4210 and S5PV210 from: > git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git > v3.5-for-usb > > Since it has a dependency on 'usb: hsotg: samsung ...' patches which have > been already in usb tree now, so would be better if you could pull this > series in your tree. Or this can be sent to upstream in the end of upcoming > merge window after pulling usb tree. But I think usb tree is better in this > case. > > If any problems, please kindly let me know. > > Thanks. > > Best regards, > Kgene. > -- > Kukjin Kim , Senior Engineer, > SW Solution Development Team, Samsung Electronics Co., Ltd. > > The following changes since commit 36be50515fe2aef61533b516fa2576a2c7fe7664: > > Linux 3.4-rc7 (2012-05-12 18:37:47 -0700) > > are available in the git repository at: > git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git > v3.5-for-usb > > Joonyoung Shim (1): > ARM: EXYNOS: Add s3c-hsotg device support for NURI board > > Lukasz Majewski (3): > ARM: EXYNOS: Add usb otg phy control for EXYNOS4210 > ARM: EXYNOS: Add s3c-hsotg device support for GONI board > ARM: EXYNOS: Add s3c-hsotg device support for Universal C210 board > > arch/arm/mach-exynos/Kconfig |3 + > arch/arm/mach-exynos/include/mach/irqs.h |1 + > arch/arm/mach-exynos/include/mach/map.h |4 + > arch/arm/mach-exynos/include/mach/regs-pmu.h |3 + > arch/arm/mach-exynos/mach-nuri.c |9 ++- > arch/arm/mach-exynos/mach-universal_c210.c | 10 +++ > arch/arm/mach-exynos/setup-usb-phy.c | 100 > ++--- > arch/arm/mach-s5pv210/Kconfig|1 + > arch/arm/mach-s5pv210/mach-goni.c|5 ++ > 9 files changed, 107 insertions(+), 29 deletions(-) Hm, this branch is against 3.4-rc7, while my tree is currently at 3.4-rc6, so it pulls in Linus's recent merges. Which might be ok, let me see how it goes... greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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/3] USB: SET SEL request definition
On Mon, Feb 06, 2012 at 05:12:33PM +0900, Anton Tikhomirov wrote: > Cc: Kukjin Kim samsung.com> > Cc: Greg Kroah-Hartman suse.de> > Cc: Felipe Balbi ti.com> What is that mess? It belongs, with real email addresses, below your signed-off-by line, if at all. > Adds SET SEL standard request definition as defined by ch9 > of the USB3.0 specification. > > Signed-off-by: Anton Tikhomirov > --- > include/linux/usb/ch9.h |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h > index 61b2905..76ff771 100644 > --- a/include/linux/usb/ch9.h > +++ b/include/linux/usb/ch9.h > @@ -88,6 +88,7 @@ > #define USB_REQ_GET_INTERFACE0x0A > #define USB_REQ_SET_INTERFACE0x0B > #define USB_REQ_SYNCH_FRAME 0x0C > +#define USB_REQ_SET_SEL 0x30 > > #define USB_REQ_SET_ENCRYPTION 0x0D/* Wireless USB */ > #define USB_REQ_GET_ENCRYPTION 0x0E Why did you insert this out of order? greg "please note my email address change as well" k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm: Fix missing api-change from subsys_interface change
On Thu, Jan 12, 2012 at 02:30:48PM +0100, Heiko Stübner wrote: > Commit 4a858cfc9a (arm: convert sysdev_class to a regular subsystem) > converted the samsung sysdevs into subsys_interface instances. > > While the original add-function only had a (struct sys_device *) > parameter, the dev_add from subsys_interface needs > (struct device *, struct subsys_interface *) > leading to "initialized from incompatible pointer type" warnings. > > Signed-off-by: Heiko Stuebner > --- > The patch applies cleanly against the current driver-core git, > hopefully it's the right tree. Thanks for fixing this up, sorry for getting it wrong. This should be done against the Samsung arm git tree, and go in through that one. The driver-core git tree is equal to Linus's tree of a few days ago, there are no driver-core patches in it at the moment. Kukjin, can you take these? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH 0/3] Support Samsung S5P OHCI device and driver
On Wed, Dec 21, 2011 at 01:38:24PM +0900, Kukjin Kim wrote: > > > > -Original Message- > > From: Greg KH [mailto:g...@kroah.com] > > Sent: Saturday, December 10, 2011 8:58 AM > > To: Kukjin Kim > > Cc: 'Jingoo Han'; linux-arm-ker...@lists.infradead.org; linux- > > u...@vger.kernel.org; linux-samsung-soc@vger.kernel.org; gre...@suse.de; > > st...@rowland.harvard.edu; 'Yulgon Kim'; 'Shim Joonyoung' > > Subject: Re: Re: [PATCH 0/3] Support Samsung S5P OHCI device and driver > > > > On Thu, Dec 01, 2011 at 07:10:17PM +0900, Kukjin Kim wrote: > > > Greg KH wrote: > > > > > > > > On Mon, Nov 21, 2011 at 11:57:16AM +, Jingoo Han wrote: > > > > > > -Original Message- > > > > > > From: Greg KH [mailto:g...@kroah.com] > > > > > > Sent: Friday, November 18, 2011 4:12 AM > > > > > > To: Jingoo Han > > > > > > Cc: linux-arm-ker...@lists.infradead.org; linux- > > u...@vger.kernel.org; > > > > > > linux-samsung-soc@vger.kernel.org; gre...@suse.de; > > > > > > st...@rowland.harvard.edu; kgene@samsung.com; > > > > yulgon@samsung.com; > > > > > > jy0922.s...@samsung.com > > > > > > Subject: Re: [PATCH 0/3] Support Samsung S5P OHCI device and > > driver > > > > > > > > > > > > On Tue, Nov 15, 2011 at 06:49:58AM +, Jingoo Han wrote: > > > > > > > Hello. > > > > > > > > > > > > > > This patch series adds USB OHCI device and initial driver for > > > > Samsung > > > > > > > S5P SoCs and is based from Linux 3.2-rc1. I have tested on > > SMDKV310 > > > > > > board > > > > > > > using EXYNOS4. > > > > > > > > > > > > This is to be sent through some arm tree, not the usb tree, right? > > > > > Right, 1st and 2nd patch are sent throught arm tree. > > > > > However, 3rd patch is sent through the usb tree. > > > > > Thanks. > > > > > [PATCH 1/3] ARM: SAMSUNG: Add USB OHCI device > > > > > [PATCH 2/3] ARM: EXYNOS: Add USB OHCI support to SMDKV310 board > > > > > [PATCH 3/3] USB: Add S5P OHCI diver > > > > > > > Hi Greg, > > > > > > Sorry for late response. I came back today from family vacation... > > > > > > Anyway, > > > > > > > So the third patch does not depend on the first two? If it does, that > > > > means I will get build errors in the USB tree when I apply that patch, > > > > right? > > > > > > > > If so, then all three should go through a single tree. > > > > > > > Yes, right. This series would be handled in same tree so that we can > > avoid > > > useless conflicts. > > > > > > I commented small thing on 3rd patch. So I think, if you're ok on his > > > updated patch, it can be sent to upstream via Samsung tree. Or your tree > > > with my ack? > > > > The samsung tree is fine to take these, thanks. > > > Hi Greg, > > OK, let me pick this up in my tree...so, when I apply, may I can have a ack > from you on 3rd patch? Yes, feel free to add: Acked-by: Greg Kroah-Hartman -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] s3c-hsudc: add a remove function
On Sun, Dec 18, 2011 at 09:37:04PM +, Russell King - ARM Linux wrote:
> On Sun, Dec 18, 2011 at 09:46:08PM +0100, Heiko Stübner wrote:
> > > > kobject: 'holders' (c7addc80): kobject_cleanup
> > > > Unable to handle kernel paging request at virtual address bf055504
> > > > pgd = c0004000
> > > > [bf055504] *pgd=371f9811, *pte=, *ppte=
> > > > Internal error: Oops: 7 [#1]
> > >
> > > Please post the entire first oops dump for the above run - it may contain
> > > useful information to properly track this down.
> >
> > kobject: 'holders' (c7addc80): kobject_cleanup
> > Unable to handle kernel paging request at virtual address bf055504
> > pgd = c0004000
> > [bf055504] *pgd=371f9811, *pte=, *ppte=
> > Internal error: Oops: 7 [#1]
> > Modules linked in: ohci_hcd usbcore leds_s3c24xx i2c_s3c2410 i2c_core
> > CPU: 0Not tainted (3.2.0-rc5-next-20111216+ #33)
> > PC is at kobject_put+0x18/0x7c
> > LR is at kobject_del+0x64/0x70
> > pc : []lr : []psr: a013
> > sp : c70bdef8 ip : c70bdf18 fp : c70bdf14
> > r10: r9 : c0114718 r8 : c7803a00
> > r7 : c7abd360 r6 : c02e1de0 r5 : c7addca0 r4 : bf0554a0
> > r3 : 0001 r2 : r1 : r0 : bf0554a0
> > Backtrace:
> > [] (kobject_put+0x0/0x7c) from []
> > (kobject_del+0x64/0x70)
> > r4:c7addc80
> > [] (kobject_del+0x0/0x70) from []
> > (kobject_delayed_cleanup+0xd4/0x174)
> > r4:c7addc80
> > [] (kobject_delayed_cleanup+0x0/0x174) from []
> > (process_one_work+0x24c/0x3a8)
>
> Right, here's what I think is happening.
>
> You're right that 0xc7addc80 is being cleaned up. So, we enter
> kobject_cleanup() with kobj = 0xc7addc80. We get to this:
>
> /* remove from sysfs if the caller did not do it */
> if (kobj->state_in_sysfs) {
> pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
> kobject_name(kobj), kobj);
> kobject_del(kobj);
> }
>
> So, we call kobject_del() on c7addc80 (which we can see in r4 in the
> backtrace):
>
> void kobject_del(struct kobject *kobj)
> {
> if (!kobj)
> return;
>
> sysfs_remove_dir(kobj);
> kobj->state_in_sysfs = 0;
> kobj_kset_leave(kobj);
> kobject_put(kobj->parent);
>
> And so we get to kobject_put(), and we call that with a pointer of
> 0xbf0554a0. This is a pointer into struct module. And this is where
> the problem lies...
>
> The struct module is free'd as part of the core of the module
> (mod->module_core) here:
>
> static void module_deallocate(struct module *mod, struct load_info *info)
> {
> kfree(info->strmap);
> percpu_modfree(mod);
> module_free(mod, mod->module_init);
> module_free(mod, mod->module_core);
> }
>
> A struct module contains:
>
> struct module
> {
> ...
> /* Sysfs stuff. */
> struct module_kobject mkobj;
>
> which in turn is defined as:
>
> struct module_kobject {
> struct kobject kobj;
> ...
> }
>
> So, we have a struct kobject contained within a data structure which is
> independently allocated and freed - and this is highly illegal. I'm
> sure GregKH will want to discuss this with Rusty...
Ugh, that sucks, yes I'll work on this when I get back from vacation the
first week in January.
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] s3c-hsudc: add a remove function
On Sun, Dec 18, 2011 at 07:50:37PM +0100, Heiko Stübner wrote: > Am Sonntag 18 Dezember 2011, 15:43:19 schrieb Russell King - ARM Linux: > > On Sun, Dec 18, 2011 at 02:44:39PM +0100, Heiko Stübner wrote: > > > Am Sonntag 18 Dezember 2011, 09:10:48 schrieb Russell King - ARM Linux: > > > > On Sat, Dec 17, 2011 at 08:26:33PM +0100, Heiko Stübner wrote: > > > > > As the driver is also buildable as a module it should need > > > > > a cleanup function for the removal of the module. > > > > > > > > My guess is that this wasn't implemented because of the embedded struct > > > > device lifetime rules for the gadget - to prevent the unbinding of the > > > > driver. > > > > > > > > Until the struct device lifetime gets fixed, you must not allow the > > > > module nor the data structure containing the struct device to be > > > > freed. > > > > > > I understand where this problem comes from (the release method is > > > potentially gone with the module before it is called) but after more > > > reading, I have a hard time believing that a lot of the other gadget > > > drivers would be wrong as well. Some of them since 2009 or possibly > > > earlier. > > > > > > Gadgets with embedded release methods: langwell_udc, goku_udc, fsl_qe_udc > > > (and fsl_udc_core), amd5536udc, net2280, pch_udc, cil13xxx_udc, > > > dummy_hcd, omap_udc, net2272, mc_udc_core > > > > > > Gadgets which use the release method from its pdev->dev but also free the > > > struct with the gadget in their remove method: r8a66597-udc, m66592-udc, > > > fusb300_udc. (possibly before the release function is called) > > > > > > On the other hand, the gets and puts of the udc->gadget.dev should be > > > paired correctly and it's only an intermediate device between the udc > > > and the gadget driver, so that the call to device_unregister in the > > > remove method should put the refcount to 0 and thus init the cleanup > > > (including the call to release) before the module is removed. > > > > > > So, I am very confused :-). > > > > Try this patch. If your system oopses 5 seconds after you remove the > > module, you have a lifetime bug. > I didn't get this far. With your patch the Oopses already happen during the > startup of > the system / the loading of the modules. > > A bit of the message spew I got during testing with linux-next-20111216: > > pgd = c0004000 > [bf05b504] *pgd=379af811, *pte=, *ppte= > Internal error: Oops: 7 [#1] > Modules linked in: ohci_hcd leds_s3c24xx usbcore i2c_s3c2410 i2c_core > s3c_hsudc > CPU: 0Not tainted (3.2.0-rc5-next-20111216+ #32) > PC is at kobject_put+0x18/0x7c You cut out the wording right before this. And because of that, I get to publicly mock you for going directly against how kobjects are supposed to work (see the documentation for why.) Go read the documentation, and don't try to "outsmart" the kernel, do you really think I put that warning in there just for the fun of it? greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH 0/3] Support Samsung S5P OHCI device and driver
On Thu, Dec 01, 2011 at 07:10:17PM +0900, Kukjin Kim wrote: > Greg KH wrote: > > > > On Mon, Nov 21, 2011 at 11:57:16AM +, Jingoo Han wrote: > > > > -Original Message- > > > > From: Greg KH [mailto:g...@kroah.com] > > > > Sent: Friday, November 18, 2011 4:12 AM > > > > To: Jingoo Han > > > > Cc: linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org; > > > > linux-samsung-soc@vger.kernel.org; gre...@suse.de; > > > > st...@rowland.harvard.edu; kgene@samsung.com; > > yulgon@samsung.com; > > > > jy0922.s...@samsung.com > > > > Subject: Re: [PATCH 0/3] Support Samsung S5P OHCI device and driver > > > > > > > > On Tue, Nov 15, 2011 at 06:49:58AM +, Jingoo Han wrote: > > > > > Hello. > > > > > > > > > > This patch series adds USB OHCI device and initial driver for > > Samsung > > > > > S5P SoCs and is based from Linux 3.2-rc1. I have tested on SMDKV310 > > > > board > > > > > using EXYNOS4. > > > > > > > > This is to be sent through some arm tree, not the usb tree, right? > > > Right, 1st and 2nd patch are sent throught arm tree. > > > However, 3rd patch is sent through the usb tree. > > > Thanks. > > > [PATCH 1/3] ARM: SAMSUNG: Add USB OHCI device > > > [PATCH 2/3] ARM: EXYNOS: Add USB OHCI support to SMDKV310 board > > > [PATCH 3/3] USB: Add S5P OHCI diver > > > Hi Greg, > > Sorry for late response. I came back today from family vacation... > > Anyway, > > > So the third patch does not depend on the first two? If it does, that > > means I will get build errors in the USB tree when I apply that patch, > > right? > > > > If so, then all three should go through a single tree. > > > Yes, right. This series would be handled in same tree so that we can avoid > useless conflicts. > > I commented small thing on 3rd patch. So I think, if you're ok on his > updated patch, it can be sent to upstream via Samsung tree. Or your tree > with my ack? The samsung tree is fine to take these, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH 0/3] Support Samsung S5P OHCI device and driver
On Mon, Nov 21, 2011 at 11:57:16AM +, Jingoo Han wrote: > > -Original Message- > > From: Greg KH [mailto:g...@kroah.com] > > Sent: Friday, November 18, 2011 4:12 AM > > To: Jingoo Han > > Cc: linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org; > > linux-samsung-soc@vger.kernel.org; gre...@suse.de; > > st...@rowland.harvard.edu; kgene@samsung.com; yulgon@samsung.com; > > jy0922.s...@samsung.com > > Subject: Re: [PATCH 0/3] Support Samsung S5P OHCI device and driver > > > > On Tue, Nov 15, 2011 at 06:49:58AM +, Jingoo Han wrote: > > > Hello. > > > > > > This patch series adds USB OHCI device and initial driver for Samsung > > > S5P SoCs and is based from Linux 3.2-rc1. I have tested on SMDKV310 > > board > > > using EXYNOS4. > > > > This is to be sent through some arm tree, not the usb tree, right? > Right, 1st and 2nd patch are sent throught arm tree. > However, 3rd patch is sent through the usb tree. > Thanks. > [PATCH 1/3] ARM: SAMSUNG: Add USB OHCI device > [PATCH 2/3] ARM: EXYNOS: Add USB OHCI support to SMDKV310 board > [PATCH 3/3] USB: Add S5P OHCI diver So the third patch does not depend on the first two? If it does, that means I will get build errors in the USB tree when I apply that patch, right? If so, then all three should go through a single tree. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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/3] Support Samsung S5P OHCI device and driver
On Tue, Nov 15, 2011 at 06:49:58AM +, Jingoo Han wrote: > Hello. > > This patch series adds USB OHCI device and initial driver for Samsung > S5P SoCs and is based from Linux 3.2-rc1. I have tested on SMDKV310 board > using EXYNOS4. This is to be sent through some arm tree, not the usb tree, right? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 0/9] serial: samsung: rework clock lookup and add device tree support
sions is the probe function along with SoC specific driver > >data. > >These are merged into the common Samsung uart driver and all the SoC specific > >extensions are deleted. > > > >Patch 9 adds device tree based discovery support for the uart driver. > > > > > >This patchset is based on the following tree > >https://github.com/kgene/linux-samsung.git branch: for-next > > > >with the following two patches applied > >[PATCH] serial: samsung: Add unified interrupt handler for s3c64xx and later > >SoC's > >[PATCH] ARM: SAMSUNG: Remove uart irq handling from plaform code > > > >and tested on the following boards. > >SMDK2440, SMDK2416, SMDK6410, SMDK6440, SMDK6450, SMDKC100, SMDKV210, > >SMDKV310. > > > >This patchset has dependency on the following patchset: > >[PATCH V2 0/2] Add a common macro for creating struct clk_lookup entries > > > > > >Thomas Abraham (9): > > serial: samsung: Keep a copy of the location of platform data in driver's > > private data > > serial: samsung: move handling of fclk/n clock to platform code > > serial: samsung: switch to clkdev based clock lookup > > serial: samsung: remove struct 's3c24xx_uart_clksrc' and all uses of it > > serial: samsung: remove all uses of get_clksrc and set_clksrc > > arm: samsung: register uart clocks to clock lookup list > > serial: samsung: merge all SoC specific port reset functions > > serial: samsung: merge probe() function from all SoC specific extensions > > serial: samsung: add device tree support > > > > .../devicetree/bindings/serial/samsung_uart.txt| 14 + > > arch/arm/mach-exynos4/clock.c | 106 ++-- > > arch/arm/mach-exynos4/init.c | 21 +- > > arch/arm/mach-s3c2410/mach-bast.c | 22 - > > arch/arm/mach-s3c2410/mach-vr1000.c| 24 - > > arch/arm/mach-s3c2410/s3c2410.c|6 + > > arch/arm/mach-s3c2412/clock.c |7 + > > arch/arm/mach-s3c2440/clock.c | 44 ++ > > arch/arm/mach-s3c2440/mach-anubis.c| 22 +- > > arch/arm/mach-s3c2440/mach-at2440evb.c | 22 +- > > arch/arm/mach-s3c2440/mach-osiris.c| 24 +- > > arch/arm/mach-s3c2440/mach-rx1950.c| 18 +- > > arch/arm/mach-s3c2440/mach-rx3715.c| 19 +- > > arch/arm/mach-s3c64xx/clock.c | 37 +- > > arch/arm/mach-s5p64x0/clock-s5p6440.c | 32 +- > > arch/arm/mach-s5p64x0/clock-s5p6450.c | 32 +- > > arch/arm/mach-s5p64x0/init.c | 31 - > > arch/arm/mach-s5pc100/clock.c | 33 +- > > arch/arm/mach-s5pv210/clock.c | 107 ++-- > > arch/arm/mach-s5pv210/init.c | 19 - > > arch/arm/plat-s3c24xx/s3c2443-clock.c | 23 +- > > arch/arm/plat-samsung/include/plat/regs-serial.h | 45 +- > > drivers/tty/serial/Kconfig | 45 +-- > > drivers/tty/serial/Makefile|5 - > > drivers/tty/serial/s3c2410.c | 115 > > drivers/tty/serial/s3c2412.c | 149 - > > drivers/tty/serial/s3c2440.c | 178 -- > > drivers/tty/serial/s3c6400.c | 149 - > > drivers/tty/serial/s5pv210.c | 158 - > > drivers/tty/serial/samsung.c | 639 > > > > drivers/tty/serial/samsung.h | 32 +- > > 31 files changed, 752 insertions(+), 1426 deletions(-) > > create mode 100644 > > Documentation/devicetree/bindings/serial/samsung_uart.txt > > delete mode 100644 drivers/tty/serial/s3c2410.c > > delete mode 100644 drivers/tty/serial/s3c2412.c > > delete mode 100644 drivers/tty/serial/s3c2440.c > > delete mode 100644 drivers/tty/serial/s3c6400.c > > delete mode 100644 drivers/tty/serial/s5pv210.c > > (Cc'ed Greg KH) > > Looks good for me, and I need to get the ack from Greg before applying. > > Greg, if you're ok, I'd like to send this series to upstream via > Samsung tree for supporting device tree because this touches a lot > of arch/arm/ Samsung stuff. If any problems, please let me know. No objection from me, feel free to take this through your tree. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Samsung Fixes for v3.1-rc7
On Fri, Sep 16, 2011 at 08:50:51PM +0900, Kukjin Kim wrote: > Arnd Bergmann wrote: > > > > On Thursday 15 September 2011, Kukjin Kim wrote: > > > This is Samsung fixes for v3.1 > > > > > > Please pull from: > > > git://github.com/kgene/linux-samsung.git samsung-fixes-2 > > > As you know, git.kernel.org/master.kernel.org has been down so I use > > > temporary git repo. at github now. > > > > > > These things are needed for v3.1 and if any problems, please let me > know. > > > > > > As a note, others for v3.2 will be sent in the next week... > > > > Thanks, pulled. > > > > Is it correct that you want none of these patches to be backported > > into the stable or longterm releases? Some of these look like they > > should be marked 'Cc: sta...@kernel.org'. > > > (Cc'ed Greg K-H) > > Yes, you're right. Some patches are needed to sent to sta...@kernel.org. > But unfortunately, when they have been submitted, there was no 'Cc: > sta...@kernel.org'... What do you mean? Do you mean you just forgot to add them, or you created them so long ago before there was a sta...@kernel.org? > In this case, I'm not sure which following method is proper... > - to send 'pull request' to Greg / sta...@kernel.org like bug fix during -rc > - to submit each patches with adding 'Cc: sta...@kernel.org' again > - or ? Are these in Linus's tree already? If so, send me the git commit ids and I will add them to the stable kernels. If not, wait until they are, and then send me the git commit ids, and I will then add them. Before they get to Linus, there's nothing I can do with them, and I don't accept pull requests as that makes no sense when it comes to the stable kernel patch flow. Does this help? greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: SAMSUNG: serial: Fix on handling of one clock source for UART
On Fri, May 27, 2011 at 07:04:03PM -0700, Kukjin Kim wrote: > From: Boojin Kim > > This patch fixes the way of comparison for handling of two or more > clock sources for UART. > > For example, if just only one clock source is defined even though > there are two clock sources for UART, the serial driver does not > set proper clock up. Of course, it is problem. > > So this patch changes the condition of comparison to avoid useless > setup clock and adds a flag 'NO_NEED_CHECK_CLKSRC' which means > selection of source clock is not required. > > In addition, since the Exynos4210 has only one clock source for UART > this patch adds the flag into its common_init_uarts(). > > Signed-off-by: Boojin Kim > Cc: Greg Kroah-Hartman > Signed-off-by: Kukjin Kim > --- > arch/arm/mach-exynos4/init.c |1 + > arch/arm/plat-samsung/include/plat/regs-serial.h |2 ++ > drivers/tty/serial/s5pv210.c |4 ++-- > 3 files changed, 5 insertions(+), 2 deletions(-) Is this needed in older kernels as well (like .39)? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/4] USB: Gadget: Add Samsung S3C24XX USB High-Speed controller driver
On Sat, Apr 16, 2011 at 12:00:31PM +0200, Heiko Stübner wrote:
> Am Donnerstag 14 April 2011, 19:15:23 schrieb Alan Stern:
> > On Thu, 14 Apr 2011, Greg KH wrote:
> > > On Thu, Apr 14, 2011 at 11:35:43AM -0400, Alan Stern wrote:
> > > > On Thu, 14 Apr 2011, Heiko [iso-8859-1] St?bner wrote:
> > > > > From: Thomas Abraham
> > > > >
> > > > > The Samsung's S3C2416, S3C2443 and S3C2450 includes a USB High-Speed
> > > > > device controller module. This driver enables support for USB
> > > > > high-speed gadget functionality for the Samsung S3C24xx SoC's that
> > > > > include this controller.
> > > > >
> > > > > Signed-off-by: Thomas Abraham
> > > > > Signed-off-by: Sangbeom Kim
> > > > > Signed-off-by: Kukjin Kim
> > > > > Signed-off-by: Alexander Neumann
> > > > > Signed-off-by: Heiko Stuebner
> > > >
> > > > ...
> > > >
> > > > > +static struct usb_ep_ops s3c_hsudc_ep_ops = {
> > > > > + .enable = s3c_hsudc_ep_enable,
> > > > > + .disable = s3c_hsudc_ep_disable,
> > > > > + .alloc_request = s3c_hsudc_alloc_request,
> > > > > + .free_request = s3c_hsudc_free_request,
> > > > > + .queue = s3c_hsudc_queue,
> > > > > + .dequeue = s3c_hsudc_dequeue,
> > > > > + .set_halt = s3c_hsudc_set_halt,
> > > > > +};
> > > >
> > > > There's no .set_wedge method. Why do people always leave this out?
> > >
> > > Does the code spit out a nasty warning if this isn't set? If not, I
> > > would suggest adding it so that this doesn't keep happening.
> > >
> > > Or just refuse to be able to register the structure, that would stop it
> > > right away :)
> >
> > In fact, set_wedge is optional. But it's so easy to implement, there's
> > no good reason for leaving it out.
>
> It seems Thomas [original author of the driver] will be able to implement
> said
> set_wedge function for it.
> As he will need a bit of time for this, two possible ways for going forward
> come to mind:
> (1) use current driver [as set_wedge is optional] and add it later via patch
> (2) resubmit whole driver again when set_wedge is added to it
>
> Obviously I would prefer option 1 :-), but in the end it's your decision.
It shouldn't take that much time to do this, what is the delay?
I'd prefer to get the correct version implemented and would not like to
accept a patch that everyone knows is wrong.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/4] USB: Gadget: Add Samsung S3C24XX USB High-Speed controller driver
On Thu, Apr 14, 2011 at 11:35:43AM -0400, Alan Stern wrote:
> On Thu, 14 Apr 2011, Heiko [iso-8859-1] St?bner wrote:
>
> > From: Thomas Abraham
> >
> > The Samsung's S3C2416, S3C2443 and S3C2450 includes a USB High-Speed
> > device controller module. This driver enables support for USB high-speed
> > gadget functionality for the Samsung S3C24xx SoC's that include this
> > controller.
> >
> > Signed-off-by: Thomas Abraham
> > Signed-off-by: Sangbeom Kim
> > Signed-off-by: Kukjin Kim
> > Signed-off-by: Alexander Neumann
> > Signed-off-by: Heiko Stuebner
>
> ...
>
> > +static struct usb_ep_ops s3c_hsudc_ep_ops = {
> > + .enable = s3c_hsudc_ep_enable,
> > + .disable = s3c_hsudc_ep_disable,
> > + .alloc_request = s3c_hsudc_alloc_request,
> > + .free_request = s3c_hsudc_free_request,
> > + .queue = s3c_hsudc_queue,
> > + .dequeue = s3c_hsudc_dequeue,
> > + .set_halt = s3c_hsudc_set_halt,
> > +};
>
> There's no .set_wedge method. Why do people always leave this out?
Does the code spit out a nasty warning if this isn't set? If not, I
would suggest adding it so that this doesn't keep happening.
Or just refuse to be able to register the structure, that would stop it
right away :)
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ARM: EXYNOS4: Add usb ehci device to the NURI board
On Fri, Apr 08, 2011 at 01:22:11PM +0900, Joonyoung Shim wrote: > This patch is to support usb ehci device to the NURI board. > > Signed-off-by: Joonyoung Shim > Signed-off-by: Kyungmin Park Note, I had to apply this one by hand, I don't know what tree you made it against. Hopefully the merge issues will be simple to handle, as they were pretty obvious to me. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND v3 3/4] USB: Gadget: Add Samsung S3C24XX USB High-Speed controller driver
On Wed, Mar 23, 2011 at 10:39:28PM +0100, Heiko Stübner wrote: > From: Thomas Abraham > > The Samsung's S3C2416, S3C2443 and S3C2450 includes a USB High-Speed > device controller module. This driver enables support for USB high-speed > gadget functionality for the Samsung S3C24xx SoC's that include this > controller. > > Signed-off-by: Thomas Abraham > Signed-off-by: Sangbeom Kim > Signed-off-by: Kukjin Kim > --- > Changes since v1: > - Addressed comments from Ben Dooks > > Changes since v2: > - Update the driver to match the data structure changes introduced by > commit b0fca50f5a94a268ed02cfddf8051ed9343f. > Alexander Neumann > - Move s3c_hsudc to 0x29 in usb_gadget_controller_number in gadget_chips.h > Heiko Stuebner The Renesas driver got number 0x29, so this patch no longer applies :( Care to respin this whole series against the next linux-next tree and resend it to me so I can apply it? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 0/4] USB: gadget: Add Samsung S3C24xx USB HS controller driver
On Fri, Apr 08, 2011 at 12:04:41PM +0200, Heiko Stübner wrote: > > Am Mittwoch 23 März 2011 schrieb Heiko Stübner: > > On 2010-10-13 Sangbeom Kim sent 4 patches to add support the USB High-Speed > > device controller module on Samsung's S3C2416, S3C2443 and S3C2450. [1] > > > > bdooks worte on 2010-10-22: > > > patch 3/4 needs a bit of work, otherwise looks ok. > > > I'll look at applying the s3c24xx items in the next day or so. > > > > On 2010-10-23 Kukjin Kim send a revised version of patch 3. > > It seems the patches were forgotten after this. > > > > We resurrected them while working on a s3c2416 based device. Patch 3 needed > > an update to work with current kernels. The other 3 patches were not > > modified. > > > > Tests, comments, etc very welcome. > anyone? > > If someone could tell me if I'm doing something wrong I would be really > grateful. Sorry, been on vacation and at conferences, give me some time next week to catch up on usb email and I will get back to this... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB: Gadget: COMPOSITE: Debug interface number
On Fri, Feb 04, 2011 at 06:50:27PM +0900, Jassi Brar wrote: > Hi, > > The commit 'avoid access beyond array max length' incorrectly > compares w_index with MAX_CONFIG_INTERFACES. Whereas only > lower 8bits of w_index refer to the interface number. > So, use 'intf' rather than 'w_index' for comparison. Why did you attach the patch in base64 mode? Please fix your email client to not do this and send it to me in a format that I can apply it in. Try using 'git send-email' instead. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: s3c-hsotg patches
On Thu, Jul 15, 2010 at 10:02:22AM +0100, Ben Dooks wrote: > On Wed, Jul 14, 2010 at 08:10:26AM -0700, Greg KH wrote: > > On Wed, Jul 14, 2010 at 11:39:50AM +0200, Marek Szyprowski wrote: > > > Hello, > > > > > > On Wednesday, July 07, 2010 5:13 PM Greg KH wrote: > > > > > > > On Wed, Jul 07, 2010 at 01:02:12AM +0100, Ben Dooks wrote: > > > > > Re-send of previous set, with corrected patches. > > > > > > > > Hm, what tree are these going through? Do you need me to take them > > > > through the linux-usb tree? Or are they going through a > > > > platform-specific one somewhere? > > > > > > > > confused, > > > > > > Looks that Ben is busy and has no time to answer... > > > > Then we all should be too busy to apply them :) > > I've been really ill for the last couple of weeks, so still sifting > through a pile of stuff that needs to be sorted. Completly understandable, the comment "no time to answer" I took to be a choice on your part, not external circumstances, my appologies. Hope you are better now. > They where sent to you as I much prefer the driver specific stuff to go > to the relevant maintainer and if possible have them merged via the > maintainer tree. Great. > > Seriously, if a simple question like this is ignored, what happens if a > > real problem is reported? > > > > > I'm interested in merging these patches too. These patches were > > > intended to be merged to 2.6.35-rc5. They are available on > > > git://git.fluff.org/bjdooks/linux for-2635-rc5/hsotg branch. > > > > > > IMHO these patches should go though the linux-usb tree - they > > > don't touch any platform specific code, and register definitions > > > (arch/arm/plat-samsung/include/plat/regs-usb-hsotg.h) in fact belongs > > > to the driver rather than the platform. s3c-hsotg driver belongs to > > > the linux-usb tree. > > > > Ok, I can take them all, care to resend them through email? > > Do you still want them re-sending. I'd have to check that the branch > on git://git.fluff.org/bjdooks/linux for-2635-rc5/hsotg is up to date > before having it pulled. I can't "pull" trees, sorry, I need email patches. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: s3c-hsotg patches
On Wed, Jul 14, 2010 at 11:39:50AM +0200, Marek Szyprowski wrote: > Hello, > > On Wednesday, July 07, 2010 5:13 PM Greg KH wrote: > > > On Wed, Jul 07, 2010 at 01:02:12AM +0100, Ben Dooks wrote: > > > Re-send of previous set, with corrected patches. > > > > Hm, what tree are these going through? Do you need me to take them > > through the linux-usb tree? Or are they going through a > > platform-specific one somewhere? > > > > confused, > > Looks that Ben is busy and has no time to answer... Then we all should be too busy to apply them :) Seriously, if a simple question like this is ignored, what happens if a real problem is reported? > I'm interested in merging these patches too. These patches were > intended to be merged to 2.6.35-rc5. They are available on > git://git.fluff.org/bjdooks/linux for-2635-rc5/hsotg branch. > > IMHO these patches should go though the linux-usb tree - they > don't touch any platform specific code, and register definitions > (arch/arm/plat-samsung/include/plat/regs-usb-hsotg.h) in fact belongs > to the driver rather than the platform. s3c-hsotg driver belongs to > the linux-usb tree. Ok, I can take them all, care to resend them through email? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: s3c-hsotg patches
On Wed, Jul 07, 2010 at 01:02:12AM +0100, Ben Dooks wrote: > Re-send of previous set, with corrected patches. Hm, what tree are these going through? Do you need me to take them through the linux-usb tree? Or are they going through a platform-specific one somewhere? confused, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] USB: s3c_hsotg: Add support for external USB clock
On Tue, May 25, 2010 at 05:36:48AM +0100, Ben Dooks wrote: > From: Maurus Cuelenaere > > The PLL that drives the USB clock supports 3 input clocks: 12, 24 and 48Mhz. > This patch adds support to the USB driver for setting the correct register bit > according to the given clock. > > This depends on the following patch: > [PATCH] ARM: S3C64XX: Add USB external clock definition Care to take this through the arm tree then? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
