On Wed, 5 Mar 2025 16:18:38 +0200
Svyatoslav Ryhel <clamor95@xxxxxxxxx> wrote:
> ср, 5 бер. 2025 р. о 15:45 Jonathan Cameron <jic23@xxxxxxxxxx> пише:
> >
> > 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(-)
> > > > >
> ...
> > > > > /* 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
