Hi Laurent, Krzysztof,
On Wed, Jul 31, 2024 at 12:39:05PM +0300, Laurent Pinchart wrote:
> 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@xxxxxxxxxxxxxxxx>
> > >>>>>>> ---
> > >>>>>>> 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.
The original author is no longer using the driver nor it is used for its
original purpose any more. The next users were quite probably Kieran and
Umang late last year.
>
> 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 ?
I think it's extremely unlikely the driver has been or continues to be in
use on ACPI based systems.
--
Kind regards,
Sakari Ailus
