Re: [PATCH v4 1/7] mfd: add syscon driver based on regmap
On Mon, Sep 03, 2012 at 11:09:01AM +0800, Shawn Guo wrote: > On Mon, Sep 03, 2012 at 10:31:03AM +0800, Dong Aisheng wrote: > > > > > > I think of_node_put should be moved out from here and put into > > > syscon_node_to_regmap and syscon_regmap_lookup_by_compatible. > > > > > I guess no, if you want to move of_node_put into > > syscon_regmap_lookup_by_phandle, > > then syscon_regmap_lookup_by_phandle has the same issue. > > I guess not. syscon_regmap_lookup_by_phandle itself calls of_parse_phandle, > and that's where the refcount gets incremented, so > syscon_regmap_lookup_by_phandle > should be responsible for calling of_node_put to get the refcount decremented. > Yes, the of_node_put will be done in syscon_node_to_regmap which is called by syscon_regmap_lookup_by_phandle, The reason why we do it is as i said in my last reply. I think one known issue is that syscon_node_to_regmap may not be suitable to be used by the driver who still wants to use the regmap node after calling the syscon_node_to_regmap. I still do not find such using case but i'm not sure whether it may exist. Probably the safe way currently to do is just as you said: Not put the node in syscon_node_to_regmap and let user decide. Ok, i will update it. Regards Dong Aisheng -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/7] mfd: add syscon driver based on regmap
On Mon, Sep 03, 2012 at 11:09:01AM +0800, Shawn Guo wrote: On Mon, Sep 03, 2012 at 10:31:03AM +0800, Dong Aisheng wrote: I think of_node_put should be moved out from here and put into syscon_node_to_regmap and syscon_regmap_lookup_by_compatible. I guess no, if you want to move of_node_put into syscon_regmap_lookup_by_phandle, then syscon_regmap_lookup_by_phandle has the same issue. I guess not. syscon_regmap_lookup_by_phandle itself calls of_parse_phandle, and that's where the refcount gets incremented, so syscon_regmap_lookup_by_phandle should be responsible for calling of_node_put to get the refcount decremented. Yes, the of_node_put will be done in syscon_node_to_regmap which is called by syscon_regmap_lookup_by_phandle, The reason why we do it is as i said in my last reply. I think one known issue is that syscon_node_to_regmap may not be suitable to be used by the driver who still wants to use the regmap node after calling the syscon_node_to_regmap. I still do not find such using case but i'm not sure whether it may exist. Probably the safe way currently to do is just as you said: Not put the node in syscon_node_to_regmap and let user decide. Ok, i will update it. Regards Dong Aisheng -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/7] mfd: add syscon driver based on regmap
On Mon, Sep 03, 2012 at 10:31:03AM +0800, Dong Aisheng wrote: > > > > I think of_node_put should be moved out from here and put into > > syscon_node_to_regmap and syscon_regmap_lookup_by_compatible. > > > I guess no, if you want to move of_node_put into > syscon_regmap_lookup_by_phandle, > then syscon_regmap_lookup_by_phandle has the same issue. I guess not. syscon_regmap_lookup_by_phandle itself calls of_parse_phandle, and that's where the refcount gets incremented, so syscon_regmap_lookup_by_phandle should be responsible for calling of_node_put to get the refcount decremented. -- Regards, Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/7] mfd: add syscon driver based on regmap
On Fri, Aug 31, 2012 at 09:26:29AM +0800, Shawn Guo wrote:
> On Wed, Aug 29, 2012 at 06:56:52PM +0800, Dong Aisheng wrote:
> > +config MFD_SYSCON
> > +bool "System Controller Register R/W Based on Regmap"
>
> If the driver only compiles and works for device tree probe, we should
> have the following?
>
> depends on OF
>
Correct.
For currently it only supports dt.
> > +select REGMAP_MMIO
> > +help
> > + Select this option to enable accessing system control registers
> > + via regmap.
> > +
>
>
>
> > +struct regmap *syscon_node_to_regmap(struct device_node *np)
> > +{
> > + struct syscon *syscon;
> > + struct device *dev;
> > +
> > + dev = driver_find_device(_driver.driver, NULL, np,
> > +syscon_match);
> > + of_node_put(np);
>
> This looks a little unnatural to me. Function syscon_node_to_regmap
> becomes an API visible to clients, who might never know that np will
> be put inside the API. I'm saying the client may also call of_node_put
> to put the node they get.
We probably could add a comment here for the API to avoid this happen.
>
> I think of_node_put should be moved out from here and put into
> syscon_node_to_regmap and syscon_regmap_lookup_by_compatible.
>
I guess no, if you want to move of_node_put into
syscon_regmap_lookup_by_phandle,
then syscon_regmap_lookup_by_phandle has the same issue.
Actually i had considered your concern when writing this API...
The original purpose of doing like that is saving some duplicated 'of_node_put'
and make the API easy to use.
I searched the kernel dt code and found it existed some similar cases.
e.g: of_irq_find_parent, of_get_next_parent
So it looks to me that it may be usually to do like that for the cases that
the conversion from a node to other thing since the client may only care
about the things converted. For our case, it's regmap.
So i can't think it make too much sense for all client driver have to write
duplicated and meaningless 'of_node_put' code.
Regards
Dong Aisheng
> > +
> > + if (!dev)
> > + return ERR_PTR(-EPROBE_DEFER);
> > +
> > + syscon = dev_get_drvdata(dev);
> > +
> > + return syscon->regmap;
> > +}
> > +EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
> > +
> > +struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
> > +{
> > + struct device_node *syscon_np;
> > +
> > + syscon_np = of_find_compatible_node(NULL, NULL, s);
> > + if (!syscon_np)
> > + return ERR_PTR(-ENODEV);
> > +
> > + return syscon_node_to_regmap(syscon_np);
> > +}
> > +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
> > +
> > +struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
> > + const char *property)
> > +{
> > + struct device_node *syscon_np;
> > +
> > + syscon_np = of_parse_phandle(np, property, 0);
> > + if (!syscon_np)
> > + return ERR_PTR(-ENODEV);
> > +
> > + return syscon_node_to_regmap(syscon_np);
> > +}
> > +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/7] mfd: add syscon driver based on regmap
On Fri, Aug 31, 2012 at 09:26:29AM +0800, Shawn Guo wrote:
On Wed, Aug 29, 2012 at 06:56:52PM +0800, Dong Aisheng wrote:
+config MFD_SYSCON
+bool System Controller Register R/W Based on Regmap
If the driver only compiles and works for device tree probe, we should
have the following?
depends on OF
Correct.
For currently it only supports dt.
+select REGMAP_MMIO
+help
+ Select this option to enable accessing system control registers
+ via regmap.
+
snip
+struct regmap *syscon_node_to_regmap(struct device_node *np)
+{
+ struct syscon *syscon;
+ struct device *dev;
+
+ dev = driver_find_device(syscon_driver.driver, NULL, np,
+syscon_match);
+ of_node_put(np);
This looks a little unnatural to me. Function syscon_node_to_regmap
becomes an API visible to clients, who might never know that np will
be put inside the API. I'm saying the client may also call of_node_put
to put the node they get.
We probably could add a comment here for the API to avoid this happen.
I think of_node_put should be moved out from here and put into
syscon_node_to_regmap and syscon_regmap_lookup_by_compatible.
I guess no, if you want to move of_node_put into
syscon_regmap_lookup_by_phandle,
then syscon_regmap_lookup_by_phandle has the same issue.
Actually i had considered your concern when writing this API...
The original purpose of doing like that is saving some duplicated 'of_node_put'
and make the API easy to use.
I searched the kernel dt code and found it existed some similar cases.
e.g: of_irq_find_parent, of_get_next_parent
So it looks to me that it may be usually to do like that for the cases that
the conversion from a node to other thing since the client may only care
about the things converted. For our case, it's regmap.
So i can't think it make too much sense for all client driver have to write
duplicated and meaningless 'of_node_put' code.
Regards
Dong Aisheng
+
+ if (!dev)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ syscon = dev_get_drvdata(dev);
+
+ return syscon-regmap;
+}
+EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
+
+struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
+{
+ struct device_node *syscon_np;
+
+ syscon_np = of_find_compatible_node(NULL, NULL, s);
+ if (!syscon_np)
+ return ERR_PTR(-ENODEV);
+
+ return syscon_node_to_regmap(syscon_np);
+}
+EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
+
+struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
+ const char *property)
+{
+ struct device_node *syscon_np;
+
+ syscon_np = of_parse_phandle(np, property, 0);
+ if (!syscon_np)
+ return ERR_PTR(-ENODEV);
+
+ return syscon_node_to_regmap(syscon_np);
+}
+EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/7] mfd: add syscon driver based on regmap
On Mon, Sep 03, 2012 at 10:31:03AM +0800, Dong Aisheng wrote: I think of_node_put should be moved out from here and put into syscon_node_to_regmap and syscon_regmap_lookup_by_compatible. I guess no, if you want to move of_node_put into syscon_regmap_lookup_by_phandle, then syscon_regmap_lookup_by_phandle has the same issue. I guess not. syscon_regmap_lookup_by_phandle itself calls of_parse_phandle, and that's where the refcount gets incremented, so syscon_regmap_lookup_by_phandle should be responsible for calling of_node_put to get the refcount decremented. -- Regards, Shawn -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/7] mfd: add syscon driver based on regmap
On Wed, Aug 29, 2012 at 06:56:52PM +0800, Dong Aisheng wrote:
> +config MFD_SYSCON
> +bool "System Controller Register R/W Based on Regmap"
If the driver only compiles and works for device tree probe, we should
have the following?
depends on OF
> +select REGMAP_MMIO
> +help
> + Select this option to enable accessing system control registers
> + via regmap.
> +
> +struct regmap *syscon_node_to_regmap(struct device_node *np)
> +{
> + struct syscon *syscon;
> + struct device *dev;
> +
> + dev = driver_find_device(_driver.driver, NULL, np,
> + syscon_match);
> + of_node_put(np);
This looks a little unnatural to me. Function syscon_node_to_regmap
becomes an API visible to clients, who might never know that np will
be put inside the API. I'm saying the client may also call of_node_put
to put the node they get.
I think of_node_put should be moved out from here and put into
syscon_node_to_regmap and syscon_regmap_lookup_by_compatible.
Regards,
Shawn
> +
> + if (!dev)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + syscon = dev_get_drvdata(dev);
> +
> + return syscon->regmap;
> +}
> +EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
> +
> +struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
> +{
> + struct device_node *syscon_np;
> +
> + syscon_np = of_find_compatible_node(NULL, NULL, s);
> + if (!syscon_np)
> + return ERR_PTR(-ENODEV);
> +
> + return syscon_node_to_regmap(syscon_np);
> +}
> +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
> +
> +struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
> + const char *property)
> +{
> + struct device_node *syscon_np;
> +
> + syscon_np = of_parse_phandle(np, property, 0);
> + if (!syscon_np)
> + return ERR_PTR(-ENODEV);
> +
> + return syscon_node_to_regmap(syscon_np);
> +}
> +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/7] mfd: add syscon driver based on regmap
On Wed, Aug 29, 2012 at 06:56:52PM +0800, Dong Aisheng wrote:
+config MFD_SYSCON
+bool System Controller Register R/W Based on Regmap
If the driver only compiles and works for device tree probe, we should
have the following?
depends on OF
+select REGMAP_MMIO
+help
+ Select this option to enable accessing system control registers
+ via regmap.
+
snip
+struct regmap *syscon_node_to_regmap(struct device_node *np)
+{
+ struct syscon *syscon;
+ struct device *dev;
+
+ dev = driver_find_device(syscon_driver.driver, NULL, np,
+ syscon_match);
+ of_node_put(np);
This looks a little unnatural to me. Function syscon_node_to_regmap
becomes an API visible to clients, who might never know that np will
be put inside the API. I'm saying the client may also call of_node_put
to put the node they get.
I think of_node_put should be moved out from here and put into
syscon_node_to_regmap and syscon_regmap_lookup_by_compatible.
Regards,
Shawn
+
+ if (!dev)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ syscon = dev_get_drvdata(dev);
+
+ return syscon-regmap;
+}
+EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
+
+struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
+{
+ struct device_node *syscon_np;
+
+ syscon_np = of_find_compatible_node(NULL, NULL, s);
+ if (!syscon_np)
+ return ERR_PTR(-ENODEV);
+
+ return syscon_node_to_regmap(syscon_np);
+}
+EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
+
+struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
+ const char *property)
+{
+ struct device_node *syscon_np;
+
+ syscon_np = of_parse_phandle(np, property, 0);
+ if (!syscon_np)
+ return ERR_PTR(-ENODEV);
+
+ return syscon_node_to_regmap(syscon_np);
+}
+EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
