Re: [PATCH 4/4] rtc: s35390a: convert to dev_err_probe()

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

Hello,

On 01/07/2026 07:26:29+0000, Balakrishnan.S@microchip.com wrote:
> Hi Alexandre,
> 
> Thanks for the review/feedback.
> 
> On 30/06/26 10:40 pm, Alexandre Mergnat wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Thu, 28 May 2026 09:16:47 +0530, Balakrishnan Sambath <balakrishnan.s@microchip.com> wrote:
> >> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> >> index a4678d7c6cf6..342fd2b568a3 100644
> >> --- a/drivers/rtc/rtc-s35390a.c
> >> +++ b/drivers/rtc/rtc-s35390a.c
> >> @@ -479,10 +479,8 @@ static int s35390a_probe(struct i2c_client *client)
> >>                return PTR_ERR(rtc);
> >>
> >>        err_read = s35390a_read_status(s35390a, &status1);
> >> -     if (err_read < 0) {
> >> -             dev_err(dev, "error resetting chip\n");
> >> -             return err_read;
> >> -     }
> >> +     if (err_read < 0)
> >> +             return dev_err_probe(dev, err_read, "error resetting chip\n");
> > 
> > The devm_i2c_new_dummy_device() loop above this hunk still uses
> > dev_err()+return PTR_ERR("Address %02x unavailable"). dev_err_probe() takes
> > format args, so it converts cleanly:
> > 
> >      return dev_err_probe(dev, PTR_ERR(s35390a->client[i]),
> >                           "Address %02x unavailable\n", client->addr + i);
> > 
> > Worth converting for consistency with the rest of the probe.
> Sure, I'll fix this too in next revision.
> > 
> >> @@ -493,16 +491,12 @@ static int s35390a_probe(struct i2c_client *client)
> >>                /* disable alarm (and maybe test mode) */
> >>                buf = 0;
> >>                err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
> >> -             if (err < 0) {
> >> -                     dev_err(dev, "error disabling alarm");
> >> -                     return err;
> >> -             }
> >> +             if (err < 0)
> >> +                     return dev_err_probe(dev, err, "error disabling alarm");
> > 
> > This message is missing its trailing newline (pre-existing). dev_err_probe()
> > formats as "error %pe: %pV" and does not append "\n" itself, so the line
> > runs into the next log message. Since you are touching this line, adding
> > "\n" is a cheap fix even if the issue was here before your patch.
> > I recommand to fix it ;)
> Okay noted. Will fix it too.

Honestly, my plan was to not apply those patches because once I do
that, I'll get hundreds of those. There is no benefit to the change and
I'll cite the dev_err_probe doc:

 * This helper implements common pattern present in probe functions for error
 * checking: print debug or error message depending if the error value is
 * -EPROBE_DEFER and propagate error upwards.
 * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
 * checked later by reading devices_deferred debugfs attribute.
 * It replaces the following code sequence::
 *
 * 	if (err != -EPROBE_DEFER)
 * 		dev_err(dev, ...);
 * 	else
 * 		dev_dbg(dev, ...);
 * 	return err;


We are not checking for EPROBE_DEFER in any of the drivers so there is
no point in doing the change.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com