Re: [PATCH RESEND v2] mfd: sm501: Add device property

[Yoshinori Sato]

On Fri, 01 Jul 2016 17:59:11 +0900,
Lee Jones wrote:
> 
> On Fri, 01 Jul 2016, Yoshinori Sato wrote:
> 
> > On Thu, 30 Jun 2016 16:48:00 +0900,
> > Lee Jones wrote:
> > > 
> > > On Thu, 30 Jun 2016, Yoshinori Sato wrote:
> > > 
> > > > Signed-off-by: Yoshinori Sato <ysato@xxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >  Documentation/devicetree/bindings/mfd/sm501.txt | 45 +++++++++++++++++++++++++
> > > >  drivers/mfd/sm501.c                             |  9 +++++
> > > >  2 files changed, 54 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mfd/sm501.txt
> 
> [...]
> 
> > > > diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> > > > index 65cd0d2..e2e3f9b 100644
> > > > --- a/drivers/mfd/sm501.c
> > > > +++ b/drivers/mfd/sm501.c
> 
> [...]
> 
> > > > @@ -1377,6 +1378,8 @@ static int sm501_plat_probe(struct platform_device *dev)
> > > >  {
> > > >  	struct sm501_devdata *sm;
> > > >  	int ret;
> > > > +	struct sm501_platdata private_platdata;
> > > > +	struct sm501_initdata private_initdata;
> > > >  
> > > >  	sm = kzalloc(sizeof(struct sm501_devdata), GFP_KERNEL);
> > > >  	if (sm == NULL) {
> > > > @@ -1388,6 +1391,12 @@ static int sm501_plat_probe(struct platform_device *dev)
> > > >  	sm->dev = &dev->dev;
> > > >  	sm->pdev_id = dev->id;
> > > >  	sm->platdata = dev_get_platdata(&dev->dev);
> > > > +	if (!sm->platdata) {
> > > > +		of_property_read_u32(dev->dev.of_node, "smi,devices",
> > > > +				     (u32 *)&private_initdata.devices);
> > > > +		private_platdata.init = &private_initdata;
> > > > +		sm->platdata = &private_platdata;
> > > > +	}
> > > 
> > > I've asked about this 3 times now.
> > > 
> > > What consumes this platform data?
> > > 
> > > It also looks ugly and fragile.
> > 
> > It's appropriate to use dev.of_node, isn't it?
> > If it's misunderstood, I'm sorry.
> 
> Yes, that's acceptable.
> 
> I'm talking about the whole process of allocating an entire platform
> structure (on the stack, which will most likely be wiped when we
> return from probe()) just to populate this one, undocumented
> property.
> 
> Hold up ... I've just taken a look at the driver myself.  What a
> mess.  It appears this driver pre-dates the MFD API and does
> everything I hate.
> 
> First step is to see if the Device Tree guys like your new property.
> Please document it in bindings/display/sm501fb.txt, as suggested by
> Rob, so they can review it.  Don't forget to CC me too.
> 
> We can look at the C code changes later.

OK.
Thanks comment.

> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

-- 
Yoshinori Sato
<ysato@xxxxxxxxxxxxxxxxxxxx>