[PATCH v3 2/2] mfd: lm3533: convert to use OF

[Jonathan Cameron]

On Wed, 5 Mar 2025 16:18:38 +0200
Svyatoslav Ryhel <clamor95 at gmail.com> wrote:

> ср, 5 бер. 2025 р. о 15:45 Jonathan Cameron <jic23 at kernel.org> пише:
> >
> > On Fri, 28 Feb 2025 11:30:51 +0200
> > Svyatoslav Ryhel <clamor95 at gmail.com> wrote:
> >  
> > > пт, 28 лют. 2025 р. о 10:59 Lee Jones <lee at kernel.org> пише:  
> > > >
> > > > 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 at gmail.com>
> > > > > ---
> > > > >  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(-)
> > > > >  
> ...
> > > > >       /* 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.  
> 
> I have just moved values used in pdata to private structs of each
> driver. Without changing names or purpose.
> 
> > However
> > it is worth remembering the motivation here is that you want get
> > this code upstream, the maintainers don't have that motivation.  
> 
> This driver is already upstream and it is useless and incompatible
> with majority of supported devices. Maintainers should encourage those
> who try to help and instead we have what? A total discouragement. Well
> defined path into nowhere.

That is not how I read the situation. A simple request was made to
result in more maintainable code as a direct result of that
improvement being enabled by code changes you were making.
I'm sorry to hear that discouraged you.

> 
> >
> > 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.
> >  
> 
> This is temporary placeholder used to get values from the tree and
> then pass it driver struct.

Better if it is a temporary placeholder with a meaningful name.

> 
> > >  
> > > > >       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.
> >  
> 
> So I have to add a "reasonably" named variable for each property I
> want to get from device tree? Why? It seems to be a bit of overkill,
> no? Maybe I am not aware, have variables stopped being reusable?

Lets go with yes if you want a definitive answer. In reality it's
a question of how many are needed.  If 10-100s sure reuse is fine,
if just a few sensible naming can remove the need for comments
and improve readability.

Jonathan