On Fri, 28 Feb 2025 11:30:51 +0200
Svyatoslav Ryhel <clamor95@xxxxxxxxx> wrote:
> пт, 28 лют. 2025 р. о 10:59 Lee Jones <lee@xxxxxxxxxx> пише:
> >
> > On Mon, 24 Feb 2025, Svyatoslav Ryhel wrote:
> >
> > > Remove platform data and fully relay on OF and device tree
> > > parsing and binding devices.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > > ---
> > > drivers/iio/light/lm3533-als.c | 40 ++++---
> > > drivers/leds/leds-lm3533.c | 46 +++++---
> > > drivers/mfd/lm3533-core.c | 159 ++++++++--------------------
> > > drivers/video/backlight/lm3533_bl.c | 71 ++++++++++---
> > > include/linux/mfd/lm3533.h | 35 +-----
> > > 5 files changed, 164 insertions(+), 187 deletions(-)
> > >
> > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > > index 99f0b903018c..cb52965e93c6 100644
> > > --- a/drivers/iio/light/lm3533-als.c
> > > +++ b/drivers/iio/light/lm3533-als.c
> > > @@ -16,9 +16,12 @@
> > > #include <linux/module.h>
> > > #include <linux/mutex.h>
> > > #include <linux/mfd/core.h>
> > > +#include <linux/mod_devicetable.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/property.h>
> > > #include <linux/slab.h>
> > > #include <linux/uaccess.h>
> > > +#include <linux/units.h>
> > >
> > > #include <linux/mfd/lm3533.h>
> > >
> > > @@ -56,6 +59,9 @@ struct lm3533_als {
> > >
> > > atomic_t zone;
> > > struct mutex thresh_mutex;
> > > +
> > > + unsigned pwm_mode:1; /* PWM input mode (default analog) */
> > > + u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
> > > };
> > >
> > >
> > > @@ -753,18 +759,17 @@ static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
> > > return 0;
> > > }
> > >
> > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > - const struct lm3533_als_platform_data *pdata)
> > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > {
> > > int ret;
> > >
> > > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > > + ret = lm3533_als_set_input_mode(als, als->pwm_mode);
> > > if (ret)
> > > return ret;
> > >
> > > /* ALS input is always high impedance in PWM-mode. */
> > > - if (!pdata->pwm_mode) {
> > > - ret = lm3533_als_set_resistor(als, pdata->r_select);
> > > + if (!als->pwm_mode) {
> > > + ret = lm3533_als_set_resistor(als, als->r_select);
> >
> > You're already passing 'als'.
> >
> > Just teach lm3533_als_set_resistor that 'r_select' is now contained.
> >
>
> This is not scope of this patchset. I was already accused in too much
> changes which make it unreadable. This patchset is dedicated to
> swapping platform data to use of the device tree. NOT improving
> functions, NOT rewriting arbitrary mechanics. If you feed a need for
> this change, then propose a followup. I need from this driver only one
> thing, that it could work with device tree. But it seems that it is
> better that it just rots in the garbage bin until removed cause no one
> cared.
This is not an unreasonable request as you added r_select to als.
Perhaps it belongs in a separate follow up patch. However
it is worth remembering the motivation here is that you want get
this code upstream, the maintainers don't have that motivation.
Greg KH has given various talks on the different motivations in the
past. It maybe worth a watch.
>
> > > if (ret)
> > > return ret;
> > > }
> > > @@ -828,22 +833,16 @@ static const struct iio_info lm3533_als_info = {
> > >
> > > static int lm3533_als_probe(struct platform_device *pdev)
> > > {
> > > - const struct lm3533_als_platform_data *pdata;
> > > struct lm3533 *lm3533;
> > > struct lm3533_als *als;
> > > struct iio_dev *indio_dev;
> > > + u32 val;
> >
> > Value of what, potatoes?
> >
>
> Oranges.
A well named variable would avoid need for any discussion of
what it is the value of.
>
> > > int ret;
> > >
> > > lm3533 = dev_get_drvdata(pdev->dev.parent);
> > > if (!lm3533)
> > > return -EINVAL;
> > >
> > > - pdata = dev_get_platdata(&pdev->dev);
> > > - if (!pdata) {
> > > - dev_err(&pdev->dev, "no platform data\n");
> > > - return -EINVAL;
> > > - }
> > > -
> > > indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als));
> > > if (!indio_dev)
> > > return -ENOMEM;
> > > @@ -864,13 +863,21 @@ static int lm3533_als_probe(struct platform_device *pdev)
> > >
> > > platform_set_drvdata(pdev, indio_dev);
> > >
> > > + val = 200 * KILO; /* 200kOhm */
> >
> > Better to #define magic numbers; DEFAULT_{DESCRIPTION}_OHMS
> >
>
> Why? that is not needed.
If this variable had a more useful name there would be no need for
the comment either.
val_resitor_ohms = 200 * KILLO;
or similar.
>
> > > + device_property_read_u32(&pdev->dev, "ti,resistor-value-ohm", &val);
> > > +
> > > + /* Convert resitance into R_ALS value with 2v / 10uA * R */
> >
> > Because ...
> >
>
> BACAUSE the device DOES NOT understand human readable values, only 0s
> and 1s, hence mOhms must be converted into value lm3533 chip can
> understand.
A comment that gave the motivation would be much more useful than
repeating the maths.
/* Convert resistance to equivalent register value */
>
> > > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * val);
> > > +
> > > + als->pwm_mode = device_property_read_bool(&pdev->dev, "ti,pwm-mode");
> > > +
> > > if (als->irq) {
> > > ret = lm3533_als_setup_irq(als, indio_dev);
> > > if (ret)
> > > return ret;
> > > }
> > >
> > > - ret = lm3533_als_setup(als, pdata);
> > > + ret = lm3533_als_setup(als);
> > > if (ret)
> > > goto err_free_irq;
> > >
> > > @@ -907,9 +914,16 @@ static void lm3533_als_remove(struct platform_device *pdev)
> > > free_irq(als->irq, indio_dev);
> > > }
> > >
> > > +static const struct of_device_id lm3533_als_match_table[] = {
> > > + { .compatible = "ti,lm3533-als" },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, lm3533_als_match_table);
> > > +
> > > static struct platform_driver lm3533_als_driver = {
> > > .driver = {
> > > .name = "lm3533-als",
> > > + .of_match_table = lm3533_als_match_table,
> > > },
> > > .probe = lm3533_als_probe,
> > > .remove = lm3533_als_remove,
Anyhow, I'm short on time so only looking at the IIO related part.
Jonathan
