> > > > > > 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/[email protected]/
> > >
> > > 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
