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

[Lee Jones <lee@kernel.org>]

On Sat, 08 Mar 2025, Jonathan Cameron wrote:

> On Wed, 5 Mar 2025 16:18:38 +0200
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> 
> > ср, 5 бер. 2025 р. о 15:45 Jonathan Cameron <jic23@kernel.org> пише:
> > >
> > > On Fri, 28 Feb 2025 11:30:51 +0200
> > > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > >  
> > > > пт, 28 лют. 2025 р. о 10:59 Lee Jones <lee@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@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

You clearly have more patience for this than I do!  =;-)

-- 
Lee Jones [李琼斯]