Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings

[Jonathan Cameron <jic23@kernel.org>]

> > > > > >         if (device_property_present(dev, "ti,resistor-value-ohm"))
> > > > > >                 ret = device_property_read_u32();
> > > > > >                 if (ret) //corrupt property in some fashion
> > > > > >                         return ret;
> > > > > >         } else {
> > > > > >                 //set default
> > > > > >         }
> > > > > > If there is no default then check it unconditionally.  
> > > > >
> > > > > default value is LM3533_ALS_RESISTOR_MIN and if no property is present
> > > > > clamp will ensure that als->r_select will be set to
> > > > > LM3533_ALS_RESISTOR_MIN  
> > > >
> > > > I don't see that default in the binding doc and relying in the 0 being clamped
> > > > isn't particularly readable - I'd set it explicitly.
> > > >  
> > >
> > > Oh, ye, my bad. Schema enforces one of props to be present and if pwn
> > > is present then resistor is ignored. What if I move resistor reading,
> > > clamping and conversion under !als->pwm_mode check? Then resistor must
> > > be present and hence must be checked unconditionally.  
> >
> > Sounds good.
> >  
> > >
> > > Additionally, I can comment original lm3533_als_setup with #if 0
> > > #endif then git formatting will be much cleaner and easier to review,
> > > and once we all come to result I will just remove entire commented
> > > block and Lee can pick clean commits.  
> >
> > No don't do that.  If you flatten the two helpers as a precursor patch
> > then the changes in here will be easier to review anyway.
> >  
> 
> By "flatten the two helpers" you mean incorporate
> lm3533_als_set_input_mode and lm3533_als_set_resistor into
> lm3533_als_setup first and then convert it to use DT? I am asking,
> just to be sure.
> 

yes

> > > > > > > @@ -852,25 +825,28 @@ static int lm3533_als_probe(struct platform_device *pdev)
> > > > > > >       indio_dev->channels = lm3533_als_channels;
> > > > > > >       indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
> > > > > > >       indio_dev->name = dev_name(&pdev->dev);
> > > > > > > -     iio_device_set_parent(indio_dev, pdev->dev.parent);  
> > > > > >
> > > > > > I'm not sure why this was there in the first place.  Hence not sure if it
> > > > > > is safe to remove.
> > > > > >  
> > > > >
> > > > > This is directly related to OF conversion. The iio_device_set_parent
> > > > > bound indio_dev to parent, and it causes problems with OF now since
> > > > > als output has its own node and binding it to parent if wrong. Same
> > > > > story for backlight and leds btw.  
> > > >
> > > > Is there any risk anyone was using the canonical path to get to the iio dev?
> > > > /sys/bus/platform/devices/..../iio\:deviceX
> > > > This is technically an ABI change be it a subtle one.
> > > >  
> > >
> > > Linux kernel has no users of this driver, and it is in "stale" state
> > > for more then 2 years (maybe even longer). I have cc'd Johan Hovold.
> > >
> > > https://lore.kernel.org/lkml/ZmBcvtLCzllQDWVX@hovoldconsulting.com/
> > >
> > > This this 2 y. o. discussion and there were no actions ore movements.
> > > I assume this driver in its current form has no more users. This does
> > > not mean that it cannot be revived though.  
> >
> > So, just to check, are you a user of this code or is this more trying to
> > help out with old code?
> >  
> 
> I am not insane enough to get myself into all this conversion if I did
> not need it. This is one of 2 remaining gaps in support of LG
> P880/P895 I own and support. I would really like to finally mainline
> all the patches I have locally since maintaining them becomes quite
> troublesome with time and additional patches layering on top.

Excellent! There are some odd people out there who do start on this
sort of thing despite no personal use case :)

Jonathan