Re: [PATCH v2 2/2] media: imx335: Fix reset-gpio handling

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Jul 31, 2024 at 11:06:38AM +0200, Krzysztof Kozlowski wrote:
> On 31/07/2024 11:02, Kieran Bingham wrote:
> > Quoting Umang Jain (2024-07-31 06:41:35)
> >> On 30/07/24 2:40 pm, Laurent Pinchart wrote:
> >>> On Tue, Jul 30, 2024 at 10:42:01AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 30/07/2024 10:24, Sakari Ailus wrote:
> >>>>> Hi Krzysztof,
> >>>>>
> >>>>> On Mon, Jul 29, 2024 at 04:09:39PM +0200, Krzysztof Kozlowski wrote:
> >>>>>> On 29/07/2024 13:04, Umang Jain wrote:
> >>>>>>> Rectify the logical value of reset-gpio so that it is set to
> >>>>>>> 0 (disabled) during power-on and to 1 (enabled) during power-off.
> >>>>>>>
> >>>>>>> Meanwhile at it, set the reset-gpio to GPIO_OUT_HIGH at initialization
> >>>>>>> time to make sure it starts off in reset.
> >>>>>>>
> >>>>>>> Fixes: 45d19b5fb9ae ("media: i2c: Add imx335 camera sensor driver")
> >>>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >>>>>>> ---
> >>>>>>>   drivers/media/i2c/imx335.c | 8 ++++----
> >>>>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>>>>
> >>>>>> This will break all the users, so no. At least not without mentioning
> >>>>>> ABI break and some sort of investigating how customers or users are
> >>>>>> affected.
> >>>>>
> >>>>> I know the original authors aren't using the driver anymore and it took
> >>>>> quite a bit of time until others started to contribute to it so I suspect
> >>>>> the driver hasn't been in use for that long. There are no instances of the
> >>>>> device in the in-kernel DTS either.
> >>>>>
> >>>>> Any DTS author should have also noticed the issue but of course there's a
> >>>>> risk someone could have just changed the polarity and not bothered to chech
> >>>>> what it was supposed to be.
> >>>>>
> >>>>> I agree the commit message should be more vocal about the effects on
> >>>>> existing DTS.
> >>>>
> >>>> I can imagine that all users (out of tree, in this case) inverted
> >>>> polarity in DTS based on what's implemented. You could go with some
> >>>> trivial hack, like I did for one of codecs - see 738455858a2d ("ASoC:
> >>>> codecs: wsa881x: Use proper shutdown GPIO polarity"), but I remember
> >>>> Mark Brown rejected similar commit for newer drivers.
> >>>
> >>> I don't think there's any out-of-tree user, because when we started
> >>> using the recently driver, it required lots of fixes to even work at
> >>> all. I'll let Kieran and Umang comment on that, I haven't follow the
> >>> development in details.
> >>
> >> indeed, initially we had to put up fixes like :
> >>
> >> 14a60786d72e ("media: imx335: Set reserved register to default value")
> >> 81495a59baeba ("media: imx335: Fix active area height discrepency")
> >>
> >> to make the sensor work properly on our platforms. Only after that we 
> >> had a base to support more capabilities on the sensor (multiple lanes 
> >> support, flips, TPG etc.)
> > 
> > I would also add that we had to provide control for the regulators to be
> > able to power the device as well in fea91ee73b7c ("media: i2c: imx335:
> > Enable regulator supplies").
> 
> Hm? That's not a proof of anything. Supplies are often turned on by default.
> 
> > Given the driver was posted from Intel, I would have anticipated perhaps
> > the driver was in fact only actually tested by Intel on ACPI platforms -
> > yet with no ACPI table registered in the driver - even that could likely
> > be considered broken.
> 
> Nope, that does not work like that. Their platforms and such sensors are
> often used on DT based boards.

What DT-platforms would that be ?

> Not mentioning even PRP0001.

I don't think PRP0001 is used by Intel for camera sensors.

Sakari, do you have any information about all this ? Do you think
there's a risk that the driver is currently used by anyone with a
mainline kernel ?

> > Based on that I have a high confidence that there are no current users
> > of this driver (except us).
> 
> Nope, wrong conclusions, not that many arguments.

-- 
Regards,

Laurent Pinchart