diff mbox

[RFC,1/4] libnvdimm: add to_{nvdimm,nd_region}_dev()

Message ID 20170627102851.15484-1-oohall@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Oliver O'Halloran June 27, 2017, 10:28 a.m. UTC
struct device contains the ->of_node pointer so that devices can be
assoicated with the device-tree node that created them on DT platforms.
libnvdimm hides the struct device for regions and nvdimm devices inside
of an opaque structure so this patch adds accessors for each to allow
the of_nvdimm driver to set the of_node pointer.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 drivers/nvdimm/dimm_devs.c   | 6 ++++++
 drivers/nvdimm/region_devs.c | 6 ++++++
 include/linux/libnvdimm.h    | 2 ++
 3 files changed, 14 insertions(+)

Comments

Dan Williams July 10, 2017, 11:53 p.m. UTC | #1
On Tue, Jun 27, 2017 at 3:28 AM, Oliver O'Halloran <oohall@gmail.com> wrote:
> struct device contains the ->of_node pointer so that devices can be
> assoicated with the device-tree node that created them on DT platforms.
> libnvdimm hides the struct device for regions and nvdimm devices inside
> of an opaque structure so this patch adds accessors for each to allow
> the of_nvdimm driver to set the of_node pointer.

I'd rather go the other way and pass in the of_node to the bus and
dimm registration routines. It's a generic property of the device so
we should handle it like other generic device properties that get set
at initialization time like 'attr_groups' in nvdimm_bus_descriptor, or
a new parameter to nvdimm_create().
Oliver O'Halloran July 11, 2017, 4:38 a.m. UTC | #2
On Tue, Jul 11, 2017 at 9:53 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Tue, Jun 27, 2017 at 3:28 AM, Oliver O'Halloran <oohall@gmail.com> wrote:
>> struct device contains the ->of_node pointer so that devices can be
>> assoicated with the device-tree node that created them on DT platforms.
>> libnvdimm hides the struct device for regions and nvdimm devices inside
>> of an opaque structure so this patch adds accessors for each to allow
>> the of_nvdimm driver to set the of_node pointer.
>
> I'd rather go the other way and pass in the of_node to the bus and
> dimm registration routines. It's a generic property of the device so
> we should handle it like other generic device properties that get set
> at initialization time like 'attr_groups' in nvdimm_bus_descriptor, or
> a new parameter to nvdimm_create().

Sure. I just figured it would be preferable to keep firmware specific
details inside the firmware driver rather than adding #ifdef CONFIG_OF
around the place. Do you have any objections to making nvdimm_create()
take a descriptor structure rather than adding a parameter?
Dan Williams July 11, 2017, 7:36 a.m. UTC | #3
On Mon, Jul 10, 2017 at 9:38 PM, Oliver <oohall@gmail.com> wrote:
> On Tue, Jul 11, 2017 at 9:53 AM, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Tue, Jun 27, 2017 at 3:28 AM, Oliver O'Halloran <oohall@gmail.com> wrote:
>>> struct device contains the ->of_node pointer so that devices can be
>>> assoicated with the device-tree node that created them on DT platforms.
>>> libnvdimm hides the struct device for regions and nvdimm devices inside
>>> of an opaque structure so this patch adds accessors for each to allow
>>> the of_nvdimm driver to set the of_node pointer.
>>
>> I'd rather go the other way and pass in the of_node to the bus and
>> dimm registration routines. It's a generic property of the device so
>> we should handle it like other generic device properties that get set
>> at initialization time like 'attr_groups' in nvdimm_bus_descriptor, or
>> a new parameter to nvdimm_create().
>
> Sure. I just figured it would be preferable to keep firmware specific
> details inside the firmware driver rather than adding #ifdef CONFIG_OF
> around the place. Do you have any objections to making nvdimm_create()
> take a descriptor structure rather than adding a parameter?

I don't see why we need "#ifdef CONFIG_OF". It's just a "struct
of_node *" pointer that can be forward declared as "struct of_node;"
we don't need the full definition.

Yes, I'm fine with converting nvdimm_create() to a take a descriptor,
diff mbox

Patch

diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index f0d1b7e5de01..cbddac011181 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -227,6 +227,12 @@  struct nvdimm *to_nvdimm(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(to_nvdimm);
 
+struct device *to_nvdimm_dev(struct nvdimm *nvdimm)
+{
+	return &nvdimm->dev;
+}
+EXPORT_SYMBOL_GPL(to_nvdimm_dev);
+
 struct nvdimm *nd_blk_region_to_dimm(struct nd_blk_region *ndbr)
 {
 	struct nd_region *nd_region = &ndbr->nd_region;
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index cbaab4210c39..6c3988135fd5 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -182,6 +182,12 @@  struct nd_region *to_nd_region(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(to_nd_region);
 
+struct device *to_nd_region_dev(struct nd_region *region)
+{
+	return &region->dev;
+}
+EXPORT_SYMBOL_GPL(to_nd_region_dev);
+
 struct nd_blk_region *to_nd_blk_region(struct device *dev)
 {
 	struct nd_region *nd_region = to_nd_region(dev);
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 550761477005..10fbc523ff95 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -139,6 +139,8 @@  struct nd_region *to_nd_region(struct device *dev);
 struct nd_blk_region *to_nd_blk_region(struct device *dev);
 struct nvdimm_bus_descriptor *to_nd_desc(struct nvdimm_bus *nvdimm_bus);
 struct device *to_nvdimm_bus_dev(struct nvdimm_bus *nvdimm_bus);
+struct device *to_nvdimm_dev(struct nvdimm *nvdimm);
+struct device *to_nd_region_dev(struct nd_region *region);
 const char *nvdimm_name(struct nvdimm *nvdimm);
 struct kobject *nvdimm_kobj(struct nvdimm *nvdimm);
 unsigned long nvdimm_cmd_mask(struct nvdimm *nvdimm);